Messages in this thread | | | Subject | Re: [EXT] Re: [PATCH] media: docs: dev-decoder: add restrictions about CAPTURE buffers | From | Stanimir Varbanov <> | Date | Fri, 29 Oct 2021 10:28:15 +0300 |
| |
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
| |