Re: [PATCH v4 3/3] media: nxp: imx8-isi: Add additional 32-bit RGB format support

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] V4L2_PIX_FMT_RGBX32 and V4L2_PIX_FMT_XBGR32 share the same isi_in_format, which will result in swapped color channels during M2M reads.
--

--- Patch [3]: [PATCH v4 3/3] media: nxp: imx8-isi: Add additional 32-bit RGB format support ---
commit 60dac66057b0040d017034c0952f79ec30cec366
Author: Guoniu Zhou <[email protected]>

media: nxp: imx8-isi: Add additional 32-bit RGB format support

This commit adds support for additional 32-bit RGB pixel formats (BGRA32,
RGBA32, BGRX32, RGBX32, ARGB2101010) to the i.MX8 ISI driver. Formats with
an alpha channel only support capture, as the ISI ignores alpha bits when
reading from memory.

> diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-video.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-video.c
> --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-video.c
> +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-video.c
> @@ -166,6 +166,57 @@ static const struct mxc_isi_format_info mxc_isi_formats[] = {

[ ... ]

> +	}, {
> +		.mbus_code	= MEDIA_BUS_FMT_RGB888_1X24,
> +		.fourcc		= V4L2_PIX_FMT_RGBX32,
> +		.type		= MXC_ISI_VIDEO_CAP | MXC_ISI_VIDEO_M2M_OUT
> +				| MXC_ISI_VIDEO_M2M_CAP,
> +		.isi_in_format	= CHNL_MEM_RD_CTRL_IMG_TYPE_XBGR8,

[Severity: High]
Does this mapping for V4L2_PIX_FMT_RGBX32 swap the red and blue channels
during M2M reads?

The newly added V4L2_PIX_FMT_RGBX32 format (memory layout R,G,B,X) maps to
CHNL_MEM_RD_CTRL_IMG_TYPE_XBGR8 for DMA reads.

However, earlier in mxc_isi_formats[], the pre-existing V4L2_PIX_FMT_XBGR32
format (memory layout B,G,R,X) uses this exact same configuration:

	{
		.mbus_code	= MEDIA_BUS_FMT_RGB888_1X24,
		.fourcc		= V4L2_PIX_FMT_XBGR32,
		.type		= MXC_ISI_VIDEO_CAP | MXC_ISI_VIDEO_M2M_OUT
				| MXC_ISI_VIDEO_M2M_CAP,
		.isi_in_format	= CHNL_MEM_RD_CTRL_IMG_TYPE_XBGR8,
        ...
	}

Since these two formats have opposite red and blue channel locations in
memory, can the hardware DMA controller correctly interpret both layouts
using the identical input format configuration? 

It looks like this could result in corrupted color conversions for one of 
these formats during M2M operations.

> +		.isi_out_format	= CHNL_IMG_CTRL_FORMAT_XBGR888,
> +		.mem_planes	= 1,
> +		.color_planes	= 1,
> +		.depth		= { 32 },
> +		.encoding	= MXC_ISI_ENC_RGB,

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3