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@xxxxxxxxxxxxxx: adapted information what to do if this new event is received] > >>> Signed-off-by: Michael Rodin <mrodin@xxxxxxxxxxxxxx> > >>> --- > >>> .../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@xxxxxxxxxxxxxx/ [2] https://lore.kernel.org/lkml/YqEO3%2FKekkZhVjW+@xxxxxxxxxxxxxxxxx/ > 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
