diff mbox series

[v3] media: imx258: add vblank control to support more frame rate range

Message ID 1634527576-2928-1-git-send-email-bingbu.cao@intel.com (mailing list archive)
State New, archived
Headers show
Series [v3] media: imx258: add vblank control to support more frame rate range | expand

Commit Message

Bingbu Cao Oct. 18, 2021, 3:26 a.m. UTC
Current imx258 driver enable the automatic frame length tracking control
by default and did not support VBLANK change, it's always working at 30fps.
However, in reality we need a wider frame rate range from 15 to 30.
This patch disable the automatic frame length tracking control and enable
the v4l2 VBLANK control to allow user changing frame rate per requirement.

Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
---
 drivers/media/i2c/imx258.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

Comments

Sakari Ailus Oct. 19, 2021, 12:51 p.m. UTC | #1
Hi Bingbu,

On Mon, Oct 18, 2021 at 11:26:16AM +0800, Bingbu Cao wrote:
> Current imx258 driver enable the automatic frame length tracking control
> by default and did not support VBLANK change, it's always working at 30fps.
> However, in reality we need a wider frame rate range from 15 to 30.
> This patch disable the automatic frame length tracking control and enable
> the v4l2 VBLANK control to allow user changing frame rate per requirement.
> 
> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
> ---
>  drivers/media/i2c/imx258.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> index 81cdf37216ca..2c787af7074d 100644
> --- a/drivers/media/i2c/imx258.c
> +++ b/drivers/media/i2c/imx258.c
> @@ -29,6 +29,7 @@
>  #define IMX258_VTS_MAX			0xffff
>  
>  /*Frame Length Line*/
> +#define IMX258_REG_FLL			0x0340
>  #define IMX258_FLL_MIN			0x08a6
>  #define IMX258_FLL_MAX			0xffff
>  #define IMX258_FLL_STEP			1
> @@ -241,7 +242,7 @@ static const struct imx258_reg mode_4208x3118_regs[] = {
>  	{ 0x034D, 0x70 },
>  	{ 0x034E, 0x0C },
>  	{ 0x034F, 0x30 },
> -	{ 0x0350, 0x01 },
> +	{ 0x0350, 0x00 }, /* no frame length automatic tracking control */
>  	{ 0x0202, 0x0C },
>  	{ 0x0203, 0x46 },
>  	{ 0x0204, 0x00 },
> @@ -360,7 +361,7 @@ static const struct imx258_reg mode_2104_1560_regs[] = {
>  	{ 0x034D, 0x38 },
>  	{ 0x034E, 0x06 },
>  	{ 0x034F, 0x18 },
> -	{ 0x0350, 0x01 },
> +	{ 0x0350, 0x00 }, /* no frame length automatic tracking control */
>  	{ 0x0202, 0x06 },
>  	{ 0x0203, 0x2E },
>  	{ 0x0204, 0x00 },
> @@ -479,7 +480,7 @@ static const struct imx258_reg mode_1048_780_regs[] = {
>  	{ 0x034D, 0x18 },
>  	{ 0x034E, 0x03 },
>  	{ 0x034F, 0x0C },
> -	{ 0x0350, 0x01 },
> +	{ 0x0350, 0x00 }, /* no frame length automatic tracking control */

Why is automatic frame length control disabled?

>  	{ 0x0202, 0x03 },
>  	{ 0x0203, 0x42 },
>  	{ 0x0204, 0x00 },
> @@ -753,8 +754,17 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl)
>  	struct imx258 *imx258 =
>  		container_of(ctrl->handler, struct imx258, ctrl_handler);
>  	struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd);
> +	s64 max;
>  	int ret = 0;
>  
> +	if (ctrl->id == V4L2_CID_VBLANK) {
> +		/* Update max exposure to meet expected vblanking */
> +		max = imx258->cur_mode->height + ctrl->val - 10;
> +		__v4l2_ctrl_modify_range(imx258->exposure,
> +					 imx258->exposure->minimum,
> +					 max, imx258->exposure->step, max);
> +	}
> +
>  	/*
>  	 * Applying V4L2 control value only happens
>  	 * when power is up for streaming
> @@ -773,6 +783,10 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl)
>  				IMX258_REG_VALUE_16BIT,
>  				ctrl->val);
>  		break;
> +	case V4L2_CID_VBLANK:
> +		ret = imx258_write_reg(imx258, IMX258_REG_FLL, 2,
> +				       imx258->cur_mode->height + ctrl->val);
> +		break;
>  	case V4L2_CID_DIGITAL_GAIN:
>  		ret = imx258_update_digital_gain(imx258, IMX258_REG_VALUE_16BIT,
>  				ctrl->val);
> @@ -1189,9 +1203,6 @@ static int imx258_init_controls(struct imx258 *imx258)
>  				IMX258_VTS_MAX - imx258->cur_mode->height, 1,
>  				vblank_def);
>  
> -	if (imx258->vblank)
> -		imx258->vblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> -
>  	imx258->hblank = v4l2_ctrl_new_std(
>  				ctrl_hdlr, &imx258_ctrl_ops, V4L2_CID_HBLANK,
>  				IMX258_PPL_DEFAULT - imx258->cur_mode->width,
Bingbu Cao Oct. 19, 2021, 3:30 p.m. UTC | #2
Sakari,
Bingbu Cao Oct. 19, 2021, 3:58 p.m. UTC | #3
> -----Original Message-----
> From: Cao, Bingbu
> Sent: Tuesday, October 19, 2021 11:30 PM
> To: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: linux-media@vger.kernel.org; tfiga@chromium.org;
> kieran.bingham@ideasonboard.com; bingbu.cao@linux.intel.com
> Subject: RE: [PATCH v3] media: imx258: add vblank control to support more
> frame rate range
> 
> Sakari,
> 
> ________________________
> BRs,
> Bingbu Cao
> 
> > -----Original Message-----
> > From: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Sent: Tuesday, October 19, 2021 8:52 PM
> > To: Cao, Bingbu <bingbu.cao@intel.com>
> > Cc: linux-media@vger.kernel.org; tfiga@chromium.org;
> > kieran.bingham@ideasonboard.com; bingbu.cao@linux.intel.com
> > Subject: Re: [PATCH v3] media: imx258: add vblank control to support
> > more frame rate range
> >
> > Hi Bingbu,
> >
> > On Mon, Oct 18, 2021 at 11:26:16AM +0800, Bingbu Cao wrote:
> > > Current imx258 driver enable the automatic frame length tracking
> > > control by default and did not support VBLANK change, it's always
> > working at 30fps.
> > > However, in reality we need a wider frame rate range from 15 to 30.
> > > This patch disable the automatic frame length tracking control and
> > > enable the v4l2 VBLANK control to allow user changing frame rate per
> > requirement.
> > >
> > > Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
> > > ---
> > >  drivers/media/i2c/imx258.c | 23 +++++++++++++++++------
> > >  1 file changed, 17 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> > > index 81cdf37216ca..2c787af7074d 100644
> > > --- a/drivers/media/i2c/imx258.c
> > > +++ b/drivers/media/i2c/imx258.c
> > > @@ -29,6 +29,7 @@
> > >  #define IMX258_VTS_MAX			0xffff
> > >
> > >  /*Frame Length Line*/
> > > +#define IMX258_REG_FLL			0x0340
> > >  #define IMX258_FLL_MIN			0x08a6
> > >  #define IMX258_FLL_MAX			0xffff
> > >  #define IMX258_FLL_STEP			1
> > > @@ -241,7 +242,7 @@ static const struct imx258_reg
> > > mode_4208x3118_regs[]
> > = {
> > >  	{ 0x034D, 0x70 },
> > >  	{ 0x034E, 0x0C },
> > >  	{ 0x034F, 0x30 },
> > > -	{ 0x0350, 0x01 },
> > > +	{ 0x0350, 0x00 }, /* no frame length automatic tracking control */
> > >  	{ 0x0202, 0x0C },
> > >  	{ 0x0203, 0x46 },
> > >  	{ 0x0204, 0x00 },
> > > @@ -360,7 +361,7 @@ static const struct imx258_reg
> > > mode_2104_1560_regs[]
> > = {
> > >  	{ 0x034D, 0x38 },
> > >  	{ 0x034E, 0x06 },
> > >  	{ 0x034F, 0x18 },
> > > -	{ 0x0350, 0x01 },
> > > +	{ 0x0350, 0x00 }, /* no frame length automatic tracking control */
> > >  	{ 0x0202, 0x06 },
> > >  	{ 0x0203, 0x2E },
> > >  	{ 0x0204, 0x00 },
> > > @@ -479,7 +480,7 @@ static const struct imx258_reg
> > > mode_1048_780_regs[]
> > = {
> > >  	{ 0x034D, 0x18 },
> > >  	{ 0x034E, 0x03 },
> > >  	{ 0x034F, 0x0C },
> > > -	{ 0x0350, 0x01 },
> > > +	{ 0x0350, 0x00 }, /* no frame length automatic tracking control */
> >
> > Why is automatic frame length control disabled?
> 
> My understanding:
> If automatic frame length control enabled, the frame length is changed
> automatically when COARSE_INTEGRATE_TIME + 10 > FRAME_LENGTH_LINES, it
> may not meet the requirement - less integrate time with more frame length.
> we need control the vertical blank to do that.
> 

If frame length automatic tracking control enabled, the CORSE_INTEGRATE_TIME
could be larger than FRAME_LENGTH_LINES.

> 
> >
> > >  	{ 0x0202, 0x03 },
> > >  	{ 0x0203, 0x42 },
> > >  	{ 0x0204, 0x00 },
> > > @@ -753,8 +754,17 @@ static int imx258_set_ctrl(struct v4l2_ctrl
> *ctrl)
> > >  	struct imx258 *imx258 =
> > >  		container_of(ctrl->handler, struct imx258, ctrl_handler);
> > >  	struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd);
> > > +	s64 max;
> > >  	int ret = 0;
> > >
> > > +	if (ctrl->id == V4L2_CID_VBLANK) {
> > > +		/* Update max exposure to meet expected vblanking */
> > > +		max = imx258->cur_mode->height + ctrl->val - 10;
> > > +		__v4l2_ctrl_modify_range(imx258->exposure,
> > > +					 imx258->exposure->minimum,
> > > +					 max, imx258->exposure->step, max);
> > > +	}
> > > +
> > >  	/*
> > >  	 * Applying V4L2 control value only happens
> > >  	 * when power is up for streaming
> > > @@ -773,6 +783,10 @@ static int imx258_set_ctrl(struct v4l2_ctrl
> *ctrl)
> > >  				IMX258_REG_VALUE_16BIT,
> > >  				ctrl->val);
> > >  		break;
> > > +	case V4L2_CID_VBLANK:
> > > +		ret = imx258_write_reg(imx258, IMX258_REG_FLL, 2,
> > > +				       imx258->cur_mode->height + ctrl->val);
> > > +		break;
> > >  	case V4L2_CID_DIGITAL_GAIN:
> > >  		ret = imx258_update_digital_gain(imx258,
> > IMX258_REG_VALUE_16BIT,
> > >  				ctrl->val);
> > > @@ -1189,9 +1203,6 @@ static int imx258_init_controls(struct imx258
> > *imx258)
> > >  				IMX258_VTS_MAX - imx258->cur_mode->height, 1,
> > >  				vblank_def);
> > >
> > > -	if (imx258->vblank)
> > > -		imx258->vblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > -
> > >  	imx258->hblank = v4l2_ctrl_new_std(
> > >  				ctrl_hdlr, &imx258_ctrl_ops, V4L2_CID_HBLANK,
> > >  				IMX258_PPL_DEFAULT - imx258->cur_mode->width,
> >
> > --
> > Kind regards,
> >
> > Sakari Ailus
Sakari Ailus Oct. 22, 2021, 8:37 p.m. UTC | #4
Hi Bingbu,

On Tue, Oct 19, 2021 at 03:58:41PM +0000, Cao, Bingbu wrote:
> > -----Original Message-----
> > From: Cao, Bingbu
> > Sent: Tuesday, October 19, 2021 11:30 PM
> > To: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Cc: linux-media@vger.kernel.org; tfiga@chromium.org;
> > kieran.bingham@ideasonboard.com; bingbu.cao@linux.intel.com
> > Subject: RE: [PATCH v3] media: imx258: add vblank control to support more
> > frame rate range
> > 
> > Sakari,
> > 
> > ________________________
> > BRs,
> > Bingbu Cao
> > 
> > > -----Original Message-----
> > > From: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > Sent: Tuesday, October 19, 2021 8:52 PM
> > > To: Cao, Bingbu <bingbu.cao@intel.com>
> > > Cc: linux-media@vger.kernel.org; tfiga@chromium.org;
> > > kieran.bingham@ideasonboard.com; bingbu.cao@linux.intel.com
> > > Subject: Re: [PATCH v3] media: imx258: add vblank control to support
> > > more frame rate range
> > >
> > > Hi Bingbu,
> > >
> > > On Mon, Oct 18, 2021 at 11:26:16AM +0800, Bingbu Cao wrote:
> > > > Current imx258 driver enable the automatic frame length tracking
> > > > control by default and did not support VBLANK change, it's always
> > > working at 30fps.
> > > > However, in reality we need a wider frame rate range from 15 to 30.
> > > > This patch disable the automatic frame length tracking control and
> > > > enable the v4l2 VBLANK control to allow user changing frame rate per
> > > requirement.
> > > >
> > > > Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
> > > > ---
> > > >  drivers/media/i2c/imx258.c | 23 +++++++++++++++++------
> > > >  1 file changed, 17 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> > > > index 81cdf37216ca..2c787af7074d 100644
> > > > --- a/drivers/media/i2c/imx258.c
> > > > +++ b/drivers/media/i2c/imx258.c
> > > > @@ -29,6 +29,7 @@
> > > >  #define IMX258_VTS_MAX			0xffff
> > > >
> > > >  /*Frame Length Line*/
> > > > +#define IMX258_REG_FLL			0x0340
> > > >  #define IMX258_FLL_MIN			0x08a6
> > > >  #define IMX258_FLL_MAX			0xffff
> > > >  #define IMX258_FLL_STEP			1
> > > > @@ -241,7 +242,7 @@ static const struct imx258_reg
> > > > mode_4208x3118_regs[]
> > > = {
> > > >  	{ 0x034D, 0x70 },
> > > >  	{ 0x034E, 0x0C },
> > > >  	{ 0x034F, 0x30 },
> > > > -	{ 0x0350, 0x01 },
> > > > +	{ 0x0350, 0x00 }, /* no frame length automatic tracking control */
> > > >  	{ 0x0202, 0x0C },
> > > >  	{ 0x0203, 0x46 },
> > > >  	{ 0x0204, 0x00 },
> > > > @@ -360,7 +361,7 @@ static const struct imx258_reg
> > > > mode_2104_1560_regs[]
> > > = {
> > > >  	{ 0x034D, 0x38 },
> > > >  	{ 0x034E, 0x06 },
> > > >  	{ 0x034F, 0x18 },
> > > > -	{ 0x0350, 0x01 },
> > > > +	{ 0x0350, 0x00 }, /* no frame length automatic tracking control */
> > > >  	{ 0x0202, 0x06 },
> > > >  	{ 0x0203, 0x2E },
> > > >  	{ 0x0204, 0x00 },
> > > > @@ -479,7 +480,7 @@ static const struct imx258_reg
> > > > mode_1048_780_regs[]
> > > = {
> > > >  	{ 0x034D, 0x18 },
> > > >  	{ 0x034E, 0x03 },
> > > >  	{ 0x034F, 0x0C },
> > > > -	{ 0x0350, 0x01 },
> > > > +	{ 0x0350, 0x00 }, /* no frame length automatic tracking control */
> > >
> > > Why is automatic frame length control disabled?
> > 
> > My understanding:
> > If automatic frame length control enabled, the frame length is changed
> > automatically when COARSE_INTEGRATE_TIME + 10 > FRAME_LENGTH_LINES, it
> > may not meet the requirement - less integrate time with more frame length.
> > we need control the vertical blank to do that.
> > 
> 
> If frame length automatic tracking control enabled, the CORSE_INTEGRATE_TIME
> could be larger than FRAME_LENGTH_LINES.

Both are controlled by the driver. The driver is generally responsible for
ensuring the exposure time stays within the limits for a given frame
length.

Unless this sensor does something weird, all you get by disabling this is
undefined behaviour instead of increased frame length when the exposure
time + margin exceeds frame length. This could mean broken frames.

Of course, it takes a driver bug to arrive into this situation.
Tomasz Figa Oct. 28, 2021, 1:52 p.m. UTC | #5
On Sat, Oct 23, 2021 at 5:38 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Bingbu,
>
> On Tue, Oct 19, 2021 at 03:58:41PM +0000, Cao, Bingbu wrote:
> > > -----Original Message-----
> > > From: Cao, Bingbu
> > > Sent: Tuesday, October 19, 2021 11:30 PM
> > > To: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > Cc: linux-media@vger.kernel.org; tfiga@chromium.org;
> > > kieran.bingham@ideasonboard.com; bingbu.cao@linux.intel.com
> > > Subject: RE: [PATCH v3] media: imx258: add vblank control to support more
> > > frame rate range
> > >
> > > Sakari,
> > >
> > > ________________________
> > > BRs,
> > > Bingbu Cao
> > >
> > > > -----Original Message-----
> > > > From: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > Sent: Tuesday, October 19, 2021 8:52 PM
> > > > To: Cao, Bingbu <bingbu.cao@intel.com>
> > > > Cc: linux-media@vger.kernel.org; tfiga@chromium.org;
> > > > kieran.bingham@ideasonboard.com; bingbu.cao@linux.intel.com
> > > > Subject: Re: [PATCH v3] media: imx258: add vblank control to support
> > > > more frame rate range
> > > >
> > > > Hi Bingbu,
> > > >
> > > > On Mon, Oct 18, 2021 at 11:26:16AM +0800, Bingbu Cao wrote:
> > > > > Current imx258 driver enable the automatic frame length tracking
> > > > > control by default and did not support VBLANK change, it's always
> > > > working at 30fps.
> > > > > However, in reality we need a wider frame rate range from 15 to 30.
> > > > > This patch disable the automatic frame length tracking control and
> > > > > enable the v4l2 VBLANK control to allow user changing frame rate per
> > > > requirement.
> > > > >
> > > > > Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
> > > > > ---
> > > > >  drivers/media/i2c/imx258.c | 23 +++++++++++++++++------
> > > > >  1 file changed, 17 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> > > > > index 81cdf37216ca..2c787af7074d 100644
> > > > > --- a/drivers/media/i2c/imx258.c
> > > > > +++ b/drivers/media/i2c/imx258.c
> > > > > @@ -29,6 +29,7 @@
> > > > >  #define IMX258_VTS_MAX                 0xffff
> > > > >
> > > > >  /*Frame Length Line*/
> > > > > +#define IMX258_REG_FLL                 0x0340
> > > > >  #define IMX258_FLL_MIN                 0x08a6
> > > > >  #define IMX258_FLL_MAX                 0xffff
> > > > >  #define IMX258_FLL_STEP                        1
> > > > > @@ -241,7 +242,7 @@ static const struct imx258_reg
> > > > > mode_4208x3118_regs[]
> > > > = {
> > > > >         { 0x034D, 0x70 },
> > > > >         { 0x034E, 0x0C },
> > > > >         { 0x034F, 0x30 },
> > > > > -       { 0x0350, 0x01 },
> > > > > +       { 0x0350, 0x00 }, /* no frame length automatic tracking control */
> > > > >         { 0x0202, 0x0C },
> > > > >         { 0x0203, 0x46 },
> > > > >         { 0x0204, 0x00 },
> > > > > @@ -360,7 +361,7 @@ static const struct imx258_reg
> > > > > mode_2104_1560_regs[]
> > > > = {
> > > > >         { 0x034D, 0x38 },
> > > > >         { 0x034E, 0x06 },
> > > > >         { 0x034F, 0x18 },
> > > > > -       { 0x0350, 0x01 },
> > > > > +       { 0x0350, 0x00 }, /* no frame length automatic tracking control */
> > > > >         { 0x0202, 0x06 },
> > > > >         { 0x0203, 0x2E },
> > > > >         { 0x0204, 0x00 },
> > > > > @@ -479,7 +480,7 @@ static const struct imx258_reg
> > > > > mode_1048_780_regs[]
> > > > = {
> > > > >         { 0x034D, 0x18 },
> > > > >         { 0x034E, 0x03 },
> > > > >         { 0x034F, 0x0C },
> > > > > -       { 0x0350, 0x01 },
> > > > > +       { 0x0350, 0x00 }, /* no frame length automatic tracking control */
> > > >
> > > > Why is automatic frame length control disabled?
> > >
> > > My understanding:
> > > If automatic frame length control enabled, the frame length is changed
> > > automatically when COARSE_INTEGRATE_TIME + 10 > FRAME_LENGTH_LINES, it
> > > may not meet the requirement - less integrate time with more frame length.
> > > we need control the vertical blank to do that.
> > >
> >
> > If frame length automatic tracking control enabled, the CORSE_INTEGRATE_TIME
> > could be larger than FRAME_LENGTH_LINES.
>
> Both are controlled by the driver. The driver is generally responsible for
> ensuring the exposure time stays within the limits for a given frame
> length.
>
> Unless this sensor does something weird, all you get by disabling this is
> undefined behaviour instead of increased frame length when the exposure
> time + margin exceeds frame length. This could mean broken frames.
>
> Of course, it takes a driver bug to arrive into this situation.

I'd argue that enabling the automatic control would make it much more
difficult to spot the driver bug in this case and so it would be more
desirable to keep it disabled as in this patch.

Best regards,
Tomasz
Bingbu Cao Oct. 29, 2021, 2:18 a.m. UTC | #6
Sakari and Tomasz,

Thanks for your review.
Tomasz Figa Oct. 29, 2021, 3:05 a.m. UTC | #7
On Fri, Oct 29, 2021 at 11:18 AM Cao, Bingbu <bingbu.cao@intel.com> wrote:
>
> Sakari and Tomasz,
>
> Thanks for your review.
>
> ________________________
> BRs,
> Bingbu Cao
>
> > -----Original Message-----
> > From: Tomasz Figa <tfiga@chromium.org>
> > Sent: Thursday, October 28, 2021 9:52 PM
> > To: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Cc: Cao, Bingbu <bingbu.cao@intel.com>; linux-media@vger.kernel.org;
> > kieran.bingham@ideasonboard.com; bingbu.cao@linux.intel.com
> > Subject: Re: [PATCH v3] media: imx258: add vblank control to support
> > more frame rate range
> >
> > On Sat, Oct 23, 2021 at 5:38 AM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Hi Bingbu,
> > >
> > > On Tue, Oct 19, 2021 at 03:58:41PM +0000, Cao, Bingbu wrote:
> > > > > -----Original Message-----
> > > > > From: Cao, Bingbu
> > > > > Sent: Tuesday, October 19, 2021 11:30 PM
> > > > > To: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > Cc: linux-media@vger.kernel.org; tfiga@chromium.org;
> > > > > kieran.bingham@ideasonboard.com; bingbu.cao@linux.intel.com
> > > > > Subject: RE: [PATCH v3] media: imx258: add vblank control to
> > > > > support more frame rate range
> > > > >
> > > > > Sakari,
> > > > >
> > > > > ________________________
> > > > > BRs,
> > > > > Bingbu Cao
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > Sent: Tuesday, October 19, 2021 8:52 PM
> > > > > > To: Cao, Bingbu <bingbu.cao@intel.com>
> > > > > > Cc: linux-media@vger.kernel.org; tfiga@chromium.org;
> > > > > > kieran.bingham@ideasonboard.com; bingbu.cao@linux.intel.com
> > > > > > Subject: Re: [PATCH v3] media: imx258: add vblank control to
> > > > > > support more frame rate range
> > > > > >
> > > > > > Hi Bingbu,
> > > > > >
> > > > > > On Mon, Oct 18, 2021 at 11:26:16AM +0800, Bingbu Cao wrote:
> > > > > > > Current imx258 driver enable the automatic frame length
> > > > > > > tracking control by default and did not support VBLANK change,
> > > > > > > it's always
> > > > > > working at 30fps.
> > > > > > > However, in reality we need a wider frame rate range from 15
> > to 30.
> > > > > > > This patch disable the automatic frame length tracking control
> > > > > > > and enable the v4l2 VBLANK control to allow user changing
> > > > > > > frame rate per
> > > > > > requirement.
> > > > > > >
> > > > > > > Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
> > > > > > > ---
> > > > > > >  drivers/media/i2c/imx258.c | 23 +++++++++++++++++------
> > > > > > >  1 file changed, 17 insertions(+), 6 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/media/i2c/imx258.c
> > > > > > > b/drivers/media/i2c/imx258.c index 81cdf37216ca..2c787af7074d
> > > > > > > 100644
> > > > > > > --- a/drivers/media/i2c/imx258.c
> > > > > > > +++ b/drivers/media/i2c/imx258.c
> > > > > > > @@ -29,6 +29,7 @@
> > > > > > >  #define IMX258_VTS_MAX                 0xffff
> > > > > > >
> > > > > > >  /*Frame Length Line*/
> > > > > > > +#define IMX258_REG_FLL                 0x0340
> > > > > > >  #define IMX258_FLL_MIN                 0x08a6
> > > > > > >  #define IMX258_FLL_MAX                 0xffff
> > > > > > >  #define IMX258_FLL_STEP                        1
> > > > > > > @@ -241,7 +242,7 @@ static const struct imx258_reg
> > > > > > > mode_4208x3118_regs[]
> > > > > > = {
> > > > > > >         { 0x034D, 0x70 },
> > > > > > >         { 0x034E, 0x0C },
> > > > > > >         { 0x034F, 0x30 },
> > > > > > > -       { 0x0350, 0x01 },
> > > > > > > +       { 0x0350, 0x00 }, /* no frame length automatic
> > > > > > > + tracking control */
> > > > > > >         { 0x0202, 0x0C },
> > > > > > >         { 0x0203, 0x46 },
> > > > > > >         { 0x0204, 0x00 },
> > > > > > > @@ -360,7 +361,7 @@ static const struct imx258_reg
> > > > > > > mode_2104_1560_regs[]
> > > > > > = {
> > > > > > >         { 0x034D, 0x38 },
> > > > > > >         { 0x034E, 0x06 },
> > > > > > >         { 0x034F, 0x18 },
> > > > > > > -       { 0x0350, 0x01 },
> > > > > > > +       { 0x0350, 0x00 }, /* no frame length automatic
> > > > > > > + tracking control */
> > > > > > >         { 0x0202, 0x06 },
> > > > > > >         { 0x0203, 0x2E },
> > > > > > >         { 0x0204, 0x00 },
> > > > > > > @@ -479,7 +480,7 @@ static const struct imx258_reg
> > > > > > > mode_1048_780_regs[]
> > > > > > = {
> > > > > > >         { 0x034D, 0x18 },
> > > > > > >         { 0x034E, 0x03 },
> > > > > > >         { 0x034F, 0x0C },
> > > > > > > -       { 0x0350, 0x01 },
> > > > > > > +       { 0x0350, 0x00 }, /* no frame length automatic
> > > > > > > + tracking control */
> > > > > >
> > > > > > Why is automatic frame length control disabled?
> > > > >
> > > > > My understanding:
> > > > > If automatic frame length control enabled, the frame length is
> > > > > changed automatically when COARSE_INTEGRATE_TIME + 10 >
> > > > > FRAME_LENGTH_LINES, it may not meet the requirement - less
> > integrate time with more frame length.
> > > > > we need control the vertical blank to do that.
> > > > >
> > > >
> > > > If frame length automatic tracking control enabled, the
> > > > CORSE_INTEGRATE_TIME could be larger than FRAME_LENGTH_LINES.
> > >
> > > Both are controlled by the driver. The driver is generally responsible
> > > for ensuring the exposure time stays within the limits for a given
> > > frame length.
> > >
> > > Unless this sensor does something weird, all you get by disabling this
> > > is undefined behaviour instead of increased frame length when the
> > > exposure time + margin exceeds frame length. This could mean broken
> > frames.
> > >
> > > Of course, it takes a driver bug to arrive into this situation.
> >
> > I'd argue that enabling the automatic control would make it much more
> > difficult to spot the driver bug in this case and so it would be more
> > desirable to keep it disabled as in this patch.
>
> You are right, I will remove the change in next version.

Sorry, remove what change? My comment agrees with this patch that
keeps the function disabled.

Best regards,
Tomasz
Bingbu Cao Oct. 29, 2021, 5:43 a.m. UTC | #8
> -----Original Message-----
> From: Tomasz Figa <tfiga@chromium.org>
> Sent: Friday, October 29, 2021 11:05 AM
> To: Cao, Bingbu <bingbu.cao@intel.com>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>; linux-
> media@vger.kernel.org; kieran.bingham@ideasonboard.com;
> bingbu.cao@linux.intel.com
> Subject: Re: [PATCH v3] media: imx258: add vblank control to support
> more frame rate range
> 
> On Fri, Oct 29, 2021 at 11:18 AM Cao, Bingbu <bingbu.cao@intel.com>
> wrote:
> >
> > Sakari and Tomasz,
> >
> > Thanks for your review.
> >
> > ________________________
> > BRs,
> > Bingbu Cao
> >
> > > -----Original Message-----
> > > From: Tomasz Figa <tfiga@chromium.org>
> > > Sent: Thursday, October 28, 2021 9:52 PM
> > > To: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > Cc: Cao, Bingbu <bingbu.cao@intel.com>; linux-media@vger.kernel.org;
> > > kieran.bingham@ideasonboard.com; bingbu.cao@linux.intel.com
> > > Subject: Re: [PATCH v3] media: imx258: add vblank control to support
> > > more frame rate range
> > >
> > > On Sat, Oct 23, 2021 at 5:38 AM Sakari Ailus
> > > <sakari.ailus@linux.intel.com> wrote:
> > > >
> > > > Hi Bingbu,
> > > >
> > > > On Tue, Oct 19, 2021 at 03:58:41PM +0000, Cao, Bingbu wrote:
> > > > > > -----Original Message-----
> > > > > > From: Cao, Bingbu
> > > > > > Sent: Tuesday, October 19, 2021 11:30 PM
> > > > > > To: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > Cc: linux-media@vger.kernel.org; tfiga@chromium.org;
> > > > > > kieran.bingham@ideasonboard.com; bingbu.cao@linux.intel.com
> > > > > > Subject: RE: [PATCH v3] media: imx258: add vblank control to
> > > > > > support more frame rate range
> > > > > >
> > > > > > Sakari,
> > > > > >
> > > > > > ________________________
> > > > > > BRs,
> > > > > > Bingbu Cao
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > > Sent: Tuesday, October 19, 2021 8:52 PM
> > > > > > > To: Cao, Bingbu <bingbu.cao@intel.com>
> > > > > > > Cc: linux-media@vger.kernel.org; tfiga@chromium.org;
> > > > > > > kieran.bingham@ideasonboard.com; bingbu.cao@linux.intel.com
> > > > > > > Subject: Re: [PATCH v3] media: imx258: add vblank control to
> > > > > > > support more frame rate range
> > > > > > >
> > > > > > > Hi Bingbu,
> > > > > > >
> > > > > > > On Mon, Oct 18, 2021 at 11:26:16AM +0800, Bingbu Cao wrote:
> > > > > > > > Current imx258 driver enable the automatic frame length
> > > > > > > > tracking control by default and did not support VBLANK
> > > > > > > > change, it's always
> > > > > > > working at 30fps.
> > > > > > > > However, in reality we need a wider frame rate range from
> > > > > > > > 15
> > > to 30.
> > > > > > > > This patch disable the automatic frame length tracking
> > > > > > > > control and enable the v4l2 VBLANK control to allow user
> > > > > > > > changing frame rate per
> > > > > > > requirement.
> > > > > > > >
> > > > > > > > Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
> > > > > > > > ---
> > > > > > > >  drivers/media/i2c/imx258.c | 23 +++++++++++++++++------
> > > > > > > >  1 file changed, 17 insertions(+), 6 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/media/i2c/imx258.c
> > > > > > > > b/drivers/media/i2c/imx258.c index
> > > > > > > > 81cdf37216ca..2c787af7074d
> > > > > > > > 100644
> > > > > > > > --- a/drivers/media/i2c/imx258.c
> > > > > > > > +++ b/drivers/media/i2c/imx258.c
> > > > > > > > @@ -29,6 +29,7 @@
> > > > > > > >  #define IMX258_VTS_MAX                 0xffff
> > > > > > > >
> > > > > > > >  /*Frame Length Line*/
> > > > > > > > +#define IMX258_REG_FLL                 0x0340
> > > > > > > >  #define IMX258_FLL_MIN                 0x08a6
> > > > > > > >  #define IMX258_FLL_MAX                 0xffff
> > > > > > > >  #define IMX258_FLL_STEP                        1
> > > > > > > > @@ -241,7 +242,7 @@ static const struct imx258_reg
> > > > > > > > mode_4208x3118_regs[]
> > > > > > > = {
> > > > > > > >         { 0x034D, 0x70 },
> > > > > > > >         { 0x034E, 0x0C },
> > > > > > > >         { 0x034F, 0x30 },
> > > > > > > > -       { 0x0350, 0x01 },
> > > > > > > > +       { 0x0350, 0x00 }, /* no frame length automatic
> > > > > > > > + tracking control */
> > > > > > > >         { 0x0202, 0x0C },
> > > > > > > >         { 0x0203, 0x46 },
> > > > > > > >         { 0x0204, 0x00 },
> > > > > > > > @@ -360,7 +361,7 @@ static const struct imx258_reg
> > > > > > > > mode_2104_1560_regs[]
> > > > > > > = {
> > > > > > > >         { 0x034D, 0x38 },
> > > > > > > >         { 0x034E, 0x06 },
> > > > > > > >         { 0x034F, 0x18 },
> > > > > > > > -       { 0x0350, 0x01 },
> > > > > > > > +       { 0x0350, 0x00 }, /* no frame length automatic
> > > > > > > > + tracking control */
> > > > > > > >         { 0x0202, 0x06 },
> > > > > > > >         { 0x0203, 0x2E },
> > > > > > > >         { 0x0204, 0x00 },
> > > > > > > > @@ -479,7 +480,7 @@ static const struct imx258_reg
> > > > > > > > mode_1048_780_regs[]
> > > > > > > = {
> > > > > > > >         { 0x034D, 0x18 },
> > > > > > > >         { 0x034E, 0x03 },
> > > > > > > >         { 0x034F, 0x0C },
> > > > > > > > -       { 0x0350, 0x01 },
> > > > > > > > +       { 0x0350, 0x00 }, /* no frame length automatic
> > > > > > > > + tracking control */
> > > > > > >
> > > > > > > Why is automatic frame length control disabled?
> > > > > >
> > > > > > My understanding:
> > > > > > If automatic frame length control enabled, the frame length is
> > > > > > changed automatically when COARSE_INTEGRATE_TIME + 10 >
> > > > > > FRAME_LENGTH_LINES, it may not meet the requirement - less
> > > integrate time with more frame length.
> > > > > > we need control the vertical blank to do that.
> > > > > >
> > > > >
> > > > > If frame length automatic tracking control enabled, the
> > > > > CORSE_INTEGRATE_TIME could be larger than FRAME_LENGTH_LINES.
> > > >
> > > > Both are controlled by the driver. The driver is generally
> > > > responsible for ensuring the exposure time stays within the limits
> > > > for a given frame length.
> > > >
> > > > Unless this sensor does something weird, all you get by disabling
> > > > this is undefined behaviour instead of increased frame length when
> > > > the exposure time + margin exceeds frame length. This could mean
> > > > broken
> > > frames.
> > > >
> > > > Of course, it takes a driver bug to arrive into this situation.
> > >
> > > I'd argue that enabling the automatic control would make it much
> > > more difficult to spot the driver bug in this case and so it would
> > > be more desirable to keep it disabled as in this patch.
> >
> > You are right, I will remove the change in next version.
> 
> Sorry, remove what change? My comment agrees with this patch that keeps
> the function disabled.

Tomasz, my bad, let me keep it disabled. Sakari, what do you think?


> 
> Best regards,
> Tomasz
Sakari Ailus Nov. 2, 2021, 10:58 p.m. UTC | #9
Hi Bingbu, Tomasz,

On Fri, Oct 29, 2021 at 05:43:04AM +0000, Cao, Bingbu wrote:
> > -----Original Message-----
> > From: Tomasz Figa <tfiga@chromium.org>
> > Sent: Friday, October 29, 2021 11:05 AM
> > To: Cao, Bingbu <bingbu.cao@intel.com>
> > Cc: Sakari Ailus <sakari.ailus@linux.intel.com>; linux-
> > media@vger.kernel.org; kieran.bingham@ideasonboard.com;
> > bingbu.cao@linux.intel.com
> > Subject: Re: [PATCH v3] media: imx258: add vblank control to support
> > more frame rate range
> > 
> > On Fri, Oct 29, 2021 at 11:18 AM Cao, Bingbu <bingbu.cao@intel.com>
> > wrote:
> > >
> > > Sakari and Tomasz,
> > >
> > > Thanks for your review.
> > >
> > > ________________________
> > > BRs,
> > > Bingbu Cao
> > >
> > > > -----Original Message-----
> > > > From: Tomasz Figa <tfiga@chromium.org>
> > > > Sent: Thursday, October 28, 2021 9:52 PM
> > > > To: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > Cc: Cao, Bingbu <bingbu.cao@intel.com>; linux-media@vger.kernel.org;
> > > > kieran.bingham@ideasonboard.com; bingbu.cao@linux.intel.com
> > > > Subject: Re: [PATCH v3] media: imx258: add vblank control to support
> > > > more frame rate range
> > > >
> > > > On Sat, Oct 23, 2021 at 5:38 AM Sakari Ailus
> > > > <sakari.ailus@linux.intel.com> wrote:
> > > > >
> > > > > Hi Bingbu,
> > > > >
> > > > > On Tue, Oct 19, 2021 at 03:58:41PM +0000, Cao, Bingbu wrote:
> > > > > > > -----Original Message-----
> > > > > > > From: Cao, Bingbu
> > > > > > > Sent: Tuesday, October 19, 2021 11:30 PM
> > > > > > > To: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > > Cc: linux-media@vger.kernel.org; tfiga@chromium.org;
> > > > > > > kieran.bingham@ideasonboard.com; bingbu.cao@linux.intel.com
> > > > > > > Subject: RE: [PATCH v3] media: imx258: add vblank control to
> > > > > > > support more frame rate range
> > > > > > >
> > > > > > > Sakari,
> > > > > > >
> > > > > > > ________________________
> > > > > > > BRs,
> > > > > > > Bingbu Cao
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > > > Sent: Tuesday, October 19, 2021 8:52 PM
> > > > > > > > To: Cao, Bingbu <bingbu.cao@intel.com>
> > > > > > > > Cc: linux-media@vger.kernel.org; tfiga@chromium.org;
> > > > > > > > kieran.bingham@ideasonboard.com; bingbu.cao@linux.intel.com
> > > > > > > > Subject: Re: [PATCH v3] media: imx258: add vblank control to
> > > > > > > > support more frame rate range
> > > > > > > >
> > > > > > > > Hi Bingbu,
> > > > > > > >
> > > > > > > > On Mon, Oct 18, 2021 at 11:26:16AM +0800, Bingbu Cao wrote:
> > > > > > > > > Current imx258 driver enable the automatic frame length
> > > > > > > > > tracking control by default and did not support VBLANK
> > > > > > > > > change, it's always
> > > > > > > > working at 30fps.
> > > > > > > > > However, in reality we need a wider frame rate range from
> > > > > > > > > 15
> > > > to 30.
> > > > > > > > > This patch disable the automatic frame length tracking
> > > > > > > > > control and enable the v4l2 VBLANK control to allow user
> > > > > > > > > changing frame rate per
> > > > > > > > requirement.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
> > > > > > > > > ---
> > > > > > > > >  drivers/media/i2c/imx258.c | 23 +++++++++++++++++------
> > > > > > > > >  1 file changed, 17 insertions(+), 6 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/media/i2c/imx258.c
> > > > > > > > > b/drivers/media/i2c/imx258.c index
> > > > > > > > > 81cdf37216ca..2c787af7074d
> > > > > > > > > 100644
> > > > > > > > > --- a/drivers/media/i2c/imx258.c
> > > > > > > > > +++ b/drivers/media/i2c/imx258.c
> > > > > > > > > @@ -29,6 +29,7 @@
> > > > > > > > >  #define IMX258_VTS_MAX                 0xffff
> > > > > > > > >
> > > > > > > > >  /*Frame Length Line*/
> > > > > > > > > +#define IMX258_REG_FLL                 0x0340
> > > > > > > > >  #define IMX258_FLL_MIN                 0x08a6
> > > > > > > > >  #define IMX258_FLL_MAX                 0xffff
> > > > > > > > >  #define IMX258_FLL_STEP                        1
> > > > > > > > > @@ -241,7 +242,7 @@ static const struct imx258_reg
> > > > > > > > > mode_4208x3118_regs[]
> > > > > > > > = {
> > > > > > > > >         { 0x034D, 0x70 },
> > > > > > > > >         { 0x034E, 0x0C },
> > > > > > > > >         { 0x034F, 0x30 },
> > > > > > > > > -       { 0x0350, 0x01 },
> > > > > > > > > +       { 0x0350, 0x00 }, /* no frame length automatic
> > > > > > > > > + tracking control */
> > > > > > > > >         { 0x0202, 0x0C },
> > > > > > > > >         { 0x0203, 0x46 },
> > > > > > > > >         { 0x0204, 0x00 },
> > > > > > > > > @@ -360,7 +361,7 @@ static const struct imx258_reg
> > > > > > > > > mode_2104_1560_regs[]
> > > > > > > > = {
> > > > > > > > >         { 0x034D, 0x38 },
> > > > > > > > >         { 0x034E, 0x06 },
> > > > > > > > >         { 0x034F, 0x18 },
> > > > > > > > > -       { 0x0350, 0x01 },
> > > > > > > > > +       { 0x0350, 0x00 }, /* no frame length automatic
> > > > > > > > > + tracking control */
> > > > > > > > >         { 0x0202, 0x06 },
> > > > > > > > >         { 0x0203, 0x2E },
> > > > > > > > >         { 0x0204, 0x00 },
> > > > > > > > > @@ -479,7 +480,7 @@ static const struct imx258_reg
> > > > > > > > > mode_1048_780_regs[]
> > > > > > > > = {
> > > > > > > > >         { 0x034D, 0x18 },
> > > > > > > > >         { 0x034E, 0x03 },
> > > > > > > > >         { 0x034F, 0x0C },
> > > > > > > > > -       { 0x0350, 0x01 },
> > > > > > > > > +       { 0x0350, 0x00 }, /* no frame length automatic
> > > > > > > > > + tracking control */
> > > > > > > >
> > > > > > > > Why is automatic frame length control disabled?
> > > > > > >
> > > > > > > My understanding:
> > > > > > > If automatic frame length control enabled, the frame length is
> > > > > > > changed automatically when COARSE_INTEGRATE_TIME + 10 >
> > > > > > > FRAME_LENGTH_LINES, it may not meet the requirement - less
> > > > integrate time with more frame length.
> > > > > > > we need control the vertical blank to do that.
> > > > > > >
> > > > > >
> > > > > > If frame length automatic tracking control enabled, the
> > > > > > CORSE_INTEGRATE_TIME could be larger than FRAME_LENGTH_LINES.
> > > > >
> > > > > Both are controlled by the driver. The driver is generally
> > > > > responsible for ensuring the exposure time stays within the limits
> > > > > for a given frame length.
> > > > >
> > > > > Unless this sensor does something weird, all you get by disabling
> > > > > this is undefined behaviour instead of increased frame length when
> > > > > the exposure time + margin exceeds frame length. This could mean
> > > > > broken
> > > > frames.
> > > > >
> > > > > Of course, it takes a driver bug to arrive into this situation.
> > > >
> > > > I'd argue that enabling the automatic control would make it much
> > > > more difficult to spot the driver bug in this case and so it would
> > > > be more desirable to keep it disabled as in this patch.
> > >
> > > You are right, I will remove the change in next version.
> > 
> > Sorry, remove what change? My comment agrees with this patch that keeps
> > the function disabled.
> 
> Tomasz, my bad, let me keep it disabled. Sakari, what do you think?

I guess this is balancing between failing graciously and making bugs
well visible. As this isn't about debugging I'd pick the first. There are
other ways to figure out if the frame time is wrong.

If the current is to be changed I think it should be a separate patch in
any case.
diff mbox series

Patch

diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
index 81cdf37216ca..2c787af7074d 100644
--- a/drivers/media/i2c/imx258.c
+++ b/drivers/media/i2c/imx258.c
@@ -29,6 +29,7 @@ 
 #define IMX258_VTS_MAX			0xffff
 
 /*Frame Length Line*/
+#define IMX258_REG_FLL			0x0340
 #define IMX258_FLL_MIN			0x08a6
 #define IMX258_FLL_MAX			0xffff
 #define IMX258_FLL_STEP			1
@@ -241,7 +242,7 @@  static const struct imx258_reg mode_4208x3118_regs[] = {
 	{ 0x034D, 0x70 },
 	{ 0x034E, 0x0C },
 	{ 0x034F, 0x30 },
-	{ 0x0350, 0x01 },
+	{ 0x0350, 0x00 }, /* no frame length automatic tracking control */
 	{ 0x0202, 0x0C },
 	{ 0x0203, 0x46 },
 	{ 0x0204, 0x00 },
@@ -360,7 +361,7 @@  static const struct imx258_reg mode_2104_1560_regs[] = {
 	{ 0x034D, 0x38 },
 	{ 0x034E, 0x06 },
 	{ 0x034F, 0x18 },
-	{ 0x0350, 0x01 },
+	{ 0x0350, 0x00 }, /* no frame length automatic tracking control */
 	{ 0x0202, 0x06 },
 	{ 0x0203, 0x2E },
 	{ 0x0204, 0x00 },
@@ -479,7 +480,7 @@  static const struct imx258_reg mode_1048_780_regs[] = {
 	{ 0x034D, 0x18 },
 	{ 0x034E, 0x03 },
 	{ 0x034F, 0x0C },
-	{ 0x0350, 0x01 },
+	{ 0x0350, 0x00 }, /* no frame length automatic tracking control */
 	{ 0x0202, 0x03 },
 	{ 0x0203, 0x42 },
 	{ 0x0204, 0x00 },
@@ -753,8 +754,17 @@  static int imx258_set_ctrl(struct v4l2_ctrl *ctrl)
 	struct imx258 *imx258 =
 		container_of(ctrl->handler, struct imx258, ctrl_handler);
 	struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd);
+	s64 max;
 	int ret = 0;
 
+	if (ctrl->id == V4L2_CID_VBLANK) {
+		/* Update max exposure to meet expected vblanking */
+		max = imx258->cur_mode->height + ctrl->val - 10;
+		__v4l2_ctrl_modify_range(imx258->exposure,
+					 imx258->exposure->minimum,
+					 max, imx258->exposure->step, max);
+	}
+
 	/*
 	 * Applying V4L2 control value only happens
 	 * when power is up for streaming
@@ -773,6 +783,10 @@  static int imx258_set_ctrl(struct v4l2_ctrl *ctrl)
 				IMX258_REG_VALUE_16BIT,
 				ctrl->val);
 		break;
+	case V4L2_CID_VBLANK:
+		ret = imx258_write_reg(imx258, IMX258_REG_FLL, 2,
+				       imx258->cur_mode->height + ctrl->val);
+		break;
 	case V4L2_CID_DIGITAL_GAIN:
 		ret = imx258_update_digital_gain(imx258, IMX258_REG_VALUE_16BIT,
 				ctrl->val);
@@ -1189,9 +1203,6 @@  static int imx258_init_controls(struct imx258 *imx258)
 				IMX258_VTS_MAX - imx258->cur_mode->height, 1,
 				vblank_def);
 
-	if (imx258->vblank)
-		imx258->vblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
-
 	imx258->hblank = v4l2_ctrl_new_std(
 				ctrl_hdlr, &imx258_ctrl_ops, V4L2_CID_HBLANK,
 				IMX258_PPL_DEFAULT - imx258->cur_mode->width,