Message-ID: <20250731-eggplant-oarfish-of-patience-e6e65a@houat>
Date: Thu, 31 Jul 2025 15:02:17 +0200
From: Maxime Ripard <mripard@...nel.org>
To: Hans Verkuil <hverkuil@...all.nl>
Cc: Dave Stevenson <dave.stevenson@...pberrypi.com>, 
	Mauro Carvalho Chehab <mchehab@...nel.org>, Mats Randgaard <matrandg@...co.com>, 
	Alain Volmat <alain.volmat@...s.st.com>, Sakari Ailus <sakari.ailus@...ux.intel.com>, 
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/4] media: tc358743: Fix the RGB MBUS format

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@...nel.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@...nel.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

Download attachment "signature.asc" of type "application/pgp-signature" (274 bytes)

Powered by blists - more mailing lists