2022-05-21 04:16:18

by Michael Rodin

[permalink] [raw]
Subject: Improve error handling in the rcar-vin driver

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/[email protected]/
[2] https://lore.kernel.org/linux-renesas-soc/[email protected]/



2022-05-23 02:25:52

by Michael Rodin

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

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/[email protected]/

Signed-off-by: Michael Rodin <[email protected]>
---
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


2022-05-23 06:28:03

by Michael Rodin

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

From: Niklas Söderlund <[email protected]>

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 <[email protected]>
Reviewed-by: Jacopo Mondi <[email protected]>
---
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