On Fri, Jul 3, 2020 at 6:55 PM Tomasz Figa <tfiga@xxxxxxxxxxxx> 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, Can't we get around this by just requiring that DMABUFs passed to VIDIOC_QBUFs are always the same for a given index? Why would it be necessary to require all buffers to be queued before starting the codec? > - 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. Even without these I guess we can probably get something working at the cost of higher restrictions to clients (i.e. purely static set of buffers). > > Best regards, > Tomasz
