| Message ID | 20220214184318.409208-9-jacopo@jmondi.org |
|---|---|
| State | New |
| Headers | show |
| Series | media: imx: Destage imx7-mipi-csis with fixes on top | expand |
Hi Jacopo, (Adding Sakari to the recipients) Thank you for the patch. On Mon, Feb 14, 2022 at 07:43:18PM +0100, Jacopo Mondi wrote: > Add support for the RGB888_1X24 and BGR888_1X24 image formats. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > drivers/media/platform/imx/imx-mipi-csis.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c > index 9e0a478dba75..0d5870b3010a 100644 > --- a/drivers/media/platform/imx/imx-mipi-csis.c > +++ b/drivers/media/platform/imx/imx-mipi-csis.c > @@ -366,6 +366,16 @@ static const struct csis_pix_format mipi_csis_formats[] = { > .data_type = MIPI_CSI2_DATA_TYPE_RGB565, > .width = 16, > }, > + { }, { to match the rest of the array. Same below. > + .code = MEDIA_BUS_FMT_RGB888_1X24, > + .data_type = MIPI_CSI2_DATA_TYPE_RGB888, > + .width = 24, > + }, > + { > + .code = MEDIA_BUS_FMT_BGR888_1X24, > + .data_type = MIPI_CSI2_DATA_TYPE_RGB888, > + .width = 24, > + }, CSI-2 specifies the order of bits on the bus for RGB888, with data being transmitted in the B, G, R order. The recommended format when stored in memory is V4L2_PIX_FMT_BGR24 (B in byte 0, G in byte 1, R in byte 2). There is no recommended deserialized bus format though. On the output side of the CSIS, we have information. Given figure 13-23 ("Pixel alignment") in the i.MX8MP reference manual, and the definition of RGB_SWAP in MIPI_CSI2_ISP_CONFIG_CH0 that reads Swapping RGB sequence 0 MSB is R and LSB is B. 1 MSB is B and LSB is R (swapped). I think MEDIA_BUS_FMT_RGB888_1X24 is appropriate. On the input side of the CSIS, however, it's a different story, similar to YUV formats. For YUV 4:2:2 we have picked MEDIA_BUS_FMT_UYVY8_1X16 arbitrarily to represent the CSI-2 bus format. It doesn't correspond to the physical reality, but it doesn't matter much. We thus need to do the same for RGB888. If we follow the same convention as for YUV 4:2:2, which transmits data in the U, Y, V, Y order, we should then pick BGR888_1X24. However, the CSIS driver would then need to translate from the input format BGR888_1X24 to the output format RGB888_1X24, which adds a bit of complexity. Picking RGB888_1X24 for the CSI-2 bus is thus tempting, but it's a choice that will affect all drivers, so I wouldn't make this based solely on ease of implementation in a particular driver. I'm thus tempted to go for BGR888_1X24 to be consistent with YUV 4:2:2. Another option would be to add a new RGB888_CSI2 media bus format. In retrospect we should have done the same for YUV 4:2:2. Mistakes happen. Sakari, what do you think ? If we go for BGR888_1X24, the translation between BGR888_1X24 and RGB888_1X24 may not be that hard to implement. You could add an output field to the csis_pix_format structure to store the output media bus code for a given input code, and I think the implementation would then remain relatively simple. Finally, we can also support the swapped RGB format (which is non-standard in CSI-2 but is supported by some sensors, the same way as YUV permutations are often supported too), but it will require setting RGB_SWAP. You can add a rgb_swap field to csis_pix_format for this. I'd split this patch in two, adding MEDIA_BUS_FMT_RGB888_1X24 first, and then adding MEDIA_BUS_FMT_BGR888_1X24 with the new rgb_swap field. The first patch should capture the above explanation. > /* RAW (Bayer and greyscale) formats. */ > { > .code = MEDIA_BUS_FMT_SBGGR8_1X8,
Hi Laurent, Jacopo, Thanks for cc'ing me. On Tue, Feb 15, 2022 at 09:46:26AM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > (Adding Sakari to the recipients) > > Thank you for the patch. > > On Mon, Feb 14, 2022 at 07:43:18PM +0100, Jacopo Mondi wrote: > > Add support for the RGB888_1X24 and BGR888_1X24 image formats. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > drivers/media/platform/imx/imx-mipi-csis.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c > > index 9e0a478dba75..0d5870b3010a 100644 > > --- a/drivers/media/platform/imx/imx-mipi-csis.c > > +++ b/drivers/media/platform/imx/imx-mipi-csis.c > > @@ -366,6 +366,16 @@ static const struct csis_pix_format mipi_csis_formats[] = { > > .data_type = MIPI_CSI2_DATA_TYPE_RGB565, > > .width = 16, > > }, > > + { > > }, { > > to match the rest of the array. Same below. > > > + .code = MEDIA_BUS_FMT_RGB888_1X24, > > + .data_type = MIPI_CSI2_DATA_TYPE_RGB888, > > + .width = 24, > > + }, > > + { > > + .code = MEDIA_BUS_FMT_BGR888_1X24, > > + .data_type = MIPI_CSI2_DATA_TYPE_RGB888, > > + .width = 24, > > + }, > > CSI-2 specifies the order of bits on the bus for RGB888, with data being > transmitted in the B, G, R order. The recommended format when stored in > memory is V4L2_PIX_FMT_BGR24 (B in byte 0, G in byte 1, R in byte 2). > There is no recommended deserialized bus format though. > > On the output side of the CSIS, we have information. Given figure 13-23 > ("Pixel alignment") in the i.MX8MP reference manual, and the definition > of RGB_SWAP in MIPI_CSI2_ISP_CONFIG_CH0 that reads > > Swapping RGB sequence > > 0 MSB is R and LSB is B. > 1 MSB is B and LSB is R (swapped). > > I think MEDIA_BUS_FMT_RGB888_1X24 is appropriate. > > On the input side of the CSIS, however, it's a different story, similar > to YUV formats. For YUV 4:2:2 we have picked MEDIA_BUS_FMT_UYVY8_1X16 > arbitrarily to represent the CSI-2 bus format. It doesn't correspond to > the physical reality, but it doesn't matter much. We thus need to do the > same for RGB888. If we follow the same convention as for YUV 4:2:2, > which transmits data in the U, Y, V, Y order, we should then pick > BGR888_1X24. However, the CSIS driver would then need to translate from > the input format BGR888_1X24 to the output format RGB888_1X24, which > adds a bit of complexity. > > Picking RGB888_1X24 for the CSI-2 bus is thus tempting, but it's a > choice that will affect all drivers, so I wouldn't make this based > solely on ease of implementation in a particular driver. I'm thus > tempted to go for BGR888_1X24 to be consistent with YUV 4:2:2. Another > option would be to add a new RGB888_CSI2 media bus format. In retrospect > we should have done the same for YUV 4:2:2. Mistakes happen. > > Sakari, what do you think ? We first started adding support for raw Bayer formats for CSI-2 so at the time there was little room for confusing the format with another one. Also few of these formats were eventually used on the parallel bus, making some of the formats look a little bit artificial. We've discussed separating the serial bus formats a few times since but always stuck using single-sample parallel bus format for the serial buses. As the existing formats will remain as-is, we'd have a mix of differently named formats returned from an enumeration from increasing number of drivers. That isn't pretty, but then 24 bit deep Bayer formats won't fit the mbus format table anyway. I don't have a strong opinion on this, apart from maintaining the pixel order. Maybe I'd still create a parallel single-sample format for this one. If we'd differentiate the formats on CSI-2 bus, I'd just call them "serial", not "CSI-2". > > If we go for BGR888_1X24, the translation between BGR888_1X24 and > RGB888_1X24 may not be that hard to implement. You could add an output > field to the csis_pix_format structure to store the output media bus > code for a given input code, and I think the implementation would then > remain relatively simple. > > Finally, we can also support the swapped RGB format (which is > non-standard in CSI-2 but is supported by some sensors, the same way as > YUV permutations are often supported too), but it will require setting > RGB_SWAP. You can add a rgb_swap field to csis_pix_format for this. > > I'd split this patch in two, adding MEDIA_BUS_FMT_RGB888_1X24 first, and > then adding MEDIA_BUS_FMT_BGR888_1X24 with the new rgb_swap field. The > first patch should capture the above explanation. > > > /* RAW (Bayer and greyscale) formats. */ > > { > > .code = MEDIA_BUS_FMT_SBGGR8_1X8, >
Hi Laurent, On Tue, Feb 15, 2022 at 11:52:44PM +0200, Laurent Pinchart wrote: > Hi Sakari, > > On Tue, Feb 15, 2022 at 11:07:39PM +0200, Sakari Ailus wrote: > > On Tue, Feb 15, 2022 at 09:46:26AM +0200, Laurent Pinchart wrote: > > > On Mon, Feb 14, 2022 at 07:43:18PM +0100, Jacopo Mondi wrote: > > > > Add support for the RGB888_1X24 and BGR888_1X24 image formats. > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > --- > > > > drivers/media/platform/imx/imx-mipi-csis.c | 10 ++++++++++ > > > > 1 file changed, 10 insertions(+) > > > > > > > > diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c > > > > index 9e0a478dba75..0d5870b3010a 100644 > > > > --- a/drivers/media/platform/imx/imx-mipi-csis.c > > > > +++ b/drivers/media/platform/imx/imx-mipi-csis.c > > > > @@ -366,6 +366,16 @@ static const struct csis_pix_format mipi_csis_formats[] = { > > > > .data_type = MIPI_CSI2_DATA_TYPE_RGB565, > > > > .width = 16, > > > > }, > > > > + { > > > > > > }, { > > > > > > to match the rest of the array. Same below. > > > > > > > + .code = MEDIA_BUS_FMT_RGB888_1X24, > > > > + .data_type = MIPI_CSI2_DATA_TYPE_RGB888, > > > > + .width = 24, > > > > + }, > > > > + { > > > > + .code = MEDIA_BUS_FMT_BGR888_1X24, > > > > + .data_type = MIPI_CSI2_DATA_TYPE_RGB888, > > > > + .width = 24, > > > > + }, > > > > > > CSI-2 specifies the order of bits on the bus for RGB888, with data being > > > transmitted in the B, G, R order. The recommended format when stored in > > > memory is V4L2_PIX_FMT_BGR24 (B in byte 0, G in byte 1, R in byte 2). > > > There is no recommended deserialized bus format though. > > > > > > On the output side of the CSIS, we have information. Given figure 13-23 > > > ("Pixel alignment") in the i.MX8MP reference manual, and the definition > > > of RGB_SWAP in MIPI_CSI2_ISP_CONFIG_CH0 that reads > > > > > > Swapping RGB sequence > > > > > > 0 MSB is R and LSB is B. > > > 1 MSB is B and LSB is R (swapped). > > > > > > I think MEDIA_BUS_FMT_RGB888_1X24 is appropriate. > > > > > > On the input side of the CSIS, however, it's a different story, similar > > > to YUV formats. For YUV 4:2:2 we have picked MEDIA_BUS_FMT_UYVY8_1X16 > > > arbitrarily to represent the CSI-2 bus format. It doesn't correspond to > > > the physical reality, but it doesn't matter much. We thus need to do the > > > same for RGB888. If we follow the same convention as for YUV 4:2:2, > > > which transmits data in the U, Y, V, Y order, we should then pick > > > BGR888_1X24. However, the CSIS driver would then need to translate from > > > the input format BGR888_1X24 to the output format RGB888_1X24, which > > > adds a bit of complexity. > > > > > > Picking RGB888_1X24 for the CSI-2 bus is thus tempting, but it's a > > > choice that will affect all drivers, so I wouldn't make this based > > > solely on ease of implementation in a particular driver. I'm thus > > > tempted to go for BGR888_1X24 to be consistent with YUV 4:2:2. Another > > > option would be to add a new RGB888_CSI2 media bus format. In retrospect > > > we should have done the same for YUV 4:2:2. Mistakes happen. > > > > > > Sakari, what do you think ? > > > > We first started adding support for raw Bayer formats for CSI-2 so at the > > time there was little room for confusing the format with another one. Also > > few of these formats were eventually used on the parallel bus, making some > > of the formats look a little bit artificial. > > > > We've discussed separating the serial bus formats a few times since but > > always stuck using single-sample parallel bus format for the serial buses. > > As the existing formats will remain as-is, we'd have a mix of differently > > named formats returned from an enumeration from increasing number of > > drivers. That isn't pretty, but then 24 bit deep Bayer formats won't fit > > the mbus format table anyway. > > > > I don't have a strong opinion on this, apart from maintaining the pixel > > order. Maybe I'd still create a parallel single-sample format for this one. > > We have MEDIA_BUS_FMT_RGB888_1X24 and MEDIA_BUS_FMT_BGR888_1X24 already. > None of them are intrinsicly better, but I think > MEDIA_BUS_FMT_BGR888_1X24 would match the convention used by > MEDIA_BUS_FMT_UYVY8_1X16, so I would pick that one. Ack ? Agreed. > > If we'd differentiate the formats on CSI-2 bus, I'd just call them > > "serial", not "CSI-2". > > Good point. We would have to figure out how to differentiate the two > ways to transfer YUV 4:2:0 (legacy and non-legacy) over CSI-2 though, > maybe the legacy way could be named using CSI-2, while the non-legacy > way would be named serial and shared with other buses. We don't have to > solve it now. Sounds good to me. > > > If we go for BGR888_1X24, the translation between BGR888_1X24 and > > > RGB888_1X24 may not be that hard to implement. You could add an output > > > field to the csis_pix_format structure to store the output media bus > > > code for a given input code, and I think the implementation would then > > > remain relatively simple. > > > > > > Finally, we can also support the swapped RGB format (which is > > > non-standard in CSI-2 but is supported by some sensors, the same way as > > > YUV permutations are often supported too), but it will require setting > > > RGB_SWAP. You can add a rgb_swap field to csis_pix_format for this. > > > > > > I'd split this patch in two, adding MEDIA_BUS_FMT_RGB888_1X24 first, and > > > then adding MEDIA_BUS_FMT_BGR888_1X24 with the new rgb_swap field. The > > > first patch should capture the above explanation. > > > > > > > /* RAW (Bayer and greyscale) formats. */ > > > > { > > > > .code = MEDIA_BUS_FMT_SBGGR8_1X8, > > -- > Regards, > > Laurent Pinchart
diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c index 9e0a478dba75..0d5870b3010a 100644 --- a/drivers/media/platform/imx/imx-mipi-csis.c +++ b/drivers/media/platform/imx/imx-mipi-csis.c @@ -366,6 +366,16 @@ static const struct csis_pix_format mipi_csis_formats[] = { .data_type = MIPI_CSI2_DATA_TYPE_RGB565, .width = 16, }, + { + .code = MEDIA_BUS_FMT_RGB888_1X24, + .data_type = MIPI_CSI2_DATA_TYPE_RGB888, + .width = 24, + }, + { + .code = MEDIA_BUS_FMT_BGR888_1X24, + .data_type = MIPI_CSI2_DATA_TYPE_RGB888, + .width = 24, + }, /* RAW (Bayer and greyscale) formats. */ { .code = MEDIA_BUS_FMT_SBGGR8_1X8,
Add support for the RGB888_1X24 and BGR888_1X24 image formats. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- drivers/media/platform/imx/imx-mipi-csis.c | 10 ++++++++++ 1 file changed, 10 insertions(+)