| Message ID | 20250214133709.1290585-1-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: imx335: Set vblank immediately | expand |
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;
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; >
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;
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;
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. :-)
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
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;
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(-)