diff mbox series

[v2] media: ov5640: fix low resolution image abnormal issue

Message ID 20230609054525.4067113-1-guoniu.zhou@oss.nxp.com (mailing list archive)
State Superseded
Delegated to: Sakari Ailus
Headers
Series [v2] media: ov5640: fix low resolution image abnormal issue |

Commit Message

G.N. Zhou (OSS) June 9, 2023, 5:45 a.m. UTC 
  From: "Guoniu.zhou" <guoniu.zhou@nxp.com>

OV5640 will output abnormal image data when work at low resolution
(320x240, 176x144 and 160x120) after switching from high resolution,
such as 1080P, the time interval between high and low switching must
be less than 1000ms in order to OV5640 don't enter suspend state during
the time.

The reason is by 0x3824 value don't restore to initialize value when
do resolution switching. In high resolution setting array, 0x3824 is
set to 0x04, but low resolution setting array remove 0x3824 in commit
db15c1957a2d ("media: ov5640: Remove duplicated mode settings"). So
when do resolution switching from high to low, such as 1080P to 320x240,
and the time interval is less than auto suspend delay time which means
global initialize setting array will not be loaded, the output image
data are abnormal. Hence move 0x3824 from ov5640_init_setting[] table
to ov5640_setting_low_res[] table and also move 0x4407 0x460b, 0x460c
to avoid same issue.

Signed-off-by: Guoniu.zhou <guoniu.zhou@nxp.com>
---
v1->v2:
  1) Move 0x4407, 0x460b, 0x460c from ov5640_init_setting[] table to
     ov5640_setting_low_res[] table.
  2) Add mode switching test from 2592x1944 to other resolutions, the
     result is okay.
---
 drivers/media/i2c/ov5640.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Jacopo Mondi June 9, 2023, 2:14 p.m. UTC | #1 
Hello Guoniu Zhou

On Fri, Jun 09, 2023 at 01:45:25PM +0800, guoniu.zhou@oss.nxp.com wrote:
> From: "Guoniu.zhou" <guoniu.zhou@nxp.com>
>
> OV5640 will output abnormal image data when work at low resolution
> (320x240, 176x144 and 160x120) after switching from high resolution,
> such as 1080P, the time interval between high and low switching must
> be less than 1000ms in order to OV5640 don't enter suspend state during
> the time.
>
> The reason is by 0x3824 value don't restore to initialize value when
> do resolution switching. In high resolution setting array, 0x3824 is
> set to 0x04, but low resolution setting array remove 0x3824 in commit
> db15c1957a2d ("media: ov5640: Remove duplicated mode settings"). So
> when do resolution switching from high to low, such as 1080P to 320x240,
> and the time interval is less than auto suspend delay time which means
> global initialize setting array will not be loaded, the output image
> data are abnormal. Hence move 0x3824 from ov5640_init_setting[] table
> to ov5640_setting_low_res[] table and also move 0x4407 0x460b, 0x460c
> to avoid same issue.

I'm not 100% I got why the autosuspend delay plays a role here.

Also this is probably worth a:
Fixes: db15c1957a2d ("media: ov5640: Remove duplicated mode settings")

Could be added when applying probably ?
>
> Signed-off-by: Guoniu.zhou <guoniu.zhou@nxp.com>
> ---
> v1->v2:
>   1) Move 0x4407, 0x460b, 0x460c from ov5640_init_setting[] table to
>      ov5640_setting_low_res[] table.

I have checked that the registers you removed from init_settings[] are
programmed in all the other modes, hence

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>


>   2) Add mode switching test from 2592x1944 to other resolutions, the
>      result is okay.

Thanks for testing it and good catch!


> ---
>  drivers/media/i2c/ov5640.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 1536649b9e90..1bc4d72a906e 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -568,9 +568,7 @@ static const struct reg_value ov5640_init_setting[] = {
>  	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x3000, 0x00, 0, 0},
>  	{0x3002, 0x1c, 0, 0}, {0x3004, 0xff, 0, 0}, {0x3006, 0xc3, 0, 0},
>  	{0x302e, 0x08, 0, 0}, {0x4300, 0x3f, 0, 0},
> -	{0x501f, 0x00, 0, 0}, {0x4407, 0x04, 0, 0},
> -	{0x440e, 0x00, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
> -	{0x4837, 0x0a, 0, 0}, {0x3824, 0x02, 0, 0},
> +	{0x501f, 0x00, 0, 0}, {0x440e, 0x00, 0, 0}, {0x4837, 0x0a, 0, 0},
>  	{0x5000, 0xa7, 0, 0}, {0x5001, 0xa3, 0, 0}, {0x5180, 0xff, 0, 0},
>  	{0x5181, 0xf2, 0, 0}, {0x5182, 0x00, 0, 0}, {0x5183, 0x14, 0, 0},
>  	{0x5184, 0x25, 0, 0}, {0x5185, 0x24, 0, 0}, {0x5186, 0x09, 0, 0},
> @@ -634,7 +632,8 @@ static const struct reg_value ov5640_setting_low_res[] = {
>  	{0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
>  	{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
>  	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0},
> -	{0x4407, 0x04, 0, 0}, {0x5001, 0xa3, 0, 0},
> +	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
> +	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
>  };
>
>  static const struct reg_value ov5640_setting_720P_1280_720[] = {
> --
> 2.37.1
>
G.N. Zhou (OSS) June 12, 2023, 2:21 a.m. UTC | #2 
Hi Jacopo

> -----Original Message-----
> From: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Sent: 2023年6月9日 22:14
> To: G.N. Zhou (OSS) <guoniu.zhou@oss.nxp.com>
> Cc: linux-media@vger.kernel.org; jacopo.mondi@ideasonboard.com;
> sakari.ailus@linux.intel.com; mchehab@kernel.org; slongerbeam@gmail.com;
> laurent.pinchart@ideasonboard.com; jacopo@jmondi.org
> Subject: Re: [PATCH v2] media: ov5640: fix low resolution image abnormal issue
> 
> Caution: This is an external email. Please take care when clicking links or opening
> attachments. When in doubt, report the message using the 'Report this email'
> button
> 
> 
> Hello Guoniu Zhou
> 
> On Fri, Jun 09, 2023 at 01:45:25PM +0800, guoniu.zhou@oss.nxp.com wrote:
> > From: "Guoniu.zhou" <guoniu.zhou@nxp.com>
> >
> > OV5640 will output abnormal image data when work at low resolution
> > (320x240, 176x144 and 160x120) after switching from high resolution,
> > such as 1080P, the time interval between high and low switching must
> > be less than 1000ms in order to OV5640 don't enter suspend state
> > during the time.
> >
> > The reason is by 0x3824 value don't restore to initialize value when
> > do resolution switching. In high resolution setting array, 0x3824 is
> > set to 0x04, but low resolution setting array remove 0x3824 in commit
> > db15c1957a2d ("media: ov5640: Remove duplicated mode settings"). So
> > when do resolution switching from high to low, such as 1080P to
> > 320x240, and the time interval is less than auto suspend delay time
> > which means global initialize setting array will not be loaded, the
> > output image data are abnormal. Hence move 0x3824 from
> > ov5640_init_setting[] table to ov5640_setting_low_res[] table and also
> > move 0x4407 0x460b, 0x460c to avoid same issue.
> 
> I'm not 100% I got why the autosuspend delay plays a role here.

When each resolution switch, it will call ov5640_s_stream() each time and the call sequence which related to autosuspend delay as bellow:
1) when stream stop
    if mode switching time interval is less than 1000ms
       ov5640_sensor_suspend() will not be called
    else
       ov5640 will call " ov5640_sensor_suspend()"
2) when next time start streaming, call ov5640_s_stream()
    if time interval is less than 1000ms
       ov5640 setting will keep as last time and only change register in ov5640_setting_low_res[]
    else
       ov5640_s_stream()
         pm_runtime_resume_and_get()
            ov5640_sensor_resume()
              ov5640_set_power()
                ov5640_restore_mode()
                  ov5640_load_regs(sensor, ov5640_init_setting, ARRAY_SIZE(ov5640_init_setting));
                  ov5640_set_mode(sensor)
                  ....
I'm not sure if I make it clear.

> 
> Also this is probably worth a:
> Fixes: db15c1957a2d ("media: ov5640: Remove duplicated mode settings")
> 
> Could be added when applying probably ?

Sure, I will update

> >
> > Signed-off-by: Guoniu.zhou <guoniu.zhou@nxp.com>
> > ---
> > v1->v2:
> >   1) Move 0x4407, 0x460b, 0x460c from ov5640_init_setting[] table to
> >      ov5640_setting_low_res[] table.
> 
> I have checked that the registers you removed from init_settings[] are
> programmed in all the other modes, hence
> 
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thank you again.

> 
> 
> >   2) Add mode switching test from 2592x1944 to other resolutions, the
> >      result is okay.
> 
> Thanks for testing it and good catch!
> 
> 
> > ---
> >  drivers/media/i2c/ov5640.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index 1536649b9e90..1bc4d72a906e 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -568,9 +568,7 @@ static const struct reg_value ov5640_init_setting[] = {
> >       {0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x3000, 0x00, 0, 0},
> >       {0x3002, 0x1c, 0, 0}, {0x3004, 0xff, 0, 0}, {0x3006, 0xc3, 0, 0},
> >       {0x302e, 0x08, 0, 0}, {0x4300, 0x3f, 0, 0},
> > -     {0x501f, 0x00, 0, 0}, {0x4407, 0x04, 0, 0},
> > -     {0x440e, 0x00, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
> > -     {0x4837, 0x0a, 0, 0}, {0x3824, 0x02, 0, 0},
> > +     {0x501f, 0x00, 0, 0}, {0x440e, 0x00, 0, 0}, {0x4837, 0x0a, 0,
> > + 0},
> >       {0x5000, 0xa7, 0, 0}, {0x5001, 0xa3, 0, 0}, {0x5180, 0xff, 0, 0},
> >       {0x5181, 0xf2, 0, 0}, {0x5182, 0x00, 0, 0}, {0x5183, 0x14, 0, 0},
> >       {0x5184, 0x25, 0, 0}, {0x5185, 0x24, 0, 0}, {0x5186, 0x09, 0,
> > 0}, @@ -634,7 +632,8 @@ static const struct reg_value
> ov5640_setting_low_res[] = {
> >       {0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
> >       {0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
> >       {0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0},
> > -     {0x4407, 0x04, 0, 0}, {0x5001, 0xa3, 0, 0},
> > +     {0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
> > +     {0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
> >  };
> >
> >  static const struct reg_value ov5640_setting_720P_1280_720[] = {
> > --
> > 2.37.1
> >
diff mbox series

Patch

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 1536649b9e90..1bc4d72a906e 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -568,9 +568,7 @@  static const struct reg_value ov5640_init_setting[] = {
 	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x3000, 0x00, 0, 0},
 	{0x3002, 0x1c, 0, 0}, {0x3004, 0xff, 0, 0}, {0x3006, 0xc3, 0, 0},
 	{0x302e, 0x08, 0, 0}, {0x4300, 0x3f, 0, 0},
-	{0x501f, 0x00, 0, 0}, {0x4407, 0x04, 0, 0},
-	{0x440e, 0x00, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
-	{0x4837, 0x0a, 0, 0}, {0x3824, 0x02, 0, 0},
+	{0x501f, 0x00, 0, 0}, {0x440e, 0x00, 0, 0}, {0x4837, 0x0a, 0, 0},
 	{0x5000, 0xa7, 0, 0}, {0x5001, 0xa3, 0, 0}, {0x5180, 0xff, 0, 0},
 	{0x5181, 0xf2, 0, 0}, {0x5182, 0x00, 0, 0}, {0x5183, 0x14, 0, 0},
 	{0x5184, 0x25, 0, 0}, {0x5185, 0x24, 0, 0}, {0x5186, 0x09, 0, 0},
@@ -634,7 +632,8 @@  static const struct reg_value ov5640_setting_low_res[] = {
 	{0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
 	{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
 	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0},
-	{0x4407, 0x04, 0, 0}, {0x5001, 0xa3, 0, 0},
+	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
+	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
 };
 
 static const struct reg_value ov5640_setting_720P_1280_720[] = {