[PATCH 0/4] media: Fix CSI2 RGB vs BGR pixel order

[PATCH 0/4] media: Fix CSI2 RGB vs BGR pixel order

[Maxime Ripard]

Hi,

Here's an(other [1]) attempt at fixing the current mess due to the
opposite meaning of what v4l2 and the MIPI-CSI2 spec call "RGB". By v4l2
nomenclature, the format CSI calls RGB is actually BGR.

Unfortunately, a handful of CSI transceivers report through RGB media
bus pixel code, which is then understood as V4L2_PIX_FMT_RGB24 by CSI
receivers.

This is made somewhat worse the fact that media bus codes have been made
mostly with parallel busses in mind, and thus the order of pixels wasn't
clearly defined anywhere.

So the v4l2 vs CSI mismatch was confusing (but there's nothing we can do
about it), but the doc didn't really make an attempt at clearing it up
either.

We did have a convention so far though, that about half the affected
drivers were following. 

This series improves the doc, adds the missing media bus codes, and
converts the transceiver drivers to the rightful media bus format.

We'll also need that series [2] from Laurent to fix all the affected
transceivers. 

Let me know what you think,
Maxime

1: https://lore.kernel.org/r/20250606-rpi-unicam-rgb-bgr-fix-v1-1-9930b963f3eb@kernel.org
2: https://lore.kernel.org/r/20250611181528.19542-1-laurent.pinchart@ideasonboard.com

---
Maxime Ripard (4):
      media: uapi: Clarify MBUS color component order for serial buses
      media: uapi: Introduce MEDIA_BUS_FMT_BGR565_1X16
      media: tc358743: Fix the RGB MBUS format
      media: gc2145: Fix the RGB MBUS format

 .../userspace-api/media/v4l/subdev-formats.rst     | 51 +++++++++++++++++++---
 drivers/media/i2c/gc2145.c                         |  4 +-
 drivers/media/i2c/tc358743.c                       | 10 ++---
 include/uapi/linux/media-bus-format.h              |  3 +-
 4 files changed, 54 insertions(+), 14 deletions(-)
---
base-commit: 6e417fb287553495e43135125d099daf80b63fe1
change-id: 20250612-csi-bgr-rgb-b837980c00b3

Best regards,
-- 
Maxime Ripard <mripard@kernel.org>

[PATCH 1/4] media: uapi: Clarify MBUS color component order for serial buses

[Maxime Ripard]

The subdev format documentation has a subsection describing how to use
the media bus pixel codes for serial buses. While it describes the
sampling part well, it doesn't really describe the current convention
used for the components order.

Let's improve that.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 Documentation/userspace-api/media/v4l/subdev-formats.rst | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/Documentation/userspace-api/media/v4l/subdev-formats.rst b/Documentation/userspace-api/media/v4l/subdev-formats.rst
index 2a94371448dc07e5c7097421bd82f42dcd7e21aa..8e92f784abd8123f9ea950f954a60af56ee76dbe 100644
--- a/Documentation/userspace-api/media/v4l/subdev-formats.rst
+++ b/Documentation/userspace-api/media/v4l/subdev-formats.rst
@@ -158,16 +158,18 @@ formats in memory (a raw Bayer image won't be magically converted to
 JPEG just by storing it to memory), there is no one-to-one
 correspondence between them.
 
 The media bus pixel codes document parallel formats. Should the pixel data be
 transported over a serial bus, the media bus pixel code that describes a
-parallel format that transfers a sample on a single clock cycle is used. For
-instance, both MEDIA_BUS_FMT_BGR888_1X24 and MEDIA_BUS_FMT_BGR888_3X8 are used
-on parallel busses for transferring an 8 bits per sample BGR data, whereas on
-serial busses the data in this format is only referred to using
-MEDIA_BUS_FMT_BGR888_1X24. This is because there is effectively only a single
-way to transport that format on the serial busses.
+parallel format that transfers a sample on a single clock cycle is used. The
+color component order used is the same used on the serial bus. For instance,
+both MEDIA_BUS_FMT_BGR888_1X24 and MEDIA_BUS_FMT_BGR888_3X8 are used on parallel
+busses for transferring an 8 bits per sample BGR data, whereas on serial busses
+the data in this format is only referred to using MEDIA_BUS_FMT_BGR888_1X24,
+with BGR meaning that the blue component is transmitted first, then green, then
+red. This is because there is effectively only a single way to transport that
+format on the serial busses.
 
 Packed RGB Formats
 ^^^^^^^^^^^^^^^^^^
 
 Those formats transfer pixel data as red, green and blue components. The

-- 
2.49.0

[PATCH 2/4] media: uapi: Introduce MEDIA_BUS_FMT_BGR565_1X16

[Maxime Ripard]

MIPI-CSI2 sends its RGB format on the wire with the blue component
first, then green, then red. MIPI calls that format "RGB", but by v4l2
conventions it would be BGR.

MIPI-CSI2 supports three RGB variants: 444, 555, 565, 666 and 888.

We already have BGR666 and BGR888 media bus formats, we don't have any
CSI transceivers using the 444 and 555 variants, but some transceivers
use the CSI RGB565 format, while using the RGB656 media bus code.

That's a mistake, but since we don't have a BGR656 media bus code we
need to introduce one before fixing it.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 .../userspace-api/media/v4l/subdev-formats.rst     | 37 ++++++++++++++++++++++
 include/uapi/linux/media-bus-format.h              |  3 +-
 2 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/Documentation/userspace-api/media/v4l/subdev-formats.rst b/Documentation/userspace-api/media/v4l/subdev-formats.rst
index 8e92f784abd8123f9ea950f954a60af56ee76dbe..def0d24ef6cdb1a2ec9395af1468f56adf31a8de 100644
--- a/Documentation/userspace-api/media/v4l/subdev-formats.rst
+++ b/Documentation/userspace-api/media/v4l/subdev-formats.rst
@@ -625,10 +625,47 @@ The following tables list existing packed RGB formats.
       - b\ :sub:`4`
       - b\ :sub:`3`
       - b\ :sub:`2`
       - b\ :sub:`1`
       - b\ :sub:`0`
+    * .. _MEDIA-BUS-FMT-BGR565-1X16:
+
+      - MEDIA_BUS_FMT_BGR565_1X16
+      - 0x1028
+      -
+      -
+      -
+      -
+      -
+      -
+      -
+      -
+      -
+      -
+      -
+      -
+      -
+      -
+      -
+      -
+      -
+      - b\ :sub:`4`
+      - b\ :sub:`3`
+      - b\ :sub:`2`
+      - b\ :sub:`1`
+      - b\ :sub:`0`
+      - g\ :sub:`5`
+      - g\ :sub:`4`
+      - g\ :sub:`3`
+      - g\ :sub:`2`
+      - g\ :sub:`1`
+      - g\ :sub:`0`
+      - r\ :sub:`4`
+      - r\ :sub:`3`
+      - r\ :sub:`2`
+      - r\ :sub:`1`
+      - r\ :sub:`0`
     * .. _MEDIA-BUS-FMT-BGR565-2X8-BE:
 
       - MEDIA_BUS_FMT_BGR565_2X8_BE
       - 0x1005
       -
diff --git a/include/uapi/linux/media-bus-format.h b/include/uapi/linux/media-bus-format.h
index ff62056feed5b6588bfcfdff178f5b68eecd3a26..a73d91876d31844bf8c2da91ddea541181840bd2 100644
--- a/include/uapi/linux/media-bus-format.h
+++ b/include/uapi/linux/media-bus-format.h
@@ -32,17 +32,18 @@
  * new pixel codes.
  */
 
 #define MEDIA_BUS_FMT_FIXED			0x0001
 
-/* RGB - next is	0x1028 */
+/* RGB - next is	0x1029 */
 #define MEDIA_BUS_FMT_RGB444_1X12		0x1016
 #define MEDIA_BUS_FMT_RGB444_2X8_PADHI_BE	0x1001
 #define MEDIA_BUS_FMT_RGB444_2X8_PADHI_LE	0x1002
 #define MEDIA_BUS_FMT_RGB555_2X8_PADHI_BE	0x1003
 #define MEDIA_BUS_FMT_RGB555_2X8_PADHI_LE	0x1004
 #define MEDIA_BUS_FMT_RGB565_1X16		0x1017
+#define MEDIA_BUS_FMT_BGR565_1X16		0x1028
 #define MEDIA_BUS_FMT_BGR565_2X8_BE		0x1005
 #define MEDIA_BUS_FMT_BGR565_2X8_LE		0x1006
 #define MEDIA_BUS_FMT_RGB565_2X8_BE		0x1007
 #define MEDIA_BUS_FMT_RGB565_2X8_LE		0x1008
 #define MEDIA_BUS_FMT_RGB666_1X18		0x1009

-- 
2.49.0

[PATCH 3/4] media: tc358743: Fix the RGB MBUS format

[Maxime Ripard]

The tc358743 is an HDMI to MIPI-CSI2 bridge. It supports two of the
three HDMI 1.4 video formats: RGB 4:4:4 and YCbCr 422.

RGB 4:4:4 is converted to the MIPI-CSI2 RGB888 video format, and listed
in the driver as MEDIA_BUS_FMT_RGB888_1X24.

Most CSI2 receiver drivers then map MEDIA_BUS_FMT_RGB888_1X24 to
V4L2_PIX_FMT_RGB24.

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.

This essentially means that the R and B will be swapped compared to what
V4L2_PIX_FMT_RGB24 defines.

The proper MBUS format would be BGR888, so let's use that.

Fixes: d32d98642de6 ("[media] Driver for Toshiba TC358743 HDMI to CSI-2 bridge")
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/media/i2c/tc358743.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
index ca0b0b9bda1755313f066ba36ab218873b9ae438..a1c164a7716a10b0cb9ff38f88c0513b45f24771 100644
--- a/drivers/media/i2c/tc358743.c
+++ b/drivers/media/i2c/tc358743.c
@@ -688,11 +688,11 @@ static void tc358743_set_csi_color_space(struct v4l2_subdev *sd)
 		mutex_lock(&state->confctl_mutex);
 		i2c_wr16_and_or(sd, CONFCTL, ~MASK_YCBCRFMT,
 				MASK_YCBCRFMT_422_8_BIT);
 		mutex_unlock(&state->confctl_mutex);
 		break;
-	case MEDIA_BUS_FMT_RGB888_1X24:
+	case MEDIA_BUS_FMT_BGR888_1X24:
 		v4l2_dbg(2, debug, sd, "%s: RGB 888 24-bit\n", __func__);
 		i2c_wr8_and_or(sd, VOUT_SET2,
 				~(MASK_SEL422 | MASK_VOUT_422FIL_100) & 0xff,
 				0x00);
 		i2c_wr8_and_or(sd, VI_REP, ~MASK_VOUT_COLOR_SEL & 0xff,
@@ -1353,11 +1353,11 @@ static int tc358743_log_status(struct v4l2_subdev *sd)
 			(i2c_rd16(sd, CSI_STATUS) & MASK_S_HLT) ?
 			"yes" : "no");
 	v4l2_info(sd, "Color space: %s\n",
 			state->mbus_fmt_code == MEDIA_BUS_FMT_UYVY8_1X16 ?
 			"YCbCr 422 16-bit" :
-			state->mbus_fmt_code == MEDIA_BUS_FMT_RGB888_1X24 ?
+			state->mbus_fmt_code == MEDIA_BUS_FMT_BGR888_1X24 ?
 			"RGB 888 24-bit" : "Unsupported");
 
 	v4l2_info(sd, "-----%s status-----\n", is_hdmi(sd) ? "HDMI" : "DVI-D");
 	v4l2_info(sd, "HDCP encrypted content: %s\n",
 			hdmi_sys_status & MASK_S_HDCP ? "yes" : "no");
@@ -1691,11 +1691,11 @@ static int tc358743_enum_mbus_code(struct v4l2_subdev *sd,
 		struct v4l2_subdev_state *sd_state,
 		struct v4l2_subdev_mbus_code_enum *code)
 {
 	switch (code->index) {
 	case 0:
-		code->code = MEDIA_BUS_FMT_RGB888_1X24;
+		code->code = MEDIA_BUS_FMT_BGR888_1X24;
 		break;
 	case 1:
 		code->code = MEDIA_BUS_FMT_UYVY8_1X16;
 		break;
 	default:
@@ -1753,11 +1753,11 @@ static int tc358743_set_fmt(struct v4l2_subdev *sd,
 
 	if (ret)
 		return ret;
 
 	switch (code) {
-	case MEDIA_BUS_FMT_RGB888_1X24:
+	case MEDIA_BUS_FMT_BGR888_1X24:
 	case MEDIA_BUS_FMT_UYVY8_1X16:
 		break;
 	default:
 		return -EINVAL;
 	}
@@ -2172,11 +2172,11 @@ static int tc358743_probe(struct i2c_client *client)
 	sd->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
 	err = media_entity_pads_init(&sd->entity, 1, &state->pad);
 	if (err < 0)
 		goto err_hdl;
 
-	state->mbus_fmt_code = MEDIA_BUS_FMT_RGB888_1X24;
+	state->mbus_fmt_code = MEDIA_BUS_FMT_BGR888_1X24;
 
 	sd->dev = &client->dev;
 
 	mutex_init(&state->confctl_mutex);
 

-- 
2.49.0

Re: [PATCH 3/4] media: tc358743: Fix the RGB MBUS format

[Dave Stevenson]

Hi Maxime

On Thu, 12 Jun 2025 at 13:54, Maxime Ripard <mripard@kernel.org> wrote:
>
> The tc358743 is an HDMI to MIPI-CSI2 bridge. It supports two of the
> three HDMI 1.4 video formats: RGB 4:4:4 and YCbCr 422.
>
> RGB 4:4:4 is converted to the MIPI-CSI2 RGB888 video format, and listed
> in the driver as MEDIA_BUS_FMT_RGB888_1X24.
>
> Most CSI2 receiver drivers then map MEDIA_BUS_FMT_RGB888_1X24 to
> V4L2_PIX_FMT_RGB24.
>
> 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.
>
> This essentially means that the R and B will be swapped compared to what
> V4L2_PIX_FMT_RGB24 defines.
>
> The proper MBUS format would be BGR888, so let's use that.

I know where you're coming from, but this driver has been in the tree
since 2015, so there is a reasonable expectation of users. I've had an
overlay for it in our kernel tree since 4.14 (July 2018), and I know
of at least PiKVM [1] as a product based on it. I don't know if Cisco
are still supporting devices with it in.

Whilst the pixel format may now be considered to be incorrect,
changing it will break userspace applications that have been using it
for those 10 years if they're explicitly looking for
MEDIA_BUS_FMT_RGB888_1X24 or the mapping of it through to
V4L2_PIX_FMT_RGB24.
The kernel docs at [2] quote Linus as saying
"If you break existing user space setups THAT IS A REGRESSION.
It's not ok to say "but we'll fix the user space setup"
Really. NOT OK."

I'm thinking of GStreamer if the format has been specified explicitly
- it'll fail to negotiate due to v4l2src saying it can't handle the
caps.

Yes it sucks as a situation, but I'm not sure what the best solution
is. Potentially accepting both MEDIA_BUS_FMT_RGB888_1X24 and
MEDIA_BUS_FMT_BGR888_1X24 as valid MBUS codes for RGB, alongside
MEDIA_BUS_FMT_UYVY8_1X16 for UYVY?

  Dave

[1] https://pikvm.org/
[2] https://www.kernel.org/doc/html/latest/process/handling-regressions.html#quotes-from-linus-about-regression

> Fixes: d32d98642de6 ("[media] Driver for Toshiba TC358743 HDMI to CSI-2 bridge")
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>  drivers/media/i2c/tc358743.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
> index ca0b0b9bda1755313f066ba36ab218873b9ae438..a1c164a7716a10b0cb9ff38f88c0513b45f24771 100644
> --- a/drivers/media/i2c/tc358743.c
> +++ b/drivers/media/i2c/tc358743.c
> @@ -688,11 +688,11 @@ static void tc358743_set_csi_color_space(struct v4l2_subdev *sd)
>                 mutex_lock(&state->confctl_mutex);
>                 i2c_wr16_and_or(sd, CONFCTL, ~MASK_YCBCRFMT,
>                                 MASK_YCBCRFMT_422_8_BIT);
>                 mutex_unlock(&state->confctl_mutex);
>                 break;
> -       case MEDIA_BUS_FMT_RGB888_1X24:
> +       case MEDIA_BUS_FMT_BGR888_1X24:
>                 v4l2_dbg(2, debug, sd, "%s: RGB 888 24-bit\n", __func__);
>                 i2c_wr8_and_or(sd, VOUT_SET2,
>                                 ~(MASK_SEL422 | MASK_VOUT_422FIL_100) & 0xff,
>                                 0x00);
>                 i2c_wr8_and_or(sd, VI_REP, ~MASK_VOUT_COLOR_SEL & 0xff,
> @@ -1353,11 +1353,11 @@ static int tc358743_log_status(struct v4l2_subdev *sd)
>                         (i2c_rd16(sd, CSI_STATUS) & MASK_S_HLT) ?
>                         "yes" : "no");
>         v4l2_info(sd, "Color space: %s\n",
>                         state->mbus_fmt_code == MEDIA_BUS_FMT_UYVY8_1X16 ?
>                         "YCbCr 422 16-bit" :
> -                       state->mbus_fmt_code == MEDIA_BUS_FMT_RGB888_1X24 ?
> +                       state->mbus_fmt_code == MEDIA_BUS_FMT_BGR888_1X24 ?
>                         "RGB 888 24-bit" : "Unsupported");
>
>         v4l2_info(sd, "-----%s status-----\n", is_hdmi(sd) ? "HDMI" : "DVI-D");
>         v4l2_info(sd, "HDCP encrypted content: %s\n",
>                         hdmi_sys_status & MASK_S_HDCP ? "yes" : "no");
> @@ -1691,11 +1691,11 @@ static int tc358743_enum_mbus_code(struct v4l2_subdev *sd,
>                 struct v4l2_subdev_state *sd_state,
>                 struct v4l2_subdev_mbus_code_enum *code)
>  {
>         switch (code->index) {
>         case 0:
> -               code->code = MEDIA_BUS_FMT_RGB888_1X24;
> +               code->code = MEDIA_BUS_FMT_BGR888_1X24;
>                 break;
>         case 1:
>                 code->code = MEDIA_BUS_FMT_UYVY8_1X16;
>                 break;
>         default:
> @@ -1753,11 +1753,11 @@ static int tc358743_set_fmt(struct v4l2_subdev *sd,
>
>         if (ret)
>                 return ret;
>
>         switch (code) {
> -       case MEDIA_BUS_FMT_RGB888_1X24:
> +       case MEDIA_BUS_FMT_BGR888_1X24:
>         case MEDIA_BUS_FMT_UYVY8_1X16:
>                 break;
>         default:
>                 return -EINVAL;
>         }
> @@ -2172,11 +2172,11 @@ static int tc358743_probe(struct i2c_client *client)
>         sd->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
>         err = media_entity_pads_init(&sd->entity, 1, &state->pad);
>         if (err < 0)
>                 goto err_hdl;
>
> -       state->mbus_fmt_code = MEDIA_BUS_FMT_RGB888_1X24;
> +       state->mbus_fmt_code = MEDIA_BUS_FMT_BGR888_1X24;
>
>         sd->dev = &client->dev;
>
>         mutex_init(&state->confctl_mutex);
>
>
> --
> 2.49.0
>

Re: [PATCH 3/4] media: tc358743: Fix the RGB MBUS format

[Hans Verkuil]

On 12/06/2025 19:01, Dave Stevenson wrote:
> Hi Maxime
> 
> On Thu, 12 Jun 2025 at 13:54, Maxime Ripard <mripard@kernel.org> wrote:
>>
>> The tc358743 is an HDMI to MIPI-CSI2 bridge. It supports two of the
>> three HDMI 1.4 video formats: RGB 4:4:4 and YCbCr 422.
>>
>> RGB 4:4:4 is converted to the MIPI-CSI2 RGB888 video format, and listed
>> in the driver as MEDIA_BUS_FMT_RGB888_1X24.
>>
>> Most CSI2 receiver drivers then map MEDIA_BUS_FMT_RGB888_1X24 to
>> V4L2_PIX_FMT_RGB24.
>>
>> 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.
>>
>> This essentially means that the R and B will be swapped compared to what
>> V4L2_PIX_FMT_RGB24 defines.
>>
>> The proper MBUS format would be BGR888, so let's use that.
> 
> I know where you're coming from, but this driver has been in the tree
> since 2015, so there is a reasonable expectation of users. I've had an
> overlay for it in our kernel tree since 4.14 (July 2018), and I know
> of at least PiKVM [1] as a product based on it. I don't know if Cisco
> are still supporting devices with it in.

Those are all EOL, so no need to be concerned about that.

But it is the most commonly used HDMI-to-CSI bridge, so breaking uAPI is
a real concern.

See more in my review comment in the code below.

> 
> Whilst the pixel format may now be considered to be incorrect,
> changing it will break userspace applications that have been using it
> for those 10 years if they're explicitly looking for
> MEDIA_BUS_FMT_RGB888_1X24 or the mapping of it through to
> V4L2_PIX_FMT_RGB24.
> The kernel docs at [2] quote Linus as saying
> "If you break existing user space setups THAT IS A REGRESSION.
> It's not ok to say "but we'll fix the user space setup"
> Really. NOT OK."
> 
> I'm thinking of GStreamer if the format has been specified explicitly
> - it'll fail to negotiate due to v4l2src saying it can't handle the
> caps.
> 
> Yes it sucks as a situation, but I'm not sure what the best solution
> is. Potentially accepting both MEDIA_BUS_FMT_RGB888_1X24 and
> MEDIA_BUS_FMT_BGR888_1X24 as valid MBUS codes for RGB, alongside
> MEDIA_BUS_FMT_UYVY8_1X16 for UYVY?
> 
>   Dave
> 
> [1] https://pikvm.org/
> [2] https://www.kernel.org/doc/html/latest/process/handling-regressions.html#quotes-from-linus-about-regression
> 
>> Fixes: d32d98642de6 ("[media] Driver for Toshiba TC358743 HDMI to CSI-2 bridge")
>> Signed-off-by: Maxime Ripard <mripard@kernel.org>
>> ---
>>  drivers/media/i2c/tc358743.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
>> index ca0b0b9bda1755313f066ba36ab218873b9ae438..a1c164a7716a10b0cb9ff38f88c0513b45f24771 100644
>> --- a/drivers/media/i2c/tc358743.c
>> +++ b/drivers/media/i2c/tc358743.c
>> @@ -688,11 +688,11 @@ static void tc358743_set_csi_color_space(struct v4l2_subdev *sd)
>>                 mutex_lock(&state->confctl_mutex);
>>                 i2c_wr16_and_or(sd, CONFCTL, ~MASK_YCBCRFMT,
>>                                 MASK_YCBCRFMT_422_8_BIT);
>>                 mutex_unlock(&state->confctl_mutex);
>>                 break;
>> -       case MEDIA_BUS_FMT_RGB888_1X24:
>> +       case MEDIA_BUS_FMT_BGR888_1X24:
>>                 v4l2_dbg(2, debug, sd, "%s: RGB 888 24-bit\n", __func__);
>>                 i2c_wr8_and_or(sd, VOUT_SET2,
>>                                 ~(MASK_SEL422 | MASK_VOUT_422FIL_100) & 0xff,
>>                                 0x00);
>>                 i2c_wr8_and_or(sd, VI_REP, ~MASK_VOUT_COLOR_SEL & 0xff,
>> @@ -1353,11 +1353,11 @@ static int tc358743_log_status(struct v4l2_subdev *sd)
>>                         (i2c_rd16(sd, CSI_STATUS) & MASK_S_HLT) ?
>>                         "yes" : "no");
>>         v4l2_info(sd, "Color space: %s\n",
>>                         state->mbus_fmt_code == MEDIA_BUS_FMT_UYVY8_1X16 ?
>>                         "YCbCr 422 16-bit" :
>> -                       state->mbus_fmt_code == MEDIA_BUS_FMT_RGB888_1X24 ?
>> +                       state->mbus_fmt_code == MEDIA_BUS_FMT_BGR888_1X24 ?
>>                         "RGB 888 24-bit" : "Unsupported");
>>
>>         v4l2_info(sd, "-----%s status-----\n", is_hdmi(sd) ? "HDMI" : "DVI-D");
>>         v4l2_info(sd, "HDCP encrypted content: %s\n",
>>                         hdmi_sys_status & MASK_S_HDCP ? "yes" : "no");
>> @@ -1691,11 +1691,11 @@ static int tc358743_enum_mbus_code(struct v4l2_subdev *sd,
>>                 struct v4l2_subdev_state *sd_state,
>>                 struct v4l2_subdev_mbus_code_enum *code)
>>  {
>>         switch (code->index) {
>>         case 0:
>> -               code->code = MEDIA_BUS_FMT_RGB888_1X24;
>> +               code->code = MEDIA_BUS_FMT_BGR888_1X24;

So would this change break or fix the formats[] table in:

drivers/media/platform/raspberrypi/rp1-cfe/cfe-fmts.h

Are there other bridge drivers that misinterpret MEDIA_BUS_FMT_RGB888_1X24
and/or MEDIA_BUS_FMT_RGB888_1X24?

Regards,

	Hans

>>                 break;
>>         case 1:
>>                 code->code = MEDIA_BUS_FMT_UYVY8_1X16;
>>                 break;
>>         default:
>> @@ -1753,11 +1753,11 @@ static int tc358743_set_fmt(struct v4l2_subdev *sd,
>>
>>         if (ret)
>>                 return ret;
>>
>>         switch (code) {
>> -       case MEDIA_BUS_FMT_RGB888_1X24:
>> +       case MEDIA_BUS_FMT_BGR888_1X24:
>>         case MEDIA_BUS_FMT_UYVY8_1X16:
>>                 break;
>>         default:
>>                 return -EINVAL;
>>         }
>> @@ -2172,11 +2172,11 @@ static int tc358743_probe(struct i2c_client *client)
>>         sd->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
>>         err = media_entity_pads_init(&sd->entity, 1, &state->pad);
>>         if (err < 0)
>>                 goto err_hdl;
>>
>> -       state->mbus_fmt_code = MEDIA_BUS_FMT_RGB888_1X24;
>> +       state->mbus_fmt_code = MEDIA_BUS_FMT_BGR888_1X24;
>>
>>         sd->dev = &client->dev;
>>
>>         mutex_init(&state->confctl_mutex);
>>
>>
>> --
>> 2.49.0
>>
> 

Re: [PATCH 3/4] media: tc358743: Fix the RGB MBUS format

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 6522 bytes --]

On Mon, Jun 16, 2025 at 10:02:17AM +0200, Hans Verkuil wrote:
> On 12/06/2025 19:01, Dave Stevenson wrote:
> > On Thu, 12 Jun 2025 at 13:54, Maxime Ripard <mripard@kernel.org> wrote:
> >>
> >> The tc358743 is an HDMI to MIPI-CSI2 bridge. It supports two of the
> >> three HDMI 1.4 video formats: RGB 4:4:4 and YCbCr 422.
> >>
> >> RGB 4:4:4 is converted to the MIPI-CSI2 RGB888 video format, and listed
> >> in the driver as MEDIA_BUS_FMT_RGB888_1X24.
> >>
> >> Most CSI2 receiver drivers then map MEDIA_BUS_FMT_RGB888_1X24 to
> >> V4L2_PIX_FMT_RGB24.
> >>
> >> 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.
> >>
> >> This essentially means that the R and B will be swapped compared to what
> >> V4L2_PIX_FMT_RGB24 defines.
> >>
> >> The proper MBUS format would be BGR888, so let's use that.
> > 
> > I know where you're coming from, but this driver has been in the tree
> > since 2015, so there is a reasonable expectation of users. I've had an
> > overlay for it in our kernel tree since 4.14 (July 2018), and I know
> > of at least PiKVM [1] as a product based on it. I don't know if Cisco
> > are still supporting devices with it in.
> 
> Those are all EOL, so no need to be concerned about that.
> 
> But it is the most commonly used HDMI-to-CSI bridge, so breaking uAPI is
> a real concern.

Is it really broken?

Discussing it with Laurent and Sakari last week, we couldn't find any
example of a userspace where the media format was set in stone and not
propagated across the pipeline.

The uAPI however is *definitely* broken with unicam right now.

> See more in my review comment in the code below.
> 
> > Whilst the pixel format may now be considered to be incorrect,
> > changing it will break userspace applications that have been using it
> > for those 10 years if they're explicitly looking for
> > MEDIA_BUS_FMT_RGB888_1X24 or the mapping of it through to
> > V4L2_PIX_FMT_RGB24.
> > The kernel docs at [2] quote Linus as saying
> > "If you break existing user space setups THAT IS A REGRESSION.
> > It's not ok to say "but we'll fix the user space setup"
> > Really. NOT OK."
> > 
> > I'm thinking of GStreamer if the format has been specified explicitly
> > - it'll fail to negotiate due to v4l2src saying it can't handle the
> > caps.
> > 
> > Yes it sucks as a situation, but I'm not sure what the best solution
> > is. Potentially accepting both MEDIA_BUS_FMT_RGB888_1X24 and
> > MEDIA_BUS_FMT_BGR888_1X24 as valid MBUS codes for RGB, alongside
> > MEDIA_BUS_FMT_UYVY8_1X16 for UYVY?
> > 
> >   Dave
> > 
> > [1] https://pikvm.org/
> > [2] https://www.kernel.org/doc/html/latest/process/handling-regressions.html#quotes-from-linus-about-regression
> > 
> >> Fixes: d32d98642de6 ("[media] Driver for Toshiba TC358743 HDMI to CSI-2 bridge")
> >> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> >> ---
> >>  drivers/media/i2c/tc358743.c | 10 +++++-----
> >>  1 file changed, 5 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
> >> index ca0b0b9bda1755313f066ba36ab218873b9ae438..a1c164a7716a10b0cb9ff38f88c0513b45f24771 100644
> >> --- a/drivers/media/i2c/tc358743.c
> >> +++ b/drivers/media/i2c/tc358743.c
> >> @@ -688,11 +688,11 @@ static void tc358743_set_csi_color_space(struct v4l2_subdev *sd)
> >>                 mutex_lock(&state->confctl_mutex);
> >>                 i2c_wr16_and_or(sd, CONFCTL, ~MASK_YCBCRFMT,
> >>                                 MASK_YCBCRFMT_422_8_BIT);
> >>                 mutex_unlock(&state->confctl_mutex);
> >>                 break;
> >> -       case MEDIA_BUS_FMT_RGB888_1X24:
> >> +       case MEDIA_BUS_FMT_BGR888_1X24:
> >>                 v4l2_dbg(2, debug, sd, "%s: RGB 888 24-bit\n", __func__);
> >>                 i2c_wr8_and_or(sd, VOUT_SET2,
> >>                                 ~(MASK_SEL422 | MASK_VOUT_422FIL_100) & 0xff,
> >>                                 0x00);
> >>                 i2c_wr8_and_or(sd, VI_REP, ~MASK_VOUT_COLOR_SEL & 0xff,
> >> @@ -1353,11 +1353,11 @@ static int tc358743_log_status(struct v4l2_subdev *sd)
> >>                         (i2c_rd16(sd, CSI_STATUS) & MASK_S_HLT) ?
> >>                         "yes" : "no");
> >>         v4l2_info(sd, "Color space: %s\n",
> >>                         state->mbus_fmt_code == MEDIA_BUS_FMT_UYVY8_1X16 ?
> >>                         "YCbCr 422 16-bit" :
> >> -                       state->mbus_fmt_code == MEDIA_BUS_FMT_RGB888_1X24 ?
> >> +                       state->mbus_fmt_code == MEDIA_BUS_FMT_BGR888_1X24 ?
> >>                         "RGB 888 24-bit" : "Unsupported");
> >>
> >>         v4l2_info(sd, "-----%s status-----\n", is_hdmi(sd) ? "HDMI" : "DVI-D");
> >>         v4l2_info(sd, "HDCP encrypted content: %s\n",
> >>                         hdmi_sys_status & MASK_S_HDCP ? "yes" : "no");
> >> @@ -1691,11 +1691,11 @@ static int tc358743_enum_mbus_code(struct v4l2_subdev *sd,
> >>                 struct v4l2_subdev_state *sd_state,
> >>                 struct v4l2_subdev_mbus_code_enum *code)
> >>  {
> >>         switch (code->index) {
> >>         case 0:
> >> -               code->code = MEDIA_BUS_FMT_RGB888_1X24;
> >> +               code->code = MEDIA_BUS_FMT_BGR888_1X24;
> 
> So would this change break or fix the formats[] table in:
> 
> drivers/media/platform/raspberrypi/rp1-cfe/cfe-fmts.h

It's pretty much the same table than unicam, and I don't believe it
does. For both those drivers the pixels are stored in memory in the CSI
wire order, so the proper format to use is BGR24 for CSI, not RGB24.

> Are there other bridge drivers that misinterpret MEDIA_BUS_FMT_RGB888_1X24
> and/or MEDIA_BUS_FMT_RGB888_1X24?

Yes, it's kind of a mess.

adv748x, ds90ub960 and tc358743 report RGB888, and ov5640 reports
BGR888.

Then we have alvium CSI2 that supports both, and can swap color
components, so that one isn't a concern.

And finally, we have st-mipid02 which is also affected, but is a
receiver so it's easier to solve.

For RGB565, ov5640, mt9m114 and gc2145 are in that list, but the pixel
representation isn't the same than RGB888, so it's not clear to me how
they are affected.

For RGB666, no v4l2 drivers are affected, but a fair bit of KMS drivers
that use media bus formats still might.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 3/4] media: tc358743: Fix the RGB MBUS format

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 6936 bytes --]

Hi Hans,

On Wed, Jun 18, 2025 at 04:54:07PM +0200, Maxime Ripard wrote:
> On Mon, Jun 16, 2025 at 10:02:17AM +0200, Hans Verkuil wrote:
> > On 12/06/2025 19:01, Dave Stevenson wrote:
> > > On Thu, 12 Jun 2025 at 13:54, Maxime Ripard <mripard@kernel.org> wrote:
> > >>
> > >> The tc358743 is an HDMI to MIPI-CSI2 bridge. It supports two of the
> > >> three HDMI 1.4 video formats: RGB 4:4:4 and YCbCr 422.
> > >>
> > >> RGB 4:4:4 is converted to the MIPI-CSI2 RGB888 video format, and listed
> > >> in the driver as MEDIA_BUS_FMT_RGB888_1X24.
> > >>
> > >> Most CSI2 receiver drivers then map MEDIA_BUS_FMT_RGB888_1X24 to
> > >> V4L2_PIX_FMT_RGB24.
> > >>
> > >> 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.
> > >>
> > >> This essentially means that the R and B will be swapped compared to what
> > >> V4L2_PIX_FMT_RGB24 defines.
> > >>
> > >> The proper MBUS format would be BGR888, so let's use that.
> > > 
> > > I know where you're coming from, but this driver has been in the tree
> > > since 2015, so there is a reasonable expectation of users. I've had an
> > > overlay for it in our kernel tree since 4.14 (July 2018), and I know
> > > of at least PiKVM [1] as a product based on it. I don't know if Cisco
> > > are still supporting devices with it in.
> > 
> > Those are all EOL, so no need to be concerned about that.
> > 
> > But it is the most commonly used HDMI-to-CSI bridge, so breaking uAPI is
> > a real concern.
> 
> Is it really broken?
> 
> Discussing it with Laurent and Sakari last week, we couldn't find any
> example of a userspace where the media format was set in stone and not
> propagated across the pipeline.
> 
> The uAPI however is *definitely* broken with unicam right now.
> 
> > See more in my review comment in the code below.
> > 
> > > Whilst the pixel format may now be considered to be incorrect,
> > > changing it will break userspace applications that have been using it
> > > for those 10 years if they're explicitly looking for
> > > MEDIA_BUS_FMT_RGB888_1X24 or the mapping of it through to
> > > V4L2_PIX_FMT_RGB24.
> > > The kernel docs at [2] quote Linus as saying
> > > "If you break existing user space setups THAT IS A REGRESSION.
> > > It's not ok to say "but we'll fix the user space setup"
> > > Really. NOT OK."
> > > 
> > > I'm thinking of GStreamer if the format has been specified explicitly
> > > - it'll fail to negotiate due to v4l2src saying it can't handle the
> > > caps.
> > > 
> > > Yes it sucks as a situation, but I'm not sure what the best solution
> > > is. Potentially accepting both MEDIA_BUS_FMT_RGB888_1X24 and
> > > MEDIA_BUS_FMT_BGR888_1X24 as valid MBUS codes for RGB, alongside
> > > MEDIA_BUS_FMT_UYVY8_1X16 for UYVY?
> > > 
> > >   Dave
> > > 
> > > [1] https://pikvm.org/
> > > [2] https://www.kernel.org/doc/html/latest/process/handling-regressions.html#quotes-from-linus-about-regression
> > > 
> > >> Fixes: d32d98642de6 ("[media] Driver for Toshiba TC358743 HDMI to CSI-2 bridge")
> > >> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> > >> ---
> > >>  drivers/media/i2c/tc358743.c | 10 +++++-----
> > >>  1 file changed, 5 insertions(+), 5 deletions(-)
> > >>
> > >> diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
> > >> index ca0b0b9bda1755313f066ba36ab218873b9ae438..a1c164a7716a10b0cb9ff38f88c0513b45f24771 100644
> > >> --- a/drivers/media/i2c/tc358743.c
> > >> +++ b/drivers/media/i2c/tc358743.c
> > >> @@ -688,11 +688,11 @@ static void tc358743_set_csi_color_space(struct v4l2_subdev *sd)
> > >>                 mutex_lock(&state->confctl_mutex);
> > >>                 i2c_wr16_and_or(sd, CONFCTL, ~MASK_YCBCRFMT,
> > >>                                 MASK_YCBCRFMT_422_8_BIT);
> > >>                 mutex_unlock(&state->confctl_mutex);
> > >>                 break;
> > >> -       case MEDIA_BUS_FMT_RGB888_1X24:
> > >> +       case MEDIA_BUS_FMT_BGR888_1X24:
> > >>                 v4l2_dbg(2, debug, sd, "%s: RGB 888 24-bit\n", __func__);
> > >>                 i2c_wr8_and_or(sd, VOUT_SET2,
> > >>                                 ~(MASK_SEL422 | MASK_VOUT_422FIL_100) & 0xff,
> > >>                                 0x00);
> > >>                 i2c_wr8_and_or(sd, VI_REP, ~MASK_VOUT_COLOR_SEL & 0xff,
> > >> @@ -1353,11 +1353,11 @@ static int tc358743_log_status(struct v4l2_subdev *sd)
> > >>                         (i2c_rd16(sd, CSI_STATUS) & MASK_S_HLT) ?
> > >>                         "yes" : "no");
> > >>         v4l2_info(sd, "Color space: %s\n",
> > >>                         state->mbus_fmt_code == MEDIA_BUS_FMT_UYVY8_1X16 ?
> > >>                         "YCbCr 422 16-bit" :
> > >> -                       state->mbus_fmt_code == MEDIA_BUS_FMT_RGB888_1X24 ?
> > >> +                       state->mbus_fmt_code == MEDIA_BUS_FMT_BGR888_1X24 ?
> > >>                         "RGB 888 24-bit" : "Unsupported");
> > >>
> > >>         v4l2_info(sd, "-----%s status-----\n", is_hdmi(sd) ? "HDMI" : "DVI-D");
> > >>         v4l2_info(sd, "HDCP encrypted content: %s\n",
> > >>                         hdmi_sys_status & MASK_S_HDCP ? "yes" : "no");
> > >> @@ -1691,11 +1691,11 @@ static int tc358743_enum_mbus_code(struct v4l2_subdev *sd,
> > >>                 struct v4l2_subdev_state *sd_state,
> > >>                 struct v4l2_subdev_mbus_code_enum *code)
> > >>  {
> > >>         switch (code->index) {
> > >>         case 0:
> > >> -               code->code = MEDIA_BUS_FMT_RGB888_1X24;
> > >> +               code->code = MEDIA_BUS_FMT_BGR888_1X24;
> > 
> > So would this change break or fix the formats[] table in:
> > 
> > drivers/media/platform/raspberrypi/rp1-cfe/cfe-fmts.h
> 
> It's pretty much the same table than unicam, and I don't believe it
> does. For both those drivers the pixels are stored in memory in the CSI
> wire order, so the proper format to use is BGR24 for CSI, not RGB24.
> 
> > Are there other bridge drivers that misinterpret MEDIA_BUS_FMT_RGB888_1X24
> > and/or MEDIA_BUS_FMT_RGB888_1X24?
> 
> Yes, it's kind of a mess.
> 
> adv748x, ds90ub960 and tc358743 report RGB888, and ov5640 reports
> BGR888.
> 
> Then we have alvium CSI2 that supports both, and can swap color
> components, so that one isn't a concern.
> 
> And finally, we have st-mipid02 which is also affected, but is a
> receiver so it's easier to solve.
> 
> For RGB565, ov5640, mt9m114 and gc2145 are in that list, but the pixel
> representation isn't the same than RGB888, so it's not clear to me how
> they are affected.
> 
> For RGB666, no v4l2 drivers are affected, but a fair bit of KMS drivers
> that use media bus formats still might.

Can we make some progress on this, one way or another?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 3/4] media: tc358743: Fix the RGB MBUS format

[Hans Verkuil]

On 31/07/2025 12:14, Maxime Ripard wrote:
> Hi Hans,
> 
> On Wed, Jun 18, 2025 at 04:54:07PM +0200, Maxime Ripard wrote:
>> On Mon, Jun 16, 2025 at 10:02:17AM +0200, Hans Verkuil wrote:
>>> On 12/06/2025 19:01, Dave Stevenson wrote:
>>>> On Thu, 12 Jun 2025 at 13:54, Maxime Ripard <mripard@kernel.org> wrote:
>>>>>
>>>>> The tc358743 is an HDMI to MIPI-CSI2 bridge. It supports two of the
>>>>> three HDMI 1.4 video formats: RGB 4:4:4 and YCbCr 422.
>>>>>
>>>>> RGB 4:4:4 is converted to the MIPI-CSI2 RGB888 video format, and listed
>>>>> in the driver as MEDIA_BUS_FMT_RGB888_1X24.
>>>>>
>>>>> Most CSI2 receiver drivers then map MEDIA_BUS_FMT_RGB888_1X24 to
>>>>> V4L2_PIX_FMT_RGB24.
>>>>>
>>>>> 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.
>>>>>
>>>>> This essentially means that the R and B will be swapped compared to what
>>>>> V4L2_PIX_FMT_RGB24 defines.
>>>>>
>>>>> The proper MBUS format would be BGR888, so let's use that.
>>>>
>>>> I know where you're coming from, but this driver has been in the tree
>>>> since 2015, so there is a reasonable expectation of users. I've had an
>>>> overlay for it in our kernel tree since 4.14 (July 2018), and I know
>>>> of at least PiKVM [1] as a product based on it. I don't know if Cisco
>>>> are still supporting devices with it in.
>>>
>>> Those are all EOL, so no need to be concerned about that.
>>>
>>> But it is the most commonly used HDMI-to-CSI bridge, so breaking uAPI is
>>> a real concern.
>>
>> Is it really broken?
>>
>> Discussing it with Laurent and Sakari last week, we couldn't find any
>> example of a userspace where the media format was set in stone and not
>> propagated across the pipeline.
>>
>> The uAPI however is *definitely* broken with unicam right now.
>>
>>> See more in my review comment in the code below.
>>>
>>>> Whilst the pixel format may now be considered to be incorrect,
>>>> changing it will break userspace applications that have been using it
>>>> for those 10 years if they're explicitly looking for
>>>> MEDIA_BUS_FMT_RGB888_1X24 or the mapping of it through to
>>>> V4L2_PIX_FMT_RGB24.
>>>> The kernel docs at [2] quote Linus as saying
>>>> "If you break existing user space setups THAT IS A REGRESSION.
>>>> It's not ok to say "but we'll fix the user space setup"
>>>> Really. NOT OK."
>>>>
>>>> I'm thinking of GStreamer if the format has been specified explicitly
>>>> - it'll fail to negotiate due to v4l2src saying it can't handle the
>>>> caps.
>>>>
>>>> Yes it sucks as a situation, but I'm not sure what the best solution
>>>> is. Potentially accepting both MEDIA_BUS_FMT_RGB888_1X24 and
>>>> MEDIA_BUS_FMT_BGR888_1X24 as valid MBUS codes for RGB, alongside
>>>> MEDIA_BUS_FMT_UYVY8_1X16 for UYVY?
>>>>
>>>>   Dave
>>>>
>>>> [1] https://pikvm.org/
>>>> [2] https://www.kernel.org/doc/html/latest/process/handling-regressions.html#quotes-from-linus-about-regression
>>>>
>>>>> Fixes: d32d98642de6 ("[media] Driver for Toshiba TC358743 HDMI to CSI-2 bridge")
>>>>> Signed-off-by: Maxime Ripard <mripard@kernel.org>
>>>>> ---
>>>>>  drivers/media/i2c/tc358743.c | 10 +++++-----
>>>>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
>>>>> index ca0b0b9bda1755313f066ba36ab218873b9ae438..a1c164a7716a10b0cb9ff38f88c0513b45f24771 100644
>>>>> --- a/drivers/media/i2c/tc358743.c
>>>>> +++ b/drivers/media/i2c/tc358743.c
>>>>> @@ -688,11 +688,11 @@ static void tc358743_set_csi_color_space(struct v4l2_subdev *sd)
>>>>>                 mutex_lock(&state->confctl_mutex);
>>>>>                 i2c_wr16_and_or(sd, CONFCTL, ~MASK_YCBCRFMT,
>>>>>                                 MASK_YCBCRFMT_422_8_BIT);
>>>>>                 mutex_unlock(&state->confctl_mutex);
>>>>>                 break;
>>>>> -       case MEDIA_BUS_FMT_RGB888_1X24:
>>>>> +       case MEDIA_BUS_FMT_BGR888_1X24:
>>>>>                 v4l2_dbg(2, debug, sd, "%s: RGB 888 24-bit\n", __func__);
>>>>>                 i2c_wr8_and_or(sd, VOUT_SET2,
>>>>>                                 ~(MASK_SEL422 | MASK_VOUT_422FIL_100) & 0xff,
>>>>>                                 0x00);
>>>>>                 i2c_wr8_and_or(sd, VI_REP, ~MASK_VOUT_COLOR_SEL & 0xff,
>>>>> @@ -1353,11 +1353,11 @@ static int tc358743_log_status(struct v4l2_subdev *sd)
>>>>>                         (i2c_rd16(sd, CSI_STATUS) & MASK_S_HLT) ?
>>>>>                         "yes" : "no");
>>>>>         v4l2_info(sd, "Color space: %s\n",
>>>>>                         state->mbus_fmt_code == MEDIA_BUS_FMT_UYVY8_1X16 ?
>>>>>                         "YCbCr 422 16-bit" :
>>>>> -                       state->mbus_fmt_code == MEDIA_BUS_FMT_RGB888_1X24 ?
>>>>> +                       state->mbus_fmt_code == MEDIA_BUS_FMT_BGR888_1X24 ?
>>>>>                         "RGB 888 24-bit" : "Unsupported");
>>>>>
>>>>>         v4l2_info(sd, "-----%s status-----\n", is_hdmi(sd) ? "HDMI" : "DVI-D");
>>>>>         v4l2_info(sd, "HDCP encrypted content: %s\n",
>>>>>                         hdmi_sys_status & MASK_S_HDCP ? "yes" : "no");
>>>>> @@ -1691,11 +1691,11 @@ static int tc358743_enum_mbus_code(struct v4l2_subdev *sd,
>>>>>                 struct v4l2_subdev_state *sd_state,
>>>>>                 struct v4l2_subdev_mbus_code_enum *code)
>>>>>  {
>>>>>         switch (code->index) {
>>>>>         case 0:
>>>>> -               code->code = MEDIA_BUS_FMT_RGB888_1X24;
>>>>> +               code->code = MEDIA_BUS_FMT_BGR888_1X24;
>>>
>>> So would this change break or fix the formats[] table in:
>>>
>>> drivers/media/platform/raspberrypi/rp1-cfe/cfe-fmts.h
>>
>> It's pretty much the same table than unicam, and I don't believe it
>> does. For both those drivers the pixels are stored in memory in the CSI
>> wire order, so the proper format to use is BGR24 for CSI, not RGB24.
>>
>>> Are there other bridge drivers that misinterpret MEDIA_BUS_FMT_RGB888_1X24
>>> and/or MEDIA_BUS_FMT_RGB888_1X24?
>>
>> Yes, it's kind of a mess.
>>
>> adv748x, ds90ub960 and tc358743 report RGB888, and ov5640 reports
>> BGR888.
>>
>> Then we have alvium CSI2 that supports both, and can swap color
>> components, so that one isn't a concern.
>>
>> And finally, we have st-mipid02 which is also affected, but is a
>> receiver so it's easier to solve.
>>
>> For RGB565, ov5640, mt9m114 and gc2145 are in that list, but the pixel
>> representation isn't the same than RGB888, so it's not clear to me how
>> they are affected.
>>
>> For RGB666, no v4l2 drivers are affected, but a fair bit of KMS drivers
>> that use media bus formats still might.
> 
> Can we make some progress on this, one way or another?

Thank you for reminding me.

I would prefer to support both MEDIA_BUS_FMT_BGR888_1X24 and MEDIA_BUS_FMT_RGB888_1X24,
as you suggested at one point.

MEDIA_BUS_FMT_RGB888_1X24 should be enumerated last in enum_mbus_code.

If MEDIA_BUS_FMT_RGB888_1X24 is used, a warn_on_once message can be logged. Because
basically it's there for backwards compatibility only.

Clearly document this as well.

I feel uncomfortable just dropping MEDIA_BUS_FMT_RGB888_1X24. This driver is widely
used, certainly on the RPi, and I suspect there are quite a few home-brewn applications
out there that we don't know about.

Would this work for you?

Regards,

	Hans

Re: [PATCH 3/4] media: tc358743: Fix the RGB MBUS format

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 8028 bytes --]

On Thu, Jul 31, 2025 at 12:38:00PM +0200, Hans Verkuil wrote:
> On 31/07/2025 12:14, Maxime Ripard wrote:
> > Hi Hans,
> > 
> > On Wed, Jun 18, 2025 at 04:54:07PM +0200, Maxime Ripard wrote:
> >> On Mon, Jun 16, 2025 at 10:02:17AM +0200, Hans Verkuil wrote:
> >>> On 12/06/2025 19:01, Dave Stevenson wrote:
> >>>> On Thu, 12 Jun 2025 at 13:54, Maxime Ripard <mripard@kernel.org> wrote:
> >>>>>
> >>>>> The tc358743 is an HDMI to MIPI-CSI2 bridge. It supports two of the
> >>>>> three HDMI 1.4 video formats: RGB 4:4:4 and YCbCr 422.
> >>>>>
> >>>>> RGB 4:4:4 is converted to the MIPI-CSI2 RGB888 video format, and listed
> >>>>> in the driver as MEDIA_BUS_FMT_RGB888_1X24.
> >>>>>
> >>>>> Most CSI2 receiver drivers then map MEDIA_BUS_FMT_RGB888_1X24 to
> >>>>> V4L2_PIX_FMT_RGB24.
> >>>>>
> >>>>> 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.
> >>>>>
> >>>>> This essentially means that the R and B will be swapped compared to what
> >>>>> V4L2_PIX_FMT_RGB24 defines.
> >>>>>
> >>>>> The proper MBUS format would be BGR888, so let's use that.
> >>>>
> >>>> I know where you're coming from, but this driver has been in the tree
> >>>> since 2015, so there is a reasonable expectation of users. I've had an
> >>>> overlay for it in our kernel tree since 4.14 (July 2018), and I know
> >>>> of at least PiKVM [1] as a product based on it. I don't know if Cisco
> >>>> are still supporting devices with it in.
> >>>
> >>> Those are all EOL, so no need to be concerned about that.
> >>>
> >>> But it is the most commonly used HDMI-to-CSI bridge, so breaking uAPI is
> >>> a real concern.
> >>
> >> Is it really broken?
> >>
> >> Discussing it with Laurent and Sakari last week, we couldn't find any
> >> example of a userspace where the media format was set in stone and not
> >> propagated across the pipeline.
> >>
> >> The uAPI however is *definitely* broken with unicam right now.
> >>
> >>> See more in my review comment in the code below.
> >>>
> >>>> Whilst the pixel format may now be considered to be incorrect,
> >>>> changing it will break userspace applications that have been using it
> >>>> for those 10 years if they're explicitly looking for
> >>>> MEDIA_BUS_FMT_RGB888_1X24 or the mapping of it through to
> >>>> V4L2_PIX_FMT_RGB24.
> >>>> The kernel docs at [2] quote Linus as saying
> >>>> "If you break existing user space setups THAT IS A REGRESSION.
> >>>> It's not ok to say "but we'll fix the user space setup"
> >>>> Really. NOT OK."
> >>>>
> >>>> I'm thinking of GStreamer if the format has been specified explicitly
> >>>> - it'll fail to negotiate due to v4l2src saying it can't handle the
> >>>> caps.
> >>>>
> >>>> Yes it sucks as a situation, but I'm not sure what the best solution
> >>>> is. Potentially accepting both MEDIA_BUS_FMT_RGB888_1X24 and
> >>>> MEDIA_BUS_FMT_BGR888_1X24 as valid MBUS codes for RGB, alongside
> >>>> MEDIA_BUS_FMT_UYVY8_1X16 for UYVY?
> >>>>
> >>>>   Dave
> >>>>
> >>>> [1] https://pikvm.org/
> >>>> [2] https://www.kernel.org/doc/html/latest/process/handling-regressions.html#quotes-from-linus-about-regression
> >>>>
> >>>>> Fixes: d32d98642de6 ("[media] Driver for Toshiba TC358743 HDMI to CSI-2 bridge")
> >>>>> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> >>>>> ---
> >>>>>  drivers/media/i2c/tc358743.c | 10 +++++-----
> >>>>>  1 file changed, 5 insertions(+), 5 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
> >>>>> index ca0b0b9bda1755313f066ba36ab218873b9ae438..a1c164a7716a10b0cb9ff38f88c0513b45f24771 100644
> >>>>> --- a/drivers/media/i2c/tc358743.c
> >>>>> +++ b/drivers/media/i2c/tc358743.c
> >>>>> @@ -688,11 +688,11 @@ static void tc358743_set_csi_color_space(struct v4l2_subdev *sd)
> >>>>>                 mutex_lock(&state->confctl_mutex);
> >>>>>                 i2c_wr16_and_or(sd, CONFCTL, ~MASK_YCBCRFMT,
> >>>>>                                 MASK_YCBCRFMT_422_8_BIT);
> >>>>>                 mutex_unlock(&state->confctl_mutex);
> >>>>>                 break;
> >>>>> -       case MEDIA_BUS_FMT_RGB888_1X24:
> >>>>> +       case MEDIA_BUS_FMT_BGR888_1X24:
> >>>>>                 v4l2_dbg(2, debug, sd, "%s: RGB 888 24-bit\n", __func__);
> >>>>>                 i2c_wr8_and_or(sd, VOUT_SET2,
> >>>>>                                 ~(MASK_SEL422 | MASK_VOUT_422FIL_100) & 0xff,
> >>>>>                                 0x00);
> >>>>>                 i2c_wr8_and_or(sd, VI_REP, ~MASK_VOUT_COLOR_SEL & 0xff,
> >>>>> @@ -1353,11 +1353,11 @@ static int tc358743_log_status(struct v4l2_subdev *sd)
> >>>>>                         (i2c_rd16(sd, CSI_STATUS) & MASK_S_HLT) ?
> >>>>>                         "yes" : "no");
> >>>>>         v4l2_info(sd, "Color space: %s\n",
> >>>>>                         state->mbus_fmt_code == MEDIA_BUS_FMT_UYVY8_1X16 ?
> >>>>>                         "YCbCr 422 16-bit" :
> >>>>> -                       state->mbus_fmt_code == MEDIA_BUS_FMT_RGB888_1X24 ?
> >>>>> +                       state->mbus_fmt_code == MEDIA_BUS_FMT_BGR888_1X24 ?
> >>>>>                         "RGB 888 24-bit" : "Unsupported");
> >>>>>
> >>>>>         v4l2_info(sd, "-----%s status-----\n", is_hdmi(sd) ? "HDMI" : "DVI-D");
> >>>>>         v4l2_info(sd, "HDCP encrypted content: %s\n",
> >>>>>                         hdmi_sys_status & MASK_S_HDCP ? "yes" : "no");
> >>>>> @@ -1691,11 +1691,11 @@ static int tc358743_enum_mbus_code(struct v4l2_subdev *sd,
> >>>>>                 struct v4l2_subdev_state *sd_state,
> >>>>>                 struct v4l2_subdev_mbus_code_enum *code)
> >>>>>  {
> >>>>>         switch (code->index) {
> >>>>>         case 0:
> >>>>> -               code->code = MEDIA_BUS_FMT_RGB888_1X24;
> >>>>> +               code->code = MEDIA_BUS_FMT_BGR888_1X24;
> >>>
> >>> So would this change break or fix the formats[] table in:
> >>>
> >>> drivers/media/platform/raspberrypi/rp1-cfe/cfe-fmts.h
> >>
> >> It's pretty much the same table than unicam, and I don't believe it
> >> does. For both those drivers the pixels are stored in memory in the CSI
> >> wire order, so the proper format to use is BGR24 for CSI, not RGB24.
> >>
> >>> Are there other bridge drivers that misinterpret MEDIA_BUS_FMT_RGB888_1X24
> >>> and/or MEDIA_BUS_FMT_RGB888_1X24?
> >>
> >> Yes, it's kind of a mess.
> >>
> >> adv748x, ds90ub960 and tc358743 report RGB888, and ov5640 reports
> >> BGR888.
> >>
> >> Then we have alvium CSI2 that supports both, and can swap color
> >> components, so that one isn't a concern.
> >>
> >> And finally, we have st-mipid02 which is also affected, but is a
> >> receiver so it's easier to solve.
> >>
> >> For RGB565, ov5640, mt9m114 and gc2145 are in that list, but the pixel
> >> representation isn't the same than RGB888, so it's not clear to me how
> >> they are affected.
> >>
> >> For RGB666, no v4l2 drivers are affected, but a fair bit of KMS drivers
> >> that use media bus formats still might.
> > 
> > Can we make some progress on this, one way or another?
> 
> Thank you for reminding me.
> 
> I would prefer to support both MEDIA_BUS_FMT_BGR888_1X24 and MEDIA_BUS_FMT_RGB888_1X24,
> as you suggested at one point.
> 
> MEDIA_BUS_FMT_RGB888_1X24 should be enumerated last in enum_mbus_code.
> 
> If MEDIA_BUS_FMT_RGB888_1X24 is used, a warn_on_once message can be logged. Because
> basically it's there for backwards compatibility only.
> 
> Clearly document this as well.
> 
> I feel uncomfortable just dropping MEDIA_BUS_FMT_RGB888_1X24. This driver is widely
> used, certainly on the RPi, and I suspect there are quite a few home-brewn applications
> out there that we don't know about.
> 
> Would this work for you?

It makes total sense to me. I'll work on some patches implementing that.

Thanks!
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

[PATCH 4/4] media: gc2145: Fix the RGB MBUS format

[Maxime Ripard]

The GalaxyCore GC2145 is an MIPI-CSI2 sensor. is an HDMI to MIPI-CSI2
bridge. Among others, it support the MIPI-CSI2 RGB565 format, listed in
the driver as MEDIA_BUS_FMT_RGB565_1X16.

Most CSI2 receiver drivers then map MEDIA_BUS_FMT_RGB565_1X16 to
V4L2_PIX_FMT_RGB565.

However, V4L2_PIX_FMT_RGB565 is defined as having its color components
in the R, G and B order, from left to right. MIPI-CSI2 however defines
the RGB565 format with blue first.

This essentially means that the R and B will be swapped compared to what
V4L2_PIX_FMT_RGB565 defines.

The proper MBUS format would be BGR565, so let's use that.

Fixes: 03cc7fefbb09 ("media: i2c: gc2145: Galaxy Core GC2145 sensor support")
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/media/i2c/gc2145.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/gc2145.c b/drivers/media/i2c/gc2145.c
index ba02161d46e723d14e8670ef89ba10b270d7d178..12ce1c7aa0a9e028bd7d4c84356e0e166294a1f6 100644
--- a/drivers/media/i2c/gc2145.c
+++ b/drivers/media/i2c/gc2145.c
@@ -579,11 +579,11 @@ static const struct gc2145_format supported_formats[] = {
 		.colorspace	= V4L2_COLORSPACE_SRGB,
 		.datatype	= MIPI_CSI2_DT_YUV422_8B,
 		.output_fmt	= 0x03,
 	},
 	{
-		.code		= MEDIA_BUS_FMT_RGB565_1X16,
+		.code		= MEDIA_BUS_FMT_BGR565_1X16,
 		.colorspace	= V4L2_COLORSPACE_SRGB,
 		.datatype	= MIPI_CSI2_DT_RGB565,
 		.output_fmt	= 0x06,
 		.switch_bit	= true,
 	},
@@ -696,11 +696,11 @@ static int gc2145_init_state(struct v4l2_subdev *sd,
 	struct v4l2_rect *crop;
 
 	/* Initialize pad format */
 	format = v4l2_subdev_state_get_format(state, 0);
 	gc2145_update_pad_format(gc2145, &supported_modes[0], format,
-				 MEDIA_BUS_FMT_RGB565_1X16,
+				 MEDIA_BUS_FMT_BGR565_1X16,
 				 V4L2_COLORSPACE_SRGB);
 
 	/* Initialize crop rectangle. */
 	crop = v4l2_subdev_state_get_crop(state, 0);
 	*crop = supported_modes[0].crop;

-- 
2.49.0

Re: [PATCH 4/4] media: gc2145: Fix the RGB MBUS format

[Hans Verkuil]

On 12/06/2025 14:53, Maxime Ripard wrote:
> The GalaxyCore GC2145 is an MIPI-CSI2 sensor. is an HDMI to MIPI-CSI2

Is it a sensor or an HDMI-to-CSI bridge? This looks like a copy-and-paste
mistake.

Regards,

	Hans

> bridge. Among others, it support the MIPI-CSI2 RGB565 format, listed in
> the driver as MEDIA_BUS_FMT_RGB565_1X16.
> 
> Most CSI2 receiver drivers then map MEDIA_BUS_FMT_RGB565_1X16 to
> V4L2_PIX_FMT_RGB565.
> 
> However, V4L2_PIX_FMT_RGB565 is defined as having its color components
> in the R, G and B order, from left to right. MIPI-CSI2 however defines
> the RGB565 format with blue first.
> 
> This essentially means that the R and B will be swapped compared to what
> V4L2_PIX_FMT_RGB565 defines.
> 
> The proper MBUS format would be BGR565, so let's use that.
> 
> Fixes: 03cc7fefbb09 ("media: i2c: gc2145: Galaxy Core GC2145 sensor support")
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>  drivers/media/i2c/gc2145.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/gc2145.c b/drivers/media/i2c/gc2145.c
> index ba02161d46e723d14e8670ef89ba10b270d7d178..12ce1c7aa0a9e028bd7d4c84356e0e166294a1f6 100644
> --- a/drivers/media/i2c/gc2145.c
> +++ b/drivers/media/i2c/gc2145.c
> @@ -579,11 +579,11 @@ static const struct gc2145_format supported_formats[] = {
>  		.colorspace	= V4L2_COLORSPACE_SRGB,
>  		.datatype	= MIPI_CSI2_DT_YUV422_8B,
>  		.output_fmt	= 0x03,
>  	},
>  	{
> -		.code		= MEDIA_BUS_FMT_RGB565_1X16,
> +		.code		= MEDIA_BUS_FMT_BGR565_1X16,
>  		.colorspace	= V4L2_COLORSPACE_SRGB,
>  		.datatype	= MIPI_CSI2_DT_RGB565,
>  		.output_fmt	= 0x06,
>  		.switch_bit	= true,
>  	},
> @@ -696,11 +696,11 @@ static int gc2145_init_state(struct v4l2_subdev *sd,
>  	struct v4l2_rect *crop;
>  
>  	/* Initialize pad format */
>  	format = v4l2_subdev_state_get_format(state, 0);
>  	gc2145_update_pad_format(gc2145, &supported_modes[0], format,
> -				 MEDIA_BUS_FMT_RGB565_1X16,
> +				 MEDIA_BUS_FMT_BGR565_1X16,
>  				 V4L2_COLORSPACE_SRGB);
>  
>  	/* Initialize crop rectangle. */
>  	crop = v4l2_subdev_state_get_crop(state, 0);
>  	*crop = supported_modes[0].crop;
> 

Re: [PATCH 0/4] media: Fix CSI2 RGB vs BGR pixel order

[Hans Verkuil]

On 12/06/2025 14:53, Maxime Ripard wrote:
> Hi,
> 
> Here's an(other [1]) attempt at fixing the current mess due to the
> opposite meaning of what v4l2 and the MIPI-CSI2 spec call "RGB". By v4l2
> nomenclature, the format CSI calls RGB is actually BGR.
> 
> Unfortunately, a handful of CSI transceivers report through RGB media
> bus pixel code, which is then understood as V4L2_PIX_FMT_RGB24 by CSI
> receivers.
> 
> This is made somewhat worse the fact that media bus codes have been made
> mostly with parallel busses in mind, and thus the order of pixels wasn't
> clearly defined anywhere.
> 
> So the v4l2 vs CSI mismatch was confusing (but there's nothing we can do
> about it), but the doc didn't really make an attempt at clearing it up
> either.
> 
> We did have a convention so far though, that about half the affected
> drivers were following. 
> 
> This series improves the doc, adds the missing media bus codes, and
> converts the transceiver drivers to the rightful media bus format.
> 
> We'll also need that series [2] from Laurent to fix all the affected
> transceivers. 
> 
> Let me know what you think,
> Maxime
> 
> 1: https://lore.kernel.org/r/20250606-rpi-unicam-rgb-bgr-fix-v1-1-9930b963f3eb@kernel.org
> 2: https://lore.kernel.org/r/20250611181528.19542-1-laurent.pinchart@ideasonboard.com

This link seems to be wrong. Can you give the correct URL?

Regards,

	Hans

> 
> ---
> Maxime Ripard (4):
>       media: uapi: Clarify MBUS color component order for serial buses
>       media: uapi: Introduce MEDIA_BUS_FMT_BGR565_1X16
>       media: tc358743: Fix the RGB MBUS format
>       media: gc2145: Fix the RGB MBUS format
> 
>  .../userspace-api/media/v4l/subdev-formats.rst     | 51 +++++++++++++++++++---
>  drivers/media/i2c/gc2145.c                         |  4 +-
>  drivers/media/i2c/tc358743.c                       | 10 ++---
>  include/uapi/linux/media-bus-format.h              |  3 +-
>  4 files changed, 54 insertions(+), 14 deletions(-)
> ---
> base-commit: 6e417fb287553495e43135125d099daf80b63fe1
> change-id: 20250612-csi-bgr-rgb-b837980c00b3
> 
> Best regards,

Re: [PATCH 0/4] media: Fix CSI2 RGB vs BGR pixel order

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1712 bytes --]

Hi Hans,

On Mon, Jun 16, 2025 at 10:02:57AM +0200, Hans Verkuil wrote:
> On 12/06/2025 14:53, Maxime Ripard wrote:
> > Here's an(other [1]) attempt at fixing the current mess due to the
> > opposite meaning of what v4l2 and the MIPI-CSI2 spec call "RGB". By v4l2
> > nomenclature, the format CSI calls RGB is actually BGR.
> > 
> > Unfortunately, a handful of CSI transceivers report through RGB media
> > bus pixel code, which is then understood as V4L2_PIX_FMT_RGB24 by CSI
> > receivers.
> > 
> > This is made somewhat worse the fact that media bus codes have been made
> > mostly with parallel busses in mind, and thus the order of pixels wasn't
> > clearly defined anywhere.
> > 
> > So the v4l2 vs CSI mismatch was confusing (but there's nothing we can do
> > about it), but the doc didn't really make an attempt at clearing it up
> > either.
> > 
> > We did have a convention so far though, that about half the affected
> > drivers were following. 
> > 
> > This series improves the doc, adds the missing media bus codes, and
> > converts the transceiver drivers to the rightful media bus format.
> > 
> > We'll also need that series [2] from Laurent to fix all the affected
> > transceivers. 
> > 
> > Let me know what you think,
> > Maxime
> > 
> > 1: https://lore.kernel.org/r/20250606-rpi-unicam-rgb-bgr-fix-v1-1-9930b963f3eb@kernel.org
> > 2: https://lore.kernel.org/r/20250611181528.19542-1-laurent.pinchart@ideasonboard.com
> 
> This link seems to be wrong. Can you give the correct URL?

Yeah, sorry about that. Laurent sent a series to a bunch of us after
discussing it in private, but I assumed it was public.

Hopefully he'll post it soon :)

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]