| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20260209224436.GE2405149@killaraus.ideasonboard.com>
Date: Tue, 10 Feb 2026 00:44:36 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Maxime Ripard <mripard@...nel.org>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>,
Sakari Ailus <sakari.ailus@...ux.intel.com>,
Hans Verkuil <hverkuil@...nel.org>, linux-media@...r.kernel.org,
linux-kernel@...r.kernel.org, Hans Verkuil <hans.verkuil@...co.com>,
Dave Stevenson <dave.stevenson@...pberrypi.com>,
Maxime Ripard <mripard@...hat.com>
Subject: Re: [PATCH v5 2/2] media: bcm2835-unicam: Fix RGB format / mbus code
association
Hi Maxime,
Thank you for the patch.
On Mon, Feb 09, 2026 at 04:03:17PM +0100, Maxime Ripard wrote:
> From: Maxime Ripard <mripard@...hat.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@...hat.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) {
+ 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);
+ } 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.
--
Regards,
Laurent Pinchart
Powered by blists - more mailing lists