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

[Hans Verkuil <hverkuil@xs4all.nl>]

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