Re: [PATCH 1/3] media: videobuf2: Add a transfer error event

[Michael Rodin <mrodin@de.adit-jv.com>]

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