[PATCH v3 1/2] media: imx: csi: Disable SMFC before disabling IDMA channel

[PATCH v3 1/2] media: imx: csi: Disable SMFC before disabling IDMA channel

[Steve Longerbeam]

Disable the SMFC before disabling the IDMA channel, instead of after,
in csi_idmac_unsetup().

This fixes a complete system hard lockup on the SabreAuto when streaming
from the ADV7180, by repeatedly sending a stream off immediately followed
by stream on:

while true; do v4l2-ctl  -d4 --stream-mmap --stream-count=3; done

Eventually this either causes the system lockup or EOF timeouts at all
subsequent stream on, until a system reset.

The lockup occurs when disabling the IDMA channel at stream off. Stopping
the video data stream entering the IDMA channel before disabling the
channel itself appears to be a reliable fix for the hard lockup. That can
be done either by disabling the SMFC or the CSI before disabling the
channel. Disabling the SMFC before the channel is the easiest solution,
so do that.

Fixes: 4a34ec8e470cb ("[media] media: imx: Add CSI subdev driver")

Suggested-by: Peter Seiderer <ps.report@gmx.net>
Reported-by: Gaël PORTAY <gael.portay@collabora.com>
Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
Cc: stable@vger.kernel.org
---
Changes in v3:
- switch from disabling the CSI before the channel to disabling the
  SMFC before the channel.
Changes in v2:
- restore an empty line
- Add Fixes: and Cc: stable
---
 drivers/staging/media/imx/imx-media-csi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index e18f58f56dfb..8610027eac97 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -560,8 +560,8 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
 static void csi_idmac_unsetup(struct csi_priv *priv,
 			      enum vb2_buffer_state state)
 {
-	ipu_idmac_disable_channel(priv->idmac_ch);
 	ipu_smfc_disable(priv->smfc);
+	ipu_idmac_disable_channel(priv->idmac_ch);
 
 	csi_idmac_unsetup_vb2_buf(priv, state);
 }
-- 
2.17.1

Re: [PATCH v3 1/2] media: imx: csi: Disable SMFC before disabling IDMA channel

[Philipp Zabel]

Hi,

On Fri, 2019-01-18 at 17:04 -0800, Steve Longerbeam wrote:
> Disable the SMFC before disabling the IDMA channel, instead of after,
> in csi_idmac_unsetup().
> 
> This fixes a complete system hard lockup on the SabreAuto when streaming
> from the ADV7180, by repeatedly sending a stream off immediately followed
> by stream on:
> 
> while true; do v4l2-ctl  -d4 --stream-mmap --stream-count=3; done
> 
> Eventually this either causes the system lockup or EOF timeouts at all
> subsequent stream on, until a system reset.
> 
> The lockup occurs when disabling the IDMA channel at stream off. Stopping
> the video data stream entering the IDMA channel before disabling the
> channel itself appears to be a reliable fix for the hard lockup. That can
> be done either by disabling the SMFC or the CSI before disabling the
> channel. Disabling the SMFC before the channel is the easiest solution,
> so do that.
> 
> Fixes: 4a34ec8e470cb ("[media] media: imx: Add CSI subdev driver")
> 
> Suggested-by: Peter Seiderer <ps.report@gmx.net>
> Reported-by: Gaël PORTAY <gael.portay@collabora.com>
> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>

Gaël, could we get a Tested-by: for this as well?

> Cc: stable@vger.kernel.org
> ---
> Changes in v3:
> - switch from disabling the CSI before the channel to disabling the
>   SMFC before the channel.
> Changes in v2:
> - restore an empty line
> - Add Fixes: and Cc: stable
> ---
>  drivers/staging/media/imx/imx-media-csi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> index e18f58f56dfb..8610027eac97 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -560,8 +560,8 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
>  static void csi_idmac_unsetup(struct csi_priv *priv,
>  			      enum vb2_buffer_state state)
>  {
> -	ipu_idmac_disable_channel(priv->idmac_ch);
>  	ipu_smfc_disable(priv->smfc);
> +	ipu_idmac_disable_channel(priv->idmac_ch);

Steve, do you have any theory why this helps? It's a bit weird to
disable the SMFC module while the DMA channel is still enabled. I think
this warrants a big comment, given that enable order is SMFC_EN before
IDMAC channel enable.

Also ipu_smfc_disable is refcounted, so if the other CSI is capturing
simultaneously, this change has no effect.

FWIW, I don't see any regressions though.

regards
Philipp

Re: [PATCH v3 1/2] media: imx: csi: Disable SMFC before disabling IDMA channel

[Gaël PORTAY]

Philipp,

On Mon, Jan 21, 2019 at 12:49:10PM +0100, Philipp Zabel wrote:
> Hi,
> 
> On Fri, 2019-01-18 at 17:04 -0800, Steve Longerbeam wrote:
> > Disable the SMFC before disabling the IDMA channel, instead of after,
> > in csi_idmac_unsetup().
> > 
> > This fixes a complete system hard lockup on the SabreAuto when streaming
> > from the ADV7180, by repeatedly sending a stream off immediately followed
> > by stream on:
> > 
> > while true; do v4l2-ctl  -d4 --stream-mmap --stream-count=3; done
> > 
> > Eventually this either causes the system lockup or EOF timeouts at all
> > subsequent stream on, until a system reset.
> > 
> > The lockup occurs when disabling the IDMA channel at stream off. Stopping
> > the video data stream entering the IDMA channel before disabling the
> > channel itself appears to be a reliable fix for the hard lockup. That can
> > be done either by disabling the SMFC or the CSI before disabling the
> > channel. Disabling the SMFC before the channel is the easiest solution,
> > so do that.
> > 
> > Fixes: 4a34ec8e470cb ("[media] media: imx: Add CSI subdev driver")
> > 
> > Suggested-by: Peter Seiderer <ps.report@gmx.net>
> > Reported-by: Gaël PORTAY <gael.portay@collabora.com>
> > Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
> 
> Gaël, could we get a Tested-by: for this as well?
>

I have not tested the v3 yet. I have planned to do it later this day for
a all night testing and report the result then.

Gael

Re: [PATCH v3 1/2] media: imx: csi: Disable SMFC before disabling IDMA channel

[Steve Longerbeam]

On 1/21/19 3:49 AM, Philipp Zabel wrote:
> Hi,
>
> On Fri, 2019-01-18 at 17:04 -0800, Steve Longerbeam wrote:
>> Disable the SMFC before disabling the IDMA channel, instead of after,
>> in csi_idmac_unsetup().
>>
>> This fixes a complete system hard lockup on the SabreAuto when streaming
>> from the ADV7180, by repeatedly sending a stream off immediately followed
>> by stream on:
>>
>> while true; do v4l2-ctl  -d4 --stream-mmap --stream-count=3; done
>>
>> Eventually this either causes the system lockup or EOF timeouts at all
>> subsequent stream on, until a system reset.
>>
>> The lockup occurs when disabling the IDMA channel at stream off. Stopping
>> the video data stream entering the IDMA channel before disabling the
>> channel itself appears to be a reliable fix for the hard lockup. That can
>> be done either by disabling the SMFC or the CSI before disabling the
>> channel. Disabling the SMFC before the channel is the easiest solution,
>> so do that.
>>
>> Fixes: 4a34ec8e470cb ("[media] media: imx: Add CSI subdev driver")
>>
>> Suggested-by: Peter Seiderer <ps.report@gmx.net>
>> Reported-by: Gaël PORTAY <gael.portay@collabora.com>
>> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
> Gaël, could we get a Tested-by: for this as well?
>
>> Cc: stable@vger.kernel.org
>> ---
>> Changes in v3:
>> - switch from disabling the CSI before the channel to disabling the
>>    SMFC before the channel.
>> Changes in v2:
>> - restore an empty line
>> - Add Fixes: and Cc: stable
>> ---
>>   drivers/staging/media/imx/imx-media-csi.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
>> index e18f58f56dfb..8610027eac97 100644
>> --- a/drivers/staging/media/imx/imx-media-csi.c
>> +++ b/drivers/staging/media/imx/imx-media-csi.c
>> @@ -560,8 +560,8 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
>>   static void csi_idmac_unsetup(struct csi_priv *priv,
>>   			      enum vb2_buffer_state state)
>>   {
>> -	ipu_idmac_disable_channel(priv->idmac_ch);
>>   	ipu_smfc_disable(priv->smfc);
>> +	ipu_idmac_disable_channel(priv->idmac_ch);
> Steve, do you have any theory why this helps? It's a bit weird to
> disable the SMFC module while the DMA channel is still enabled.

It does fix the hang, but I only have a vague theory as to why. That by 
disabling the DMA channel while its internal FIFO is getting filled is 
causing the hang, maybe due to a simultaneous update of the channel's 
internal FIFO write pointer and the channel disable bit. By disabling 
the SMFC (or the CSI), writes to the channel's internal FIFO stop.

>   I think
> this warrants a big comment, given that enable order is SMFC_EN before
> IDMAC channel enable.
>
> Also ipu_smfc_disable is refcounted, so if the other CSI is capturing
> simultaneously, this change has no effect.

Sigh, you're right. Let me go back to disabling the CSI before the 
channel, the CSI enable/disable is not refcounted (it doesn't need to be 
since it is single use) so it doesn't have this problem.

Should we drop this patch or keep it (with a big comment)? By only 
changing the disable order to "CSI then channel", the hang is reliably 
fixed from my and Gael's testing, but my concern is that by not 
disabling the SMFC before the channel, the SMFC could still empty its 
FIFO to the channel's internal FIFO and still create a hang.

Steve

Re: [PATCH v3 1/2] media: imx: csi: Disable SMFC before disabling IDMA channel

[Steve Longerbeam]

On 1/21/19 10:43 AM, Steve Longerbeam wrote:
>
>
> On 1/21/19 3:49 AM, Philipp Zabel wrote:
>> Also ipu_smfc_disable is refcounted, so if the other CSI is capturing
>> simultaneously, this change has no effect.
>
> Sigh, you're right. Let me go back to disabling the CSI before the 
> channel, the CSI enable/disable is not refcounted (it doesn't need to 
> be since it is single use) so it doesn't have this problem.
>
> Should we drop this patch or keep it (with a big comment)? By only 
> changing the disable order to "CSI then channel", the hang is reliably 
> fixed from my and Gael's testing, but my concern is that by not 
> disabling the SMFC before the channel, the SMFC could still empty its 
> FIFO to the channel's internal FIFO and still create a hang.

Well, as you said it will have no effect if both CSI's are streaming 
with the SMFC, in which case the danger would still exist. Perhaps it 
would be best to just drop this patch.

Steve

Re: [PATCH v3 1/2] media: imx: csi: Disable SMFC before disabling IDMA channel

[Philipp Zabel]

On Mon, 2019-01-21 at 10:46 -0800, Steve Longerbeam wrote:
> 
> On 1/21/19 10:43 AM, Steve Longerbeam wrote:
> > 
> > 
> > On 1/21/19 3:49 AM, Philipp Zabel wrote:
> > > Also ipu_smfc_disable is refcounted, so if the other CSI is capturing
> > > simultaneously, this change has no effect.
> > 
> > Sigh, you're right. Let me go back to disabling the CSI before the 
> > channel, the CSI enable/disable is not refcounted (it doesn't need to 
> > be since it is single use) so it doesn't have this problem.
> > 
> > Should we drop this patch or keep it (with a big comment)? By only 
> > changing the disable order to "CSI then channel", the hang is reliably 
> > fixed from my and Gael's testing, but my concern is that by not 
> > disabling the SMFC before the channel, the SMFC could still empty its 
> > FIFO to the channel's internal FIFO and still create a hang.
> 
> Well, as you said it will have no effect if both CSI's are streaming 
> with the SMFC, in which case the danger would still exist. Perhaps it 
> would be best to just drop this patch.

Hm, if we can't guarantee the intended effect with this patch, and
stopping the CSI first helps reliably, it's indeed better to just do
that instead.

regards
Philipp

Re: [PATCH v3 1/2] media: imx: csi: Disable SMFC before disabling IDMA channel

[Gaël PORTAY]

Philipp,

On Mon, Jan 21, 2019 at 12:49:10PM +0100, Philipp Zabel wrote:
> Hi,
> 
> On Fri, 2019-01-18 at 17:04 -0800, Steve Longerbeam wrote:
> > Disable the SMFC before disabling the IDMA channel, instead of after,
> > in csi_idmac_unsetup().
> > 
> > This fixes a complete system hard lockup on the SabreAuto when streaming
> > from the ADV7180, by repeatedly sending a stream off immediately followed
> > by stream on:
> > 
> > while true; do v4l2-ctl  -d4 --stream-mmap --stream-count=3; done
> > 
> > Eventually this either causes the system lockup or EOF timeouts at all
> > subsequent stream on, until a system reset.
> > 
> > The lockup occurs when disabling the IDMA channel at stream off. Stopping
> > the video data stream entering the IDMA channel before disabling the
> > channel itself appears to be a reliable fix for the hard lockup. That can
> > be done either by disabling the SMFC or the CSI before disabling the
> > channel. Disabling the SMFC before the channel is the easiest solution,
> > so do that.
> > 
> > Fixes: 4a34ec8e470cb ("[media] media: imx: Add CSI subdev driver")
> > 
> > Suggested-by: Peter Seiderer <ps.report@gmx.net>
> > Reported-by: Gaël PORTAY <gael.portay@collabora.com>
> > Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
> 
> Gaël, could we get a Tested-by: for this as well?
> 

I have tested the patchset v4 (not that one), and it has passed my test.

Regards,
Gael

[PATCH v3 2/2] media: imx: prpencvf: Stop upstream before disabling IDMA channel

[Steve Longerbeam]

Upstream must be stopped immediately after receiving the last EOF and
before disabling the IDMA channel. This can be accomplished by moving
upstream stream off to just after receiving the last EOF completion in
prp_stop(). For symmetry also move upstream stream on to end of
prp_start().

This fixes a complete system hard lockup on the SabreAuto when streaming
from the ADV7180, by repeatedly sending a stream off immediately followed
by stream on:

while true; do v4l2-ctl  -d1 --stream-mmap --stream-count=3; done

Eventually this either causes the system lockup or EOF timeouts at all
subsequent stream on, until a system reset.

The lockup occurs when disabling the IDMA channel at stream off. Stopping
the video data stream entering the IDMA channel before disabling the
channel itself appears to be a reliable fix for the hard lockup.

Fixes: f0d9c8924e2c3 ("[media] media: imx: Add IC subdev drivers")

Reported-by: Gaël PORTAY <gael.portay@collabora.com>
Tested-by: Gaël PORTAY <gael.portay@collabora.com>
Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
Cc: stable@vger.kernel.org
---
Changes in v3:
- Reword the commit subject and message. No functional changes.
Changes in v2:
- Add Fixes: and Cc: stable
---
 drivers/staging/media/imx/imx-ic-prpencvf.c | 26 ++++++++++++++-------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
index 33ada6612fee..f53cdb608528 100644
--- a/drivers/staging/media/imx/imx-ic-prpencvf.c
+++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
@@ -707,12 +707,23 @@ static int prp_start(struct prp_priv *priv)
 		goto out_free_nfb4eof_irq;
 	}
 
+	/* start upstream */
+	ret = v4l2_subdev_call(priv->src_sd, video, s_stream, 1);
+	ret = (ret && ret != -ENOIOCTLCMD) ? ret : 0;
+	if (ret) {
+		v4l2_err(&ic_priv->sd,
+			 "upstream stream on failed: %d\n", ret);
+		goto out_free_eof_irq;
+	}
+
 	/* start the EOF timeout timer */
 	mod_timer(&priv->eof_timeout_timer,
 		  jiffies + msecs_to_jiffies(IMX_MEDIA_EOF_TIMEOUT));
 
 	return 0;
 
+out_free_eof_irq:
+	devm_free_irq(ic_priv->dev, priv->eof_irq, priv);
 out_free_nfb4eof_irq:
 	devm_free_irq(ic_priv->dev, priv->nfb4eof_irq, priv);
 out_unsetup:
@@ -744,6 +755,12 @@ static void prp_stop(struct prp_priv *priv)
 	if (ret == 0)
 		v4l2_warn(&ic_priv->sd, "wait last EOF timeout\n");
 
+	/* stop upstream */
+	ret = v4l2_subdev_call(priv->src_sd, video, s_stream, 0);
+	if (ret && ret != -ENOIOCTLCMD)
+		v4l2_warn(&ic_priv->sd,
+			  "upstream stream off failed: %d\n", ret);
+
 	devm_free_irq(ic_priv->dev, priv->eof_irq, priv);
 	devm_free_irq(ic_priv->dev, priv->nfb4eof_irq, priv);
 
@@ -1174,15 +1191,6 @@ static int prp_s_stream(struct v4l2_subdev *sd, int enable)
 	if (ret)
 		goto out;
 
-	/* start/stop upstream */
-	ret = v4l2_subdev_call(priv->src_sd, video, s_stream, enable);
-	ret = (ret && ret != -ENOIOCTLCMD) ? ret : 0;
-	if (ret) {
-		if (enable)
-			prp_stop(priv);
-		goto out;
-	}
-
 update_count:
 	priv->stream_count += enable ? 1 : -1;
 	if (priv->stream_count < 0)
-- 
2.17.1