* Improve error handling in the rcar-vin driver @ 2022-05-19 18:00 Michael Rodin 2022-05-19 18:00 ` [PATCH 1/3] media: videobuf2: Add a transfer error event Michael Rodin ` (2 more replies) 0 siblings, 3 replies; 18+ messages in thread From: Michael Rodin @ 2022-05-19 18:00 UTC (permalink / raw) To: Mauro Carvalho Chehab, Niklas Söderlund, linux-media, linux-kernel, linux-renesas-soc Cc: Michael Rodin, michael, erosca 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/ ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/3] media: videobuf2: Add a transfer error event 2022-05-19 18:00 Improve error handling in the rcar-vin driver Michael Rodin @ 2022-05-19 18:00 ` Michael Rodin 2022-05-19 18:00 ` [PATCH 2/3] rcar-csi2: Do not try to recover after transfer error Michael Rodin 2022-05-19 18:00 ` [PATCH 3/3] rcar-vin: handle transfer errors from subdevices and stop streaming if required Michael Rodin 2 siblings, 0 replies; 18+ messages in thread From: Michael Rodin @ 2022-05-19 18:00 UTC (permalink / raw) To: Mauro Carvalho Chehab, Niklas Söderlund, linux-media, linux-kernel, linux-renesas-soc Cc: Michael Rodin, michael, erosca, Niklas Söderlund 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/3] rcar-csi2: Do not try to recover after transfer error 2022-05-19 18:00 Improve error handling in the rcar-vin driver Michael Rodin 2022-05-19 18:00 ` [PATCH 1/3] media: videobuf2: Add a transfer error event Michael Rodin @ 2022-05-19 18:00 ` Michael Rodin 2022-05-19 18:00 ` [PATCH 3/3] rcar-vin: handle transfer errors from subdevices and stop streaming if required Michael Rodin 2 siblings, 0 replies; 18+ messages in thread From: Michael Rodin @ 2022-05-19 18:00 UTC (permalink / raw) To: Mauro Carvalho Chehab, Niklas Söderlund, linux-media, linux-kernel, linux-renesas-soc Cc: Michael Rodin, michael, erosca, Niklas Söderlund 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 3/3] rcar-vin: handle transfer errors from subdevices and stop streaming if required 2022-05-19 18:00 Improve error handling in the rcar-vin driver Michael Rodin 2022-05-19 18:00 ` [PATCH 1/3] media: videobuf2: Add a transfer error event Michael Rodin 2022-05-19 18:00 ` [PATCH 2/3] rcar-csi2: Do not try to recover after transfer error Michael Rodin @ 2022-05-19 18:00 ` Michael Rodin 2022-05-19 21:00 ` Niklas Söderlund 2 siblings, 1 reply; 18+ messages in thread From: Michael Rodin @ 2022-05-19 18:00 UTC (permalink / raw) To: Mauro Carvalho Chehab, Niklas Söderlund, linux-media, linux-kernel, linux-renesas-soc Cc: Michael Rodin, michael, erosca 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] rcar-vin: handle transfer errors from subdevices and stop streaming if required 2022-05-19 18:00 ` [PATCH 3/3] rcar-vin: handle transfer errors from subdevices and stop streaming if required Michael Rodin @ 2022-05-19 21:00 ` Niklas Söderlund 2022-05-20 19:50 ` Michael Rodin 0 siblings, 1 reply; 18+ messages in thread From: Niklas Söderlund @ 2022-05-19 21:00 UTC (permalink / raw) To: Michael Rodin Cc: Mauro Carvalho Chehab, linux-media, linux-kernel, linux-renesas-soc, michael, erosca 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] rcar-vin: handle transfer errors from subdevices and stop streaming if required 2022-05-19 21:00 ` Niklas Söderlund @ 2022-05-20 19:50 ` Michael Rodin 2022-06-08 21:04 ` Niklas Söderlund 0 siblings, 1 reply; 18+ messages in thread From: Michael Rodin @ 2022-05-20 19:50 UTC (permalink / raw) To: Niklas Söderlund Cc: Michael Rodin, Mauro Carvalho Chehab, linux-media, linux-kernel, linux-renesas-soc, michael, erosca 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] rcar-vin: handle transfer errors from subdevices and stop streaming if required 2022-05-20 19:50 ` Michael Rodin @ 2022-06-08 21:04 ` Niklas Söderlund 2022-06-28 18:00 ` [PATCH v2 0/3] Improve error handling in the rcar-vin driver Michael Rodin 0 siblings, 1 reply; 18+ messages in thread From: Niklas Söderlund @ 2022-06-08 21:04 UTC (permalink / raw) To: Michael Rodin Cc: Mauro Carvalho Chehab, linux-media, linux-kernel, linux-renesas-soc, michael, erosca 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 0/3] Improve error handling in the rcar-vin driver 2022-06-08 21:04 ` Niklas Söderlund @ 2022-06-28 18:00 ` Michael Rodin 2022-06-28 18:00 ` [PATCH 1/3] media: videobuf2: Add a transfer error event Michael Rodin ` (3 more replies) 0 siblings, 4 replies; 18+ messages in thread From: Michael Rodin @ 2022-06-28 18:00 UTC (permalink / raw) To: Mauro Carvalho Chehab, Niklas Söderlund, linux-media, linux-kernel, linux-renesas-soc Cc: Michael Rodin, michael, erosca 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/ ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/3] media: videobuf2: Add a transfer error event 2022-06-28 18:00 ` [PATCH v2 0/3] Improve error handling in the rcar-vin driver Michael Rodin @ 2022-06-28 18:00 ` Michael Rodin 2022-07-04 15:59 ` Nicolas Dufresne 2022-06-28 18:00 ` [PATCH 2/3] rcar-csi2: Do not try to recover after transfer error Michael Rodin ` (2 subsequent siblings) 3 siblings, 1 reply; 18+ messages in thread From: Michael Rodin @ 2022-06-28 18:00 UTC (permalink / raw) To: Mauro Carvalho Chehab, Niklas Söderlund, linux-media, linux-kernel, linux-renesas-soc Cc: Michael Rodin, michael, erosca, Niklas Söderlund 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] media: videobuf2: Add a transfer error event 2022-06-28 18:00 ` [PATCH 1/3] media: videobuf2: Add a transfer error event Michael Rodin @ 2022-07-04 15:59 ` Nicolas Dufresne 2022-07-15 16:15 ` Michael Rodin 0 siblings, 1 reply; 18+ messages in thread From: Nicolas Dufresne @ 2022-07-04 15:59 UTC (permalink / raw) To: Michael Rodin, Mauro Carvalho Chehab, Niklas Söderlund, linux-media, linux-kernel, linux-renesas-soc Cc: michael, erosca, Niklas Söderlund 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 */ ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] media: videobuf2: Add a transfer error event 2022-07-04 15:59 ` Nicolas Dufresne @ 2022-07-15 16:15 ` Michael Rodin 2022-08-02 9:32 ` Hans Verkuil 0 siblings, 1 reply; 18+ messages in thread From: Michael Rodin @ 2022-07-15 16:15 UTC (permalink / raw) To: Nicolas Dufresne Cc: Michael Rodin, Mauro Carvalho Chehab, Niklas Söderlund, linux-media, linux-kernel, linux-renesas-soc, michael, erosca, Niklas Söderlund 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] media: videobuf2: Add a transfer error event 2022-07-15 16:15 ` Michael Rodin @ 2022-08-02 9:32 ` Hans Verkuil 2022-08-08 17:03 ` Michael Rodin 0 siblings, 1 reply; 18+ messages in thread From: Hans Verkuil @ 2022-08-02 9:32 UTC (permalink / raw) To: Michael Rodin, Nicolas Dufresne Cc: Mauro Carvalho Chehab, Niklas Söderlund, linux-media, linux-kernel, linux-renesas-soc, michael, erosca, Niklas Söderlund 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 */ >> > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] media: videobuf2: Add a transfer error event 2022-08-02 9:32 ` Hans Verkuil @ 2022-08-08 17:03 ` Michael Rodin 2022-08-09 7:12 ` Hans Verkuil 0 siblings, 1 reply; 18+ messages in thread From: Michael Rodin @ 2022-08-08 17:03 UTC (permalink / raw) To: Hans Verkuil Cc: Michael Rodin, Nicolas Dufresne, Mauro Carvalho Chehab, Niklas Söderlund, linux-media, linux-kernel, linux-renesas-soc, michael, erosca, Niklas Söderlund 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] media: videobuf2: Add a transfer error event 2022-08-08 17:03 ` Michael Rodin @ 2022-08-09 7:12 ` Hans Verkuil 0 siblings, 0 replies; 18+ messages in thread From: Hans Verkuil @ 2022-08-09 7:12 UTC (permalink / raw) To: Michael Rodin Cc: Nicolas Dufresne, Mauro Carvalho Chehab, Niklas Söderlund, linux-media, linux-kernel, linux-renesas-soc, michael, erosca, Niklas Söderlund 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 */ >>>> >>> >> > ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/3] rcar-csi2: Do not try to recover after transfer error 2022-06-28 18:00 ` [PATCH v2 0/3] Improve error handling in the rcar-vin driver Michael Rodin 2022-06-28 18:00 ` [PATCH 1/3] media: videobuf2: Add a transfer error event Michael Rodin @ 2022-06-28 18:00 ` Michael Rodin 2022-06-28 18:00 ` [PATCH 3/3] media: rcar-vin: Allow userspace to subscribe to V4L2_EVENT_XFER_ERROR Michael Rodin 2022-07-05 9:46 ` [PATCH v2 0/3] Improve error handling in the rcar-vin driver Niklas Söderlund 3 siblings, 0 replies; 18+ messages in thread From: Michael Rodin @ 2022-06-28 18:00 UTC (permalink / raw) To: Mauro Carvalho Chehab, Niklas Söderlund, linux-media, linux-kernel, linux-renesas-soc Cc: Michael Rodin, michael, erosca, Niklas Söderlund, Jacopo Mondi 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 3/3] media: rcar-vin: Allow userspace to subscribe to V4L2_EVENT_XFER_ERROR 2022-06-28 18:00 ` [PATCH v2 0/3] Improve error handling in the rcar-vin driver Michael Rodin 2022-06-28 18:00 ` [PATCH 1/3] media: videobuf2: Add a transfer error event Michael Rodin 2022-06-28 18:00 ` [PATCH 2/3] rcar-csi2: Do not try to recover after transfer error Michael Rodin @ 2022-06-28 18:00 ` Michael Rodin 2022-07-05 9:46 ` [PATCH v2 0/3] Improve error handling in the rcar-vin driver Niklas Söderlund 3 siblings, 0 replies; 18+ messages in thread From: Michael Rodin @ 2022-06-28 18:00 UTC (permalink / raw) To: Mauro Carvalho Chehab, Niklas Söderlund, linux-media, linux-kernel, linux-renesas-soc Cc: Michael Rodin, michael, erosca 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 0/3] Improve error handling in the rcar-vin driver 2022-06-28 18:00 ` [PATCH v2 0/3] Improve error handling in the rcar-vin driver Michael Rodin ` (2 preceding siblings ...) 2022-06-28 18:00 ` [PATCH 3/3] media: rcar-vin: Allow userspace to subscribe to V4L2_EVENT_XFER_ERROR Michael Rodin @ 2022-07-05 9:46 ` Niklas Söderlund 2022-07-15 13:42 ` Michael Rodin 3 siblings, 1 reply; 18+ messages in thread From: Niklas Söderlund @ 2022-07-05 9:46 UTC (permalink / raw) To: Michael Rodin Cc: Mauro Carvalho Chehab, linux-media, linux-kernel, linux-renesas-soc, michael, erosca 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 0/3] Improve error handling in the rcar-vin driver 2022-07-05 9:46 ` [PATCH v2 0/3] Improve error handling in the rcar-vin driver Niklas Söderlund @ 2022-07-15 13:42 ` Michael Rodin 0 siblings, 0 replies; 18+ messages in thread From: Michael Rodin @ 2022-07-15 13:42 UTC (permalink / raw) To: Niklas Söderlund, Hans Verkuil Cc: Michael Rodin, Mauro Carvalho Chehab, linux-media, linux-kernel, linux-renesas-soc, michael, erosca 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2022-08-09 7:12 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-19 18:00 Improve error handling in the rcar-vin driver Michael Rodin 2022-05-19 18:00 ` [PATCH 1/3] media: videobuf2: Add a transfer error event Michael Rodin 2022-05-19 18:00 ` [PATCH 2/3] rcar-csi2: Do not try to recover after transfer error Michael Rodin 2022-05-19 18:00 ` [PATCH 3/3] rcar-vin: handle transfer errors from subdevices and stop streaming if required Michael Rodin 2022-05-19 21:00 ` Niklas Söderlund 2022-05-20 19:50 ` Michael Rodin 2022-06-08 21:04 ` Niklas Söderlund 2022-06-28 18:00 ` [PATCH v2 0/3] Improve error handling in the rcar-vin driver Michael Rodin 2022-06-28 18:00 ` [PATCH 1/3] media: videobuf2: Add a transfer error event Michael Rodin 2022-07-04 15:59 ` Nicolas Dufresne 2022-07-15 16:15 ` Michael Rodin 2022-08-02 9:32 ` Hans Verkuil 2022-08-08 17:03 ` Michael Rodin 2022-08-09 7:12 ` Hans Verkuil 2022-06-28 18:00 ` [PATCH 2/3] rcar-csi2: Do not try to recover after transfer error Michael Rodin 2022-06-28 18:00 ` [PATCH 3/3] media: rcar-vin: Allow userspace to subscribe to V4L2_EVENT_XFER_ERROR Michael Rodin 2022-07-05 9:46 ` [PATCH v2 0/3] Improve error handling in the rcar-vin driver Niklas Söderlund 2022-07-15 13:42 ` Michael Rodin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.