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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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 ?

> 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.

> > 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