[PATCH] media: imx335: Set vblank immediately

[PATCH] media: imx335: Set vblank immediately

[Paul Elder]

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.

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);
+
+		break;
 	case V4L2_CID_EXPOSURE:
 		exposure = ctrl->val;
 		analog_gain = imx335->again_ctrl->val;
-- 
2.39.2

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

[nicolas]

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 ?

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);
> +
> +		break;
>  	case V4L2_CID_EXPOSURE:
>  		exposure = ctrl->val;
>  		analog_gain = imx335->again_ctrl->val;

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

[Kieran Bingham]

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

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

[Sakari Ailus]

Hi Paul,

On Fri, Feb 14, 2025 at 10:37:09PM +0900, Paul Elder wrote:
> 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.
> 
> 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,

Indentation.

You're also missing an error check here.

> +					  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);
> +
> +		break;
>  	case V4L2_CID_EXPOSURE:
>  		exposure = ctrl->val;
>  		analog_gain = imx335->again_ctrl->val;

-- 
Regards,

Sakari Ailus

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

[Paul Elder]

Hi Sakari,

Thanks for the review.

On Sun, Feb 16, 2025 at 10:01:27AM +0000, Sakari Ailus wrote:
> Hi Paul,
> 
> On Fri, Feb 14, 2025 at 10:37:09PM +0900, Paul Elder wrote:
> > 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.
> > 
> > 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,
> 
> Indentation.
> 
> You're also missing an error check here.

I reasoned that it's fine to not have the error check.

afaiu, the only change this has to error is if try/s_ctrl on
V4L2_CID_EXPOSURE fails when the change to the range of valid exposure
values requires a new exposure value to be set. Setting the exposure
control comes back to this function, and goes through the switch-case
and imx335_update_exp_gain() below, which doesn't fail.

Also the imx219 has the exact same pattern in imx219_set_ctrl.


Thanks,

Paul

> > +					  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);
> > +
> > +		break;
> >  	case V4L2_CID_EXPOSURE:
> >  		exposure = ctrl->val;
> >  		analog_gain = imx335->again_ctrl->val;

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

[Sakari Ailus]

Hi Paul,

On Wed, Feb 26, 2025 at 05:49:07PM +0900, Paul Elder wrote:
> Hi Sakari,
> 
> Thanks for the review.
> 
> On Sun, Feb 16, 2025 at 10:01:27AM +0000, Sakari Ailus wrote:
> > Hi Paul,
> > 
> > On Fri, Feb 14, 2025 at 10:37:09PM +0900, Paul Elder wrote:
> > > 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.
> > > 
> > > 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,
> > 
> > Indentation.
> > 
> > You're also missing an error check here.
> 
> I reasoned that it's fine to not have the error check.
> 
> afaiu, the only change this has to error is if try/s_ctrl on
> V4L2_CID_EXPOSURE fails when the change to the range of valid exposure
> values requires a new exposure value to be set. Setting the exposure
> control comes back to this function, and goes through the switch-case
> and imx335_update_exp_gain() below, which doesn't fail.

It will fail if cci_write() it calls does.

> 
> Also the imx219 has the exact same pattern in imx219_set_ctrl.

Feel free to fix it. :-)

-- 
Kind regards,

Sakari Ailus

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

[Paul Elder]

On Wed, Feb 26, 2025 at 08:56:06AM +0000, Sakari Ailus wrote:
> Hi Paul,
> 
> On Wed, Feb 26, 2025 at 05:49:07PM +0900, Paul Elder wrote:
> > Hi Sakari,
> > 
> > Thanks for the review.
> > 
> > On Sun, Feb 16, 2025 at 10:01:27AM +0000, Sakari Ailus wrote:
> > > Hi Paul,
> > > 
> > > On Fri, Feb 14, 2025 at 10:37:09PM +0900, Paul Elder wrote:
> > > > 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.
> > > > 
> > > > 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,
> > > 
> > > Indentation.
> > > 
> > > You're also missing an error check here.
> > 
> > I reasoned that it's fine to not have the error check.
> > 
> > afaiu, the only change this has to error is if try/s_ctrl on
> > V4L2_CID_EXPOSURE fails when the change to the range of valid exposure
> > values requires a new exposure value to be set. Setting the exposure
> > control comes back to this function, and goes through the switch-case
> > and imx335_update_exp_gain() below, which doesn't fail.
> 
> It will fail if cci_write() it calls does.

Hm good point. I suppose we don't actually want to continue on if it
does fail... ok I'll fix it.

> 
> > 
> > Also the imx219 has the exact same pattern in imx219_set_ctrl.
> 
> Feel free to fix it. :-)

:)


Paul