Re: [PATCH v3 01/12] media: ov5640: Fix timings setup code

[Maxime Ripard]

Hi Daniel,

On Fri, May 18, 2018 at 10:32:43AM +0200, Daniel Mack wrote:
> On Thursday, May 17, 2018 10:53 AM, Maxime Ripard wrote:
> > From: Samuel Bobrowicz 
> > 
> > The current code, when changing the mode and changing the scaling or
> > sampling parameters, will look at the horizontal and vertical total size,
> > which, since 5999f381e023 ("media: ov5640: Add horizontal and vertical
> > totals") has been moved from the static register initialization to after
> > the mode change.
> > 
> > That means that the values are no longer set up before the code retrieves
> > them, which is obviously a bug.
> 
> I tried 'for-4.18-5' before your patch set now and noticed it didn't work. I
> then bisected the regression down to the same commit that you mentioned
> above.
> 
> The old code (before 5999f381e023) used to have the timing registers
> embedded in a large register blob. Omitting them during the bulk upload and
> writing them later doesn't work here, even if the values are the same. It
> seems that the order in which registers are written matters.
> 
> Hence your patch in this email doesn't fix it for me either. I have to move
> ov5640_set_timings() before ov5640_load_regs() to revive my platform.

That's kind of weird, it definitely works for parallel. Do you have a
bit more details on what doesn't work? What commands / apps are you
running?

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com


signature.asc
Description: PGP signature

Re: [PATCH v3 01/12] media: ov5640: Fix timings setup code

[Daniel Mack]

Hi,

On Thursday, May 17, 2018 10:53 AM, Maxime Ripard wrote:

From: Samuel Bobrowicz 

The current code, when changing the mode and changing the scaling or
sampling parameters, will look at the horizontal and vertical total size,
which, since 5999f381e023 ("media: ov5640: Add horizontal and vertical
totals") has been moved from the static register initialization to after
the mode change.

That means that the values are no longer set up before the code retrieves
them, which is obviously a bug.


I tried 'for-4.18-5' before your patch set now and noticed it didn't 
work. I then bisected the regression down to the same commit that you 
mentioned above.


The old code (before 5999f381e023) used to have the timing registers 
embedded in a large register blob. Omitting them during the bulk upload 
and writing them later doesn't work here, even if the values are the 
same. It seems that the order in which registers are written matters.


Hence your patch in this email doesn't fix it for me either. I have to 
move ov5640_set_timings() before ov5640_load_regs() to revive my platform.


One of the subsequent patches in this series introduces a new regression 
for me, unfortunately, possibly for the same reason. I'll dig a bit more.


What cameras are you testing this with? MIPI or parallel?


Thanks,
Daniel




Fixes: 5999f381e023 ("media: ov5640: Add horizontal and vertical totals")
Signed-off-by: Samuel Bobrowicz 
Signed-off-by: Maxime Ripard 
---
  drivers/media/i2c/ov5640.c | 12 
  1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index e480e53b369b..4bd968b478db 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -1462,6 +1462,10 @@ static int ov5640_set_mode_exposure_calc(struct 
ov5640_dev *sensor,
if (ret < 0)
return ret;
  
+	ret = ov5640_set_timings(sensor, mode);

+   if (ret < 0)
+   return ret;
+
/* read capture VTS */
ret = ov5640_get_vts(sensor);
if (ret < 0)
@@ -1589,6 +1593,10 @@ static int ov5640_set_mode_direct(struct ov5640_dev 
*sensor,
if (ret < 0)
return ret;
  
+	ret = ov5640_set_timings(sensor, mode);

+   if (ret < 0)
+   return ret;
+
/* turn auto gain/exposure back on for direct mode */
ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_gain, 1);
if (ret)
@@ -1633,10 +1641,6 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
ret = ov5640_set_mode_direct(sensor, mode, exposure);
}
  
-	if (ret < 0)

-   return ret;
-
-   ret = ov5640_set_timings(sensor, mode);
if (ret < 0)
return ret;
  

[PATCH v3 01/12] media: ov5640: Fix timings setup code

[Maxime Ripard]

From: Samuel Bobrowicz 

The current code, when changing the mode and changing the scaling or
sampling parameters, will look at the horizontal and vertical total size,
which, since 5999f381e023 ("media: ov5640: Add horizontal and vertical
totals") has been moved from the static register initialization to after
the mode change.

That means that the values are no longer set up before the code retrieves
them, which is obviously a bug.

Fixes: 5999f381e023 ("media: ov5640: Add horizontal and vertical totals")
Signed-off-by: Samuel Bobrowicz 
Signed-off-by: Maxime Ripard 
---
 drivers/media/i2c/ov5640.c | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index e480e53b369b..4bd968b478db 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -1462,6 +1462,10 @@ static int ov5640_set_mode_exposure_calc(struct 
ov5640_dev *sensor,
if (ret < 0)
return ret;
 
+   ret = ov5640_set_timings(sensor, mode);
+   if (ret < 0)
+   return ret;
+
/* read capture VTS */
ret = ov5640_get_vts(sensor);
if (ret < 0)
@@ -1589,6 +1593,10 @@ static int ov5640_set_mode_direct(struct ov5640_dev 
*sensor,
if (ret < 0)
return ret;
 
+   ret = ov5640_set_timings(sensor, mode);
+   if (ret < 0)
+   return ret;
+
/* turn auto gain/exposure back on for direct mode */
ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.auto_gain, 1);
if (ret)
@@ -1633,10 +1641,6 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
ret = ov5640_set_mode_direct(sensor, mode, exposure);
}
 
-   if (ret < 0)
-   return ret;
-
-   ret = ov5640_set_timings(sensor, mode);
if (ret < 0)
return ret;
 
-- 
2.17.0