* [PATCH v2] media: imx258: add vblank control to support more frame rate range
@ 2021-10-15 7:05 Bingbu Cao
2021-10-15 19:49 ` Kieran Bingham
0 siblings, 1 reply; 3+ messages in thread
From: Bingbu Cao @ 2021-10-15 7:05 UTC (permalink / raw)
To: linux-media, sakari.ailus; +Cc: senozhatsky, tfiga, 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(-)
---
v1->v2: remove a wrong 'break'
---
diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
index 81cdf37216ca..3f46744b1a26 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 },
{ 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 },
{ 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 },
{ 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] 3+ messages in thread
* Re: [PATCH v2] media: imx258: add vblank control to support more frame rate range
2021-10-15 7:05 [PATCH v2] media: imx258: add vblank control to support more frame rate range Bingbu Cao
@ 2021-10-15 19:49 ` Kieran Bingham
2021-10-18 2:33 ` Cao, Bingbu
0 siblings, 1 reply; 3+ messages in thread
From: Kieran Bingham @ 2021-10-15 19:49 UTC (permalink / raw)
To: Bingbu Cao, linux-media, sakari.ailus
Cc: senozhatsky, tfiga, bingbu.cao, bingbu.cao
Hi Bingbu,
Quoting Bingbu Cao (2021-10-15 08:05:30)
> 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(-)
> ---
> v1->v2: remove a wrong 'break'
> ---
> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> index 81cdf37216ca..3f46744b1a26 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 },
The commit message implies that the register 0x0350 controls the
"automatic frame length tracking".
Is it worth adding that register description as a comment at the end of
the line, to help future readers?
> + { 0x0350, 0x00 }, /* automatic frame length tracking */
Without datasheets, these long register lists are very terse ...
If register names /functions can at least be identified then I suspect
it would help with future maintenance of the code?
Or is it too futile to imagine that these registers might improve in
documentation as time goes on...
--
Kieran
> { 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 },
> { 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 },
> { 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 [flat|nested] 3+ messages in thread
* RE: [PATCH v2] media: imx258: add vblank control to support more frame rate range
2021-10-15 19:49 ` Kieran Bingham
@ 2021-10-18 2:33 ` Cao, Bingbu
0 siblings, 0 replies; 3+ messages in thread
From: Cao, Bingbu @ 2021-10-18 2:33 UTC (permalink / raw)
To: Kieran Bingham, linux-media, sakari.ailus; +Cc: senozhatsky, tfiga, bingbu.cao
Hi, Bingham
Thanks for your review.
________________________
BRs,
Bingbu Cao
> -----Original Message-----
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Sent: Saturday, October 16, 2021 3:50 AM
> To: Cao, Bingbu <bingbu.cao@intel.com>; linux-media@vger.kernel.org;
> sakari.ailus@linux.intel.com
> Cc: senozhatsky@chromium.org; tfiga@chromium.org; Cao, Bingbu
> <bingbu.cao@intel.com>; bingbu.cao@linux.intel.com
> Subject: Re: [PATCH v2] media: imx258: add vblank control to support
> more frame rate range
>
> Hi Bingbu,
>
> Quoting Bingbu Cao (2021-10-15 08:05:30)
> > 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(-)
> > ---
> > v1->v2: remove a wrong 'break'
> > ---
> > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> > index 81cdf37216ca..3f46744b1a26 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 },
>
> The commit message implies that the register 0x0350 controls the
> "automatic frame length tracking".
>
> Is it worth adding that register description as a comment at the end of
> the line, to help future readers?
Yes, will add later.
>
> > + { 0x0350, 0x00 }, /* automatic frame length tracking */
>
> Without datasheets, these long register lists are very terse ...
>
> If register names /functions can at least be identified then I suspect
> it would help with future maintenance of the code?
>
> Or is it too futile to imagine that these registers might improve in
> documentation as time goes on...
Yes, it could be done in a separate patch to add register descriptions. 😊
>
> --
> Kieran
>
>
> > { 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 },
> > { 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 },
> > { 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 [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-10-18 2:33 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-15 7:05 [PATCH v2] media: imx258: add vblank control to support more frame rate range Bingbu Cao
2021-10-15 19:49 ` Kieran Bingham
2021-10-18 2:33 ` Cao, Bingbu
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.