Am 24.08.22 um 17:49 schrieb Dmitry Osipenko:
On 8/24/22 18:24, Christian König wrote:
Am 24.08.22 um 12:22 schrieb Dmitry Osipenko:
Move dma-buf attachment API functions to the dynamic locking specification. The strict locking convention prevents deadlock situations for dma-buf importers and exporters.
Previously, the "unlocked" versions of the attachment API functions weren't taking the reservation lock and this patch makes them to take the lock.
Intel and AMD GPU drivers already were mapping the attached dma-bufs under the held lock during attachment, hence these drivers are updated to use the locked functions.
Signed-off-by: Dmitry Osipenko dmitry.osipenko@collabora.com
drivers/dma-buf/dma-buf.c | 115 ++++++++++++++------- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 4 +- drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 8 +- drivers/gpu/drm/i915/gem/i915_gem_object.c | 12 +++ include/linux/dma-buf.h | 20 ++-- 5 files changed, 110 insertions(+), 49 deletions(-)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 4556a12bd741..f2a5a122da4a 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -559,7 +559,7 @@ static struct file *dma_buf_getfile(struct dma_buf *dmabuf, int flags) * 2. Userspace passes this file-descriptors to all drivers it wants this buffer * to share with: First the file descriptor is converted to a &dma_buf using * dma_buf_get(). Then the buffer is attached to the device using
- * dma_buf_attach().
- * dma_buf_attach_unlocked().
Now I get why this is confusing me so much.
The _unlocked postfix implies that there is another function which should be called with the locks already held, but this is not the case for attach/detach (because they always need to grab the lock themselves).
That's correct. The attach/detach ops of exporter can take the lock (like i915 exporter does it), hence importer must not grab the lock around dma_buf_attach() invocation.
So I suggest to drop the _unlocked postfix for the attach/detach functions. Another step would then be to unify attach/detach with dynamic_attach/dynamic_detach when both have the same locking convention anyway.
It's not a problem to change the name, but it's unclear to me why we should do it. The _unlocked postfix tells importer that reservation must be unlocked and it must be unlocked in case of dma_buf_attach().
Dropping the postfix will make dma_buf_attach() inconsistent with the rest of the _unlocked functions(?). Are you sure we need to rename it?
The idea of the postfix was to distinguish between two different versions of the same function, e.g. dma_buf_vmap_unlocked() vs normal dma_buf_vmap().
When we don't have those two types of the same function I don't think it makes to much sense to keep that. We should just properly document which functions expect what and that's what your documentation patch does.
Regards, Christian.
Sorry that this is going so much back and forth, it's really complicated to keep all the stuff in my head at the moment :)
Not a problem at all, I expected that it will take some time for this patchset to settle down.