[PATCH v2 0/2] media: i2c: ov5640: Re-work MIPI startup sequence

[PATCH v2 0/2] media: i2c: ov5640: Re-work MIPI startup sequence

[Jacopo Mondi]

Hello,
   this series fixes capture operations on i.MX6Q platforms (and possible other
platforms reported not working) using MIPI CSI-2 interface.

This iteration expands the v1 version with an additional fix, initially
submitted by Maxime in his series:
[PATCH v3 00/12] media: ov5640: Misc cleanup and improvements
https://www.spinics.net/lists/linux-media/msg134436.html

The original patch has been reported not fully fixing the issues by Daniel Mack
in his comment here below (on a Qualcomm platform if I'm not wrong):
https://www.spinics.net/lists/linux-media/msg134524.html
On my i.MX6Q testing platform that patch alone does not fix MIPI capture
neither.

The version I'm sending here re-introduces some of the timings parameters in the
initial configuration blob (not in the single mode ones), which apparently has
to be at least initially programmed to allow the driver to later program them
singularly in the 'set_timings()' function. Unfortunately I do not have a real
rationale behind this which explains why it has to be done this way :(

For the MIPI startup sequence re-work patch, no changes compared to v1.
Steve reported he has verified the LP-11 timout issue is solved on his testing
platform too. For more details, please refer to the v1 cover letter:
https://www.mail-archive.com/linux-media@vger.kernel.org/msg133352.html

Thanks
   j

Jacopo Mondi (1):
  media: i2c: ov5640: Re-work MIPI startup sequence

Samuel Bobrowicz (1):
  media: ov5640: Fix timings setup code

 drivers/media/i2c/ov5640.c | 107 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 82 insertions(+), 25 deletions(-)

--
2.7.4

[PATCH v2 1/2] media: i2c: ov5640: Re-work MIPI startup sequence

[Jacopo Mondi]

Rework the MIPI interface startup sequence with the following changes:

- Remove MIPI bus initialization from the initial settings blob
- At set_power(1) time power up MIPI Tx/Rx and set data and clock lanes in
  LP11 during 'sleep' and 'idle' with MIPI clock in non-continuous mode.
- At s_stream time enable/disable the MIPI interface output.
- Restore default settings at set_power(0) time.

Before this commit the sensor MIPI interface was initialized with settings
that require a start/stop sequence at power-up time in order to force lanes
into LP11 state, as they were initialized in LP00 when in 'sleep mode',
which is assumed to be the sensor manual definition for the D-PHY defined
stop mode.

The stream start/stop was performed by enabling disabling clock gating,
and had the side effect to change the lanes sleep mode configuration when
stream was stopped.

Clock gating/ungating:
-       ret = ov5640_mod_reg(sensor, OV5640_REG_MIPI_CTRL00, BIT(5),
-                            on ? 0 : BIT(5));
-       if (ret)

Set lanes in LP11 when in 'sleep mode':
-       ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00,
-                              on ? 0x00 : 0x70);

This commit fixes an issue reported by Jagan Teki on i.MX6 platforms that
prevents the host interface from powering up correctly:
https://lkml.org/lkml/2018/6/1/38

It also improves MIPI capture operations stability on my testing platform
where MIPI capture often failed and returned all-purple frames.

fixes: f22996db44e2 ("media: ov5640: add support of DVP parallel interface")
Reported-by: Jagan Teki <jagan@amarulasolutions.com>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov5640.c | 91 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 71 insertions(+), 20 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 1ecbb7a..7bbd1d7 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -286,10 +286,10 @@ static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
 	{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
 	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x3000, 0x00, 0, 0},
 	{0x3002, 0x1c, 0, 0}, {0x3004, 0xff, 0, 0}, {0x3006, 0xc3, 0, 0},
-	{0x300e, 0x45, 0, 0}, {0x302e, 0x08, 0, 0}, {0x4300, 0x3f, 0, 0},
+	{0x302e, 0x08, 0, 0}, {0x4300, 0x3f, 0, 0},
 	{0x501f, 0x00, 0, 0}, {0x4713, 0x03, 0, 0}, {0x4407, 0x04, 0, 0},
 	{0x440e, 0x00, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
-	{0x4837, 0x0a, 0, 0}, {0x4800, 0x04, 0, 0}, {0x3824, 0x02, 0, 0},
+	{0x4837, 0x0a, 0, 0}, {0x3824, 0x02, 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},
@@ -1102,12 +1102,18 @@ static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
 {
 	int ret;

-	ret = ov5640_mod_reg(sensor, OV5640_REG_MIPI_CTRL00, BIT(5),
-			     on ? 0 : BIT(5));
-	if (ret)
-		return ret;
-	ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00,
-			       on ? 0x00 : 0x70);
+	/*
+	 * Enable/disable the MIPI interface
+	 *
+	 * 0x300e = on ? 0x45 : 0x40
+	 * [7:5] = 001	: 2 data lanes mode
+	 * [4] = 0	: Power up MIPI HS Tx
+	 * [3] = 0	: Power up MIPI LS Rx
+	 * [2] = 1/0	: MIPI interface enable/disable
+	 * [1:0] = 01/00: FIXME: 'debug'
+	 */
+	ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00,
+			       on ? 0x45 : 0x40);
 	if (ret)
 		return ret;

@@ -1786,23 +1792,68 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
 		if (ret)
 			goto power_off;

+		/* We're done here for DVP bus, while CSI-2 needs setup. */
+		if (sensor->ep.bus_type != V4L2_MBUS_CSI2)
+			return 0;
+
+		/*
+		 * Power up MIPI HS Tx and LS Rx; 2 data lanes mode
+		 *
+		 * 0x300e = 0x40
+		 * [7:5] = 001	: 2 data lanes mode
+		 * [4] = 0	: Power up MIPI HS Tx
+		 * [3] = 0	: Power up MIPI LS Rx
+		 * [2] = 0	: MIPI interface disabled
+		 */
+		ret = ov5640_write_reg(sensor,
+				       OV5640_REG_IO_MIPI_CTRL00, 0x40);
+		if (ret)
+			goto power_off;
+
+		/*
+		 * Gate clock and set LP11 in 'no packets mode' (idle)
+		 *
+		 * 0x4800 = 0x24
+		 * [5] = 1	: Gate clock when 'no packets'
+		 * [2] = 1	: MIPI bus in LP11 when 'no packets'
+		 */
+		ret = ov5640_write_reg(sensor,
+				       OV5640_REG_MIPI_CTRL00, 0x24);
+		if (ret)
+			goto power_off;
+
+		/*
+		 * Set data lanes and clock in LP11 when 'sleeping'
+		 *
+		 * 0x3019 = 0x70
+		 * [6] = 1	: MIPI data lane 2 in LP11 when 'sleeping'
+		 * [5] = 1	: MIPI data lane 1 in LP11 when 'sleeping'
+		 * [4] = 1	: MIPI clock lane in LP11 when 'sleeping'
+		 */
+		ret = ov5640_write_reg(sensor,
+				       OV5640_REG_PAD_OUTPUT00, 0x70);
+		if (ret)
+			goto power_off;
+
+		/* Give lanes some time to coax into LP11 state. */
+		usleep_range(500, 1000);
+
+	} else {
 		if (sensor->ep.bus_type == V4L2_MBUS_CSI2) {
-			/*
-			 * start streaming briefly followed by stream off in
-			 * order to coax the clock lane into LP-11 state.
-			 */
-			ret = ov5640_set_stream_mipi(sensor, true);
-			if (ret)
-				goto power_off;
-			usleep_range(1000, 2000);
-			ret = ov5640_set_stream_mipi(sensor, false);
-			if (ret)
-				goto power_off;
+			/* Reset MIPI bus settings to their default values. */
+			ov5640_write_reg(sensor,
+					 OV5640_REG_IO_MIPI_CTRL00, 0x58);
+			ov5640_write_reg(sensor,
+					 OV5640_REG_MIPI_CTRL00, 0x04);
+			ov5640_write_reg(sensor,
+					 OV5640_REG_PAD_OUTPUT00, 0x00);
 		}

-		return 0;
+		ov5640_set_power_off(sensor);
 	}

+	return 0;
+
 power_off:
 	ov5640_set_power_off(sensor);
 	return ret;
--
2.7.4

[PATCH v2 2/2] media: ov5640: Fix timings setup code

[Jacopo Mondi]

From: Samuel Bobrowicz <sam@elite-embedded.com>

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.

In addition, restore timings settings in the initial configuration register
blob only, to have MIPI capture operations work again.

Fixes: 5999f381e023 ("media: ov5640: Add horizontal and vertical totals")
Signed-off-by: Samuel Bobrowicz <sam@elite-embedded.com>
Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
[re-introduce timing parameters in initial configuration blob]
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

---
Compared to Maxime's and Sam's original version, this one re-introduces timing
configuration parameters in the init_settings configuration blob.

On my testing platform this fixes MIPI capture operations, that with the
original patch version applied was failing anyway.

Thanks
   j
---
 drivers/media/i2c/ov5640.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 7bbd1d7..bbcb908 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -277,7 +277,9 @@ static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
 	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
 	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
-	{0x3810, 0x00, 0, 0},
+	{0x3808, 0x02, 0, 0}, {0x3809, 0x80, 0, 0}, {0x380a, 0x01, 0, 0},
+	{0x380b, 0xe0, 0, 0}, {0x380c, 0x07, 0, 0}, {0x380d, 0x68, 0, 0},
+	{0x380e, 0x03, 0, 0}, {0x380f, 0xd8, 0, 0}, {0x3810, 0x00, 0, 0},
 	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
 	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
 	{0x3709, 0x52, 0, 0}, {0x370c, 0x03, 0, 0}, {0x3a02, 0x03, 0, 0},
@@ -1484,6 +1486,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)
@@ -1611,6 +1617,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)
@@ -1658,10 +1668,6 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
 	if (ret < 0)
 		return ret;

-	ret = ov5640_set_timings(sensor, mode);
-	if (ret < 0)
-		return ret;
-
 	ret = ov5640_set_binning(sensor, dn_mode != SCALING);
 	if (ret < 0)
 		return ret;
--
2.7.4

Re: [PATCH v2 0/2] media: i2c: ov5640: Re-work MIPI startup sequence

[Steve Longerbeam]

Hi Jacopo,

Sorry to report my testing on SabreSD has same result
as last time. This series fixes the LP-11 timeout at stream
on but captured images are still blank. I tried the 640x480
mode with UYVY2X8. Here is the pad config:

# media-ctl --get-v4l2 "'ov5640 1-003c':0"
         [fmt:UYVY8_2X8/640x480@1/30 field:none colorspace:srgb 
xfer:srgb ycbcr:601 quantization:full-range]

Steve

On 07/10/2018 11:36 AM, Jacopo Mondi wrote:
> Hello,
>     this series fixes capture operations on i.MX6Q platforms (and possible other
> platforms reported not working) using MIPI CSI-2 interface.
>
> This iteration expands the v1 version with an additional fix, initially
> submitted by Maxime in his series:
> [PATCH v3 00/12] media: ov5640: Misc cleanup and improvements
> https://www.spinics.net/lists/linux-media/msg134436.html
>
> The original patch has been reported not fully fixing the issues by Daniel Mack
> in his comment here below (on a Qualcomm platform if I'm not wrong):
> https://www.spinics.net/lists/linux-media/msg134524.html
> On my i.MX6Q testing platform that patch alone does not fix MIPI capture
> neither.
>
> The version I'm sending here re-introduces some of the timings parameters in the
> initial configuration blob (not in the single mode ones), which apparently has
> to be at least initially programmed to allow the driver to later program them
> singularly in the 'set_timings()' function. Unfortunately I do not have a real
> rationale behind this which explains why it has to be done this way :(
>
> For the MIPI startup sequence re-work patch, no changes compared to v1.
> Steve reported he has verified the LP-11 timout issue is solved on his testing
> platform too. For more details, please refer to the v1 cover letter:
> https://www.mail-archive.com/linux-media@vger.kernel.org/msg133352.html
>
> Thanks
>     j
>
> Jacopo Mondi (1):
>    media: i2c: ov5640: Re-work MIPI startup sequence
>
> Samuel Bobrowicz (1):
>    media: ov5640: Fix timings setup code
>
>   drivers/media/i2c/ov5640.c | 107 ++++++++++++++++++++++++++++++++++-----------
>   1 file changed, 82 insertions(+), 25 deletions(-)
>
> --
> 2.7.4
>

Re: [PATCH v2 0/2] media: i2c: ov5640: Re-work MIPI startup sequence

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 3105 bytes --]

Hi Steve,

On Tue, Jul 10, 2018 at 02:10:54PM -0700, Steve Longerbeam wrote:
> Hi Jacopo,
>
> Sorry to report my testing on SabreSD has same result
> as last time. This series fixes the LP-11 timeout at stream
> on but captured images are still blank. I tried the 640x480
> mode with UYVY2X8. Here is the pad config:

This saddens me :(

I'm capturing with the same format and sizes... this shouldn't be the
issue

Could you confirm this matches what you have in your tree?
5dc2c80 media: ov5640: Fix timings setup code
b35e757 media: i2c: ov5640: Re-work MIPI startup sequence
3c4a737 media: ov5640: fix frame interval enumeration
41cb1c7 media: ov5640: adjust xclk_max
c3f3ba3 media: ov5640: add support of module orientation
ce85705 media: ov5640: add HFLIP/VFLIP controls support
8663341 media: ov5640: Program the visible resolution
476dec0 media: ov5640: Add horizontal and vertical totals
dba13a0 media: ov5640: Change horizontal and vertical resolutions name
8f57c2f media: ov5640: Init properly the SCLK dividers

Thanks
   j

>
> # media-ctl --get-v4l2 "'ov5640 1-003c':0"
>         [fmt:UYVY8_2X8/640x480@1/30 field:none colorspace:srgb xfer:srgb
> ycbcr:601 quantization:full-range]
>
> Steve
>
> On 07/10/2018 11:36 AM, Jacopo Mondi wrote:
> >Hello,
> >    this series fixes capture operations on i.MX6Q platforms (and possible other
> >platforms reported not working) using MIPI CSI-2 interface.
> >
> >This iteration expands the v1 version with an additional fix, initially
> >submitted by Maxime in his series:
> >[PATCH v3 00/12] media: ov5640: Misc cleanup and improvements
> >https://www.spinics.net/lists/linux-media/msg134436.html
> >
> >The original patch has been reported not fully fixing the issues by Daniel Mack
> >in his comment here below (on a Qualcomm platform if I'm not wrong):
> >https://www.spinics.net/lists/linux-media/msg134524.html
> >On my i.MX6Q testing platform that patch alone does not fix MIPI capture
> >neither.
> >
> >The version I'm sending here re-introduces some of the timings parameters in the
> >initial configuration blob (not in the single mode ones), which apparently has
> >to be at least initially programmed to allow the driver to later program them
> >singularly in the 'set_timings()' function. Unfortunately I do not have a real
> >rationale behind this which explains why it has to be done this way :(
> >
> >For the MIPI startup sequence re-work patch, no changes compared to v1.
> >Steve reported he has verified the LP-11 timout issue is solved on his testing
> >platform too. For more details, please refer to the v1 cover letter:
> >https://www.mail-archive.com/linux-media@vger.kernel.org/msg133352.html
> >
> >Thanks
> >    j
> >
> >Jacopo Mondi (1):
> >   media: i2c: ov5640: Re-work MIPI startup sequence
> >
> >Samuel Bobrowicz (1):
> >   media: ov5640: Fix timings setup code
> >
> >  drivers/media/i2c/ov5640.c | 107 ++++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 82 insertions(+), 25 deletions(-)
> >
> >--
> >2.7.4
> >
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 0/2] media: i2c: ov5640: Re-work MIPI startup sequence

[Steve Longerbeam]

Hi Jacopo,

Pardon the late reply, see below.

On 07/11/2018 12:21 AM, jacopo mondi wrote:
> Hi Steve,
>
> On Tue, Jul 10, 2018 at 02:10:54PM -0700, Steve Longerbeam wrote:
>> Hi Jacopo,
>>
>> Sorry to report my testing on SabreSD has same result
>> as last time. This series fixes the LP-11 timeout at stream
>> on but captured images are still blank. I tried the 640x480
>> mode with UYVY2X8. Here is the pad config:
> This saddens me :(
>
> I'm capturing with the same format and sizes... this shouldn't be the
> issue
>
> Could you confirm this matches what you have in your tree?
> 5dc2c80 media: ov5640: Fix timings setup code
> b35e757 media: i2c: ov5640: Re-work MIPI startup sequence
> 3c4a737 media: ov5640: fix frame interval enumeration
> 41cb1c7 media: ov5640: adjust xclk_max
> c3f3ba3 media: ov5640: add support of module orientation
> ce85705 media: ov5640: add HFLIP/VFLIP controls support
> 8663341 media: ov5640: Program the visible resolution
> 476dec0 media: ov5640: Add horizontal and vertical totals
> dba13a0 media: ov5640: Change horizontal and vertical resolutions name
> 8f57c2f media: ov5640: Init properly the SCLK dividers

Yes, I have that commit sequence.

FWIW, I can verify what Jagan Teki reported earlier, that the driver still
works on the SabreSD platform at:

dba13a0 media: ov5640: Change horizontal and vertical resolutions name

and is broken at:

476dec0 media: ov5640: Add horizontal and vertical totals

with LP-11 timeout at the mipi csi-2 receiver:

[   80.763189] imx6-mipi-csi2: LP-11 timeout, phy_state = 0x00000230
[   80.769599] ipu1_csi1: pipeline start failed with -110


Steve

>
>
>
>> # media-ctl --get-v4l2 "'ov5640 1-003c':0"
>>          [fmt:UYVY8_2X8/640x480@1/30 field:none colorspace:srgb xfer:srgb
>> ycbcr:601 quantization:full-range]
>>
>> Steve
>>
>> On 07/10/2018 11:36 AM, Jacopo Mondi wrote:
>>> Hello,
>>>     this series fixes capture operations on i.MX6Q platforms (and possible other
>>> platforms reported not working) using MIPI CSI-2 interface.
>>>
>>> This iteration expands the v1 version with an additional fix, initially
>>> submitted by Maxime in his series:
>>> [PATCH v3 00/12] media: ov5640: Misc cleanup and improvements
>>> https://www.spinics.net/lists/linux-media/msg134436.html
>>>
>>> The original patch has been reported not fully fixing the issues by Daniel Mack
>>> in his comment here below (on a Qualcomm platform if I'm not wrong):
>>> https://www.spinics.net/lists/linux-media/msg134524.html
>>> On my i.MX6Q testing platform that patch alone does not fix MIPI capture
>>> neither.
>>>
>>> The version I'm sending here re-introduces some of the timings parameters in the
>>> initial configuration blob (not in the single mode ones), which apparently has
>>> to be at least initially programmed to allow the driver to later program them
>>> singularly in the 'set_timings()' function. Unfortunately I do not have a real
>>> rationale behind this which explains why it has to be done this way :(
>>>
>>> For the MIPI startup sequence re-work patch, no changes compared to v1.
>>> Steve reported he has verified the LP-11 timout issue is solved on his testing
>>> platform too. For more details, please refer to the v1 cover letter:
>>> https://www.mail-archive.com/linux-media@vger.kernel.org/msg133352.html
>>>
>>> Thanks
>>>     j
>>>
>>> Jacopo Mondi (1):
>>>    media: i2c: ov5640: Re-work MIPI startup sequence
>>>
>>> Samuel Bobrowicz (1):
>>>    media: ov5640: Fix timings setup code
>>>
>>>   drivers/media/i2c/ov5640.c | 107 ++++++++++++++++++++++++++++++++++-----------
>>>   1 file changed, 82 insertions(+), 25 deletions(-)
>>>
>>> --
>>> 2.7.4
>>>

Re: [PATCH v2 0/2] media: i2c: ov5640: Re-work MIPI startup sequence

[Steve Longerbeam]

Hi Jacopo,


On 07/14/2018 11:57 AM, Steve Longerbeam wrote:
> Hi Jacopo,
>
> Pardon the late reply, see below.
>
> On 07/11/2018 12:21 AM, jacopo mondi wrote:
>> Hi Steve,
>>
>> On Tue, Jul 10, 2018 at 02:10:54PM -0700, Steve Longerbeam wrote:
>>> Hi Jacopo,
>>>
>>> Sorry to report my testing on SabreSD has same result
>>> as last time. This series fixes the LP-11 timeout at stream
>>> on but captured images are still blank. I tried the 640x480
>>> mode with UYVY2X8. Here is the pad config:
>> This saddens me :(
>>
>> I'm capturing with the same format and sizes... this shouldn't be the
>> issue
>>
>> Could you confirm this matches what you have in your tree?
>> 5dc2c80 media: ov5640: Fix timings setup code
>> b35e757 media: i2c: ov5640: Re-work MIPI startup sequence
>> 3c4a737 media: ov5640: fix frame interval enumeration
>> 41cb1c7 media: ov5640: adjust xclk_max
>> c3f3ba3 media: ov5640: add support of module orientation
>> ce85705 media: ov5640: add HFLIP/VFLIP controls support
>> 8663341 media: ov5640: Program the visible resolution
>> 476dec0 media: ov5640: Add horizontal and vertical totals
>> dba13a0 media: ov5640: Change horizontal and vertical resolutions name
>> 8f57c2f media: ov5640: Init properly the SCLK dividers
>
> Yes, I have that commit sequence.
>
> FWIW, I can verify what Jagan Teki reported earlier, that the driver 
> still
> works on the SabreSD platform at:
>
> dba13a0 media: ov5640: Change horizontal and vertical resolutions name
>
> and is broken at:
>
> 476dec0 media: ov5640: Add horizontal and vertical totals
>
> with LP-11 timeout at the mipi csi-2 receiver:
>
> [   80.763189] imx6-mipi-csi2: LP-11 timeout, phy_state = 0x00000230
> [   80.769599] ipu1_csi1: pipeline start failed with -110

And I discovered the bug in 476dec0 "media: ov5640: Add horizontal and
vertical totals". The call to ov5640_set_timings() needs to be moved 
before the
calls to ov5640_get_vts() and ov5640_get_hts(). But I see you have 
discovered
that as well, and fixed in the second patch in your series.


Steve

>
>
>
>>
>>
>>
>>> # media-ctl --get-v4l2 "'ov5640 1-003c':0"
>>>          [fmt:UYVY8_2X8/640x480@1/30 field:none colorspace:srgb 
>>> xfer:srgb
>>> ycbcr:601 quantization:full-range]
>>>
>>> Steve
>>>
>>> On 07/10/2018 11:36 AM, Jacopo Mondi wrote:
>>>> Hello,
>>>>     this series fixes capture operations on i.MX6Q platforms (and 
>>>> possible other
>>>> platforms reported not working) using MIPI CSI-2 interface.
>>>>
>>>> This iteration expands the v1 version with an additional fix, 
>>>> initially
>>>> submitted by Maxime in his series:
>>>> [PATCH v3 00/12] media: ov5640: Misc cleanup and improvements
>>>> https://www.spinics.net/lists/linux-media/msg134436.html
>>>>
>>>> The original patch has been reported not fully fixing the issues by 
>>>> Daniel Mack
>>>> in his comment here below (on a Qualcomm platform if I'm not wrong):
>>>> https://www.spinics.net/lists/linux-media/msg134524.html
>>>> On my i.MX6Q testing platform that patch alone does not fix MIPI 
>>>> capture
>>>> neither.
>>>>
>>>> The version I'm sending here re-introduces some of the timings 
>>>> parameters in the
>>>> initial configuration blob (not in the single mode ones), which 
>>>> apparently has
>>>> to be at least initially programmed to allow the driver to later 
>>>> program them
>>>> singularly in the 'set_timings()' function. Unfortunately I do not 
>>>> have a real
>>>> rationale behind this which explains why it has to be done this way :(
>>>>
>>>> For the MIPI startup sequence re-work patch, no changes compared to 
>>>> v1.
>>>> Steve reported he has verified the LP-11 timout issue is solved on 
>>>> his testing
>>>> platform too. For more details, please refer to the v1 cover letter:
>>>> https://www.mail-archive.com/linux-media@vger.kernel.org/msg133352.html 
>>>>
>>>>
>>>> Thanks
>>>>     j
>>>>
>>>> Jacopo Mondi (1):
>>>>    media: i2c: ov5640: Re-work MIPI startup sequence
>>>>
>>>> Samuel Bobrowicz (1):
>>>>    media: ov5640: Fix timings setup code
>>>>
>>>>   drivers/media/i2c/ov5640.c | 107 
>>>> ++++++++++++++++++++++++++++++++++-----------
>>>>   1 file changed, 82 insertions(+), 25 deletions(-)
>>>>
>>>> -- 
>>>> 2.7.4
>>>>
>

Re: [PATCH v2 0/2] media: i2c: ov5640: Re-work MIPI startup sequence

[Steve Longerbeam]

On 07/14/2018 12:41 PM, Steve Longerbeam wrote:
> Hi Jacopo,
>
>
> On 07/14/2018 11:57 AM, Steve Longerbeam wrote:
>> Hi Jacopo,
>>
>> Pardon the late reply, see below.
>>
>> On 07/11/2018 12:21 AM, jacopo mondi wrote:
>>> Hi Steve,
>>>
>>> On Tue, Jul 10, 2018 at 02:10:54PM -0700, Steve Longerbeam wrote:
>>>> Hi Jacopo,
>>>>
>>>> Sorry to report my testing on SabreSD has same result
>>>> as last time. This series fixes the LP-11 timeout at stream
>>>> on but captured images are still blank. I tried the 640x480
>>>> mode with UYVY2X8. Here is the pad config:
>>> This saddens me :(
>>>
>>> I'm capturing with the same format and sizes... this shouldn't be the
>>> issue
>>>
>>> Could you confirm this matches what you have in your tree?
>>> 5dc2c80 media: ov5640: Fix timings setup code
>>> b35e757 media: i2c: ov5640: Re-work MIPI startup sequence
>>> 3c4a737 media: ov5640: fix frame interval enumeration
>>> 41cb1c7 media: ov5640: adjust xclk_max
>>> c3f3ba3 media: ov5640: add support of module orientation
>>> ce85705 media: ov5640: add HFLIP/VFLIP controls support
>>> 8663341 media: ov5640: Program the visible resolution
>>> 476dec0 media: ov5640: Add horizontal and vertical totals
>>> dba13a0 media: ov5640: Change horizontal and vertical resolutions name
>>> 8f57c2f media: ov5640: Init properly the SCLK dividers
>>
>> Yes, I have that commit sequence.
>>
>> FWIW, I can verify what Jagan Teki reported earlier, that the driver 
>> still
>> works on the SabreSD platform at:
>>
>> dba13a0 media: ov5640: Change horizontal and vertical resolutions name
>>
>> and is broken at:
>>
>> 476dec0 media: ov5640: Add horizontal and vertical totals
>>
>> with LP-11 timeout at the mipi csi-2 receiver:
>>
>> [   80.763189] imx6-mipi-csi2: LP-11 timeout, phy_state = 0x00000230
>> [   80.769599] ipu1_csi1: pipeline start failed with -110
>
> And I discovered the bug in 476dec0 "media: ov5640: Add horizontal and
> vertical totals". The call to ov5640_set_timings() needs to be moved 
> before the
> calls to ov5640_get_vts() and ov5640_get_hts(). But I see you have 
> discovered
> that as well, and fixed in the second patch in your series.
>

But strangely, if I revert to 476dec0, and then move the call to 
ov5640_set_timings()
to just after ov5640_load_regs() in ov5640_set_mode_exposure_calc() and
ov5640_set_mode_direct(), the LP-11 timeouts are still present. So I can 
confirm
this strangeness which you already pointed out below [1].


>
>>
>>>
>>>>
>>>>>
>>>>> The version I'm sending here re-introduces some of the timings 
>>>>> parameters in the
>>>>> initial configuration blob (not in the single mode ones), which 
>>>>> apparently has
>>>>> to be at least initially programmed to allow the driver to later 
>>>>> program them
>>>>> singularly in the 'set_timings()' function. Unfortunately I do not 
>>>>> have a real
>>>>> rationale behind this which explains why it has to be done this 
>>>>> way :(
>>>>>

[1] here :)

Steve

Re: [PATCH v2 0/2] media: i2c: ov5640: Re-work MIPI startup sequence

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 3454 bytes --]

Hi Steve,
   thanks for keep testing it

On Sat, Jul 14, 2018 at 01:02:32PM -0700, Steve Longerbeam wrote:
>
>
> On 07/14/2018 12:41 PM, Steve Longerbeam wrote:
> >Hi Jacopo,
> >
> >
> >On 07/14/2018 11:57 AM, Steve Longerbeam wrote:
> >>Hi Jacopo,
> >>
> >>Pardon the late reply, see below.
> >>
> >>On 07/11/2018 12:21 AM, jacopo mondi wrote:
> >>>Hi Steve,
> >>>
> >>>On Tue, Jul 10, 2018 at 02:10:54PM -0700, Steve Longerbeam wrote:
> >>>>Hi Jacopo,
> >>>>
> >>>>Sorry to report my testing on SabreSD has same result
> >>>>as last time. This series fixes the LP-11 timeout at stream
> >>>>on but captured images are still blank. I tried the 640x480
> >>>>mode with UYVY2X8. Here is the pad config:
> >>>This saddens me :(
> >>>
> >>>I'm capturing with the same format and sizes... this shouldn't be the
> >>>issue
> >>>
> >>>Could you confirm this matches what you have in your tree?
> >>>5dc2c80 media: ov5640: Fix timings setup code
> >>>b35e757 media: i2c: ov5640: Re-work MIPI startup sequence
> >>>3c4a737 media: ov5640: fix frame interval enumeration
> >>>41cb1c7 media: ov5640: adjust xclk_max
> >>>c3f3ba3 media: ov5640: add support of module orientation
> >>>ce85705 media: ov5640: add HFLIP/VFLIP controls support
> >>>8663341 media: ov5640: Program the visible resolution
> >>>476dec0 media: ov5640: Add horizontal and vertical totals
> >>>dba13a0 media: ov5640: Change horizontal and vertical resolutions name
> >>>8f57c2f media: ov5640: Init properly the SCLK dividers
> >>
> >>Yes, I have that commit sequence.
> >>
> >>FWIW, I can verify what Jagan Teki reported earlier, that the driver
> >>still
> >>works on the SabreSD platform at:
> >>
> >>dba13a0 media: ov5640: Change horizontal and vertical resolutions name
> >>
> >>and is broken at:
> >>
> >>476dec0 media: ov5640: Add horizontal and vertical totals
> >>
> >>with LP-11 timeout at the mipi csi-2 receiver:
> >>
> >>[   80.763189] imx6-mipi-csi2: LP-11 timeout, phy_state = 0x00000230
> >>[   80.769599] ipu1_csi1: pipeline start failed with -110
> >
> >And I discovered the bug in 476dec0 "media: ov5640: Add horizontal and
> >vertical totals". The call to ov5640_set_timings() needs to be moved
> >before the
> >calls to ov5640_get_vts() and ov5640_get_hts(). But I see you have
> >discovered
> >that as well, and fixed in the second patch in your series.
> >

I'm sorry I'm not sur I'm following. Does this mean that with that bug
you are referring to up here fixed by my last patch you have capture
working?

Thanks
   j

>
> But strangely, if I revert to 476dec0, and then move the call to
> ov5640_set_timings()
> to just after ov5640_load_regs() in ov5640_set_mode_exposure_calc() and
> ov5640_set_mode_direct(), the LP-11 timeouts are still present. So I can
> confirm
> this strangeness which you already pointed out below [1].
>
>
> >
> >>
> >>>
> >>>>
> >>>>>
> >>>>>The version I'm sending here re-introduces some of the timings
> >>>>>parameters in the
> >>>>>initial configuration blob (not in the single mode ones), which
> >>>>>apparently has
> >>>>>to be at least initially programmed to allow the driver to later
> >>>>>program them
> >>>>>singularly in the 'set_timings()' function. Unfortunately I do not
> >>>>>have a real
> >>>>>rationale behind this which explains why it has to be done this
> >>>>>way :(
> >>>>>
>
> [1] here :)
>
> Steve
>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 0/2] media: i2c: ov5640: Re-work MIPI startup sequence

[Steve Longerbeam]

On 07/16/2018 01:29 AM, jacopo mondi wrote:
> Hi Steve,
>     thanks for keep testing it
>
> On Sat, Jul 14, 2018 at 01:02:32PM -0700, Steve Longerbeam wrote:
>>
>> On 07/14/2018 12:41 PM, Steve Longerbeam wrote:
>>> Hi Jacopo,
>>>
>>>
>>> On 07/14/2018 11:57 AM, Steve Longerbeam wrote:
>>>> Hi Jacopo,
>>>>
>>>> Pardon the late reply, see below.
>>>>
>>>> On 07/11/2018 12:21 AM, jacopo mondi wrote:
>>>>> Hi Steve,
>>>>>
>>>>> On Tue, Jul 10, 2018 at 02:10:54PM -0700, Steve Longerbeam wrote:
>>>>>> Hi Jacopo,
>>>>>>
>>>>>> Sorry to report my testing on SabreSD has same result
>>>>>> as last time. This series fixes the LP-11 timeout at stream
>>>>>> on but captured images are still blank. I tried the 640x480
>>>>>> mode with UYVY2X8. Here is the pad config:
>>>>> This saddens me :(
>>>>>
>>>>> I'm capturing with the same format and sizes... this shouldn't be the
>>>>> issue
>>>>>
>>>>> Could you confirm this matches what you have in your tree?
>>>>> 5dc2c80 media: ov5640: Fix timings setup code
>>>>> b35e757 media: i2c: ov5640: Re-work MIPI startup sequence
>>>>> 3c4a737 media: ov5640: fix frame interval enumeration
>>>>> 41cb1c7 media: ov5640: adjust xclk_max
>>>>> c3f3ba3 media: ov5640: add support of module orientation
>>>>> ce85705 media: ov5640: add HFLIP/VFLIP controls support
>>>>> 8663341 media: ov5640: Program the visible resolution
>>>>> 476dec0 media: ov5640: Add horizontal and vertical totals
>>>>> dba13a0 media: ov5640: Change horizontal and vertical resolutions name
>>>>> 8f57c2f media: ov5640: Init properly the SCLK dividers
>>>> Yes, I have that commit sequence.
>>>>
>>>> FWIW, I can verify what Jagan Teki reported earlier, that the driver
>>>> still
>>>> works on the SabreSD platform at:
>>>>
>>>> dba13a0 media: ov5640: Change horizontal and vertical resolutions name
>>>>
>>>> and is broken at:
>>>>
>>>> 476dec0 media: ov5640: Add horizontal and vertical totals
>>>>
>>>> with LP-11 timeout at the mipi csi-2 receiver:
>>>>
>>>> [   80.763189] imx6-mipi-csi2: LP-11 timeout, phy_state = 0x00000230
>>>> [   80.769599] ipu1_csi1: pipeline start failed with -110
>>> And I discovered the bug in 476dec0 "media: ov5640: Add horizontal and
>>> vertical totals". The call to ov5640_set_timings() needs to be moved
>>> before the
>>> calls to ov5640_get_vts() and ov5640_get_hts(). But I see you have
>>> discovered
>>> that as well, and fixed in the second patch in your series.
>>>
> I'm sorry I'm not sur I'm following. Does this mean that with that bug
> you are referring to up here fixed by my last patch you have capture
> working?

No, capture still not working for me on SabreSD, even after fixing
the bug in 476dec0 "media: ov5640: Add horizontal and vertical totals",
by either using your patchset, or by running version 476dec0 of ov5640.c
with the call to ov5640_set_timings() moved to the correct places as
described below.

Steve

>> But strangely, if I revert to 476dec0, and then move the call to
>> ov5640_set_timings()
>> to just after ov5640_load_regs() in ov5640_set_mode_exposure_calc() and
>> ov5640_set_mode_direct(), the LP-11 timeouts are still present. So I can
>> confirm
>> this strangeness which you already pointed out below [1].
>>
>>
>>>>>>> The version I'm sending here re-introduces some of the timings
>>>>>>> parameters in the
>>>>>>> initial configuration blob (not in the single mode ones), which
>>>>>>> apparently has
>>>>>>> to be at least initially programmed to allow the driver to later
>>>>>>> program them
>>>>>>> singularly in the 'set_timings()' function. Unfortunately I do not
>>>>>>> have a real
>>>>>>> rationale behind this which explains why it has to be done this
>>>>>>> way :(
>>>>>>>
>> [1] here :)
>>
>> Steve
>>
>>

Re: [PATCH v2 0/2] media: i2c: ov5640: Re-work MIPI startup sequence

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 4874 bytes --]

Hi Steve,
   sorry for resurecting this.

On Mon, Jul 16, 2018 at 09:26:13AM -0700, Steve Longerbeam wrote:
>
>
> On 07/16/2018 01:29 AM, jacopo mondi wrote:
> >Hi Steve,
> >    thanks for keep testing it
> >
> >On Sat, Jul 14, 2018 at 01:02:32PM -0700, Steve Longerbeam wrote:
> >>
> >>On 07/14/2018 12:41 PM, Steve Longerbeam wrote:
> >>>Hi Jacopo,
> >>>
> >>>
> >>>On 07/14/2018 11:57 AM, Steve Longerbeam wrote:
> >>>>Hi Jacopo,
> >>>>
> >>>>Pardon the late reply, see below.
> >>>>
> >>>>On 07/11/2018 12:21 AM, jacopo mondi wrote:
> >>>>>Hi Steve,
> >>>>>
> >>>>>On Tue, Jul 10, 2018 at 02:10:54PM -0700, Steve Longerbeam wrote:
> >>>>>>Hi Jacopo,
> >>>>>>
> >>>>>>Sorry to report my testing on SabreSD has same result
> >>>>>>as last time. This series fixes the LP-11 timeout at stream
> >>>>>>on but captured images are still blank. I tried the 640x480
> >>>>>>mode with UYVY2X8. Here is the pad config:
> >>>>>This saddens me :(
> >>>>>
> >>>>>I'm capturing with the same format and sizes... this shouldn't be the
> >>>>>issue
> >>>>>
> >>>>>Could you confirm this matches what you have in your tree?
> >>>>>5dc2c80 media: ov5640: Fix timings setup code
> >>>>>b35e757 media: i2c: ov5640: Re-work MIPI startup sequence
> >>>>>3c4a737 media: ov5640: fix frame interval enumeration
> >>>>>41cb1c7 media: ov5640: adjust xclk_max
> >>>>>c3f3ba3 media: ov5640: add support of module orientation
> >>>>>ce85705 media: ov5640: add HFLIP/VFLIP controls support
> >>>>>8663341 media: ov5640: Program the visible resolution
> >>>>>476dec0 media: ov5640: Add horizontal and vertical totals
> >>>>>dba13a0 media: ov5640: Change horizontal and vertical resolutions name
> >>>>>8f57c2f media: ov5640: Init properly the SCLK dividers
> >>>>Yes, I have that commit sequence.
> >>>>
> >>>>FWIW, I can verify what Jagan Teki reported earlier, that the driver
> >>>>still
> >>>>works on the SabreSD platform at:
> >>>>
> >>>>dba13a0 media: ov5640: Change horizontal and vertical resolutions name
> >>>>
> >>>>and is broken at:
> >>>>
> >>>>476dec0 media: ov5640: Add horizontal and vertical totals
> >>>>
> >>>>with LP-11 timeout at the mipi csi-2 receiver:
> >>>>
> >>>>[   80.763189] imx6-mipi-csi2: LP-11 timeout, phy_state = 0x00000230
> >>>>[   80.769599] ipu1_csi1: pipeline start failed with -110
> >>>And I discovered the bug in 476dec0 "media: ov5640: Add horizontal and
> >>>vertical totals". The call to ov5640_set_timings() needs to be moved
> >>>before the
> >>>calls to ov5640_get_vts() and ov5640_get_hts(). But I see you have
> >>>discovered
> >>>that as well, and fixed in the second patch in your series.
> >>>
> >I'm sorry I'm not sur I'm following. Does this mean that with that bug
> >you are referring to up here fixed by my last patch you have capture
> >working?
>
> No, capture still not working for me on SabreSD, even after fixing
> the bug in 476dec0 "media: ov5640: Add horizontal and vertical totals",
> by either using your patchset, or by running version 476dec0 of ov5640.c
> with the call to ov5640_set_timings() moved to the correct places as
> described below.
>

I've been reported a bug on exposure handling that makes the first
captured frames all black. Both me and Hugues have tried to fix the
issue (him with a more complete series, but that's another topic).
See [1] and [2]

It might be possible that you're getting blank frames with this series
applied? I never seen them as I'm skipping the first frames when
capturing, but I've now tested and without the exposure fixes (either
[1] or [2]) I actually have blank frames.

If that's the case for you too (which I hope so much) would you be
available to test again this series with exposure fixes on top?
On my platform that actually makes all frames correct.

Thanks
   j

[1] [PATCH 0/2] media: ov5640: Fix set_timings and auto-exposure
[2] [PATCH v2 0/5] Fix OV5640 exposure & gain

> Steve
>
> >>But strangely, if I revert to 476dec0, and then move the call to
> >>ov5640_set_timings()
> >>to just after ov5640_load_regs() in ov5640_set_mode_exposure_calc() and
> >>ov5640_set_mode_direct(), the LP-11 timeouts are still present. So I can
> >>confirm
> >>this strangeness which you already pointed out below [1].
> >>
> >>
> >>>>>>>The version I'm sending here re-introduces some of the timings
> >>>>>>>parameters in the
> >>>>>>>initial configuration blob (not in the single mode ones), which
> >>>>>>>apparently has
> >>>>>>>to be at least initially programmed to allow the driver to later
> >>>>>>>program them
> >>>>>>>singularly in the 'set_timings()' function. Unfortunately I do not
> >>>>>>>have a real
> >>>>>>>rationale behind this which explains why it has to be done this
> >>>>>>>way :(
> >>>>>>>
> >>[1] here :)
> >>
> >>Steve
> >>
> >>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 0/2] media: i2c: ov5640: Re-work MIPI startup sequence

[Steve Longerbeam]

Hi Jacopo,


On 08/14/2018 08:35 AM, jacopo mondi wrote:
> Hi Steve,
>     sorry for resurecting this.
>
> <snip>
>>> I'm sorry I'm not sur I'm following. Does this mean that with that bug
>>> you are referring to up here fixed by my last patch you have capture
>>> working?
>> No, capture still not working for me on SabreSD, even after fixing
>> the bug in 476dec0 "media: ov5640: Add horizontal and vertical totals",
>> by either using your patchset, or by running version 476dec0 of ov5640.c
>> with the call to ov5640_set_timings() moved to the correct places as
>> described below.
>>
> I've been reported a bug on exposure handling that makes the first
> captured frames all black. Both me and Hugues have tried to fix the
> issue (him with a more complete series, but that's another topic).
> See [1] and [2]
>
> It might be possible that you're getting blank frames with this series
> applied? I never seen them as I'm skipping the first frames when
> capturing, but I've now tested and without the exposure fixes (either
> [1] or [2]) I actually have blank frames.
>
> If that's the case for you too (which I hope so much) would you be
> available to test again this series with exposure fixes on top?
> On my platform that actually makes all frames correct.
>
> Thanks
>     j
>
> [1] [PATCH 0/2] media: ov5640: Fix set_timings and auto-exposure
> [2] [PATCH v2 0/5] Fix OV5640 exposure & gain
>

It's not clear to me which patch sets you would like me to test.
Just [1] and [2], or [1], [2], and "media: i2c: ov5640: Re-work MIPI 
startup sequence"?

Steve

Re: [PATCH v2 0/2] media: i2c: ov5640: Re-work MIPI startup sequence

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 2127 bytes --]

Hi Steve,

On Tue, Aug 14, 2018 at 09:51:04AM -0700, Steve Longerbeam wrote:
> Hi Jacopo,
>
>
> On 08/14/2018 08:35 AM, jacopo mondi wrote:
> >Hi Steve,
> >    sorry for resurecting this.
> >
> ><snip>
> >>>I'm sorry I'm not sur I'm following. Does this mean that with that bug
> >>>you are referring to up here fixed by my last patch you have capture
> >>>working?
> >>No, capture still not working for me on SabreSD, even after fixing
> >>the bug in 476dec0 "media: ov5640: Add horizontal and vertical totals",
> >>by either using your patchset, or by running version 476dec0 of ov5640.c
> >>with the call to ov5640_set_timings() moved to the correct places as
> >>described below.
> >>
> >I've been reported a bug on exposure handling that makes the first
> >captured frames all black. Both me and Hugues have tried to fix the
> >issue (him with a more complete series, but that's another topic).
> >See [1] and [2]
> >
> >It might be possible that you're getting blank frames with this series
> >applied? I never seen them as I'm skipping the first frames when
> >capturing, but I've now tested and without the exposure fixes (either
> >[1] or [2]) I actually have blank frames.
> >
> >If that's the case for you too (which I hope so much) would you be
> >available to test again this series with exposure fixes on top?
> >On my platform that actually makes all frames correct.
> >
> >Thanks
> >    j
> >
> >[1] [PATCH 0/2] media: ov5640: Fix set_timings and auto-exposure
> >[2] [PATCH v2 0/5] Fix OV5640 exposure & gain
> >
>
> It's not clear to me which patch sets you would like me to test.
> Just [1] and [2], or [1], [2], and "media: i2c: ov5640: Re-work MIPI startup
> sequence"?
>
I have tested on my board the following:
v4.18-rc2 + MIPI Fix + Timings + Hugues' exposure fix

Without Hugues' patches I get blank frames (the first ones at least)
Without MIPI startup reowkr and timings I get the LP-11 error on the
CSI-2 bus.

As Hugues' series has to be rebased on mine, I have prepared a branch
here for you if you feel like testing it:
git://jmondi.org/linux ov5640/timings_exposure

Thanks
   j


> Steve
>
>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 0/2] media: i2c: ov5640: Re-work MIPI startup sequence

[Steve Longerbeam]

On 08/14/2018 10:38 AM, jacopo mondi wrote:
> Hi Steve,
>
> On Tue, Aug 14, 2018 at 09:51:04AM -0700, Steve Longerbeam wrote:
>> Hi Jacopo,
>>
>>
>> On 08/14/2018 08:35 AM, jacopo mondi wrote:
>>> Hi Steve,
>>>     sorry for resurecting this.
>>>
>>> <snip>
>>>>> I'm sorry I'm not sur I'm following. Does this mean that with that bug
>>>>> you are referring to up here fixed by my last patch you have capture
>>>>> working?
>>>> No, capture still not working for me on SabreSD, even after fixing
>>>> the bug in 476dec0 "media: ov5640: Add horizontal and vertical totals",
>>>> by either using your patchset, or by running version 476dec0 of ov5640.c
>>>> with the call to ov5640_set_timings() moved to the correct places as
>>>> described below.
>>>>
>>> I've been reported a bug on exposure handling that makes the first
>>> captured frames all black. Both me and Hugues have tried to fix the
>>> issue (him with a more complete series, but that's another topic).
>>> See [1] and [2]
>>>
>>> It might be possible that you're getting blank frames with this series
>>> applied? I never seen them as I'm skipping the first frames when
>>> capturing, but I've now tested and without the exposure fixes (either
>>> [1] or [2]) I actually have blank frames.
>>>
>>> If that's the case for you too (which I hope so much) would you be
>>> available to test again this series with exposure fixes on top?
>>> On my platform that actually makes all frames correct.
>>>
>>> Thanks
>>>     j
>>>
>>> [1] [PATCH 0/2] media: ov5640: Fix set_timings and auto-exposure
>>> [2] [PATCH v2 0/5] Fix OV5640 exposure & gain
>>>
>> It's not clear to me which patch sets you would like me to test.
>> Just [1] and [2], or [1], [2], and "media: i2c: ov5640: Re-work MIPI startup
>> sequence"?
>>
> I have tested on my board the following:
> v4.18-rc2 + MIPI Fix + Timings + Hugues' exposure fix
>
> Without Hugues' patches I get blank frames (the first ones at least)
> Without MIPI startup reowkr and timings I get the LP-11 error on the
> CSI-2 bus.
>
> As Hugues' series has to be rebased on mine, I have prepared a branch
> here for you if you feel like testing it:
> git://jmondi.org/linux ov5640/timings_exposure

Hi Jacopo, that branch works on SabreSD!

Feel free to add

Tested-by: Steve Longerbeam <slongerbeam@gmail.com>
on i.MX6q SabreSD with MIPI CSI-2 OV5640 module

to whichever ov5640 patches are appropriate.

Steve

Re: [PATCH v2 0/2] media: i2c: ov5640: Re-work MIPI startup sequence

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 3096 bytes --]

Hi Steve,

On Tue, Aug 14, 2018 at 04:53:26PM -0700, Steve Longerbeam wrote:
>
>
> On 08/14/2018 10:38 AM, jacopo mondi wrote:
> >Hi Steve,
> >
> >On Tue, Aug 14, 2018 at 09:51:04AM -0700, Steve Longerbeam wrote:
> >>Hi Jacopo,
> >>
> >>
> >>On 08/14/2018 08:35 AM, jacopo mondi wrote:
> >>>Hi Steve,
> >>>    sorry for resurecting this.
> >>>
> >>><snip>
> >>>>>I'm sorry I'm not sur I'm following. Does this mean that with that bug
> >>>>>you are referring to up here fixed by my last patch you have capture
> >>>>>working?
> >>>>No, capture still not working for me on SabreSD, even after fixing
> >>>>the bug in 476dec0 "media: ov5640: Add horizontal and vertical totals",
> >>>>by either using your patchset, or by running version 476dec0 of ov5640.c
> >>>>with the call to ov5640_set_timings() moved to the correct places as
> >>>>described below.
> >>>>
> >>>I've been reported a bug on exposure handling that makes the first
> >>>captured frames all black. Both me and Hugues have tried to fix the
> >>>issue (him with a more complete series, but that's another topic).
> >>>See [1] and [2]
> >>>
> >>>It might be possible that you're getting blank frames with this series
> >>>applied? I never seen them as I'm skipping the first frames when
> >>>capturing, but I've now tested and without the exposure fixes (either
> >>>[1] or [2]) I actually have blank frames.
> >>>
> >>>If that's the case for you too (which I hope so much) would you be
> >>>available to test again this series with exposure fixes on top?
> >>>On my platform that actually makes all frames correct.
> >>>
> >>>Thanks
> >>>    j
> >>>
> >>>[1] [PATCH 0/2] media: ov5640: Fix set_timings and auto-exposure
> >>>[2] [PATCH v2 0/5] Fix OV5640 exposure & gain
> >>>
> >>It's not clear to me which patch sets you would like me to test.
> >>Just [1] and [2], or [1], [2], and "media: i2c: ov5640: Re-work MIPI startup
> >>sequence"?
> >>
> >I have tested on my board the following:
> >v4.18-rc2 + MIPI Fix + Timings + Hugues' exposure fix
> >
> >Without Hugues' patches I get blank frames (the first ones at least)
> >Without MIPI startup reowkr and timings I get the LP-11 error on the
> >CSI-2 bus.
> >
> >As Hugues' series has to be rebased on mine, I have prepared a branch
> >here for you if you feel like testing it:
> >git://jmondi.org/linux ov5640/timings_exposure
>
> Hi Jacopo, that branch works on SabreSD!
>

YEAH! I'm so happy about this (and not to having asked you to test
yet-another-broken-patchset for no reason :)

> Feel free to add
>
> Tested-by: Steve Longerbeam <slongerbeam@gmail.com>
> on i.MX6q SabreSD with MIPI CSI-2 OV5640 module
>
> to whichever ov5640 patches are appropriate.

Great, I'll send a v3 now collecting your tags and the most recent
version of the timings fix that was not included in v2 (but in the
branch you have tested).

My ideal plan would be to have these two patches merged, then Hugues'
series to fix exposure handling merged on top. This would, in my mind
make capture on CSI-2 work properly.

Of course, more testing is very welcome.

Thanks again
   j

>
> Steve
>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]