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

-----Original Message-----
From: Nicolas Dufresne [mailto:nicolas@ndufresne.ca]
Sent: Tuesday, October 26, 2021 10:12 PM
To: Alexandre Courbot <redacted>; Mauro Carvalho Chehab
[off-list ref]; Hans Verkuil [off-list ref]; Tomasz Figa
[off-list ref]
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 :

quoted

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 <redacted>
---
 .../userspace-api/media/v4l/dev-decoder.rst     | 17

+++++++++++++++++

quoted

 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``

quoted

      timestamps will not retain the order of ``OUTPUT`` timestamps.

+.. note::
+
+   The backing memory of ``CAPTURE`` buffers that are used as reference

frames

quoted

+   by the stream may be read by the hardware even after they are

dequeued.

quoted

+   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

quoted

+   client should make sure that each ``CAPTURE`` buffer is always queued

with

quoted

+   the same backing memory for as long as the ``CAPTURE`` queue is

streaming.

quoted

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

quoted

 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