Improve error handling in the rcar-vin driver

Improve error handling in the rcar-vin driver

[Michael Rodin]

Hello,

this series is a followup to the other series [1] started by Niklas Söderlund
where only the first patch has been merged. The overall idea is to be more
compliant with the Renesas hardware manual which requires a reset or stop
of capture in the VIN module before reset of CSI2. Another goal (achieved
by the patch 3/3) is to be more resilient with respect to non-critical CSI2
errors so the driver does not end in an endless restart loop.

Patch 1/3 has been taken from [1] with some additional documentation changes.
Patch 2/3 is included without any changes.
Patch 3/3 is based on discussions in [2], where I also found one method to
trigger CSI2 errors in the video pipeline on a Salvator board for testing
by manipulating CSI2-related registers in the adv7482:

$ i2cset -f -y 4 0x64 0x00 0x04
$ i2cset -f -y 4 0x64 0x00 0x24

[1] https://lore.kernel.org/linux-renesas-soc/20211108160220.767586-1-niklas.soderlund+renesas@ragnatech.se/
[2] https://lore.kernel.org/linux-renesas-soc/20220309192707.GA62903@vmlxhi-121.adit-jv.com/

[PATCH 1/3] media: videobuf2: Add a transfer error event

[Michael Rodin]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Add a new V4L2_EVENT_XFER_ERROR event to signal if a unrecoverable error
happens during video transfer.

The use-case that sparked this new event is to signal to the video
device driver that an error has happen on the CSI-2 bus from the CSI-2
receiver subdevice.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
[mrodin@de.adit-jv.com: added information what to do if this new event is received]
Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>
---
 Documentation/userspace-api/media/v4l/vidioc-dqevent.rst     | 9 +++++++++
 Documentation/userspace-api/media/videodev2.h.rst.exceptions | 1 +
 include/uapi/linux/videodev2.h                               | 1 +
 3 files changed, 11 insertions(+)

diff --git a/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst b/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst
index 6eb4007..ed0a5ff 100644
--- a/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst
+++ b/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst
@@ -182,6 +182,15 @@ call.
 	the regions changes. This event has a struct
 	:c:type:`v4l2_event_motion_det`
 	associated with it.
+    * - ``V4L2_EVENT_XFER_ERROR``
+      - 7
+      - This event is triggered when an transfer error is detected while
+	streaming. For example if a unrecoverable error is detected on a video
+	bus in the pipeline. If a driver receives this event from an upstream
+	subdevice, it has to check if it is affected by this error and then try
+	to recover from this error. If an internal recovery is not possible,
+	then it can forward the event to userspace so the streaming application
+	has to restart streaming if it wants to continue.
     * - ``V4L2_EVENT_PRIVATE_START``
       - 0x08000000
       - Base event number for driver-private events.
diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
index 9cbb7a0..25bde61 100644
--- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
+++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
@@ -500,6 +500,7 @@ replace define V4L2_EVENT_CTRL event-type
 replace define V4L2_EVENT_FRAME_SYNC event-type
 replace define V4L2_EVENT_SOURCE_CHANGE event-type
 replace define V4L2_EVENT_MOTION_DET event-type
+replace define V4L2_EVENT_XFER_ERROR event-type
 replace define V4L2_EVENT_PRIVATE_START event-type
 
 replace define V4L2_EVENT_CTRL_CH_VALUE ctrl-changes-flags
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 6d465dc..8c75bd8 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -2383,6 +2383,7 @@ struct v4l2_streamparm {
 #define V4L2_EVENT_FRAME_SYNC			4
 #define V4L2_EVENT_SOURCE_CHANGE		5
 #define V4L2_EVENT_MOTION_DET			6
+#define V4L2_EVENT_XFER_ERROR			7
 #define V4L2_EVENT_PRIVATE_START		0x08000000
 
 /* Payload for V4L2_EVENT_VSYNC */
-- 
2.7.4

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

[Michael Rodin]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

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>
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/platform/renesas/rcar-vin/rcar-csi2.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-csi2.c b/drivers/media/platform/renesas/rcar-vin/rcar-csi2.c
index fea8f00..5a64941 100644
--- a/drivers/media/platform/renesas/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/renesas/rcar-vin/rcar-csi2.c
@@ -943,21 +943,22 @@ 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_XFER_ERROR,
+	};
 
-	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);
+	/* Disable further interrupts to not spam the transfer error event. */
+	rcsi2_write(priv, INTEN_REG, 0);
+
+	dev_err(priv->dev, "Transfer error detected.\n");
+
+	v4l2_subdev_notify_event(&priv->subdev, &event);
 
 	return IRQ_HANDLED;
 }
-- 
2.7.4

[PATCH 3/3] rcar-vin: handle transfer errors from subdevices and stop streaming if required

[Michael Rodin]

When a subdevice sends a transfer error event during streaming and we can
not capture new frames, then we know for sure that this is an unrecoverable
failure and not just a temporary glitch. In this case we can not ignore the
transfer error any more and have to notify userspace. In response to the
transfer error event userspace can try to restart streaming and hope that
it works again.

This patch is based on the patch [1] from Niklas Söderlund, however it adds
more logic to check whether the VIN hardware module is actually affected by
the transfer errors reported by the usptream device. For this it takes some
ideas from the imx driver where EOF interrupts are monitored by the
eof_timeout_timer added by commit 4a34ec8e470c ("[media] media: imx: Add
CSI subdev driver").

[1] https://lore.kernel.org/linux-renesas-soc/20211108160220.767586-4-niklas.soderlund+renesas@ragnatech.se/

Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>
---
 drivers/media/platform/renesas/rcar-vin/rcar-dma.c | 34 ++++++++++++++++++++++
 .../media/platform/renesas/rcar-vin/rcar-v4l2.c    | 18 +++++++++++-
 drivers/media/platform/renesas/rcar-vin/rcar-vin.h |  7 +++++
 3 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
index 2272f1c..596a367 100644
--- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
@@ -13,6 +13,7 @@
 #include <linux/delay.h>
 #include <linux/interrupt.h>
 #include <linux/pm_runtime.h>
+#include <media/v4l2-event.h>
 
 #include <media/videobuf2-dma-contig.h>
 
@@ -1060,6 +1061,9 @@ static irqreturn_t rvin_irq(int irq, void *data)
 		vin_dbg(vin, "Dropping frame %u\n", vin->sequence);
 	}
 
+	cancel_delayed_work(&vin->frame_timeout);
+	schedule_delayed_work(&vin->frame_timeout, msecs_to_jiffies(FRAME_TIMEOUT_MS));
+
 	vin->sequence++;
 
 	/* Prepare for next frame */
@@ -1283,6 +1287,7 @@ int rvin_start_streaming(struct rvin_dev *vin)
 	spin_lock_irqsave(&vin->qlock, flags);
 
 	vin->sequence = 0;
+	vin->xfer_error = false;
 
 	ret = rvin_capture_start(vin);
 	if (ret)
@@ -1290,6 +1295,10 @@ int rvin_start_streaming(struct rvin_dev *vin)
 
 	spin_unlock_irqrestore(&vin->qlock, flags);
 
+	/* We start the frame watchdog only after we have successfully started streaming */
+	if (!ret)
+		schedule_delayed_work(&vin->frame_timeout, msecs_to_jiffies(FRAME_TIMEOUT_MS));
+
 	return ret;
 }
 
@@ -1332,6 +1341,12 @@ void rvin_stop_streaming(struct rvin_dev *vin)
 	}
 
 	vin->state = STOPPING;
+	/*
+	 * Since we are now stopping and don't expect more frames to be captured, make sure that
+	 * there is no pending work for error handling.
+	 */
+	cancel_delayed_work_sync(&vin->frame_timeout);
+	vin->xfer_error = false;
 
 	/* Wait until only scratch buffer is used, max 3 interrupts. */
 	retries = 0;
@@ -1424,6 +1439,23 @@ void rvin_dma_unregister(struct rvin_dev *vin)
 	v4l2_device_unregister(&vin->v4l2_dev);
 }
 
+static void rvin_frame_timeout(struct work_struct *work)
+{
+	struct delayed_work *dwork = to_delayed_work(work);
+	struct rvin_dev *vin = container_of(dwork, struct rvin_dev, frame_timeout);
+	struct v4l2_event event = {
+		.type = V4L2_EVENT_XFER_ERROR,
+	};
+
+	vin_dbg(vin, "Frame timeout!\n");
+
+	if (!vin->xfer_error)
+		return;
+	vin_err(vin, "Unrecoverable transfer error detected, stopping streaming\n");
+	vb2_queue_error(&vin->queue);
+	v4l2_event_queue(&vin->vdev, &event);
+}
+
 int rvin_dma_register(struct rvin_dev *vin, int irq)
 {
 	struct vb2_queue *q = &vin->queue;
@@ -1470,6 +1502,8 @@ int rvin_dma_register(struct rvin_dev *vin, int irq)
 		goto error;
 	}
 
+	INIT_DELAYED_WORK(&vin->frame_timeout, rvin_frame_timeout);
+
 	return 0;
 error:
 	rvin_dma_unregister(vin);
diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c b/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c
index 2e2aa9d..bd7f6fe2 100644
--- a/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c
@@ -648,6 +648,8 @@ static int rvin_subscribe_event(struct v4l2_fh *fh,
 	switch (sub->type) {
 	case V4L2_EVENT_SOURCE_CHANGE:
 		return v4l2_event_subscribe(fh, sub, 4, NULL);
+	case V4L2_EVENT_XFER_ERROR:
+		return v4l2_event_subscribe(fh, sub, 1, NULL);
 	}
 	return v4l2_ctrl_subscribe_event(fh, sub);
 }
@@ -1000,9 +1002,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_XFER_ERROR:
+			if (vin->state != STOPPED && vin->state != STOPPING) {
+				vin_dbg(vin, "Subdevice signaled transfer error.\n");
+				vin->xfer_error = true;
+			}
+			break;
+		default:
+			break;
+		}
+
 		break;
 	default:
 		break;
diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
index 1f94589..4726a69 100644
--- a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
@@ -31,6 +31,9 @@
 /* Max number on VIN instances that can be in a system */
 #define RCAR_VIN_NUM 32
 
+/* maximum time we wait before signalling an error to userspace */
+#define FRAME_TIMEOUT_MS 1000
+
 struct rvin_group;
 
 enum model_id {
@@ -207,6 +210,8 @@ struct rvin_info {
  * @std:		active video standard of the video source
  *
  * @alpha:		Alpha component to fill in for supported pixel formats
+ * @xfer_error:		Indicates if any transfer errors occurred in the current streaming session.
+ * @frame_timeout:	Watchdog for monitoring regular capturing of frames in rvin_irq.
  */
 struct rvin_dev {
 	struct device *dev;
@@ -251,6 +256,8 @@ struct rvin_dev {
 	v4l2_std_id std;
 
 	unsigned int alpha;
+	bool xfer_error;
+	struct delayed_work frame_timeout;
 };
 
 #define vin_to_source(vin)		((vin)->parallel.subdev)
-- 
2.7.4

Re: [PATCH 3/3] rcar-vin: handle transfer errors from subdevices and stop streaming if required

[Niklas Söderlund]

Hi Michael,

Thanks for your work.

I like this patch, I think it captures the issue discussed in the 
previous thread quiet nicely. One small nit below.

On 2022-05-19 20:00:09 +0200, Michael Rodin wrote:
> When a subdevice sends a transfer error event during streaming and we can
> not capture new frames, then we know for sure that this is an unrecoverable
> failure and not just a temporary glitch. In this case we can not ignore the
> transfer error any more and have to notify userspace. In response to the
> transfer error event userspace can try to restart streaming and hope that
> it works again.
> 
> This patch is based on the patch [1] from Niklas Söderlund, however it adds
> more logic to check whether the VIN hardware module is actually affected by
> the transfer errors reported by the usptream device. For this it takes some
> ideas from the imx driver where EOF interrupts are monitored by the
> eof_timeout_timer added by commit 4a34ec8e470c ("[media] media: imx: Add
> CSI subdev driver").
> 
> [1] https://lore.kernel.org/linux-renesas-soc/20211108160220.767586-4-niklas.soderlund+renesas@ragnatech.se/
> 
> Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>
> ---
>  drivers/media/platform/renesas/rcar-vin/rcar-dma.c | 34 ++++++++++++++++++++++
>  .../media/platform/renesas/rcar-vin/rcar-v4l2.c    | 18 +++++++++++-
>  drivers/media/platform/renesas/rcar-vin/rcar-vin.h |  7 +++++
>  3 files changed, 58 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> index 2272f1c..596a367 100644
> --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> @@ -13,6 +13,7 @@
>  #include <linux/delay.h>
>  #include <linux/interrupt.h>
>  #include <linux/pm_runtime.h>
> +#include <media/v4l2-event.h>
>  
>  #include <media/videobuf2-dma-contig.h>
>  
> @@ -1060,6 +1061,9 @@ static irqreturn_t rvin_irq(int irq, void *data)
>  		vin_dbg(vin, "Dropping frame %u\n", vin->sequence);
>  	}
>  
> +	cancel_delayed_work(&vin->frame_timeout);
> +	schedule_delayed_work(&vin->frame_timeout, msecs_to_jiffies(FRAME_TIMEOUT_MS));
> +
>  	vin->sequence++;
>  
>  	/* Prepare for next frame */
> @@ -1283,6 +1287,7 @@ int rvin_start_streaming(struct rvin_dev *vin)
>  	spin_lock_irqsave(&vin->qlock, flags);
>  
>  	vin->sequence = 0;
> +	vin->xfer_error = false;
>  
>  	ret = rvin_capture_start(vin);
>  	if (ret)
> @@ -1290,6 +1295,10 @@ int rvin_start_streaming(struct rvin_dev *vin)
>  
>  	spin_unlock_irqrestore(&vin->qlock, flags);
>  
> +	/* We start the frame watchdog only after we have successfully started streaming */
> +	if (!ret)
> +		schedule_delayed_work(&vin->frame_timeout, msecs_to_jiffies(FRAME_TIMEOUT_MS));
> +
>  	return ret;
>  }
>  
> @@ -1332,6 +1341,12 @@ void rvin_stop_streaming(struct rvin_dev *vin)
>  	}
>  
>  	vin->state = STOPPING;
> +	/*
> +	 * Since we are now stopping and don't expect more frames to be captured, make sure that
> +	 * there is no pending work for error handling.
> +	 */
> +	cancel_delayed_work_sync(&vin->frame_timeout);
> +	vin->xfer_error = false;

Do we need to set xfer_error to false here? The delayed work is canceled 
and we reset the xfer_error when we start in rvin_start_streaming().

>  
>  	/* Wait until only scratch buffer is used, max 3 interrupts. */
>  	retries = 0;
> @@ -1424,6 +1439,23 @@ void rvin_dma_unregister(struct rvin_dev *vin)
>  	v4l2_device_unregister(&vin->v4l2_dev);
>  }
>  
> +static void rvin_frame_timeout(struct work_struct *work)
> +{
> +	struct delayed_work *dwork = to_delayed_work(work);
> +	struct rvin_dev *vin = container_of(dwork, struct rvin_dev, frame_timeout);
> +	struct v4l2_event event = {
> +		.type = V4L2_EVENT_XFER_ERROR,
> +	};
> +
> +	vin_dbg(vin, "Frame timeout!\n");
> +
> +	if (!vin->xfer_error)
> +		return;
> +	vin_err(vin, "Unrecoverable transfer error detected, stopping streaming\n");
> +	vb2_queue_error(&vin->queue);
> +	v4l2_event_queue(&vin->vdev, &event);
> +}
> +
>  int rvin_dma_register(struct rvin_dev *vin, int irq)
>  {
>  	struct vb2_queue *q = &vin->queue;
> @@ -1470,6 +1502,8 @@ int rvin_dma_register(struct rvin_dev *vin, int irq)
>  		goto error;
>  	}
>  
> +	INIT_DELAYED_WORK(&vin->frame_timeout, rvin_frame_timeout);
> +
>  	return 0;
>  error:
>  	rvin_dma_unregister(vin);
> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c b/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c
> index 2e2aa9d..bd7f6fe2 100644
> --- a/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c
> @@ -648,6 +648,8 @@ static int rvin_subscribe_event(struct v4l2_fh *fh,
>  	switch (sub->type) {
>  	case V4L2_EVENT_SOURCE_CHANGE:
>  		return v4l2_event_subscribe(fh, sub, 4, NULL);
> +	case V4L2_EVENT_XFER_ERROR:
> +		return v4l2_event_subscribe(fh, sub, 1, NULL);
>  	}
>  	return v4l2_ctrl_subscribe_event(fh, sub);
>  }
> @@ -1000,9 +1002,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_XFER_ERROR:
> +			if (vin->state != STOPPED && vin->state != STOPPING) {
> +				vin_dbg(vin, "Subdevice signaled transfer error.\n");
> +				vin->xfer_error = true;
> +			}
> +			break;
> +		default:
> +			break;
> +		}
> +
>  		break;
>  	default:
>  		break;
> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
> index 1f94589..4726a69 100644
> --- a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
> @@ -31,6 +31,9 @@
>  /* Max number on VIN instances that can be in a system */
>  #define RCAR_VIN_NUM 32
>  
> +/* maximum time we wait before signalling an error to userspace */
> +#define FRAME_TIMEOUT_MS 1000
> +
>  struct rvin_group;
>  
>  enum model_id {
> @@ -207,6 +210,8 @@ struct rvin_info {
>   * @std:		active video standard of the video source
>   *
>   * @alpha:		Alpha component to fill in for supported pixel formats
> + * @xfer_error:		Indicates if any transfer errors occurred in the current streaming session.
> + * @frame_timeout:	Watchdog for monitoring regular capturing of frames in rvin_irq.
>   */
>  struct rvin_dev {
>  	struct device *dev;
> @@ -251,6 +256,8 @@ struct rvin_dev {
>  	v4l2_std_id std;
>  
>  	unsigned int alpha;
> +	bool xfer_error;
> +	struct delayed_work frame_timeout;
>  };
>  
>  #define vin_to_source(vin)		((vin)->parallel.subdev)
> -- 
> 2.7.4
> 

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH 3/3] rcar-vin: handle transfer errors from subdevices and stop streaming if required

[Michael Rodin]

Hi Niklas,

On Thu, May 19, 2022 at 11:00:20PM +0200, Niklas Söderlund wrote:
> Hi Michael,
> 
> Thanks for your work.
> 
> I like this patch, I think it captures the issue discussed in the 
> previous thread quiet nicely. One small nit below.
> 
> On 2022-05-19 20:00:09 +0200, Michael Rodin wrote:
> > When a subdevice sends a transfer error event during streaming and we can
> > not capture new frames, then we know for sure that this is an unrecoverable
> > failure and not just a temporary glitch. In this case we can not ignore the
> > transfer error any more and have to notify userspace. In response to the
> > transfer error event userspace can try to restart streaming and hope that
> > it works again.
> > 
> > This patch is based on the patch [1] from Niklas Söderlund, however it adds
> > more logic to check whether the VIN hardware module is actually affected by
> > the transfer errors reported by the usptream device. For this it takes some
> > ideas from the imx driver where EOF interrupts are monitored by the
> > eof_timeout_timer added by commit 4a34ec8e470c ("[media] media: imx: Add
> > CSI subdev driver").
> > 
> > [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_linux-2Drenesas-2Dsoc_20211108160220.767586-2D4-2Dniklas.soderlund-2Brenesas-40ragnatech.se_&d=DwIDAw&c=euGZstcaTDllvimEN8b7jXrwqOf-v5A_CdpgnVfiiMM&r=sWsgk3pKkv5GeIDM2RZlPY8TjNFU2D0oBeOj6QNBadE&m=on7B_2z5sGrhiuvQgbA4XC0_qMRWNTZoWGRMzD9N0Ag&s=_LetePiuy8odH72QwAj6k-I0YOANjzkNwTnqqFr0_ck&e=
> > 
> > Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>
> > ---
> >  drivers/media/platform/renesas/rcar-vin/rcar-dma.c | 34 ++++++++++++++++++++++
> >  .../media/platform/renesas/rcar-vin/rcar-v4l2.c    | 18 +++++++++++-
> >  drivers/media/platform/renesas/rcar-vin/rcar-vin.h |  7 +++++
> >  3 files changed, 58 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> > index 2272f1c..596a367 100644
> > --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/delay.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/pm_runtime.h>
> > +#include <media/v4l2-event.h>
> >  
> >  #include <media/videobuf2-dma-contig.h>
> >  
> > @@ -1060,6 +1061,9 @@ static irqreturn_t rvin_irq(int irq, void *data)
> >  		vin_dbg(vin, "Dropping frame %u\n", vin->sequence);
> >  	}
> >  
> > +	cancel_delayed_work(&vin->frame_timeout);
> > +	schedule_delayed_work(&vin->frame_timeout, msecs_to_jiffies(FRAME_TIMEOUT_MS));
> > +
> >  	vin->sequence++;
> >  
> >  	/* Prepare for next frame */
> > @@ -1283,6 +1287,7 @@ int rvin_start_streaming(struct rvin_dev *vin)
> >  	spin_lock_irqsave(&vin->qlock, flags);
> >  
> >  	vin->sequence = 0;
> > +	vin->xfer_error = false;
> >  
> >  	ret = rvin_capture_start(vin);
> >  	if (ret)
> > @@ -1290,6 +1295,10 @@ int rvin_start_streaming(struct rvin_dev *vin)
> >  
> >  	spin_unlock_irqrestore(&vin->qlock, flags);
> >  
> > +	/* We start the frame watchdog only after we have successfully started streaming */
> > +	if (!ret)
> > +		schedule_delayed_work(&vin->frame_timeout, msecs_to_jiffies(FRAME_TIMEOUT_MS));
> > +
> >  	return ret;
> >  }
> >  
> > @@ -1332,6 +1341,12 @@ void rvin_stop_streaming(struct rvin_dev *vin)
> >  	}
> >  
> >  	vin->state = STOPPING;
> > +	/*
> > +	 * Since we are now stopping and don't expect more frames to be captured, make sure that
> > +	 * there is no pending work for error handling.
> > +	 */
> > +	cancel_delayed_work_sync(&vin->frame_timeout);
> > +	vin->xfer_error = false;
> 
> Do we need to set xfer_error to false here? The delayed work is canceled 
> and we reset the xfer_error when we start in rvin_start_streaming().
> 

You are right, this seems to be redundant. But I think that there might be
a different case where we have to reset xfer_error:

 1. A non-critical transfer error has occurred during streaming from a
    HDMI source.
 2. Frames are still captured for an hour without any further problems,
    since it was just a short glitch
 3. Now the source (e.g. HDMI signal generator) has been powered off by the
    user so it does not send new frames.
 4. Timeout occurs due to 3 but since xfer_error has been set 1 hour ago,
    userspace is notified about a transfer error and assumes that streaming
    has been stopped because of this.

To avoid this scenario I think maybe we have to restrict validity of
xfer_error. Maybe it would be better to make xfer_error a counter which is
set after a transfer error to e.g. 10 frames and then decremented after
each captured frame so after 10 successfully captured frames we know that a
timeout has occurred definitely not due to a transfer error?

Another possible improvement might be to make FRAME_TIMEOUT_MS configurable,
maybe via a v4l2 control from userspace? Or we could also define the timeout
as a multiple of the frame interval of the source. This would allow us to
reduce the timeout further based on the particular source so the userspace
does not have to wait for a second until it knows that it has to restart
streaming.

What do you think?

> >  
> >  	/* Wait until only scratch buffer is used, max 3 interrupts. */
> >  	retries = 0;
> > @@ -1424,6 +1439,23 @@ void rvin_dma_unregister(struct rvin_dev *vin)
> >  	v4l2_device_unregister(&vin->v4l2_dev);
> >  }
> >  
> > +static void rvin_frame_timeout(struct work_struct *work)
> > +{
> > +	struct delayed_work *dwork = to_delayed_work(work);
> > +	struct rvin_dev *vin = container_of(dwork, struct rvin_dev, frame_timeout);
> > +	struct v4l2_event event = {
> > +		.type = V4L2_EVENT_XFER_ERROR,
> > +	};
> > +
> > +	vin_dbg(vin, "Frame timeout!\n");
> > +
> > +	if (!vin->xfer_error)
> > +		return;
> > +	vin_err(vin, "Unrecoverable transfer error detected, stopping streaming\n");
> > +	vb2_queue_error(&vin->queue);
> > +	v4l2_event_queue(&vin->vdev, &event);
> > +}
> > +
> >  int rvin_dma_register(struct rvin_dev *vin, int irq)
> >  {
> >  	struct vb2_queue *q = &vin->queue;
> > @@ -1470,6 +1502,8 @@ int rvin_dma_register(struct rvin_dev *vin, int irq)
> >  		goto error;
> >  	}
> >  
> > +	INIT_DELAYED_WORK(&vin->frame_timeout, rvin_frame_timeout);
> > +
> >  	return 0;
> >  error:
> >  	rvin_dma_unregister(vin);
> > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c b/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c
> > index 2e2aa9d..bd7f6fe2 100644
> > --- a/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c
> > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c
> > @@ -648,6 +648,8 @@ static int rvin_subscribe_event(struct v4l2_fh *fh,
> >  	switch (sub->type) {
> >  	case V4L2_EVENT_SOURCE_CHANGE:
> >  		return v4l2_event_subscribe(fh, sub, 4, NULL);
> > +	case V4L2_EVENT_XFER_ERROR:
> > +		return v4l2_event_subscribe(fh, sub, 1, NULL);
> >  	}
> >  	return v4l2_ctrl_subscribe_event(fh, sub);
> >  }
> > @@ -1000,9 +1002,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_XFER_ERROR:
> > +			if (vin->state != STOPPED && vin->state != STOPPING) {
> > +				vin_dbg(vin, "Subdevice signaled transfer error.\n");
> > +				vin->xfer_error = true;
> > +			}
> > +			break;
> > +		default:
> > +			break;
> > +		}
> > +
> >  		break;
> >  	default:
> >  		break;
> > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
> > index 1f94589..4726a69 100644
> > --- a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
> > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
> > @@ -31,6 +31,9 @@
> >  /* Max number on VIN instances that can be in a system */
> >  #define RCAR_VIN_NUM 32
> >  
> > +/* maximum time we wait before signalling an error to userspace */
> > +#define FRAME_TIMEOUT_MS 1000
> > +
> >  struct rvin_group;
> >  
> >  enum model_id {
> > @@ -207,6 +210,8 @@ struct rvin_info {
> >   * @std:		active video standard of the video source
> >   *
> >   * @alpha:		Alpha component to fill in for supported pixel formats
> > + * @xfer_error:		Indicates if any transfer errors occurred in the current streaming session.
> > + * @frame_timeout:	Watchdog for monitoring regular capturing of frames in rvin_irq.
> >   */
> >  struct rvin_dev {
> >  	struct device *dev;
> > @@ -251,6 +256,8 @@ struct rvin_dev {
> >  	v4l2_std_id std;
> >  
> >  	unsigned int alpha;
> > +	bool xfer_error;
> > +	struct delayed_work frame_timeout;
> >  };
> >  
> >  #define vin_to_source(vin)		((vin)->parallel.subdev)
> > -- 
> > 2.7.4
> > 
> 
> -- 
> Kind Regards,
> Niklas Söderlund

-- 
Best Regards,
Michael

Re: [PATCH 3/3] rcar-vin: handle transfer errors from subdevices and stop streaming if required

[Niklas Söderlund]

Hi Michael,

On 2022-05-20 21:50:41 +0200, Michael Rodin wrote:

[snip]

> > 
> > Do we need to set xfer_error to false here? The delayed work is canceled 
> > and we reset the xfer_error when we start in rvin_start_streaming().
> > 
> 
> You are right, this seems to be redundant. But I think that there might be
> a different case where we have to reset xfer_error:
> 
>  1. A non-critical transfer error has occurred during streaming from a
>     HDMI source.
>  2. Frames are still captured for an hour without any further problems,
>     since it was just a short glitch
>  3. Now the source (e.g. HDMI signal generator) has been powered off by the
>     user so it does not send new frames.
>  4. Timeout occurs due to 3 but since xfer_error has been set 1 hour ago,
>     userspace is notified about a transfer error and assumes that streaming
>     has been stopped because of this.
> 
> To avoid this scenario I think maybe we have to restrict validity of
> xfer_error. Maybe it would be better to make xfer_error a counter which is
> set after a transfer error to e.g. 10 frames and then decremented after
> each captured frame so after 10 successfully captured frames we know that a
> timeout has occurred definitely not due to a transfer error?
> 
> Another possible improvement might be to make FRAME_TIMEOUT_MS configurable,
> maybe via a v4l2 control from userspace? Or we could also define the timeout
> as a multiple of the frame interval of the source. This would allow us to
> reduce the timeout further based on the particular source so the userspace
> does not have to wait for a second until it knows that it has to restart
> streaming.
> 
> What do you think?

I discussed this problem last week at a conference and the consensus was 
that this problem of timeouts and the like should in the first hand be 
handled in user-space. The reason being that there might be use-cases 
that are better dealt with there.

If the monitor thread is is strictly needed for some reason in kernel 
thread it should likely be moved to the V4L2 core as all drivers would 
then be able to use it instead of deeding on slightly different 
implementations in each driver.

So I fear we are back to only try to signal xfer errors in the driver 
and then leave it to either user-space or some new V4L2 code to help 
monitoring.

Sorry for only understanding this so late in the review, it took some 
time for me to understand it but once explained to me it made sens.

-- 
Kind Regards,
Niklas Söderlund

[PATCH v2 0/3] Improve error handling in the rcar-vin driver

[Michael Rodin]

Hello,

this series is a followup to the other series [1] started by Niklas Söderlund
where only the first patch has been merged. The overall idea is to be more
compliant with the Renesas hardware manual which requires a reset or stop
of capture in the VIN module before reset of CSI2. Another goal is to be
more resilient with respect to non-critical CSI2 errors so the driver does
not end in an endless restart loop. Compared to the previous version [2] of
this series the patch 3 is replaced based on the conclusion in [3] so now
userspace has to take care of figuring out if a transfer error was harmless
or unrecoverable. Other patches are adapted accordingly so no assumptions
about criticality of transfer errors are made in the kernel and the
decision is left up to userspace.

[1] https://lore.kernel.org/linux-renesas-soc/20211108160220.767586-1-niklas.soderlund+renesas@ragnatech.se/
[2] https://lore.kernel.org/all/1652983210-1194-1-git-send-email-mrodin@de.adit-jv.com/
[3] https://lore.kernel.org/all/YqEO3%2FKekkZhVjW+@oden.dyn.berto.se/

[PATCH 1/3] media: videobuf2: Add a transfer error event

[Michael Rodin]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Add a new V4L2_EVENT_XFER_ERROR event to signal if an error happens during
video transfer.

The use-case that sparked this new event is to signal to the video
device driver that an error has happen on the CSI-2 bus from the CSI-2
receiver subdevice.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
[mrodin@de.adit-jv.com: adapted information what to do if this new event is received]
Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>
---
 .../userspace-api/media/v4l/vidioc-dqevent.rst         | 10 ++++++++++
 .../userspace-api/media/videodev2.h.rst.exceptions     |  1 +
 include/uapi/linux/videodev2.h                         |  1 +
 3 files changed, 12 insertions(+)

diff --git a/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst b/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst
index 6eb40073c906..3cf0b4859784 100644
--- a/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst
+++ b/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst
@@ -182,6 +182,16 @@ call.
 	the regions changes. This event has a struct
 	:c:type:`v4l2_event_motion_det`
 	associated with it.
+    * - ``V4L2_EVENT_XFER_ERROR``
+      - 7
+      - This event is triggered when an transfer error is detected while
+	streaming. For example if an error is detected on a video bus in
+	the pipeline. If a driver receives this event from an upstream
+	subdevice, it has to forward the event to userspace. The streaming
+	application has to check if the transfer error is unrecoverable,
+	i.e. no new buffers can be dequeued from the kernel after the
+	expected time. If the error is unrecoverable, the streaming
+	application should restart streaming if it wants to continue.
     * - ``V4L2_EVENT_PRIVATE_START``
       - 0x08000000
       - Base event number for driver-private events.
diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
index 9cbb7a0c354a..25bde61a1519 100644
--- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
+++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
@@ -500,6 +500,7 @@ replace define V4L2_EVENT_CTRL event-type
 replace define V4L2_EVENT_FRAME_SYNC event-type
 replace define V4L2_EVENT_SOURCE_CHANGE event-type
 replace define V4L2_EVENT_MOTION_DET event-type
+replace define V4L2_EVENT_XFER_ERROR event-type
 replace define V4L2_EVENT_PRIVATE_START event-type
 
 replace define V4L2_EVENT_CTRL_CH_VALUE ctrl-changes-flags
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 5311ac4fde35..44db724d4541 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -2385,6 +2385,7 @@ struct v4l2_streamparm {
 #define V4L2_EVENT_FRAME_SYNC			4
 #define V4L2_EVENT_SOURCE_CHANGE		5
 #define V4L2_EVENT_MOTION_DET			6
+#define V4L2_EVENT_XFER_ERROR			7
 #define V4L2_EVENT_PRIVATE_START		0x08000000
 
 /* Payload for V4L2_EVENT_VSYNC */
-- 
2.25.1

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

[Michael Rodin]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

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 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>
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
[mrodin@de.adit-jv.com: removed the statement about stopping the pipeline from the commit message]
Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>
---
 .../media/platform/renesas/rcar-vin/rcar-csi2.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-csi2.c b/drivers/media/platform/renesas/rcar-vin/rcar-csi2.c
index fea8f00a9152..5a6494167c82 100644
--- a/drivers/media/platform/renesas/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/renesas/rcar-vin/rcar-csi2.c
@@ -943,21 +943,22 @@ 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_XFER_ERROR,
+	};
 
-	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);
+	/* Disable further interrupts to not spam the transfer error event. */
+	rcsi2_write(priv, INTEN_REG, 0);
+
+	dev_err(priv->dev, "Transfer error detected.\n");
+
+	v4l2_subdev_notify_event(&priv->subdev, &event);
 
 	return IRQ_HANDLED;
 }
-- 
2.25.1

[PATCH 3/3] media: rcar-vin: Allow userspace to subscribe to V4L2_EVENT_XFER_ERROR

[Michael Rodin]

Userspace should be able to subscribe to V4L2_EVENT_XFER_ERROR in order to
implement recovery from possible transfer errors. We can set the event
queue size to 1, since only the timestamp of the latest transfer error is
relevant to determine if a recovery is needed for the current streaming
session.

Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>
---
 drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c b/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c
index 2e2aa9d746ee..8118c8d41a66 100644
--- a/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c
@@ -648,6 +648,8 @@ static int rvin_subscribe_event(struct v4l2_fh *fh,
 	switch (sub->type) {
 	case V4L2_EVENT_SOURCE_CHANGE:
 		return v4l2_event_subscribe(fh, sub, 4, NULL);
+	case V4L2_EVENT_XFER_ERROR:
+		return v4l2_event_subscribe(fh, sub, 1, NULL);
 	}
 	return v4l2_ctrl_subscribe_event(fh, sub);
 }
-- 
2.25.1

Re: [PATCH 1/3] media: videobuf2: Add a transfer error event

[Nicolas Dufresne]

Hi Micheal,

thanks for your work, I have some questions below ...

Le mardi 28 juin 2022 à 20:00 +0200, Michael Rodin a écrit :
> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> Add a new V4L2_EVENT_XFER_ERROR event to signal if an error happens during
> video transfer.
> 
> The use-case that sparked this new event is to signal to the video
> device driver that an error has happen on the CSI-2 bus from the CSI-2
> receiver subdevice.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> [mrodin@de.adit-jv.com: adapted information what to do if this new event is received]
> Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>
> ---
>  .../userspace-api/media/v4l/vidioc-dqevent.rst         | 10 ++++++++++
>  .../userspace-api/media/videodev2.h.rst.exceptions     |  1 +
>  include/uapi/linux/videodev2.h                         |  1 +
>  3 files changed, 12 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst b/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst
> index 6eb40073c906..3cf0b4859784 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst
> @@ -182,6 +182,16 @@ call.
>  	the regions changes. This event has a struct
>  	:c:type:`v4l2_event_motion_det`
>  	associated with it.
> +    * - ``V4L2_EVENT_XFER_ERROR``

I'm not sure why this event is specific to XFER. Is there uses cases were a
future implementation would have both XFER and RECEIVER error ?

> +      - 7
> +      - This event is triggered when an transfer error is detected while
> +	streaming. For example if an error is detected on a video bus in
> +	the pipeline. If a driver receives this event from an upstream
> +	subdevice, it has to forward the event to userspace. The streaming
> +	application has to check if the transfer error is unrecoverable,
> +	i.e. no new buffers can be dequeued from the kernel after the
> +	expected time. If the error is unrecoverable, the streaming
> +	application should restart streaming if it wants to continue.

The process to determine if an error is recoverable or not isn't clear to me. As
an application developer, I would not know what to do here. Recoverable error
already have a designed mechanism, it consist of marking done a buffer with the
flag V4L2_BUF_FLAG_ERROR. I would like to understand what the existing mechanism
needed to be replaced, and the placement should be documented.

Nicolas

>      * - ``V4L2_EVENT_PRIVATE_START``
>        - 0x08000000
>        - Base event number for driver-private events.
> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> index 9cbb7a0c354a..25bde61a1519 100644
> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> @@ -500,6 +500,7 @@ replace define V4L2_EVENT_CTRL event-type
>  replace define V4L2_EVENT_FRAME_SYNC event-type
>  replace define V4L2_EVENT_SOURCE_CHANGE event-type
>  replace define V4L2_EVENT_MOTION_DET event-type
> +replace define V4L2_EVENT_XFER_ERROR event-type
>  replace define V4L2_EVENT_PRIVATE_START event-type
>  
>  replace define V4L2_EVENT_CTRL_CH_VALUE ctrl-changes-flags
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 5311ac4fde35..44db724d4541 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -2385,6 +2385,7 @@ struct v4l2_streamparm {
>  #define V4L2_EVENT_FRAME_SYNC			4
>  #define V4L2_EVENT_SOURCE_CHANGE		5
>  #define V4L2_EVENT_MOTION_DET			6
> +#define V4L2_EVENT_XFER_ERROR			7
>  #define V4L2_EVENT_PRIVATE_START		0x08000000
>  
>  /* Payload for V4L2_EVENT_VSYNC */

Re: [PATCH v2 0/3] Improve error handling in the rcar-vin driver

[Niklas Söderlund]

Hi Michael,

Thanks for your persistent work with this series.

On 2022-06-28 20:00:19 +0200, Michael Rodin wrote:
> Hello,
> 
> this series is a followup to the other series [1] started by Niklas Söderlund
> where only the first patch has been merged. The overall idea is to be more
> compliant with the Renesas hardware manual which requires a reset or stop
> of capture in the VIN module before reset of CSI2. Another goal is to be
> more resilient with respect to non-critical CSI2 errors so the driver does
> not end in an endless restart loop. Compared to the previous version [2] of
> this series the patch 3 is replaced based on the conclusion in [3] so now
> userspace has to take care of figuring out if a transfer error was harmless
> or unrecoverable. Other patches are adapted accordingly so no assumptions
> about criticality of transfer errors are made in the kernel and the
> decision is left up to userspace.

I like this solution as it truly pushes the decision to user-space. What 
bugs me a little bit is that we don't have a way to communicate errors 
that we know are unrecoverable (it was for this case the work in this 
area started) and ones that could be recoverable (the use-case added on 
top).

I would also like to hear what Hans thinks as he had good suggestions 
for how to handle the cases we know can't be recovers in [4].

> 
> [1] https://lore.kernel.org/linux-renesas-soc/20211108160220.767586-1-niklas.soderlund+renesas@ragnatech.se/
> [2] https://lore.kernel.org/all/1652983210-1194-1-git-send-email-mrodin@de.adit-jv.com/
> [3] https://lore.kernel.org/all/YqEO3%2FKekkZhVjW+@oden.dyn.berto.se/

4. https://lore.kernel.org/all/1fddc966-5a23-63b4-185e-c17aa6d65b54@xs4all.nl/

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH v2 0/3] Improve error handling in the rcar-vin driver

[Michael Rodin]

Hi Niklas, Hans,

On Tue, Jul 05, 2022 at 11:46:22AM +0200, Niklas Söderlund wrote:
> Hi Michael,
> 
> Thanks for your persistent work with this series.

Thank you for the feedback!

> On 2022-06-28 20:00:19 +0200, Michael Rodin wrote:
> > Hello,
> > 
> > this series is a followup to the other series [1] started by Niklas Söderlund
> > where only the first patch has been merged. The overall idea is to be more
> > compliant with the Renesas hardware manual which requires a reset or stop
> > of capture in the VIN module before reset of CSI2. Another goal is to be
> > more resilient with respect to non-critical CSI2 errors so the driver does
> > not end in an endless restart loop. Compared to the previous version [2] of
> > this series the patch 3 is replaced based on the conclusion in [3] so now
> > userspace has to take care of figuring out if a transfer error was harmless
> > or unrecoverable. Other patches are adapted accordingly so no assumptions
> > about criticality of transfer errors are made in the kernel and the
> > decision is left up to userspace.
> 
> I like this solution as it truly pushes the decision to user-space. What 
> bugs me a little bit is that we don't have a way to communicate errors 
> that we know are unrecoverable (it was for this case the work in this 
> area started) and ones that could be recoverable (the use-case added on 
> top).

Yes, it's not nice that V4L2_EVENT_XFER_ERROR does not tell userspace
whether an error is recoverable (i.e. the event can be ignored) or not
(i.e. a restart of streaming is required) but the other possible option
would be (as concluded in [3]) to implement a frame timeout monitoring
thread in v4l2 core. I am not sure if it is possible to implement this
second option cleanly...

> I would also like to hear what Hans thinks as he had good suggestions 
> for how to handle the cases we know can't be recovers in [4].

A a new function vb2_queue_error_with_event() suggested by Hans seems to be
redundant now, since it would not be used by rcar-vin (unless we implement
frame timeout monitoring in the v4l2 core). Or do you have an idea, which
drivers could be the first users of it, e.g. staging/media/imx I mentioned
before?

> > 
> > [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_linux-2Drenesas-2Dsoc_20211108160220.767586-2D1-2Dniklas.soderlund-2Brenesas-40ragnatech.se_&d=DwIDAw&c=euGZstcaTDllvimEN8b7jXrwqOf-v5A_CdpgnVfiiMM&r=sWsgk3pKkv5GeIDM2RZlPY8TjNFU2D0oBeOj6QNBadE&m=ecX7IwfatUO7SNPiyQ6x_8K9t2eWJf3y8GNuNHJ_0W0&s=Cli6jADEgMmCOLVoFekRRXzmty9WBXtoSF9utZJNMXY&e=
> > [2] https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_all_1652983210-2D1194-2D1-2Dgit-2Dsend-2Demail-2Dmrodin-40de.adit-2Djv.com_&d=DwIDAw&c=euGZstcaTDllvimEN8b7jXrwqOf-v5A_CdpgnVfiiMM&r=sWsgk3pKkv5GeIDM2RZlPY8TjNFU2D0oBeOj6QNBadE&m=ecX7IwfatUO7SNPiyQ6x_8K9t2eWJf3y8GNuNHJ_0W0&s=6CysfSY0OoAenEwCzigeyPOb8vyaa4GgzkJSR-ny83U&e=
> > [3] https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_all_YqEO3-252FKekkZhVjW-2B-40oden.dyn.berto.se_&d=DwIDAw&c=euGZstcaTDllvimEN8b7jXrwqOf-v5A_CdpgnVfiiMM&r=sWsgk3pKkv5GeIDM2RZlPY8TjNFU2D0oBeOj6QNBadE&m=ecX7IwfatUO7SNPiyQ6x_8K9t2eWJf3y8GNuNHJ_0W0&s=67JE_QR4x7omrtC7wzbpn2OgW75TAR80-R8WQyE-bVo&e=
> 
> 4. https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_all_1fddc966-2D5a23-2D63b4-2D185e-2Dc17aa6d65b54-40xs4all.nl_&d=DwIDAw&c=euGZstcaTDllvimEN8b7jXrwqOf-v5A_CdpgnVfiiMM&r=sWsgk3pKkv5GeIDM2RZlPY8TjNFU2D0oBeOj6QNBadE&m=ecX7IwfatUO7SNPiyQ6x_8K9t2eWJf3y8GNuNHJ_0W0&s=I18yWgde2UKZY4AiwB5s-Lf12eebHOcHFZFOlTcO2oQ&e=
> 
> -- 
> Kind Regards,
> Niklas Söderlund

-- 
Best Regards,
Michael

Re: [PATCH 1/3] media: videobuf2: Add a transfer error event

[Michael Rodin]

Hi Nicolas,

On Mon, Jul 04, 2022 at 11:59:58AM -0400, Nicolas Dufresne wrote:
> Hi Micheal,
> 
> thanks for your work, I have some questions below ...

Thank you for your feedback!

> Le mardi 28 juin 2022 à 20:00 +0200, Michael Rodin a écrit :
> > From: Niklas Söderlund <https://urldefense.proofpoint.com/v2/url?u=http-3A__niklas.soderlund-2Brenesas-40ragnatech.se&d=DwIFaQ&c=euGZstcaTDllvimEN8b7jXrwqOf-v5A_CdpgnVfiiMM&r=sWsgk3pKkv5GeIDM2RZlPY8TjNFU2D0oBeOj6QNBadE&m=7ktiIpDjee6bMSPLXXR7KVvJ_y234VytWEydKF2TWEo&s=-GUWUbGDkkrTAXiF_75xnL13cn3HYL2r2ZN0XwlG41U&e=>
> > 
> > Add a new V4L2_EVENT_XFER_ERROR event to signal if an error happens during
> > video transfer.
> > 
> > The use-case that sparked this new event is to signal to the video
> > device driver that an error has happen on the CSI-2 bus from the CSI-2
> > receiver subdevice.
> > 
> > Signed-off-by: Niklas Söderlund <https://urldefense.proofpoint.com/v2/url?u=http-3A__niklas.soderlund-2Brenesas-40ragnatech.se&d=DwIFaQ&c=euGZstcaTDllvimEN8b7jXrwqOf-v5A_CdpgnVfiiMM&r=sWsgk3pKkv5GeIDM2RZlPY8TjNFU2D0oBeOj6QNBadE&m=7ktiIpDjee6bMSPLXXR7KVvJ_y234VytWEydKF2TWEo&s=-GUWUbGDkkrTAXiF_75xnL13cn3HYL2r2ZN0XwlG41U&e=>
> > [mrodin@de.adit-jv.com: adapted information what to do if this new event is received]
> > Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>
> > ---
> >  .../userspace-api/media/v4l/vidioc-dqevent.rst         | 10 ++++++++++
> >  .../userspace-api/media/videodev2.h.rst.exceptions     |  1 +
> >  include/uapi/linux/videodev2.h                         |  1 +
> >  3 files changed, 12 insertions(+)
> > 
> > diff --git a/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst b/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst
> > index 6eb40073c906..3cf0b4859784 100644
> > --- a/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst
> > +++ b/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst
> > @@ -182,6 +182,16 @@ call.
> >  	the regions changes. This event has a struct
> >  	:c:type:`v4l2_event_motion_det`
> >  	associated with it.
> > +    * - ``V4L2_EVENT_XFER_ERROR``
> 
> I'm not sure why this event is specific to XFER. Is there uses cases were a
> future implementation would have both XFER and RECEIVER error ?

I am not sure whether I understand you correctly, do you mean that there is
already a method to signal a receiver error? Or that we should name it
V4L2_EVENT_RECEIVER_ERROR? I think that "transfer error" is a good name for
this event, because it could be sent by receiver or by transmitter drivers,
depending on their hardware error detection capabilities. We could have
e.g. a video transmitter which can detect an error coupled with a video
receiver which can not detect any errors.

> > +      - 7
> > +      - This event is triggered when an transfer error is detected while
> > +	streaming. For example if an error is detected on a video bus in
> > +	the pipeline. If a driver receives this event from an upstream
> > +	subdevice, it has to forward the event to userspace. The streaming
> > +	application has to check if the transfer error is unrecoverable,
> > +	i.e. no new buffers can be dequeued from the kernel after the
> > +	expected time. If the error is unrecoverable, the streaming
> > +	application should restart streaming if it wants to continue.
> 
> The process to determine if an error is recoverable or not isn't clear to me. As
> an application developer, I would not know what to do here. Recoverable error
> already have a designed mechanism, it consist of marking done a buffer with the
> flag V4L2_BUF_FLAG_ERROR. I would like to understand what the existing mechanism
> needed to be replaced, and the placement should be documented.

"Recoverable" means in this context that kernel space continues to capture
video buffers (which do not necessarily have the flag V4L2_BUF_FLAG_ERROR).
So probably we should not say "recoverable" or "unrecoverable" in the
context of this event to avoid confusion. V4L2_EVENT_XFER_ERROR just tells
userspace that it should restart streaming if the buffer flow stops after
this event. So would it be sufficient for an application developer if we
drop all statements about "recoverability" from the event description?

> Nicolas
> 
> >      * - ``V4L2_EVENT_PRIVATE_START``
> >        - 0x08000000
> >        - Base event number for driver-private events.
> > diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> > index 9cbb7a0c354a..25bde61a1519 100644
> > --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> > +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> > @@ -500,6 +500,7 @@ replace define V4L2_EVENT_CTRL event-type
> >  replace define V4L2_EVENT_FRAME_SYNC event-type
> >  replace define V4L2_EVENT_SOURCE_CHANGE event-type
> >  replace define V4L2_EVENT_MOTION_DET event-type
> > +replace define V4L2_EVENT_XFER_ERROR event-type
> >  replace define V4L2_EVENT_PRIVATE_START event-type
> >  
> >  replace define V4L2_EVENT_CTRL_CH_VALUE ctrl-changes-flags
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index 5311ac4fde35..44db724d4541 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -2385,6 +2385,7 @@ struct v4l2_streamparm {
> >  #define V4L2_EVENT_FRAME_SYNC			4
> >  #define V4L2_EVENT_SOURCE_CHANGE		5
> >  #define V4L2_EVENT_MOTION_DET			6
> > +#define V4L2_EVENT_XFER_ERROR			7
> >  #define V4L2_EVENT_PRIVATE_START		0x08000000
> >  
> >  /* Payload for V4L2_EVENT_VSYNC */
> 

-- 
Best Regards,
Michael

Re: [PATCH 1/3] media: videobuf2: Add a transfer error event

[Hans Verkuil]

Hi Michael,

Apologies for the late reply...

On 7/15/22 18:15, Michael Rodin wrote:
> Hi Nicolas,
> 
> On Mon, Jul 04, 2022 at 11:59:58AM -0400, Nicolas Dufresne wrote:
>> Hi Micheal,
>>
>> thanks for your work, I have some questions below ...
> 
> Thank you for your feedback!
> 
>> Le mardi 28 juin 2022 à 20:00 +0200, Michael Rodin a écrit :
>>> From: Niklas Söderlund <https://urldefense.proofpoint.com/v2/url?u=http-3A__niklas.soderlund-2Brenesas-40ragnatech.se&d=DwIFaQ&c=euGZstcaTDllvimEN8b7jXrwqOf-v5A_CdpgnVfiiMM&r=sWsgk3pKkv5GeIDM2RZlPY8TjNFU2D0oBeOj6QNBadE&m=7ktiIpDjee6bMSPLXXR7KVvJ_y234VytWEydKF2TWEo&s=-GUWUbGDkkrTAXiF_75xnL13cn3HYL2r2ZN0XwlG41U&e=>
>>>
>>> Add a new V4L2_EVENT_XFER_ERROR event to signal if an error happens during
>>> video transfer.
>>>
>>> The use-case that sparked this new event is to signal to the video
>>> device driver that an error has happen on the CSI-2 bus from the CSI-2
>>> receiver subdevice.
>>>
>>> Signed-off-by: Niklas Söderlund <https://urldefense.proofpoint.com/v2/url?u=http-3A__niklas.soderlund-2Brenesas-40ragnatech.se&d=DwIFaQ&c=euGZstcaTDllvimEN8b7jXrwqOf-v5A_CdpgnVfiiMM&r=sWsgk3pKkv5GeIDM2RZlPY8TjNFU2D0oBeOj6QNBadE&m=7ktiIpDjee6bMSPLXXR7KVvJ_y234VytWEydKF2TWEo&s=-GUWUbGDkkrTAXiF_75xnL13cn3HYL2r2ZN0XwlG41U&e=>
>>> [mrodin@de.adit-jv.com: adapted information what to do if this new event is received]
>>> Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>
>>> ---
>>>  .../userspace-api/media/v4l/vidioc-dqevent.rst         | 10 ++++++++++
>>>  .../userspace-api/media/videodev2.h.rst.exceptions     |  1 +
>>>  include/uapi/linux/videodev2.h                         |  1 +
>>>  3 files changed, 12 insertions(+)
>>>
>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst b/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst
>>> index 6eb40073c906..3cf0b4859784 100644
>>> --- a/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst
>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst
>>> @@ -182,6 +182,16 @@ call.
>>>  	the regions changes. This event has a struct
>>>  	:c:type:`v4l2_event_motion_det`
>>>  	associated with it.
>>> +    * - ``V4L2_EVENT_XFER_ERROR``
>>
>> I'm not sure why this event is specific to XFER. Is there uses cases were a
>> future implementation would have both XFER and RECEIVER error ?
> 
> I am not sure whether I understand you correctly, do you mean that there is
> already a method to signal a receiver error? Or that we should name it
> V4L2_EVENT_RECEIVER_ERROR? I think that "transfer error" is a good name for
> this event, because it could be sent by receiver or by transmitter drivers,
> depending on their hardware error detection capabilities. We could have
> e.g. a video transmitter which can detect an error coupled with a video
> receiver which can not detect any errors.
> 
>>> +      - 7
>>> +      - This event is triggered when an transfer error is detected while
>>> +	streaming. For example if an error is detected on a video bus in
>>> +	the pipeline. If a driver receives this event from an upstream
>>> +	subdevice, it has to forward the event to userspace. The streaming
>>> +	application has to check if the transfer error is unrecoverable,
>>> +	i.e. no new buffers can be dequeued from the kernel after the
>>> +	expected time. If the error is unrecoverable, the streaming
>>> +	application should restart streaming if it wants to continue.
>>
>> The process to determine if an error is recoverable or not isn't clear to me. As
>> an application developer, I would not know what to do here. Recoverable error
>> already have a designed mechanism, it consist of marking done a buffer with the
>> flag V4L2_BUF_FLAG_ERROR. I would like to understand what the existing mechanism
>> needed to be replaced, and the placement should be documented.
> 
> "Recoverable" means in this context that kernel space continues to capture
> video buffers (which do not necessarily have the flag V4L2_BUF_FLAG_ERROR).
> So probably we should not say "recoverable" or "unrecoverable" in the
> context of this event to avoid confusion. V4L2_EVENT_XFER_ERROR just tells
> userspace that it should restart streaming if the buffer flow stops after
> this event. So would it be sufficient for an application developer if we
> drop all statements about "recoverability" from the event description?

Here you touch on the core problem of this patch: you are basically saying
that userspace has to 1) subscribe to this event, 2) poll for it, 3) if it
arrives start a timer, 4) if the timer triggers and no new buffers have been
received in the meantime, then 5) restart streaming.

So in other words, you are just too lazy to do this in the driver and want
to hand it off to userspace.

That's not how it works. Usually the driver will know if the error is
recoverable or not (i.e. if an HDMI receiver loses signal, that's definitely
unrecoverable, and it's something the driver can know and call vb2_queue_error).

If it is really unknown, then you indeed need some monitoring thread. And
that's fine. Even better if you can make some helper things in the V4L2 core.

But you can't just kick that to userspace IMHO. I can guarantee that almost
no userspace application will do this and it is really not the job of userspace
to deal with such issues.

Regards,

	Hans

> 
>> Nicolas
>>
>>>      * - ``V4L2_EVENT_PRIVATE_START``
>>>        - 0x08000000
>>>        - Base event number for driver-private events.
>>> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>>> index 9cbb7a0c354a..25bde61a1519 100644
>>> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>>> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>>> @@ -500,6 +500,7 @@ replace define V4L2_EVENT_CTRL event-type
>>>  replace define V4L2_EVENT_FRAME_SYNC event-type
>>>  replace define V4L2_EVENT_SOURCE_CHANGE event-type
>>>  replace define V4L2_EVENT_MOTION_DET event-type
>>> +replace define V4L2_EVENT_XFER_ERROR event-type
>>>  replace define V4L2_EVENT_PRIVATE_START event-type
>>>  
>>>  replace define V4L2_EVENT_CTRL_CH_VALUE ctrl-changes-flags
>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>> index 5311ac4fde35..44db724d4541 100644
>>> --- a/include/uapi/linux/videodev2.h
>>> +++ b/include/uapi/linux/videodev2.h
>>> @@ -2385,6 +2385,7 @@ struct v4l2_streamparm {
>>>  #define V4L2_EVENT_FRAME_SYNC			4
>>>  #define V4L2_EVENT_SOURCE_CHANGE		5
>>>  #define V4L2_EVENT_MOTION_DET			6
>>> +#define V4L2_EVENT_XFER_ERROR			7
>>>  #define V4L2_EVENT_PRIVATE_START		0x08000000
>>>  
>>>  /* Payload for V4L2_EVENT_VSYNC */
>>
> 

Re: [PATCH 1/3] media: videobuf2: Add a transfer error event

[Michael Rodin]

Hi Hans,

On Tue, Aug 02, 2022 at 11:32:03AM +0200, Hans Verkuil wrote:
> Hi Michael,
> 
> Apologies for the late reply...

Thank you very much for your feedback, very appreciated!

> On 7/15/22 18:15, Michael Rodin wrote:
> > Hi Nicolas,
> > 
> > On Mon, Jul 04, 2022 at 11:59:58AM -0400, Nicolas Dufresne wrote:
> >> Hi Micheal,
> >>
> >> thanks for your work, I have some questions below ...
> > 
> > Thank you for your feedback!
> > 
> >> Le mardi 28 juin 2022 à 20:00 +0200, Michael Rodin a écrit :
> >>> From: Niklas Söderlund <https://urldefense.proofpoint.com/v2/url?u=http-3A__niklas.soderlund-2Brenesas-40ragnatech.se&d=DwIFaQ&c=euGZstcaTDllvimEN8b7jXrwqOf-v5A_CdpgnVfiiMM&r=sWsgk3pKkv5GeIDM2RZlPY8TjNFU2D0oBeOj6QNBadE&m=7ktiIpDjee6bMSPLXXR7KVvJ_y234VytWEydKF2TWEo&s=-GUWUbGDkkrTAXiF_75xnL13cn3HYL2r2ZN0XwlG41U&e=>
> >>>
> >>> Add a new V4L2_EVENT_XFER_ERROR event to signal if an error happens during
> >>> video transfer.
> >>>
> >>> The use-case that sparked this new event is to signal to the video
> >>> device driver that an error has happen on the CSI-2 bus from the CSI-2
> >>> receiver subdevice.
> >>>
> >>> Signed-off-by: Niklas Söderlund <https://urldefense.proofpoint.com/v2/url?u=http-3A__niklas.soderlund-2Brenesas-40ragnatech.se&d=DwIFaQ&c=euGZstcaTDllvimEN8b7jXrwqOf-v5A_CdpgnVfiiMM&r=sWsgk3pKkv5GeIDM2RZlPY8TjNFU2D0oBeOj6QNBadE&m=7ktiIpDjee6bMSPLXXR7KVvJ_y234VytWEydKF2TWEo&s=-GUWUbGDkkrTAXiF_75xnL13cn3HYL2r2ZN0XwlG41U&e=>
> >>> [mrodin@de.adit-jv.com: adapted information what to do if this new event is received]
> >>> Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>
> >>> ---
> >>>  .../userspace-api/media/v4l/vidioc-dqevent.rst         | 10 ++++++++++
> >>>  .../userspace-api/media/videodev2.h.rst.exceptions     |  1 +
> >>>  include/uapi/linux/videodev2.h                         |  1 +
> >>>  3 files changed, 12 insertions(+)
> >>>
> >>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst b/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst
> >>> index 6eb40073c906..3cf0b4859784 100644
> >>> --- a/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst
> >>> +++ b/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst
> >>> @@ -182,6 +182,16 @@ call.
> >>>  	the regions changes. This event has a struct
> >>>  	:c:type:`v4l2_event_motion_det`
> >>>  	associated with it.
> >>> +    * - ``V4L2_EVENT_XFER_ERROR``
> >>
> >> I'm not sure why this event is specific to XFER. Is there uses cases were a
> >> future implementation would have both XFER and RECEIVER error ?
> > 
> > I am not sure whether I understand you correctly, do you mean that there is
> > already a method to signal a receiver error? Or that we should name it
> > V4L2_EVENT_RECEIVER_ERROR? I think that "transfer error" is a good name for
> > this event, because it could be sent by receiver or by transmitter drivers,
> > depending on their hardware error detection capabilities. We could have
> > e.g. a video transmitter which can detect an error coupled with a video
> > receiver which can not detect any errors.
> > 
> >>> +      - 7
> >>> +      - This event is triggered when an transfer error is detected while
> >>> +	streaming. For example if an error is detected on a video bus in
> >>> +	the pipeline. If a driver receives this event from an upstream
> >>> +	subdevice, it has to forward the event to userspace. The streaming
> >>> +	application has to check if the transfer error is unrecoverable,
> >>> +	i.e. no new buffers can be dequeued from the kernel after the
> >>> +	expected time. If the error is unrecoverable, the streaming
> >>> +	application should restart streaming if it wants to continue.
> >>
> >> The process to determine if an error is recoverable or not isn't clear to me. As
> >> an application developer, I would not know what to do here. Recoverable error
> >> already have a designed mechanism, it consist of marking done a buffer with the
> >> flag V4L2_BUF_FLAG_ERROR. I would like to understand what the existing mechanism
> >> needed to be replaced, and the placement should be documented.
> > 
> > "Recoverable" means in this context that kernel space continues to capture
> > video buffers (which do not necessarily have the flag V4L2_BUF_FLAG_ERROR).
> > So probably we should not say "recoverable" or "unrecoverable" in the
> > context of this event to avoid confusion. V4L2_EVENT_XFER_ERROR just tells
> > userspace that it should restart streaming if the buffer flow stops after
> > this event. So would it be sufficient for an application developer if we
> > drop all statements about "recoverability" from the event description?
> 
> Here you touch on the core problem of this patch: you are basically saying
> that userspace has to 1) subscribe to this event, 2) poll for it, 3) if it
> arrives start a timer, 4) if the timer triggers and no new buffers have been
> received in the meantime, then 5) restart streaming.
> 
> So in other words, you are just too lazy to do this in the driver and want
> to hand it off to userspace.
> 
> That's not how it works. Usually the driver will know if the error is
> recoverable or not (i.e. if an HDMI receiver loses signal, that's definitely
> unrecoverable, and it's something the driver can know and call vb2_queue_error).
> 
> If it is really unknown, then you indeed need some monitoring thread. And
> that's fine. Even better if you can make some helper things in the V4L2 core.
> 
> But you can't just kick that to userspace IMHO. I can guarantee that almost
> no userspace application will do this and it is really not the job of userspace
> to deal with such issues.

From my understanding this means that my previous patch [1] already went in
the right direction by implementing a monitoring thread in rcar-vin. But on
the other hand Niklas has pointed out that it's not good to have this in a
driver [2]. Therefore it sounds like the only acceptable solution would be to
move this monitoring thread to the V4L2/VB2 core, which would then monitor
capture drivers for frame timeouts and maybe also notify userspace based
on this. What do you think? If you already have a solution in mind, I would
very appreciate if you could give me a few hints for an implementation!

[1] https://lore.kernel.org/lkml/1652983210-1194-4-git-send-email-mrodin@de.adit-jv.com/
[2] https://lore.kernel.org/lkml/YqEO3%2FKekkZhVjW+@oden.dyn.berto.se/

> Regards,
> 
> 	Hans
> 
> > 
> >> Nicolas
> >>
> >>>      * - ``V4L2_EVENT_PRIVATE_START``
> >>>        - 0x08000000
> >>>        - Base event number for driver-private events.
> >>> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> >>> index 9cbb7a0c354a..25bde61a1519 100644
> >>> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> >>> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> >>> @@ -500,6 +500,7 @@ replace define V4L2_EVENT_CTRL event-type
> >>>  replace define V4L2_EVENT_FRAME_SYNC event-type
> >>>  replace define V4L2_EVENT_SOURCE_CHANGE event-type
> >>>  replace define V4L2_EVENT_MOTION_DET event-type
> >>> +replace define V4L2_EVENT_XFER_ERROR event-type
> >>>  replace define V4L2_EVENT_PRIVATE_START event-type
> >>>  
> >>>  replace define V4L2_EVENT_CTRL_CH_VALUE ctrl-changes-flags
> >>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> >>> index 5311ac4fde35..44db724d4541 100644
> >>> --- a/include/uapi/linux/videodev2.h
> >>> +++ b/include/uapi/linux/videodev2.h
> >>> @@ -2385,6 +2385,7 @@ struct v4l2_streamparm {
> >>>  #define V4L2_EVENT_FRAME_SYNC			4
> >>>  #define V4L2_EVENT_SOURCE_CHANGE		5
> >>>  #define V4L2_EVENT_MOTION_DET			6
> >>> +#define V4L2_EVENT_XFER_ERROR			7
> >>>  #define V4L2_EVENT_PRIVATE_START		0x08000000
> >>>  
> >>>  /* Payload for V4L2_EVENT_VSYNC */
> >>
> > 
> 

-- 
Best Regards,
Michael

Re: [PATCH 1/3] media: videobuf2: Add a transfer error event

[Hans Verkuil]

Hi Michael,

On 08/08/2022 19:03, Michael Rodin wrote:
> Hi Hans,
> 
> On Tue, Aug 02, 2022 at 11:32:03AM +0200, Hans Verkuil wrote:
>> Hi Michael,
>>
>> Apologies for the late reply...
> 
> Thank you very much for your feedback, very appreciated!
> 
>> On 7/15/22 18:15, Michael Rodin wrote:
>>> Hi Nicolas,
>>>
>>> On Mon, Jul 04, 2022 at 11:59:58AM -0400, Nicolas Dufresne wrote:
>>>> Hi Micheal,
>>>>
>>>> thanks for your work, I have some questions below ...
>>>
>>> Thank you for your feedback!
>>>
>>>> Le mardi 28 juin 2022 à 20:00 +0200, Michael Rodin a écrit :
>>>>> From: Niklas Söderlund <https://urldefense.proofpoint.com/v2/url?u=http-3A__niklas.soderlund-2Brenesas-40ragnatech.se&d=DwIFaQ&c=euGZstcaTDllvimEN8b7jXrwqOf-v5A_CdpgnVfiiMM&r=sWsgk3pKkv5GeIDM2RZlPY8TjNFU2D0oBeOj6QNBadE&m=7ktiIpDjee6bMSPLXXR7KVvJ_y234VytWEydKF2TWEo&s=-GUWUbGDkkrTAXiF_75xnL13cn3HYL2r2ZN0XwlG41U&e=>
>>>>>
>>>>> Add a new V4L2_EVENT_XFER_ERROR event to signal if an error happens during
>>>>> video transfer.
>>>>>
>>>>> The use-case that sparked this new event is to signal to the video
>>>>> device driver that an error has happen on the CSI-2 bus from the CSI-2
>>>>> receiver subdevice.
>>>>>
>>>>> Signed-off-by: Niklas Söderlund <https://urldefense.proofpoint.com/v2/url?u=http-3A__niklas.soderlund-2Brenesas-40ragnatech.se&d=DwIFaQ&c=euGZstcaTDllvimEN8b7jXrwqOf-v5A_CdpgnVfiiMM&r=sWsgk3pKkv5GeIDM2RZlPY8TjNFU2D0oBeOj6QNBadE&m=7ktiIpDjee6bMSPLXXR7KVvJ_y234VytWEydKF2TWEo&s=-GUWUbGDkkrTAXiF_75xnL13cn3HYL2r2ZN0XwlG41U&e=>
>>>>> [mrodin@de.adit-jv.com: adapted information what to do if this new event is received]
>>>>> Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>
>>>>> ---
>>>>>  .../userspace-api/media/v4l/vidioc-dqevent.rst         | 10 ++++++++++
>>>>>  .../userspace-api/media/videodev2.h.rst.exceptions     |  1 +
>>>>>  include/uapi/linux/videodev2.h                         |  1 +
>>>>>  3 files changed, 12 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst b/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst
>>>>> index 6eb40073c906..3cf0b4859784 100644
>>>>> --- a/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst
>>>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-dqevent.rst
>>>>> @@ -182,6 +182,16 @@ call.
>>>>>  	the regions changes. This event has a struct
>>>>>  	:c:type:`v4l2_event_motion_det`
>>>>>  	associated with it.
>>>>> +    * - ``V4L2_EVENT_XFER_ERROR``
>>>>
>>>> I'm not sure why this event is specific to XFER. Is there uses cases were a
>>>> future implementation would have both XFER and RECEIVER error ?
>>>
>>> I am not sure whether I understand you correctly, do you mean that there is
>>> already a method to signal a receiver error? Or that we should name it
>>> V4L2_EVENT_RECEIVER_ERROR? I think that "transfer error" is a good name for
>>> this event, because it could be sent by receiver or by transmitter drivers,
>>> depending on their hardware error detection capabilities. We could have
>>> e.g. a video transmitter which can detect an error coupled with a video
>>> receiver which can not detect any errors.
>>>
>>>>> +      - 7
>>>>> +      - This event is triggered when an transfer error is detected while
>>>>> +	streaming. For example if an error is detected on a video bus in
>>>>> +	the pipeline. If a driver receives this event from an upstream
>>>>> +	subdevice, it has to forward the event to userspace. The streaming
>>>>> +	application has to check if the transfer error is unrecoverable,
>>>>> +	i.e. no new buffers can be dequeued from the kernel after the
>>>>> +	expected time. If the error is unrecoverable, the streaming
>>>>> +	application should restart streaming if it wants to continue.
>>>>
>>>> The process to determine if an error is recoverable or not isn't clear to me. As
>>>> an application developer, I would not know what to do here. Recoverable error
>>>> already have a designed mechanism, it consist of marking done a buffer with the
>>>> flag V4L2_BUF_FLAG_ERROR. I would like to understand what the existing mechanism
>>>> needed to be replaced, and the placement should be documented.
>>>
>>> "Recoverable" means in this context that kernel space continues to capture
>>> video buffers (which do not necessarily have the flag V4L2_BUF_FLAG_ERROR).
>>> So probably we should not say "recoverable" or "unrecoverable" in the
>>> context of this event to avoid confusion. V4L2_EVENT_XFER_ERROR just tells
>>> userspace that it should restart streaming if the buffer flow stops after
>>> this event. So would it be sufficient for an application developer if we
>>> drop all statements about "recoverability" from the event description?
>>
>> Here you touch on the core problem of this patch: you are basically saying
>> that userspace has to 1) subscribe to this event, 2) poll for it, 3) if it
>> arrives start a timer, 4) if the timer triggers and no new buffers have been
>> received in the meantime, then 5) restart streaming.
>>
>> So in other words, you are just too lazy to do this in the driver and want
>> to hand it off to userspace.
>>
>> That's not how it works. Usually the driver will know if the error is
>> recoverable or not (i.e. if an HDMI receiver loses signal, that's definitely
>> unrecoverable, and it's something the driver can know and call vb2_queue_error).
>>
>> If it is really unknown, then you indeed need some monitoring thread. And
>> that's fine. Even better if you can make some helper things in the V4L2 core.
>>
>> But you can't just kick that to userspace IMHO. I can guarantee that almost
>> no userspace application will do this and it is really not the job of userspace
>> to deal with such issues.
> 
> From my understanding this means that my previous patch [1] already went in
> the right direction by implementing a monitoring thread in rcar-vin. But on
> the other hand Niklas has pointed out that it's not good to have this in a
> driver [2]. Therefore it sounds like the only acceptable solution would be to
> move this monitoring thread to the V4L2/VB2 core, which would then monitor
> capture drivers for frame timeouts and maybe also notify userspace based
> on this. What do you think? If you already have a solution in mind, I would
> very appreciate if you could give me a few hints for an implementation!
> 
> [1] https://lore.kernel.org/lkml/1652983210-1194-4-git-send-email-mrodin@de.adit-jv.com/
> [2] https://lore.kernel.org/lkml/YqEO3%2FKekkZhVjW+@oden.dyn.berto.se/

Yes, [1] goes in the right direction. The V4L2_EVENT_XFER_ERROR addition can
be dropped, since a call to vb2_queue_error() is sufficient for non-recoverable
transfer errors.

BTW, I think a timer might work better than a workqueue here: no need to cancel
and reschedule the workqueue in interrupt context, instead the isr can just call
mod_timer. Calling vb2_queue_error can be done from interrupt context as well,
so that can safely be called from the timer function.

I would not be opposed to keep this as driver-specific functionality: it's really
not much code. But if you do want to add this to vb2, then that can be done, of
course. I think vb2_queue needs a new field: watchdog_timeout. If 0, there is no
watchdog running, if non-0 vb2 would start a timer after start_streaming (and
stop it in stop_streaming), and a new vb2_watchdog_reset() is added that calls
mod_timer and that the driver has to call from the interrupt.

Finally, the timer function would call vb2_queue_error().

Eventually, a vb2_queue op can be added as well (e.g. watchdog_timeout) to do
driver specific stuff. It doesn't seem to be needed for rcar-vin, though.

Regards,

	Hans

> 
>> Regards,
>>
>> 	Hans
>>
>>>
>>>> Nicolas
>>>>
>>>>>      * - ``V4L2_EVENT_PRIVATE_START``
>>>>>        - 0x08000000
>>>>>        - Base event number for driver-private events.
>>>>> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>>>>> index 9cbb7a0c354a..25bde61a1519 100644
>>>>> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>>>>> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>>>>> @@ -500,6 +500,7 @@ replace define V4L2_EVENT_CTRL event-type
>>>>>  replace define V4L2_EVENT_FRAME_SYNC event-type
>>>>>  replace define V4L2_EVENT_SOURCE_CHANGE event-type
>>>>>  replace define V4L2_EVENT_MOTION_DET event-type
>>>>> +replace define V4L2_EVENT_XFER_ERROR event-type
>>>>>  replace define V4L2_EVENT_PRIVATE_START event-type
>>>>>  
>>>>>  replace define V4L2_EVENT_CTRL_CH_VALUE ctrl-changes-flags
>>>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>>>> index 5311ac4fde35..44db724d4541 100644
>>>>> --- a/include/uapi/linux/videodev2.h
>>>>> +++ b/include/uapi/linux/videodev2.h
>>>>> @@ -2385,6 +2385,7 @@ struct v4l2_streamparm {
>>>>>  #define V4L2_EVENT_FRAME_SYNC			4
>>>>>  #define V4L2_EVENT_SOURCE_CHANGE		5
>>>>>  #define V4L2_EVENT_MOTION_DET			6
>>>>> +#define V4L2_EVENT_XFER_ERROR			7
>>>>>  #define V4L2_EVENT_PRIVATE_START		0x08000000
>>>>>  
>>>>>  /* Payload for V4L2_EVENT_VSYNC */
>>>>
>>>
>>
>