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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx] >>>>> Sent: Tuesday, October 26, 2021 10:12 PM >>>>> To: Alexandre Courbot <acourbot@xxxxxxxxxxxx>; Mauro Carvalho Chehab >>>>> <mchehab@xxxxxxxxxx>; Hans Verkuil <hverkuil-cisco@xxxxxxxxx>; Tomasz Figa >>>>> <tfiga@xxxxxxxxxxxx> >>>>> Cc: linux-media@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx >>>>> 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@xxxxxxxxxxxx> >>>>>> --- >>>>>> .../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 >>>>> >>>> >>> >> >>
