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

[Maxime Ripard]

On Thu, Feb 12, 2026 at 11:18:43AM +0200, Laurent Pinchart wrote:
> 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@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'm puzzled by this.

Sorry, I was thinking about an earlier case where I missed a condition.
It's still not working though, see below

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

The application I tested this with was using RGB24 /
MEDIA_BUS_FMT_RGB888_1X24 to get the right pixel order.

So, with the changes in those patches, the lookup by mbus code will now
return the BGR24 fmtinfo, but fmt->pixelformat is RGB24.

I've changed it to test fmt->pixelformat instead of fmtinfo->fourcc, and
it works fine now.

Maxime

Attachment: signature.asc
Description: PGP signature