Re: [PATCH v7 2/2] media: i2c: add driver for ITE IT6625/IT6626

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] it6625_get_fmt() leaks active hardware state into the TRY format's width, height, and field, violating V4L2 API contracts.
- [Medium] Incorrect pixel clock and frame height calculation for interlaced signals.
--

--- Patch [2]: [PATCH v7 2/2] media: i2c: add driver for ITE IT6625/IT6626 ---
commit ad2f8d71f0d1ad68405361b447bff0d6bd237454
Author: Hermes Wu <Hermes.wu@ite.com.tw>

media: i2c: add driver for ITE IT6625/IT6626

Add a V4L2 subdevice driver for the ITE IT6625/IT6626 HDMI-to-MIPI
CSI-2 bridge chips. IT6625 accepts an HDMI 2.0 input and IT6626 an
HDMI 2.1 input, converting it to a D-PHY (or C/D-PHY on IT6626)
MIPI CSI-2 output.

> diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c
> new file mode 100644
> index 0000000000000..9fb03ddb2acd9
> --- /dev/null
> +++ b/drivers/media/i2c/it6625.c
[ ... ]
> +static int it6625_get_detected_timings(struct it6625 *it6625,
> +				       struct v4l2_dv_timings *timings)
> +{
[ ... ]
> +	bt->hfrontporch = ((buffer2[0] & 0xff) << 8) + buffer2[1];
> +	bt->hsync = ((buffer2[2] & 0xff) << 8) + buffer2[3];
> +	bt->hbackporch = ((buffer2[4] & 0xff) << 8) + buffer2[5];
> +	bt->vfrontporch = ((buffer2[6] & 0xff) << 8) + buffer2[7];
> +	bt->vsync = ((buffer2[8] & 0xff) << 8) + buffer2[9];
> +	bt->vbackporch = ((buffer2[10] & 0xff) << 8) + buffer2[11];
> +
> +	bt->pixelclock = it6625_get_pclk(it6625);
> +	if (bt->interlaced == V4L2_DV_INTERLACED) {
> +		bt->height *= 2;
> +		bt->il_vsync = bt->vsync + 1;
> +		bt->pixelclock /= 2;
> +	}

[Severity: Medium]
Are the interlaced field vertical porches and pixel clock handled correctly
here?

Halving the pixel clock and omitting the second field's porches
(il_vfrontporch, il_vbackporch) will cause the V4L2 core macro
V4L2_DV_BT_FRAME_HEIGHT() to compute an artificially shrunken frame height.

When combined with the halved pixel clock, frame rate calculations like
fps_from_bt_timings() will report incorrect values for interlaced signals.

> +
> +	return 0;
> +}
[ ... ]
> +static int it6625_get_fmt(struct v4l2_subdev *sd,
> +			  struct v4l2_subdev_state *sd_state,
> +			  struct v4l2_subdev_format *format)
> +{
> +	struct it6625 *it6625 = sd_to_6625(sd);
> +	struct v4l2_dv_timings timings;
> +
> +	if (format->pad != 0)
> +		return -EINVAL;
> +
> +	it6625_get_timings(it6625, &timings);
> +	format->format.width = timings.bt.width;
> +	format->format.height = timings.bt.height;
> +	format->format.field = timings.bt.interlaced == V4L2_DV_INTERLACED ?
> +			       V4L2_FIELD_INTERLACED : V4L2_FIELD_NONE;

[Severity: Medium]
Does this code leak the active hardware state into the TRY format state?

Active hardware timings are unconditionally written to the format structure
here, bypassing the V4L2_SUBDEV_FORMAT_TRY state isolation entirely.

> +
> +	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> +		struct v4l2_mbus_framefmt *fmt;
> +
> +		fmt = v4l2_subdev_state_get_format(sd_state, format->pad);
> +		format->format.code = fmt->code;
> +		format->format.colorspace = fmt->colorspace;
> +	} else {
> +		scoped_guard(mutex, &it6625->it6625_lock) {
> +			format->format.colorspace =
> +				format_to_colorspace(it6625->csi_format);
> +			format->format.code = it6625->mbus_fmt_code;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int it6625_set_fmt(struct v4l2_subdev *sd,
> +			  struct v4l2_subdev_state *sd_state,
> +			  struct v4l2_subdev_format *format)
> +{
[ ... ]
> +	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> +		struct v4l2_mbus_framefmt *fmt;
> +
> +		fmt = v4l2_subdev_state_get_format(sd_state, format->pad);
> +		fmt->code = format->format.code;
> +		fmt->colorspace = format_to_colorspace(it6625_formats[ret].csi_format);
> +		format->format.colorspace = fmt->colorspace;
> +		v4l2_dbg(1, debug, sd, "%s: try format code = 0x%x",
> +			 __func__, format->format.code);
> +		return 0;
> +	}

[Severity: Medium]
Will this leave the TRY state dimensions uninitialized or stale?

The TRY branch in it6625_set_fmt() updates the code and colorspace, but
fails to update the width and height in the sd_state to reflect the
requested format dimensions.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723-upstream-it6625-it6626-v7-0-f8175bb0ea12@ite.com.tw?part=2