| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAPY8ntDnJexjt+wEBYdMXCPv7RUhJSoBupzcz+V9OiKQYK+yxw@mail.gmail.com>
Date: Mon, 13 Oct 2025 18:15:24 +0100
From: Dave Stevenson <dave.stevenson@...pberrypi.com>
To: Maxime Ripard <mripard@...nel.org>
Cc: 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>,
Hans Verkuil <hverkuil@...nel.org>, Laurent Pinchart <laurent.pinchart@...asonboard.com>,
linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
Hans Verkuil <hans.verkuil@...co.com>
Subject: Re: [PATCH v4 3/4] media: tc358743: Fix the RGB MBUS format
On Mon, 13 Oct 2025 at 12:01, Maxime Ripard <mripard@...nel.org> wrote:
>
> The tc358743 is an HDMI to MIPI-CSI2 bridge. It can output all three
> HDMI 1.4 video formats: RGB 4:4:4, YCbCr 4:2:2, and YCbCr 4:4:4.
>
> 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.
>
> Fixes: d32d98642de6 ("[media] Driver for Toshiba TC358743 HDMI to CSI-2 bridge")
> Signed-off-by: Maxime Ripard <mripard@...nel.org>
Apologies, I thought I'd already sent:
Tested-by: Dave Stevenson <dave.stevenson@...pberrypi.com>
Reviewed-by: Dave Stevenson <dave.stevenson@...pberrypi.com>
Patches 1 and 2 look fine, but I'm not sure it's in my scope to give
an R-b. I can do if it's worth it.
Patch 4 also looks good as it copies this pattern, but I have no
knowledge of that sensor.
Dave
> ---
> drivers/media/i2c/tc358743.c | 53 ++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 44 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
> index a0ca19359c43145988535d7816012ef607555b87..feb089d8de724dd25801a0fb621c768542e05254 100644
> --- a/drivers/media/i2c/tc358743.c
> +++ b/drivers/media/i2c/tc358743.c
> @@ -754,10 +754,11 @@ static void tc358743_set_ref_clk(struct v4l2_subdev *sd)
> }
>
> static void tc358743_set_csi_color_space(struct v4l2_subdev *sd)
> {
> struct tc358743_state *state = to_state(sd);
> + struct device *dev = &state->i2c_client->dev;
>
> switch (state->mbus_fmt_code) {
> case MEDIA_BUS_FMT_UYVY8_1X16:
> v4l2_dbg(2, debug, sd, "%s: YCbCr 422 16-bit\n", __func__);
> i2c_wr8_and_or(sd, VOUT_SET2,
> @@ -769,11 +770,21 @@ static void tc358743_set_csi_color_space(struct v4l2_subdev *sd)
> 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:
> - v4l2_dbg(2, debug, sd, "%s: RGB 888 24-bit\n", __func__);
> + /*
> + * The driver was initially introduced with RGB888
> + * support, but CSI really means BGR.
> + *
> + * Since we might have applications that would have
> + * hard-coded the RGB888, let's support both.
> + */
> + dev_warn_once(dev, "RGB format isn't actually supported by the hardware. The application should be fixed to use BGR.");
> + fallthrough;
> + case MEDIA_BUS_FMT_BGR888_1X24:
> + v4l2_dbg(2, debug, sd, "%s: BGR 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,
> MASK_VOUT_COLOR_RGB_FULL);
> @@ -1430,15 +1441,30 @@ static int tc358743_log_status(struct v4l2_subdev *sd)
> (i2c_rd16(sd, CSI_STATUS) & MASK_S_RXACT) ?
> "yes" : "no");
> v4l2_info(sd, "Stopped: %s\n",
> (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 ?
> - "RGB 888 24-bit" : "Unsupported");
> +
> + switch (state->mbus_fmt_code) {
> + case MEDIA_BUS_FMT_BGR888_1X24:
> + /*
> + * The driver was initially introduced with RGB888
> + * support, but CSI really means BGR.
> + *
> + * Since we might have applications that would have
> + * hard-coded the RGB888, let's support both.
> + */
> + case MEDIA_BUS_FMT_RGB888_1X24:
> + v4l2_info(sd, "Color space: BGR 888 24-bit\n");
> + break;
> + case MEDIA_BUS_FMT_UYVY8_1X16:
> + v4l2_info(sd, "Color space: YCbCr 422 16-bit\n");
> + break;
> + default:
> + v4l2_info(sd, "Color space: Unsupported\n");
> + break;
> + }
>
> 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");
> v4l2_info(sd, "Input color space: %s %s range\n",
> @@ -1771,24 +1797,32 @@ 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;
> break;
> case 1:
> code->code = MEDIA_BUS_FMT_UYVY8_1X16;
> break;
> + case 2:
> + /*
> + * We need to keep RGB888 for backward compatibility,
> + * but we should list it last for userspace to pick BGR.
> + */
> + code->code = MEDIA_BUS_FMT_RGB888_1X24;
> + break;
> default:
> return -EINVAL;
> }
> return 0;
> }
>
> static u32 tc358743_g_colorspace(u32 code)
> {
> switch (code) {
> + case MEDIA_BUS_FMT_BGR888_1X24:
> case MEDIA_BUS_FMT_RGB888_1X24:
> return V4L2_COLORSPACE_SRGB;
> case MEDIA_BUS_FMT_UYVY8_1X16:
> return V4L2_COLORSPACE_SMPTE170M;
> default:
> @@ -1822,11 +1856,12 @@ static int tc358743_set_fmt(struct v4l2_subdev *sd,
> struct tc358743_state *state = to_state(sd);
>
> u32 code = format->format.code; /* is overwritten by get_fmt */
> int ret = tc358743_get_fmt(sd, sd_state, format);
>
> - if (code == MEDIA_BUS_FMT_RGB888_1X24 ||
> + if (code == MEDIA_BUS_FMT_BGR888_1X24 ||
> + code == MEDIA_BUS_FMT_RGB888_1X24 ||
> code == MEDIA_BUS_FMT_UYVY8_1X16)
> format->format.code = code;
> format->format.colorspace = tc358743_g_colorspace(format->format.code);
>
> if (ret)
> @@ -2242,11 +2277,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.51.0
>
Powered by blists - more mailing lists