On Sat, Oct 23, 2021 at 5:38 AM Sakari Ailus
[off-list ref] wrote:

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