Hi Alexandre, Tomasz, Thank you very much for your feedback. Yes, unfortunately we cannot disable dmabuf mode as it is currently mandatory for Android. AFAIU the work to have this ready in the main v4l2 spec requires time. But in the virtio-video spec we indeed can mention something like that the device does not expected the backing memory for an existing resource id to change. Best regards, Dmitry. On Freitag, 3. Juli 2020 11:55:29 CEST Tomasz Figa wrote: > On Fri, Jul 3, 2020 at 11:27 AM Alexandre Courbot <acourbot@xxxxxxxxxxxx> wrote: > > On Fri, Jul 3, 2020 at 6:18 PM Michael S. Tsirkin <mst@xxxxxxxxxx> wrote: > > > On Fri, Jul 03, 2020 at 02:45:15PM +0900, Alexandre Courbot wrote: > > > > Hi Dmitry, > > > > > > > > On Thu, Jul 2, 2020 at 10:47 PM Dmitry Sepp <dmitry.sepp@xxxxxxxxxxxxxxx> wrote: > > > > > Hi Keiichi, > > > > > > > > > > Thanks for the clarification. I believe we should explicitly > > > > > describe this in the VIRTIO_VIDEO_CMD_RESOURCE_ATTACH section. And > > > > > I also still see a problem there. If it is a guest allocated > > > > > resource, we cannot consider it to be free until the device really > > > > > releases it. And it won't happen until we issue the next ATTACH > > > > > call. Also, as we discussed before, it might be not possible to > > > > > free individual buffers, but the whole queue only. > > > > > > > > In the case of the encoder, a V4L2 driver is not supposed to let > > > > user-space dequeue an input frame while it is used as reference for > > > > the encoding process. So if we add a similar rule in the virtio-video > > > > specification, I suppose this would solve the problem? > > > > > > > > For the decoder case, we have a bigger problem though. Since DMABUFs > > > > can be arbitrarily attached to any V4L2 buffer ID, we may end up > > > > re-queueing the DMABUF of a decoded frame that is still used as > > > > reference under a different V4L2 buffer ID. In this case I don't think > > > > the driver has a way to know that the memory resource should not be > > > > overwritten, and it will thus happily use it as a decode target. The > > > > easiest fix is probably to update the V4L2 stateful specification to > > > > require that reused DMABUFs must always be assigned to the same V4L2 > > > > buffer ID, and must be kept alive as long as decoding is in progress, > > > > or until a resolution change event is received. We can then apply a > > > > similar rule to the virtio device. > > > > > > Sounds like a generic kind of problem - how do other devices solve it? > > > > Most users of V4L2 drivers use MMAP buffers, which don't suffer from > > this problem: since MMAP buffers are managed by V4L2 and the same V4L2 > > buffer ID always corresponds to the same backing memory, the driver > > just needs to refrain from decoding into a given V4L2 buffer as long > > as it is used as a reference frames. This is something that all > > drivers currently do AFAICT. > > > > The problem only occurs if you let userspace map anything to V4L2 > > buffers (USERPTR or DMABUF buffers). In order to guarantee the same > > reliable behavior as with MMAP buffers, you must enforce the same > > rule: always the same backing memory for a given V4L2 buffer. > > > > ... or you can rotate between a large enough number of buffers to > > leave enough space for the reference tag to expire on your frames, but > > that's clearly not a good way to address the problem. > > FWIW, it's typically solved with regular devices by completely > disallowing the DMABUF/USERPTR modes and only allowing the > V4L2-managed MMAP mode for affected buffer queues. > > However, that's quite a severe limitation and with a careful API > extension, DMABUF could be still handled. Namely: > - pre-registration of buffers at initialization time: that would > likely mean mandating VIDIOC_QBUF for all indexes before any > decoding/encoding can start, > - enforcement of index-buffer mapping: VIDIOC_QBUF would have to fail > if one attempts to queue another buffer at the same index, > - ability to explicitly release existing buffers: there is > VIDIOC_RELEASE_BUF in the works which could be used to release a > specific index, > - ability to explicitly add new buffers: a combination of > VIDIOC_CREATEBUFS + VIDIOC_QBUF could be already used to achieve this. > > Best regards, > Tomasz
