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

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

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,

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel