| Message ID | 1634527576-2928-1-git-send-email-bingbu.cao@intel.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Sakari Ailus |
| Headers |
Received: from vger.kernel.org ([23.128.96.18])
by www.linuxtv.org with esmtp (Exim 4.92)
(envelope-from <linux-media-owner@vger.kernel.org>)
id 1mcJLs-007Afk-4I; Mon, 18 Oct 2021 03:30:16 +0000
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
id S231580AbhJRDcX (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other);
Sun, 17 Oct 2021 23:32:23 -0400
Received: from mga04.intel.com ([192.55.52.120]:31684 "EHLO mga04.intel.com"
rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
id S231552AbhJRDcW (ORCPT <rfc822;linux-media@vger.kernel.org>);
Sun, 17 Oct 2021 23:32:22 -0400
X-IronPort-AV: E=McAfee;i="6200,9189,10140"; a="226929982"
X-IronPort-AV: E=Sophos;i="5.85,381,1624345200";
d="scan'208";a="226929982"
Received: from orsmga006.jf.intel.com ([10.7.209.51])
by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384;
17 Oct 2021 20:30:10 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.85,381,1624345200";
d="scan'208";a="443241784"
Received: from ipu5-build.bj.intel.com ([10.238.232.188])
by orsmga006.jf.intel.com with ESMTP; 17 Oct 2021 20:30:08 -0700
From: Bingbu Cao <bingbu.cao@intel.com>
To: linux-media@vger.kernel.org, sakari.ailus@linux.intel.com,
tfiga@chromium.org, kieran.bingham@ideasonboard.com
Cc: bingbu.cao@intel.com, bingbu.cao@linux.intel.com
Subject: [PATCH v3] media: imx258: add vblank control to support more frame
rate range
Date: Mon, 18 Oct 2021 11:26:16 +0800
Message-Id: <1634527576-2928-1-git-send-email-bingbu.cao@intel.com>
X-Mailer: git-send-email 2.7.4
Precedence: bulk
List-ID: <linux-media.vger.kernel.org>
X-Mailing-List: linux-media@vger.kernel.org
X-LSpam-Score: -4.7 (----)
X-LSpam-Report: No,
score=-4.7 required=5.0 tests=BAYES_00=-1.9,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1,RCVD_IN_DNSWL_MED=-2.3
autolearn=ham autolearn_force=no
|
| Series |
[v3] media: imx258: add vblank control to support more frame rate range
|
|
Commit Message
Cao, Bingbu
Oct. 18, 2021, 3:26 a.m. UTC
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@intel.com>
---
drivers/media/i2c/imx258.c | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)
Comments
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@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[] = { > { 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? > { 0x0202, 0x03 }, > { 0x0203, 0x42 }, > { 0x0204, 0x00 }, > @@ -753,8 +754,17 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl) > struct imx258 *imx258 = > container_of(ctrl->handler, struct imx258, ctrl_handler); > struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd); > + s64 max; > int ret = 0; > > + if (ctrl->id == V4L2_CID_VBLANK) { > + /* Update max exposure to meet expected vblanking */ > + max = imx258->cur_mode->height + ctrl->val - 10; > + __v4l2_ctrl_modify_range(imx258->exposure, > + imx258->exposure->minimum, > + max, imx258->exposure->step, max); > + } > + > /* > * Applying V4L2 control value only happens > * when power is up for streaming > @@ -773,6 +783,10 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl) > IMX258_REG_VALUE_16BIT, > ctrl->val); > break; > + case V4L2_CID_VBLANK: > + ret = imx258_write_reg(imx258, IMX258_REG_FLL, 2, > + imx258->cur_mode->height + ctrl->val); > + break; > case V4L2_CID_DIGITAL_GAIN: > ret = imx258_update_digital_gain(imx258, IMX258_REG_VALUE_16BIT, > ctrl->val); > @@ -1189,9 +1203,6 @@ static int imx258_init_controls(struct imx258 *imx258) > IMX258_VTS_MAX - imx258->cur_mode->height, 1, > vblank_def); > > - if (imx258->vblank) > - imx258->vblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; > - > imx258->hblank = v4l2_ctrl_new_std( > ctrl_hdlr, &imx258_ctrl_ops, V4L2_CID_HBLANK, > IMX258_PPL_DEFAULT - imx258->cur_mode->width,
Sakari,
> -----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 > > > -----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: > > > 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@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[] > > = { > > > { 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. > > > > > > { 0x0202, 0x03 }, > > > { 0x0203, 0x42 }, > > > { 0x0204, 0x00 }, > > > @@ -753,8 +754,17 @@ static int imx258_set_ctrl(struct v4l2_ctrl > *ctrl) > > > struct imx258 *imx258 = > > > container_of(ctrl->handler, struct imx258, ctrl_handler); > > > struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd); > > > + s64 max; > > > int ret = 0; > > > > > > + if (ctrl->id == V4L2_CID_VBLANK) { > > > + /* Update max exposure to meet expected vblanking */ > > > + max = imx258->cur_mode->height + ctrl->val - 10; > > > + __v4l2_ctrl_modify_range(imx258->exposure, > > > + imx258->exposure->minimum, > > > + max, imx258->exposure->step, max); > > > + } > > > + > > > /* > > > * Applying V4L2 control value only happens > > > * when power is up for streaming > > > @@ -773,6 +783,10 @@ static int imx258_set_ctrl(struct v4l2_ctrl > *ctrl) > > > IMX258_REG_VALUE_16BIT, > > > ctrl->val); > > > break; > > > + case V4L2_CID_VBLANK: > > > + ret = imx258_write_reg(imx258, IMX258_REG_FLL, 2, > > > + imx258->cur_mode->height + ctrl->val); > > > + break; > > > case V4L2_CID_DIGITAL_GAIN: > > > ret = imx258_update_digital_gain(imx258, > > IMX258_REG_VALUE_16BIT, > > > ctrl->val); > > > @@ -1189,9 +1203,6 @@ static int imx258_init_controls(struct imx258 > > *imx258) > > > IMX258_VTS_MAX - imx258->cur_mode->height, 1, > > > vblank_def); > > > > > > - if (imx258->vblank) > > > - imx258->vblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > > - > > > imx258->hblank = v4l2_ctrl_new_std( > > > ctrl_hdlr, &imx258_ctrl_ops, V4L2_CID_HBLANK, > > > IMX258_PPL_DEFAULT - imx258->cur_mode->width, > > > > -- > > Kind regards, > > > > Sakari Ailus
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@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 > > > > > -----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: > > > > 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@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[] > > > = { > > > > { 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.
On Sat, Oct 23, 2021 at 5:38 AM Sakari Ailus <sakari.ailus@linux.intel.com> 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@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 > > > > > > > -----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: > > > > > 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@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[] > > > > = { > > > > > { 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
Sakari and Tomasz, Thanks for your review.
On Fri, Oct 29, 2021 at 11:18 AM Cao, Bingbu <bingbu.cao@intel.com> wrote: > > 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 > > <sakari.ailus@linux.intel.com> 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@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 > > > > > > > > > > > -----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: > > > > > > > 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@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[] > > > > > > = { > > > > > > > { 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
> -----Original Message----- > From: Tomasz Figa <tfiga@chromium.org> > Sent: Friday, October 29, 2021 11:05 AM > To: Cao, Bingbu <bingbu.cao@intel.com> > Cc: Sakari Ailus <sakari.ailus@linux.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 Fri, Oct 29, 2021 at 11:18 AM Cao, Bingbu <bingbu.cao@intel.com> > wrote: > > > > 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 > > > <sakari.ailus@linux.intel.com> 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@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 > > > > > > > > > > > > > -----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: > > > > > > > > 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@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[] > > > > > > > = { > > > > > > > > { 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
Hi Bingbu, Tomasz, On Fri, Oct 29, 2021 at 05:43:04AM +0000, Cao, Bingbu wrote: > > -----Original Message----- > > From: Tomasz Figa <tfiga@chromium.org> > > Sent: Friday, October 29, 2021 11:05 AM > > To: Cao, Bingbu <bingbu.cao@intel.com> > > Cc: Sakari Ailus <sakari.ailus@linux.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 Fri, Oct 29, 2021 at 11:18 AM Cao, Bingbu <bingbu.cao@intel.com> > > wrote: > > > > > > 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 > > > > <sakari.ailus@linux.intel.com> 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@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 > > > > > > > > > > > > > > > -----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: > > > > > > > > > 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@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[] > > > > > > > > = { > > > > > > > > > { 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? I guess this is balancing between failing graciously and making bugs well visible. As this isn't about debugging I'd pick the first. There are other ways to figure out if the frame time is wrong. If the current is to be changed I think it should be a separate patch in any case.
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 */ { 0x0202, 0x03 }, { 0x0203, 0x42 }, { 0x0204, 0x00 }, @@ -753,8 +754,17 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl) struct imx258 *imx258 = container_of(ctrl->handler, struct imx258, ctrl_handler); struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd); + s64 max; int ret = 0; + if (ctrl->id == V4L2_CID_VBLANK) { + /* Update max exposure to meet expected vblanking */ + max = imx258->cur_mode->height + ctrl->val - 10; + __v4l2_ctrl_modify_range(imx258->exposure, + imx258->exposure->minimum, + max, imx258->exposure->step, max); + } + /* * Applying V4L2 control value only happens * when power is up for streaming @@ -773,6 +783,10 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl) IMX258_REG_VALUE_16BIT, ctrl->val); break; + case V4L2_CID_VBLANK: + ret = imx258_write_reg(imx258, IMX258_REG_FLL, 2, + imx258->cur_mode->height + ctrl->val); + break; case V4L2_CID_DIGITAL_GAIN: ret = imx258_update_digital_gain(imx258, IMX258_REG_VALUE_16BIT, ctrl->val); @@ -1189,9 +1203,6 @@ static int imx258_init_controls(struct imx258 *imx258) IMX258_VTS_MAX - imx258->cur_mode->height, 1, vblank_def); - if (imx258->vblank) - imx258->vblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; - imx258->hblank = v4l2_ctrl_new_std( ctrl_hdlr, &imx258_ctrl_ops, V4L2_CID_HBLANK, IMX258_PPL_DEFAULT - imx258->cur_mode->width,