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 _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel