* [PATCH v3] media: imx258: add vblank control to support more frame rate range
@ 2021-10-18 3:26 Bingbu Cao
2021-10-19 12:51 ` Sakari Ailus
0 siblings, 1 reply; 10+ messages in thread
From: Bingbu Cao @ 2021-10-18 3:26 UTC (permalink / raw)
To: linux-media, sakari.ailus, tfiga, kieran.bingham; +Cc: bingbu.cao, bingbu.cao
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 */
{ 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,
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3] media: imx258: add vblank control to support more frame rate range
2021-10-18 3:26 [PATCH v3] media: imx258: add vblank control to support more frame rate range Bingbu Cao
@ 2021-10-19 12:51 ` Sakari Ailus
2021-10-19 15:30 ` Cao, Bingbu
0 siblings, 1 reply; 10+ messages in thread
From: Sakari Ailus @ 2021-10-19 12:51 UTC (permalink / raw)
To: Bingbu Cao; +Cc: linux-media, tfiga, kieran.bingham, bingbu.cao
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,
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v3] media: imx258: add vblank control to support more frame rate range
2021-10-19 12:51 ` Sakari Ailus
@ 2021-10-19 15:30 ` Cao, Bingbu
2021-10-19 15:58 ` Cao, Bingbu
0 siblings, 1 reply; 10+ messages in thread
From: Cao, Bingbu @ 2021-10-19 15:30 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media, tfiga, kieran.bingham, bingbu.cao
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.
>
> > { 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v3] media: imx258: add vblank control to support more frame rate range
2021-10-19 15:30 ` Cao, Bingbu
@ 2021-10-19 15:58 ` Cao, Bingbu
2021-10-22 20:37 ` Sakari Ailus
0 siblings, 1 reply; 10+ messages in thread
From: Cao, Bingbu @ 2021-10-19 15:58 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media, tfiga, kieran.bingham, bingbu.cao
> -----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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] media: imx258: add vblank control to support more frame rate range
2021-10-19 15:58 ` Cao, Bingbu
@ 2021-10-22 20:37 ` Sakari Ailus
2021-10-28 13:52 ` Tomasz Figa
0 siblings, 1 reply; 10+ messages in thread
From: Sakari Ailus @ 2021-10-22 20:37 UTC (permalink / raw)
To: Cao, Bingbu; +Cc: linux-media, tfiga, kieran.bingham, bingbu.cao
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.
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] media: imx258: add vblank control to support more frame rate range
2021-10-22 20:37 ` Sakari Ailus
@ 2021-10-28 13:52 ` Tomasz Figa
2021-10-29 2:18 ` Cao, Bingbu
0 siblings, 1 reply; 10+ messages in thread
From: Tomasz Figa @ 2021-10-28 13:52 UTC (permalink / raw)
To: Sakari Ailus; +Cc: Cao, Bingbu, linux-media, kieran.bingham, bingbu.cao
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v3] media: imx258: add vblank control to support more frame rate range
2021-10-28 13:52 ` Tomasz Figa
@ 2021-10-29 2:18 ` Cao, Bingbu
2021-10-29 3:05 ` Tomasz Figa
0 siblings, 1 reply; 10+ messages in thread
From: Cao, Bingbu @ 2021-10-29 2:18 UTC (permalink / raw)
To: Tomasz Figa, Sakari Ailus; +Cc: linux-media, kieran.bingham, bingbu.cao
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.
>
> Best regards,
> Tomasz
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] media: imx258: add vblank control to support more frame rate range
2021-10-29 2:18 ` Cao, Bingbu
@ 2021-10-29 3:05 ` Tomasz Figa
2021-10-29 5:43 ` Cao, Bingbu
0 siblings, 1 reply; 10+ messages in thread
From: Tomasz Figa @ 2021-10-29 3:05 UTC (permalink / raw)
To: Cao, Bingbu; +Cc: Sakari Ailus, linux-media, kieran.bingham, bingbu.cao
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v3] media: imx258: add vblank control to support more frame rate range
2021-10-29 3:05 ` Tomasz Figa
@ 2021-10-29 5:43 ` Cao, Bingbu
2021-11-02 22:58 ` Sakari Ailus
0 siblings, 1 reply; 10+ messages in thread
From: Cao, Bingbu @ 2021-10-29 5:43 UTC (permalink / raw)
To: Tomasz Figa; +Cc: Sakari Ailus, linux-media, kieran.bingham, bingbu.cao
> -----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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] media: imx258: add vblank control to support more frame rate range
2021-10-29 5:43 ` Cao, Bingbu
@ 2021-11-02 22:58 ` Sakari Ailus
0 siblings, 0 replies; 10+ messages in thread
From: Sakari Ailus @ 2021-11-02 22:58 UTC (permalink / raw)
To: Cao, Bingbu; +Cc: Tomasz Figa, linux-media, kieran.bingham, bingbu.cao
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.
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-11-02 22:59 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-18 3:26 [PATCH v3] media: imx258: add vblank control to support more frame rate range Bingbu Cao
2021-10-19 12:51 ` Sakari Ailus
2021-10-19 15:30 ` Cao, Bingbu
2021-10-19 15:58 ` Cao, Bingbu
2021-10-22 20:37 ` Sakari Ailus
2021-10-28 13:52 ` Tomasz Figa
2021-10-29 2:18 ` Cao, Bingbu
2021-10-29 3:05 ` Tomasz Figa
2021-10-29 5:43 ` Cao, Bingbu
2021-11-02 22:58 ` Sakari Ailus
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.