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

[Sakari Ailus <sakari.ailus@iki.fi>]

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

-- 
Sakari Ailus