Message-ID: <20250618-dancing-rare-skua-eb7ffd@houat>
Date: Wed, 18 Jun 2025 16:54:07 +0200
From: Maxime Ripard <mripard@...nel.org>
To: Hans Verkuil <hans@...erkuil.nl>
Cc: Dave Stevenson <dave.stevenson@...pberrypi.com>, 
	Mauro Carvalho Chehab <mchehab@...nel.org>, Hans Verkuil <hverkuil@...all.nl>, 
	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 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.

Maxime

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

Powered by blists - more mailing lists