> -----Original Message-----
> From: Tomasz Figa <tfiga@xxxxxxxxxxxx>
> Sent: Friday, October 29, 2021 11:05 AM
> To: Cao, Bingbu <bingbu.cao@xxxxxxxxx>
> Cc: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>; linux-
> media@xxxxxxxxxxxxxxx; kieran.bingham@xxxxxxxxxxxxxxxx;
> bingbu.cao@xxxxxxxxxxxxxxx
> 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@xxxxxxxxx>
> wrote:
> >
> > Sakari and Tomasz,
> >
> > Thanks for your review.
> >
> > ________________________
> > BRs,
> > Bingbu Cao
> >
> > > -----Original Message-----
> > > From: Tomasz Figa <tfiga@xxxxxxxxxxxx>
> > > Sent: Thursday, October 28, 2021 9:52 PM
> > > To: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > > Cc: Cao, Bingbu <bingbu.cao@xxxxxxxxx>; linux-media@xxxxxxxxxxxxxxx;
> > > kieran.bingham@xxxxxxxxxxxxxxxx; bingbu.cao@xxxxxxxxxxxxxxx
> > > 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@xxxxxxxxxxxxxxx> 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@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.
> > > >
> > > > 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
