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

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

On Wed, Feb 11, 2026 at 11:35:32AM +0100, Maxime Ripard wrote:
> 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@redhat.com>
> > > 
> > > 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@redhat.com>
> > > ---
> > >  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'm puzzled by this. Let's consider the incorrect case where the media
bus format on the subdev pad (format->code) is BGR888_1X24 and the pixel
format (fmt->pixelformat) BGR24.

The unicam_find_format_by_code() function iterates over
unicam_image_formats and finds the entry that has been patched to

 	}, {
 		.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,
 	}, {

fmtinfo->fourcc is V4L2_PIX_FMT_RGB24, which is not equal to
fmt->pixelformat (BGR24). What am I missing ?

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

-- 
Regards,

Laurent Pinchart