On Wed, Jul 8, 2020 at 6:35 AM Alexandre Courbot <acourbot@xxxxxxxxxxxx> wrote: > > 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? > There are decoders (e.g. s5p-mfc) which require all the framebuffers to be registered beforehands. > > - 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). I suppose you mean without the last two? Best regards, Tomasz
