Re: [PATCH v3 06/22] media: imx219: Don't update exposure limits while setting format

[Dave Stevenson <dave.stevenson@raspberrypi.com>]

Hi Sakari

On Wed, 25 Mar 2026 at 10:58, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
>
> Don't update exposure limits explicitly while setting format. This is
> already done through the s_ctrl() callback.

done through the s_ctrl() callback for V4L2_CID_VBLANK.

I believe this change leaves a potential for future failure if any
modes get added because the ctrl handler framework swallows any s_ctrl
that doesn't change the value.
vblank is an offset on top of height. exposure is absolute in the
number of lines.

As an example, take the current mode of 1920x1080 with fll_def 1763,
therefore a vblank of 1763-1080 = 683, and max exposure of 1080-4 =
1076. Add a new mode of 640x480 with fll_def of 1163, therefore vblank
is 1163-480 = 683, and max exposure is 480-4 = 476. set_pad_format for
either mode will set vblank to 683. As it's the same value,
imx219_set_ctrl will not be called to update vblank, leaving the wrong
max exposure value.

You could add V4L2_CTRL_FLAG_EXECUTE_ON_WRITE to V4L2_CID_VBLANK to
always call s_ctrl, but that would have a larger downside if userspace
repeatedly set it to the same value.

It doesn't affect any of the current modes, but it's going to be a
very ugly one for the unwary. imx219 is often pointed to as a
reference for those writing new drivers, so this would likely
propagate.

  Dave

>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/i2c/imx219.c | 14 --------------
>  1 file changed, 14 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index a72630ad1561..ca6a5939773d 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -867,8 +867,6 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>         crop->top = (IMX219_NATIVE_HEIGHT - crop->height) / 2;
>
>         if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> -               int exposure_max;
> -               int exposure_def;
>                 int llp_min;
>                 int pixel_rate;
>
> @@ -887,18 +885,6 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>                 if (ret)
>                         return ret;
>
> -               /* 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);
> -               if (ret)
> -                       return ret;
> -
>                 /*
>                  * With analog binning the default minimum line length of 3448
>                  * can cause artefacts with RAW10 formats, because the ADC
> --
> 2.47.3
>