On Thu, Aug 18, 2022 at 02:58:10PM +0200, Christian König wrote:
The only thing I'm not 100% convinced of is dma_buf_try_get(), I've seen this incorrectly used so many times that I can't count them any more.
Would that be somehow avoidable? Or could you at least explain the use case a bit better.
I didn't see a way, maybe you know of one
For GEM objects we usually don't use the reference count of the DMA-buf, but rather that of the GEM object for this. But that's not an ideal solution either.
You can't really ignore the dmabuf refcount. At some point you have to deal with the dmabuf being asynchronously released by userspace.
down_write(&vdev->memory_lock); list_for_each_entry_safe(priv, tmp, &vdev->dmabufs, dmabufs_elm) { if (!dma_buf_try_get(priv->dmabuf)) continue;
What would happen if you don't skip destroyed dma-bufs here? In other words why do you maintain that list in the first place?
The list is to keep track of the dmabufs that were created, it is not optional.
The only question is what do you do about invoking dma_buf_move_notify() on a dmabuf that is already undergoing destruction.
For instance undergoing destruction means the dmabuf core has already done this:
mutex_lock(&db_list.lock); list_del(&dmabuf->list_node); mutex_unlock(&db_list.lock); dma_buf_stats_teardown(dmabuf);
So it seems non-ideal to continue to use it.
However, dma_buf_move_notify() specifically has no issue with that level of destruction since it only does
list_for_each_entry(attach, &dmabuf->attachments, node)
And attachments must be empty if the file refcount is zero.
So we could delete the try_buf and just rely on move being safe on partially destroyed dma_buf's as part of the API design.
Jason