| Message ID | 20210115002148.4079591-5-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | rcar-csi2: Update handling of transfer error | expand |
Hello! On 15.01.2021 3:21, Niklas Söderlund wrote: > Instead of restarting the R-Car CSI-2 receiver if a transmission error > is detected inform the R-Car VIN driver of the error so it can stop the ^ , woiuldn't hurt here? > whole pipeline and inform user-space. This is done to reflect a updated ^ an > usage recommendation in later versions of the datasheet. > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> [...] MBR, Sergei
Hi Niklas, On Fri, Jan 15, 2021 at 01:21:48AM +0100, Niklas Söderlund wrote: > Instead of restarting the R-Car CSI-2 receiver if a transmission error > is detected inform the R-Car VIN driver of the error so it can stop the > whole pipeline and inform user-space. This is done to reflect a updated > usage recommendation in later versions of the datasheet. I wonder if there's any userspace component that relies on the auto-restart procedure. > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> If this aligns the behaviour with the manual (and it seems also saner than attemping a restart) I think it's good. Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org> Thanks j > --- > drivers/media/platform/rcar-vin/rcar-csi2.c | 14 ++++++-------- > 1 file changed, 6 insertions(+), 8 deletions(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c > index 945d2eb8723367f0..a7212ecc46572a3b 100644 > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c > @@ -773,21 +773,19 @@ static irqreturn_t rcsi2_irq(int irq, void *data) > > rcsi2_write(priv, INTERRSTATE_REG, err_status); > > - dev_info(priv->dev, "Transfer error, restarting CSI-2 receiver\n"); > - > return IRQ_WAKE_THREAD; > } > > static irqreturn_t rcsi2_irq_thread(int irq, void *data) > { > struct rcar_csi2 *priv = data; > + struct v4l2_event event = { > + .type = V4L2_EVENT_EOS, > + }; > > - mutex_lock(&priv->lock); > - rcsi2_stop(priv); > - usleep_range(1000, 2000); > - if (rcsi2_start(priv)) > - dev_warn(priv->dev, "Failed to restart CSI-2 receiver\n"); > - mutex_unlock(&priv->lock); > + dev_err(priv->dev, "Transfer error detected.\n"); > + > + v4l2_subdev_notify_event(&priv->subdev, &event); > > return IRQ_HANDLED; > } > -- > 2.30.0 >
On 15/01/2021 01:21, Niklas Söderlund wrote: > Instead of restarting the R-Car CSI-2 receiver if a transmission error > is detected inform the R-Car VIN driver of the error so it can stop the > whole pipeline and inform user-space. This is done to reflect a updated > usage recommendation in later versions of the datasheet. > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > drivers/media/platform/rcar-vin/rcar-csi2.c | 14 ++++++-------- > 1 file changed, 6 insertions(+), 8 deletions(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c > index 945d2eb8723367f0..a7212ecc46572a3b 100644 > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c > @@ -773,21 +773,19 @@ static irqreturn_t rcsi2_irq(int irq, void *data) > > rcsi2_write(priv, INTERRSTATE_REG, err_status); > > - dev_info(priv->dev, "Transfer error, restarting CSI-2 receiver\n"); > - > return IRQ_WAKE_THREAD; > } > > static irqreturn_t rcsi2_irq_thread(int irq, void *data) > { > struct rcar_csi2 *priv = data; > + struct v4l2_event event = { > + .type = V4L2_EVENT_EOS, > + }; > > - mutex_lock(&priv->lock); > - rcsi2_stop(priv); > - usleep_range(1000, 2000); > - if (rcsi2_start(priv)) > - dev_warn(priv->dev, "Failed to restart CSI-2 receiver\n"); > - mutex_unlock(&priv->lock); > + dev_err(priv->dev, "Transfer error detected.\n"); You probably want to call vb2_queue_error() here. Typically once something like this happens you have to restart everything and marking the queue as 'error' will ensure that VIDIOC_QBUF will return an error until the queue is reset (STREAMOFF). It doesn't hurt to also raise the EOS event, I'm fine with that. Regards, Hans > + > + v4l2_subdev_notify_event(&priv->subdev, &event); > > return IRQ_HANDLED; > } >
Hi Hans, Thanks for your feedback. On 2021-01-25 10:44:48 +0100, Hans Verkuil wrote: > On 15/01/2021 01:21, Niklas Söderlund wrote: > > Instead of restarting the R-Car CSI-2 receiver if a transmission error > > is detected inform the R-Car VIN driver of the error so it can stop the > > whole pipeline and inform user-space. This is done to reflect a updated > > usage recommendation in later versions of the datasheet. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > --- > > drivers/media/platform/rcar-vin/rcar-csi2.c | 14 ++++++-------- > > 1 file changed, 6 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c > > index 945d2eb8723367f0..a7212ecc46572a3b 100644 > > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c > > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c > > @@ -773,21 +773,19 @@ static irqreturn_t rcsi2_irq(int irq, void *data) > > > > rcsi2_write(priv, INTERRSTATE_REG, err_status); > > > > - dev_info(priv->dev, "Transfer error, restarting CSI-2 receiver\n"); > > - > > return IRQ_WAKE_THREAD; > > } > > > > static irqreturn_t rcsi2_irq_thread(int irq, void *data) > > { > > struct rcar_csi2 *priv = data; > > + struct v4l2_event event = { > > + .type = V4L2_EVENT_EOS, > > + }; > > > > - mutex_lock(&priv->lock); > > - rcsi2_stop(priv); > > - usleep_range(1000, 2000); > > - if (rcsi2_start(priv)) > > - dev_warn(priv->dev, "Failed to restart CSI-2 receiver\n"); > > - mutex_unlock(&priv->lock); > > + dev_err(priv->dev, "Transfer error detected.\n"); > > You probably want to call vb2_queue_error() here. Typically once > something like this happens you have to restart everything and marking > the queue as 'error' will ensure that VIDIOC_QBUF will return an error > until the queue is reset (STREAMOFF). The CSI-2 driver is a bridge driver and does not deal with buffers. Instead the idea here is to signal EOS so that the VIN driver (and user-space) can detect it and indeed as you point out deal with signaling vb2 error. I will respin this series as it needs to be rebased anyhow. > > It doesn't hurt to also raise the EOS event, I'm fine with that. > > Regards, > > Hans > > > + > > + v4l2_subdev_notify_event(&priv->subdev, &event); > > > > return IRQ_HANDLED; > > } > > >
diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c index 945d2eb8723367f0..a7212ecc46572a3b 100644 --- a/drivers/media/platform/rcar-vin/rcar-csi2.c +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c @@ -773,21 +773,19 @@ static irqreturn_t rcsi2_irq(int irq, void *data) rcsi2_write(priv, INTERRSTATE_REG, err_status); - dev_info(priv->dev, "Transfer error, restarting CSI-2 receiver\n"); - return IRQ_WAKE_THREAD; } static irqreturn_t rcsi2_irq_thread(int irq, void *data) { struct rcar_csi2 *priv = data; + struct v4l2_event event = { + .type = V4L2_EVENT_EOS, + }; - mutex_lock(&priv->lock); - rcsi2_stop(priv); - usleep_range(1000, 2000); - if (rcsi2_start(priv)) - dev_warn(priv->dev, "Failed to restart CSI-2 receiver\n"); - mutex_unlock(&priv->lock); + dev_err(priv->dev, "Transfer error detected.\n"); + + v4l2_subdev_notify_event(&priv->subdev, &event); return IRQ_HANDLED; }
Instead of restarting the R-Car CSI-2 receiver if a transmission error is detected inform the R-Car VIN driver of the error so it can stop the whole pipeline and inform user-space. This is done to reflect a updated usage recommendation in later versions of the datasheet. Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> --- drivers/media/platform/rcar-vin/rcar-csi2.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)