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