diff mbox series

[8/8] media: imx: imx-mipi-csis: Add RGB/BGR888

Message ID 20220214184318.409208-9-jacopo@jmondi.org
State New
Headers show
Series media: imx: Destage imx7-mipi-csis with fixes on top | expand

Commit Message

Jacopo Mondi Feb. 14, 2022, 6:43 p.m. UTC
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(+)

Comments

Laurent Pinchart Feb. 15, 2022, 7:46 a.m. UTC | #1
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,
Sakari Ailus Feb. 15, 2022, 9:07 p.m. UTC | #2
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,
>
Sakari Ailus Feb. 15, 2022, 11:10 p.m. UTC | #3
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 mbox series

Patch

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,