Hi Michael,
On 2020-07-02 14:33:41 +0200, Michael Rodin wrote:
> Hi Niklas,
>
> On Wed, Jul 01, 2020 at 12:17:10AM +0200, Niklas Söderlund wrote:
> > Hi Michael,
> >
> > Thanks for your RFC.
>
> Thanks your your feedback!
>
> > On 2020-06-19 19:46:10 +0200, Michael Rodin wrote:
> > > Data flow from an upstream subdevice can stop permanently due to:
> > > - CSI2 transmission errors
> > > - silent failure of the source subdevice
> > > - disconnection of the source subdevice
> > > In those cases userspace waits for new buffers for an infinitely long time.
> > > In order to address this issue, use a timer to monitor, that rvin_irq() is
> > > capturing at least one frame within a IRQ_TIMEOUT_MS period. Otherwise send
> > > a new private v4l2 event to userspace. This event is exported to userspace
> > > via a new uapi header.
> >
> > I think there is value for user-space to detecting the error cases
> > above. But I think the problem could be addressed at a different lever.
> > Defining a VIN specific events and controls for something that applies
> > any video device might not be the neatest solution.
> >
> > Another thing hits me when reading this series, could this not be done
> > in user-space? In 2/2 you add a control which sets the timeout based on
> > the framerate, so user-space must know about that to be able to set the
> > control. User-space also knows when it receives/dequeus a buffer from
> > the video device so the timeout logic could be implemented in the
> > application. Given that the application anyhow needs special care to
> > handle the VIN specific event and control I wonder if it's not neater to
> > make it handle all of it. Do you see any specific benefit of having
> > parts of it in the driver?
>
> Originally I have started this patch series to implement a replacement for
> the CSI-2 error handling you have added in
> commit 4ab44ff ("media: rcar-csi2: restart CSI-2 link if error is detected"),
> which is not correct for multiple reasons:
> 1. The commit message states that the user is informed that something is
> not right. But you have just added new messages which only appear in
> dmesg. This might be sufficient for a desktop PC but not for an embedded
> system, where the user normally can not see dmesg log. So I think that
> V4L2 events are the correct solution for this kind of notification,
> because they are passed directly to the application and the developer
> can implement handling for this issue or display an error in the
> custom human-machine interface.
> 2. It is not correct to restart the CSI-2 link if you don't restart VIN
> module as well. Renesas HW manual R19UH0105EJ0200 Rev.2.00
> (Jul 31, 2019) requires a reset or stop of capture in the VIN module
> before a reset of CSI-2 module (chapter 25.3.13 "Software Reset"). This
> also applies to CSI-2 error handling.
> 3. The CSI-2 driver restarts CSI-2 module for any CSI-2 error. However not
> all CSI2 errors are critical. In some setups they are really harmless so
> streaming can continue without any unnecessary restart. In some setups
> they _always_ occur at each start of streaming and are harmless as
> well, so automatic restart in CSI-2 module ends in an endless restart
> loop, which never comes to an end and breaks streaming instead of any
> help. It is better to leave such errors unhandled and therefore it is
> important to detect whether DMA transfers really stop in rvin_irq().
> 4. Video streaming applications in the automotive/embedded industry often
> want to control when the video streaming pipeline is stopped or started
> to be able to do some tasks in between, so an automatic restart of the
> pipeline is not acceptable for them. It should be at least optional and
> we should do this in rcar-dma.c, e.g. in the proposed irq timeout
> function. However I am not sure yet how to implement a restart of
> streaming inside of the rcar-vin driver correctly.
>
> I think, my patch series provides technically a good solution for the
> described issues. Also it is generic enough to allow also handling of
> failures in upstream subdevices connected to an R-Car3 CSI2 receiver or
> even parallel video input devices in cases when such failures can not be
> fixed or detected in the subdevice drivers and result in a stop of data
> flow on the chip level.
>
> Theoretically, applications also could use timeout parameter of the poll()
> syscall to implement the timeout (which can be e.g. a multiple of the frame
> interval), but the problem is that userspace does not know whether the
> timeout happened because there are no DMA transfers in the driver (i.e. one
> of the upstream subdevices or VIN failed) or because the driver is just
> using the "scratch buffer". The event which I have introduced explicitly
> monitors whether rcar-vin regularly receives new frames from upstream
> and allows applications to try a recovery (I have now renamed the event to
> "FRAME_TIMEOUT" to be more precise about its purpose).
>
> Another reason, why I think that the new v4l2 event is the right solution,
> are proprietary applications, where it is not possible to change the code
> to add any additional handling of driver failures but it is possible to
> start/stop streaming via inter process communication. Since V4L2 events can
> be subscribed and received by a separate process, it is possible to
> implement a middleware in user space, which monitors V4L2 events. Typically
> this middleware could also take over all of the complicated media-ctl
> configuration and monitoring of source changes and other events from
> subdevices, but this is a bit off-topic. I think that (private) V4L2 events
> are really useful in embedded systems where applications/middlewares are
> aware of the underlying hardware and want to be better informed about
> hardware related events than desktop applications.
>
> So if my arguments sound reasonable and you don't reject the overall
> concept of the series, I would send an improved version, where I have fixed
> some details of the timer implementation. I have also a patch for rcar-csi2
> driver with a private CSI-2 error event, which is useful to let the
> application know that the reason for the frame timeout event might be a
> CSI-2 error and not e.g. paused playback.
I'm not arguing that something is needed for user-space to detect these
situations. I'm arguing that adding these events directly into rcar-vin
and/or rcar-csi2 is the wrong level. In my view either support should be
added in the V4L2 subsystem core or in the user-space applications.
Adding custom events and logic to a single driver does not create
reusable and generic solution which can be used by many.
If you wish to work the problem in the kernel I think you should do so
on a V4L2 level and not in the individual driver. As the needs you
describe above could be useful for others users.
>
> > >
> > > Signed-off-by: Michael Rodin <mrodin@xxxxxxxxxxxxxx>
> > > ---
> > > drivers/media/platform/rcar-vin/rcar-dma.c | 21 +++++++++++++++++++++
> > > drivers/media/platform/rcar-vin/rcar-v4l2.c | 1 +
> > > drivers/media/platform/rcar-vin/rcar-vin.h | 6 ++++++
> > > include/uapi/linux/rcar-vin.h | 10 ++++++++++
> > > 4 files changed, 38 insertions(+)
> > > create mode 100644 include/uapi/linux/rcar-vin.h
> > >
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > index 1a30cd0..bf8d733 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > @@ -937,6 +937,20 @@ static void rvin_capture_stop(struct rvin_dev *vin)
> > > #define RVIN_TIMEOUT_MS 100
> > > #define RVIN_RETRIES 10
> > >
> > > +static const struct v4l2_event rvin_irq_timeout = {
> > > + .type = V4L2_EVENT_RCAR_VIN_IRQ_TIMEOUT,
> > > +};
> > > +
> > > +static void rvin_irq_timer_function(struct timer_list *timer)
> > > +{
> > > + struct rvin_dev *vin = container_of(timer, struct rvin_dev,
> > > + irq_timer);
> > > +
> > > + vin_err(vin, "%s: frame completion timeout after %i ms!\n",
> > > + __func__, IRQ_TIMEOUT_MS);
> > > + v4l2_event_queue(&vin->vdev, &rvin_irq_timeout);
> > > +}
> > > +
> > > static irqreturn_t rvin_irq(int irq, void *data)
> > > {
> > > struct rvin_dev *vin = data;
> > > @@ -1008,6 +1022,8 @@ static irqreturn_t rvin_irq(int irq, void *data)
> > > vin_dbg(vin, "Dropping frame %u\n", vin->sequence);
> > > }
> > >
> > > + mod_timer(&vin->irq_timer, jiffies + msecs_to_jiffies(IRQ_TIMEOUT_MS));
> > > +
> > > vin->sequence++;
> > >
> > > /* Prepare for next frame */
> > > @@ -1252,6 +1268,8 @@ static int rvin_start_streaming(struct vb2_queue *vq, unsigned int count)
> > > if (ret)
> > > dma_free_coherent(vin->dev, vin->format.sizeimage, vin->scratch,
> > > vin->scratch_phys);
> > > + else
> > > + mod_timer(&vin->irq_timer, jiffies + msecs_to_jiffies(IRQ_TIMEOUT_MS));
> > >
> > > return ret;
> > > }
> > > @@ -1305,6 +1323,8 @@ static void rvin_stop_streaming(struct vb2_queue *vq)
> > > /* Free scratch buffer. */
> > > dma_free_coherent(vin->dev, vin->format.sizeimage, vin->scratch,
> > > vin->scratch_phys);
> > > +
> > > + del_timer_sync(&vin->irq_timer);
> > > }
> > >
> > > static const struct vb2_ops rvin_qops = {
> > > @@ -1370,6 +1390,7 @@ int rvin_dma_register(struct rvin_dev *vin, int irq)
> > > goto error;
> > > }
> > >
> > > + timer_setup(&vin->irq_timer, rvin_irq_timer_function, 0);
> > > return 0;
> > > error:
> > > rvin_dma_unregister(vin);
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > index f421e25..c644134 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > @@ -581,6 +581,7 @@ static int rvin_subscribe_event(struct v4l2_fh *fh,
> > > {
> > > switch (sub->type) {
> > > case V4L2_EVENT_SOURCE_CHANGE:
> > > + case V4L2_EVENT_RCAR_VIN_IRQ_TIMEOUT:
> > > return v4l2_event_subscribe(fh, sub, 4, NULL);
> > > }
> > > return v4l2_ctrl_subscribe_event(fh, sub);
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> > > index c19d077..7408f67 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> > > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> > > @@ -14,12 +14,14 @@
> > > #define __RCAR_VIN__
> > >
> > > #include <linux/kref.h>
> > > +#include <linux/rcar-vin.h>
> > >
> > > #include <media/v4l2-async.h>
> > > #include <media/v4l2-ctrls.h>
> > > #include <media/v4l2-dev.h>
> > > #include <media/v4l2-device.h>
> > > #include <media/videobuf2-v4l2.h>
> > > +#include <media/v4l2-event.h>
> > >
> > > /* Number of HW buffers */
> > > #define HW_BUFFER_NUM 3
> > > @@ -30,6 +32,8 @@
> > > /* Max number on VIN instances that can be in a system */
> > > #define RCAR_VIN_NUM 8
> > >
> > > +#define IRQ_TIMEOUT_MS 1000
> > > +
> > > struct rvin_group;
> > >
> > > enum model_id {
> > > @@ -196,6 +200,7 @@ struct rvin_info {
> > > * @compose: active composing
> > > * @src_rect: active size of the video source
> > > * @std: active video standard of the video source
> > > + * @irq_timer: monitors regular capturing of frames in rvin_irq()
> > > *
> > > * @alpha: Alpha component to fill in for supported pixel formats
> > > */
> > > @@ -240,6 +245,7 @@ struct rvin_dev {
> > > struct v4l2_rect src_rect;
> > > v4l2_std_id std;
> > >
> > > + struct timer_list irq_timer;
> > > unsigned int alpha;
> > > };
> > >
> > > diff --git a/include/uapi/linux/rcar-vin.h b/include/uapi/linux/rcar-vin.h
> > > new file mode 100644
> > > index 00000000..4eb7f5e
> > > --- /dev/null
> > > +++ b/include/uapi/linux/rcar-vin.h
> > > @@ -0,0 +1,10 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > > +
> > > +#ifndef RCAR_VIN_USER_H
> > > +#define RCAR_VIN_USER_H
> > > +
> > > +/* class for events sent by the rcar-vin driver */
> > > +#define V4L2_EVENT_RCAR_VIN_CLASS V4L2_EVENT_PRIVATE_START
> > > +#define V4L2_EVENT_RCAR_VIN_IRQ_TIMEOUT (V4L2_EVENT_RCAR_VIN_CLASS | 0x1)
> > > +
> > > +#endif /* RCAR_VIN_USER_H */
> > > --
> > > 2.7.4
> > >
> >
> > --
> > Regards,
> > Niklas Söderlund
>
> --
> Best Regards,
> Michael
--
Regards,
Niklas Söderlund
