Re: [PATCH v5 03/10] media: imx219: Account rate_factor in setting upper exposure limit

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The patch updates the exposure maximum limit calculation in `imx219_set_ctrl()` to account for analog binning (initially via `rate_factor`, later rewritten using `bin_hv` in commit "media: imx219: Fix vertical blanking and exposure for analogue binning"), but misses the identical calculation in `imx219_set_pad_format()`. This logic error persists through the end of the series. When a binned format is set, `__v4l2_ctrl_s_ctrl(vblank)` correctly updates the exposure limit via `imx219_set_ctrl()`. However, immediately afterward, `imx219_set_pad_format()` recalculates and overwrites `exposure_max` using the unscaled formula `mode->fll_def - IMX219_EXPOSURE_OFFSET` (instead of `(mode->fll_def / bin_hv) - IMX219_EXPOSURE_OFFSET`). This allows userspace to set an exposure time greater than the frame length, violating sensor hardware constraints.
--

commit 5813edc0a3e8ed48f469607d03f8489ba7a6eb19
Author: Sakari Ailus <sakari.ailus@linux.intel.com>

media: imx219: Account rate_factor in setting upper exposure limit

This commit modifies the maximum exposure time calculation in imx219_set_ctrl()
to account for the rate_factor multiplier when analog binning is used. This
ensures the sensor's exposure time margin is correctly respected.

> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index d8fe7db18b6cd..e681f80f9e3ef 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -451,7 +451,8 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
>  		int exposure_max, exposure_def;
>  
>  		/* Update max exposure while meeting expected vblanking */
> -		exposure_max = format->height + ctrl->val - IMX219_EXPOSURE_OFFSET;
> +		exposure_max = format->height + ctrl->val -
> +			IMX219_EXPOSURE_OFFSET * rate_factor;

[Severity: High]
This is a pre-existing issue, but does imx219_set_pad_format() also need to
be updated to apply scaling to its exposure maximum calculation?

When a binned format is set, __v4l2_ctrl_s_ctrl(vblank) correctly updates
the exposure limit here in imx219_set_ctrl(). However, immediately afterward,
imx219_set_pad_format() recalculates and overwrites exposure_max using the
unscaled formula:

drivers/media/i2c/imx219.c:imx219_set_pad_format() {
	...
		/* Update max exposure while meeting expected vblanking */
		exposure_max = mode->fll_def - IMX219_EXPOSURE_OFFSET;
		exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ?
				exposure_max : IMX219_EXPOSURE_DEFAULT;
		ret = __v4l2_ctrl_modify_range(imx219->exposure,
					       imx219->exposure->minimum,
					       exposure_max,
					       imx219->exposure->step,
					       exposure_def);
	...
}

It appears this logic error persists until it is rewritten using bin_hv in
the later commit "media: imx219: Fix vertical blanking and exposure for
analogue binning".

Until then, does this allow userspace to set an exposure time greater than
the actual programmed frame length, violating sensor hardware constraints and
potentially resulting in sensor streaming failures or I2C timeouts?

>  		exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ?
>  				exposure_max : IMX219_EXPOSURE_DEFAULT;
>  		ret = __v4l2_ctrl_modify_range(imx219->exposure,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260607215356.842932-1-sakari.ailus@linux.intel.com?part=3