[PATCH] media: docs: dev-decoder: add restrictions about CAPTURE buffers

[PATCH] media: docs: dev-decoder: add restrictions about CAPTURE buffers

[Alexandre Courbot]

CAPTURE buffers might be read by the hardware after they are dequeued,
which goes against the general idea that userspace has full control over
dequeued buffers. Explain why and document the restrictions that this
implies for userspace.

Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
---
 .../userspace-api/media/v4l/dev-decoder.rst     | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Documentation/userspace-api/media/v4l/dev-decoder.rst b/Documentation/userspace-api/media/v4l/dev-decoder.rst
index 5b9b83feeceb..3cf2b496f2d0 100644
--- a/Documentation/userspace-api/media/v4l/dev-decoder.rst
+++ b/Documentation/userspace-api/media/v4l/dev-decoder.rst
@@ -752,6 +752,23 @@ available to dequeue. Specifically:
      buffers are out-of-order compared to the ``OUTPUT`` buffers): ``CAPTURE``
      timestamps will not retain the order of ``OUTPUT`` timestamps.
 
+.. note::
+
+   The backing memory of ``CAPTURE`` buffers that are used as reference frames
+   by the stream may be read by the hardware even after they are dequeued.
+   Consequently, the client should avoid writing into this memory while the
+   ``CAPTURE`` queue is streaming. Failure to observe this may result in
+   corruption of decoded frames.
+
+   Similarly, when using a memory type other than ``V4L2_MEMORY_MMAP``, the
+   client should make sure that each ``CAPTURE`` buffer is always queued with
+   the same backing memory for as long as the ``CAPTURE`` queue is streaming.
+   The reason for this is that V4L2 buffer indices can be used by drivers to
+   identify frames. Thus, if the backing memory of a reference frame is
+   submitted under a different buffer ID, the driver may misidentify it and
+   decode a new frame into it while it is still in use, resulting in corruption
+   of the following frames.
+
 During the decoding, the decoder may initiate one of the special sequences, as
 listed below. The sequences will result in the decoder returning all the
 ``CAPTURE`` buffers that originated from all the ``OUTPUT`` buffers processed
-- 
2.33.0.1079.g6e70778dc9-goog

Re: [PATCH] media: docs: dev-decoder: add restrictions about CAPTURE buffers

[Tomasz Figa]

On Mon, Oct 18, 2021 at 6:14 PM Alexandre Courbot <acourbot@chromium.org> wrote:
>
> CAPTURE buffers might be read by the hardware after they are dequeued,
> which goes against the general idea that userspace has full control over
> dequeued buffers. Explain why and document the restrictions that this
> implies for userspace.
>
> Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
> ---
>  .../userspace-api/media/v4l/dev-decoder.rst     | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/Documentation/userspace-api/media/v4l/dev-decoder.rst b/Documentation/userspace-api/media/v4l/dev-decoder.rst
> index 5b9b83feeceb..3cf2b496f2d0 100644
> --- a/Documentation/userspace-api/media/v4l/dev-decoder.rst
> +++ b/Documentation/userspace-api/media/v4l/dev-decoder.rst
> @@ -752,6 +752,23 @@ available to dequeue. Specifically:
>       buffers are out-of-order compared to the ``OUTPUT`` buffers): ``CAPTURE``
>       timestamps will not retain the order of ``OUTPUT`` timestamps.
>
> +.. note::
> +
> +   The backing memory of ``CAPTURE`` buffers that are used as reference frames
> +   by the stream may be read by the hardware even after they are dequeued.
> +   Consequently, the client should avoid writing into this memory while the
> +   ``CAPTURE`` queue is streaming. Failure to observe this may result in
> +   corruption of decoded frames.
> +
> +   Similarly, when using a memory type other than ``V4L2_MEMORY_MMAP``, the
> +   client should make sure that each ``CAPTURE`` buffer is always queued with
> +   the same backing memory for as long as the ``CAPTURE`` queue is streaming.
> +   The reason for this is that V4L2 buffer indices can be used by drivers to
> +   identify frames. Thus, if the backing memory of a reference frame is
> +   submitted under a different buffer ID, the driver may misidentify it and
> +   decode a new frame into it while it is still in use, resulting in corruption
> +   of the following frames.
> +
>  During the decoding, the decoder may initiate one of the special sequences, as
>  listed below. The sequences will result in the decoder returning all the
>  ``CAPTURE`` buffers that originated from all the ``OUTPUT`` buffers processed
> --
> 2.33.0.1079.g6e70778dc9-goog
>

Acked-by: Tomasz Figa <tfiga@chromium.org>

Best regards,
Tomasz

Re: [PATCH] media: docs: dev-decoder: add restrictions about CAPTURE buffers

[Nicolas Dufresne]

Le lundi 18 octobre 2021 à 18:14 +0900, Alexandre Courbot a écrit :
> CAPTURE buffers might be read by the hardware after they are dequeued,
> which goes against the general idea that userspace has full control over
> dequeued buffers. Explain why and document the restrictions that this
> implies for userspace.
> 
> Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
> ---
>  .../userspace-api/media/v4l/dev-decoder.rst     | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/dev-decoder.rst b/Documentation/userspace-api/media/v4l/dev-decoder.rst
> index 5b9b83feeceb..3cf2b496f2d0 100644
> --- a/Documentation/userspace-api/media/v4l/dev-decoder.rst
> +++ b/Documentation/userspace-api/media/v4l/dev-decoder.rst
> @@ -752,6 +752,23 @@ available to dequeue. Specifically:
>       buffers are out-of-order compared to the ``OUTPUT`` buffers): ``CAPTURE``
>       timestamps will not retain the order of ``OUTPUT`` timestamps.
>  
> +.. note::
> +
> +   The backing memory of ``CAPTURE`` buffers that are used as reference frames
> +   by the stream may be read by the hardware even after they are dequeued.
> +   Consequently, the client should avoid writing into this memory while the
> +   ``CAPTURE`` queue is streaming. Failure to observe this may result in
> +   corruption of decoded frames.
> +
> +   Similarly, when using a memory type other than ``V4L2_MEMORY_MMAP``, the
> +   client should make sure that each ``CAPTURE`` buffer is always queued with
> +   the same backing memory for as long as the ``CAPTURE`` queue is streaming.
> +   The reason for this is that V4L2 buffer indices can be used by drivers to
> +   identify frames. Thus, if the backing memory of a reference frame is
> +   submitted under a different buffer ID, the driver may misidentify it and
> +   decode a new frame into it while it is still in use, resulting in corruption
> +   of the following frames.
> +

I think this is nice addition, but insufficient. We should extend the API with a
flags that let application know if the buffers are reference or secondary. For
the context, we have a mix of CODEC that will output usable reference frames and
needs careful manipulation and many other drivers where the buffers *maybe*
secondary, meaning they may have been post-processed and modifying these in-
place may have no impact.

The problem is the "may", that will depends on the chosen CAPTURE format. I
believe we should flag this, this flag should be set by the driver, on CAPTURE
queue. The information is known after S_FMT, so Format Flag, Reqbufs
capabilities or querybuf flags are candidates. I think the buffer flags are the
best named flag, though we don't expect this to differ per buffer. Though,
userspace needs to call querybuf for all buf in order to export or map them.

What userspace can do with this is to export the DMABuf as read-only, and signal
this internally in its own context. This is great to avoid any unwanted side
effect described here.

>  During the decoding, the decoder may initiate one of the special sequences, as
>  listed below. The sequences will result in the decoder returning all the
>  ``CAPTURE`` buffers that originated from all the ``OUTPUT`` buffers processed

RE: [EXT] Re: [PATCH] media: docs: dev-decoder: add restrictions about CAPTURE buffers

[Ming Qian]

> -----Original Message-----
> From: Nicolas Dufresne [mailto:nicolas@ndufresne.ca]
> Sent: Tuesday, October 26, 2021 10:12 PM
> To: Alexandre Courbot <acourbot@chromium.org>; Mauro Carvalho Chehab
> <mchehab@kernel.org>; Hans Verkuil <hverkuil-cisco@xs4all.nl>; Tomasz Figa
> <tfiga@chromium.org>
> Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [EXT] Re: [PATCH] media: docs: dev-decoder: add restrictions about
> CAPTURE buffers
> 
> Caution: EXT Email
> 
> Le lundi 18 octobre 2021 à 18:14 +0900, Alexandre Courbot a écrit :
> > CAPTURE buffers might be read by the hardware after they are dequeued,
> > which goes against the general idea that userspace has full control
> > over dequeued buffers. Explain why and document the restrictions that
> > this implies for userspace.
> >
> > Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
> > ---
> >  .../userspace-api/media/v4l/dev-decoder.rst     | 17
> +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/Documentation/userspace-api/media/v4l/dev-decoder.rst
> > b/Documentation/userspace-api/media/v4l/dev-decoder.rst
> > index 5b9b83feeceb..3cf2b496f2d0 100644
> > --- a/Documentation/userspace-api/media/v4l/dev-decoder.rst
> > +++ b/Documentation/userspace-api/media/v4l/dev-decoder.rst
> > @@ -752,6 +752,23 @@ available to dequeue. Specifically:
> >       buffers are out-of-order compared to the ``OUTPUT`` buffers):
> ``CAPTURE``
> >       timestamps will not retain the order of ``OUTPUT`` timestamps.
> >
> > +.. note::
> > +
> > +   The backing memory of ``CAPTURE`` buffers that are used as reference
> frames
> > +   by the stream may be read by the hardware even after they are
> dequeued.
> > +   Consequently, the client should avoid writing into this memory while the
> > +   ``CAPTURE`` queue is streaming. Failure to observe this may result in
> > +   corruption of decoded frames.
> > +
> > +   Similarly, when using a memory type other than
> ``V4L2_MEMORY_MMAP``, the
> > +   client should make sure that each ``CAPTURE`` buffer is always queued
> with
> > +   the same backing memory for as long as the ``CAPTURE`` queue is
> streaming.
> > +   The reason for this is that V4L2 buffer indices can be used by drivers to
> > +   identify frames. Thus, if the backing memory of a reference frame is
> > +   submitted under a different buffer ID, the driver may misidentify it and
> > +   decode a new frame into it while it is still in use, resulting in corruption
> > +   of the following frames.
> > +
> 
> I think this is nice addition, but insufficient. We should extend the API with a
> flags that let application know if the buffers are reference or secondary. For the
> context, we have a mix of CODEC that will output usable reference frames and
> needs careful manipulation and many other drivers where the buffers *maybe*
> secondary, meaning they may have been post-processed and modifying these
> in- place may have no impact.
> 
> The problem is the "may", that will depends on the chosen CAPTURE format. I
> believe we should flag this, this flag should be set by the driver, on CAPTURE
> queue. The information is known after S_FMT, so Format Flag, Reqbufs
> capabilities or querybuf flags are candidates. I think the buffer flags are the
> best named flag, though we don't expect this to differ per buffer. Though,
> userspace needs to call querybuf for all buf in order to export or map them.
> 
> What userspace can do with this is to export the DMABuf as read-only, and
> signal this internally in its own context. This is great to avoid any unwanted
> side effect described here.

I think a flag should be add to tell a buffer is reference or secondary.
But for some codec, it's hard to determine the buffer flag when reqbufs.
The buffer flag should be dynamically updated by driver.
User can check the flag after dqbuf every time.

> 
> >  During the decoding, the decoder may initiate one of the special
> > sequences, as  listed below. The sequences will result in the decoder
> > returning all the  ``CAPTURE`` buffers that originated from all the
> > ``OUTPUT`` buffers processed
> 

Re: [PATCH] media: docs: dev-decoder: add restrictions about CAPTURE buffers

[Tomasz Figa]

On Tue, Oct 26, 2021 at 11:12 PM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
>
> Le lundi 18 octobre 2021 à 18:14 +0900, Alexandre Courbot a écrit :
> > CAPTURE buffers might be read by the hardware after they are dequeued,
> > which goes against the general idea that userspace has full control over
> > dequeued buffers. Explain why and document the restrictions that this
> > implies for userspace.
> >
> > Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
> > ---
> >  .../userspace-api/media/v4l/dev-decoder.rst     | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/Documentation/userspace-api/media/v4l/dev-decoder.rst b/Documentation/userspace-api/media/v4l/dev-decoder.rst
> > index 5b9b83feeceb..3cf2b496f2d0 100644
> > --- a/Documentation/userspace-api/media/v4l/dev-decoder.rst
> > +++ b/Documentation/userspace-api/media/v4l/dev-decoder.rst
> > @@ -752,6 +752,23 @@ available to dequeue. Specifically:
> >       buffers are out-of-order compared to the ``OUTPUT`` buffers): ``CAPTURE``
> >       timestamps will not retain the order of ``OUTPUT`` timestamps.
> >
> > +.. note::
> > +
> > +   The backing memory of ``CAPTURE`` buffers that are used as reference frames
> > +   by the stream may be read by the hardware even after they are dequeued.
> > +   Consequently, the client should avoid writing into this memory while the
> > +   ``CAPTURE`` queue is streaming. Failure to observe this may result in
> > +   corruption of decoded frames.
> > +
> > +   Similarly, when using a memory type other than ``V4L2_MEMORY_MMAP``, the
> > +   client should make sure that each ``CAPTURE`` buffer is always queued with
> > +   the same backing memory for as long as the ``CAPTURE`` queue is streaming.
> > +   The reason for this is that V4L2 buffer indices can be used by drivers to
> > +   identify frames. Thus, if the backing memory of a reference frame is
> > +   submitted under a different buffer ID, the driver may misidentify it and
> > +   decode a new frame into it while it is still in use, resulting in corruption
> > +   of the following frames.
> > +
>
> I think this is nice addition, but insufficient. We should extend the API with a
> flags that let application know if the buffers are reference or secondary. For
> the context, we have a mix of CODEC that will output usable reference frames and
> needs careful manipulation and many other drivers where the buffers *maybe*
> secondary, meaning they may have been post-processed and modifying these in-
> place may have no impact.
>
> The problem is the "may", that will depends on the chosen CAPTURE format. I
> believe we should flag this, this flag should be set by the driver, on CAPTURE
> queue. The information is known after S_FMT, so Format Flag, Reqbufs
> capabilities or querybuf flags are candidates. I think the buffer flags are the
> best named flag, though we don't expect this to differ per buffer. Though,
> userspace needs to call querybuf for all buf in order to export or map them.
>
> What userspace can do with this is to export the DMABuf as read-only, and signal
> this internally in its own context. This is great to avoid any unwanted side
> effect described here.

I agree with the idea of having a way for the kernel to tell the
userspace the exact state of the buffer, but right now the untold
expectation of the kernel was as per what this patch adds. If one
wants their userspace to be portable across different decoders, they
need to keep the assumption. So the natural way to go here is to stay
safe by default and have a flag that tells the userspace that the
buffer can be freely reused.

Best regards,
Tomasz

Re: [EXT] Re: [PATCH] media: docs: dev-decoder: add restrictions about CAPTURE buffers

[Stanimir Varbanov]

On 10/29/21 5:10 AM, Ming Qian wrote:
>> -----Original Message-----
>> From: Nicolas Dufresne [mailto:nicolas@ndufresne.ca]
>> Sent: Tuesday, October 26, 2021 10:12 PM
>> To: Alexandre Courbot <acourbot@chromium.org>; Mauro Carvalho Chehab
>> <mchehab@kernel.org>; Hans Verkuil <hverkuil-cisco@xs4all.nl>; Tomasz Figa
>> <tfiga@chromium.org>
>> Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org
>> Subject: [EXT] Re: [PATCH] media: docs: dev-decoder: add restrictions about
>> CAPTURE buffers
>>
>> Caution: EXT Email
>>
>> Le lundi 18 octobre 2021 à 18:14 +0900, Alexandre Courbot a écrit :
>>> CAPTURE buffers might be read by the hardware after they are dequeued,
>>> which goes against the general idea that userspace has full control
>>> over dequeued buffers. Explain why and document the restrictions that
>>> this implies for userspace.
>>>
>>> Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
>>> ---
>>>  .../userspace-api/media/v4l/dev-decoder.rst     | 17
>> +++++++++++++++++
>>>  1 file changed, 17 insertions(+)
>>>
>>> diff --git a/Documentation/userspace-api/media/v4l/dev-decoder.rst
>>> b/Documentation/userspace-api/media/v4l/dev-decoder.rst
>>> index 5b9b83feeceb..3cf2b496f2d0 100644
>>> --- a/Documentation/userspace-api/media/v4l/dev-decoder.rst
>>> +++ b/Documentation/userspace-api/media/v4l/dev-decoder.rst
>>> @@ -752,6 +752,23 @@ available to dequeue. Specifically:
>>>       buffers are out-of-order compared to the ``OUTPUT`` buffers):
>> ``CAPTURE``
>>>       timestamps will not retain the order of ``OUTPUT`` timestamps.
>>>
>>> +.. note::
>>> +
>>> +   The backing memory of ``CAPTURE`` buffers that are used as reference
>> frames
>>> +   by the stream may be read by the hardware even after they are
>> dequeued.
>>> +   Consequently, the client should avoid writing into this memory while the
>>> +   ``CAPTURE`` queue is streaming. Failure to observe this may result in
>>> +   corruption of decoded frames.
>>> +
>>> +   Similarly, when using a memory type other than
>> ``V4L2_MEMORY_MMAP``, the
>>> +   client should make sure that each ``CAPTURE`` buffer is always queued
>> with
>>> +   the same backing memory for as long as the ``CAPTURE`` queue is
>> streaming.
>>> +   The reason for this is that V4L2 buffer indices can be used by drivers to
>>> +   identify frames. Thus, if the backing memory of a reference frame is
>>> +   submitted under a different buffer ID, the driver may misidentify it and
>>> +   decode a new frame into it while it is still in use, resulting in corruption
>>> +   of the following frames.
>>> +
>>
>> I think this is nice addition, but insufficient. We should extend the API with a
>> flags that let application know if the buffers are reference or secondary. For the
>> context, we have a mix of CODEC that will output usable reference frames and
>> needs careful manipulation and many other drivers where the buffers *maybe*
>> secondary, meaning they may have been post-processed and modifying these
>> in- place may have no impact.
>>
>> The problem is the "may", that will depends on the chosen CAPTURE format. I
>> believe we should flag this, this flag should be set by the driver, on CAPTURE
>> queue. The information is known after S_FMT, so Format Flag, Reqbufs
>> capabilities or querybuf flags are candidates. I think the buffer flags are the
>> best named flag, though we don't expect this to differ per buffer. Though,
>> userspace needs to call querybuf for all buf in order to export or map them.
>>
>> What userspace can do with this is to export the DMABuf as read-only, and
>> signal this internally in its own context. This is great to avoid any unwanted
>> side effect described here.
> 
> I think a flag should be add to tell a buffer is reference or secondary.
> But for some codec, it's hard to determine the buffer flag when reqbufs.
> The buffer flag should be dynamically updated by driver.
> User can check the flag after dqbuf every time.

+1

I'm not familiar with stateless decoders where on the reqbuf time it
could work, debut for stateful coders it should be a dynamic flag on
every capture buffer.

> 
>>
>>>  During the decoding, the decoder may initiate one of the special
>>> sequences, as  listed below. The sequences will result in the decoder
>>> returning all the  ``CAPTURE`` buffers that originated from all the
>>> ``OUTPUT`` buffers processed
>>
> 

-- 
regards,
Stan

Re: [PATCH] media: docs: dev-decoder: add restrictions about CAPTURE buffers

[Nicolas Dufresne]

Le vendredi 29 octobre 2021 à 12:04 +0900, Tomasz Figa a écrit :
> On Tue, Oct 26, 2021 at 11:12 PM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
> > 
> > Le lundi 18 octobre 2021 à 18:14 +0900, Alexandre Courbot a écrit :
> > > CAPTURE buffers might be read by the hardware after they are dequeued,
> > > which goes against the general idea that userspace has full control over
> > > dequeued buffers. Explain why and document the restrictions that this
> > > implies for userspace.
> > > 
> > > Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
> > > ---
> > >  .../userspace-api/media/v4l/dev-decoder.rst     | 17 +++++++++++++++++
> > >  1 file changed, 17 insertions(+)
> > > 
> > > diff --git a/Documentation/userspace-api/media/v4l/dev-decoder.rst b/Documentation/userspace-api/media/v4l/dev-decoder.rst
> > > index 5b9b83feeceb..3cf2b496f2d0 100644
> > > --- a/Documentation/userspace-api/media/v4l/dev-decoder.rst
> > > +++ b/Documentation/userspace-api/media/v4l/dev-decoder.rst
> > > @@ -752,6 +752,23 @@ available to dequeue. Specifically:
> > >       buffers are out-of-order compared to the ``OUTPUT`` buffers): ``CAPTURE``
> > >       timestamps will not retain the order of ``OUTPUT`` timestamps.
> > > 
> > > +.. note::
> > > +
> > > +   The backing memory of ``CAPTURE`` buffers that are used as reference frames
> > > +   by the stream may be read by the hardware even after they are dequeued.
> > > +   Consequently, the client should avoid writing into this memory while the
> > > +   ``CAPTURE`` queue is streaming. Failure to observe this may result in
> > > +   corruption of decoded frames.
> > > +
> > > +   Similarly, when using a memory type other than ``V4L2_MEMORY_MMAP``, the
> > > +   client should make sure that each ``CAPTURE`` buffer is always queued with
> > > +   the same backing memory for as long as the ``CAPTURE`` queue is streaming.
> > > +   The reason for this is that V4L2 buffer indices can be used by drivers to
> > > +   identify frames. Thus, if the backing memory of a reference frame is
> > > +   submitted under a different buffer ID, the driver may misidentify it and
> > > +   decode a new frame into it while it is still in use, resulting in corruption
> > > +   of the following frames.
> > > +
> > 
> > I think this is nice addition, but insufficient. We should extend the API with a
> > flags that let application know if the buffers are reference or secondary. For
> > the context, we have a mix of CODEC that will output usable reference frames and
> > needs careful manipulation and many other drivers where the buffers *maybe*
> > secondary, meaning they may have been post-processed and modifying these in-
> > place may have no impact.
> > 
> > The problem is the "may", that will depends on the chosen CAPTURE format. I
> > believe we should flag this, this flag should be set by the driver, on CAPTURE
> > queue. The information is known after S_FMT, so Format Flag, Reqbufs
> > capabilities or querybuf flags are candidates. I think the buffer flags are the
> > best named flag, though we don't expect this to differ per buffer. Though,
> > userspace needs to call querybuf for all buf in order to export or map them.
> > 
> > What userspace can do with this is to export the DMABuf as read-only, and signal
> > this internally in its own context. This is great to avoid any unwanted side
> > effect described here.
> 
> I agree with the idea of having a way for the kernel to tell the
> userspace the exact state of the buffer, but right now the untold
> expectation of the kernel was as per what this patch adds. If one
> wants their userspace to be portable across different decoders, they
> need to keep the assumption. So the natural way to go here is to stay
> safe by default and have a flag that tells the userspace that the
> buffer can be freely reused.

On the V4L2 side, this is what I am asking, a flag to signal that the buffer can
be freely reused (or secondary). The last part was an example of what userland
that cares about robustness can do with it.

> 
> Best regards,
> Tomasz

Re: [EXT] Re: [PATCH] media: docs: dev-decoder: add restrictions about CAPTURE buffers

[Nicolas Dufresne]

Le vendredi 29 octobre 2021 à 10:28 +0300, Stanimir Varbanov a écrit :
> 
> On 10/29/21 5:10 AM, Ming Qian wrote:
> > > -----Original Message-----
> > > From: Nicolas Dufresne [mailto:nicolas@ndufresne.ca]
> > > Sent: Tuesday, October 26, 2021 10:12 PM
> > > To: Alexandre Courbot <acourbot@chromium.org>; Mauro Carvalho Chehab
> > > <mchehab@kernel.org>; Hans Verkuil <hverkuil-cisco@xs4all.nl>; Tomasz Figa
> > > <tfiga@chromium.org>
> > > Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: [EXT] Re: [PATCH] media: docs: dev-decoder: add restrictions about
> > > CAPTURE buffers
> > > 
> > > Caution: EXT Email
> > > 
> > > Le lundi 18 octobre 2021 à 18:14 +0900, Alexandre Courbot a écrit :
> > > > CAPTURE buffers might be read by the hardware after they are dequeued,
> > > > which goes against the general idea that userspace has full control
> > > > over dequeued buffers. Explain why and document the restrictions that
> > > > this implies for userspace.
> > > > 
> > > > Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
> > > > ---
> > > >  .../userspace-api/media/v4l/dev-decoder.rst     | 17
> > > +++++++++++++++++
> > > >  1 file changed, 17 insertions(+)
> > > > 
> > > > diff --git a/Documentation/userspace-api/media/v4l/dev-decoder.rst
> > > > b/Documentation/userspace-api/media/v4l/dev-decoder.rst
> > > > index 5b9b83feeceb..3cf2b496f2d0 100644
> > > > --- a/Documentation/userspace-api/media/v4l/dev-decoder.rst
> > > > +++ b/Documentation/userspace-api/media/v4l/dev-decoder.rst
> > > > @@ -752,6 +752,23 @@ available to dequeue. Specifically:
> > > >       buffers are out-of-order compared to the ``OUTPUT`` buffers):
> > > ``CAPTURE``
> > > >       timestamps will not retain the order of ``OUTPUT`` timestamps.
> > > > 
> > > > +.. note::
> > > > +
> > > > +   The backing memory of ``CAPTURE`` buffers that are used as reference
> > > frames
> > > > +   by the stream may be read by the hardware even after they are
> > > dequeued.
> > > > +   Consequently, the client should avoid writing into this memory while the
> > > > +   ``CAPTURE`` queue is streaming. Failure to observe this may result in
> > > > +   corruption of decoded frames.
> > > > +
> > > > +   Similarly, when using a memory type other than
> > > ``V4L2_MEMORY_MMAP``, the
> > > > +   client should make sure that each ``CAPTURE`` buffer is always queued
> > > with
> > > > +   the same backing memory for as long as the ``CAPTURE`` queue is
> > > streaming.
> > > > +   The reason for this is that V4L2 buffer indices can be used by drivers to
> > > > +   identify frames. Thus, if the backing memory of a reference frame is
> > > > +   submitted under a different buffer ID, the driver may misidentify it and
> > > > +   decode a new frame into it while it is still in use, resulting in corruption
> > > > +   of the following frames.
> > > > +
> > > 
> > > I think this is nice addition, but insufficient. We should extend the API with a
> > > flags that let application know if the buffers are reference or secondary. For the
> > > context, we have a mix of CODEC that will output usable reference frames and
> > > needs careful manipulation and many other drivers where the buffers *maybe*
> > > secondary, meaning they may have been post-processed and modifying these
> > > in- place may have no impact.
> > > 
> > > The problem is the "may", that will depends on the chosen CAPTURE format. I
> > > believe we should flag this, this flag should be set by the driver, on CAPTURE
> > > queue. The information is known after S_FMT, so Format Flag, Reqbufs
> > > capabilities or querybuf flags are candidates. I think the buffer flags are the
> > > best named flag, though we don't expect this to differ per buffer. Though,
> > > userspace needs to call querybuf for all buf in order to export or map them.
> > > 
> > > What userspace can do with this is to export the DMABuf as read-only, and
> > > signal this internally in its own context. This is great to avoid any unwanted
> > > side effect described here.
> > 
> > I think a flag should be add to tell a buffer is reference or secondary.
> > But for some codec, it's hard to determine the buffer flag when reqbufs.
> > The buffer flag should be dynamically updated by driver.
> > User can check the flag after dqbuf every time.
> 
> +1
> 
> I'm not familiar with stateless decoders where on the reqbuf time it
> could work, debut for stateful coders it should be a dynamic flag on
> every capture buffer.

This isn't very clear request here, on which C structure to you say you would
prefer this ?

There is no difference for stateful/stateless for this regard. The capture
format must have been configured prior to reqbuf, so nothing post S_FMT(CAPTURE)
can every be very dynamic. I think the flag in S_FMT is miss-named and would
create confusion. struct v4l2_reqbufs vs struc v4l2_buffer are equivalent. The
seconds opens for flexibility.

In fact, for some certain CODEC, there exist B-Frames that are never used as
references, so these could indeed can be written to even if the buffer are not
secondary. I think recommending to flag this in v4l2_buffer, and read through
VIDIOC_QUERYBUF would be the best choice.

> 
> > 
> > > 
> > > >  During the decoding, the decoder may initiate one of the special
> > > > sequences, as  listed below. The sequences will result in the decoder
> > > > returning all the  ``CAPTURE`` buffers that originated from all the
> > > > ``OUTPUT`` buffers processed
> > > 
> > 
> 

Re: [EXT] Re: [PATCH] media: docs: dev-decoder: add restrictions about CAPTURE buffers

[Alexandre Courbot]

Thanks for the comments. It looks like we are having a consensus that
the described behavior is the current (untold) expectation, and how a
client should behave if it wants to support all V4L2 decoders. OTOH it
would also be nice to be able to signal the client that CAPTURE
buffers are not used by the hardware and can thus be overwritten/freed
at will.

I have discussed a bit with Nicolas on IRC and we were wondering where
such a flag signaling that capability should be. We could have:

1) Something global to the currently set format, i.e. maybe take some
space from the reserved area of v4l2_pix_format_mplane to add a flag.
The property would then be global to all buffers, and apply between
calls to STREAMON and STREAMOFF.

2) An additional flag to the v4l2_buffer structure that would signal
whether the buffer is currently writable. This means the writable
property of buffers can be signaled on a finer grain (i.e. reference
frames would not be writable, but non-reference ones would be), and we
can even update the status of a given buffer after it is not used as a
reference (the client would have to QUERYBUF to get the updated status
though). OTOH a client that needs to know whether the buffers are
writable before streaming would need to query them all one-by-one.

I am not sure whether we need something as precise as 2), or whether
1) would be enough and globally simpler. Any more thoughts?

Cheers,
Alex.

On Mon, Nov 1, 2021 at 11:52 PM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
>
> Le vendredi 29 octobre 2021 à 10:28 +0300, Stanimir Varbanov a écrit :
> >
> > On 10/29/21 5:10 AM, Ming Qian wrote:
> > > > -----Original Message-----
> > > > From: Nicolas Dufresne [mailto:nicolas@ndufresne.ca]
> > > > Sent: Tuesday, October 26, 2021 10:12 PM
> > > > To: Alexandre Courbot <acourbot@chromium.org>; Mauro Carvalho Chehab
> > > > <mchehab@kernel.org>; Hans Verkuil <hverkuil-cisco@xs4all.nl>; Tomasz Figa
> > > > <tfiga@chromium.org>
> > > > Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org
> > > > Subject: [EXT] Re: [PATCH] media: docs: dev-decoder: add restrictions about
> > > > CAPTURE buffers
> > > >
> > > > Caution: EXT Email
> > > >
> > > > Le lundi 18 octobre 2021 à 18:14 +0900, Alexandre Courbot a écrit :
> > > > > CAPTURE buffers might be read by the hardware after they are dequeued,
> > > > > which goes against the general idea that userspace has full control
> > > > > over dequeued buffers. Explain why and document the restrictions that
> > > > > this implies for userspace.
> > > > >
> > > > > Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
> > > > > ---
> > > > >  .../userspace-api/media/v4l/dev-decoder.rst     | 17
> > > > +++++++++++++++++
> > > > >  1 file changed, 17 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/userspace-api/media/v4l/dev-decoder.rst
> > > > > b/Documentation/userspace-api/media/v4l/dev-decoder.rst
> > > > > index 5b9b83feeceb..3cf2b496f2d0 100644
> > > > > --- a/Documentation/userspace-api/media/v4l/dev-decoder.rst
> > > > > +++ b/Documentation/userspace-api/media/v4l/dev-decoder.rst
> > > > > @@ -752,6 +752,23 @@ available to dequeue. Specifically:
> > > > >       buffers are out-of-order compared to the ``OUTPUT`` buffers):
> > > > ``CAPTURE``
> > > > >       timestamps will not retain the order of ``OUTPUT`` timestamps.
> > > > >
> > > > > +.. note::
> > > > > +
> > > > > +   The backing memory of ``CAPTURE`` buffers that are used as reference
> > > > frames
> > > > > +   by the stream may be read by the hardware even after they are
> > > > dequeued.
> > > > > +   Consequently, the client should avoid writing into this memory while the
> > > > > +   ``CAPTURE`` queue is streaming. Failure to observe this may result in
> > > > > +   corruption of decoded frames.
> > > > > +
> > > > > +   Similarly, when using a memory type other than
> > > > ``V4L2_MEMORY_MMAP``, the
> > > > > +   client should make sure that each ``CAPTURE`` buffer is always queued
> > > > with
> > > > > +   the same backing memory for as long as the ``CAPTURE`` queue is
> > > > streaming.
> > > > > +   The reason for this is that V4L2 buffer indices can be used by drivers to
> > > > > +   identify frames. Thus, if the backing memory of a reference frame is
> > > > > +   submitted under a different buffer ID, the driver may misidentify it and
> > > > > +   decode a new frame into it while it is still in use, resulting in corruption
> > > > > +   of the following frames.
> > > > > +
> > > >
> > > > I think this is nice addition, but insufficient. We should extend the API with a
> > > > flags that let application know if the buffers are reference or secondary. For the
> > > > context, we have a mix of CODEC that will output usable reference frames and
> > > > needs careful manipulation and many other drivers where the buffers *maybe*
> > > > secondary, meaning they may have been post-processed and modifying these
> > > > in- place may have no impact.
> > > >
> > > > The problem is the "may", that will depends on the chosen CAPTURE format. I
> > > > believe we should flag this, this flag should be set by the driver, on CAPTURE
> > > > queue. The information is known after S_FMT, so Format Flag, Reqbufs
> > > > capabilities or querybuf flags are candidates. I think the buffer flags are the
> > > > best named flag, though we don't expect this to differ per buffer. Though,
> > > > userspace needs to call querybuf for all buf in order to export or map them.
> > > >
> > > > What userspace can do with this is to export the DMABuf as read-only, and
> > > > signal this internally in its own context. This is great to avoid any unwanted
> > > > side effect described here.
> > >
> > > I think a flag should be add to tell a buffer is reference or secondary.
> > > But for some codec, it's hard to determine the buffer flag when reqbufs.
> > > The buffer flag should be dynamically updated by driver.
> > > User can check the flag after dqbuf every time.
> >
> > +1
> >
> > I'm not familiar with stateless decoders where on the reqbuf time it
> > could work, debut for stateful coders it should be a dynamic flag on
> > every capture buffer.
>
> This isn't very clear request here, on which C structure to you say you would
> prefer this ?
>
> There is no difference for stateful/stateless for this regard. The capture
> format must have been configured prior to reqbuf, so nothing post S_FMT(CAPTURE)
> can every be very dynamic. I think the flag in S_FMT is miss-named and would
> create confusion. struct v4l2_reqbufs vs struc v4l2_buffer are equivalent. The
> seconds opens for flexibility.
>
> In fact, for some certain CODEC, there exist B-Frames that are never used as
> references, so these could indeed can be written to even if the buffer are not
> secondary. I think recommending to flag this in v4l2_buffer, and read through
> VIDIOC_QUERYBUF would be the best choice.
>
> >
> > >
> > > >
> > > > >  During the decoding, the decoder may initiate one of the special
> > > > > sequences, as  listed below. The sequences will result in the decoder
> > > > > returning all the  ``CAPTURE`` buffers that originated from all the
> > > > > ``OUTPUT`` buffers processed
> > > >
> > >
> >
>
>

Re: [EXT] Re: [PATCH] media: docs: dev-decoder: add restrictions about CAPTURE buffers

[Hans Verkuil]

On 03/11/2021 23:58, Alexandre Courbot wrote:
> Thanks for the comments. It looks like we are having a consensus that
> the described behavior is the current (untold) expectation, and how a
> client should behave if it wants to support all V4L2 decoders. OTOH it
> would also be nice to be able to signal the client that CAPTURE
> buffers are not used by the hardware and can thus be overwritten/freed
> at will.
> 
> I have discussed a bit with Nicolas on IRC and we were wondering where
> such a flag signaling that capability should be. We could have:
> 
> 1) Something global to the currently set format, i.e. maybe take some
> space from the reserved area of v4l2_pix_format_mplane to add a flag.
> The property would then be global to all buffers, and apply between
> calls to STREAMON and STREAMOFF.

VIDIOC_ENUM_FMT is already used to signal format flags, so it could be
put there.

Regards,

	Hans

> 
> 2) An additional flag to the v4l2_buffer structure that would signal
> whether the buffer is currently writable. This means the writable
> property of buffers can be signaled on a finer grain (i.e. reference
> frames would not be writable, but non-reference ones would be), and we
> can even update the status of a given buffer after it is not used as a
> reference (the client would have to QUERYBUF to get the updated status
> though). OTOH a client that needs to know whether the buffers are
> writable before streaming would need to query them all one-by-one.
> 
> I am not sure whether we need something as precise as 2), or whether
> 1) would be enough and globally simpler. Any more thoughts?
> 
> Cheers,
> Alex.
> 
> On Mon, Nov 1, 2021 at 11:52 PM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
>>
>> Le vendredi 29 octobre 2021 à 10:28 +0300, Stanimir Varbanov a écrit :
>>>
>>> On 10/29/21 5:10 AM, Ming Qian wrote:
>>>>> -----Original Message-----
>>>>> From: Nicolas Dufresne [mailto:nicolas@ndufresne.ca]
>>>>> Sent: Tuesday, October 26, 2021 10:12 PM
>>>>> To: Alexandre Courbot <acourbot@chromium.org>; Mauro Carvalho Chehab
>>>>> <mchehab@kernel.org>; Hans Verkuil <hverkuil-cisco@xs4all.nl>; Tomasz Figa
>>>>> <tfiga@chromium.org>
>>>>> Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org
>>>>> Subject: [EXT] Re: [PATCH] media: docs: dev-decoder: add restrictions about
>>>>> CAPTURE buffers
>>>>>
>>>>> Caution: EXT Email
>>>>>
>>>>> Le lundi 18 octobre 2021 à 18:14 +0900, Alexandre Courbot a écrit :
>>>>>> CAPTURE buffers might be read by the hardware after they are dequeued,
>>>>>> which goes against the general idea that userspace has full control
>>>>>> over dequeued buffers. Explain why and document the restrictions that
>>>>>> this implies for userspace.
>>>>>>
>>>>>> Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
>>>>>> ---
>>>>>>  .../userspace-api/media/v4l/dev-decoder.rst     | 17
>>>>> +++++++++++++++++
>>>>>>  1 file changed, 17 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/userspace-api/media/v4l/dev-decoder.rst
>>>>>> b/Documentation/userspace-api/media/v4l/dev-decoder.rst
>>>>>> index 5b9b83feeceb..3cf2b496f2d0 100644
>>>>>> --- a/Documentation/userspace-api/media/v4l/dev-decoder.rst
>>>>>> +++ b/Documentation/userspace-api/media/v4l/dev-decoder.rst
>>>>>> @@ -752,6 +752,23 @@ available to dequeue. Specifically:
>>>>>>       buffers are out-of-order compared to the ``OUTPUT`` buffers):
>>>>> ``CAPTURE``
>>>>>>       timestamps will not retain the order of ``OUTPUT`` timestamps.
>>>>>>
>>>>>> +.. note::
>>>>>> +
>>>>>> +   The backing memory of ``CAPTURE`` buffers that are used as reference
>>>>> frames
>>>>>> +   by the stream may be read by the hardware even after they are
>>>>> dequeued.
>>>>>> +   Consequently, the client should avoid writing into this memory while the
>>>>>> +   ``CAPTURE`` queue is streaming. Failure to observe this may result in
>>>>>> +   corruption of decoded frames.
>>>>>> +
>>>>>> +   Similarly, when using a memory type other than
>>>>> ``V4L2_MEMORY_MMAP``, the
>>>>>> +   client should make sure that each ``CAPTURE`` buffer is always queued
>>>>> with
>>>>>> +   the same backing memory for as long as the ``CAPTURE`` queue is
>>>>> streaming.
>>>>>> +   The reason for this is that V4L2 buffer indices can be used by drivers to
>>>>>> +   identify frames. Thus, if the backing memory of a reference frame is
>>>>>> +   submitted under a different buffer ID, the driver may misidentify it and
>>>>>> +   decode a new frame into it while it is still in use, resulting in corruption
>>>>>> +   of the following frames.
>>>>>> +
>>>>>
>>>>> I think this is nice addition, but insufficient. We should extend the API with a
>>>>> flags that let application know if the buffers are reference or secondary. For the
>>>>> context, we have a mix of CODEC that will output usable reference frames and
>>>>> needs careful manipulation and many other drivers where the buffers *maybe*
>>>>> secondary, meaning they may have been post-processed and modifying these
>>>>> in- place may have no impact.
>>>>>
>>>>> The problem is the "may", that will depends on the chosen CAPTURE format. I
>>>>> believe we should flag this, this flag should be set by the driver, on CAPTURE
>>>>> queue. The information is known after S_FMT, so Format Flag, Reqbufs
>>>>> capabilities or querybuf flags are candidates. I think the buffer flags are the
>>>>> best named flag, though we don't expect this to differ per buffer. Though,
>>>>> userspace needs to call querybuf for all buf in order to export or map them.
>>>>>
>>>>> What userspace can do with this is to export the DMABuf as read-only, and
>>>>> signal this internally in its own context. This is great to avoid any unwanted
>>>>> side effect described here.
>>>>
>>>> I think a flag should be add to tell a buffer is reference or secondary.
>>>> But for some codec, it's hard to determine the buffer flag when reqbufs.
>>>> The buffer flag should be dynamically updated by driver.
>>>> User can check the flag after dqbuf every time.
>>>
>>> +1
>>>
>>> I'm not familiar with stateless decoders where on the reqbuf time it
>>> could work, debut for stateful coders it should be a dynamic flag on
>>> every capture buffer.
>>
>> This isn't very clear request here, on which C structure to you say you would
>> prefer this ?
>>
>> There is no difference for stateful/stateless for this regard. The capture
>> format must have been configured prior to reqbuf, so nothing post S_FMT(CAPTURE)
>> can every be very dynamic. I think the flag in S_FMT is miss-named and would
>> create confusion. struct v4l2_reqbufs vs struc v4l2_buffer are equivalent. The
>> seconds opens for flexibility.
>>
>> In fact, for some certain CODEC, there exist B-Frames that are never used as
>> references, so these could indeed can be written to even if the buffer are not
>> secondary. I think recommending to flag this in v4l2_buffer, and read through
>> VIDIOC_QUERYBUF would be the best choice.
>>
>>>
>>>>
>>>>>
>>>>>>  During the decoding, the decoder may initiate one of the special
>>>>>> sequences, as  listed below. The sequences will result in the decoder
>>>>>> returning all the  ``CAPTURE`` buffers that originated from all the
>>>>>> ``OUTPUT`` buffers processed
>>>>>
>>>>
>>>
>>
>>