[PATCH v4 0/3] media: imx: Stop stream before disabling IDMA channels

[PATCH v4 0/3] media: imx: Stop stream before disabling IDMA channels

[Steve Longerbeam]

Repeatedly sending a stream off immediately followed by stream on can
eventually cause a complete system hard lockup on the SabreAuto when
streaming from the ADV7180:

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 channels 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.

In the CSI subdevice, this can be done by disabling the CSI before
disabling the CSI IDMA channel, instead of after. In the IC-PRPENVVF
subdevice, this can be done by stopping upstream before disabling the
PRPENC/VF IDMA channel.

History:
v4:
- Disabling SMFC will have no effect if both CSI's are streaming. So
  go back to disabling CSI before channel as in v2, but split up
  csi_idmac_stop such that ipu_csi_disable can still be called within
  csi_stop.

v3:
- Switch to disabling the SMFC before the channel, instead of the CSI
  before the channel.

v2:
- Whitespace fixes
- Add Fixes: and Cc: stable@vger.kernel.org
- No functional changes.


Steve Longerbeam (3):
  media: imx: csi: Disable CSI immediately after last EOF
  media: imx: csi: Stop upstream before disabling IDMA channel
  media: imx: prpencvf: Stop upstream before disabling IDMA channel

 drivers/staging/media/imx/imx-ic-prpencvf.c | 26 ++++++++-----
 drivers/staging/media/imx/imx-media-csi.c   | 42 +++++++++++++--------
 2 files changed, 44 insertions(+), 24 deletions(-)

-- 
2.17.1

[PATCH v4 1/3] media: imx: csi: Disable CSI immediately after last EOF

[Steve Longerbeam]

Disable the CSI immediately after receiving the last EOF before stream
off (and thus before disabling the IDMA channel). Do this by moving the
wait for EOF completion into a new function csi_idmac_wait_last_eof().

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. Disabling
the CSI before disabling the IDMA channel appears to be a reliable fix for
the hard lockup.

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

Reported-by: Gaël PORTAY <gael.portay@collabora.com>
Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
Cc: stable@vger.kernel.org
---
Changes in v4:
- Disabling SMFC will have no effect if both CSI's are streaming. So
  go back to disabling CSI before channel as in v2, but split up
  csi_idmac_stop such that ipu_csi_disable can still be called within
  csi_stop.
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 | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index 7abfe0aa1418..920e38885292 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -662,7 +662,7 @@ static int csi_idmac_start(struct csi_priv *priv)
 	return ret;
 }
 
-static void csi_idmac_stop(struct csi_priv *priv)
+static void csi_idmac_wait_last_eof(struct csi_priv *priv)
 {
 	unsigned long flags;
 	int ret;
@@ -679,7 +679,10 @@ static void csi_idmac_stop(struct csi_priv *priv)
 		&priv->last_eof_comp, msecs_to_jiffies(IMX_MEDIA_EOF_TIMEOUT));
 	if (ret == 0)
 		v4l2_warn(&priv->sd, "wait last EOF timeout\n");
+}
 
+static void csi_idmac_stop(struct csi_priv *priv)
+{
 	devm_free_irq(priv->dev, priv->eof_irq, priv);
 	devm_free_irq(priv->dev, priv->nfb4eof_irq, priv);
 
@@ -786,6 +789,16 @@ static int csi_start(struct csi_priv *priv)
 
 static void csi_stop(struct csi_priv *priv)
 {
+	if (priv->dest == IPU_CSI_DEST_IDMAC)
+		csi_idmac_wait_last_eof(priv);
+
+	/*
+	 * Disable the CSI asap, after syncing with the last EOF.
+	 * Doing so after the IDMA channel is disabled has shown to
+	 * create hard system-wide hangs.
+	 */
+	ipu_csi_disable(priv->csi);
+
 	if (priv->dest == IPU_CSI_DEST_IDMAC) {
 		csi_idmac_stop(priv);
 
@@ -793,8 +806,6 @@ static void csi_stop(struct csi_priv *priv)
 		if (priv->fim)
 			imx_media_fim_set_stream(priv->fim, NULL, false);
 	}
-
-	ipu_csi_disable(priv->csi);
 }
 
 static const struct csi_skip_desc csi_skip[12] = {
-- 
2.17.1

[PATCH v4 2/3] media: imx: csi: Stop upstream before disabling IDMA channel

[Steve Longerbeam]

Move upstream stream off to just after receiving the last EOF completion
and disabling the CSI (and thus before disabling the IDMA channel) in
csi_stop(). For symmetry also move upstream stream on to beginning of
csi_start().

Doing this makes csi_s_stream() more symmetric with prp_s_stream() which
will require the same change to fix a hard lockup.

Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
Cc: stable@vger.kernel.org
---
 drivers/staging/media/imx/imx-media-csi.c | 25 ++++++++++++-----------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index 920e38885292..d851ca2497b4 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -753,10 +753,16 @@ static int csi_start(struct csi_priv *priv)
 
 	output_fi = &priv->frame_interval[priv->active_output_pad];
 
+	/* start upstream */
+	ret = v4l2_subdev_call(priv->src_sd, video, s_stream, 1);
+	ret = (ret && ret != -ENOIOCTLCMD) ? ret : 0;
+	if (ret)
+		return ret;
+
 	if (priv->dest == IPU_CSI_DEST_IDMAC) {
 		ret = csi_idmac_start(priv);
 		if (ret)
-			return ret;
+			goto stop_upstream;
 	}
 
 	ret = csi_setup(priv);
@@ -784,6 +790,8 @@ static int csi_start(struct csi_priv *priv)
 idmac_stop:
 	if (priv->dest == IPU_CSI_DEST_IDMAC)
 		csi_idmac_stop(priv);
+stop_upstream:
+	v4l2_subdev_call(priv->src_sd, video, s_stream, 0);
 	return ret;
 }
 
@@ -799,6 +807,9 @@ static void csi_stop(struct csi_priv *priv)
 	 */
 	ipu_csi_disable(priv->csi);
 
+	/* stop upstream */
+	v4l2_subdev_call(priv->src_sd, video, s_stream, 0);
+
 	if (priv->dest == IPU_CSI_DEST_IDMAC) {
 		csi_idmac_stop(priv);
 
@@ -966,23 +977,13 @@ static int csi_s_stream(struct v4l2_subdev *sd, int enable)
 		goto update_count;
 
 	if (enable) {
-		/* upstream must be started first, before starting CSI */
-		ret = v4l2_subdev_call(priv->src_sd, video, s_stream, 1);
-		ret = (ret && ret != -ENOIOCTLCMD) ? ret : 0;
-		if (ret)
-			goto out;
-
 		dev_dbg(priv->dev, "stream ON\n");
 		ret = csi_start(priv);
-		if (ret) {
-			v4l2_subdev_call(priv->src_sd, video, s_stream, 0);
+		if (ret)
 			goto out;
-		}
 	} else {
 		dev_dbg(priv->dev, "stream OFF\n");
-		/* CSI must be stopped first, then stop upstream */
 		csi_stop(priv);
-		v4l2_subdev_call(priv->src_sd, video, s_stream, 0);
 	}
 
 update_count:
-- 
2.17.1

[PATCH v4 3/3] 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 v4:
- none.
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 053a911d477a..3637693c2bc8 100644
--- a/drivers/staging/media/imx/imx-ic-prpencvf.c
+++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
@@ -706,12 +706,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:
@@ -743,6 +754,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);
 
@@ -1173,15 +1190,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

Re: [PATCH v4 0/3] media: imx: Stop stream before disabling IDMA channels

[Hans Verkuil]

Hi Steve,

On 01/22/2019 12:35 AM, Steve Longerbeam wrote:
> Repeatedly sending a stream off immediately followed by stream on can
> eventually cause a complete system hard lockup on the SabreAuto when
> streaming from the ADV7180:
> 
> 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 channels 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.
> 
> In the CSI subdevice, this can be done by disabling the CSI before
> disabling the CSI IDMA channel, instead of after. In the IC-PRPENVVF
> subdevice, this can be done by stopping upstream before disabling the
> PRPENC/VF IDMA channel.

Please let me know when you are satisfied with this patch series and I
can make a pull request for this.

Thanks!

	Hans

> 
> History:
> v4:
> - Disabling SMFC will have no effect if both CSI's are streaming. So
>   go back to disabling CSI before channel as in v2, but split up
>   csi_idmac_stop such that ipu_csi_disable can still be called within
>   csi_stop.
> 
> v3:
> - Switch to disabling the SMFC before the channel, instead of the CSI
>   before the channel.
> 
> v2:
> - Whitespace fixes
> - Add Fixes: and Cc: stable@vger.kernel.org
> - No functional changes.
> 
> 
> Steve Longerbeam (3):
>   media: imx: csi: Disable CSI immediately after last EOF
>   media: imx: csi: Stop upstream before disabling IDMA channel
>   media: imx: prpencvf: Stop upstream before disabling IDMA channel
> 
>  drivers/staging/media/imx/imx-ic-prpencvf.c | 26 ++++++++-----
>  drivers/staging/media/imx/imx-media-csi.c   | 42 +++++++++++++--------
>  2 files changed, 44 insertions(+), 24 deletions(-)
> 

Re: [PATCH v4 0/3] media: imx: Stop stream before disabling IDMA channels

[Steve Longerbeam]

Hi Hans,

On 1/21/19 11:34 PM, Hans Verkuil wrote:
> Hi Steve,
>
> On 01/22/2019 12:35 AM, Steve Longerbeam wrote:
>> Repeatedly sending a stream off immediately followed by stream on can
>> eventually cause a complete system hard lockup on the SabreAuto when
>> streaming from the ADV7180:
>>
>> 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 channels 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.
>>
>> In the CSI subdevice, this can be done by disabling the CSI before
>> disabling the CSI IDMA channel, instead of after. In the IC-PRPENVVF
>> subdevice, this can be done by stopping upstream before disabling the
>> PRPENC/VF IDMA channel.
> Please let me know when you are satisfied with this patch series and I
> can make a pull request for this.

I'm satisfied with it now, it addresses Philipp's concerns from v3.

Also Gael reported successful testing.

Steve


>
> Thanks!
>
> 	Hans
>
>> History:
>> v4:
>> - Disabling SMFC will have no effect if both CSI's are streaming. So
>>    go back to disabling CSI before channel as in v2, but split up
>>    csi_idmac_stop such that ipu_csi_disable can still be called within
>>    csi_stop.
>>
>> v3:
>> - Switch to disabling the SMFC before the channel, instead of the CSI
>>    before the channel.
>>
>> v2:
>> - Whitespace fixes
>> - Add Fixes: and Cc: stable@vger.kernel.org
>> - No functional changes.
>>
>>
>> Steve Longerbeam (3):
>>    media: imx: csi: Disable CSI immediately after last EOF
>>    media: imx: csi: Stop upstream before disabling IDMA channel
>>    media: imx: prpencvf: Stop upstream before disabling IDMA channel
>>
>>   drivers/staging/media/imx/imx-ic-prpencvf.c | 26 ++++++++-----
>>   drivers/staging/media/imx/imx-media-csi.c   | 42 +++++++++++++--------
>>   2 files changed, 44 insertions(+), 24 deletions(-)
>>

Re: [PATCH v4 0/3] media: imx: Stop stream before disabling IDMA channels

[Gaël PORTAY]

Steve,

On Mon, Jan 21, 2019 at 03:35:49PM -0800, Steve Longerbeam wrote:
> Repeatedly sending a stream off immediately followed by stream on can
> eventually cause a complete system hard lockup on the SabreAuto when
> streaming from the ADV7180:
> 
> 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 channels 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.
> 
> In the CSI subdevice, this can be done by disabling the CSI before
> disabling the CSI IDMA channel, instead of after. In the IC-PRPENVVF
> subdevice, this can be done by stopping upstream before disabling the
> PRPENC/VF IDMA channel.
> 
> History:
> v4:
> - Disabling SMFC will have no effect if both CSI's are streaming. So
>   go back to disabling CSI before channel as in v2, but split up
>   csi_idmac_stop such that ipu_csi_disable can still be called within
>   csi_stop.
> 
> v3:
> - Switch to disabling the SMFC before the channel, instead of the CSI
>   before the channel.
> 
> v2:
> - Whitespace fixes
> - Add Fixes: and Cc: stable@vger.kernel.org
> - No functional changes.
> 
> 
> Steve Longerbeam (3):
>   media: imx: csi: Disable CSI immediately after last EOF
>   media: imx: csi: Stop upstream before disabling IDMA channel
>   media: imx: prpencvf: Stop upstream before disabling IDMA channel
> 
>  drivers/staging/media/imx/imx-ic-prpencvf.c | 26 ++++++++-----
>  drivers/staging/media/imx/imx-media-csi.c   | 42 +++++++++++++--------
>  2 files changed, 44 insertions(+), 24 deletions(-)
> 
> -- 
> 2.17.1
> 

The system is still up and running this morning after a whole night long
of tests.

You have my Tested-by.

Thanks,
Gael