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@xxxxxxxxxxxxx>
---
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.
