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@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>>  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
>>
>