Re: [PATCH 05/10] media: i2c: ov9282: add refresh of missing ranges on a mode change

[Dave Stevenson]

Hi Richard

On Mon, 14 Sept 2026 at 20:21, Richard Leitner
<richard.leitner@xxxxxxxxx> wrote:
>
> ov9282_update_controls() updates the pixel rate, hblank and vblank, but
> currently misses the exposure range and flash_duration. Both are dependent
> on the line time and therefore the pixel format.

The exposure control is dependent on vblank / frame height, not the line time.

> Refresh both, and commit cur_mode and code in ov9282_set_pad_format()
> before the call so the refresh and any nested s_ctrl see the incoming
> format.
>
> Signed-off-by: Richard Leitner <richard.leitner@xxxxxxxxx>
> ---
>  drivers/media/i2c/ov9282.c | 39 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 34 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index 4c88de1965171..e64d8343c18e9 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -557,6 +557,8 @@ static int ov9282_update_controls(struct ov9282 *ov9282,
>  {
>         u32 hblank_min;
>         s64 pixel_rate;
> +       u32 exposure_us;
> +       u32 lpfr;
>         int ret;
>
>         ret = __v4l2_ctrl_s_ctrl(ov9282->link_freq_ctrl, mode->link_freq_idx);
> @@ -577,8 +579,22 @@ static int ov9282_update_controls(struct ov9282 *ov9282,
>         if (ret)
>                 return ret;
>
> -       return __v4l2_ctrl_modify_range(ov9282->vblank_ctrl, mode->vblank_min,
> -                                       mode->vblank_max, 1, mode->vblank);
> +       ret = __v4l2_ctrl_modify_range(ov9282->vblank_ctrl, mode->vblank_min,
> +                                      mode->vblank_max, 1, mode->vblank);
> +       if (ret)
> +               return ret;
> +
> +       lpfr = ov9282->vblank_ctrl->val + mode->height;
> +       ret = __v4l2_ctrl_modify_range(ov9282->exp_ctrl, OV9282_EXPOSURE_MIN,
> +                                      lpfr - OV9282_EXPOSURE_OFFSET,
> +                                      OV9282_EXPOSURE_STEP,
> +                                      OV9282_EXPOSURE_DEFAULT);

I would have expected that this should be handled by the first clause
in ov9282_set_ctrl where if vblank is changed it updates exp_ctrl.
That's how many drivers handle it.

Looking closer they tend to set an explicit vblank on mode change
though, and it is that which triggers the set_ctrl.
There has been previous debate as to whether changing mode should
reset blanking to give a defined frame rate. Memory says that Sakari
was in favour of doing that, but I can't find the thread.
Sashiko also flagged on a previous patchset of mine that if the
previous mode happened to have had vblank adjusted to be the same as
the new default value that the new mode selects, then the control
handler framework won't call set_ctrl(VBLANK), and so the exposure
range (depending on mode height and VBLANK) won't get updated.

All a little messy, so I can be persuaded that there are enough holes
in the current driver that the easiest solution is just to update the
control ranges here if you say that set_ctrl(VBLANK) doesn't get
called in this case.

 Dave

> +       if (ret)
> +               return ret;
> +
> +       exposure_us = ov9282_exposure_to_us(ov9282, ov9282->exp_ctrl->val);
> +       return __v4l2_ctrl_modify_range(ov9282->flash_duration, 0, exposure_us,
> +                                       1, OV9282_STROBE_FRAME_SPAN_DEFAULT);
>  }
>
>  /**
> @@ -855,10 +871,23 @@ static int ov9282_set_pad_format(struct v4l2_subdev *sd,
>                 framefmt = v4l2_subdev_state_get_format(sd_state, fmt->pad);
>                 *framefmt = fmt->format;
>         } else {
> +               const struct ov9282_mode *old_mode = ov9282->cur_mode;
> +               u32 old_code = ov9282->code;
> +
> +               /*
> +                * Commit before refreshing the ranges. ov9282_update_controls()
> +                * and the nested ov9282_set_ctrl() calls it triggers derive the
> +                * frame length and the line time from cur_mode and code, so
> +                * they have to describe the incoming format, not the outgoing
> +                * one.
> +                */
> +               ov9282->cur_mode = mode;
> +               ov9282->code = code;
> +
>                 ret = ov9282_update_controls(ov9282, mode, fmt);
> -               if (!ret) {
> -                       ov9282->cur_mode = mode;
> -                       ov9282->code = code;
> +               if (ret) {
> +                       ov9282->cur_mode = old_mode;
> +                       ov9282->code = old_code;
>                 }
>         }
>
>
> --
> 2.53.0
>
>