[PATCH v2 0/4] rcar-csi2: Update handling of transfer error

[PATCH v2 0/4] rcar-csi2: Update handling of transfer error

[Niklas Söderlund]

Hello,

This series adapts the R-Car CSI-2 receiver recovery logic to match
updates in the datasheet. The later datasheets recommend that the whole
video pipeline shall be stopped if an transmission error is detected
instead of just restarting the CSI-2 receiver.

To do this we leverage the recent changes to support suspend/resume of
time the whole pipeline and inform the C-Car VIN driver of the detected
error so it can stop the whole pipeline and inform user-space of the
detected fault.

Patch 1/4 and 2/4 fixes faults in the VIN driver that where detected
when working on this. Patch 3/4 prepares the VIN driver to deal with the
EOS event from R-Car CSI-2 driver And patch 4/4 changes the error logic
of the CSI-2 receiver to match the datasheet.

This is tested on M3-N and a fault is injected by quickly removing and
re-inserting the HDMI cable while streaming. This method does not always
hit and is time consuming. To consistently prove correctness of handling
a fake fault was introduced by a HACK and a debugfs entry.

Niklas Söderlund (4):
  rcar-vin: Do not try to stop stream if not running
  rcar-vin: Route events to correct video device
  rcar-vin: Stop stream when subdevice signal EOS
  rcar-csi2: Do not try to recover after transfer error

 drivers/media/platform/rcar-vin/rcar-csi2.c | 14 +++--
 drivers/media/platform/rcar-vin/rcar-dma.c  |  5 ++
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 58 ++++++++++++++++++---
 3 files changed, 63 insertions(+), 14 deletions(-)

-- 
2.30.0

[PATCH v2 1/4] rcar-vin: Do not try to stop stream if not running

[Niklas Söderlund]

Do not attempt to stop the streaming if the stream is not running.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-dma.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index 48280ddb15b9b0ee..f30dafbdf61ca15f 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -1301,6 +1301,11 @@ void rvin_stop_streaming(struct rvin_dev *vin)
 
 	spin_lock_irqsave(&vin->qlock, flags);
 
+	if (vin->state == STOPPED) {
+		spin_unlock_irqrestore(&vin->qlock, flags);
+		return;
+	}
+
 	vin->state = STOPPING;
 
 	/* Wait until only scratch buffer is used, max 3 interrupts. */
-- 
2.30.0

Re: [PATCH v2 1/4] rcar-vin: Do not try to stop stream if not running

[Jacopo Mondi]

Hi Niklas,

On Fri, Jan 15, 2021 at 01:21:45AM +0100, Niklas Söderlund wrote:
> Do not attempt to stop the streaming if the stream is not running.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

With the comment on v1 clarified for the double stoppage case:
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks
  j

> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index 48280ddb15b9b0ee..f30dafbdf61ca15f 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -1301,6 +1301,11 @@ void rvin_stop_streaming(struct rvin_dev *vin)
>
>  	spin_lock_irqsave(&vin->qlock, flags);
>
> +	if (vin->state == STOPPED) {
> +		spin_unlock_irqrestore(&vin->qlock, flags);
> +		return;
> +	}
> +
>  	vin->state = STOPPING;
>
>  	/* Wait until only scratch buffer is used, max 3 interrupts. */
> --
> 2.30.0
>

[PATCH v2 2/4] rcar-vin: Route events to correct video device

[Niklas Söderlund]

The event route for VIN running with a media controller (Gen3) is
incorrect as all events are only routed to the video device that are
used to register the async notifier.

Remedy this be examining which subdevice generated the event and route
it to all VIN(s) that are connected to that subdevice.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 44 ++++++++++++++++++---
 1 file changed, 38 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index e6ea2b7991b8dcb3..457a65bf6b664f05 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -966,18 +966,50 @@ void rvin_v4l2_unregister(struct rvin_dev *vin)
 	video_unregister_device(&vin->vdev);
 }
 
+static void rvin_notify_video_device(struct rvin_dev *vin,
+				     unsigned int notification, void *arg)
+{
+	switch (notification) {
+	case V4L2_DEVICE_NOTIFY_EVENT:
+		v4l2_event_queue(&vin->vdev, arg);
+		break;
+	default:
+		break;
+	}
+}
+
 static void rvin_notify(struct v4l2_subdev *sd,
 			unsigned int notification, void *arg)
 {
+	struct v4l2_subdev *remote;
+	struct rvin_group *group;
+	struct media_pad *pad;
 	struct rvin_dev *vin =
 		container_of(sd->v4l2_dev, struct rvin_dev, v4l2_dev);
+	unsigned int i;
 
-	switch (notification) {
-	case V4L2_DEVICE_NOTIFY_EVENT:
-		v4l2_event_queue(&vin->vdev, arg);
-		break;
-	default:
-		break;
+	/* If no media controller, no need to route the event. */
+	if (!vin->info->use_mc) {
+		rvin_notify_video_device(vin, notification, arg);
+		return;
+	}
+
+	group = vin->group;
+
+	for (i = 0; i < RCAR_VIN_NUM; i++) {
+		vin = group->vin[i];
+		if (!vin)
+			continue;
+
+		pad = media_entity_remote_pad(&vin->pad);
+		if (!pad)
+			continue;
+
+		remote = media_entity_to_v4l2_subdev(pad->entity);
+		if (remote != sd)
+			continue;
+
+		rvin_notify_video_device(vin, notification, arg);
 	}
 }
 
-- 
2.30.0

[PATCH v2 3/4] rcar-vin: Stop stream when subdevice signal EOS

[Niklas Söderlund]

When a subdevice signals end of stream stop the VIN in addition to
informing user-space of the event.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 457a65bf6b664f05..176eae2dd5151ac9 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -969,9 +969,23 @@ void rvin_v4l2_unregister(struct rvin_dev *vin)
 static void rvin_notify_video_device(struct rvin_dev *vin,
 				     unsigned int notification, void *arg)
 {
+	const struct v4l2_event *event;
+
 	switch (notification) {
 	case V4L2_DEVICE_NOTIFY_EVENT:
-		v4l2_event_queue(&vin->vdev, arg);
+		event = arg;
+
+		switch (event->type) {
+		case V4L2_EVENT_EOS:
+			rvin_stop_streaming(vin);
+			v4l2_info(&vin->v4l2_dev,
+				  "Subdevice signaled end of stream, stopping.\n");
+			break;
+		default:
+			break;
+		}
+
+		v4l2_event_queue(&vin->vdev, event);
 		break;
 	default:
 		break;
-- 
2.30.0

Re: [PATCH v2 3/4] rcar-vin: Stop stream when subdevice signal EOS

[Hans Verkuil]

On 15/01/2021 01:21, Niklas Söderlund wrote:
> When a subdevice signals end of stream stop the VIN in addition to
> informing user-space of the event.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index 457a65bf6b664f05..176eae2dd5151ac9 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -969,9 +969,23 @@ void rvin_v4l2_unregister(struct rvin_dev *vin)
>  static void rvin_notify_video_device(struct rvin_dev *vin,
>  				     unsigned int notification, void *arg)
>  {
> +	const struct v4l2_event *event;
> +
>  	switch (notification) {
>  	case V4L2_DEVICE_NOTIFY_EVENT:
> -		v4l2_event_queue(&vin->vdev, arg);
> +		event = arg;
> +
> +		switch (event->type) {
> +		case V4L2_EVENT_EOS:
> +			rvin_stop_streaming(vin);
> +			v4l2_info(&vin->v4l2_dev,

Can you make this v4l2_dbg to avoid polluting the kernel log?

Regards,

	Hans

> +				  "Subdevice signaled end of stream, stopping.\n");
> +			break;
> +		default:
> +			break;
> +		}
> +
> +		v4l2_event_queue(&vin->vdev, event);
>  		break;
>  	default:
>  		break;
> 

[PATCH v2 4/4] rcar-csi2: Do not try to recover after transfer error

[Niklas Söderlund]

Instead of restarting the R-Car CSI-2 receiver if a transmission error
is detected inform the R-Car VIN driver of the error so it can stop the
whole pipeline and inform user-space. This is done to reflect a updated
usage recommendation in later versions of the datasheet.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-csi2.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index 945d2eb8723367f0..a7212ecc46572a3b 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -773,21 +773,19 @@ static irqreturn_t rcsi2_irq(int irq, void *data)
 
 	rcsi2_write(priv, INTERRSTATE_REG, err_status);
 
-	dev_info(priv->dev, "Transfer error, restarting CSI-2 receiver\n");
-
 	return IRQ_WAKE_THREAD;
 }
 
 static irqreturn_t rcsi2_irq_thread(int irq, void *data)
 {
 	struct rcar_csi2 *priv = data;
+	struct v4l2_event event = {
+		.type = V4L2_EVENT_EOS,
+	};
 
-	mutex_lock(&priv->lock);
-	rcsi2_stop(priv);
-	usleep_range(1000, 2000);
-	if (rcsi2_start(priv))
-		dev_warn(priv->dev, "Failed to restart CSI-2 receiver\n");
-	mutex_unlock(&priv->lock);
+	dev_err(priv->dev, "Transfer error detected.\n");
+
+	v4l2_subdev_notify_event(&priv->subdev, &event);
 
 	return IRQ_HANDLED;
 }
-- 
2.30.0

Re: [PATCH v2 4/4] rcar-csi2: Do not try to recover after transfer error

[Sergei Shtylyov]

Hello!

On 15.01.2021 3:21, Niklas Söderlund wrote:

> Instead of restarting the R-Car CSI-2 receiver if a transmission error
> is detected inform the R-Car VIN driver of the error so it can stop the
              ^ , woiuldn't hurt here?


> whole pipeline and inform user-space. This is done to reflect a updated
                                                                 ^ an

> usage recommendation in later versions of the datasheet.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
[...]

MBR, Sergei

Re: [PATCH v2 4/4] rcar-csi2: Do not try to recover after transfer error

[Jacopo Mondi]

Hi Niklas,

On Fri, Jan 15, 2021 at 01:21:48AM +0100, Niklas Söderlund wrote:
> Instead of restarting the R-Car CSI-2 receiver if a transmission error
> is detected inform the R-Car VIN driver of the error so it can stop the
> whole pipeline and inform user-space. This is done to reflect a updated
> usage recommendation in later versions of the datasheet.

I wonder if there's any userspace component that relies on the
auto-restart procedure.

>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

If this aligns the behaviour with the manual (and it seems also saner
than attemping a restart) I think it's good.

Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks
  j

> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index 945d2eb8723367f0..a7212ecc46572a3b 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -773,21 +773,19 @@ static irqreturn_t rcsi2_irq(int irq, void *data)
>
>  	rcsi2_write(priv, INTERRSTATE_REG, err_status);
>
> -	dev_info(priv->dev, "Transfer error, restarting CSI-2 receiver\n");
> -
>  	return IRQ_WAKE_THREAD;
>  }
>
>  static irqreturn_t rcsi2_irq_thread(int irq, void *data)
>  {
>  	struct rcar_csi2 *priv = data;
> +	struct v4l2_event event = {
> +		.type = V4L2_EVENT_EOS,
> +	};
>
> -	mutex_lock(&priv->lock);
> -	rcsi2_stop(priv);
> -	usleep_range(1000, 2000);
> -	if (rcsi2_start(priv))
> -		dev_warn(priv->dev, "Failed to restart CSI-2 receiver\n");
> -	mutex_unlock(&priv->lock);
> +	dev_err(priv->dev, "Transfer error detected.\n");
> +
> +	v4l2_subdev_notify_event(&priv->subdev, &event);
>
>  	return IRQ_HANDLED;
>  }
> --
> 2.30.0
>

Re: [PATCH v2 4/4] rcar-csi2: Do not try to recover after transfer error

[Hans Verkuil]

On 15/01/2021 01:21, Niklas Söderlund wrote:
> Instead of restarting the R-Car CSI-2 receiver if a transmission error
> is detected inform the R-Car VIN driver of the error so it can stop the
> whole pipeline and inform user-space. This is done to reflect a updated
> usage recommendation in later versions of the datasheet.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index 945d2eb8723367f0..a7212ecc46572a3b 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -773,21 +773,19 @@ static irqreturn_t rcsi2_irq(int irq, void *data)
>  
>  	rcsi2_write(priv, INTERRSTATE_REG, err_status);
>  
> -	dev_info(priv->dev, "Transfer error, restarting CSI-2 receiver\n");
> -
>  	return IRQ_WAKE_THREAD;
>  }
>  
>  static irqreturn_t rcsi2_irq_thread(int irq, void *data)
>  {
>  	struct rcar_csi2 *priv = data;
> +	struct v4l2_event event = {
> +		.type = V4L2_EVENT_EOS,
> +	};
>  
> -	mutex_lock(&priv->lock);
> -	rcsi2_stop(priv);
> -	usleep_range(1000, 2000);
> -	if (rcsi2_start(priv))
> -		dev_warn(priv->dev, "Failed to restart CSI-2 receiver\n");
> -	mutex_unlock(&priv->lock);
> +	dev_err(priv->dev, "Transfer error detected.\n");

You probably want to call vb2_queue_error() here. Typically once
something like this happens you have to restart everything and marking
the queue as 'error' will ensure that VIDIOC_QBUF will return an error
until the queue is reset (STREAMOFF).

It doesn't hurt to also raise the EOS event, I'm fine with that.

Regards,

	Hans

> +
> +	v4l2_subdev_notify_event(&priv->subdev, &event);
>  
>  	return IRQ_HANDLED;
>  }
> 

Re: [PATCH v2 4/4] rcar-csi2: Do not try to recover after transfer error

[Niklas Söderlund]

Hi Hans,

Thanks for your feedback.

On 2021-01-25 10:44:48 +0100, Hans Verkuil wrote:
> On 15/01/2021 01:21, Niklas Söderlund wrote:
> > Instead of restarting the R-Car CSI-2 receiver if a transmission error
> > is detected inform the R-Car VIN driver of the error so it can stop the
> > whole pipeline and inform user-space. This is done to reflect a updated
> > usage recommendation in later versions of the datasheet.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 14 ++++++--------
> >  1 file changed, 6 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index 945d2eb8723367f0..a7212ecc46572a3b 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -773,21 +773,19 @@ static irqreturn_t rcsi2_irq(int irq, void *data)
> >  
> >  	rcsi2_write(priv, INTERRSTATE_REG, err_status);
> >  
> > -	dev_info(priv->dev, "Transfer error, restarting CSI-2 receiver\n");
> > -
> >  	return IRQ_WAKE_THREAD;
> >  }
> >  
> >  static irqreturn_t rcsi2_irq_thread(int irq, void *data)
> >  {
> >  	struct rcar_csi2 *priv = data;
> > +	struct v4l2_event event = {
> > +		.type = V4L2_EVENT_EOS,
> > +	};
> >  
> > -	mutex_lock(&priv->lock);
> > -	rcsi2_stop(priv);
> > -	usleep_range(1000, 2000);
> > -	if (rcsi2_start(priv))
> > -		dev_warn(priv->dev, "Failed to restart CSI-2 receiver\n");
> > -	mutex_unlock(&priv->lock);
> > +	dev_err(priv->dev, "Transfer error detected.\n");
> 
> You probably want to call vb2_queue_error() here. Typically once
> something like this happens you have to restart everything and marking
> the queue as 'error' will ensure that VIDIOC_QBUF will return an error
> until the queue is reset (STREAMOFF).

The CSI-2 driver is a bridge driver and does not deal with buffers.  
Instead the idea here is to signal EOS so that the VIN driver (and 
user-space) can detect it and indeed as you point out deal with 
signaling vb2 error.

I will respin this series as it needs to be rebased anyhow.

> 
> It doesn't hurt to also raise the EOS event, I'm fine with that.
> 
> Regards,
> 
> 	Hans
> 
> > +
> > +	v4l2_subdev_notify_event(&priv->subdev, &event);
> >  
> >  	return IRQ_HANDLED;
> >  }
> > 
> 

-- 
Regards,
Niklas Söderlund