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

[Richard Leitner <richard.leitner@linux.dev>]

On Mon, Sep 28, 2026 at 02:15:05PM +0100, Dave Stevenson wrote:
> Hi Richard
> 
> On Mon, 14 Sept 2026 at 20:21, Richard Leitner
> <richard.leitner@linux.dev> 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.

Sure. You're right. Thanks for the catch!

> 
> > 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@linux.dev>
> > ---
> >  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.

Thanks for the explanation!

If resetting vblank and hblank on a mode change is the "preferred" or
"usual" implementation, then I'm of course fine with that. Either in
this series or in a future one ;-)

> 
> 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.

Regarding the vblank set_ctrl on mode change: I will do another testing
round and either add a comment on the behaviour or remove that
modify_range call.

thanks!

regards;rl

> 
>  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
> >
> >