Re: [PATCH] media: imx335: Set vblank immediately

[Kieran Bingham <kieran.bingham@ideasonboard.com>]

Quoting nicolas@ndufresne.ca (2025-02-14 14:44:05)
> Hi,
> 
> Le vendredi 14 février 2025 à 22:37 +0900, Paul Elder a écrit :
> > When the vblank v4l2 control is set, it does not get written to the
> > hardware immediately. It only gets updated when exposure is set.
> > Change
> > the behavior such that the vblank is written immediately when the
> > control is set.
> 
> Not my field of competence, but won't this cause a flicker ?

No it shouldn't. The controls will only take effect on the next frame
boundary, but what was missing here before was that the VBLANK was not
being changed at all unless userspace also sets the Exposure control
directly.

That means that setting the exposure, and then setting the framerate
would not update the framerate (adjusted through the blanking), meaning
the framerate could not be updated at runtime. (Or it could be set but
would not take effect).

Note that in the event that VBLANK is updated, the exposure and blanking
is fully recalculated and reprogrammed to the hardware through
imx335_update_exp_gain(), which is required because the exposure is
proportional against the total frame duration. Perhaps that's specific
area you were concerned about potential for flicker?

Perhaps the commit message should make it clear that currently setting
the vblank does not take effect at all *unless* the exposure is also
*changed*. And Setting the vblank without changing the exposure is a
valid use case to change the frame rate.



> Nicolas
> 
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > ---
> >  drivers/media/i2c/imx335.c | 19 +++++++++++++------
> >  1 file changed, 13 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> > index fcfd1d851bd4..e73a23bbbc89 100644
> > --- a/drivers/media/i2c/imx335.c
> > +++ b/drivers/media/i2c/imx335.c
> > @@ -559,12 +559,12 @@ static int imx335_set_ctrl(struct v4l2_ctrl
> > *ctrl)
> >                       imx335->vblank,
> >                       imx335->vblank + imx335->cur_mode->height);
> >  
> > -             return __v4l2_ctrl_modify_range(imx335->exp_ctrl,
> > -                                             IMX335_EXPOSURE_MIN,
> > -                                             imx335->vblank +
> > -                                             imx335->cur_mode-
> > >height -
> > -
> >                                               IMX335_EXPOSURE_OFFSET,
> > -                                             1,
> > IMX335_EXPOSURE_DEFAULT);
> > +              __v4l2_ctrl_modify_range(imx335->exp_ctrl,
> > +                                       IMX335_EXPOSURE_MIN,
> > +                                       imx335->vblank +
> > +                                       imx335->cur_mode->height -
> > +                                       IMX335_EXPOSURE_OFFSET,
> > +                                       1,
> > IMX335_EXPOSURE_DEFAULT);
> >       }
> >  
> >       /*
> > @@ -575,6 +575,13 @@ static int imx335_set_ctrl(struct v4l2_ctrl
> > *ctrl)
> >               return 0;
> >  
> >       switch (ctrl->id) {
> > +     case V4L2_CID_VBLANK:
> > +             exposure = imx335->exp_ctrl->val;
> > +             analog_gain = imx335->again_ctrl->val;
> > +
> > +             ret = imx335_update_exp_gain(imx335, exposure,
> > analog_gain);
> > +

This is what I would expect here,


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


> > +             break;
> >       case V4L2_CID_EXPOSURE:
> >               exposure = ctrl->val;
> >               analog_gain = imx335->again_ctrl->val;
>