Hi Jacopo
> -----Original Message-----
> From: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> Sent: 2023年6月9日 22:14
> To: G.N. Zhou (OSS) <guoniu.zhou@xxxxxxxxxxx>
> Cc: linux-media@xxxxxxxxxxxxxxx; jacopo.mondi@xxxxxxxxxxxxxxxx;
> sakari.ailus@xxxxxxxxxxxxxxx; mchehab@xxxxxxxxxx; slongerbeam@xxxxxxxxx;
> laurent.pinchart@xxxxxxxxxxxxxxxx; jacopo@xxxxxxxxxx
> 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@xxxxxxxxxxx wrote:
> > From: "Guoniu.zhou" <guoniu.zhou@xxxxxxx>
> >
> > 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@xxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxx>
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
> >
