| Message ID | 20250214133709.1290585-1-paul.elder@ideasonboard.com (mailing list archive) |
|---|---|
| State | New |
| 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;
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(-)