[PATCH 1/2] [RFC] media: rcar-vin: send a V4L2 event to vdev if no frame captured after a timeout

[PATCH 1/2] [RFC] media: rcar-vin: send a V4L2 event to vdev if no frame captured after a timeout

[Michael Rodin]

Data flow from an upstream subdevice can stop permanently due to:
 - CSI2 transmission errors
 - silent failure of the source subdevice
 - disconnection of the source subdevice
In those cases userspace waits for new buffers for an infinitely long time.
In order to address this issue, use a timer to monitor, that rvin_irq() is
capturing at least one frame within a IRQ_TIMEOUT_MS period. Otherwise send
a new private v4l2 event to userspace. This event is exported to userspace
via a new uapi header.

Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>
---
 drivers/media/platform/rcar-vin/rcar-dma.c  | 21 +++++++++++++++++++++
 drivers/media/platform/rcar-vin/rcar-v4l2.c |  1 +
 drivers/media/platform/rcar-vin/rcar-vin.h  |  6 ++++++
 include/uapi/linux/rcar-vin.h               | 10 ++++++++++
 4 files changed, 38 insertions(+)
 create mode 100644 include/uapi/linux/rcar-vin.h

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index 1a30cd0..bf8d733 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -937,6 +937,20 @@ static void rvin_capture_stop(struct rvin_dev *vin)
 #define RVIN_TIMEOUT_MS 100
 #define RVIN_RETRIES 10
 
+static const struct v4l2_event rvin_irq_timeout = {
+	.type = V4L2_EVENT_RCAR_VIN_IRQ_TIMEOUT,
+};
+
+static void rvin_irq_timer_function(struct timer_list *timer)
+{
+	struct rvin_dev *vin = container_of(timer, struct rvin_dev,
+					    irq_timer);
+
+	vin_err(vin, "%s: frame completion timeout after %i ms!\n",
+		__func__, IRQ_TIMEOUT_MS);
+	v4l2_event_queue(&vin->vdev, &rvin_irq_timeout);
+}
+
 static irqreturn_t rvin_irq(int irq, void *data)
 {
 	struct rvin_dev *vin = data;
@@ -1008,6 +1022,8 @@ static irqreturn_t rvin_irq(int irq, void *data)
 		vin_dbg(vin, "Dropping frame %u\n", vin->sequence);
 	}
 
+	mod_timer(&vin->irq_timer, jiffies + msecs_to_jiffies(IRQ_TIMEOUT_MS));
+
 	vin->sequence++;
 
 	/* Prepare for next frame */
@@ -1252,6 +1268,8 @@ static int rvin_start_streaming(struct vb2_queue *vq, unsigned int count)
 	if (ret)
 		dma_free_coherent(vin->dev, vin->format.sizeimage, vin->scratch,
 				  vin->scratch_phys);
+	else
+		mod_timer(&vin->irq_timer, jiffies + msecs_to_jiffies(IRQ_TIMEOUT_MS));
 
 	return ret;
 }
@@ -1305,6 +1323,8 @@ static void rvin_stop_streaming(struct vb2_queue *vq)
 	/* Free scratch buffer. */
 	dma_free_coherent(vin->dev, vin->format.sizeimage, vin->scratch,
 			  vin->scratch_phys);
+
+	del_timer_sync(&vin->irq_timer);
 }
 
 static const struct vb2_ops rvin_qops = {
@@ -1370,6 +1390,7 @@ int rvin_dma_register(struct rvin_dev *vin, int irq)
 		goto error;
 	}
 
+	timer_setup(&vin->irq_timer, rvin_irq_timer_function, 0);
 	return 0;
 error:
 	rvin_dma_unregister(vin);
diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index f421e25..c644134 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -581,6 +581,7 @@ static int rvin_subscribe_event(struct v4l2_fh *fh,
 {
 	switch (sub->type) {
 	case V4L2_EVENT_SOURCE_CHANGE:
+	case V4L2_EVENT_RCAR_VIN_IRQ_TIMEOUT:
 		return v4l2_event_subscribe(fh, sub, 4, NULL);
 	}
 	return v4l2_ctrl_subscribe_event(fh, sub);
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index c19d077..7408f67 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -14,12 +14,14 @@
 #define __RCAR_VIN__
 
 #include <linux/kref.h>
+#include <linux/rcar-vin.h>
 
 #include <media/v4l2-async.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-dev.h>
 #include <media/v4l2-device.h>
 #include <media/videobuf2-v4l2.h>
+#include <media/v4l2-event.h>
 
 /* Number of HW buffers */
 #define HW_BUFFER_NUM 3
@@ -30,6 +32,8 @@
 /* Max number on VIN instances that can be in a system */
 #define RCAR_VIN_NUM 8
 
+#define IRQ_TIMEOUT_MS 1000
+
 struct rvin_group;
 
 enum model_id {
@@ -196,6 +200,7 @@ struct rvin_info {
  * @compose:		active composing
  * @src_rect:		active size of the video source
  * @std:		active video standard of the video source
+ * @irq_timer:		monitors regular capturing of frames in rvin_irq()
  *
  * @alpha:		Alpha component to fill in for supported pixel formats
  */
@@ -240,6 +245,7 @@ struct rvin_dev {
 	struct v4l2_rect src_rect;
 	v4l2_std_id std;
 
+	struct timer_list irq_timer;
 	unsigned int alpha;
 };
 
diff --git a/include/uapi/linux/rcar-vin.h b/include/uapi/linux/rcar-vin.h
new file mode 100644
index 00000000..4eb7f5e
--- /dev/null
+++ b/include/uapi/linux/rcar-vin.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+
+#ifndef RCAR_VIN_USER_H
+#define RCAR_VIN_USER_H
+
+/* class for events sent by the rcar-vin driver */
+#define V4L2_EVENT_RCAR_VIN_CLASS	V4L2_EVENT_PRIVATE_START
+#define V4L2_EVENT_RCAR_VIN_IRQ_TIMEOUT	(V4L2_EVENT_RCAR_VIN_CLASS | 0x1)
+
+#endif /* RCAR_VIN_USER_H */
-- 
2.7.4

[PATCH 2/2] [RFC] media: rcar-vin: make timeout of the irq timer configurable via v4l2 control

[Michael Rodin]

Different framerates imply different periods between successive frames.
Therefore it is reasonable to make the timeout of the irq_timer
configurable at the run time so userspace can avoid unnecessary long
waiting times. Therefore this commit adds a new custom control to rcar-vin.
This allows to change the timeout before the start of streaming.

Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 17 ++++++++++++++++-
 drivers/media/platform/rcar-vin/rcar-dma.c  | 14 +++++++++-----
 drivers/media/platform/rcar-vin/rcar-vin.h  |  5 +++++
 include/uapi/linux/rcar-vin.h               |  7 +++++++
 include/uapi/linux/v4l2-controls.h          |  6 ++++++
 5 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 7440c89..81faa87 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -884,6 +884,17 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
 	return 0;
 }
 
+static const struct v4l2_ctrl_config rvin_ctrl_irq_timeout = {
+	.ops = &rvin_ctrl_ops,
+	.id = V4L2_CID_RCAR_VIN_IRQ_TIMEOUT,
+	.name = "frame completion timeout",
+	.type = V4L2_CTRL_TYPE_INTEGER,
+	.min = 0,
+	.max = S32_MAX,
+	.step = 1,
+	.def = IRQ_TIMEOUT_MS,
+};
+
 static int rvin_mc_init(struct rvin_dev *vin)
 {
 	int ret;
@@ -901,13 +912,17 @@ static int rvin_mc_init(struct rvin_dev *vin)
 	if (ret)
 		rvin_group_put(vin);
 
-	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 1);
+	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 2);
 	if (ret < 0)
 		return ret;
 
 	v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops,
 			  V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255);
 
+	vin->ctrl_irq_timeout = v4l2_ctrl_new_custom(&vin->ctrl_handler,
+						     &rvin_ctrl_irq_timeout,
+						     NULL);
+
 	if (vin->ctrl_handler.error) {
 		ret = vin->ctrl_handler.error;
 		v4l2_ctrl_handler_free(&vin->ctrl_handler);
diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index bf8d733..062f338 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -947,7 +947,7 @@ static void rvin_irq_timer_function(struct timer_list *timer)
 					    irq_timer);
 
 	vin_err(vin, "%s: frame completion timeout after %i ms!\n",
-		__func__, IRQ_TIMEOUT_MS);
+		__func__, jiffies_to_msecs(vin->irq_timeout));
 	v4l2_event_queue(&vin->vdev, &rvin_irq_timeout);
 }
 
@@ -1022,7 +1022,7 @@ static irqreturn_t rvin_irq(int irq, void *data)
 		vin_dbg(vin, "Dropping frame %u\n", vin->sequence);
 	}
 
-	mod_timer(&vin->irq_timer, jiffies + msecs_to_jiffies(IRQ_TIMEOUT_MS));
+	mod_timer(&vin->irq_timer, jiffies + vin->irq_timeout);
 
 	vin->sequence++;
 
@@ -1233,6 +1233,7 @@ static int rvin_start_streaming(struct vb2_queue *vq, unsigned int count)
 	struct rvin_dev *vin = vb2_get_drv_priv(vq);
 	unsigned long flags;
 	int ret;
+	s32 irq_timeout = v4l2_ctrl_g_ctrl(vin->ctrl_irq_timeout);
 
 	/* Allocate scratch buffer. */
 	vin->scratch = dma_alloc_coherent(vin->dev, vin->format.sizeimage,
@@ -1265,11 +1266,14 @@ static int rvin_start_streaming(struct vb2_queue *vq, unsigned int count)
 
 	spin_unlock_irqrestore(&vin->qlock, flags);
 out:
-	if (ret)
+	if (ret) {
 		dma_free_coherent(vin->dev, vin->format.sizeimage, vin->scratch,
 				  vin->scratch_phys);
-	else
-		mod_timer(&vin->irq_timer, jiffies + msecs_to_jiffies(IRQ_TIMEOUT_MS));
+	} else {
+		vin->irq_timeout = msecs_to_jiffies(irq_timeout);
+		mod_timer(&vin->irq_timer, jiffies + vin->irq_timeout);
+		vin_dbg(vin, "%s: set irq timer to %i ms\n", __func__, irq_timeout);
+	}
 
 	return ret;
 }
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index 7408f67..4e514e7 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -201,6 +201,9 @@ struct rvin_info {
  * @src_rect:		active size of the video source
  * @std:		active video standard of the video source
  * @irq_timer:		monitors regular capturing of frames in rvin_irq()
+ * @irq_timeout:	timeout value in jiffies, which is currently used by
+ *			@irq_timer and can be changed via @ctrl_irq_timeout
+ * @ctrl_irq_timeout:	controls value of @irq_timeout
  *
  * @alpha:		Alpha component to fill in for supported pixel formats
  */
@@ -246,6 +249,8 @@ struct rvin_dev {
 	v4l2_std_id std;
 
 	struct timer_list irq_timer;
+	unsigned long irq_timeout;
+	struct v4l2_ctrl *ctrl_irq_timeout;
 	unsigned int alpha;
 };
 
diff --git a/include/uapi/linux/rcar-vin.h b/include/uapi/linux/rcar-vin.h
index 4eb7f5e..6c72584 100644
--- a/include/uapi/linux/rcar-vin.h
+++ b/include/uapi/linux/rcar-vin.h
@@ -3,8 +3,15 @@
 #ifndef RCAR_VIN_USER_H
 #define RCAR_VIN_USER_H
 
+#include <linux/v4l2-controls.h>
+
 /* class for events sent by the rcar-vin driver */
 #define V4L2_EVENT_RCAR_VIN_CLASS	V4L2_EVENT_PRIVATE_START
 #define V4L2_EVENT_RCAR_VIN_IRQ_TIMEOUT	(V4L2_EVENT_RCAR_VIN_CLASS | 0x1)
 
+/* private controls for rcar-vin */
+enum rcar_vin_ctrl_id {
+	V4L2_CID_RCAR_VIN_IRQ_TIMEOUT = V4L2_CID_USER_RCAR_VIN_BASE
+};
+
 #endif /* RCAR_VIN_USER_H */
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index 6227141..cf4f085 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -198,6 +198,12 @@ enum v4l2_colorfx {
  */
 #define V4L2_CID_USER_ATMEL_ISC_BASE		(V4L2_CID_USER_BASE + 0x10c0)
 
+/*
+ * The base for the rcar-vin driver controls.
+ * We reserve 16 controls for this driver.
+ */
+#define V4L2_CID_USER_RCAR_VIN_BASE		(V4L2_CID_USER_BASE + 0x10d0)
+
 /* MPEG-class control IDs */
 /* The MPEG controls are applicable to all codec controls
  * and the 'MPEG' part of the define is historical */
-- 
2.7.4

Re: [PATCH 1/2] [RFC] media: rcar-vin: send a V4L2 event to vdev if no frame captured after a timeout

[Niklas Söderlund]

Hi Michael,

Thanks for your RFC.

On 2020-06-19 19:46:10 +0200, Michael Rodin wrote:
> Data flow from an upstream subdevice can stop permanently due to:
>  - CSI2 transmission errors
>  - silent failure of the source subdevice
>  - disconnection of the source subdevice
> In those cases userspace waits for new buffers for an infinitely long time.
> In order to address this issue, use a timer to monitor, that rvin_irq() is
> capturing at least one frame within a IRQ_TIMEOUT_MS period. Otherwise send
> a new private v4l2 event to userspace. This event is exported to userspace
> via a new uapi header.

I think there is value for user-space to detecting the error cases 
above. But I think the problem could be addressed at a different lever.  
Defining a VIN specific events and controls for something that applies 
any video device might not be the neatest solution.

Another thing hits me when reading this series, could this not be done 
in user-space? In 2/2 you add a control which sets the timeout based on 
the framerate, so user-space must know about that to be able to set the 
control. User-space also knows when it receives/dequeus a buffer from 
the video device so the timeout logic could be implemented in the 
application. Given that the application anyhow needs special care to 
handle the VIN specific event and control I wonder if it's not neater to 
make it handle all of it. Do you see any specific benefit of having 
parts of it in the driver?

> 
> Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>
> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c  | 21 +++++++++++++++++++++
>  drivers/media/platform/rcar-vin/rcar-v4l2.c |  1 +
>  drivers/media/platform/rcar-vin/rcar-vin.h  |  6 ++++++
>  include/uapi/linux/rcar-vin.h               | 10 ++++++++++
>  4 files changed, 38 insertions(+)
>  create mode 100644 include/uapi/linux/rcar-vin.h
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index 1a30cd0..bf8d733 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -937,6 +937,20 @@ static void rvin_capture_stop(struct rvin_dev *vin)
>  #define RVIN_TIMEOUT_MS 100
>  #define RVIN_RETRIES 10
>  
> +static const struct v4l2_event rvin_irq_timeout = {
> +	.type = V4L2_EVENT_RCAR_VIN_IRQ_TIMEOUT,
> +};
> +
> +static void rvin_irq_timer_function(struct timer_list *timer)
> +{
> +	struct rvin_dev *vin = container_of(timer, struct rvin_dev,
> +					    irq_timer);
> +
> +	vin_err(vin, "%s: frame completion timeout after %i ms!\n",
> +		__func__, IRQ_TIMEOUT_MS);
> +	v4l2_event_queue(&vin->vdev, &rvin_irq_timeout);
> +}
> +
>  static irqreturn_t rvin_irq(int irq, void *data)
>  {
>  	struct rvin_dev *vin = data;
> @@ -1008,6 +1022,8 @@ static irqreturn_t rvin_irq(int irq, void *data)
>  		vin_dbg(vin, "Dropping frame %u\n", vin->sequence);
>  	}
>  
> +	mod_timer(&vin->irq_timer, jiffies + msecs_to_jiffies(IRQ_TIMEOUT_MS));
> +
>  	vin->sequence++;
>  
>  	/* Prepare for next frame */
> @@ -1252,6 +1268,8 @@ static int rvin_start_streaming(struct vb2_queue *vq, unsigned int count)
>  	if (ret)
>  		dma_free_coherent(vin->dev, vin->format.sizeimage, vin->scratch,
>  				  vin->scratch_phys);
> +	else
> +		mod_timer(&vin->irq_timer, jiffies + msecs_to_jiffies(IRQ_TIMEOUT_MS));
>  
>  	return ret;
>  }
> @@ -1305,6 +1323,8 @@ static void rvin_stop_streaming(struct vb2_queue *vq)
>  	/* Free scratch buffer. */
>  	dma_free_coherent(vin->dev, vin->format.sizeimage, vin->scratch,
>  			  vin->scratch_phys);
> +
> +	del_timer_sync(&vin->irq_timer);
>  }
>  
>  static const struct vb2_ops rvin_qops = {
> @@ -1370,6 +1390,7 @@ int rvin_dma_register(struct rvin_dev *vin, int irq)
>  		goto error;
>  	}
>  
> +	timer_setup(&vin->irq_timer, rvin_irq_timer_function, 0);
>  	return 0;
>  error:
>  	rvin_dma_unregister(vin);
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index f421e25..c644134 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -581,6 +581,7 @@ static int rvin_subscribe_event(struct v4l2_fh *fh,
>  {
>  	switch (sub->type) {
>  	case V4L2_EVENT_SOURCE_CHANGE:
> +	case V4L2_EVENT_RCAR_VIN_IRQ_TIMEOUT:
>  		return v4l2_event_subscribe(fh, sub, 4, NULL);
>  	}
>  	return v4l2_ctrl_subscribe_event(fh, sub);
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> index c19d077..7408f67 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -14,12 +14,14 @@
>  #define __RCAR_VIN__
>  
>  #include <linux/kref.h>
> +#include <linux/rcar-vin.h>
>  
>  #include <media/v4l2-async.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-dev.h>
>  #include <media/v4l2-device.h>
>  #include <media/videobuf2-v4l2.h>
> +#include <media/v4l2-event.h>
>  
>  /* Number of HW buffers */
>  #define HW_BUFFER_NUM 3
> @@ -30,6 +32,8 @@
>  /* Max number on VIN instances that can be in a system */
>  #define RCAR_VIN_NUM 8
>  
> +#define IRQ_TIMEOUT_MS 1000
> +
>  struct rvin_group;
>  
>  enum model_id {
> @@ -196,6 +200,7 @@ struct rvin_info {
>   * @compose:		active composing
>   * @src_rect:		active size of the video source
>   * @std:		active video standard of the video source
> + * @irq_timer:		monitors regular capturing of frames in rvin_irq()
>   *
>   * @alpha:		Alpha component to fill in for supported pixel formats
>   */
> @@ -240,6 +245,7 @@ struct rvin_dev {
>  	struct v4l2_rect src_rect;
>  	v4l2_std_id std;
>  
> +	struct timer_list irq_timer;
>  	unsigned int alpha;
>  };
>  
> diff --git a/include/uapi/linux/rcar-vin.h b/include/uapi/linux/rcar-vin.h
> new file mode 100644
> index 00000000..4eb7f5e
> --- /dev/null
> +++ b/include/uapi/linux/rcar-vin.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +
> +#ifndef RCAR_VIN_USER_H
> +#define RCAR_VIN_USER_H
> +
> +/* class for events sent by the rcar-vin driver */
> +#define V4L2_EVENT_RCAR_VIN_CLASS	V4L2_EVENT_PRIVATE_START
> +#define V4L2_EVENT_RCAR_VIN_IRQ_TIMEOUT	(V4L2_EVENT_RCAR_VIN_CLASS | 0x1)
> +
> +#endif /* RCAR_VIN_USER_H */
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH 1/2] [RFC] media: rcar-vin: send a V4L2 event to vdev if no frame captured after a timeout

[Michael Rodin]

Hi Niklas,

On Wed, Jul 01, 2020 at 12:17:10AM +0200, Niklas Söderlund wrote:
> Hi Michael,
> 
> Thanks for your RFC.

Thanks your your feedback!

> On 2020-06-19 19:46:10 +0200, Michael Rodin wrote:
> > Data flow from an upstream subdevice can stop permanently due to:
> >  - CSI2 transmission errors
> >  - silent failure of the source subdevice
> >  - disconnection of the source subdevice
> > In those cases userspace waits for new buffers for an infinitely long time.
> > In order to address this issue, use a timer to monitor, that rvin_irq() is
> > capturing at least one frame within a IRQ_TIMEOUT_MS period. Otherwise send
> > a new private v4l2 event to userspace. This event is exported to userspace
> > via a new uapi header.
> 
> I think there is value for user-space to detecting the error cases 
> above. But I think the problem could be addressed at a different lever.  
> Defining a VIN specific events and controls for something that applies 
> any video device might not be the neatest solution.
> 
> Another thing hits me when reading this series, could this not be done 
> in user-space? In 2/2 you add a control which sets the timeout based on 
> the framerate, so user-space must know about that to be able to set the 
> control. User-space also knows when it receives/dequeus a buffer from 
> the video device so the timeout logic could be implemented in the 
> application. Given that the application anyhow needs special care to 
> handle the VIN specific event and control I wonder if it's not neater to 
> make it handle all of it. Do you see any specific benefit of having 
> parts of it in the driver?

Originally I have started this patch series to implement a replacement for
the CSI-2 error handling you have added in
commit 4ab44ff ("media: rcar-csi2: restart CSI-2 link if error is detected"),
which is not correct for multiple reasons:
1. The commit message states that the user is informed that something is
   not right. But you have just added new messages which only appear in
   dmesg. This might be sufficient for a desktop PC but not for an embedded
   system, where the user normally can not see dmesg log. So I think that
   V4L2 events are the correct solution for this kind of notification,
   because they are passed directly to the application and the developer
   can implement handling for this issue or display an error in the
   custom human-machine interface.
2. It is not correct to restart the CSI-2 link if you don't restart VIN
   module as well. Renesas HW manual R19UH0105EJ0200 Rev.2.00
   (Jul 31, 2019) requires a reset or stop of capture in the VIN module
   before a reset of CSI-2 module (chapter 25.3.13 "Software Reset"). This
   also applies to CSI-2 error handling.
3. The CSI-2 driver restarts CSI-2 module for any CSI-2 error. However not
   all CSI2 errors are critical. In some setups they are really harmless so
   streaming can continue without any unnecessary restart. In some setups
   they _always_ occur at each start of streaming and are harmless as
   well, so automatic restart in CSI-2 module ends in an endless restart
   loop, which never comes to an end and breaks streaming instead of any
   help. It is better to leave such errors unhandled and therefore it is
   important to detect whether DMA transfers really stop in rvin_irq().
4. Video streaming applications in the automotive/embedded industry often
   want to control when the video streaming pipeline is stopped or started
   to be able to do some tasks in between, so an automatic restart of the
   pipeline is not acceptable for them. It should be at least optional and
   we should do this in rcar-dma.c, e.g. in the proposed irq timeout
   function. However I am not sure yet how to implement a restart of
   streaming inside of the rcar-vin driver correctly.

I think, my patch series provides technically a good solution for the
described issues. Also it is generic enough to allow also handling of
failures in upstream subdevices connected to an R-Car3 CSI2 receiver or
even parallel video input devices in cases when such failures can not be
fixed or detected in the subdevice drivers and result in a stop of data
flow on the chip level.

Theoretically, applications also could use timeout parameter of the poll()
syscall to implement the timeout (which can be e.g. a multiple of the frame
interval), but the problem is that userspace does not know whether the
timeout happened because there are no DMA transfers in the driver (i.e. one
of the upstream subdevices or VIN failed) or because the driver is just
using the "scratch buffer". The event which I have introduced explicitly
monitors whether rcar-vin regularly receives new frames from upstream
and allows applications to try a recovery (I have now renamed the event to
"FRAME_TIMEOUT" to be more precise about its purpose).

Another reason, why I think that the new v4l2 event is the right solution,
are proprietary applications, where it is not possible to change the code
to add any additional handling of driver failures but it is possible to
start/stop streaming via inter process communication. Since V4L2 events can
be subscribed and received by a separate process, it is possible to
implement a middleware in user space, which monitors V4L2 events. Typically
this middleware could also take over all of the complicated media-ctl
configuration and monitoring of source changes and other events from
subdevices, but this is a bit off-topic. I think that (private) V4L2 events
are really useful in embedded systems where applications/middlewares are
aware of the underlying hardware and want to be better informed about
hardware related events than desktop applications.

So if my arguments sound reasonable and you don't reject the overall
concept of the series, I would send an improved version, where I have fixed
some details of the timer implementation. I have also a patch for rcar-csi2
driver with a private CSI-2 error event, which is useful to let the
application know that the reason for the frame timeout event might be a
CSI-2 error and not e.g. paused playback.

> > 
> > Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-dma.c  | 21 +++++++++++++++++++++
> >  drivers/media/platform/rcar-vin/rcar-v4l2.c |  1 +
> >  drivers/media/platform/rcar-vin/rcar-vin.h  |  6 ++++++
> >  include/uapi/linux/rcar-vin.h               | 10 ++++++++++
> >  4 files changed, 38 insertions(+)
> >  create mode 100644 include/uapi/linux/rcar-vin.h
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > index 1a30cd0..bf8d733 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -937,6 +937,20 @@ static void rvin_capture_stop(struct rvin_dev *vin)
> >  #define RVIN_TIMEOUT_MS 100
> >  #define RVIN_RETRIES 10
> >  
> > +static const struct v4l2_event rvin_irq_timeout = {
> > +	.type = V4L2_EVENT_RCAR_VIN_IRQ_TIMEOUT,
> > +};
> > +
> > +static void rvin_irq_timer_function(struct timer_list *timer)
> > +{
> > +	struct rvin_dev *vin = container_of(timer, struct rvin_dev,
> > +					    irq_timer);
> > +
> > +	vin_err(vin, "%s: frame completion timeout after %i ms!\n",
> > +		__func__, IRQ_TIMEOUT_MS);
> > +	v4l2_event_queue(&vin->vdev, &rvin_irq_timeout);
> > +}
> > +
> >  static irqreturn_t rvin_irq(int irq, void *data)
> >  {
> >  	struct rvin_dev *vin = data;
> > @@ -1008,6 +1022,8 @@ static irqreturn_t rvin_irq(int irq, void *data)
> >  		vin_dbg(vin, "Dropping frame %u\n", vin->sequence);
> >  	}
> >  
> > +	mod_timer(&vin->irq_timer, jiffies + msecs_to_jiffies(IRQ_TIMEOUT_MS));
> > +
> >  	vin->sequence++;
> >  
> >  	/* Prepare for next frame */
> > @@ -1252,6 +1268,8 @@ static int rvin_start_streaming(struct vb2_queue *vq, unsigned int count)
> >  	if (ret)
> >  		dma_free_coherent(vin->dev, vin->format.sizeimage, vin->scratch,
> >  				  vin->scratch_phys);
> > +	else
> > +		mod_timer(&vin->irq_timer, jiffies + msecs_to_jiffies(IRQ_TIMEOUT_MS));
> >  
> >  	return ret;
> >  }
> > @@ -1305,6 +1323,8 @@ static void rvin_stop_streaming(struct vb2_queue *vq)
> >  	/* Free scratch buffer. */
> >  	dma_free_coherent(vin->dev, vin->format.sizeimage, vin->scratch,
> >  			  vin->scratch_phys);
> > +
> > +	del_timer_sync(&vin->irq_timer);
> >  }
> >  
> >  static const struct vb2_ops rvin_qops = {
> > @@ -1370,6 +1390,7 @@ int rvin_dma_register(struct rvin_dev *vin, int irq)
> >  		goto error;
> >  	}
> >  
> > +	timer_setup(&vin->irq_timer, rvin_irq_timer_function, 0);
> >  	return 0;
> >  error:
> >  	rvin_dma_unregister(vin);
> > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > index f421e25..c644134 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > @@ -581,6 +581,7 @@ static int rvin_subscribe_event(struct v4l2_fh *fh,
> >  {
> >  	switch (sub->type) {
> >  	case V4L2_EVENT_SOURCE_CHANGE:
> > +	case V4L2_EVENT_RCAR_VIN_IRQ_TIMEOUT:
> >  		return v4l2_event_subscribe(fh, sub, 4, NULL);
> >  	}
> >  	return v4l2_ctrl_subscribe_event(fh, sub);
> > diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> > index c19d077..7408f67 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> > @@ -14,12 +14,14 @@
> >  #define __RCAR_VIN__
> >  
> >  #include <linux/kref.h>
> > +#include <linux/rcar-vin.h>
> >  
> >  #include <media/v4l2-async.h>
> >  #include <media/v4l2-ctrls.h>
> >  #include <media/v4l2-dev.h>
> >  #include <media/v4l2-device.h>
> >  #include <media/videobuf2-v4l2.h>
> > +#include <media/v4l2-event.h>
> >  
> >  /* Number of HW buffers */
> >  #define HW_BUFFER_NUM 3
> > @@ -30,6 +32,8 @@
> >  /* Max number on VIN instances that can be in a system */
> >  #define RCAR_VIN_NUM 8
> >  
> > +#define IRQ_TIMEOUT_MS 1000
> > +
> >  struct rvin_group;
> >  
> >  enum model_id {
> > @@ -196,6 +200,7 @@ struct rvin_info {
> >   * @compose:		active composing
> >   * @src_rect:		active size of the video source
> >   * @std:		active video standard of the video source
> > + * @irq_timer:		monitors regular capturing of frames in rvin_irq()
> >   *
> >   * @alpha:		Alpha component to fill in for supported pixel formats
> >   */
> > @@ -240,6 +245,7 @@ struct rvin_dev {
> >  	struct v4l2_rect src_rect;
> >  	v4l2_std_id std;
> >  
> > +	struct timer_list irq_timer;
> >  	unsigned int alpha;
> >  };
> >  
> > diff --git a/include/uapi/linux/rcar-vin.h b/include/uapi/linux/rcar-vin.h
> > new file mode 100644
> > index 00000000..4eb7f5e
> > --- /dev/null
> > +++ b/include/uapi/linux/rcar-vin.h
> > @@ -0,0 +1,10 @@
> > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > +
> > +#ifndef RCAR_VIN_USER_H
> > +#define RCAR_VIN_USER_H
> > +
> > +/* class for events sent by the rcar-vin driver */
> > +#define V4L2_EVENT_RCAR_VIN_CLASS	V4L2_EVENT_PRIVATE_START
> > +#define V4L2_EVENT_RCAR_VIN_IRQ_TIMEOUT	(V4L2_EVENT_RCAR_VIN_CLASS | 0x1)
> > +
> > +#endif /* RCAR_VIN_USER_H */
> > -- 
> > 2.7.4
> > 
> 
> -- 
> Regards,
> Niklas Söderlund

-- 
Best Regards,
Michael

Re: [PATCH 1/2] [RFC] media: rcar-vin: send a V4L2 event to vdev if no frame captured after a timeout

[Niklas Söderlund]

Hi Michael,

On 2020-07-02 14:33:41 +0200, Michael Rodin wrote:
> Hi Niklas,
> 
> On Wed, Jul 01, 2020 at 12:17:10AM +0200, Niklas Söderlund wrote:
> > Hi Michael,
> > 
> > Thanks for your RFC.
> 
> Thanks your your feedback!
> 
> > On 2020-06-19 19:46:10 +0200, Michael Rodin wrote:
> > > Data flow from an upstream subdevice can stop permanently due to:
> > >  - CSI2 transmission errors
> > >  - silent failure of the source subdevice
> > >  - disconnection of the source subdevice
> > > In those cases userspace waits for new buffers for an infinitely long time.
> > > In order to address this issue, use a timer to monitor, that rvin_irq() is
> > > capturing at least one frame within a IRQ_TIMEOUT_MS period. Otherwise send
> > > a new private v4l2 event to userspace. This event is exported to userspace
> > > via a new uapi header.
> > 
> > I think there is value for user-space to detecting the error cases 
> > above. But I think the problem could be addressed at a different lever.  
> > Defining a VIN specific events and controls for something that applies 
> > any video device might not be the neatest solution.
> > 
> > Another thing hits me when reading this series, could this not be done 
> > in user-space? In 2/2 you add a control which sets the timeout based on 
> > the framerate, so user-space must know about that to be able to set the 
> > control. User-space also knows when it receives/dequeus a buffer from 
> > the video device so the timeout logic could be implemented in the 
> > application. Given that the application anyhow needs special care to 
> > handle the VIN specific event and control I wonder if it's not neater to 
> > make it handle all of it. Do you see any specific benefit of having 
> > parts of it in the driver?
> 
> Originally I have started this patch series to implement a replacement for
> the CSI-2 error handling you have added in
> commit 4ab44ff ("media: rcar-csi2: restart CSI-2 link if error is detected"),
> which is not correct for multiple reasons:
> 1. The commit message states that the user is informed that something is
>    not right. But you have just added new messages which only appear in
>    dmesg. This might be sufficient for a desktop PC but not for an embedded
>    system, where the user normally can not see dmesg log. So I think that
>    V4L2 events are the correct solution for this kind of notification,
>    because they are passed directly to the application and the developer
>    can implement handling for this issue or display an error in the
>    custom human-machine interface.
> 2. It is not correct to restart the CSI-2 link if you don't restart VIN
>    module as well. Renesas HW manual R19UH0105EJ0200 Rev.2.00
>    (Jul 31, 2019) requires a reset or stop of capture in the VIN module
>    before a reset of CSI-2 module (chapter 25.3.13 "Software Reset"). This
>    also applies to CSI-2 error handling.
> 3. The CSI-2 driver restarts CSI-2 module for any CSI-2 error. However not
>    all CSI2 errors are critical. In some setups they are really harmless so
>    streaming can continue without any unnecessary restart. In some setups
>    they _always_ occur at each start of streaming and are harmless as
>    well, so automatic restart in CSI-2 module ends in an endless restart
>    loop, which never comes to an end and breaks streaming instead of any
>    help. It is better to leave such errors unhandled and therefore it is
>    important to detect whether DMA transfers really stop in rvin_irq().
> 4. Video streaming applications in the automotive/embedded industry often
>    want to control when the video streaming pipeline is stopped or started
>    to be able to do some tasks in between, so an automatic restart of the
>    pipeline is not acceptable for them. It should be at least optional and
>    we should do this in rcar-dma.c, e.g. in the proposed irq timeout
>    function. However I am not sure yet how to implement a restart of
>    streaming inside of the rcar-vin driver correctly.
> 
> I think, my patch series provides technically a good solution for the
> described issues. Also it is generic enough to allow also handling of
> failures in upstream subdevices connected to an R-Car3 CSI2 receiver or
> even parallel video input devices in cases when such failures can not be
> fixed or detected in the subdevice drivers and result in a stop of data
> flow on the chip level.
> 
> Theoretically, applications also could use timeout parameter of the poll()
> syscall to implement the timeout (which can be e.g. a multiple of the frame
> interval), but the problem is that userspace does not know whether the
> timeout happened because there are no DMA transfers in the driver (i.e. one
> of the upstream subdevices or VIN failed) or because the driver is just
> using the "scratch buffer". The event which I have introduced explicitly
> monitors whether rcar-vin regularly receives new frames from upstream
> and allows applications to try a recovery (I have now renamed the event to
> "FRAME_TIMEOUT" to be more precise about its purpose).
> 
> Another reason, why I think that the new v4l2 event is the right solution,
> are proprietary applications, where it is not possible to change the code
> to add any additional handling of driver failures but it is possible to
> start/stop streaming via inter process communication. Since V4L2 events can
> be subscribed and received by a separate process, it is possible to
> implement a middleware in user space, which monitors V4L2 events. Typically
> this middleware could also take over all of the complicated media-ctl
> configuration and monitoring of source changes and other events from
> subdevices, but this is a bit off-topic. I think that (private) V4L2 events
> are really useful in embedded systems where applications/middlewares are
> aware of the underlying hardware and want to be better informed about
> hardware related events than desktop applications.
> 
> So if my arguments sound reasonable and you don't reject the overall
> concept of the series, I would send an improved version, where I have fixed
> some details of the timer implementation. I have also a patch for rcar-csi2
> driver with a private CSI-2 error event, which is useful to let the
> application know that the reason for the frame timeout event might be a
> CSI-2 error and not e.g. paused playback.

I'm not arguing that something is needed for user-space to detect these 
situations. I'm arguing that adding these events directly into rcar-vin 
and/or rcar-csi2 is the wrong level. In my view either support should be 
added in the V4L2 subsystem core or in the user-space applications.  
Adding custom events and logic to a single driver does not create 
reusable and generic solution which can be used by many.

If you wish to work the problem in the kernel I think you should do so 
on a V4L2 level and not in the individual driver. As the needs you 
describe above could be useful for others users.

> 
> > > 
> > > Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>
> > > ---
> > >  drivers/media/platform/rcar-vin/rcar-dma.c  | 21 +++++++++++++++++++++
> > >  drivers/media/platform/rcar-vin/rcar-v4l2.c |  1 +
> > >  drivers/media/platform/rcar-vin/rcar-vin.h  |  6 ++++++
> > >  include/uapi/linux/rcar-vin.h               | 10 ++++++++++
> > >  4 files changed, 38 insertions(+)
> > >  create mode 100644 include/uapi/linux/rcar-vin.h
> > > 
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > index 1a30cd0..bf8d733 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > @@ -937,6 +937,20 @@ static void rvin_capture_stop(struct rvin_dev *vin)
> > >  #define RVIN_TIMEOUT_MS 100
> > >  #define RVIN_RETRIES 10
> > >  
> > > +static const struct v4l2_event rvin_irq_timeout = {
> > > +	.type = V4L2_EVENT_RCAR_VIN_IRQ_TIMEOUT,
> > > +};
> > > +
> > > +static void rvin_irq_timer_function(struct timer_list *timer)
> > > +{
> > > +	struct rvin_dev *vin = container_of(timer, struct rvin_dev,
> > > +					    irq_timer);
> > > +
> > > +	vin_err(vin, "%s: frame completion timeout after %i ms!\n",
> > > +		__func__, IRQ_TIMEOUT_MS);
> > > +	v4l2_event_queue(&vin->vdev, &rvin_irq_timeout);
> > > +}
> > > +
> > >  static irqreturn_t rvin_irq(int irq, void *data)
> > >  {
> > >  	struct rvin_dev *vin = data;
> > > @@ -1008,6 +1022,8 @@ static irqreturn_t rvin_irq(int irq, void *data)
> > >  		vin_dbg(vin, "Dropping frame %u\n", vin->sequence);
> > >  	}
> > >  
> > > +	mod_timer(&vin->irq_timer, jiffies + msecs_to_jiffies(IRQ_TIMEOUT_MS));
> > > +
> > >  	vin->sequence++;
> > >  
> > >  	/* Prepare for next frame */
> > > @@ -1252,6 +1268,8 @@ static int rvin_start_streaming(struct vb2_queue *vq, unsigned int count)
> > >  	if (ret)
> > >  		dma_free_coherent(vin->dev, vin->format.sizeimage, vin->scratch,
> > >  				  vin->scratch_phys);
> > > +	else
> > > +		mod_timer(&vin->irq_timer, jiffies + msecs_to_jiffies(IRQ_TIMEOUT_MS));
> > >  
> > >  	return ret;
> > >  }
> > > @@ -1305,6 +1323,8 @@ static void rvin_stop_streaming(struct vb2_queue *vq)
> > >  	/* Free scratch buffer. */
> > >  	dma_free_coherent(vin->dev, vin->format.sizeimage, vin->scratch,
> > >  			  vin->scratch_phys);
> > > +
> > > +	del_timer_sync(&vin->irq_timer);
> > >  }
> > >  
> > >  static const struct vb2_ops rvin_qops = {
> > > @@ -1370,6 +1390,7 @@ int rvin_dma_register(struct rvin_dev *vin, int irq)
> > >  		goto error;
> > >  	}
> > >  
> > > +	timer_setup(&vin->irq_timer, rvin_irq_timer_function, 0);
> > >  	return 0;
> > >  error:
> > >  	rvin_dma_unregister(vin);
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > index f421e25..c644134 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > @@ -581,6 +581,7 @@ static int rvin_subscribe_event(struct v4l2_fh *fh,
> > >  {
> > >  	switch (sub->type) {
> > >  	case V4L2_EVENT_SOURCE_CHANGE:
> > > +	case V4L2_EVENT_RCAR_VIN_IRQ_TIMEOUT:
> > >  		return v4l2_event_subscribe(fh, sub, 4, NULL);
> > >  	}
> > >  	return v4l2_ctrl_subscribe_event(fh, sub);
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> > > index c19d077..7408f67 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> > > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> > > @@ -14,12 +14,14 @@
> > >  #define __RCAR_VIN__
> > >  
> > >  #include <linux/kref.h>
> > > +#include <linux/rcar-vin.h>
> > >  
> > >  #include <media/v4l2-async.h>
> > >  #include <media/v4l2-ctrls.h>
> > >  #include <media/v4l2-dev.h>
> > >  #include <media/v4l2-device.h>
> > >  #include <media/videobuf2-v4l2.h>
> > > +#include <media/v4l2-event.h>
> > >  
> > >  /* Number of HW buffers */
> > >  #define HW_BUFFER_NUM 3
> > > @@ -30,6 +32,8 @@
> > >  /* Max number on VIN instances that can be in a system */
> > >  #define RCAR_VIN_NUM 8
> > >  
> > > +#define IRQ_TIMEOUT_MS 1000
> > > +
> > >  struct rvin_group;
> > >  
> > >  enum model_id {
> > > @@ -196,6 +200,7 @@ struct rvin_info {
> > >   * @compose:		active composing
> > >   * @src_rect:		active size of the video source
> > >   * @std:		active video standard of the video source
> > > + * @irq_timer:		monitors regular capturing of frames in rvin_irq()
> > >   *
> > >   * @alpha:		Alpha component to fill in for supported pixel formats
> > >   */
> > > @@ -240,6 +245,7 @@ struct rvin_dev {
> > >  	struct v4l2_rect src_rect;
> > >  	v4l2_std_id std;
> > >  
> > > +	struct timer_list irq_timer;
> > >  	unsigned int alpha;
> > >  };
> > >  
> > > diff --git a/include/uapi/linux/rcar-vin.h b/include/uapi/linux/rcar-vin.h
> > > new file mode 100644
> > > index 00000000..4eb7f5e
> > > --- /dev/null
> > > +++ b/include/uapi/linux/rcar-vin.h
> > > @@ -0,0 +1,10 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > > +
> > > +#ifndef RCAR_VIN_USER_H
> > > +#define RCAR_VIN_USER_H
> > > +
> > > +/* class for events sent by the rcar-vin driver */
> > > +#define V4L2_EVENT_RCAR_VIN_CLASS	V4L2_EVENT_PRIVATE_START
> > > +#define V4L2_EVENT_RCAR_VIN_IRQ_TIMEOUT	(V4L2_EVENT_RCAR_VIN_CLASS | 0x1)
> > > +
> > > +#endif /* RCAR_VIN_USER_H */
> > > -- 
> > > 2.7.4
> > > 
> > 
> > -- 
> > Regards,
> > Niklas Söderlund
> 
> -- 
> Best Regards,
> Michael

-- 
Regards,
Niklas Söderlund