> -----Original Message-----
> From: Cao, Bingbu
> Sent: Tuesday, October 19, 2021 11:30 PM
> To: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> Cc: linux-media@xxxxxxxxxxxxxxx; tfiga@xxxxxxxxxxxx;
> kieran.bingham@xxxxxxxxxxxxxxxx; bingbu.cao@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx>
> > Sent: Tuesday, October 19, 2021 8:52 PM
> > To: Cao, Bingbu <bingbu.cao@xxxxxxxxx>
> > Cc: linux-media@xxxxxxxxxxxxxxx; tfiga@xxxxxxxxxxxx;
> > kieran.bingham@xxxxxxxxxxxxxxxx; bingbu.cao@xxxxxxxxxxxxxxx
> > 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@xxxxxxxxx>
> > > ---
> > > 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
