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
[off-list ref] wrote:

quoted

Hi Bingbu,

On Tue, Oct 19, 2021 at 03:58:41PM +0000, Cao, Bingbu wrote:

quoted

quoted

-----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

quoted

-----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:

quoted

Current imx258 driver enable the automatic frame length
tracking control by default and did not support VBLANK change,
it's always

working at 30fps.

quoted

However, in reality we need a wider frame rate range from 15

to 30.

quoted

quoted

quoted

quoted

quoted

This patch disable the automatic frame length tracking control
and enable the v4l2 VBLANK control to allow user changing
frame rate per

requirement.

quoted

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[]

= {

quoted

        { 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[]

= {

quoted

        { 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[]

= {

quoted

        { 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.

quoted

quoted

quoted

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.

quoted

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.

Best regards,
Tomasz