Re: [PATCH v3 10/20] media: imx355: Remove redundant fll_min, and implement fixed offset

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] When changing the format via `imx355_set_pad_format()`, the driver attempts to reset `vblank` to its default value using `__v4l2_ctrl_s_ctrl()`. It relies on this call to trigger the `s_ctrl` callback (`imx355_set_ctrl()`), which updates the `exposure` control's maximum limit based on the new format's height. However, the V4L2 control framework explicitly optimizes out calls to `s_ctrl` if the new control value equals the current value. If a user manually sets `vblank` to a value that matches the target format's `vblank_def` prior to a format switch, `s_ctrl` is skipped and the `exposure` bounds are not updated. This leaves the control completely out-of-sync with the new format's Frame Length Lines (FLL).
--

--- Patch [10]: [PATCH v3 10/20] media: imx355: Remove redundant fll_min, and implement fixed offset ---
commit 3cb4d69cef75e79b15075f8e2c250b2bb53d96a4
Author: Dave Stevenson <dave.stevenson@raspberrypi.com>
Subject: media: imx355: Remove redundant fll_min, and implement fixed offset

This commit removes the redundant fll_min field from the imx355 mode
configurations, as it is always identical to fll_def. It also sets a fixed
minimum vertical blanking (vblank) limit of 20 to improve the maximum frame
rate achievable across all modes.

> diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
> index 2a97e076bff76..c6f27cf269b40 100644
> --- a/drivers/media/i2c/imx355.c
> +++ b/drivers/media/i2c/imx355.c
[ ... ]
> @@ -889,10 +874,9 @@ imx355_set_pad_format(struct v4l2_subdev *sd,
>  		/* Update limits and set FPS to default */
>  		height = imx355->cur_mode->height;
>  		vblank_def = imx355->cur_mode->fll_def - height;
> -		vblank_min = imx355->cur_mode->fll_min - height;
>  		height = IMX355_FLL_MAX - height;
> -		__v4l2_ctrl_modify_range(imx355->vblank, vblank_min, height, 1,
> -					 vblank_def);
> +		__v4l2_ctrl_modify_range(imx355->vblank, IMX355_VBLANK_MIN,
> +					 height, 1, vblank_def);
>  		__v4l2_ctrl_s_ctrl(imx355->vblank, vblank_def);

[Severity: High]
This is a pre-existing issue, but I noticed a potential problem in how
exposure limits are updated during a format change in
imx355_set_pad_format().

The code relies on __v4l2_ctrl_s_ctrl() to reset the vblank value, expecting
it to trigger the s_ctrl callback (imx355_set_ctrl) to update the exposure
control limits.

However, the V4L2 control framework optimizes out calls to s_ctrl if the
new control value matches the current value. If userspace manually sets
vblank to a value that exactly equals the new format's vblank_def prior to
the format switch, the s_ctrl callback will be skipped.

Could this leave the exposure limits out of sync with the new format's Frame
Length Lines (FLL), resulting in userspace seeing stale limits and potentially
setting an invalid exposure value?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-media-imx355-v3-0-9df386a623d7@raspberrypi.com?part=10