[PATCH] media: imx: mipi csi-2: Don't fail if initial state times-out

[PATCH] media: imx: mipi csi-2: Don't fail if initial state times-out

[Ezequiel Garcia]

Not all sensors will be able to guarantee a proper initial state.
This may be either because the driver is not properly written,
or (probably unlikely) because the hardware won't support it.

While the right solution in the former case is to fix the sensor
driver, the real world not always allows right solutions, due to lack
of available documentation and support on these sensors.

Let's relax this requirement, and allow the driver to support stream start,
even if the sensor initial sequence wasn't the expected.
A warning is still emitted, so users should be hinted that something is off.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/staging/media/imx/imx6-mipi-csi2.c | 33 ++++++----------------
 1 file changed, 9 insertions(+), 24 deletions(-)

diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c
index f29e28df36ed..10342434e797 100644
--- a/drivers/staging/media/imx/imx6-mipi-csi2.c
+++ b/drivers/staging/media/imx/imx6-mipi-csi2.c
@@ -243,7 +243,7 @@ static int __maybe_unused csi2_dphy_wait_ulp(struct csi2_dev *csi2)
 }
 
 /* Waits for low-power LP-11 state on data and clock lanes. */
-static int csi2_dphy_wait_stopstate(struct csi2_dev *csi2)
+static void csi2_dphy_wait_stopstate(struct csi2_dev *csi2)
 {
 	u32 mask, reg;
 	int ret;
@@ -253,29 +253,21 @@ static int csi2_dphy_wait_stopstate(struct csi2_dev *csi2)
 
 	ret = readl_poll_timeout(csi2->base + CSI2_PHY_STATE, reg,
 				 (reg & mask) == mask, 0, 500000);
-	if (ret) {
-		v4l2_err(&csi2->sd, "LP-11 timeout, phy_state = 0x%08x\n", reg);
-		return ret;
-	}
-
-	return 0;
+	if (ret)
+		v4l2_warn(&csi2->sd, "LP-11 timeout, phy_state = 0x%08x\n", reg);
 }
 
 /* Wait for active clock on the clock lane. */
-static int csi2_dphy_wait_clock_lane(struct csi2_dev *csi2)
+static void csi2_dphy_wait_clock_lane(struct csi2_dev *csi2)
 {
 	u32 reg;
 	int ret;
 
 	ret = readl_poll_timeout(csi2->base + CSI2_PHY_STATE, reg,
 				 (reg & PHY_RXCLKACTIVEHS), 0, 500000);
-	if (ret) {
-		v4l2_err(&csi2->sd, "clock lane timeout, phy_state = 0x%08x\n",
-			 reg);
-		return ret;
-	}
-
-	return 0;
+	if (ret)
+		v4l2_warn(&csi2->sd, "clock lane timeout, phy_state = 0x%08x\n",
+			  reg);
 }
 
 /* Setup the i.MX CSI2IPU Gasket */
@@ -316,9 +308,7 @@ static int csi2_start(struct csi2_dev *csi2)
 	csi2_enable(csi2, true);
 
 	/* Step 5 */
-	ret = csi2_dphy_wait_stopstate(csi2);
-	if (ret)
-		goto err_assert_reset;
+	csi2_dphy_wait_stopstate(csi2);
 
 	/* Step 6 */
 	ret = v4l2_subdev_call(csi2->src_sd, video, s_stream, 1);
@@ -327,14 +317,9 @@ static int csi2_start(struct csi2_dev *csi2)
 		goto err_assert_reset;
 
 	/* Step 7 */
-	ret = csi2_dphy_wait_clock_lane(csi2);
-	if (ret)
-		goto err_stop_upstream;
-
+	csi2_dphy_wait_clock_lane(csi2);
 	return 0;
 
-err_stop_upstream:
-	v4l2_subdev_call(csi2->src_sd, video, s_stream, 0);
 err_assert_reset:
 	csi2_enable(csi2, false);
 err_disable_clk:
-- 
2.20.1

Re: [PATCH] media: imx: mipi csi-2: Don't fail if initial state times-out

[Steve Longerbeam]

Thanks, there was earlier talk of relaxing those CSI-2 bus startup 
requirements, but somehow it fell through the cracks.

Acked-by: Steve Longerbeam <slongerbeam@gmail.com>

On 6/25/19 1:39 PM, Ezequiel Garcia wrote:
> Not all sensors will be able to guarantee a proper initial state.
> This may be either because the driver is not properly written,
> or (probably unlikely) because the hardware won't support it.
>
> While the right solution in the former case is to fix the sensor
> driver, the real world not always allows right solutions, due to lack
> of available documentation and support on these sensors.
>
> Let's relax this requirement, and allow the driver to support stream start,
> even if the sensor initial sequence wasn't the expected.
> A warning is still emitted, so users should be hinted that something is off.
>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>   drivers/staging/media/imx/imx6-mipi-csi2.c | 33 ++++++----------------
>   1 file changed, 9 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c
> index f29e28df36ed..10342434e797 100644
> --- a/drivers/staging/media/imx/imx6-mipi-csi2.c
> +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c
> @@ -243,7 +243,7 @@ static int __maybe_unused csi2_dphy_wait_ulp(struct csi2_dev *csi2)
>   }
>   
>   /* Waits for low-power LP-11 state on data and clock lanes. */
> -static int csi2_dphy_wait_stopstate(struct csi2_dev *csi2)
> +static void csi2_dphy_wait_stopstate(struct csi2_dev *csi2)
>   {
>   	u32 mask, reg;
>   	int ret;
> @@ -253,29 +253,21 @@ static int csi2_dphy_wait_stopstate(struct csi2_dev *csi2)
>   
>   	ret = readl_poll_timeout(csi2->base + CSI2_PHY_STATE, reg,
>   				 (reg & mask) == mask, 0, 500000);
> -	if (ret) {
> -		v4l2_err(&csi2->sd, "LP-11 timeout, phy_state = 0x%08x\n", reg);
> -		return ret;
> -	}
> -
> -	return 0;
> +	if (ret)
> +		v4l2_warn(&csi2->sd, "LP-11 timeout, phy_state = 0x%08x\n", reg);
>   }
>   
>   /* Wait for active clock on the clock lane. */
> -static int csi2_dphy_wait_clock_lane(struct csi2_dev *csi2)
> +static void csi2_dphy_wait_clock_lane(struct csi2_dev *csi2)
>   {
>   	u32 reg;
>   	int ret;
>   
>   	ret = readl_poll_timeout(csi2->base + CSI2_PHY_STATE, reg,
>   				 (reg & PHY_RXCLKACTIVEHS), 0, 500000);
> -	if (ret) {
> -		v4l2_err(&csi2->sd, "clock lane timeout, phy_state = 0x%08x\n",
> -			 reg);
> -		return ret;
> -	}
> -
> -	return 0;
> +	if (ret)
> +		v4l2_warn(&csi2->sd, "clock lane timeout, phy_state = 0x%08x\n",
> +			  reg);
>   }
>   
>   /* Setup the i.MX CSI2IPU Gasket */
> @@ -316,9 +308,7 @@ static int csi2_start(struct csi2_dev *csi2)
>   	csi2_enable(csi2, true);
>   
>   	/* Step 5 */
> -	ret = csi2_dphy_wait_stopstate(csi2);
> -	if (ret)
> -		goto err_assert_reset;
> +	csi2_dphy_wait_stopstate(csi2);
>   
>   	/* Step 6 */
>   	ret = v4l2_subdev_call(csi2->src_sd, video, s_stream, 1);
> @@ -327,14 +317,9 @@ static int csi2_start(struct csi2_dev *csi2)
>   		goto err_assert_reset;
>   
>   	/* Step 7 */
> -	ret = csi2_dphy_wait_clock_lane(csi2);
> -	if (ret)
> -		goto err_stop_upstream;
> -
> +	csi2_dphy_wait_clock_lane(csi2);
>   	return 0;
>   
> -err_stop_upstream:
> -	v4l2_subdev_call(csi2->src_sd, video, s_stream, 0);
>   err_assert_reset:
>   	csi2_enable(csi2, false);
>   err_disable_clk:

Re: [PATCH] media: imx: mipi csi-2: Don't fail if initial state times-out

[Philipp Zabel]

On Tue, 2019-06-25 at 17:39 -0300, Ezequiel Garcia wrote:
> Not all sensors will be able to guarantee a proper initial state.
> This may be either because the driver is not properly written,
> or (probably unlikely) because the hardware won't support it.
> 
> While the right solution in the former case is to fix the sensor
> driver, the real world not always allows right solutions, due to lack
> of available documentation and support on these sensors.

Do you have a real world example for this?

> Let's relax this requirement, and allow the driver to support stream start,
> even if the sensor initial sequence wasn't the expected.
> A warning is still emitted, so users should be hinted that something is off.

I think the messages could use a note that they may be due to a sensor
or sensor driver bug, and that there might be image artifacts or
outright failure to capture as a consequence.

> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

> ---
>  drivers/staging/media/imx/imx6-mipi-csi2.c | 33 ++++++----------------
>  1 file changed, 9 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c
> index f29e28df36ed..10342434e797 100644
> --- a/drivers/staging/media/imx/imx6-mipi-csi2.c
> +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c
> @@ -243,7 +243,7 @@ static int __maybe_unused csi2_dphy_wait_ulp(struct csi2_dev *csi2)
>  }
>  
>  /* Waits for low-power LP-11 state on data and clock lanes. */
> -static int csi2_dphy_wait_stopstate(struct csi2_dev *csi2)
> +static void csi2_dphy_wait_stopstate(struct csi2_dev *csi2)
>  {
>  	u32 mask, reg;
>  	int ret;
> @@ -253,29 +253,21 @@ static int csi2_dphy_wait_stopstate(struct csi2_dev *csi2)
>  
>  	ret = readl_poll_timeout(csi2->base + CSI2_PHY_STATE, reg,
>  				 (reg & mask) == mask, 0, 500000);
> -	if (ret) {
> -		v4l2_err(&csi2->sd, "LP-11 timeout, phy_state = 0x%08x\n", reg);
> -		return ret;
> -	}
> -
> -	return 0;
> +	if (ret)
> +		v4l2_warn(&csi2->sd, "LP-11 timeout, phy_state = 0x%08x\n", reg);
>  }
>  
>  /* Wait for active clock on the clock lane. */
> -static int csi2_dphy_wait_clock_lane(struct csi2_dev *csi2)
> +static void csi2_dphy_wait_clock_lane(struct csi2_dev *csi2)
>  {
>  	u32 reg;
>  	int ret;
>  
>  	ret = readl_poll_timeout(csi2->base + CSI2_PHY_STATE, reg,
>  				 (reg & PHY_RXCLKACTIVEHS), 0, 500000);
> -	if (ret) {
> -		v4l2_err(&csi2->sd, "clock lane timeout, phy_state = 0x%08x\n",
> -			 reg);
> -		return ret;
> -	}
> -
> -	return 0;
> +	if (ret)
> +		v4l2_warn(&csi2->sd, "clock lane timeout, phy_state = 0x%08x\n",
> +			  reg);
>  }
>  
>  /* Setup the i.MX CSI2IPU Gasket */
> @@ -316,9 +308,7 @@ static int csi2_start(struct csi2_dev *csi2)
>  	csi2_enable(csi2, true);
>  
>  	/* Step 5 */
> -	ret = csi2_dphy_wait_stopstate(csi2);
> -	if (ret)
> -		goto err_assert_reset;
> +	csi2_dphy_wait_stopstate(csi2);
>  
>  	/* Step 6 */
>  	ret = v4l2_subdev_call(csi2->src_sd, video, s_stream, 1);
> @@ -327,14 +317,9 @@ static int csi2_start(struct csi2_dev *csi2)
>  		goto err_assert_reset;
>  
>  	/* Step 7 */
> -	ret = csi2_dphy_wait_clock_lane(csi2);
> -	if (ret)
> -		goto err_stop_upstream;
> -
> +	csi2_dphy_wait_clock_lane(csi2);
>  	return 0;
>  
> -err_stop_upstream:
> -	v4l2_subdev_call(csi2->src_sd, video, s_stream, 0);
>  err_assert_reset:
>  	csi2_enable(csi2, false);
>  err_disable_clk:

Re: [PATCH] media: imx: mipi csi-2: Don't fail if initial state times-out

[Fabio Estevam]

Hi Philipp,

On Wed, Jun 26, 2019 at 4:45 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:

> I think the messages could use a note that they may be due to a sensor
> or sensor driver bug, and that there might be image artifacts or
> outright failure to capture as a consequence.

Yes, this a good idea.

With this patch I could successfully test camera capture on a
imx6dl-wandboard connected to a OV5645.

Without this patch the Gsteamer pipeline fails.

Tested-by: Fabio Estevam <festevam@gmail.com>

Re: [PATCH] media: imx: mipi csi-2: Don't fail if initial state times-out

[Steve Longerbeam]

On 6/26/19 12:53 PM, Fabio Estevam wrote:
> Hi Philipp,
>
> On Wed, Jun 26, 2019 at 4:45 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
>> I think the messages could use a note that they may be due to a sensor
>> or sensor driver bug, and that there might be image artifacts or
>> outright failure to capture as a consequence.
> Yes, this a good idea.
>
> With this patch I could successfully test camera capture on a
> imx6dl-wandboard connected to a OV5645.
>
> Without this patch the Gsteamer pipeline fails.

Did you only get the LP-11 timeout warning message with this patch on 
the OV5645, or both the LP-11 timeout and clock lane timeout warnings? 
As I said it's OK to relax the LP-11 timeout to be warning only, but 
clock lane timeout should remain an error, since without an active clock 
on the clock lane it's guaranteed that no data will be received from the 
sensor.

Steve

>
> Tested-by: Fabio Estevam <festevam@gmail.com>

Re: [PATCH] media: imx: mipi csi-2: Don't fail if initial state times-out

[Fabio Estevam]

Hi Steve,

On Wed, Jun 26, 2019 at 6:19 PM Steve Longerbeam <slongerbeam@gmail.com> wrote:

> Did you only get the LP-11 timeout warning message with this patch on
> the OV5645, or both the LP-11 timeout and clock lane timeout warnings?

With this patch applied I get only the LP-11 timeout warnings, not
clock lane timeouts.

Thanks

Re: [PATCH] media: imx: mipi csi-2: Don't fail if initial state times-out

[Steve Longerbeam]

Hi Fabio,

On 6/26/19 4:22 PM, Fabio Estevam wrote:
> Hi Steve,
>
> On Wed, Jun 26, 2019 at 6:19 PM Steve Longerbeam <slongerbeam@gmail.com> wrote:
>
>> Did you only get the LP-11 timeout warning message with this patch on
>> the OV5645, or both the LP-11 timeout and clock lane timeout warnings?
> With this patch applied I get only the LP-11 timeout warnings, not
> clock lane timeouts.

Ok thanks for the confirmation that the imx6 CSI-2 receiver is able to 
successfully move to stream on without seeing the LP-11 state in this 
case. So in my opinion the next version of this patch should make LP-11 
timeout a warning only, but keep the error return on clock lane timeouts.

Steve

Re: [PATCH] media: imx: mipi csi-2: Don't fail if initial state times-out

[Philipp Zabel]

On Wed, 2019-06-26 at 16:29 -0700, Steve Longerbeam wrote:
> Hi Fabio,
> 
> On 6/26/19 4:22 PM, Fabio Estevam wrote:
> > Hi Steve,
> > 
> > On Wed, Jun 26, 2019 at 6:19 PM Steve Longerbeam <slongerbeam@gmail.com> wrote:
> > 
> > > Did you only get the LP-11 timeout warning message with this patch on
> > > the OV5645, or both the LP-11 timeout and clock lane timeout warnings?
> > 
> > With this patch applied I get only the LP-11 timeout warnings, not
> > clock lane timeouts.
> 
> Ok thanks for the confirmation that the imx6 CSI-2 receiver is able to 
> successfully move to stream on without seeing the LP-11 state in this 
> case.

Are there any visual artifacts in the first frame(s) in this case?

> So in my opinion the next version of this patch should make LP-11 
> timeout a warning only, but keep the error return on clock lane timeouts.

I agree.

regards
Philipp

Re: [PATCH] media: imx: mipi csi-2: Don't fail if initial state times-out

[Fabio Estevam]

Hi Philipp,

On Thu, Jun 27, 2019 at 5:43 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:

> Are there any visual artifacts in the first frame(s) in this case?

I do not observe visual artifacts when running gst-launch-1.0 v4l2src ! kmssink

> > So in my opinion the next version of this patch should make LP-11
> > timeout a warning only, but keep the error return on clock lane timeouts.
>
> I agree.

Here is a reworked version of Ezequiel's patch as per the suggestions:
http://code.bulix.org/g5qap5-780475

Does this one look good?

Thanks

Re: [PATCH] media: imx: mipi csi-2: Don't fail if initial state times-out

[Philipp Zabel]

Hi Fabio,

On Thu, 2019-06-27 at 09:38 -0300, Fabio Estevam wrote:
> Hi Philipp,
> 
> On Thu, Jun 27, 2019 at 5:43 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> 
> > Are there any visual artifacts in the first frame(s) in this case?
> 
> I do not observe visual artifacts when running gst-launch-1.0 v4l2src ! kmssink
> 
> > > So in my opinion the next version of this patch should make LP-11
> > > timeout a warning only, but keep the error return on clock lane timeouts.
> > 
> > I agree.
> 
> Here is a reworked version of Ezequiel's patch as per the suggestions:
> http://code.bulix.org/g5qap5-780475
> 
> Does this one look good?

Limiting the change to wait_stopstate is fine, the actual message
makes assumptions that could be misleading. How about:

"Timeout waiting for LP-11 state on all active lanes.
 This is most likely caused by a bug in the sensor driver.
 Capture might fail or contain visual artifacts."

I'd like to keep the phy_state register output though, if only as
dev_dbg(). It contains useful output for debugging, for example if only
some of the lanes are in stop state, which could indicate an issue with
connections or lane configuration.

regards
Philipp

Re: [PATCH] media: imx: mipi csi-2: Don't fail if initial state times-out

[Ezequiel Garcia]

On Thu, 2019-06-27 at 14:56 +0200, Philipp Zabel wrote:
> Hi Fabio,
> 
> On Thu, 2019-06-27 at 09:38 -0300, Fabio Estevam wrote:
> > Hi Philipp,
> > 
> > On Thu, Jun 27, 2019 at 5:43 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > 
> > > Are there any visual artifacts in the first frame(s) in this case?
> > 
> > I do not observe visual artifacts when running gst-launch-1.0 v4l2src ! kmssink
> > 
> > > > So in my opinion the next version of this patch should make LP-11
> > > > timeout a warning only, but keep the error return on clock lane timeouts.
> > > 
> > > I agree.
> > 
> > Here is a reworked version of Ezequiel's patch as per the suggestions:
> > http://code.bulix.org/g5qap5-780475
> > 
> > Does this one look good?
> 
> Limiting the change to wait_stopstate is fine, the actual message
> makes assumptions that could be misleading. How about:
> 
> "Timeout waiting for LP-11 state on all active lanes.
>  This is most likely caused by a bug in the sensor driver.
>  Capture might fail or contain visual artifacts."
> 
> I'd like to keep the phy_state register output though, if only as
> dev_dbg(). It contains useful output for debugging, for example if only
> some of the lanes are in stop state, which could indicate an issue with
> connections or lane configuration.
> 
> 

I think Philipp's suggestions looks very good, both the text and keeping
the phy state. I think both should be kept in the warning.

Fabio: feel free to submit a v2, or let me know so I'll add it to my TODO.

Thanks,
Eze

Re: [PATCH] media: imx: mipi csi-2: Don't fail if initial state times-out

[Fabio Estevam]

On Thu, Jun 27, 2019 at 3:45 PM Ezequiel Garcia <ezequiel@collabora.com> wrote:

> I think Philipp's suggestions looks very good, both the text and keeping
> the phy state. I think both should be kept in the warning.
>
> Fabio: feel free to submit a v2, or let me know so I'll add it to my TODO.

I have just sent a v2 with Philipp's suggestions. Thanks

Re: [PATCH] media: imx: mipi csi-2: Don't fail if initial state times-out

[Steve Longerbeam]

On 6/27/19 5:56 AM, Philipp Zabel wrote:
> Hi Fabio,
>
> On Thu, 2019-06-27 at 09:38 -0300, Fabio Estevam wrote:
>> Hi Philipp,
>>
>> On Thu, Jun 27, 2019 at 5:43 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>>
>>> Are there any visual artifacts in the first frame(s) in this case?
>> I do not observe visual artifacts when running gst-launch-1.0 v4l2src ! kmssink
>>
>>>> So in my opinion the next version of this patch should make LP-11
>>>> timeout a warning only, but keep the error return on clock lane timeouts.
>>> I agree.
>> Here is a reworked version of Ezequiel's patch as per the suggestions:
>> http://code.bulix.org/g5qap5-780475
>>
>> Does this one look good?
> Limiting the change to wait_stopstate is fine, the actual message
> makes assumptions that could be misleading. How about:
>
> "Timeout waiting for LP-11 state on all active lanes.
>   This is most likely caused by a bug in the sensor driver.
>   Capture might fail or contain visual artifacts."

Yes I agree that is more descriptive, if a bit wordy for a kernel error 
message. I think it could be reduced, something like:

"LP-11 wait timeout on all lanes, likely a sensor driver bug, expect 
capture failures."


>
> I'd like to keep the phy_state register output though, if only as
> dev_dbg(). It contains useful output for debugging, for example if only
> some of the lanes are in stop state, which could indicate an issue with
> connections or lane configuration.

Agreed!

Steve

Re: [PATCH] media: imx: mipi csi-2: Don't fail if initial state times-out

[Philipp Zabel]

On Thu, 2019-06-27 at 15:12 -0700, Steve Longerbeam wrote:
> 
> On 6/27/19 5:56 AM, Philipp Zabel wrote:
> > Hi Fabio,
> > 
> > On Thu, 2019-06-27 at 09:38 -0300, Fabio Estevam wrote:
> > > Hi Philipp,
> > > 
> > > On Thu, Jun 27, 2019 at 5:43 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > > 
> > > > Are there any visual artifacts in the first frame(s) in this case?
> > > 
> > > I do not observe visual artifacts when running gst-launch-1.0 v4l2src ! kmssink
> > > 
> > > > > So in my opinion the next version of this patch should make LP-11
> > > > > timeout a warning only, but keep the error return on clock lane timeouts.
> > > > 
> > > > I agree.
> > > 
> > > Here is a reworked version of Ezequiel's patch as per the suggestions:
> > > http://code.bulix.org/g5qap5-780475
> > > 
> > > Does this one look good?
> > 
> > Limiting the change to wait_stopstate is fine, the actual message
> > makes assumptions that could be misleading. How about:
> > 
> > "Timeout waiting for LP-11 state on all active lanes.
> >   This is most likely caused by a bug in the sensor driver.
> >   Capture might fail or contain visual artifacts."
> 
> Yes I agree that is more descriptive, if a bit wordy for a kernel error 
> message.

Haha, yes, I remember that thought crossing my mind.

>  I think it could be reduced, something like:
>
> "LP-11 wait timeout on all lanes, likely a sensor driver bug, expect 
> capture failures."

Much better, thanks.

regards
Philipp

Re: [PATCH] media: imx: mipi csi-2: Don't fail if initial state times-out

[Ezequiel Garcia]

On Thu, 2019-06-27 at 10:43 +0200, Philipp Zabel wrote:
> On Wed, 2019-06-26 at 16:29 -0700, Steve Longerbeam wrote:
> > Hi Fabio,
> > 
> > On 6/26/19 4:22 PM, Fabio Estevam wrote:
> > > Hi Steve,
> > > 
> > > On Wed, Jun 26, 2019 at 6:19 PM Steve Longerbeam <slongerbeam@gmail.com> wrote:
> > > 
> > > > Did you only get the LP-11 timeout warning message with this patch on
> > > > the OV5645, or both the LP-11 timeout and clock lane timeout warnings?
> > > 
> > > With this patch applied I get only the LP-11 timeout warnings, not
> > > clock lane timeouts.
> > 
> > Ok thanks for the confirmation that the imx6 CSI-2 receiver is able to 
> > successfully move to stream on without seeing the LP-11 state in this 
> > case.
> 
> Are there any visual artifacts in the first frame(s) in this case?
> 

I'll check.

> > So in my opinion the next version of this patch should make LP-11 
> > timeout a warning only, but keep the error return on clock lane timeouts.
> 
> I agree.
> 

Right. I only saw the LP-11 timeout, and wasn't really sure about the
clock lane timeout. I'll drop that, improve the warning message and submit a v2.

Thanks,
Ezequiel 

Re: [PATCH] media: imx: mipi csi-2: Don't fail if initial state times-out

[Laurent Pinchart]

Hi Ezequiel,

Thank you for the patch.

On Tue, Jun 25, 2019 at 05:39:45PM -0300, Ezequiel Garcia wrote:
> Not all sensors will be able to guarantee a proper initial state.
> This may be either because the driver is not properly written,
> or (probably unlikely) because the hardware won't support it.
> 
> While the right solution in the former case is to fix the sensor
> driver, the real world not always allows right solutions, due to lack
> of available documentation and support on these sensors.
> 
> Let's relax this requirement, and allow the driver to support stream start,
> even if the sensor initial sequence wasn't the expected.
> A warning is still emitted, so users should be hinted that something is off.

I'm not sure this is a very good idea. Failure to detect the LP-11 state
may mean that the sensor is completely powered off, but it may also mean
that it is already streaming data. I don't know how the CSI-2 receiver
state machine will operate in the first case, but in the second case it
will not be able to synchronise to the incoming stream, so it won't work
anyway.

I think you should instead fix the problem in the sensor driver, as you
hinted. Relaxing the requirement here will only make it more confusing,
it's a hack, and isn't portable across CSI-2 receivers. The same buggy
sensor driver won't work with other CSI-2 receivers whose internal state
machine require starting in the LP-11 state.

Which sensor are you using ?

> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  drivers/staging/media/imx/imx6-mipi-csi2.c | 33 ++++++----------------
>  1 file changed, 9 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c
> index f29e28df36ed..10342434e797 100644
> --- a/drivers/staging/media/imx/imx6-mipi-csi2.c
> +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c
> @@ -243,7 +243,7 @@ static int __maybe_unused csi2_dphy_wait_ulp(struct csi2_dev *csi2)
>  }
>  
>  /* Waits for low-power LP-11 state on data and clock lanes. */
> -static int csi2_dphy_wait_stopstate(struct csi2_dev *csi2)
> +static void csi2_dphy_wait_stopstate(struct csi2_dev *csi2)
>  {
>  	u32 mask, reg;
>  	int ret;
> @@ -253,29 +253,21 @@ static int csi2_dphy_wait_stopstate(struct csi2_dev *csi2)
>  
>  	ret = readl_poll_timeout(csi2->base + CSI2_PHY_STATE, reg,
>  				 (reg & mask) == mask, 0, 500000);
> -	if (ret) {
> -		v4l2_err(&csi2->sd, "LP-11 timeout, phy_state = 0x%08x\n", reg);
> -		return ret;
> -	}
> -
> -	return 0;
> +	if (ret)
> +		v4l2_warn(&csi2->sd, "LP-11 timeout, phy_state = 0x%08x\n", reg);
>  }
>  
>  /* Wait for active clock on the clock lane. */
> -static int csi2_dphy_wait_clock_lane(struct csi2_dev *csi2)
> +static void csi2_dphy_wait_clock_lane(struct csi2_dev *csi2)
>  {
>  	u32 reg;
>  	int ret;
>  
>  	ret = readl_poll_timeout(csi2->base + CSI2_PHY_STATE, reg,
>  				 (reg & PHY_RXCLKACTIVEHS), 0, 500000);
> -	if (ret) {
> -		v4l2_err(&csi2->sd, "clock lane timeout, phy_state = 0x%08x\n",
> -			 reg);
> -		return ret;
> -	}
> -
> -	return 0;
> +	if (ret)
> +		v4l2_warn(&csi2->sd, "clock lane timeout, phy_state = 0x%08x\n",
> +			  reg);
>  }
>  
>  /* Setup the i.MX CSI2IPU Gasket */
> @@ -316,9 +308,7 @@ static int csi2_start(struct csi2_dev *csi2)
>  	csi2_enable(csi2, true);
>  
>  	/* Step 5 */
> -	ret = csi2_dphy_wait_stopstate(csi2);
> -	if (ret)
> -		goto err_assert_reset;
> +	csi2_dphy_wait_stopstate(csi2);
>  
>  	/* Step 6 */
>  	ret = v4l2_subdev_call(csi2->src_sd, video, s_stream, 1);
> @@ -327,14 +317,9 @@ static int csi2_start(struct csi2_dev *csi2)
>  		goto err_assert_reset;
>  
>  	/* Step 7 */
> -	ret = csi2_dphy_wait_clock_lane(csi2);
> -	if (ret)
> -		goto err_stop_upstream;
> -
> +	csi2_dphy_wait_clock_lane(csi2);
>  	return 0;
>  
> -err_stop_upstream:
> -	v4l2_subdev_call(csi2->src_sd, video, s_stream, 0);
>  err_assert_reset:
>  	csi2_enable(csi2, false);
>  err_disable_clk:

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: imx: mipi csi-2: Don't fail if initial state times-out

[Ezequiel Garcia]

Hi Laurent, Philipp,

Thank you for the prompt review! I was pretty sure this would
raise your wise eyebrows :-)

On Wed, 2019-06-26 at 11:00 +0300, Laurent Pinchart wrote:
> Hi Ezequiel,
> 
> Thank you for the patch.
> 
> On Tue, Jun 25, 2019 at 05:39:45PM -0300, Ezequiel Garcia wrote:
> > Not all sensors will be able to guarantee a proper initial state.
> > This may be either because the driver is not properly written,
> > or (probably unlikely) because the hardware won't support it.
> > 
> > While the right solution in the former case is to fix the sensor
> > driver, the real world not always allows right solutions, due to lack
> > of available documentation and support on these sensors.
> > 
> > Let's relax this requirement, and allow the driver to support stream start,
> > even if the sensor initial sequence wasn't the expected.
> > A warning is still emitted, so users should be hinted that something is off.
> 
> I'm not sure this is a very good idea. Failure to detect the LP-11 state
> may mean that the sensor is completely powered off, but it may also mean
> that it is already streaming data. I don't know how the CSI-2 receiver
> state machine will operate in the first case, but in the second case it
> will not be able to synchronise to the incoming stream, so it won't work
> anyway.
> 
> I think you should instead fix the problem in the sensor driver, as you
> hinted. 

Sure, we all agree that the sensor fix is the right solution.

> Relaxing the requirement here will only make it more confusing,
> it's a hack, and isn't portable across CSI-2 receivers. 

We can emit a warning as suggested by Philipp, stating that the sensor
is buggy and needs fixing.

> The same buggy
> sensor driver won't work with other CSI-2 receivers whose internal state
> machine require starting in the LP-11 state.
> 

Right. But the same buggy sensor driver probaly does work already with other
CSI-2 receivers, hence why it hasn't been detected when it was submitted.

> Which sensor are you using ?
> 

I noticed this problem on OV5645 on a Wandoboard. Looks to be
basically the same issue as the one Jacopo fixed here:

Author: Jacopo Mondi <jacopo@jmondi.org>
Date:   Fri Jul 6 05:51:52 2018 -0400

    media: ov5640: Re-work MIPI startup sequence

I fixed it for OV5645 (and will submit soon), but that is
not the point.

The point is that buggy drivers exist, and while I'd love
to fix them all, it won't be always possible (due to lack of
datasheets, and due to some of these sensors having no active
maintainer).

Without this commit, such a buggy sensor will not work for sure;
while with the commit, there is a chance it will work.

Why would we prevent the latter from happening?

Thanks,
Eze

Re: [PATCH] media: imx: mipi csi-2: Don't fail if initial state times-out

[Steve Longerbeam]

Hi Laurent,

On 6/26/19 1:00 AM, Laurent Pinchart wrote:
> Hi Ezequiel,
>
> Thank you for the patch.
>
> On Tue, Jun 25, 2019 at 05:39:45PM -0300, Ezequiel Garcia wrote:
>> Not all sensors will be able to guarantee a proper initial state.
>> This may be either because the driver is not properly written,
>> or (probably unlikely) because the hardware won't support it.
>>
>> While the right solution in the former case is to fix the sensor
>> driver, the real world not always allows right solutions, due to lack
>> of available documentation and support on these sensors.
>>
>> Let's relax this requirement, and allow the driver to support stream start,
>> even if the sensor initial sequence wasn't the expected.
>> A warning is still emitted, so users should be hinted that something is off.
> I'm not sure this is a very good idea. Failure to detect the LP-11 state
> may mean that the sensor is completely powered off, but it may also mean
> that it is already streaming data. I don't know how the CSI-2 receiver
> state machine will operate in the first case, but in the second case it
> will not be able to synchronise to the incoming stream, so it won't work
> anyway.

 From my experience, at least with the OV5640 and the DS90Ux940, it can 
be difficult to coax some CSI-2 transmitters into a quiescent LP-11 bus 
state after power on, and yet the CSI-2 receiver state machine, at least 
in the imx6, is able to move forward to stream on anyway. So I think it 
makes sense to at least relax the LP-11 requirement at stream on for the 
imx6 receiver driver, and print a warning. But on second thought I agree 
the active clock lane requirement before stream on needs to remain.

In the second case you point out above (sensor is already actively 
streaming at stream on), why do you think that the receiver will not be 
able to synchronize?


>
> I think you should instead fix the problem in the sensor driver, as you
> hinted. Relaxing the requirement here will only make it more confusing,
> it's a hack, and isn't portable across CSI-2 receivers. The same buggy
> sensor driver won't work with other CSI-2 receivers whose internal state
> machine require starting in the LP-11 state.

Agreed, if it's possible the sensor driver should be fixed to enter 
LP-11 at power on.

Steve

> Which sensor are you using ?
>
>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
>> ---
>>   drivers/staging/media/imx/imx6-mipi-csi2.c | 33 ++++++----------------
>>   1 file changed, 9 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c
>> index f29e28df36ed..10342434e797 100644
>> --- a/drivers/staging/media/imx/imx6-mipi-csi2.c
>> +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c
>> @@ -243,7 +243,7 @@ static int __maybe_unused csi2_dphy_wait_ulp(struct csi2_dev *csi2)
>>   }
>>   
>>   /* Waits for low-power LP-11 state on data and clock lanes. */
>> -static int csi2_dphy_wait_stopstate(struct csi2_dev *csi2)
>> +static void csi2_dphy_wait_stopstate(struct csi2_dev *csi2)
>>   {
>>   	u32 mask, reg;
>>   	int ret;
>> @@ -253,29 +253,21 @@ static int csi2_dphy_wait_stopstate(struct csi2_dev *csi2)
>>   
>>   	ret = readl_poll_timeout(csi2->base + CSI2_PHY_STATE, reg,
>>   				 (reg & mask) == mask, 0, 500000);
>> -	if (ret) {
>> -		v4l2_err(&csi2->sd, "LP-11 timeout, phy_state = 0x%08x\n", reg);
>> -		return ret;
>> -	}
>> -
>> -	return 0;
>> +	if (ret)
>> +		v4l2_warn(&csi2->sd, "LP-11 timeout, phy_state = 0x%08x\n", reg);
>>   }
>>   
>>   /* Wait for active clock on the clock lane. */
>> -static int csi2_dphy_wait_clock_lane(struct csi2_dev *csi2)
>> +static void csi2_dphy_wait_clock_lane(struct csi2_dev *csi2)
>>   {
>>   	u32 reg;
>>   	int ret;
>>   
>>   	ret = readl_poll_timeout(csi2->base + CSI2_PHY_STATE, reg,
>>   				 (reg & PHY_RXCLKACTIVEHS), 0, 500000);
>> -	if (ret) {
>> -		v4l2_err(&csi2->sd, "clock lane timeout, phy_state = 0x%08x\n",
>> -			 reg);
>> -		return ret;
>> -	}
>> -
>> -	return 0;
>> +	if (ret)
>> +		v4l2_warn(&csi2->sd, "clock lane timeout, phy_state = 0x%08x\n",
>> +			  reg);
>>   }
>>   
>>   /* Setup the i.MX CSI2IPU Gasket */
>> @@ -316,9 +308,7 @@ static int csi2_start(struct csi2_dev *csi2)
>>   	csi2_enable(csi2, true);
>>   
>>   	/* Step 5 */
>> -	ret = csi2_dphy_wait_stopstate(csi2);
>> -	if (ret)
>> -		goto err_assert_reset;
>> +	csi2_dphy_wait_stopstate(csi2);
>>   
>>   	/* Step 6 */
>>   	ret = v4l2_subdev_call(csi2->src_sd, video, s_stream, 1);
>> @@ -327,14 +317,9 @@ static int csi2_start(struct csi2_dev *csi2)
>>   		goto err_assert_reset;
>>   
>>   	/* Step 7 */
>> -	ret = csi2_dphy_wait_clock_lane(csi2);
>> -	if (ret)
>> -		goto err_stop_upstream;
>> -
>> +	csi2_dphy_wait_clock_lane(csi2);
>>   	return 0;
>>   
>> -err_stop_upstream:
>> -	v4l2_subdev_call(csi2->src_sd, video, s_stream, 0);
>>   err_assert_reset:
>>   	csi2_enable(csi2, false);
>>   err_disable_clk:

Re: [PATCH] media: imx: mipi csi-2: Don't fail if initial state times-out

[Jacopo Mondi]

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

Hi Ezequiel,

On Tue, Jun 25, 2019 at 05:39:45PM -0300, Ezequiel Garcia wrote:
> Not all sensors will be able to guarantee a proper initial state.
> This may be either because the driver is not properly written,
> or (probably unlikely) because the hardware won't support it.
>
> While the right solution in the former case is to fix the sensor
> driver, the real world not always allows right solutions, due to lack
> of available documentation and support on these sensors.
>

We hit this in the past with the ov5640 sensor, whose driver was not
properly initializing its data lanes in LP-11 state, so yes, I'm not
surprised other sensors might fail in the same way.

Do you get frames after you hit the error? I recall it was not
possible with ov5640, even with something similar to what you've done
here in the CSI-2 receiver driver.

> Let's relax this requirement, and allow the driver to support stream start,
> even if the sensor initial sequence wasn't the expected.
> A warning is still emitted, so users should be hinted that something is off.
>

It seems you're also silencing errors related to clock lane detection.
I would mention that.

> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  drivers/staging/media/imx/imx6-mipi-csi2.c | 33 ++++++----------------
>  1 file changed, 9 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c
> index f29e28df36ed..10342434e797 100644
> --- a/drivers/staging/media/imx/imx6-mipi-csi2.c
> +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c
> @@ -243,7 +243,7 @@ static int __maybe_unused csi2_dphy_wait_ulp(struct csi2_dev *csi2)
>  }
>
>  /* Waits for low-power LP-11 state on data and clock lanes. */
> -static int csi2_dphy_wait_stopstate(struct csi2_dev *csi2)
> +static void csi2_dphy_wait_stopstate(struct csi2_dev *csi2)
>  {
>  	u32 mask, reg;
>  	int ret;
> @@ -253,29 +253,21 @@ static int csi2_dphy_wait_stopstate(struct csi2_dev *csi2)
>
>  	ret = readl_poll_timeout(csi2->base + CSI2_PHY_STATE, reg,
>  				 (reg & mask) == mask, 0, 500000);
> -	if (ret) {
> -		v4l2_err(&csi2->sd, "LP-11 timeout, phy_state = 0x%08x\n", reg);
> -		return ret;
> -	}
> -
> -	return 0;
> +	if (ret)
> +		v4l2_warn(&csi2->sd, "LP-11 timeout, phy_state = 0x%08x\n", reg);
>  }
>
>  /* Wait for active clock on the clock lane. */
> -static int csi2_dphy_wait_clock_lane(struct csi2_dev *csi2)
> +static void csi2_dphy_wait_clock_lane(struct csi2_dev *csi2)
>  {
>  	u32 reg;
>  	int ret;
>
>  	ret = readl_poll_timeout(csi2->base + CSI2_PHY_STATE, reg,
>  				 (reg & PHY_RXCLKACTIVEHS), 0, 500000);
> -	if (ret) {
> -		v4l2_err(&csi2->sd, "clock lane timeout, phy_state = 0x%08x\n",
> -			 reg);
> -		return ret;
> -	}
> -
> -	return 0;
> +	if (ret)
> +		v4l2_warn(&csi2->sd, "clock lane timeout, phy_state = 0x%08x\n",
> +			  reg);
>  }
>
>  /* Setup the i.MX CSI2IPU Gasket */
> @@ -316,9 +308,7 @@ static int csi2_start(struct csi2_dev *csi2)
>  	csi2_enable(csi2, true);
>
>  	/* Step 5 */
> -	ret = csi2_dphy_wait_stopstate(csi2);
> -	if (ret)
> -		goto err_assert_reset;
> +	csi2_dphy_wait_stopstate(csi2);
>
>  	/* Step 6 */
>  	ret = v4l2_subdev_call(csi2->src_sd, video, s_stream, 1);
> @@ -327,14 +317,9 @@ static int csi2_start(struct csi2_dev *csi2)
>  		goto err_assert_reset;
>
>  	/* Step 7 */
> -	ret = csi2_dphy_wait_clock_lane(csi2);
> -	if (ret)
> -		goto err_stop_upstream;
> -
> +	csi2_dphy_wait_clock_lane(csi2);
>  	return 0;
>
> -err_stop_upstream:
> -	v4l2_subdev_call(csi2->src_sd, video, s_stream, 0);
>  err_assert_reset:
>  	csi2_enable(csi2, false);
>  err_disable_clk:
> --
> 2.20.1
>

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