Message-ID: <20250731-teal-oryx-of-shopping-ced228@houat>
Date: Thu, 31 Jul 2025 12:14:26 +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

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?

Maxime

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

Powered by blists - more mailing lists