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

Commit Message

Michael Rodin June 28, 2022, 6 p.m. UTC

From: Niklas Söderlund <[e-mail]>

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 <[e-mail]>
[[e-mail]: adapted information what to do if this new event is received]
Signed-off-by: Michael Rodin <[e-mail]>
---
 .../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(+)

Comments

Nicolas Dufresne July 4, 2022, 3:59 p.m. UTC | #1

Hi Micheal,

thanks for your work, I have some questions below ...

Le mardi 28 juin 2022 à 20:00 +0200, Michael Rodin a écrit :
> From: Niklas Söderlund <[e-mail]>
> 
> 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 <[e-mail]>
> [[e-mail]: adapted information what to do if this new event is received]
> Signed-off-by: Michael Rodin <[e-mail]>
> ---
>  .../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 ?

> +      - 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.

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 */

Michael Rodin July 15, 2022, 4:15 p.m. UTC | #2

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=>
> > [[e-mail]: adapted information what to do if this new event is received]
> > Signed-off-by: Michael Rodin <[e-mail]>
> > ---
> >  .../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 */
>

Hans Verkuil Aug. 2, 2022, 9:32 a.m. UTC | #3

Hi Michael,

Apologies for the late reply...

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=>
>>> [[e-mail]: adapted information what to do if this new event is received]
>>> Signed-off-by: Michael Rodin <[e-mail]>
>>> ---
>>>  .../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.

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 */
>>
>

Michael Rodin Aug. 8, 2022, 5:03 p.m. UTC | #4

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=>
> >>> [[e-mail]: adapted information what to do if this new event is received]
> >>> Signed-off-by: Michael Rodin <[e-mail]>
> >>> ---
> >>>  .../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/[e-mail]/
[2] https://lore.kernel.org/lkml/[e-mail]/

> 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 */
> >>
> > 
>

Hans Verkuil Aug. 9, 2022, 7:12 a.m. UTC | #5

Hi Michael,

On 08/08/2022 19:03, Michael Rodin wrote:
> 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=>
>>>>> [[e-mail]: adapted information what to do if this new event is received]
>>>>> Signed-off-by: Michael Rodin <[e-mail]>
>>>>> ---
>>>>>  .../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/[e-mail]/
> [2] https://lore.kernel.org/lkml/[e-mail]/

Yes, [1] goes in the right direction. The V4L2_EVENT_XFER_ERROR addition can
be dropped, since a call to vb2_queue_error() is sufficient for non-recoverable
transfer errors.

BTW, I think a timer might work better than a workqueue here: no need to cancel
and reschedule the workqueue in interrupt context, instead the isr can just call
mod_timer. Calling vb2_queue_error can be done from interrupt context as well,
so that can safely be called from the timer function.

I would not be opposed to keep this as driver-specific functionality: it's really
not much code. But if you do want to add this to vb2, then that can be done, of
course. I think vb2_queue needs a new field: watchdog_timeout. If 0, there is no
watchdog running, if non-0 vb2 would start a timer after start_streaming (and
stop it in stop_streaming), and a new vb2_watchdog_reset() is added that calls
mod_timer and that the driver has to call from the interrupt.

Finally, the timer function would call vb2_queue_error().

Eventually, a vb2_queue op can be added as well (e.g. watchdog_timeout) to do
driver specific stuff. It doesn't seem to be needed for rcar-vin, though.

Regards,

	Hans

> 
>> 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 */
>>>>
>>>
>>
>

Patch

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``
+      - 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.
     * - ``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 */