Re: [PATCH v5 2/2] media: bcm2835-unicam: Fix RGB format / mbus code association

[Maxime Ripard]

Hi Laurent,

On Tue, Feb 10, 2026 at 12:44:36AM +0200, Laurent Pinchart wrote:
> On Mon, Feb 09, 2026 at 04:03:17PM +0100, Maxime Ripard wrote:
> > From: Maxime Ripard <mripard@xxxxxxxxxx>
> > 
> > The Unicam driver is a MIPI-CSI2 Receiver, that can capture RGB 4:4:4,
> > YCbCr 4:2:2, and raw formats.
> > 
> > RGB 4:4:4 is converted to the MIPI-CSI2 RGB888 video format, and
> > associated to the MEDIA_BUS_FMT_RGB888_1X24 media bus code.
> > 
> > However, V4L2_PIX_FMT_RGB24 is defined as having its color components in
> > the R, G and B order, from left to right. MIPI-CSI2 however defines the
> > RGB888 format with blue first, and that's what MEDIA_BUS_FMT_RGB888_1X24
> > defines too.
> > 
> > This essentially means that the R and B will be swapped compared to what
> > V4L2_PIX_FMT_RGB24 defines. The same situation occurs with
> > V4L2_PIX_FMT_BGR24 being associated to MEDIA_BUS_FMT_BGR888_1X24.
> > 
> > In order to fix the swapped components, we need to change the
> > association of V4L2_PIX_FMT_BGR24 to MEDIA_BUS_FMT_RGB888_1X24, and of
> > V4L2_PIX_FMT_RGB24 to MEDIA_BUS_FMT_BGR888_1X24.
> > 
> > Since the media bus code is exposed to userspace, and validated by
> > unicam's link_validate implementation, we need to explicitly accept (and
> > warn) the old association still to preserve backward compatibility.
> > 
> > Signed-off-by: Maxime Ripard <mripard@xxxxxxxxxx>
> > ---
> >  drivers/media/platform/broadcom/bcm2835-unicam.c | 36 +++++++++++++++++++++---
> >  1 file changed, 32 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > index f10064107d543caf867249d0566a0f42d6d8c4c6..5e4850831c931d346146aa8e22c53f0655e462c9 100644
> > --- a/drivers/media/platform/broadcom/bcm2835-unicam.c
> > +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > @@ -340,16 +340,16 @@ static const struct unicam_format_info unicam_image_formats[] = {
> >  		.code		= MEDIA_BUS_FMT_RGB565_1X16,
> >  		.depth		= 16,
> >  		.csi_dt		= MIPI_CSI2_DT_RGB565,
> >  	}, {
> >  		.fourcc		= V4L2_PIX_FMT_RGB24, /* rgb */
> > -		.code		= MEDIA_BUS_FMT_RGB888_1X24,
> > +		.code		= MEDIA_BUS_FMT_BGR888_1X24,
> >  		.depth		= 24,
> >  		.csi_dt		= MIPI_CSI2_DT_RGB888,
> >  	}, {
> >  		.fourcc		= V4L2_PIX_FMT_BGR24, /* bgr */
> > -		.code		= MEDIA_BUS_FMT_BGR888_1X24,
> > +		.code		= MEDIA_BUS_FMT_RGB888_1X24,
> >  		.depth		= 24,
> >  		.csi_dt		= MIPI_CSI2_DT_RGB888,
> >  	}, {
> >  	/* Bayer Formats */
> >  		.fourcc		= V4L2_PIX_FMT_SBGGR8,
> > @@ -2153,12 +2153,40 @@ static int unicam_video_link_validate(struct media_link *link)
> >  		if (WARN_ON(!fmtinfo)) {
> >  			ret = -EPIPE;
> >  			goto out;
> >  		}
> >  
> > -		if (fmtinfo->code != format->code ||
> > -		    fmt->height != format->height ||
> > +		/*
> > +		 * Unicam initially associated BGR24 to BGR888_1X24 and
> > +		 * RGB24 to RGB888_1X24.
> > +		 *
> > +		 * In order to allow the applications using the old
> > +		 * behaviour to run, let's accept the old combination,
> > +		 * but warn about it.
> > +		 */
> > +		if (fmtinfo->code != format->code) {
> > +			if (fmtinfo->fourcc == V4L2_PIX_FMT_BGR24 &&
> > +			    format->code == MEDIA_BUS_FMT_BGR888_1X24) {
> > +				dev_warn_once(node->dev->dev,
> > +					      "MIPI-CSI media bus code for RGB88 is RGB888_1X24. The application must be fixed.");
> 
> s/RGB88/RGB888/
> 
> But I find this confusing, the message doesn't make it clear if RGB888 +
> RGB888_1X24 is expected or is an error. I think the following would be
> clearer.
> 
> 				dev_warn_once(node->dev->dev,
> 					      "Incorrect pixel format BGR24 for BGR888_1X24. Fix your application to use RGB24.");
> 
> > +			} else if (fmtinfo->fourcc == V4L2_PIX_FMT_RGB24 &&
> > +				   format->code == MEDIA_BUS_FMT_RGB888_1X24) {
> > +				dev_warn_once(node->dev->dev,
> > +					      "MIPI-CSI media bus code for BGR888 is BGR888_1X24. The application must be fixed.");
> 
> 				dev_warn_once(node->dev->dev,
> 					      "Incorrect pixel format RGB24 for RGB888_1X24. Fix your application to use BGR24.");
> 
> Or you could combine those two (untested):
> 
> diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
> index f10064107d54..e9f191536765 100644
> --- a/drivers/media/platform/broadcom/bcm2835-unicam.c
> +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
> @@ -2148,22 +2148,38 @@ static int unicam_video_link_validate(struct media_link *link)
>  		const struct v4l2_pix_format *fmt = &node->fmt.fmt.pix;
>  		const struct unicam_format_info *fmtinfo;
> 
> -		fmtinfo = unicam_find_format_by_fourcc(fmt->pixelformat,
> -						       UNICAM_SD_PAD_SOURCE_IMAGE);
> +		fmtinfo = unicam_find_format_by_code(format->code,
> +						     UNICAM_SD_PAD_SOURCE_IMAGE);
>  		if (WARN_ON(!fmtinfo)) {
>  			ret = -EPIPE;
>  			goto out;
>  		}
> 
> -		if (fmtinfo->code != format->code ||
> -		    fmt->height != format->height ||
> +		if (fmtinfo->fourcc != fmt->pixelformat) {

I gave it a try, and fmtinfo->fourcc would always be equal to
fmt->pixelformat, so we never enter that branch and don't warn.

I kept the old lookup and condition...

> +			if ((fmtinfo->fourcc == V4L2_PIX_FMT_BGR24 &&
> +			     format->code == MEDIA_BUS_FMT_BGR888_1X24) ||
> +			    (fmtinfo->fourcc == V4L2_PIX_FMT_RGB24 &&
> +			     format->code == MEDIA_BUS_FMT_RGB888_1X24)) {
> +                                dev_warn_once(node->dev->dev,
> +                                              "Incorrect pixel format %p4CC for 0x%04x. Fix your application to use %p4CC.\n",
> +                                              &fmt->pixelformat, format->code, &fmtinfo->fourcc);

... Added a call to unicam_find_format_by_code() here to print what we should do ...

> +			} else {
> +				dev_dbg(node->dev->dev,
> +					"image: format mismatch: 0x%04x <=> %p4CCx\n",
> +					format->code, &fmt->pixelformat);
> +				ret = -EPIPE
> +				goto out;
> +			}
> +		}
> +
> +		if (fmt->height != format->height ||
>  		    fmt->width != format->width ||
>  		    fmt->field != format->field) {
>  			dev_dbg(node->dev->dev,
> -				"image: (%u x %u) 0x%08x %s != (%u x %u) 0x%08x %s\n",
> -				fmt->width, fmt->height, fmtinfo->code,
> +				"image: (%u x %u) %s != (%u x %u) %s\n",
> +				fmt->width, fmt->height,
>  				v4l2_field_names[fmt->field],
> -				format->width, format->height, format->code,
> +				format->width, format->height,
>  				v4l2_field_names[format->field]);
>  			ret = -EPIPE;
>  		}
> 
> > +			} else {
> > +				dev_dbg(node->dev->dev,
> > +					"image: (%u x %u) 0x%08x %s != (%u x %u) 0x%08x %s\n",
> > +					fmt->width, fmt->height, fmtinfo->code,
> > +					v4l2_field_names[fmt->field],
> > +					format->width, format->height, format->code,
> > +					v4l2_field_names[format->field]);
> 
> As this message is printed specifically due to a format mismatch, I
> would drop the size and field information:
> 
> 				dev_dbg(node->dev->dev,
> 					"image: format mismatch: 0x%04x <=> %p4CC\n",
> 					format->code, &fmt->pixelformat);
> 
> > +				ret = -EPIPE;
> > +				goto out;
> > +			}
> > +		}
> > +
> > +		if (fmt->height != format->height ||
> >  		    fmt->width != format->width ||
> >  		    fmt->field != format->field) {
> >  			dev_dbg(node->dev->dev,
> >  				"image: (%u x %u) 0x%08x %s != (%u x %u) 0x%08x %s\n",
> >  				fmt->width, fmt->height, fmtinfo->code,
> 
> And here you can drop the format.
> 
> I like the approach in this v5. As far as I can see, it won't break
> userspace, will warn of incorrect format usage, and implements the
> backward compatibility in the driver that initially got it wrong,
> unicam. I'm happy with it.

And the rest works fine. I'll send a new version by the end of the week
unless some other review comes up. Thanks!

Maxime

Attachment: signature.asc
Description: PGP signature