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.
Best regards,
Tomasz
