linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 0/6] Move all drivers to a common dma-buf locking convention
@ 2022-07-15  0:52 Dmitry Osipenko
  2022-07-15  0:52 ` [PATCH v1 1/6] dma-buf: Add _unlocked postfix to function names Dmitry Osipenko
                   ` (6 more replies)
  0 siblings, 7 replies; 18+ messages in thread
From: Dmitry Osipenko @ 2022-07-15  0:52 UTC (permalink / raw)
  To: David Airlie, Gerd Hoffmann, Gurchetan Singh, Chia-I Wu,
	Daniel Vetter, Daniel Almeida, Gert Wollny, Gustavo Padovan,
	Daniel Stone, Tomeu Vizoso, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, Rob Clark, Sumit Semwal, Christian König,
	Pan, Xinhui, Thierry Reding, Tomasz Figa, Marek Szyprowski,
	Mauro Carvalho Chehab, Alex Deucher, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin,
	Thomas Hellström
  Cc: dri-devel, linux-kernel, Dmitry Osipenko, linux-media,
	linaro-mm-sig, amd-gfx, intel-gfx, kernel, virtualization,
	spice-devel, linux-rdma, linux-arm-msm

Hello,

This series moves all drivers to a dynamic dma-buf locking specification.
From now on all dma-buf importers are made responsible for holding
dma-buf's reservation lock around all operations performed over dma-bufs.
This common locking convention allows us to utilize reservation lock more
broadly around kernel without fearing of potential dead locks.

This patchset passes all i915 selftests. It was also tested using VirtIO,
Panfrost, Lima and Tegra drivers. I tested cases of display+GPU,
display+V4L and GPU+V4L dma-buf sharing, which covers majority of kernel
drivers since rest of the drivers share same or similar code paths.

This is a continuation of [1] where Christian König asked to factor out
the dma-buf locking changes into separate series.

[1] https://lore.kernel.org/dri-devel/20220526235040.678984-1-dmitry.osipenko@collabora.com/

Dmitry Osipenko (6):
  dma-buf: Add _unlocked postfix to function names
  drm/gem: Take reservation lock for vmap/vunmap operations
  dma-buf: Move all dma-bufs to dynamic locking specification
  dma-buf: Acquire wait-wound context on attachment
  media: videobuf2: Stop using internal dma-buf lock
  dma-buf: Remove internal lock

 drivers/dma-buf/dma-buf.c                     | 198 +++++++++++-------
 drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c   |   4 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c       |   4 +-
 drivers/gpu/drm/armada/armada_gem.c           |  14 +-
 drivers/gpu/drm/drm_client.c                  |   4 +-
 drivers/gpu/drm/drm_gem.c                     |  28 +++
 drivers/gpu/drm/drm_gem_cma_helper.c          |   6 +-
 drivers/gpu/drm/drm_gem_framebuffer_helper.c  |   6 +-
 drivers/gpu/drm/drm_gem_shmem_helper.c        |   6 +-
 drivers/gpu/drm/drm_prime.c                   |  12 +-
 drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c   |   6 +-
 drivers/gpu/drm/exynos/exynos_drm_gem.c       |   2 +-
 drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c    |  20 +-
 .../gpu/drm/i915/gem/i915_gem_execbuffer.c    |   2 +-
 drivers/gpu/drm/i915/gem/i915_gem_object.h    |   6 +-
 .../drm/i915/gem/selftests/i915_gem_dmabuf.c  |  20 +-
 drivers/gpu/drm/i915/i915_gem_evict.c         |   2 +-
 drivers/gpu/drm/i915/i915_gem_ww.c            |  26 ++-
 drivers/gpu/drm/i915/i915_gem_ww.h            |  15 +-
 drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c     |   8 +-
 drivers/gpu/drm/qxl/qxl_object.c              |  17 +-
 drivers/gpu/drm/qxl/qxl_prime.c               |   4 +-
 drivers/gpu/drm/tegra/gem.c                   |  27 +--
 drivers/infiniband/core/umem_dmabuf.c         |  11 +-
 .../common/videobuf2/videobuf2-dma-contig.c   |  26 +--
 .../media/common/videobuf2/videobuf2-dma-sg.c |  23 +-
 .../common/videobuf2/videobuf2-vmalloc.c      |  17 +-
 .../platform/nvidia/tegra-vde/dmabuf-cache.c  |  12 +-
 drivers/misc/fastrpc.c                        |  12 +-
 drivers/xen/gntdev-dmabuf.c                   |  14 +-
 include/drm/drm_gem.h                         |   3 +
 include/linux/dma-buf.h                       |  49 ++---
 32 files changed, 347 insertions(+), 257 deletions(-)

-- 
2.36.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v1 1/6] dma-buf: Add _unlocked postfix to function names
  2022-07-15  0:52 [PATCH v1 0/6] Move all drivers to a common dma-buf locking convention Dmitry Osipenko
@ 2022-07-15  0:52 ` Dmitry Osipenko
  2022-07-15  7:19   ` Christian König
  2022-07-15  0:52 ` [PATCH v1 2/6] drm/gem: Take reservation lock for vmap/vunmap operations Dmitry Osipenko
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 18+ messages in thread
From: Dmitry Osipenko @ 2022-07-15  0:52 UTC (permalink / raw)
  To: David Airlie, Gerd Hoffmann, Gurchetan Singh, Chia-I Wu,
	Daniel Vetter, Daniel Almeida, Gert Wollny, Gustavo Padovan,
	Daniel Stone, Tomeu Vizoso, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, Rob Clark, Sumit Semwal, Christian König,
	Pan, Xinhui, Thierry Reding, Tomasz Figa, Marek Szyprowski,
	Mauro Carvalho Chehab, Alex Deucher, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin,
	Thomas Hellström
  Cc: dri-devel, linux-kernel, Dmitry Osipenko, linux-media,
	linaro-mm-sig, amd-gfx, intel-gfx, kernel, virtualization,
	spice-devel, linux-rdma, linux-arm-msm

Add _unlocked postfix to the dma-buf API function names in a preparation
to move all non-dynamic dma-buf users over to the dynamic locking
specification. This patch only renames API functions, preparing drivers
to the common locking convention. Later on we will make the "unlocked"
functions to take the reservation lock.

Suggested-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 drivers/dma-buf/dma-buf.c                     | 76 ++++++++++---------
 drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c   |  4 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c       |  4 +-
 drivers/gpu/drm/armada/armada_gem.c           | 14 ++--
 drivers/gpu/drm/drm_gem_cma_helper.c          |  6 +-
 drivers/gpu/drm/drm_gem_shmem_helper.c        |  6 +-
 drivers/gpu/drm/drm_prime.c                   | 12 +--
 drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c   |  6 +-
 drivers/gpu/drm/exynos/exynos_drm_gem.c       |  2 +-
 drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c    | 12 +--
 .../drm/i915/gem/selftests/i915_gem_dmabuf.c  | 20 ++---
 drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c     |  8 +-
 drivers/gpu/drm/tegra/gem.c                   | 27 +++----
 drivers/infiniband/core/umem_dmabuf.c         | 11 +--
 .../common/videobuf2/videobuf2-dma-contig.c   | 15 ++--
 .../media/common/videobuf2/videobuf2-dma-sg.c | 12 +--
 .../common/videobuf2/videobuf2-vmalloc.c      |  6 +-
 .../platform/nvidia/tegra-vde/dmabuf-cache.c  | 12 +--
 drivers/misc/fastrpc.c                        | 12 +--
 drivers/xen/gntdev-dmabuf.c                   | 14 ++--
 include/linux/dma-buf.h                       | 34 +++++----
 21 files changed, 161 insertions(+), 152 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 44574fbe7482..d16237a6ffaa 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -795,7 +795,7 @@ static struct sg_table * __map_dma_buf(struct dma_buf_attachment *attach,
 }
 
 /**
- * dma_buf_dynamic_attach - Add the device to dma_buf's attachments list
+ * dma_buf_dynamic_attach_unlocked - Add the device to dma_buf's attachments list
  * @dmabuf:		[in]	buffer to attach device to.
  * @dev:		[in]	device to be attached.
  * @importer_ops:	[in]	importer operations for the attachment
@@ -817,9 +817,9 @@ static struct sg_table * __map_dma_buf(struct dma_buf_attachment *attach,
  * indicated with the error code -EBUSY.
  */
 struct dma_buf_attachment *
-dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev,
-		       const struct dma_buf_attach_ops *importer_ops,
-		       void *importer_priv)
+dma_buf_dynamic_attach_unlocked(struct dma_buf *dmabuf, struct device *dev,
+				const struct dma_buf_attach_ops *importer_ops,
+				void *importer_priv)
 {
 	struct dma_buf_attachment *attach;
 	int ret;
@@ -892,25 +892,25 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev,
 	if (dma_buf_is_dynamic(attach->dmabuf))
 		dma_resv_unlock(attach->dmabuf->resv);
 
-	dma_buf_detach(dmabuf, attach);
+	dma_buf_detach_unlocked(dmabuf, attach);
 	return ERR_PTR(ret);
 }
-EXPORT_SYMBOL_NS_GPL(dma_buf_dynamic_attach, DMA_BUF);
+EXPORT_SYMBOL_NS_GPL(dma_buf_dynamic_attach_unlocked, DMA_BUF);
 
 /**
- * dma_buf_attach - Wrapper for dma_buf_dynamic_attach
+ * dma_buf_attach_unlocked - Wrapper for dma_buf_dynamic_attach
  * @dmabuf:	[in]	buffer to attach device to.
  * @dev:	[in]	device to be attached.
  *
- * Wrapper to call dma_buf_dynamic_attach() for drivers which still use a static
- * mapping.
+ * Wrapper to call dma_buf_dynamic_attach_unlocked() for drivers which still
+ * use a static mapping.
  */
-struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
-					  struct device *dev)
+struct dma_buf_attachment *dma_buf_attach_unlocked(struct dma_buf *dmabuf,
+						   struct device *dev)
 {
-	return dma_buf_dynamic_attach(dmabuf, dev, NULL, NULL);
+	return dma_buf_dynamic_attach_unlocked(dmabuf, dev, NULL, NULL);
 }
-EXPORT_SYMBOL_NS_GPL(dma_buf_attach, DMA_BUF);
+EXPORT_SYMBOL_NS_GPL(dma_buf_attach_unlocked, DMA_BUF);
 
 static void __unmap_dma_buf(struct dma_buf_attachment *attach,
 			    struct sg_table *sg_table,
@@ -923,7 +923,7 @@ static void __unmap_dma_buf(struct dma_buf_attachment *attach,
 }
 
 /**
- * dma_buf_detach - Remove the given attachment from dmabuf's attachments list
+ * dma_buf_detach_unlocked - Remove the given attachment from dmabuf's attachments list
  * @dmabuf:	[in]	buffer to detach from.
  * @attach:	[in]	attachment to be detached; is free'd after this call.
  *
@@ -931,7 +931,8 @@ static void __unmap_dma_buf(struct dma_buf_attachment *attach,
  *
  * Optionally this calls &dma_buf_ops.detach for device-specific detach.
  */
-void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach)
+void dma_buf_detach_unlocked(struct dma_buf *dmabuf,
+			     struct dma_buf_attachment *attach)
 {
 	if (WARN_ON(!dmabuf || !attach))
 		return;
@@ -956,14 +957,14 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach)
 
 	kfree(attach);
 }
-EXPORT_SYMBOL_NS_GPL(dma_buf_detach, DMA_BUF);
+EXPORT_SYMBOL_NS_GPL(dma_buf_detach_unlocked, DMA_BUF);
 
 /**
  * dma_buf_pin - Lock down the DMA-buf
  * @attach:	[in]	attachment which should be pinned
  *
- * Only dynamic importers (who set up @attach with dma_buf_dynamic_attach()) may
- * call this, and only for limited use cases like scanout and not for temporary
+ * Only dynamic importers (who set up @attach with dma_buf_dynamic_attach_unlocked())
+ * may call this, and only for limited use cases like scanout and not for temporary
  * pin operations. It is not permitted to allow userspace to pin arbitrary
  * amounts of buffers through this interface.
  *
@@ -1010,7 +1011,7 @@ void dma_buf_unpin(struct dma_buf_attachment *attach)
 EXPORT_SYMBOL_NS_GPL(dma_buf_unpin, DMA_BUF);
 
 /**
- * dma_buf_map_attachment - Returns the scatterlist table of the attachment;
+ * dma_buf_map_attachment_locked - Returns the scatterlist table of the attachment;
  * mapped into _device_ address space. Is a wrapper for map_dma_buf() of the
  * dma_buf_ops.
  * @attach:	[in]	attachment whose scatterlist is to be returned
@@ -1030,8 +1031,9 @@ EXPORT_SYMBOL_NS_GPL(dma_buf_unpin, DMA_BUF);
  * Important: Dynamic importers must wait for the exclusive fence of the struct
  * dma_resv attached to the DMA-BUF first.
  */
-struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
-					enum dma_data_direction direction)
+struct sg_table *
+dma_buf_map_attachment_unlocked(struct dma_buf_attachment *attach,
+				enum dma_data_direction direction)
 {
 	struct sg_table *sg_table;
 	int r;
@@ -1097,10 +1099,10 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
 #endif /* CONFIG_DMA_API_DEBUG */
 	return sg_table;
 }
-EXPORT_SYMBOL_NS_GPL(dma_buf_map_attachment, DMA_BUF);
+EXPORT_SYMBOL_NS_GPL(dma_buf_map_attachment_unlocked, DMA_BUF);
 
 /**
- * dma_buf_unmap_attachment - unmaps and decreases usecount of the buffer;might
+ * dma_buf_unmap_attachment_unlocked - unmaps and decreases usecount of the buffer;might
  * deallocate the scatterlist associated. Is a wrapper for unmap_dma_buf() of
  * dma_buf_ops.
  * @attach:	[in]	attachment to unmap buffer from
@@ -1109,9 +1111,9 @@ EXPORT_SYMBOL_NS_GPL(dma_buf_map_attachment, DMA_BUF);
  *
  * This unmaps a DMA mapping for @attached obtained by dma_buf_map_attachment().
  */
-void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
-				struct sg_table *sg_table,
-				enum dma_data_direction direction)
+void dma_buf_unmap_attachment_unlocked(struct dma_buf_attachment *attach,
+				       struct sg_table *sg_table,
+				       enum dma_data_direction direction)
 {
 	might_sleep();
 
@@ -1133,7 +1135,7 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
 	    !IS_ENABLED(CONFIG_DMABUF_MOVE_NOTIFY))
 		dma_buf_unpin(attach);
 }
-EXPORT_SYMBOL_NS_GPL(dma_buf_unmap_attachment, DMA_BUF);
+EXPORT_SYMBOL_NS_GPL(dma_buf_unmap_attachment_unlocked, DMA_BUF);
 
 /**
  * dma_buf_move_notify - notify attachments that DMA-buf is moving
@@ -1330,7 +1332,7 @@ EXPORT_SYMBOL_NS_GPL(dma_buf_end_cpu_access, DMA_BUF);
 
 
 /**
- * dma_buf_mmap - Setup up a userspace mmap with the given vma
+ * dma_buf_mmap_unlocked - Setup up a userspace mmap with the given vma
  * @dmabuf:	[in]	buffer that should back the vma
  * @vma:	[in]	vma for the mmap
  * @pgoff:	[in]	offset in pages where this mmap should start within the
@@ -1343,8 +1345,8 @@ EXPORT_SYMBOL_NS_GPL(dma_buf_end_cpu_access, DMA_BUF);
  *
  * Can return negative error values, returns 0 on success.
  */
-int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma,
-		 unsigned long pgoff)
+int dma_buf_mmap_unlocked(struct dma_buf *dmabuf, struct vm_area_struct *vma,
+			  unsigned long pgoff)
 {
 	if (WARN_ON(!dmabuf || !vma))
 		return -EINVAL;
@@ -1368,10 +1370,10 @@ int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma,
 
 	return dmabuf->ops->mmap(dmabuf, vma);
 }
-EXPORT_SYMBOL_NS_GPL(dma_buf_mmap, DMA_BUF);
+EXPORT_SYMBOL_NS_GPL(dma_buf_mmap_unlocked, DMA_BUF);
 
 /**
- * dma_buf_vmap - Create virtual mapping for the buffer object into kernel
+ * dma_buf_vmap_unlocked - Create virtual mapping for the buffer object into kernel
  * address space. Same restrictions as for vmap and friends apply.
  * @dmabuf:	[in]	buffer to vmap
  * @map:	[out]	returns the vmap pointer
@@ -1386,7 +1388,7 @@ EXPORT_SYMBOL_NS_GPL(dma_buf_mmap, DMA_BUF);
  *
  * Returns 0 on success, or a negative errno code otherwise.
  */
-int dma_buf_vmap(struct dma_buf *dmabuf, struct iosys_map *map)
+int dma_buf_vmap_unlocked(struct dma_buf *dmabuf, struct iosys_map *map)
 {
 	struct iosys_map ptr;
 	int ret = 0;
@@ -1422,14 +1424,14 @@ int dma_buf_vmap(struct dma_buf *dmabuf, struct iosys_map *map)
 	mutex_unlock(&dmabuf->lock);
 	return ret;
 }
-EXPORT_SYMBOL_NS_GPL(dma_buf_vmap, DMA_BUF);
+EXPORT_SYMBOL_NS_GPL(dma_buf_vmap_unlocked, DMA_BUF);
 
 /**
- * dma_buf_vunmap - Unmap a vmap obtained by dma_buf_vmap.
+ * dma_buf_vunmap_unlocked - Unmap a vmap obtained by dma_buf_vmap.
  * @dmabuf:	[in]	buffer to vunmap
  * @map:	[in]	vmap pointer to vunmap
  */
-void dma_buf_vunmap(struct dma_buf *dmabuf, struct iosys_map *map)
+void dma_buf_vunmap_unlocked(struct dma_buf *dmabuf, struct iosys_map *map)
 {
 	if (WARN_ON(!dmabuf))
 		return;
@@ -1446,7 +1448,7 @@ void dma_buf_vunmap(struct dma_buf *dmabuf, struct iosys_map *map)
 	}
 	mutex_unlock(&dmabuf->lock);
 }
-EXPORT_SYMBOL_NS_GPL(dma_buf_vunmap, DMA_BUF);
+EXPORT_SYMBOL_NS_GPL(dma_buf_vunmap_unlocked, DMA_BUF);
 
 #ifdef CONFIG_DEBUG_FS
 static int dma_buf_debug_show(struct seq_file *s, void *unused)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
index 782cbca37538..d9ed5a4fbc6f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
@@ -449,8 +449,8 @@ struct drm_gem_object *amdgpu_gem_prime_import(struct drm_device *dev,
 	if (IS_ERR(obj))
 		return obj;
 
-	attach = dma_buf_dynamic_attach(dma_buf, dev->dev,
-					&amdgpu_dma_buf_attach_ops, obj);
+	attach = dma_buf_dynamic_attach_unlocked(dma_buf, dev->dev,
+						 &amdgpu_dma_buf_attach_ops, obj);
 	if (IS_ERR(attach)) {
 		drm_gem_object_put(obj);
 		return ERR_CAST(attach);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 170935c294f5..e354ad140a0a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -883,7 +883,7 @@ static int amdgpu_ttm_backend_bind(struct ttm_device *bdev,
 			struct sg_table *sgt;
 
 			attach = gtt->gobj->import_attach;
-			sgt = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL);
+			sgt = dma_buf_map_attachment_unlocked(attach, DMA_BIDIRECTIONAL);
 			if (IS_ERR(sgt))
 				return PTR_ERR(sgt);
 
@@ -1008,7 +1008,7 @@ static void amdgpu_ttm_backend_unbind(struct ttm_device *bdev,
 		struct dma_buf_attachment *attach;
 
 		attach = gtt->gobj->import_attach;
-		dma_buf_unmap_attachment(attach, ttm->sg, DMA_BIDIRECTIONAL);
+		dma_buf_unmap_attachment_unlocked(attach, ttm->sg, DMA_BIDIRECTIONAL);
 		ttm->sg = NULL;
 	}
 
diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c
index 147abf1a3968..f71f3b2d20e3 100644
--- a/drivers/gpu/drm/armada/armada_gem.c
+++ b/drivers/gpu/drm/armada/armada_gem.c
@@ -66,8 +66,8 @@ void armada_gem_free_object(struct drm_gem_object *obj)
 	if (dobj->obj.import_attach) {
 		/* We only ever display imported data */
 		if (dobj->sgt)
-			dma_buf_unmap_attachment(dobj->obj.import_attach,
-						 dobj->sgt, DMA_TO_DEVICE);
+			dma_buf_unmap_attachment_unlocked(dobj->obj.import_attach,
+							  dobj->sgt, DMA_TO_DEVICE);
 		drm_prime_gem_destroy(&dobj->obj, NULL);
 	}
 
@@ -364,7 +364,7 @@ int armada_gem_pwrite_ioctl(struct drm_device *dev, void *data,
 
 	if (args->offset > dobj->obj.size ||
 	    args->size > dobj->obj.size - args->offset) {
-		DRM_ERROR("invalid size: object size %u\n", dobj->obj.size);
+		DRM_ERROR("invalid size: object size %zu\n", dobj->obj.size);
 		ret = -EINVAL;
 		goto unref;
 	}
@@ -514,13 +514,13 @@ armada_gem_prime_import(struct drm_device *dev, struct dma_buf *buf)
 		}
 	}
 
-	attach = dma_buf_attach(buf, dev->dev);
+	attach = dma_buf_attach_unlocked(buf, dev->dev);
 	if (IS_ERR(attach))
 		return ERR_CAST(attach);
 
 	dobj = armada_gem_alloc_private_object(dev, buf->size);
 	if (!dobj) {
-		dma_buf_detach(buf, attach);
+		dma_buf_detach_unlocked(buf, attach);
 		return ERR_PTR(-ENOMEM);
 	}
 
@@ -539,8 +539,8 @@ int armada_gem_map_import(struct armada_gem_object *dobj)
 {
 	int ret;
 
-	dobj->sgt = dma_buf_map_attachment(dobj->obj.import_attach,
-					   DMA_TO_DEVICE);
+	dobj->sgt = dma_buf_map_attachment_unlocked(dobj->obj.import_attach,
+						    DMA_TO_DEVICE);
 	if (IS_ERR(dobj->sgt)) {
 		ret = PTR_ERR(dobj->sgt);
 		dobj->sgt = NULL;
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
index 42abee9a0f4f..ee3333f346b7 100644
--- a/drivers/gpu/drm/drm_gem_cma_helper.c
+++ b/drivers/gpu/drm/drm_gem_cma_helper.c
@@ -232,7 +232,7 @@ void drm_gem_cma_free(struct drm_gem_cma_object *cma_obj)
 
 	if (gem_obj->import_attach) {
 		if (cma_obj->vaddr)
-			dma_buf_vunmap(gem_obj->import_attach->dmabuf, &map);
+			dma_buf_vunmap_unlocked(gem_obj->import_attach->dmabuf, &map);
 		drm_prime_gem_destroy(gem_obj, cma_obj->sgt);
 	} else if (cma_obj->vaddr) {
 		if (cma_obj->map_noncoherent)
@@ -581,7 +581,7 @@ drm_gem_cma_prime_import_sg_table_vmap(struct drm_device *dev,
 	struct iosys_map map;
 	int ret;
 
-	ret = dma_buf_vmap(attach->dmabuf, &map);
+	ret = dma_buf_vmap_unlocked(attach->dmabuf, &map);
 	if (ret) {
 		DRM_ERROR("Failed to vmap PRIME buffer\n");
 		return ERR_PTR(ret);
@@ -589,7 +589,7 @@ drm_gem_cma_prime_import_sg_table_vmap(struct drm_device *dev,
 
 	obj = drm_gem_cma_prime_import_sg_table(dev, attach, sgt);
 	if (IS_ERR(obj)) {
-		dma_buf_vunmap(attach->dmabuf, &map);
+		dma_buf_vunmap_unlocked(attach->dmabuf, &map);
 		return obj;
 	}
 
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 8ad0e02991ca..c5e7a84ead06 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -299,7 +299,7 @@ static int drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem,
 	}
 
 	if (obj->import_attach) {
-		ret = dma_buf_vmap(obj->import_attach->dmabuf, map);
+		ret = dma_buf_vmap_unlocked(obj->import_attach->dmabuf, map);
 		if (!ret) {
 			if (WARN_ON(map->is_iomem)) {
 				ret = -EIO;
@@ -382,7 +382,7 @@ static void drm_gem_shmem_vunmap_locked(struct drm_gem_shmem_object *shmem,
 		return;
 
 	if (obj->import_attach) {
-		dma_buf_vunmap(obj->import_attach->dmabuf, map);
+		dma_buf_vunmap_unlocked(obj->import_attach->dmabuf, map);
 	} else {
 		vunmap(shmem->vaddr);
 		drm_gem_shmem_put_pages(shmem);
@@ -617,7 +617,7 @@ int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct
 		drm_gem_object_put(obj);
 		vma->vm_private_data = NULL;
 
-		return dma_buf_mmap(obj->dma_buf, vma, 0);
+		return dma_buf_mmap_unlocked(obj->dma_buf, vma, 0);
 	}
 
 	ret = drm_gem_shmem_get_pages(shmem);
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index a3f180653b8b..b75ef1756873 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -930,13 +930,13 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev,
 	if (!dev->driver->gem_prime_import_sg_table)
 		return ERR_PTR(-EINVAL);
 
-	attach = dma_buf_attach(dma_buf, attach_dev);
+	attach = dma_buf_attach_unlocked(dma_buf, attach_dev);
 	if (IS_ERR(attach))
 		return ERR_CAST(attach);
 
 	get_dma_buf(dma_buf);
 
-	sgt = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL);
+	sgt = dma_buf_map_attachment_unlocked(attach, DMA_BIDIRECTIONAL);
 	if (IS_ERR(sgt)) {
 		ret = PTR_ERR(sgt);
 		goto fail_detach;
@@ -954,9 +954,9 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev,
 	return obj;
 
 fail_unmap:
-	dma_buf_unmap_attachment(attach, sgt, DMA_BIDIRECTIONAL);
+	dma_buf_unmap_attachment_unlocked(attach, sgt, DMA_BIDIRECTIONAL);
 fail_detach:
-	dma_buf_detach(dma_buf, attach);
+	dma_buf_detach_unlocked(dma_buf, attach);
 	dma_buf_put(dma_buf);
 
 	return ERR_PTR(ret);
@@ -1052,9 +1052,9 @@ void drm_prime_gem_destroy(struct drm_gem_object *obj, struct sg_table *sg)
 
 	attach = obj->import_attach;
 	if (sg)
-		dma_buf_unmap_attachment(attach, sg, DMA_BIDIRECTIONAL);
+		dma_buf_unmap_attachment_unlocked(attach, sg, DMA_BIDIRECTIONAL);
 	dma_buf = attach->dmabuf;
-	dma_buf_detach(attach->dmabuf, attach);
+	dma_buf_detach_unlocked(attach->dmabuf, attach);
 	/* remove the reference */
 	dma_buf_put(dma_buf);
 }
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
index 3fa2da149639..ae6c1eda0a72 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
@@ -65,7 +65,7 @@ static void etnaviv_gem_prime_release(struct etnaviv_gem_object *etnaviv_obj)
 	struct iosys_map map = IOSYS_MAP_INIT_VADDR(etnaviv_obj->vaddr);
 
 	if (etnaviv_obj->vaddr)
-		dma_buf_vunmap(etnaviv_obj->base.import_attach->dmabuf, &map);
+		dma_buf_vunmap_unlocked(etnaviv_obj->base.import_attach->dmabuf, &map);
 
 	/* Don't drop the pages for imported dmabuf, as they are not
 	 * ours, just free the array we allocated:
@@ -82,7 +82,7 @@ static void *etnaviv_gem_prime_vmap_impl(struct etnaviv_gem_object *etnaviv_obj)
 
 	lockdep_assert_held(&etnaviv_obj->lock);
 
-	ret = dma_buf_vmap(etnaviv_obj->base.import_attach->dmabuf, &map);
+	ret = dma_buf_vmap_unlocked(etnaviv_obj->base.import_attach->dmabuf, &map);
 	if (ret)
 		return NULL;
 	return map.vaddr;
@@ -91,7 +91,7 @@ static void *etnaviv_gem_prime_vmap_impl(struct etnaviv_gem_object *etnaviv_obj)
 static int etnaviv_gem_prime_mmap_obj(struct etnaviv_gem_object *etnaviv_obj,
 		struct vm_area_struct *vma)
 {
-	return dma_buf_mmap(etnaviv_obj->base.dma_buf, vma, 0);
+	return dma_buf_mmap_unlocked(etnaviv_obj->base.dma_buf, vma, 0);
 }
 
 static const struct etnaviv_gem_ops etnaviv_gem_prime_ops = {
diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
index 3e493f48e0d4..8e95a3c5caf8 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
@@ -366,7 +366,7 @@ static int exynos_drm_gem_mmap(struct drm_gem_object *obj, struct vm_area_struct
 	int ret;
 
 	if (obj->import_attach)
-		return dma_buf_mmap(obj->dma_buf, vma, 0);
+		return dma_buf_mmap_unlocked(obj->dma_buf, vma, 0);
 
 	vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
 
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
index f5062d0c6333..5ecea7df98b1 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
@@ -241,8 +241,8 @@ static int i915_gem_object_get_pages_dmabuf(struct drm_i915_gem_object *obj)
 
 	assert_object_held(obj);
 
-	pages = dma_buf_map_attachment(obj->base.import_attach,
-				       DMA_BIDIRECTIONAL);
+	pages = dma_buf_map_attachment_unlocked(obj->base.import_attach,
+						DMA_BIDIRECTIONAL);
 	if (IS_ERR(pages))
 		return PTR_ERR(pages);
 
@@ -270,8 +270,8 @@ static int i915_gem_object_get_pages_dmabuf(struct drm_i915_gem_object *obj)
 static void i915_gem_object_put_pages_dmabuf(struct drm_i915_gem_object *obj,
 					     struct sg_table *pages)
 {
-	dma_buf_unmap_attachment(obj->base.import_attach, pages,
-				 DMA_BIDIRECTIONAL);
+	dma_buf_unmap_attachment_unlocked(obj->base.import_attach, pages,
+					  DMA_BIDIRECTIONAL);
 }
 
 static const struct drm_i915_gem_object_ops i915_gem_object_dmabuf_ops = {
@@ -306,7 +306,7 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
 		return ERR_PTR(-E2BIG);
 
 	/* need to attach */
-	attach = dma_buf_attach(dma_buf, dev->dev);
+	attach = dma_buf_attach_unlocked(dma_buf, dev->dev);
 	if (IS_ERR(attach))
 		return ERR_CAST(attach);
 
@@ -337,7 +337,7 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
 	return &obj->base;
 
 fail_detach:
-	dma_buf_detach(dma_buf, attach);
+	dma_buf_detach_unlocked(dma_buf, attach);
 	dma_buf_put(dma_buf);
 
 	return ERR_PTR(ret);
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
index 62c61af77a42..6053af920a22 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
@@ -207,13 +207,13 @@ static int igt_dmabuf_import_same_driver(struct drm_i915_private *i915,
 	i915_gem_object_unlock(import_obj);
 
 	/* Now try a fake an importer */
-	import_attach = dma_buf_attach(dmabuf, obj->base.dev->dev);
+	import_attach = dma_buf_attach_unlocked(dmabuf, obj->base.dev->dev);
 	if (IS_ERR(import_attach)) {
 		err = PTR_ERR(import_attach);
 		goto out_import;
 	}
 
-	st = dma_buf_map_attachment(import_attach, DMA_BIDIRECTIONAL);
+	st = dma_buf_map_attachment_unlocked(import_attach, DMA_BIDIRECTIONAL);
 	if (IS_ERR(st)) {
 		err = PTR_ERR(st);
 		goto out_detach;
@@ -226,9 +226,9 @@ static int igt_dmabuf_import_same_driver(struct drm_i915_private *i915,
 		timeout = -ETIME;
 	}
 	err = timeout > 0 ? 0 : timeout;
-	dma_buf_unmap_attachment(import_attach, st, DMA_BIDIRECTIONAL);
+	dma_buf_unmap_attachment_unlocked(import_attach, st, DMA_BIDIRECTIONAL);
 out_detach:
-	dma_buf_detach(dmabuf, import_attach);
+	dma_buf_detach_unlocked(dmabuf, import_attach);
 out_import:
 	i915_gem_object_put(import_obj);
 out_dmabuf:
@@ -296,7 +296,7 @@ static int igt_dmabuf_import(void *arg)
 		goto out_obj;
 	}
 
-	err = dma_buf_vmap(dmabuf, &map);
+	err = dma_buf_vmap_unlocked(dmabuf, &map);
 	dma_map = err ? NULL : map.vaddr;
 	if (!dma_map) {
 		pr_err("dma_buf_vmap failed\n");
@@ -337,7 +337,7 @@ static int igt_dmabuf_import(void *arg)
 
 	err = 0;
 out_dma_map:
-	dma_buf_vunmap(dmabuf, &map);
+	dma_buf_vunmap_unlocked(dmabuf, &map);
 out_obj:
 	i915_gem_object_put(obj);
 out_dmabuf:
@@ -358,7 +358,7 @@ static int igt_dmabuf_import_ownership(void *arg)
 	if (IS_ERR(dmabuf))
 		return PTR_ERR(dmabuf);
 
-	err = dma_buf_vmap(dmabuf, &map);
+	err = dma_buf_vmap_unlocked(dmabuf, &map);
 	ptr = err ? NULL : map.vaddr;
 	if (!ptr) {
 		pr_err("dma_buf_vmap failed\n");
@@ -367,7 +367,7 @@ static int igt_dmabuf_import_ownership(void *arg)
 	}
 
 	memset(ptr, 0xc5, PAGE_SIZE);
-	dma_buf_vunmap(dmabuf, &map);
+	dma_buf_vunmap_unlocked(dmabuf, &map);
 
 	obj = to_intel_bo(i915_gem_prime_import(&i915->drm, dmabuf));
 	if (IS_ERR(obj)) {
@@ -418,7 +418,7 @@ static int igt_dmabuf_export_vmap(void *arg)
 	}
 	i915_gem_object_put(obj);
 
-	err = dma_buf_vmap(dmabuf, &map);
+	err = dma_buf_vmap_unlocked(dmabuf, &map);
 	ptr = err ? NULL : map.vaddr;
 	if (!ptr) {
 		pr_err("dma_buf_vmap failed\n");
@@ -435,7 +435,7 @@ static int igt_dmabuf_export_vmap(void *arg)
 	memset(ptr, 0xc5, dmabuf->size);
 
 	err = 0;
-	dma_buf_vunmap(dmabuf, &map);
+	dma_buf_vunmap_unlocked(dmabuf, &map);
 out:
 	dma_buf_put(dmabuf);
 	return err;
diff --git a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
index 393f82e26927..a725a91c2ff9 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
@@ -119,13 +119,13 @@ struct drm_gem_object *omap_gem_prime_import(struct drm_device *dev,
 		}
 	}
 
-	attach = dma_buf_attach(dma_buf, dev->dev);
+	attach = dma_buf_attach_unlocked(dma_buf, dev->dev);
 	if (IS_ERR(attach))
 		return ERR_CAST(attach);
 
 	get_dma_buf(dma_buf);
 
-	sgt = dma_buf_map_attachment(attach, DMA_TO_DEVICE);
+	sgt = dma_buf_map_attachment_unlocked(attach, DMA_TO_DEVICE);
 	if (IS_ERR(sgt)) {
 		ret = PTR_ERR(sgt);
 		goto fail_detach;
@@ -142,9 +142,9 @@ struct drm_gem_object *omap_gem_prime_import(struct drm_device *dev,
 	return obj;
 
 fail_unmap:
-	dma_buf_unmap_attachment(attach, sgt, DMA_TO_DEVICE);
+	dma_buf_unmap_attachment_unlocked(attach, sgt, DMA_TO_DEVICE);
 fail_detach:
-	dma_buf_detach(dma_buf, attach);
+	dma_buf_detach_unlocked(dma_buf, attach);
 	dma_buf_put(dma_buf);
 
 	return ERR_PTR(ret);
diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c
index 81991090adcc..bbfe196ff6f6 100644
--- a/drivers/gpu/drm/tegra/gem.c
+++ b/drivers/gpu/drm/tegra/gem.c
@@ -78,15 +78,15 @@ static struct host1x_bo_mapping *tegra_bo_pin(struct device *dev, struct host1x_
 	if (gem->import_attach) {
 		struct dma_buf *buf = gem->import_attach->dmabuf;
 
-		map->attach = dma_buf_attach(buf, dev);
+		map->attach = dma_buf_attach_unlocked(buf, dev);
 		if (IS_ERR(map->attach)) {
 			err = PTR_ERR(map->attach);
 			goto free;
 		}
 
-		map->sgt = dma_buf_map_attachment(map->attach, direction);
+		map->sgt = dma_buf_map_attachment_unlocked(map->attach, direction);
 		if (IS_ERR(map->sgt)) {
-			dma_buf_detach(buf, map->attach);
+			dma_buf_detach_unlocked(buf, map->attach);
 			err = PTR_ERR(map->sgt);
 			map->sgt = NULL;
 			goto free;
@@ -160,8 +160,9 @@ static struct host1x_bo_mapping *tegra_bo_pin(struct device *dev, struct host1x_
 static void tegra_bo_unpin(struct host1x_bo_mapping *map)
 {
 	if (map->attach) {
-		dma_buf_unmap_attachment(map->attach, map->sgt, map->direction);
-		dma_buf_detach(map->attach->dmabuf, map->attach);
+		dma_buf_unmap_attachment_unlocked(map->attach, map->sgt,
+						  map->direction);
+		dma_buf_detach_unlocked(map->attach->dmabuf, map->attach);
 	} else {
 		dma_unmap_sgtable(map->dev, map->sgt, map->direction, 0);
 		sg_free_table(map->sgt);
@@ -181,7 +182,7 @@ static void *tegra_bo_mmap(struct host1x_bo *bo)
 	if (obj->vaddr) {
 		return obj->vaddr;
 	} else if (obj->gem.import_attach) {
-		ret = dma_buf_vmap(obj->gem.import_attach->dmabuf, &map);
+		ret = dma_buf_vmap_unlocked(obj->gem.import_attach->dmabuf, &map);
 		return ret ? NULL : map.vaddr;
 	} else {
 		return vmap(obj->pages, obj->num_pages, VM_MAP,
@@ -197,7 +198,7 @@ static void tegra_bo_munmap(struct host1x_bo *bo, void *addr)
 	if (obj->vaddr)
 		return;
 	else if (obj->gem.import_attach)
-		dma_buf_vunmap(obj->gem.import_attach->dmabuf, &map);
+		dma_buf_vunmap_unlocked(obj->gem.import_attach->dmabuf, &map);
 	else
 		vunmap(addr);
 }
@@ -453,7 +454,7 @@ static struct tegra_bo *tegra_bo_import(struct drm_device *drm,
 	if (IS_ERR(bo))
 		return bo;
 
-	attach = dma_buf_attach(buf, drm->dev);
+	attach = dma_buf_attach_unlocked(buf, drm->dev);
 	if (IS_ERR(attach)) {
 		err = PTR_ERR(attach);
 		goto free;
@@ -461,7 +462,7 @@ static struct tegra_bo *tegra_bo_import(struct drm_device *drm,
 
 	get_dma_buf(buf);
 
-	bo->sgt = dma_buf_map_attachment(attach, DMA_TO_DEVICE);
+	bo->sgt = dma_buf_map_attachment_unlocked(attach, DMA_TO_DEVICE);
 	if (IS_ERR(bo->sgt)) {
 		err = PTR_ERR(bo->sgt);
 		goto detach;
@@ -479,9 +480,9 @@ static struct tegra_bo *tegra_bo_import(struct drm_device *drm,
 
 detach:
 	if (!IS_ERR_OR_NULL(bo->sgt))
-		dma_buf_unmap_attachment(attach, bo->sgt, DMA_TO_DEVICE);
+		dma_buf_unmap_attachment_unlocked(attach, bo->sgt, DMA_TO_DEVICE);
 
-	dma_buf_detach(buf, attach);
+	dma_buf_detach_unlocked(buf, attach);
 	dma_buf_put(buf);
 free:
 	drm_gem_object_release(&bo->gem);
@@ -508,8 +509,8 @@ void tegra_bo_free_object(struct drm_gem_object *gem)
 		tegra_bo_iommu_unmap(tegra, bo);
 
 	if (gem->import_attach) {
-		dma_buf_unmap_attachment(gem->import_attach, bo->sgt,
-					 DMA_TO_DEVICE);
+		dma_buf_unmap_attachment_unlocked(gem->import_attach, bo->sgt,
+						  DMA_TO_DEVICE);
 		drm_prime_gem_destroy(gem, NULL);
 	} else {
 		tegra_bo_free(gem->dev, bo);
diff --git a/drivers/infiniband/core/umem_dmabuf.c b/drivers/infiniband/core/umem_dmabuf.c
index fce80a4a5147..12b73e6ad054 100644
--- a/drivers/infiniband/core/umem_dmabuf.c
+++ b/drivers/infiniband/core/umem_dmabuf.c
@@ -25,7 +25,8 @@ int ib_umem_dmabuf_map_pages(struct ib_umem_dmabuf *umem_dmabuf)
 	if (umem_dmabuf->sgt)
 		goto wait_fence;
 
-	sgt = dma_buf_map_attachment(umem_dmabuf->attach, DMA_BIDIRECTIONAL);
+	sgt = dma_buf_map_attachment_unlocked(umem_dmabuf->attach,
+					      DMA_BIDIRECTIONAL);
 	if (IS_ERR(sgt))
 		return PTR_ERR(sgt);
 
@@ -96,8 +97,8 @@ void ib_umem_dmabuf_unmap_pages(struct ib_umem_dmabuf *umem_dmabuf)
 		umem_dmabuf->last_sg_trim = 0;
 	}
 
-	dma_buf_unmap_attachment(umem_dmabuf->attach, umem_dmabuf->sgt,
-				 DMA_BIDIRECTIONAL);
+	dma_buf_unmap_attachment_unlocked(umem_dmabuf->attach, umem_dmabuf->sgt,
+					  DMA_BIDIRECTIONAL);
 
 	umem_dmabuf->sgt = NULL;
 }
@@ -143,7 +144,7 @@ struct ib_umem_dmabuf *ib_umem_dmabuf_get(struct ib_device *device,
 	if (!ib_umem_num_pages(umem))
 		goto out_free_umem;
 
-	umem_dmabuf->attach = dma_buf_dynamic_attach(
+	umem_dmabuf->attach = dma_buf_dynamic_attach_unlocked(
 					dmabuf,
 					device->dma_device,
 					ops,
@@ -222,7 +223,7 @@ void ib_umem_dmabuf_release(struct ib_umem_dmabuf *umem_dmabuf)
 		dma_buf_unpin(umem_dmabuf->attach);
 	dma_resv_unlock(dmabuf->resv);
 
-	dma_buf_detach(dmabuf, umem_dmabuf->attach);
+	dma_buf_detach_unlocked(dmabuf, umem_dmabuf->attach);
 	dma_buf_put(dmabuf);
 	kfree(umem_dmabuf);
 }
diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
index 678b359717c4..de762dbdaf78 100644
--- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
@@ -101,7 +101,7 @@ static void *vb2_dc_vaddr(struct vb2_buffer *vb, void *buf_priv)
 	if (buf->db_attach) {
 		struct iosys_map map;
 
-		if (!dma_buf_vmap(buf->db_attach->dmabuf, &map))
+		if (!dma_buf_vmap_unlocked(buf->db_attach->dmabuf, &map))
 			buf->vaddr = map.vaddr;
 
 		return buf->vaddr;
@@ -711,7 +711,7 @@ static int vb2_dc_map_dmabuf(void *mem_priv)
 	}
 
 	/* get the associated scatterlist for this buffer */
-	sgt = dma_buf_map_attachment(buf->db_attach, buf->dma_dir);
+	sgt = dma_buf_map_attachment_unlocked(buf->db_attach, buf->dma_dir);
 	if (IS_ERR(sgt)) {
 		pr_err("Error getting dmabuf scatterlist\n");
 		return -EINVAL;
@@ -722,7 +722,8 @@ static int vb2_dc_map_dmabuf(void *mem_priv)
 	if (contig_size < buf->size) {
 		pr_err("contiguous chunk is too small %lu/%lu\n",
 		       contig_size, buf->size);
-		dma_buf_unmap_attachment(buf->db_attach, sgt, buf->dma_dir);
+		dma_buf_unmap_attachment_unlocked(buf->db_attach, sgt,
+						  buf->dma_dir);
 		return -EFAULT;
 	}
 
@@ -750,10 +751,10 @@ static void vb2_dc_unmap_dmabuf(void *mem_priv)
 	}
 
 	if (buf->vaddr) {
-		dma_buf_vunmap(buf->db_attach->dmabuf, &map);
+		dma_buf_vunmap_unlocked(buf->db_attach->dmabuf, &map);
 		buf->vaddr = NULL;
 	}
-	dma_buf_unmap_attachment(buf->db_attach, sgt, buf->dma_dir);
+	dma_buf_unmap_attachment_unlocked(buf->db_attach, sgt, buf->dma_dir);
 
 	buf->dma_addr = 0;
 	buf->dma_sgt = NULL;
@@ -768,7 +769,7 @@ static void vb2_dc_detach_dmabuf(void *mem_priv)
 		vb2_dc_unmap_dmabuf(buf);
 
 	/* detach this attachment */
-	dma_buf_detach(buf->db_attach->dmabuf, buf->db_attach);
+	dma_buf_detach_unlocked(buf->db_attach->dmabuf, buf->db_attach);
 	kfree(buf);
 }
 
@@ -792,7 +793,7 @@ static void *vb2_dc_attach_dmabuf(struct vb2_buffer *vb, struct device *dev,
 	buf->vb = vb;
 
 	/* create attachment for the dmabuf with the user device */
-	dba = dma_buf_attach(dbuf, buf->dev);
+	dba = dma_buf_attach_unlocked(dbuf, buf->dev);
 	if (IS_ERR(dba)) {
 		pr_err("failed to attach dmabuf\n");
 		kfree(buf);
diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
index fa69158a65b1..39e11600304a 100644
--- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
@@ -309,7 +309,7 @@ static void *vb2_dma_sg_vaddr(struct vb2_buffer *vb, void *buf_priv)
 
 	if (!buf->vaddr) {
 		if (buf->db_attach) {
-			ret = dma_buf_vmap(buf->db_attach->dmabuf, &map);
+			ret = dma_buf_vmap_unlocked(buf->db_attach->dmabuf, &map);
 			buf->vaddr = ret ? NULL : map.vaddr;
 		} else {
 			buf->vaddr = vm_map_ram(buf->pages, buf->num_pages, -1);
@@ -565,7 +565,7 @@ static int vb2_dma_sg_map_dmabuf(void *mem_priv)
 	}
 
 	/* get the associated scatterlist for this buffer */
-	sgt = dma_buf_map_attachment(buf->db_attach, buf->dma_dir);
+	sgt = dma_buf_map_attachment_unlocked(buf->db_attach, buf->dma_dir);
 	if (IS_ERR(sgt)) {
 		pr_err("Error getting dmabuf scatterlist\n");
 		return -EINVAL;
@@ -594,10 +594,10 @@ static void vb2_dma_sg_unmap_dmabuf(void *mem_priv)
 	}
 
 	if (buf->vaddr) {
-		dma_buf_vunmap(buf->db_attach->dmabuf, &map);
+		dma_buf_vunmap_unlocked(buf->db_attach->dmabuf, &map);
 		buf->vaddr = NULL;
 	}
-	dma_buf_unmap_attachment(buf->db_attach, sgt, buf->dma_dir);
+	dma_buf_unmap_attachment_unlocked(buf->db_attach, sgt, buf->dma_dir);
 
 	buf->dma_sgt = NULL;
 }
@@ -611,7 +611,7 @@ static void vb2_dma_sg_detach_dmabuf(void *mem_priv)
 		vb2_dma_sg_unmap_dmabuf(buf);
 
 	/* detach this attachment */
-	dma_buf_detach(buf->db_attach->dmabuf, buf->db_attach);
+	dma_buf_detach_unlocked(buf->db_attach->dmabuf, buf->db_attach);
 	kfree(buf);
 }
 
@@ -633,7 +633,7 @@ static void *vb2_dma_sg_attach_dmabuf(struct vb2_buffer *vb, struct device *dev,
 
 	buf->dev = dev;
 	/* create attachment for the dmabuf with the user device */
-	dba = dma_buf_attach(dbuf, buf->dev);
+	dba = dma_buf_attach_unlocked(dbuf, buf->dev);
 	if (IS_ERR(dba)) {
 		pr_err("failed to attach dmabuf\n");
 		kfree(buf);
diff --git a/drivers/media/common/videobuf2/videobuf2-vmalloc.c b/drivers/media/common/videobuf2/videobuf2-vmalloc.c
index 948152f1596b..7831bf545874 100644
--- a/drivers/media/common/videobuf2/videobuf2-vmalloc.c
+++ b/drivers/media/common/videobuf2/videobuf2-vmalloc.c
@@ -376,7 +376,7 @@ static int vb2_vmalloc_map_dmabuf(void *mem_priv)
 	struct iosys_map map;
 	int ret;
 
-	ret = dma_buf_vmap(buf->dbuf, &map);
+	ret = dma_buf_vmap_unlocked(buf->dbuf, &map);
 	if (ret)
 		return -EFAULT;
 	buf->vaddr = map.vaddr;
@@ -389,7 +389,7 @@ static void vb2_vmalloc_unmap_dmabuf(void *mem_priv)
 	struct vb2_vmalloc_buf *buf = mem_priv;
 	struct iosys_map map = IOSYS_MAP_INIT_VADDR(buf->vaddr);
 
-	dma_buf_vunmap(buf->dbuf, &map);
+	dma_buf_vunmap_unlocked(buf->dbuf, &map);
 	buf->vaddr = NULL;
 }
 
@@ -399,7 +399,7 @@ static void vb2_vmalloc_detach_dmabuf(void *mem_priv)
 	struct iosys_map map = IOSYS_MAP_INIT_VADDR(buf->vaddr);
 
 	if (buf->vaddr)
-		dma_buf_vunmap(buf->dbuf, &map);
+		dma_buf_vunmap_unlocked(buf->dbuf, &map);
 
 	kfree(buf);
 }
diff --git a/drivers/media/platform/nvidia/tegra-vde/dmabuf-cache.c b/drivers/media/platform/nvidia/tegra-vde/dmabuf-cache.c
index 69c346148070..58e4595f3a10 100644
--- a/drivers/media/platform/nvidia/tegra-vde/dmabuf-cache.c
+++ b/drivers/media/platform/nvidia/tegra-vde/dmabuf-cache.c
@@ -38,8 +38,8 @@ static void tegra_vde_release_entry(struct tegra_vde_cache_entry *entry)
 	if (entry->vde->domain)
 		tegra_vde_iommu_unmap(entry->vde, entry->iova);
 
-	dma_buf_unmap_attachment(entry->a, entry->sgt, entry->dma_dir);
-	dma_buf_detach(dmabuf, entry->a);
+	dma_buf_unmap_attachment_unlocked(entry->a, entry->sgt, entry->dma_dir);
+	dma_buf_detach_unlocked(dmabuf, entry->a);
 	dma_buf_put(dmabuf);
 
 	list_del(&entry->list);
@@ -95,14 +95,14 @@ int tegra_vde_dmabuf_cache_map(struct tegra_vde *vde,
 		goto ref;
 	}
 
-	attachment = dma_buf_attach(dmabuf, dev);
+	attachment = dma_buf_attach_unlocked(dmabuf, dev);
 	if (IS_ERR(attachment)) {
 		dev_err(dev, "Failed to attach dmabuf\n");
 		err = PTR_ERR(attachment);
 		goto err_unlock;
 	}
 
-	sgt = dma_buf_map_attachment(attachment, dma_dir);
+	sgt = dma_buf_map_attachment_unlocked(attachment, dma_dir);
 	if (IS_ERR(sgt)) {
 		dev_err(dev, "Failed to get dmabufs sg_table\n");
 		err = PTR_ERR(sgt);
@@ -152,9 +152,9 @@ int tegra_vde_dmabuf_cache_map(struct tegra_vde *vde,
 err_free:
 	kfree(entry);
 err_unmap:
-	dma_buf_unmap_attachment(attachment, sgt, dma_dir);
+	dma_buf_unmap_attachment_unlocked(attachment, sgt, dma_dir);
 err_detach:
-	dma_buf_detach(dmabuf, attachment);
+	dma_buf_detach_unlocked(dmabuf, attachment);
 err_unlock:
 	mutex_unlock(&vde->map_lock);
 
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 93ebd174d848..558e8056eb80 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -310,9 +310,9 @@ static void fastrpc_free_map(struct kref *ref)
 				return;
 			}
 		}
-		dma_buf_unmap_attachment(map->attach, map->table,
-					 DMA_BIDIRECTIONAL);
-		dma_buf_detach(map->buf, map->attach);
+		dma_buf_unmap_attachment_unlocked(map->attach, map->table,
+						  DMA_BIDIRECTIONAL);
+		dma_buf_detach_unlocked(map->buf, map->attach);
 		dma_buf_put(map->buf);
 	}
 
@@ -719,14 +719,14 @@ static int fastrpc_map_create(struct fastrpc_user *fl, int fd,
 		goto get_err;
 	}
 
-	map->attach = dma_buf_attach(map->buf, sess->dev);
+	map->attach = dma_buf_attach_unlocked(map->buf, sess->dev);
 	if (IS_ERR(map->attach)) {
 		dev_err(sess->dev, "Failed to attach dmabuf\n");
 		err = PTR_ERR(map->attach);
 		goto attach_err;
 	}
 
-	map->table = dma_buf_map_attachment(map->attach, DMA_BIDIRECTIONAL);
+	map->table = dma_buf_map_attachment_unlocked(map->attach, DMA_BIDIRECTIONAL);
 	if (IS_ERR(map->table)) {
 		err = PTR_ERR(map->table);
 		goto map_err;
@@ -763,7 +763,7 @@ static int fastrpc_map_create(struct fastrpc_user *fl, int fd,
 	return 0;
 
 map_err:
-	dma_buf_detach(map->buf, map->attach);
+	dma_buf_detach_unlocked(map->buf, map->attach);
 attach_err:
 	dma_buf_put(map->buf);
 get_err:
diff --git a/drivers/xen/gntdev-dmabuf.c b/drivers/xen/gntdev-dmabuf.c
index 940e5e9e8a54..5a50e2697e95 100644
--- a/drivers/xen/gntdev-dmabuf.c
+++ b/drivers/xen/gntdev-dmabuf.c
@@ -592,7 +592,7 @@ dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv, struct device *dev,
 	gntdev_dmabuf->priv = priv;
 	gntdev_dmabuf->fd = fd;
 
-	attach = dma_buf_attach(dma_buf, dev);
+	attach = dma_buf_attach_unlocked(dma_buf, dev);
 	if (IS_ERR(attach)) {
 		ret = ERR_CAST(attach);
 		goto fail_free_obj;
@@ -600,7 +600,7 @@ dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv, struct device *dev,
 
 	gntdev_dmabuf->u.imp.attach = attach;
 
-	sgt = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL);
+	sgt = dma_buf_map_attachment_unlocked(attach, DMA_BIDIRECTIONAL);
 	if (IS_ERR(sgt)) {
 		ret = ERR_CAST(sgt);
 		goto fail_detach;
@@ -658,9 +658,9 @@ dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv, struct device *dev,
 fail_end_access:
 	dmabuf_imp_end_foreign_access(gntdev_dmabuf->u.imp.refs, count);
 fail_unmap:
-	dma_buf_unmap_attachment(attach, sgt, DMA_BIDIRECTIONAL);
+	dma_buf_unmap_attachment_unlocked(attach, sgt, DMA_BIDIRECTIONAL);
 fail_detach:
-	dma_buf_detach(dma_buf, attach);
+	dma_buf_detach_unlocked(dma_buf, attach);
 fail_free_obj:
 	dmabuf_imp_free_storage(gntdev_dmabuf);
 fail_put:
@@ -708,10 +708,10 @@ static int dmabuf_imp_release(struct gntdev_dmabuf_priv *priv, u32 fd)
 	attach = gntdev_dmabuf->u.imp.attach;
 
 	if (gntdev_dmabuf->u.imp.sgt)
-		dma_buf_unmap_attachment(attach, gntdev_dmabuf->u.imp.sgt,
-					 DMA_BIDIRECTIONAL);
+		dma_buf_unmap_attachment_unlocked(attach, gntdev_dmabuf->u.imp.sgt,
+						  DMA_BIDIRECTIONAL);
 	dma_buf = attach->dmabuf;
-	dma_buf_detach(attach->dmabuf, attach);
+	dma_buf_detach_unlocked(attach->dmabuf, attach);
 	dma_buf_put(dma_buf);
 
 	dmabuf_imp_free_storage(gntdev_dmabuf);
diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index 71731796c8c3..9ab09569dec1 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -601,14 +601,16 @@ dma_buf_attachment_is_dynamic(struct dma_buf_attachment *attach)
 	return !!attach->importer_ops;
 }
 
-struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
-					  struct device *dev);
+struct dma_buf_attachment *dma_buf_attach_unlocked(struct dma_buf *dmabuf,
+						   struct device *dev);
 struct dma_buf_attachment *
-dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev,
-		       const struct dma_buf_attach_ops *importer_ops,
-		       void *importer_priv);
-void dma_buf_detach(struct dma_buf *dmabuf,
-		    struct dma_buf_attachment *attach);
+dma_buf_dynamic_attach_unlocked(struct dma_buf *dmabuf, struct device *dev,
+				const struct dma_buf_attach_ops *importer_ops,
+				void *importer_priv);
+
+void dma_buf_detach_unlocked(struct dma_buf *dmabuf,
+			     struct dma_buf_attachment *attach);
+
 int dma_buf_pin(struct dma_buf_attachment *attach);
 void dma_buf_unpin(struct dma_buf_attachment *attach);
 
@@ -618,18 +620,20 @@ int dma_buf_fd(struct dma_buf *dmabuf, int flags);
 struct dma_buf *dma_buf_get(int fd);
 void dma_buf_put(struct dma_buf *dmabuf);
 
-struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *,
-					enum dma_data_direction);
-void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct sg_table *,
-				enum dma_data_direction);
+struct sg_table *dma_buf_map_attachment_unlocked(struct dma_buf_attachment *,
+						 enum dma_data_direction);
+void dma_buf_unmap_attachment_unlocked(struct dma_buf_attachment *,
+				       struct sg_table *,
+				       enum dma_data_direction);
+
 void dma_buf_move_notify(struct dma_buf *dma_buf);
 int dma_buf_begin_cpu_access(struct dma_buf *dma_buf,
 			     enum dma_data_direction dir);
 int dma_buf_end_cpu_access(struct dma_buf *dma_buf,
 			   enum dma_data_direction dir);
 
-int dma_buf_mmap(struct dma_buf *, struct vm_area_struct *,
-		 unsigned long);
-int dma_buf_vmap(struct dma_buf *dmabuf, struct iosys_map *map);
-void dma_buf_vunmap(struct dma_buf *dmabuf, struct iosys_map *map);
+int dma_buf_mmap_unlocked(struct dma_buf *, struct vm_area_struct *,
+			  unsigned long);
+int dma_buf_vmap_unlocked(struct dma_buf *dmabuf, struct iosys_map *map);
+void dma_buf_vunmap_unlocked(struct dma_buf *dmabuf, struct iosys_map *map);
 #endif /* __DMA_BUF_H__ */
-- 
2.36.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v1 2/6] drm/gem: Take reservation lock for vmap/vunmap operations
  2022-07-15  0:52 [PATCH v1 0/6] Move all drivers to a common dma-buf locking convention Dmitry Osipenko
  2022-07-15  0:52 ` [PATCH v1 1/6] dma-buf: Add _unlocked postfix to function names Dmitry Osipenko
@ 2022-07-15  0:52 ` Dmitry Osipenko
  2022-07-15  7:31   ` Christian König
  2022-07-15  0:52 ` [PATCH v1 3/6] dma-buf: Move all dma-bufs to dynamic locking specification Dmitry Osipenko
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 18+ messages in thread
From: Dmitry Osipenko @ 2022-07-15  0:52 UTC (permalink / raw)
  To: David Airlie, Gerd Hoffmann, Gurchetan Singh, Chia-I Wu,
	Daniel Vetter, Daniel Almeida, Gert Wollny, Gustavo Padovan,
	Daniel Stone, Tomeu Vizoso, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, Rob Clark, Sumit Semwal, Christian König,
	Pan, Xinhui, Thierry Reding, Tomasz Figa, Marek Szyprowski,
	Mauro Carvalho Chehab, Alex Deucher, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin,
	Thomas Hellström
  Cc: dri-devel, linux-kernel, Dmitry Osipenko, linux-media,
	linaro-mm-sig, amd-gfx, intel-gfx, kernel, virtualization,
	spice-devel, linux-rdma, linux-arm-msm

The new common dma-buf locking convention will require buffer importers
to hold the reservation lock around mapping operations. Make DRM GEM core
to take the lock around the vmapping operations and update QXL and i915
drivers to use the locked functions for the case where DRM core now holds
the lock. This patch prepares DRM core and drivers to transition to the
common dma-buf locking convention where vmapping of exported GEMs will
be done under the held reservation lock.

Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 drivers/gpu/drm/drm_client.c                 |  4 +--
 drivers/gpu/drm/drm_gem.c                    | 28 ++++++++++++++++++++
 drivers/gpu/drm/drm_gem_framebuffer_helper.c |  6 ++---
 drivers/gpu/drm/drm_prime.c                  |  4 +--
 drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c   |  2 +-
 drivers/gpu/drm/qxl/qxl_object.c             | 17 ++++++------
 drivers/gpu/drm/qxl/qxl_prime.c              |  4 +--
 include/drm/drm_gem.h                        |  3 +++
 8 files changed, 50 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
index 2b230b4d6942..fbcb1e995384 100644
--- a/drivers/gpu/drm/drm_client.c
+++ b/drivers/gpu/drm/drm_client.c
@@ -323,7 +323,7 @@ drm_client_buffer_vmap(struct drm_client_buffer *buffer,
 	 * fd_install step out of the driver backend hooks, to make that
 	 * final step optional for internal users.
 	 */
-	ret = drm_gem_vmap(buffer->gem, map);
+	ret = drm_gem_vmap_unlocked(buffer->gem, map);
 	if (ret)
 		return ret;
 
@@ -345,7 +345,7 @@ void drm_client_buffer_vunmap(struct drm_client_buffer *buffer)
 {
 	struct iosys_map *map = &buffer->map;
 
-	drm_gem_vunmap(buffer->gem, map);
+	drm_gem_vunmap_unlocked(buffer->gem, map);
 }
 EXPORT_SYMBOL(drm_client_buffer_vunmap);
 
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index eb0c2d041f13..9769c33cad99 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1155,6 +1155,8 @@ void drm_gem_print_info(struct drm_printer *p, unsigned int indent,
 
 int drm_gem_pin(struct drm_gem_object *obj)
 {
+	dma_resv_assert_held(obj->resv);
+
 	if (obj->funcs->pin)
 		return obj->funcs->pin(obj);
 	else
@@ -1163,6 +1165,8 @@ int drm_gem_pin(struct drm_gem_object *obj)
 
 void drm_gem_unpin(struct drm_gem_object *obj)
 {
+	dma_resv_assert_held(obj->resv);
+
 	if (obj->funcs->unpin)
 		obj->funcs->unpin(obj);
 }
@@ -1171,6 +1175,8 @@ int drm_gem_vmap(struct drm_gem_object *obj, struct iosys_map *map)
 {
 	int ret;
 
+	dma_resv_assert_held(obj->resv);
+
 	if (!obj->funcs->vmap)
 		return -EOPNOTSUPP;
 
@@ -1186,6 +1192,8 @@ EXPORT_SYMBOL(drm_gem_vmap);
 
 void drm_gem_vunmap(struct drm_gem_object *obj, struct iosys_map *map)
 {
+	dma_resv_assert_held(obj->resv);
+
 	if (iosys_map_is_null(map))
 		return;
 
@@ -1197,6 +1205,26 @@ void drm_gem_vunmap(struct drm_gem_object *obj, struct iosys_map *map)
 }
 EXPORT_SYMBOL(drm_gem_vunmap);
 
+int drm_gem_vmap_unlocked(struct drm_gem_object *obj, struct iosys_map *map)
+{
+	int ret;
+
+	dma_resv_lock(obj->resv, NULL);
+	ret = drm_gem_vmap(obj, map);
+	dma_resv_unlock(obj->resv);
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_gem_vmap_unlocked);
+
+void drm_gem_vunmap_unlocked(struct drm_gem_object *obj, struct iosys_map *map)
+{
+	dma_resv_lock(obj->resv, NULL);
+	drm_gem_vunmap(obj, map);
+	dma_resv_unlock(obj->resv);
+}
+EXPORT_SYMBOL(drm_gem_vunmap_unlocked);
+
 /**
  * drm_gem_lock_reservations - Sets up the ww context and acquires
  * the lock on an array of GEM objects.
diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
index 880a4975507f..e35e224e6303 100644
--- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
+++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
@@ -354,7 +354,7 @@ int drm_gem_fb_vmap(struct drm_framebuffer *fb, struct iosys_map *map,
 			ret = -EINVAL;
 			goto err_drm_gem_vunmap;
 		}
-		ret = drm_gem_vmap(obj, &map[i]);
+		ret = drm_gem_vmap_unlocked(obj, &map[i]);
 		if (ret)
 			goto err_drm_gem_vunmap;
 	}
@@ -376,7 +376,7 @@ int drm_gem_fb_vmap(struct drm_framebuffer *fb, struct iosys_map *map,
 		obj = drm_gem_fb_get_obj(fb, i);
 		if (!obj)
 			continue;
-		drm_gem_vunmap(obj, &map[i]);
+		drm_gem_vunmap_unlocked(obj, &map[i]);
 	}
 	return ret;
 }
@@ -403,7 +403,7 @@ void drm_gem_fb_vunmap(struct drm_framebuffer *fb, struct iosys_map *map)
 			continue;
 		if (iosys_map_is_null(&map[i]))
 			continue;
-		drm_gem_vunmap(obj, &map[i]);
+		drm_gem_vunmap_unlocked(obj, &map[i]);
 	}
 }
 EXPORT_SYMBOL(drm_gem_fb_vunmap);
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index b75ef1756873..1bd234fd21a5 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -678,7 +678,7 @@ int drm_gem_dmabuf_vmap(struct dma_buf *dma_buf, struct iosys_map *map)
 {
 	struct drm_gem_object *obj = dma_buf->priv;
 
-	return drm_gem_vmap(obj, map);
+	return drm_gem_vmap_unlocked(obj, map);
 }
 EXPORT_SYMBOL(drm_gem_dmabuf_vmap);
 
@@ -694,7 +694,7 @@ void drm_gem_dmabuf_vunmap(struct dma_buf *dma_buf, struct iosys_map *map)
 {
 	struct drm_gem_object *obj = dma_buf->priv;
 
-	drm_gem_vunmap(obj, map);
+	drm_gem_vunmap_unlocked(obj, map);
 }
 EXPORT_SYMBOL(drm_gem_dmabuf_vunmap);
 
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
index 5ecea7df98b1..cc54a5b1d6ae 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
@@ -72,7 +72,7 @@ static int i915_gem_dmabuf_vmap(struct dma_buf *dma_buf,
 	struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf);
 	void *vaddr;
 
-	vaddr = i915_gem_object_pin_map_unlocked(obj, I915_MAP_WB);
+	vaddr = i915_gem_object_pin_map(obj, I915_MAP_WB);
 	if (IS_ERR(vaddr))
 		return PTR_ERR(vaddr);
 
diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
index 695d9308d1f0..06a58dad5f5c 100644
--- a/drivers/gpu/drm/qxl/qxl_object.c
+++ b/drivers/gpu/drm/qxl/qxl_object.c
@@ -168,9 +168,16 @@ int qxl_bo_vmap_locked(struct qxl_bo *bo, struct iosys_map *map)
 		bo->map_count++;
 		goto out;
 	}
-	r = ttm_bo_vmap(&bo->tbo, &bo->map);
+
+	r = __qxl_bo_pin(bo);
 	if (r)
 		return r;
+
+	r = ttm_bo_vmap(&bo->tbo, &bo->map);
+	if (r) {
+		__qxl_bo_unpin(bo);
+		return r;
+	}
 	bo->map_count = 1;
 
 	/* TODO: Remove kptr in favor of map everywhere. */
@@ -192,12 +199,6 @@ int qxl_bo_vmap(struct qxl_bo *bo, struct iosys_map *map)
 	if (r)
 		return r;
 
-	r = __qxl_bo_pin(bo);
-	if (r) {
-		qxl_bo_unreserve(bo);
-		return r;
-	}
-
 	r = qxl_bo_vmap_locked(bo, map);
 	qxl_bo_unreserve(bo);
 	return r;
@@ -247,6 +248,7 @@ void qxl_bo_vunmap_locked(struct qxl_bo *bo)
 		return;
 	bo->kptr = NULL;
 	ttm_bo_vunmap(&bo->tbo, &bo->map);
+	__qxl_bo_unpin(bo);
 }
 
 int qxl_bo_vunmap(struct qxl_bo *bo)
@@ -258,7 +260,6 @@ int qxl_bo_vunmap(struct qxl_bo *bo)
 		return r;
 
 	qxl_bo_vunmap_locked(bo);
-	__qxl_bo_unpin(bo);
 	qxl_bo_unreserve(bo);
 	return 0;
 }
diff --git a/drivers/gpu/drm/qxl/qxl_prime.c b/drivers/gpu/drm/qxl/qxl_prime.c
index 142d01415acb..9169c26357d3 100644
--- a/drivers/gpu/drm/qxl/qxl_prime.c
+++ b/drivers/gpu/drm/qxl/qxl_prime.c
@@ -59,7 +59,7 @@ int qxl_gem_prime_vmap(struct drm_gem_object *obj, struct iosys_map *map)
 	struct qxl_bo *bo = gem_to_qxl_bo(obj);
 	int ret;
 
-	ret = qxl_bo_vmap(bo, map);
+	ret = qxl_bo_vmap_locked(bo, map);
 	if (ret < 0)
 		return ret;
 
@@ -71,5 +71,5 @@ void qxl_gem_prime_vunmap(struct drm_gem_object *obj,
 {
 	struct qxl_bo *bo = gem_to_qxl_bo(obj);
 
-	qxl_bo_vunmap(bo);
+	qxl_bo_vunmap_locked(bo);
 }
diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index 87cffc9efa85..bf3700415229 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -420,4 +420,7 @@ void drm_gem_unlock_reservations(struct drm_gem_object **objs, int count,
 int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
 			    u32 handle, u64 *offset);
 
+int drm_gem_vmap_unlocked(struct drm_gem_object *obj, struct iosys_map *map);
+void drm_gem_vunmap_unlocked(struct drm_gem_object *obj, struct iosys_map *map);
+
 #endif /* __DRM_GEM_H__ */
-- 
2.36.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v1 3/6] dma-buf: Move all dma-bufs to dynamic locking specification
  2022-07-15  0:52 [PATCH v1 0/6] Move all drivers to a common dma-buf locking convention Dmitry Osipenko
  2022-07-15  0:52 ` [PATCH v1 1/6] dma-buf: Add _unlocked postfix to function names Dmitry Osipenko
  2022-07-15  0:52 ` [PATCH v1 2/6] drm/gem: Take reservation lock for vmap/vunmap operations Dmitry Osipenko
@ 2022-07-15  0:52 ` Dmitry Osipenko
  2022-07-15  0:52 ` [PATCH v1 4/6] dma-buf: Acquire wait-wound context on attachment Dmitry Osipenko
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 18+ messages in thread
From: Dmitry Osipenko @ 2022-07-15  0:52 UTC (permalink / raw)
  To: David Airlie, Gerd Hoffmann, Gurchetan Singh, Chia-I Wu,
	Daniel Vetter, Daniel Almeida, Gert Wollny, Gustavo Padovan,
	Daniel Stone, Tomeu Vizoso, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, Rob Clark, Sumit Semwal, Christian König,
	Pan, Xinhui, Thierry Reding, Tomasz Figa, Marek Szyprowski,
	Mauro Carvalho Chehab, Alex Deucher, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin,
	Thomas Hellström
  Cc: dri-devel, linux-kernel, Dmitry Osipenko, linux-media,
	linaro-mm-sig, amd-gfx, intel-gfx, kernel, virtualization,
	spice-devel, linux-rdma, linux-arm-msm

This patch moves the non-dynamic dma-buf users over to the dynamic
locking specification. From now on all dma-buf importers are responsible
for holding dma-buf's reservation lock around operations performed over
dma-bufs. This strict locking convention prevents dead lock situation for
dma-buf importers and exporters.

Previously the "unlocked" versions of the dma-buf 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 imported dma-bufs under
the held lock, hence the "locked" variant of the functions are added
for them and the drivers are updated to use the "locked" versions.

Intel driver is also updated to not lock the exported buffer on
attachment since lock is now held by importer. We also need to move
the ww context acquirement from exporters (i915 driver) to importers,
otherwise lockdep won't be happy. This will be done in the next patch
since i915 is the only driver that uses ww context on attachment today
and it's not critical to make this change separately for i915 driver.

Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 drivers/dma-buf/dma-buf.c                  | 125 +++++++++++++++------
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c    |   4 +-
 drivers/gpu/drm/drm_prime.c                |   4 +-
 drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c |  12 +-
 include/linux/dma-buf.h                    |   6 +
 5 files changed, 104 insertions(+), 47 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index d16237a6ffaa..0ee588276534 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -841,14 +841,14 @@ dma_buf_dynamic_attach_unlocked(struct dma_buf *dmabuf, struct device *dev,
 	attach->importer_ops = importer_ops;
 	attach->importer_priv = importer_priv;
 
+	dma_resv_lock(dmabuf->resv, NULL);
+
 	if (dmabuf->ops->attach) {
 		ret = dmabuf->ops->attach(dmabuf, attach);
 		if (ret)
 			goto err_attach;
 	}
-	dma_resv_lock(dmabuf->resv, NULL);
 	list_add(&attach->node, &dmabuf->attachments);
-	dma_resv_unlock(dmabuf->resv);
 
 	/* When either the importer or the exporter can't handle dynamic
 	 * mappings we cache the mapping here to avoid issues with the
@@ -859,7 +859,6 @@ dma_buf_dynamic_attach_unlocked(struct dma_buf *dmabuf, struct device *dev,
 		struct sg_table *sgt;
 
 		if (dma_buf_is_dynamic(attach->dmabuf)) {
-			dma_resv_lock(attach->dmabuf->resv, NULL);
 			ret = dmabuf->ops->pin(attach);
 			if (ret)
 				goto err_unlock;
@@ -872,15 +871,16 @@ dma_buf_dynamic_attach_unlocked(struct dma_buf *dmabuf, struct device *dev,
 			ret = PTR_ERR(sgt);
 			goto err_unpin;
 		}
-		if (dma_buf_is_dynamic(attach->dmabuf))
-			dma_resv_unlock(attach->dmabuf->resv);
 		attach->sgt = sgt;
 		attach->dir = DMA_BIDIRECTIONAL;
 	}
 
+	dma_resv_unlock(dmabuf->resv);
+
 	return attach;
 
 err_attach:
+	dma_resv_unlock(attach->dmabuf->resv);
 	kfree(attach);
 	return ERR_PTR(ret);
 
@@ -889,8 +889,7 @@ dma_buf_dynamic_attach_unlocked(struct dma_buf *dmabuf, struct device *dev,
 		dmabuf->ops->unpin(attach);
 
 err_unlock:
-	if (dma_buf_is_dynamic(attach->dmabuf))
-		dma_resv_unlock(attach->dmabuf->resv);
+	dma_resv_unlock(dmabuf->resv);
 
 	dma_buf_detach_unlocked(dmabuf, attach);
 	return ERR_PTR(ret);
@@ -937,24 +936,23 @@ void dma_buf_detach_unlocked(struct dma_buf *dmabuf,
 	if (WARN_ON(!dmabuf || !attach))
 		return;
 
-	if (attach->sgt) {
-		if (dma_buf_is_dynamic(attach->dmabuf))
-			dma_resv_lock(attach->dmabuf->resv, NULL);
+	if (WARN_ON(dmabuf != attach->dmabuf))
+		return;
 
+	dma_resv_lock(dmabuf->resv, NULL);
+
+	if (attach->sgt) {
 		__unmap_dma_buf(attach, attach->sgt, attach->dir);
 
-		if (dma_buf_is_dynamic(attach->dmabuf)) {
+		if (dma_buf_is_dynamic(attach->dmabuf))
 			dmabuf->ops->unpin(attach);
-			dma_resv_unlock(attach->dmabuf->resv);
-		}
 	}
-
-	dma_resv_lock(dmabuf->resv, NULL);
 	list_del(&attach->node);
-	dma_resv_unlock(dmabuf->resv);
+
 	if (dmabuf->ops->detach)
 		dmabuf->ops->detach(dmabuf, attach);
 
+	dma_resv_unlock(dmabuf->resv);
 	kfree(attach);
 }
 EXPORT_SYMBOL_NS_GPL(dma_buf_detach_unlocked, DMA_BUF);
@@ -1030,10 +1028,11 @@ EXPORT_SYMBOL_NS_GPL(dma_buf_unpin, DMA_BUF);
  *
  * Important: Dynamic importers must wait for the exclusive fence of the struct
  * dma_resv attached to the DMA-BUF first.
+ *
+ * Importer is responsible for holding dmabuf's reservation lock.
  */
-struct sg_table *
-dma_buf_map_attachment_unlocked(struct dma_buf_attachment *attach,
-				enum dma_data_direction direction)
+struct sg_table *dma_buf_map_attachment_locked(struct dma_buf_attachment *attach,
+					       enum dma_data_direction direction)
 {
 	struct sg_table *sg_table;
 	int r;
@@ -1043,8 +1042,7 @@ dma_buf_map_attachment_unlocked(struct dma_buf_attachment *attach,
 	if (WARN_ON(!attach || !attach->dmabuf))
 		return ERR_PTR(-EINVAL);
 
-	if (dma_buf_attachment_is_dynamic(attach))
-		dma_resv_assert_held(attach->dmabuf->resv);
+	dma_resv_assert_held(attach->dmabuf->resv);
 
 	if (attach->sgt) {
 		/*
@@ -1059,7 +1057,6 @@ dma_buf_map_attachment_unlocked(struct dma_buf_attachment *attach,
 	}
 
 	if (dma_buf_is_dynamic(attach->dmabuf)) {
-		dma_resv_assert_held(attach->dmabuf->resv);
 		if (!IS_ENABLED(CONFIG_DMABUF_MOVE_NOTIFY)) {
 			r = attach->dmabuf->ops->pin(attach);
 			if (r)
@@ -1099,10 +1096,38 @@ dma_buf_map_attachment_unlocked(struct dma_buf_attachment *attach,
 #endif /* CONFIG_DMA_API_DEBUG */
 	return sg_table;
 }
+EXPORT_SYMBOL_NS_GPL(dma_buf_map_attachment_locked, DMA_BUF);
+
+/**
+ * dma_buf_map_attachment_unlocked - Returns the scatterlist table of the attachment;
+ * mapped into _device_ address space. Is a wrapper for map_dma_buf() of the
+ * dma_buf_ops.
+ * @attach:	[in]	attachment whose scatterlist is to be returned
+ * @direction:	[in]	direction of DMA transfer
+ *
+ * Unlocked variant of dma_buf_map_attachment().
+ */
+struct sg_table *
+dma_buf_map_attachment_unlocked(struct dma_buf_attachment *attach,
+				enum dma_data_direction direction)
+{
+	struct sg_table *sg_table;
+
+	might_sleep();
+
+	if (WARN_ON(!attach || !attach->dmabuf))
+		return ERR_PTR(-EINVAL);
+
+	dma_resv_lock(attach->dmabuf->resv, NULL);
+	sg_table = dma_buf_map_attachment_locked(attach, direction);
+	dma_resv_unlock(attach->dmabuf->resv);
+
+	return sg_table;
+}
 EXPORT_SYMBOL_NS_GPL(dma_buf_map_attachment_unlocked, DMA_BUF);
 
 /**
- * dma_buf_unmap_attachment_unlocked - unmaps and decreases usecount of the buffer;might
+ * dma_buf_unmap_attachment_locked - unmaps and decreases usecount of the buffer;might
  * deallocate the scatterlist associated. Is a wrapper for unmap_dma_buf() of
  * dma_buf_ops.
  * @attach:	[in]	attachment to unmap buffer from
@@ -1110,31 +1135,51 @@ EXPORT_SYMBOL_NS_GPL(dma_buf_map_attachment_unlocked, DMA_BUF);
  * @direction:  [in]    direction of DMA transfer
  *
  * This unmaps a DMA mapping for @attached obtained by dma_buf_map_attachment().
+ *
+ * Importer is responsible for holding dmabuf's reservation lock.
  */
-void dma_buf_unmap_attachment_unlocked(struct dma_buf_attachment *attach,
-				       struct sg_table *sg_table,
-				       enum dma_data_direction direction)
+void dma_buf_unmap_attachment_locked(struct dma_buf_attachment *attach,
+				     struct sg_table *sg_table,
+				     enum dma_data_direction direction)
 {
 	might_sleep();
 
-	if (WARN_ON(!attach || !attach->dmabuf || !sg_table))
-		return;
-
-	if (dma_buf_attachment_is_dynamic(attach))
-		dma_resv_assert_held(attach->dmabuf->resv);
+	dma_resv_assert_held(attach->dmabuf->resv);
 
 	if (attach->sgt == sg_table)
 		return;
 
-	if (dma_buf_is_dynamic(attach->dmabuf))
-		dma_resv_assert_held(attach->dmabuf->resv);
-
 	__unmap_dma_buf(attach, sg_table, direction);
 
 	if (dma_buf_is_dynamic(attach->dmabuf) &&
 	    !IS_ENABLED(CONFIG_DMABUF_MOVE_NOTIFY))
 		dma_buf_unpin(attach);
 }
+EXPORT_SYMBOL_NS_GPL(dma_buf_unmap_attachment_locked, DMA_BUF);
+
+/**
+ * dma_buf_unmap_attachment_unlocked - unmaps and decreases usecount of the buffer;might
+ * deallocate the scatterlist associated. Is a wrapper for unmap_dma_buf() of
+ * dma_buf_ops.
+ * @attach:	[in]	attachment to unmap buffer from
+ * @sg_table:	[in]	scatterlist info of the buffer to unmap
+ * @direction:	[in]	direction of DMA transfer
+ *
+ * Unlocked variant of dma_buf_unmap_attachment().
+ */
+void dma_buf_unmap_attachment_unlocked(struct dma_buf_attachment *attach,
+				       struct sg_table *sg_table,
+				       enum dma_data_direction direction)
+{
+	might_sleep();
+
+	if (WARN_ON(!attach || !attach->dmabuf || !sg_table))
+		return;
+
+	dma_resv_lock(attach->dmabuf->resv, NULL);
+	dma_buf_unmap_attachment_locked(attach, sg_table, direction);
+	dma_resv_unlock(attach->dmabuf->resv);
+}
 EXPORT_SYMBOL_NS_GPL(dma_buf_unmap_attachment_unlocked, DMA_BUF);
 
 /**
@@ -1348,6 +1393,8 @@ EXPORT_SYMBOL_NS_GPL(dma_buf_end_cpu_access, DMA_BUF);
 int dma_buf_mmap_unlocked(struct dma_buf *dmabuf, struct vm_area_struct *vma,
 			  unsigned long pgoff)
 {
+	int ret;
+
 	if (WARN_ON(!dmabuf || !vma))
 		return -EINVAL;
 
@@ -1368,7 +1415,11 @@ int dma_buf_mmap_unlocked(struct dma_buf *dmabuf, struct vm_area_struct *vma,
 	vma_set_file(vma, dmabuf->file);
 	vma->vm_pgoff = pgoff;
 
-	return dmabuf->ops->mmap(dmabuf, vma);
+	dma_resv_lock(dmabuf->resv, NULL);
+	ret = dmabuf->ops->mmap(dmabuf, vma);
+	dma_resv_unlock(dmabuf->resv);
+
+	return ret;
 }
 EXPORT_SYMBOL_NS_GPL(dma_buf_mmap_unlocked, DMA_BUF);
 
@@ -1401,6 +1452,7 @@ int dma_buf_vmap_unlocked(struct dma_buf *dmabuf, struct iosys_map *map)
 	if (!dmabuf->ops->vmap)
 		return -EINVAL;
 
+	dma_resv_lock(dmabuf->resv, NULL);
 	mutex_lock(&dmabuf->lock);
 	if (dmabuf->vmapping_counter) {
 		dmabuf->vmapping_counter++;
@@ -1422,6 +1474,7 @@ int dma_buf_vmap_unlocked(struct dma_buf *dmabuf, struct iosys_map *map)
 
 out_unlock:
 	mutex_unlock(&dmabuf->lock);
+	dma_resv_unlock(dmabuf->resv);
 	return ret;
 }
 EXPORT_SYMBOL_NS_GPL(dma_buf_vmap_unlocked, DMA_BUF);
@@ -1440,6 +1493,7 @@ void dma_buf_vunmap_unlocked(struct dma_buf *dmabuf, struct iosys_map *map)
 	BUG_ON(dmabuf->vmapping_counter == 0);
 	BUG_ON(!iosys_map_is_equal(&dmabuf->vmap_ptr, map));
 
+	dma_resv_lock(dmabuf->resv, NULL);
 	mutex_lock(&dmabuf->lock);
 	if (--dmabuf->vmapping_counter == 0) {
 		if (dmabuf->ops->vunmap)
@@ -1447,6 +1501,7 @@ void dma_buf_vunmap_unlocked(struct dma_buf *dmabuf, struct iosys_map *map)
 		iosys_map_clear(&dmabuf->vmap_ptr);
 	}
 	mutex_unlock(&dmabuf->lock);
+	dma_resv_unlock(dmabuf->resv);
 }
 EXPORT_SYMBOL_NS_GPL(dma_buf_vunmap_unlocked, DMA_BUF);
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index e354ad140a0a..0d8b82e5b22f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -883,7 +883,7 @@ static int amdgpu_ttm_backend_bind(struct ttm_device *bdev,
 			struct sg_table *sgt;
 
 			attach = gtt->gobj->import_attach;
-			sgt = dma_buf_map_attachment_unlocked(attach, DMA_BIDIRECTIONAL);
+			sgt = dma_buf_map_attachment_locked(attach, DMA_BIDIRECTIONAL);
 			if (IS_ERR(sgt))
 				return PTR_ERR(sgt);
 
@@ -1008,7 +1008,7 @@ static void amdgpu_ttm_backend_unbind(struct ttm_device *bdev,
 		struct dma_buf_attachment *attach;
 
 		attach = gtt->gobj->import_attach;
-		dma_buf_unmap_attachment_unlocked(attach, ttm->sg, DMA_BIDIRECTIONAL);
+		dma_buf_unmap_attachment_locked(attach, ttm->sg, DMA_BIDIRECTIONAL);
 		ttm->sg = NULL;
 	}
 
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 1bd234fd21a5..b75ef1756873 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -678,7 +678,7 @@ int drm_gem_dmabuf_vmap(struct dma_buf *dma_buf, struct iosys_map *map)
 {
 	struct drm_gem_object *obj = dma_buf->priv;
 
-	return drm_gem_vmap_unlocked(obj, map);
+	return drm_gem_vmap(obj, map);
 }
 EXPORT_SYMBOL(drm_gem_dmabuf_vmap);
 
@@ -694,7 +694,7 @@ void drm_gem_dmabuf_vunmap(struct dma_buf *dma_buf, struct iosys_map *map)
 {
 	struct drm_gem_object *obj = dma_buf->priv;
 
-	drm_gem_vunmap_unlocked(obj, map);
+	drm_gem_vunmap(obj, map);
 }
 EXPORT_SYMBOL(drm_gem_dmabuf_vunmap);
 
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
index cc54a5b1d6ae..c199bf71c373 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
@@ -174,10 +174,6 @@ static int i915_gem_dmabuf_attach(struct dma_buf *dmabuf,
 		return -EOPNOTSUPP;
 
 	for_i915_gem_ww(&ww, err, true) {
-		err = i915_gem_object_lock(obj, &ww);
-		if (err)
-			continue;
-
 		err = i915_gem_object_migrate(obj, &ww, INTEL_REGION_SMEM);
 		if (err)
 			continue;
@@ -241,8 +237,8 @@ static int i915_gem_object_get_pages_dmabuf(struct drm_i915_gem_object *obj)
 
 	assert_object_held(obj);
 
-	pages = dma_buf_map_attachment_unlocked(obj->base.import_attach,
-						DMA_BIDIRECTIONAL);
+	pages = dma_buf_map_attachment_locked(obj->base.import_attach,
+					      DMA_BIDIRECTIONAL);
 	if (IS_ERR(pages))
 		return PTR_ERR(pages);
 
@@ -270,8 +266,8 @@ static int i915_gem_object_get_pages_dmabuf(struct drm_i915_gem_object *obj)
 static void i915_gem_object_put_pages_dmabuf(struct drm_i915_gem_object *obj,
 					     struct sg_table *pages)
 {
-	dma_buf_unmap_attachment_unlocked(obj->base.import_attach, pages,
-					  DMA_BIDIRECTIONAL);
+	dma_buf_unmap_attachment_locked(obj->base.import_attach, pages,
+					DMA_BIDIRECTIONAL);
 }
 
 static const struct drm_i915_gem_object_ops i915_gem_object_dmabuf_ops = {
diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index 9ab09569dec1..da924a56d58f 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -626,6 +626,12 @@ void dma_buf_unmap_attachment_unlocked(struct dma_buf_attachment *,
 				       struct sg_table *,
 				       enum dma_data_direction);
 
+struct sg_table *dma_buf_map_attachment_locked(struct dma_buf_attachment *,
+					       enum dma_data_direction);
+void dma_buf_unmap_attachment_locked(struct dma_buf_attachment *attach,
+				     struct sg_table *sg_table,
+				     enum dma_data_direction direction);
+
 void dma_buf_move_notify(struct dma_buf *dma_buf);
 int dma_buf_begin_cpu_access(struct dma_buf *dma_buf,
 			     enum dma_data_direction dir);
-- 
2.36.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v1 4/6] dma-buf: Acquire wait-wound context on attachment
  2022-07-15  0:52 [PATCH v1 0/6] Move all drivers to a common dma-buf locking convention Dmitry Osipenko
                   ` (2 preceding siblings ...)
  2022-07-15  0:52 ` [PATCH v1 3/6] dma-buf: Move all dma-bufs to dynamic locking specification Dmitry Osipenko
@ 2022-07-15  0:52 ` Dmitry Osipenko
  2022-07-15  3:38   ` kernel test robot
  2022-07-15  6:50   ` Christian König
  2022-07-15  0:52 ` [PATCH v1 5/6] media: videobuf2: Stop using internal dma-buf lock Dmitry Osipenko
                   ` (2 subsequent siblings)
  6 siblings, 2 replies; 18+ messages in thread
From: Dmitry Osipenko @ 2022-07-15  0:52 UTC (permalink / raw)
  To: David Airlie, Gerd Hoffmann, Gurchetan Singh, Chia-I Wu,
	Daniel Vetter, Daniel Almeida, Gert Wollny, Gustavo Padovan,
	Daniel Stone, Tomeu Vizoso, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, Rob Clark, Sumit Semwal, Christian König,
	Pan, Xinhui, Thierry Reding, Tomasz Figa, Marek Szyprowski,
	Mauro Carvalho Chehab, Alex Deucher, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin,
	Thomas Hellström
  Cc: dri-devel, linux-kernel, Dmitry Osipenko, linux-media,
	linaro-mm-sig, amd-gfx, intel-gfx, kernel, virtualization,
	spice-devel, linux-rdma, linux-arm-msm

Intel i915 GPU driver uses wait-wound mutex to lock multiple GEMs on the
attachment to the i915 dma-buf. In order to let all drivers utilize shared
wait-wound context during attachment in a general way, make dma-buf core to
acquire the ww context internally for the attachment operation and update
i915 driver to use the importer's ww context instead of the internal one.

From now on all dma-buf exporters shall use the importer's ww context for
the attachment operation.

Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 drivers/dma-buf/dma-buf.c                     |  8 +++++-
 drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c    |  2 +-
 .../gpu/drm/i915/gem/i915_gem_execbuffer.c    |  2 +-
 drivers/gpu/drm/i915/gem/i915_gem_object.h    |  6 ++---
 drivers/gpu/drm/i915/i915_gem_evict.c         |  2 +-
 drivers/gpu/drm/i915/i915_gem_ww.c            | 26 +++++++++++++++----
 drivers/gpu/drm/i915/i915_gem_ww.h            | 15 +++++++++--
 7 files changed, 47 insertions(+), 14 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 0ee588276534..37545ecb845a 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -807,6 +807,8 @@ static struct sg_table * __map_dma_buf(struct dma_buf_attachment *attach,
  * Optionally this calls &dma_buf_ops.attach to allow device-specific attach
  * functionality.
  *
+ * Exporters shall use ww_ctx acquired by this function.
+ *
  * Returns:
  *
  * A pointer to newly created &dma_buf_attachment on success, or a negative
@@ -822,6 +824,7 @@ dma_buf_dynamic_attach_unlocked(struct dma_buf *dmabuf, struct device *dev,
 				void *importer_priv)
 {
 	struct dma_buf_attachment *attach;
+	struct ww_acquire_ctx ww_ctx;
 	int ret;
 
 	if (WARN_ON(!dmabuf || !dev))
@@ -841,7 +844,8 @@ dma_buf_dynamic_attach_unlocked(struct dma_buf *dmabuf, struct device *dev,
 	attach->importer_ops = importer_ops;
 	attach->importer_priv = importer_priv;
 
-	dma_resv_lock(dmabuf->resv, NULL);
+	ww_acquire_init(&ww_ctx, &reservation_ww_class);
+	dma_resv_lock(dmabuf->resv, &ww_ctx);
 
 	if (dmabuf->ops->attach) {
 		ret = dmabuf->ops->attach(dmabuf, attach);
@@ -876,11 +880,13 @@ dma_buf_dynamic_attach_unlocked(struct dma_buf *dmabuf, struct device *dev,
 	}
 
 	dma_resv_unlock(dmabuf->resv);
+	ww_acquire_fini(&ww_ctx);
 
 	return attach;
 
 err_attach:
 	dma_resv_unlock(attach->dmabuf->resv);
+	ww_acquire_fini(&ww_ctx);
 	kfree(attach);
 	return ERR_PTR(ret);
 
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
index c199bf71c373..9173f0232b16 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
@@ -173,7 +173,7 @@ static int i915_gem_dmabuf_attach(struct dma_buf *dmabuf,
 	if (!i915_gem_object_can_migrate(obj, INTEL_REGION_SMEM))
 		return -EOPNOTSUPP;
 
-	for_i915_gem_ww(&ww, err, true) {
+	for_i915_dmabuf_ww(&ww, dmabuf, err, true) {
 		err = i915_gem_object_migrate(obj, &ww, INTEL_REGION_SMEM);
 		if (err)
 			continue;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 30fe847c6664..ad7d602fc43a 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -3409,7 +3409,7 @@ i915_gem_do_execbuffer(struct drm_device *dev,
 		goto err_vma;
 	}
 
-	ww_acquire_done(&eb.ww.ctx);
+	ww_acquire_done(eb.ww.ctx);
 	eb_capture_stage(&eb);
 
 	out_fence = eb_requests_create(&eb, in_fence, out_fence_fd);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index e11d82a9f7c3..5ae38f94a5c7 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -178,9 +178,9 @@ static inline int __i915_gem_object_lock(struct drm_i915_gem_object *obj,
 	int ret;
 
 	if (intr)
-		ret = dma_resv_lock_interruptible(obj->base.resv, ww ? &ww->ctx : NULL);
+		ret = dma_resv_lock_interruptible(obj->base.resv, ww ? ww->ctx : NULL);
 	else
-		ret = dma_resv_lock(obj->base.resv, ww ? &ww->ctx : NULL);
+		ret = dma_resv_lock(obj->base.resv, ww ? ww->ctx : NULL);
 
 	if (!ret && ww) {
 		i915_gem_object_get(obj);
@@ -216,7 +216,7 @@ static inline bool i915_gem_object_trylock(struct drm_i915_gem_object *obj,
 	if (!ww)
 		return dma_resv_trylock(obj->base.resv);
 	else
-		return ww_mutex_trylock(&obj->base.resv->lock, &ww->ctx);
+		return ww_mutex_trylock(&obj->base.resv->lock, ww->ctx);
 }
 
 static inline void i915_gem_object_unlock(struct drm_i915_gem_object *obj)
diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
index f025ee4fa526..047f72e32d47 100644
--- a/drivers/gpu/drm/i915/i915_gem_evict.c
+++ b/drivers/gpu/drm/i915/i915_gem_evict.c
@@ -458,7 +458,7 @@ int i915_gem_evict_vm(struct i915_address_space *vm, struct i915_gem_ww_ctx *ww)
 			 * need the object ref.
 			 */
 			if (dying_vma(vma) ||
-			    (ww && (dma_resv_locking_ctx(vma->obj->base.resv) == &ww->ctx))) {
+			    (ww && (dma_resv_locking_ctx(vma->obj->base.resv) == ww->ctx))) {
 				__i915_vma_pin(vma);
 				list_add(&vma->evict_link, &locked_eviction_list);
 				continue;
diff --git a/drivers/gpu/drm/i915/i915_gem_ww.c b/drivers/gpu/drm/i915/i915_gem_ww.c
index 3f6ff139478e..c47898993c7d 100644
--- a/drivers/gpu/drm/i915/i915_gem_ww.c
+++ b/drivers/gpu/drm/i915/i915_gem_ww.c
@@ -6,12 +6,20 @@
 #include "i915_gem_ww.h"
 #include "gem/i915_gem_object.h"
 
-void i915_gem_ww_ctx_init(struct i915_gem_ww_ctx *ww, bool intr)
+void i915_gem_ww_ctx_prep(struct i915_gem_ww_ctx *ww,
+			  struct ww_acquire_ctx *ww_ctx,
+			  bool intr)
 {
-	ww_acquire_init(&ww->ctx, &reservation_ww_class);
 	INIT_LIST_HEAD(&ww->obj_list);
 	ww->intr = intr;
 	ww->contended = NULL;
+	ww->ctx = ww_ctx;
+}
+
+void i915_gem_ww_ctx_init(struct i915_gem_ww_ctx *ww, bool intr)
+{
+	ww_acquire_init(&ww->ww_ctx, &reservation_ww_class);
+	i915_gem_ww_ctx_prep(ww, &ww->ww_ctx, intr);
 }
 
 static void i915_gem_ww_ctx_unlock_all(struct i915_gem_ww_ctx *ww)
@@ -36,7 +44,15 @@ void i915_gem_ww_ctx_fini(struct i915_gem_ww_ctx *ww)
 {
 	i915_gem_ww_ctx_unlock_all(ww);
 	WARN_ON(ww->contended);
-	ww_acquire_fini(&ww->ctx);
+
+	if (ww->ctx == &ww->ww_ctx)
+		ww_acquire_fini(ww->ctx);
+}
+
+void i915_gem_ww_ctx_fini2(struct i915_gem_ww_ctx *ww)
+{
+	i915_gem_ww_ctx_unlock_all(ww);
+	WARN_ON(ww->contended);
 }
 
 int __must_check i915_gem_ww_ctx_backoff(struct i915_gem_ww_ctx *ww)
@@ -48,9 +64,9 @@ int __must_check i915_gem_ww_ctx_backoff(struct i915_gem_ww_ctx *ww)
 
 	i915_gem_ww_ctx_unlock_all(ww);
 	if (ww->intr)
-		ret = dma_resv_lock_slow_interruptible(ww->contended->base.resv, &ww->ctx);
+		ret = dma_resv_lock_slow_interruptible(ww->contended->base.resv, ww->ctx);
 	else
-		dma_resv_lock_slow(ww->contended->base.resv, &ww->ctx);
+		dma_resv_lock_slow(ww->contended->base.resv, ww->ctx);
 
 	if (!ret)
 		list_add_tail(&ww->contended->obj_link, &ww->obj_list);
diff --git a/drivers/gpu/drm/i915/i915_gem_ww.h b/drivers/gpu/drm/i915/i915_gem_ww.h
index 86f0fe343de6..e9b0fd4debbf 100644
--- a/drivers/gpu/drm/i915/i915_gem_ww.h
+++ b/drivers/gpu/drm/i915/i915_gem_ww.h
@@ -8,13 +8,17 @@
 #include <drm/drm_drv.h>
 
 struct i915_gem_ww_ctx {
-	struct ww_acquire_ctx ctx;
+	struct ww_acquire_ctx *ctx;
+	struct ww_acquire_ctx ww_ctx;
 	struct list_head obj_list;
 	struct drm_i915_gem_object *contended;
 	bool intr;
 };
 
-void i915_gem_ww_ctx_init(struct i915_gem_ww_ctx *ctx, bool intr);
+void i915_gem_ww_ctx_prep(struct i915_gem_ww_ctx *ww,
+			  struct ww_acquire_ctx *ww_ctx,
+			  bool intr);
+void i915_gem_ww_ctx_init(struct i915_gem_ww_ctx *ww, bool intr);
 void i915_gem_ww_ctx_fini(struct i915_gem_ww_ctx *ctx);
 int __must_check i915_gem_ww_ctx_backoff(struct i915_gem_ww_ctx *ctx);
 void i915_gem_ww_unlock_single(struct drm_i915_gem_object *obj);
@@ -38,4 +42,11 @@ static inline int __i915_gem_ww_fini(struct i915_gem_ww_ctx *ww, int err)
 	for (i915_gem_ww_ctx_init(_ww, _intr), (_err) = -EDEADLK; \
 	     (_err) == -EDEADLK;				  \
 	     (_err) = __i915_gem_ww_fini(_ww, _err))
+
+#define for_i915_dmabuf_ww(_ww, _dmabuf, _err, _intr)		  \
+	for (i915_gem_ww_ctx_prep(_ww, dma_resv_locking_ctx((_dmabuf)->resv), _intr), \
+	     (_err) = -EDEADLK; 				  \
+	     (_err) == -EDEADLK;				  \
+	     (_err) = __i915_gem_ww_fini(_ww, _err))
+
 #endif
-- 
2.36.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v1 5/6] media: videobuf2: Stop using internal dma-buf lock
  2022-07-15  0:52 [PATCH v1 0/6] Move all drivers to a common dma-buf locking convention Dmitry Osipenko
                   ` (3 preceding siblings ...)
  2022-07-15  0:52 ` [PATCH v1 4/6] dma-buf: Acquire wait-wound context on attachment Dmitry Osipenko
@ 2022-07-15  0:52 ` Dmitry Osipenko
  2022-07-15  0:52 ` [PATCH v1 6/6] dma-buf: Remove internal lock Dmitry Osipenko
  2022-07-19  9:13 ` [PATCH v1 0/6] Move all drivers to a common dma-buf locking convention Tomasz Figa
  6 siblings, 0 replies; 18+ messages in thread
From: Dmitry Osipenko @ 2022-07-15  0:52 UTC (permalink / raw)
  To: David Airlie, Gerd Hoffmann, Gurchetan Singh, Chia-I Wu,
	Daniel Vetter, Daniel Almeida, Gert Wollny, Gustavo Padovan,
	Daniel Stone, Tomeu Vizoso, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, Rob Clark, Sumit Semwal, Christian König,
	Pan, Xinhui, Thierry Reding, Tomasz Figa, Marek Szyprowski,
	Mauro Carvalho Chehab, Alex Deucher, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin,
	Thomas Hellström
  Cc: dri-devel, linux-kernel, Dmitry Osipenko, linux-media,
	linaro-mm-sig, amd-gfx, intel-gfx, kernel, virtualization,
	spice-devel, linux-rdma, linux-arm-msm

All drivers that use dma-bufs have been moved to the updated locking
specification and now dma-buf reservation is guaranteed to be locked
by importers during the mapping operations. There is no need to take
the internal dma-buf lock anymore. Remove locking from the videobuf2
memory allocators.

Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 drivers/media/common/videobuf2/videobuf2-dma-contig.c | 11 +----------
 drivers/media/common/videobuf2/videobuf2-dma-sg.c     | 11 +----------
 drivers/media/common/videobuf2/videobuf2-vmalloc.c    | 11 +----------
 3 files changed, 3 insertions(+), 30 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
index de762dbdaf78..2c69bf0470e7 100644
--- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
@@ -382,18 +382,12 @@ static struct sg_table *vb2_dc_dmabuf_ops_map(
 	struct dma_buf_attachment *db_attach, enum dma_data_direction dma_dir)
 {
 	struct vb2_dc_attachment *attach = db_attach->priv;
-	/* stealing dmabuf mutex to serialize map/unmap operations */
-	struct mutex *lock = &db_attach->dmabuf->lock;
 	struct sg_table *sgt;
 
-	mutex_lock(lock);
-
 	sgt = &attach->sgt;
 	/* return previously mapped sg table */
-	if (attach->dma_dir == dma_dir) {
-		mutex_unlock(lock);
+	if (attach->dma_dir == dma_dir)
 		return sgt;
-	}
 
 	/* release any previous cache */
 	if (attach->dma_dir != DMA_NONE) {
@@ -409,14 +403,11 @@ static struct sg_table *vb2_dc_dmabuf_ops_map(
 	if (dma_map_sgtable(db_attach->dev, sgt, dma_dir,
 			    DMA_ATTR_SKIP_CPU_SYNC)) {
 		pr_err("failed to map scatterlist\n");
-		mutex_unlock(lock);
 		return ERR_PTR(-EIO);
 	}
 
 	attach->dma_dir = dma_dir;
 
-	mutex_unlock(lock);
-
 	return sgt;
 }
 
diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
index 39e11600304a..e63e718c0bf7 100644
--- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
@@ -424,18 +424,12 @@ static struct sg_table *vb2_dma_sg_dmabuf_ops_map(
 	struct dma_buf_attachment *db_attach, enum dma_data_direction dma_dir)
 {
 	struct vb2_dma_sg_attachment *attach = db_attach->priv;
-	/* stealing dmabuf mutex to serialize map/unmap operations */
-	struct mutex *lock = &db_attach->dmabuf->lock;
 	struct sg_table *sgt;
 
-	mutex_lock(lock);
-
 	sgt = &attach->sgt;
 	/* return previously mapped sg table */
-	if (attach->dma_dir == dma_dir) {
-		mutex_unlock(lock);
+	if (attach->dma_dir == dma_dir)
 		return sgt;
-	}
 
 	/* release any previous cache */
 	if (attach->dma_dir != DMA_NONE) {
@@ -446,14 +440,11 @@ static struct sg_table *vb2_dma_sg_dmabuf_ops_map(
 	/* mapping to the client with new direction */
 	if (dma_map_sgtable(db_attach->dev, sgt, dma_dir, 0)) {
 		pr_err("failed to map scatterlist\n");
-		mutex_unlock(lock);
 		return ERR_PTR(-EIO);
 	}
 
 	attach->dma_dir = dma_dir;
 
-	mutex_unlock(lock);
-
 	return sgt;
 }
 
diff --git a/drivers/media/common/videobuf2/videobuf2-vmalloc.c b/drivers/media/common/videobuf2/videobuf2-vmalloc.c
index 7831bf545874..41db707e43a4 100644
--- a/drivers/media/common/videobuf2/videobuf2-vmalloc.c
+++ b/drivers/media/common/videobuf2/videobuf2-vmalloc.c
@@ -267,18 +267,12 @@ static struct sg_table *vb2_vmalloc_dmabuf_ops_map(
 	struct dma_buf_attachment *db_attach, enum dma_data_direction dma_dir)
 {
 	struct vb2_vmalloc_attachment *attach = db_attach->priv;
-	/* stealing dmabuf mutex to serialize map/unmap operations */
-	struct mutex *lock = &db_attach->dmabuf->lock;
 	struct sg_table *sgt;
 
-	mutex_lock(lock);
-
 	sgt = &attach->sgt;
 	/* return previously mapped sg table */
-	if (attach->dma_dir == dma_dir) {
-		mutex_unlock(lock);
+	if (attach->dma_dir == dma_dir)
 		return sgt;
-	}
 
 	/* release any previous cache */
 	if (attach->dma_dir != DMA_NONE) {
@@ -289,14 +283,11 @@ static struct sg_table *vb2_vmalloc_dmabuf_ops_map(
 	/* mapping to the client with new direction */
 	if (dma_map_sgtable(db_attach->dev, sgt, dma_dir, 0)) {
 		pr_err("failed to map scatterlist\n");
-		mutex_unlock(lock);
 		return ERR_PTR(-EIO);
 	}
 
 	attach->dma_dir = dma_dir;
 
-	mutex_unlock(lock);
-
 	return sgt;
 }
 
-- 
2.36.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v1 6/6] dma-buf: Remove internal lock
  2022-07-15  0:52 [PATCH v1 0/6] Move all drivers to a common dma-buf locking convention Dmitry Osipenko
                   ` (4 preceding siblings ...)
  2022-07-15  0:52 ` [PATCH v1 5/6] media: videobuf2: Stop using internal dma-buf lock Dmitry Osipenko
@ 2022-07-15  0:52 ` Dmitry Osipenko
  2022-07-19  9:13 ` [PATCH v1 0/6] Move all drivers to a common dma-buf locking convention Tomasz Figa
  6 siblings, 0 replies; 18+ messages in thread
From: Dmitry Osipenko @ 2022-07-15  0:52 UTC (permalink / raw)
  To: David Airlie, Gerd Hoffmann, Gurchetan Singh, Chia-I Wu,
	Daniel Vetter, Daniel Almeida, Gert Wollny, Gustavo Padovan,
	Daniel Stone, Tomeu Vizoso, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, Rob Clark, Sumit Semwal, Christian König,
	Pan, Xinhui, Thierry Reding, Tomasz Figa, Marek Szyprowski,
	Mauro Carvalho Chehab, Alex Deucher, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin,
	Thomas Hellström
  Cc: dri-devel, linux-kernel, Dmitry Osipenko, linux-media,
	linaro-mm-sig, amd-gfx, intel-gfx, kernel, virtualization,
	spice-devel, linux-rdma, linux-arm-msm

The internal dma-buf lock isn't needed anymore because the updated
locking specification claims that dma-buf reservation must be locked
by importers, and thus, the internal data is already protected by the
reservation lock. Remove the obsoleted internal lock.

Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 drivers/dma-buf/dma-buf.c | 5 -----
 include/linux/dma-buf.h   | 9 ---------
 2 files changed, 14 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 37545ecb845a..4cc739537ebd 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -656,7 +656,6 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info)
 
 	dmabuf->file = file;
 
-	mutex_init(&dmabuf->lock);
 	INIT_LIST_HEAD(&dmabuf->attachments);
 
 	mutex_lock(&db_list.lock);
@@ -1459,7 +1458,6 @@ int dma_buf_vmap_unlocked(struct dma_buf *dmabuf, struct iosys_map *map)
 		return -EINVAL;
 
 	dma_resv_lock(dmabuf->resv, NULL);
-	mutex_lock(&dmabuf->lock);
 	if (dmabuf->vmapping_counter) {
 		dmabuf->vmapping_counter++;
 		BUG_ON(iosys_map_is_null(&dmabuf->vmap_ptr));
@@ -1479,7 +1477,6 @@ int dma_buf_vmap_unlocked(struct dma_buf *dmabuf, struct iosys_map *map)
 	*map = dmabuf->vmap_ptr;
 
 out_unlock:
-	mutex_unlock(&dmabuf->lock);
 	dma_resv_unlock(dmabuf->resv);
 	return ret;
 }
@@ -1500,13 +1497,11 @@ void dma_buf_vunmap_unlocked(struct dma_buf *dmabuf, struct iosys_map *map)
 	BUG_ON(!iosys_map_is_equal(&dmabuf->vmap_ptr, map));
 
 	dma_resv_lock(dmabuf->resv, NULL);
-	mutex_lock(&dmabuf->lock);
 	if (--dmabuf->vmapping_counter == 0) {
 		if (dmabuf->ops->vunmap)
 			dmabuf->ops->vunmap(dmabuf, map);
 		iosys_map_clear(&dmabuf->vmap_ptr);
 	}
-	mutex_unlock(&dmabuf->lock);
 	dma_resv_unlock(dmabuf->resv);
 }
 EXPORT_SYMBOL_NS_GPL(dma_buf_vunmap_unlocked, DMA_BUF);
diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index da924a56d58f..abdd99042c77 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -326,15 +326,6 @@ struct dma_buf {
 	/** @ops: dma_buf_ops associated with this buffer object. */
 	const struct dma_buf_ops *ops;
 
-	/**
-	 * @lock:
-	 *
-	 * Used internally to serialize list manipulation, attach/detach and
-	 * vmap/unmap. Note that in many cases this is superseeded by
-	 * dma_resv_lock() on @resv.
-	 */
-	struct mutex lock;
-
 	/**
 	 * @vmapping_counter:
 	 *
-- 
2.36.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH v1 4/6] dma-buf: Acquire wait-wound context on attachment
  2022-07-15  0:52 ` [PATCH v1 4/6] dma-buf: Acquire wait-wound context on attachment Dmitry Osipenko
@ 2022-07-15  3:38   ` kernel test robot
  2022-07-15  6:50   ` Christian König
  1 sibling, 0 replies; 18+ messages in thread
From: kernel test robot @ 2022-07-15  3:38 UTC (permalink / raw)
  To: Dmitry Osipenko, David Airlie, Gerd Hoffmann, Gurchetan Singh,
	Chia-I Wu, Daniel Vetter, Daniel Almeida, Gert Wollny,
	Gustavo Padovan, Daniel Stone, Tomeu Vizoso, Maarten Lankhorst,
	Maxime Ripard, Thomas Zimmermann, Rob Clark, Sumit Semwal,
	Christian König, Pan, Xinhui, Thierry Reding, Tomasz Figa,
	Marek Szyprowski, Mauro Carvalho Chehab, Alex Deucher,
	Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin,
	Thomas Hellström
  Cc: kbuild-all, linux-media, dri-devel, linux-kernel

Hi Dmitry,

I love your patch! Yet something to improve:

[auto build test ERROR on next-20220714]
[also build test ERROR on v5.19-rc6]
[cannot apply to drm-misc/drm-misc-next drm-intel/for-linux-next media-tree/master linus/master v5.19-rc6 v5.19-rc5 v5.19-rc4]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Dmitry-Osipenko/Move-all-drivers-to-a-common-dma-buf-locking-convention/20220715-085556
base:    37b355fdaf31ee18bda9a93c2a438dc1cbf57ec9
config: x86_64-allmodconfig (https://download.01.org/0day-ci/archive/20220715/202207151112.Yi2gyyRX-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/ed55f535b8492ef30d7e94aae5811c772403ab4f
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Dmitry-Osipenko/Move-all-drivers-to-a-common-dma-buf-locking-convention/20220715-085556
        git checkout ed55f535b8492ef30d7e94aae5811c772403ab4f
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/gpu/drm/i915/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/i915/i915_gem_ww.c:52:6: error: no previous prototype for 'i915_gem_ww_ctx_fini2' [-Werror=missing-prototypes]
      52 | void i915_gem_ww_ctx_fini2(struct i915_gem_ww_ctx *ww)
         |      ^~~~~~~~~~~~~~~~~~~~~
   cc1: all warnings being treated as errors


vim +/i915_gem_ww_ctx_fini2 +52 drivers/gpu/drm/i915/i915_gem_ww.c

    51	
  > 52	void i915_gem_ww_ctx_fini2(struct i915_gem_ww_ctx *ww)
    53	{
    54		i915_gem_ww_ctx_unlock_all(ww);
    55		WARN_ON(ww->contended);
    56	}
    57	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v1 4/6] dma-buf: Acquire wait-wound context on attachment
  2022-07-15  0:52 ` [PATCH v1 4/6] dma-buf: Acquire wait-wound context on attachment Dmitry Osipenko
  2022-07-15  3:38   ` kernel test robot
@ 2022-07-15  6:50   ` Christian König
  2022-07-15  6:59     ` Dmitry Osipenko
  1 sibling, 1 reply; 18+ messages in thread
From: Christian König @ 2022-07-15  6:50 UTC (permalink / raw)
  To: Dmitry Osipenko, David Airlie, Gerd Hoffmann, Gurchetan Singh,
	Chia-I Wu, Daniel Vetter, Daniel Almeida, Gert Wollny,
	Gustavo Padovan, Daniel Stone, Tomeu Vizoso, Maarten Lankhorst,
	Maxime Ripard, Thomas Zimmermann, Rob Clark, Sumit Semwal, Pan,
	Xinhui, Thierry Reding, Tomasz Figa, Marek Szyprowski,
	Mauro Carvalho Chehab, Alex Deucher, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin,
	Thomas Hellström
  Cc: dri-devel, linux-kernel, Dmitry Osipenko, linux-media,
	linaro-mm-sig, amd-gfx, intel-gfx, kernel, virtualization,
	spice-devel, linux-rdma, linux-arm-msm

Am 15.07.22 um 02:52 schrieb Dmitry Osipenko:
> Intel i915 GPU driver uses wait-wound mutex to lock multiple GEMs on the
> attachment to the i915 dma-buf. In order to let all drivers utilize shared
> wait-wound context during attachment in a general way, make dma-buf core to
> acquire the ww context internally for the attachment operation and update
> i915 driver to use the importer's ww context instead of the internal one.
>
>  From now on all dma-buf exporters shall use the importer's ww context for
> the attachment operation.
>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> ---
>   drivers/dma-buf/dma-buf.c                     |  8 +++++-
>   drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c    |  2 +-
>   .../gpu/drm/i915/gem/i915_gem_execbuffer.c    |  2 +-
>   drivers/gpu/drm/i915/gem/i915_gem_object.h    |  6 ++---
>   drivers/gpu/drm/i915/i915_gem_evict.c         |  2 +-
>   drivers/gpu/drm/i915/i915_gem_ww.c            | 26 +++++++++++++++----
>   drivers/gpu/drm/i915/i915_gem_ww.h            | 15 +++++++++--
>   7 files changed, 47 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 0ee588276534..37545ecb845a 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -807,6 +807,8 @@ static struct sg_table * __map_dma_buf(struct dma_buf_attachment *attach,
>    * Optionally this calls &dma_buf_ops.attach to allow device-specific attach
>    * functionality.
>    *
> + * Exporters shall use ww_ctx acquired by this function.
> + *
>    * Returns:
>    *
>    * A pointer to newly created &dma_buf_attachment on success, or a negative
> @@ -822,6 +824,7 @@ dma_buf_dynamic_attach_unlocked(struct dma_buf *dmabuf, struct device *dev,
>   				void *importer_priv)
>   {
>   	struct dma_buf_attachment *attach;
> +	struct ww_acquire_ctx ww_ctx;
>   	int ret;
>   
>   	if (WARN_ON(!dmabuf || !dev))
> @@ -841,7 +844,8 @@ dma_buf_dynamic_attach_unlocked(struct dma_buf *dmabuf, struct device *dev,
>   	attach->importer_ops = importer_ops;
>   	attach->importer_priv = importer_priv;
>   
> -	dma_resv_lock(dmabuf->resv, NULL);
> +	ww_acquire_init(&ww_ctx, &reservation_ww_class);
> +	dma_resv_lock(dmabuf->resv, &ww_ctx);

That won't work like this. The core property of a WW context is that you 
need to unwind all the locks and re-quire them with the contended one first.

When you statically lock the imported one here you can't do that any more.

Regards,
Christian.

>   
>   	if (dmabuf->ops->attach) {
>   		ret = dmabuf->ops->attach(dmabuf, attach);
> @@ -876,11 +880,13 @@ dma_buf_dynamic_attach_unlocked(struct dma_buf *dmabuf, struct device *dev,
>   	}
>   
>   	dma_resv_unlock(dmabuf->resv);
> +	ww_acquire_fini(&ww_ctx);
>   
>   	return attach;
>   
>   err_attach:
>   	dma_resv_unlock(attach->dmabuf->resv);
> +	ww_acquire_fini(&ww_ctx);
>   	kfree(attach);
>   	return ERR_PTR(ret);
>   
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> index c199bf71c373..9173f0232b16 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> @@ -173,7 +173,7 @@ static int i915_gem_dmabuf_attach(struct dma_buf *dmabuf,
>   	if (!i915_gem_object_can_migrate(obj, INTEL_REGION_SMEM))
>   		return -EOPNOTSUPP;
>   
> -	for_i915_gem_ww(&ww, err, true) {
> +	for_i915_dmabuf_ww(&ww, dmabuf, err, true) {
>   		err = i915_gem_object_migrate(obj, &ww, INTEL_REGION_SMEM);
>   		if (err)
>   			continue;
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index 30fe847c6664..ad7d602fc43a 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -3409,7 +3409,7 @@ i915_gem_do_execbuffer(struct drm_device *dev,
>   		goto err_vma;
>   	}
>   
> -	ww_acquire_done(&eb.ww.ctx);
> +	ww_acquire_done(eb.ww.ctx);
>   	eb_capture_stage(&eb);
>   
>   	out_fence = eb_requests_create(&eb, in_fence, out_fence_fd);
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> index e11d82a9f7c3..5ae38f94a5c7 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> @@ -178,9 +178,9 @@ static inline int __i915_gem_object_lock(struct drm_i915_gem_object *obj,
>   	int ret;
>   
>   	if (intr)
> -		ret = dma_resv_lock_interruptible(obj->base.resv, ww ? &ww->ctx : NULL);
> +		ret = dma_resv_lock_interruptible(obj->base.resv, ww ? ww->ctx : NULL);
>   	else
> -		ret = dma_resv_lock(obj->base.resv, ww ? &ww->ctx : NULL);
> +		ret = dma_resv_lock(obj->base.resv, ww ? ww->ctx : NULL);
>   
>   	if (!ret && ww) {
>   		i915_gem_object_get(obj);
> @@ -216,7 +216,7 @@ static inline bool i915_gem_object_trylock(struct drm_i915_gem_object *obj,
>   	if (!ww)
>   		return dma_resv_trylock(obj->base.resv);
>   	else
> -		return ww_mutex_trylock(&obj->base.resv->lock, &ww->ctx);
> +		return ww_mutex_trylock(&obj->base.resv->lock, ww->ctx);
>   }
>   
>   static inline void i915_gem_object_unlock(struct drm_i915_gem_object *obj)
> diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
> index f025ee4fa526..047f72e32d47 100644
> --- a/drivers/gpu/drm/i915/i915_gem_evict.c
> +++ b/drivers/gpu/drm/i915/i915_gem_evict.c
> @@ -458,7 +458,7 @@ int i915_gem_evict_vm(struct i915_address_space *vm, struct i915_gem_ww_ctx *ww)
>   			 * need the object ref.
>   			 */
>   			if (dying_vma(vma) ||
> -			    (ww && (dma_resv_locking_ctx(vma->obj->base.resv) == &ww->ctx))) {
> +			    (ww && (dma_resv_locking_ctx(vma->obj->base.resv) == ww->ctx))) {
>   				__i915_vma_pin(vma);
>   				list_add(&vma->evict_link, &locked_eviction_list);
>   				continue;
> diff --git a/drivers/gpu/drm/i915/i915_gem_ww.c b/drivers/gpu/drm/i915/i915_gem_ww.c
> index 3f6ff139478e..c47898993c7d 100644
> --- a/drivers/gpu/drm/i915/i915_gem_ww.c
> +++ b/drivers/gpu/drm/i915/i915_gem_ww.c
> @@ -6,12 +6,20 @@
>   #include "i915_gem_ww.h"
>   #include "gem/i915_gem_object.h"
>   
> -void i915_gem_ww_ctx_init(struct i915_gem_ww_ctx *ww, bool intr)
> +void i915_gem_ww_ctx_prep(struct i915_gem_ww_ctx *ww,
> +			  struct ww_acquire_ctx *ww_ctx,
> +			  bool intr)
>   {
> -	ww_acquire_init(&ww->ctx, &reservation_ww_class);
>   	INIT_LIST_HEAD(&ww->obj_list);
>   	ww->intr = intr;
>   	ww->contended = NULL;
> +	ww->ctx = ww_ctx;
> +}
> +
> +void i915_gem_ww_ctx_init(struct i915_gem_ww_ctx *ww, bool intr)
> +{
> +	ww_acquire_init(&ww->ww_ctx, &reservation_ww_class);
> +	i915_gem_ww_ctx_prep(ww, &ww->ww_ctx, intr);
>   }
>   
>   static void i915_gem_ww_ctx_unlock_all(struct i915_gem_ww_ctx *ww)
> @@ -36,7 +44,15 @@ void i915_gem_ww_ctx_fini(struct i915_gem_ww_ctx *ww)
>   {
>   	i915_gem_ww_ctx_unlock_all(ww);
>   	WARN_ON(ww->contended);
> -	ww_acquire_fini(&ww->ctx);
> +
> +	if (ww->ctx == &ww->ww_ctx)
> +		ww_acquire_fini(ww->ctx);
> +}
> +
> +void i915_gem_ww_ctx_fini2(struct i915_gem_ww_ctx *ww)
> +{
> +	i915_gem_ww_ctx_unlock_all(ww);
> +	WARN_ON(ww->contended);
>   }
>   
>   int __must_check i915_gem_ww_ctx_backoff(struct i915_gem_ww_ctx *ww)
> @@ -48,9 +64,9 @@ int __must_check i915_gem_ww_ctx_backoff(struct i915_gem_ww_ctx *ww)
>   
>   	i915_gem_ww_ctx_unlock_all(ww);
>   	if (ww->intr)
> -		ret = dma_resv_lock_slow_interruptible(ww->contended->base.resv, &ww->ctx);
> +		ret = dma_resv_lock_slow_interruptible(ww->contended->base.resv, ww->ctx);
>   	else
> -		dma_resv_lock_slow(ww->contended->base.resv, &ww->ctx);
> +		dma_resv_lock_slow(ww->contended->base.resv, ww->ctx);
>   
>   	if (!ret)
>   		list_add_tail(&ww->contended->obj_link, &ww->obj_list);
> diff --git a/drivers/gpu/drm/i915/i915_gem_ww.h b/drivers/gpu/drm/i915/i915_gem_ww.h
> index 86f0fe343de6..e9b0fd4debbf 100644
> --- a/drivers/gpu/drm/i915/i915_gem_ww.h
> +++ b/drivers/gpu/drm/i915/i915_gem_ww.h
> @@ -8,13 +8,17 @@
>   #include <drm/drm_drv.h>
>   
>   struct i915_gem_ww_ctx {
> -	struct ww_acquire_ctx ctx;
> +	struct ww_acquire_ctx *ctx;
> +	struct ww_acquire_ctx ww_ctx;
>   	struct list_head obj_list;
>   	struct drm_i915_gem_object *contended;
>   	bool intr;
>   };
>   
> -void i915_gem_ww_ctx_init(struct i915_gem_ww_ctx *ctx, bool intr);
> +void i915_gem_ww_ctx_prep(struct i915_gem_ww_ctx *ww,
> +			  struct ww_acquire_ctx *ww_ctx,
> +			  bool intr);
> +void i915_gem_ww_ctx_init(struct i915_gem_ww_ctx *ww, bool intr);
>   void i915_gem_ww_ctx_fini(struct i915_gem_ww_ctx *ctx);
>   int __must_check i915_gem_ww_ctx_backoff(struct i915_gem_ww_ctx *ctx);
>   void i915_gem_ww_unlock_single(struct drm_i915_gem_object *obj);
> @@ -38,4 +42,11 @@ static inline int __i915_gem_ww_fini(struct i915_gem_ww_ctx *ww, int err)
>   	for (i915_gem_ww_ctx_init(_ww, _intr), (_err) = -EDEADLK; \
>   	     (_err) == -EDEADLK;				  \
>   	     (_err) = __i915_gem_ww_fini(_ww, _err))
> +
> +#define for_i915_dmabuf_ww(_ww, _dmabuf, _err, _intr)		  \
> +	for (i915_gem_ww_ctx_prep(_ww, dma_resv_locking_ctx((_dmabuf)->resv), _intr), \
> +	     (_err) = -EDEADLK; 				  \
> +	     (_err) == -EDEADLK;				  \
> +	     (_err) = __i915_gem_ww_fini(_ww, _err))
> +
>   #endif


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v1 4/6] dma-buf: Acquire wait-wound context on attachment
  2022-07-15  6:50   ` Christian König
@ 2022-07-15  6:59     ` Dmitry Osipenko
  2022-07-19 20:05       ` Dmitry Osipenko
  0 siblings, 1 reply; 18+ messages in thread
From: Dmitry Osipenko @ 2022-07-15  6:59 UTC (permalink / raw)
  To: Christian König, David Airlie, Gerd Hoffmann,
	Gurchetan Singh, Chia-I Wu, Daniel Vetter, Daniel Almeida,
	Gert Wollny, Gustavo Padovan, Daniel Stone, Tomeu Vizoso,
	Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Rob Clark,
	Sumit Semwal, Pan, Xinhui, Thierry Reding, Tomasz Figa,
	Marek Szyprowski, Mauro Carvalho Chehab, Alex Deucher,
	Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin,
	Thomas Hellström
  Cc: dri-devel, linux-kernel, Dmitry Osipenko, linux-media,
	linaro-mm-sig, amd-gfx, intel-gfx, kernel, virtualization,
	spice-devel, linux-rdma, linux-arm-msm

On 7/15/22 09:50, Christian König wrote:
> Am 15.07.22 um 02:52 schrieb Dmitry Osipenko:
>> Intel i915 GPU driver uses wait-wound mutex to lock multiple GEMs on the
>> attachment to the i915 dma-buf. In order to let all drivers utilize
>> shared
>> wait-wound context during attachment in a general way, make dma-buf
>> core to
>> acquire the ww context internally for the attachment operation and update
>> i915 driver to use the importer's ww context instead of the internal one.
>>
>>  From now on all dma-buf exporters shall use the importer's ww context
>> for
>> the attachment operation.
>>
>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>> ---
>>   drivers/dma-buf/dma-buf.c                     |  8 +++++-
>>   drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c    |  2 +-
>>   .../gpu/drm/i915/gem/i915_gem_execbuffer.c    |  2 +-
>>   drivers/gpu/drm/i915/gem/i915_gem_object.h    |  6 ++---
>>   drivers/gpu/drm/i915/i915_gem_evict.c         |  2 +-
>>   drivers/gpu/drm/i915/i915_gem_ww.c            | 26 +++++++++++++++----
>>   drivers/gpu/drm/i915/i915_gem_ww.h            | 15 +++++++++--
>>   7 files changed, 47 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>> index 0ee588276534..37545ecb845a 100644
>> --- a/drivers/dma-buf/dma-buf.c
>> +++ b/drivers/dma-buf/dma-buf.c
>> @@ -807,6 +807,8 @@ static struct sg_table * __map_dma_buf(struct
>> dma_buf_attachment *attach,
>>    * Optionally this calls &dma_buf_ops.attach to allow
>> device-specific attach
>>    * functionality.
>>    *
>> + * Exporters shall use ww_ctx acquired by this function.
>> + *
>>    * Returns:
>>    *
>>    * A pointer to newly created &dma_buf_attachment on success, or a
>> negative
>> @@ -822,6 +824,7 @@ dma_buf_dynamic_attach_unlocked(struct dma_buf
>> *dmabuf, struct device *dev,
>>                   void *importer_priv)
>>   {
>>       struct dma_buf_attachment *attach;
>> +    struct ww_acquire_ctx ww_ctx;
>>       int ret;
>>         if (WARN_ON(!dmabuf || !dev))
>> @@ -841,7 +844,8 @@ dma_buf_dynamic_attach_unlocked(struct dma_buf
>> *dmabuf, struct device *dev,
>>       attach->importer_ops = importer_ops;
>>       attach->importer_priv = importer_priv;
>>   -    dma_resv_lock(dmabuf->resv, NULL);
>> +    ww_acquire_init(&ww_ctx, &reservation_ww_class);
>> +    dma_resv_lock(dmabuf->resv, &ww_ctx);
> 
> That won't work like this. The core property of a WW context is that you
> need to unwind all the locks and re-quire them with the contended one
> first.
> 
> When you statically lock the imported one here you can't do that any more.

You're right. I felt that something is missing here, but couldn't
notice. I'll think more about this and enable
CONFIG_DEBUG_WW_MUTEX_SLOWPATH. Thank you!

-- 
Best regards,
Dmitry

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v1 1/6] dma-buf: Add _unlocked postfix to function names
  2022-07-15  0:52 ` [PATCH v1 1/6] dma-buf: Add _unlocked postfix to function names Dmitry Osipenko
@ 2022-07-15  7:19   ` Christian König
  2022-07-15  9:31     ` Dmitry Osipenko
  0 siblings, 1 reply; 18+ messages in thread
From: Christian König @ 2022-07-15  7:19 UTC (permalink / raw)
  To: Dmitry Osipenko, David Airlie, Gerd Hoffmann, Gurchetan Singh,
	Chia-I Wu, Daniel Vetter, Daniel Almeida, Gert Wollny,
	Gustavo Padovan, Daniel Stone, Tomeu Vizoso, Maarten Lankhorst,
	Maxime Ripard, Thomas Zimmermann, Rob Clark, Sumit Semwal, Pan,
	Xinhui, Thierry Reding, Tomasz Figa, Marek Szyprowski,
	Mauro Carvalho Chehab, Alex Deucher, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin,
	Thomas Hellström
  Cc: dri-devel, linux-kernel, Dmitry Osipenko, linux-media,
	linaro-mm-sig, amd-gfx, intel-gfx, kernel, virtualization,
	spice-devel, linux-rdma, linux-arm-msm

Am 15.07.22 um 02:52 schrieb Dmitry Osipenko:
> Add _unlocked postfix to the dma-buf API function names in a preparation
> to move all non-dynamic dma-buf users over to the dynamic locking
> specification. This patch only renames API functions, preparing drivers
> to the common locking convention. Later on we will make the "unlocked"
> functions to take the reservation lock.
>
> Suggested-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> ---
>   drivers/dma-buf/dma-buf.c                     | 76 ++++++++++---------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c   |  4 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c       |  4 +-
>   drivers/gpu/drm/armada/armada_gem.c           | 14 ++--
>   drivers/gpu/drm/drm_gem_cma_helper.c          |  6 +-
>   drivers/gpu/drm/drm_gem_shmem_helper.c        |  6 +-
>   drivers/gpu/drm/drm_prime.c                   | 12 +--
>   drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c   |  6 +-
>   drivers/gpu/drm/exynos/exynos_drm_gem.c       |  2 +-
>   drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c    | 12 +--
>   .../drm/i915/gem/selftests/i915_gem_dmabuf.c  | 20 ++---
>   drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c     |  8 +-
>   drivers/gpu/drm/tegra/gem.c                   | 27 +++----
>   drivers/infiniband/core/umem_dmabuf.c         | 11 +--
>   .../common/videobuf2/videobuf2-dma-contig.c   | 15 ++--
>   .../media/common/videobuf2/videobuf2-dma-sg.c | 12 +--
>   .../common/videobuf2/videobuf2-vmalloc.c      |  6 +-
>   .../platform/nvidia/tegra-vde/dmabuf-cache.c  | 12 +--
>   drivers/misc/fastrpc.c                        | 12 +--
>   drivers/xen/gntdev-dmabuf.c                   | 14 ++--
>   include/linux/dma-buf.h                       | 34 +++++----
>   21 files changed, 161 insertions(+), 152 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 44574fbe7482..d16237a6ffaa 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -795,7 +795,7 @@ static struct sg_table * __map_dma_buf(struct dma_buf_attachment *attach,
>   }
>   
>   /**
> - * dma_buf_dynamic_attach - Add the device to dma_buf's attachments list
> + * dma_buf_dynamic_attach_unlocked - Add the device to dma_buf's attachments list
>    * @dmabuf:		[in]	buffer to attach device to.
>    * @dev:		[in]	device to be attached.
>    * @importer_ops:	[in]	importer operations for the attachment
> @@ -817,9 +817,9 @@ static struct sg_table * __map_dma_buf(struct dma_buf_attachment *attach,
>    * indicated with the error code -EBUSY.
>    */
>   struct dma_buf_attachment *
> -dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev,
> -		       const struct dma_buf_attach_ops *importer_ops,
> -		       void *importer_priv)
> +dma_buf_dynamic_attach_unlocked(struct dma_buf *dmabuf, struct device *dev,
> +				const struct dma_buf_attach_ops *importer_ops,
> +				void *importer_priv)
>   {
>   	struct dma_buf_attachment *attach;
>   	int ret;
> @@ -892,25 +892,25 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev,
>   	if (dma_buf_is_dynamic(attach->dmabuf))
>   		dma_resv_unlock(attach->dmabuf->resv);
>   
> -	dma_buf_detach(dmabuf, attach);
> +	dma_buf_detach_unlocked(dmabuf, attach);
>   	return ERR_PTR(ret);
>   }
> -EXPORT_SYMBOL_NS_GPL(dma_buf_dynamic_attach, DMA_BUF);
> +EXPORT_SYMBOL_NS_GPL(dma_buf_dynamic_attach_unlocked, DMA_BUF);
>   
>   /**
> - * dma_buf_attach - Wrapper for dma_buf_dynamic_attach
> + * dma_buf_attach_unlocked - Wrapper for dma_buf_dynamic_attach
>    * @dmabuf:	[in]	buffer to attach device to.
>    * @dev:	[in]	device to be attached.
>    *
> - * Wrapper to call dma_buf_dynamic_attach() for drivers which still use a static
> - * mapping.
> + * Wrapper to call dma_buf_dynamic_attach_unlocked() for drivers which still
> + * use a static mapping.
>    */
> -struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
> -					  struct device *dev)
> +struct dma_buf_attachment *dma_buf_attach_unlocked(struct dma_buf *dmabuf,
> +						   struct device *dev)
>   {
> -	return dma_buf_dynamic_attach(dmabuf, dev, NULL, NULL);
> +	return dma_buf_dynamic_attach_unlocked(dmabuf, dev, NULL, NULL);
>   }
> -EXPORT_SYMBOL_NS_GPL(dma_buf_attach, DMA_BUF);
> +EXPORT_SYMBOL_NS_GPL(dma_buf_attach_unlocked, DMA_BUF);
>   
>   static void __unmap_dma_buf(struct dma_buf_attachment *attach,
>   			    struct sg_table *sg_table,
> @@ -923,7 +923,7 @@ static void __unmap_dma_buf(struct dma_buf_attachment *attach,
>   }
>   
>   /**
> - * dma_buf_detach - Remove the given attachment from dmabuf's attachments list
> + * dma_buf_detach_unlocked - Remove the given attachment from dmabuf's attachments list
>    * @dmabuf:	[in]	buffer to detach from.
>    * @attach:	[in]	attachment to be detached; is free'd after this call.
>    *
> @@ -931,7 +931,8 @@ static void __unmap_dma_buf(struct dma_buf_attachment *attach,
>    *
>    * Optionally this calls &dma_buf_ops.detach for device-specific detach.
>    */
> -void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach)
> +void dma_buf_detach_unlocked(struct dma_buf *dmabuf,
> +			     struct dma_buf_attachment *attach)
>   {
>   	if (WARN_ON(!dmabuf || !attach))
>   		return;
> @@ -956,14 +957,14 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach)
>   
>   	kfree(attach);
>   }
> -EXPORT_SYMBOL_NS_GPL(dma_buf_detach, DMA_BUF);
> +EXPORT_SYMBOL_NS_GPL(dma_buf_detach_unlocked, DMA_BUF);
>   
>   /**
>    * dma_buf_pin - Lock down the DMA-buf
>    * @attach:	[in]	attachment which should be pinned
>    *
> - * Only dynamic importers (who set up @attach with dma_buf_dynamic_attach()) may
> - * call this, and only for limited use cases like scanout and not for temporary
> + * Only dynamic importers (who set up @attach with dma_buf_dynamic_attach_unlocked())
> + * may call this, and only for limited use cases like scanout and not for temporary
>    * pin operations. It is not permitted to allow userspace to pin arbitrary
>    * amounts of buffers through this interface.
>    *
> @@ -1010,7 +1011,7 @@ void dma_buf_unpin(struct dma_buf_attachment *attach)
>   EXPORT_SYMBOL_NS_GPL(dma_buf_unpin, DMA_BUF);
>   
>   /**
> - * dma_buf_map_attachment - Returns the scatterlist table of the attachment;
> + * dma_buf_map_attachment_locked - Returns the scatterlist table of the attachment;
>    * mapped into _device_ address space. Is a wrapper for map_dma_buf() of the
>    * dma_buf_ops.
>    * @attach:	[in]	attachment whose scatterlist is to be returned
> @@ -1030,8 +1031,9 @@ EXPORT_SYMBOL_NS_GPL(dma_buf_unpin, DMA_BUF);
>    * Important: Dynamic importers must wait for the exclusive fence of the struct
>    * dma_resv attached to the DMA-BUF first.
>    */
> -struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
> -					enum dma_data_direction direction)
> +struct sg_table *
> +dma_buf_map_attachment_unlocked(struct dma_buf_attachment *attach,
> +				enum dma_data_direction direction)

The locking state of mapping and unmapping operations depend on if the 
attachment is dynamic or not.

So this here is not a good idea at all since it suggests that the 
function is always called without holding the lock.

>   {
>   	struct sg_table *sg_table;
>   	int r;
> @@ -1097,10 +1099,10 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach,
>   #endif /* CONFIG_DMA_API_DEBUG */
>   	return sg_table;
>   }
> -EXPORT_SYMBOL_NS_GPL(dma_buf_map_attachment, DMA_BUF);
> +EXPORT_SYMBOL_NS_GPL(dma_buf_map_attachment_unlocked, DMA_BUF);
>   
>   /**
> - * dma_buf_unmap_attachment - unmaps and decreases usecount of the buffer;might
> + * dma_buf_unmap_attachment_unlocked - unmaps and decreases usecount of the buffer;might
>    * deallocate the scatterlist associated. Is a wrapper for unmap_dma_buf() of
>    * dma_buf_ops.
>    * @attach:	[in]	attachment to unmap buffer from
> @@ -1109,9 +1111,9 @@ EXPORT_SYMBOL_NS_GPL(dma_buf_map_attachment, DMA_BUF);
>    *
>    * This unmaps a DMA mapping for @attached obtained by dma_buf_map_attachment().
>    */
> -void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
> -				struct sg_table *sg_table,
> -				enum dma_data_direction direction)
> +void dma_buf_unmap_attachment_unlocked(struct dma_buf_attachment *attach,
> +				       struct sg_table *sg_table,
> +				       enum dma_data_direction direction)

Same here of course.

Regards,
Christian.


>   {
>   	might_sleep();
>   
> @@ -1133,7 +1135,7 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach,
>   	    !IS_ENABLED(CONFIG_DMABUF_MOVE_NOTIFY))
>   		dma_buf_unpin(attach);
>   }
> -EXPORT_SYMBOL_NS_GPL(dma_buf_unmap_attachment, DMA_BUF);
> +EXPORT_SYMBOL_NS_GPL(dma_buf_unmap_attachment_unlocked, DMA_BUF);
>   
>   /**
>    * dma_buf_move_notify - notify attachments that DMA-buf is moving
> @@ -1330,7 +1332,7 @@ EXPORT_SYMBOL_NS_GPL(dma_buf_end_cpu_access, DMA_BUF);
>   
>   
>   /**
> - * dma_buf_mmap - Setup up a userspace mmap with the given vma
> + * dma_buf_mmap_unlocked - Setup up a userspace mmap with the given vma
>    * @dmabuf:	[in]	buffer that should back the vma
>    * @vma:	[in]	vma for the mmap
>    * @pgoff:	[in]	offset in pages where this mmap should start within the
> @@ -1343,8 +1345,8 @@ EXPORT_SYMBOL_NS_GPL(dma_buf_end_cpu_access, DMA_BUF);
>    *
>    * Can return negative error values, returns 0 on success.
>    */
> -int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma,
> -		 unsigned long pgoff)
> +int dma_buf_mmap_unlocked(struct dma_buf *dmabuf, struct vm_area_struct *vma,
> +			  unsigned long pgoff)
>   {
>   	if (WARN_ON(!dmabuf || !vma))
>   		return -EINVAL;
> @@ -1368,10 +1370,10 @@ int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma,
>   
>   	return dmabuf->ops->mmap(dmabuf, vma);
>   }
> -EXPORT_SYMBOL_NS_GPL(dma_buf_mmap, DMA_BUF);
> +EXPORT_SYMBOL_NS_GPL(dma_buf_mmap_unlocked, DMA_BUF);
>   
>   /**
> - * dma_buf_vmap - Create virtual mapping for the buffer object into kernel
> + * dma_buf_vmap_unlocked - Create virtual mapping for the buffer object into kernel
>    * address space. Same restrictions as for vmap and friends apply.
>    * @dmabuf:	[in]	buffer to vmap
>    * @map:	[out]	returns the vmap pointer
> @@ -1386,7 +1388,7 @@ EXPORT_SYMBOL_NS_GPL(dma_buf_mmap, DMA_BUF);
>    *
>    * Returns 0 on success, or a negative errno code otherwise.
>    */
> -int dma_buf_vmap(struct dma_buf *dmabuf, struct iosys_map *map)
> +int dma_buf_vmap_unlocked(struct dma_buf *dmabuf, struct iosys_map *map)
>   {
>   	struct iosys_map ptr;
>   	int ret = 0;
> @@ -1422,14 +1424,14 @@ int dma_buf_vmap(struct dma_buf *dmabuf, struct iosys_map *map)
>   	mutex_unlock(&dmabuf->lock);
>   	return ret;
>   }
> -EXPORT_SYMBOL_NS_GPL(dma_buf_vmap, DMA_BUF);
> +EXPORT_SYMBOL_NS_GPL(dma_buf_vmap_unlocked, DMA_BUF);
>   
>   /**
> - * dma_buf_vunmap - Unmap a vmap obtained by dma_buf_vmap.
> + * dma_buf_vunmap_unlocked - Unmap a vmap obtained by dma_buf_vmap.
>    * @dmabuf:	[in]	buffer to vunmap
>    * @map:	[in]	vmap pointer to vunmap
>    */
> -void dma_buf_vunmap(struct dma_buf *dmabuf, struct iosys_map *map)
> +void dma_buf_vunmap_unlocked(struct dma_buf *dmabuf, struct iosys_map *map)
>   {
>   	if (WARN_ON(!dmabuf))
>   		return;
> @@ -1446,7 +1448,7 @@ void dma_buf_vunmap(struct dma_buf *dmabuf, struct iosys_map *map)
>   	}
>   	mutex_unlock(&dmabuf->lock);
>   }
> -EXPORT_SYMBOL_NS_GPL(dma_buf_vunmap, DMA_BUF);
> +EXPORT_SYMBOL_NS_GPL(dma_buf_vunmap_unlocked, DMA_BUF);
>   
>   #ifdef CONFIG_DEBUG_FS
>   static int dma_buf_debug_show(struct seq_file *s, void *unused)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> index 782cbca37538..d9ed5a4fbc6f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> @@ -449,8 +449,8 @@ struct drm_gem_object *amdgpu_gem_prime_import(struct drm_device *dev,
>   	if (IS_ERR(obj))
>   		return obj;
>   
> -	attach = dma_buf_dynamic_attach(dma_buf, dev->dev,
> -					&amdgpu_dma_buf_attach_ops, obj);
> +	attach = dma_buf_dynamic_attach_unlocked(dma_buf, dev->dev,
> +						 &amdgpu_dma_buf_attach_ops, obj);
>   	if (IS_ERR(attach)) {
>   		drm_gem_object_put(obj);
>   		return ERR_CAST(attach);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 170935c294f5..e354ad140a0a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -883,7 +883,7 @@ static int amdgpu_ttm_backend_bind(struct ttm_device *bdev,
>   			struct sg_table *sgt;
>   
>   			attach = gtt->gobj->import_attach;
> -			sgt = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL);
> +			sgt = dma_buf_map_attachment_unlocked(attach, DMA_BIDIRECTIONAL);
>   			if (IS_ERR(sgt))
>   				return PTR_ERR(sgt);
>   
> @@ -1008,7 +1008,7 @@ static void amdgpu_ttm_backend_unbind(struct ttm_device *bdev,
>   		struct dma_buf_attachment *attach;
>   
>   		attach = gtt->gobj->import_attach;
> -		dma_buf_unmap_attachment(attach, ttm->sg, DMA_BIDIRECTIONAL);
> +		dma_buf_unmap_attachment_unlocked(attach, ttm->sg, DMA_BIDIRECTIONAL);
>   		ttm->sg = NULL;
>   	}
>   
> diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c
> index 147abf1a3968..f71f3b2d20e3 100644
> --- a/drivers/gpu/drm/armada/armada_gem.c
> +++ b/drivers/gpu/drm/armada/armada_gem.c
> @@ -66,8 +66,8 @@ void armada_gem_free_object(struct drm_gem_object *obj)
>   	if (dobj->obj.import_attach) {
>   		/* We only ever display imported data */
>   		if (dobj->sgt)
> -			dma_buf_unmap_attachment(dobj->obj.import_attach,
> -						 dobj->sgt, DMA_TO_DEVICE);
> +			dma_buf_unmap_attachment_unlocked(dobj->obj.import_attach,
> +							  dobj->sgt, DMA_TO_DEVICE);
>   		drm_prime_gem_destroy(&dobj->obj, NULL);
>   	}
>   
> @@ -364,7 +364,7 @@ int armada_gem_pwrite_ioctl(struct drm_device *dev, void *data,
>   
>   	if (args->offset > dobj->obj.size ||
>   	    args->size > dobj->obj.size - args->offset) {
> -		DRM_ERROR("invalid size: object size %u\n", dobj->obj.size);
> +		DRM_ERROR("invalid size: object size %zu\n", dobj->obj.size);
>   		ret = -EINVAL;
>   		goto unref;
>   	}
> @@ -514,13 +514,13 @@ armada_gem_prime_import(struct drm_device *dev, struct dma_buf *buf)
>   		}
>   	}
>   
> -	attach = dma_buf_attach(buf, dev->dev);
> +	attach = dma_buf_attach_unlocked(buf, dev->dev);
>   	if (IS_ERR(attach))
>   		return ERR_CAST(attach);
>   
>   	dobj = armada_gem_alloc_private_object(dev, buf->size);
>   	if (!dobj) {
> -		dma_buf_detach(buf, attach);
> +		dma_buf_detach_unlocked(buf, attach);
>   		return ERR_PTR(-ENOMEM);
>   	}
>   
> @@ -539,8 +539,8 @@ int armada_gem_map_import(struct armada_gem_object *dobj)
>   {
>   	int ret;
>   
> -	dobj->sgt = dma_buf_map_attachment(dobj->obj.import_attach,
> -					   DMA_TO_DEVICE);
> +	dobj->sgt = dma_buf_map_attachment_unlocked(dobj->obj.import_attach,
> +						    DMA_TO_DEVICE);
>   	if (IS_ERR(dobj->sgt)) {
>   		ret = PTR_ERR(dobj->sgt);
>   		dobj->sgt = NULL;
> diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
> index 42abee9a0f4f..ee3333f346b7 100644
> --- a/drivers/gpu/drm/drm_gem_cma_helper.c
> +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
> @@ -232,7 +232,7 @@ void drm_gem_cma_free(struct drm_gem_cma_object *cma_obj)
>   
>   	if (gem_obj->import_attach) {
>   		if (cma_obj->vaddr)
> -			dma_buf_vunmap(gem_obj->import_attach->dmabuf, &map);
> +			dma_buf_vunmap_unlocked(gem_obj->import_attach->dmabuf, &map);
>   		drm_prime_gem_destroy(gem_obj, cma_obj->sgt);
>   	} else if (cma_obj->vaddr) {
>   		if (cma_obj->map_noncoherent)
> @@ -581,7 +581,7 @@ drm_gem_cma_prime_import_sg_table_vmap(struct drm_device *dev,
>   	struct iosys_map map;
>   	int ret;
>   
> -	ret = dma_buf_vmap(attach->dmabuf, &map);
> +	ret = dma_buf_vmap_unlocked(attach->dmabuf, &map);
>   	if (ret) {
>   		DRM_ERROR("Failed to vmap PRIME buffer\n");
>   		return ERR_PTR(ret);
> @@ -589,7 +589,7 @@ drm_gem_cma_prime_import_sg_table_vmap(struct drm_device *dev,
>   
>   	obj = drm_gem_cma_prime_import_sg_table(dev, attach, sgt);
>   	if (IS_ERR(obj)) {
> -		dma_buf_vunmap(attach->dmabuf, &map);
> +		dma_buf_vunmap_unlocked(attach->dmabuf, &map);
>   		return obj;
>   	}
>   
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index 8ad0e02991ca..c5e7a84ead06 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -299,7 +299,7 @@ static int drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem,
>   	}
>   
>   	if (obj->import_attach) {
> -		ret = dma_buf_vmap(obj->import_attach->dmabuf, map);
> +		ret = dma_buf_vmap_unlocked(obj->import_attach->dmabuf, map);
>   		if (!ret) {
>   			if (WARN_ON(map->is_iomem)) {
>   				ret = -EIO;
> @@ -382,7 +382,7 @@ static void drm_gem_shmem_vunmap_locked(struct drm_gem_shmem_object *shmem,
>   		return;
>   
>   	if (obj->import_attach) {
> -		dma_buf_vunmap(obj->import_attach->dmabuf, map);
> +		dma_buf_vunmap_unlocked(obj->import_attach->dmabuf, map);
>   	} else {
>   		vunmap(shmem->vaddr);
>   		drm_gem_shmem_put_pages(shmem);
> @@ -617,7 +617,7 @@ int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct
>   		drm_gem_object_put(obj);
>   		vma->vm_private_data = NULL;
>   
> -		return dma_buf_mmap(obj->dma_buf, vma, 0);
> +		return dma_buf_mmap_unlocked(obj->dma_buf, vma, 0);
>   	}
>   
>   	ret = drm_gem_shmem_get_pages(shmem);
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index a3f180653b8b..b75ef1756873 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -930,13 +930,13 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev,
>   	if (!dev->driver->gem_prime_import_sg_table)
>   		return ERR_PTR(-EINVAL);
>   
> -	attach = dma_buf_attach(dma_buf, attach_dev);
> +	attach = dma_buf_attach_unlocked(dma_buf, attach_dev);
>   	if (IS_ERR(attach))
>   		return ERR_CAST(attach);
>   
>   	get_dma_buf(dma_buf);
>   
> -	sgt = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL);
> +	sgt = dma_buf_map_attachment_unlocked(attach, DMA_BIDIRECTIONAL);
>   	if (IS_ERR(sgt)) {
>   		ret = PTR_ERR(sgt);
>   		goto fail_detach;
> @@ -954,9 +954,9 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev,
>   	return obj;
>   
>   fail_unmap:
> -	dma_buf_unmap_attachment(attach, sgt, DMA_BIDIRECTIONAL);
> +	dma_buf_unmap_attachment_unlocked(attach, sgt, DMA_BIDIRECTIONAL);
>   fail_detach:
> -	dma_buf_detach(dma_buf, attach);
> +	dma_buf_detach_unlocked(dma_buf, attach);
>   	dma_buf_put(dma_buf);
>   
>   	return ERR_PTR(ret);
> @@ -1052,9 +1052,9 @@ void drm_prime_gem_destroy(struct drm_gem_object *obj, struct sg_table *sg)
>   
>   	attach = obj->import_attach;
>   	if (sg)
> -		dma_buf_unmap_attachment(attach, sg, DMA_BIDIRECTIONAL);
> +		dma_buf_unmap_attachment_unlocked(attach, sg, DMA_BIDIRECTIONAL);
>   	dma_buf = attach->dmabuf;
> -	dma_buf_detach(attach->dmabuf, attach);
> +	dma_buf_detach_unlocked(attach->dmabuf, attach);
>   	/* remove the reference */
>   	dma_buf_put(dma_buf);
>   }
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
> index 3fa2da149639..ae6c1eda0a72 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
> @@ -65,7 +65,7 @@ static void etnaviv_gem_prime_release(struct etnaviv_gem_object *etnaviv_obj)
>   	struct iosys_map map = IOSYS_MAP_INIT_VADDR(etnaviv_obj->vaddr);
>   
>   	if (etnaviv_obj->vaddr)
> -		dma_buf_vunmap(etnaviv_obj->base.import_attach->dmabuf, &map);
> +		dma_buf_vunmap_unlocked(etnaviv_obj->base.import_attach->dmabuf, &map);
>   
>   	/* Don't drop the pages for imported dmabuf, as they are not
>   	 * ours, just free the array we allocated:
> @@ -82,7 +82,7 @@ static void *etnaviv_gem_prime_vmap_impl(struct etnaviv_gem_object *etnaviv_obj)
>   
>   	lockdep_assert_held(&etnaviv_obj->lock);
>   
> -	ret = dma_buf_vmap(etnaviv_obj->base.import_attach->dmabuf, &map);
> +	ret = dma_buf_vmap_unlocked(etnaviv_obj->base.import_attach->dmabuf, &map);
>   	if (ret)
>   		return NULL;
>   	return map.vaddr;
> @@ -91,7 +91,7 @@ static void *etnaviv_gem_prime_vmap_impl(struct etnaviv_gem_object *etnaviv_obj)
>   static int etnaviv_gem_prime_mmap_obj(struct etnaviv_gem_object *etnaviv_obj,
>   		struct vm_area_struct *vma)
>   {
> -	return dma_buf_mmap(etnaviv_obj->base.dma_buf, vma, 0);
> +	return dma_buf_mmap_unlocked(etnaviv_obj->base.dma_buf, vma, 0);
>   }
>   
>   static const struct etnaviv_gem_ops etnaviv_gem_prime_ops = {
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> index 3e493f48e0d4..8e95a3c5caf8 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> @@ -366,7 +366,7 @@ static int exynos_drm_gem_mmap(struct drm_gem_object *obj, struct vm_area_struct
>   	int ret;
>   
>   	if (obj->import_attach)
> -		return dma_buf_mmap(obj->dma_buf, vma, 0);
> +		return dma_buf_mmap_unlocked(obj->dma_buf, vma, 0);
>   
>   	vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
>   
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> index f5062d0c6333..5ecea7df98b1 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> @@ -241,8 +241,8 @@ static int i915_gem_object_get_pages_dmabuf(struct drm_i915_gem_object *obj)
>   
>   	assert_object_held(obj);
>   
> -	pages = dma_buf_map_attachment(obj->base.import_attach,
> -				       DMA_BIDIRECTIONAL);
> +	pages = dma_buf_map_attachment_unlocked(obj->base.import_attach,
> +						DMA_BIDIRECTIONAL);
>   	if (IS_ERR(pages))
>   		return PTR_ERR(pages);
>   
> @@ -270,8 +270,8 @@ static int i915_gem_object_get_pages_dmabuf(struct drm_i915_gem_object *obj)
>   static void i915_gem_object_put_pages_dmabuf(struct drm_i915_gem_object *obj,
>   					     struct sg_table *pages)
>   {
> -	dma_buf_unmap_attachment(obj->base.import_attach, pages,
> -				 DMA_BIDIRECTIONAL);
> +	dma_buf_unmap_attachment_unlocked(obj->base.import_attach, pages,
> +					  DMA_BIDIRECTIONAL);
>   }
>   
>   static const struct drm_i915_gem_object_ops i915_gem_object_dmabuf_ops = {
> @@ -306,7 +306,7 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
>   		return ERR_PTR(-E2BIG);
>   
>   	/* need to attach */
> -	attach = dma_buf_attach(dma_buf, dev->dev);
> +	attach = dma_buf_attach_unlocked(dma_buf, dev->dev);
>   	if (IS_ERR(attach))
>   		return ERR_CAST(attach);
>   
> @@ -337,7 +337,7 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
>   	return &obj->base;
>   
>   fail_detach:
> -	dma_buf_detach(dma_buf, attach);
> +	dma_buf_detach_unlocked(dma_buf, attach);
>   	dma_buf_put(dma_buf);
>   
>   	return ERR_PTR(ret);
> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
> index 62c61af77a42..6053af920a22 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
> @@ -207,13 +207,13 @@ static int igt_dmabuf_import_same_driver(struct drm_i915_private *i915,
>   	i915_gem_object_unlock(import_obj);
>   
>   	/* Now try a fake an importer */
> -	import_attach = dma_buf_attach(dmabuf, obj->base.dev->dev);
> +	import_attach = dma_buf_attach_unlocked(dmabuf, obj->base.dev->dev);
>   	if (IS_ERR(import_attach)) {
>   		err = PTR_ERR(import_attach);
>   		goto out_import;
>   	}
>   
> -	st = dma_buf_map_attachment(import_attach, DMA_BIDIRECTIONAL);
> +	st = dma_buf_map_attachment_unlocked(import_attach, DMA_BIDIRECTIONAL);
>   	if (IS_ERR(st)) {
>   		err = PTR_ERR(st);
>   		goto out_detach;
> @@ -226,9 +226,9 @@ static int igt_dmabuf_import_same_driver(struct drm_i915_private *i915,
>   		timeout = -ETIME;
>   	}
>   	err = timeout > 0 ? 0 : timeout;
> -	dma_buf_unmap_attachment(import_attach, st, DMA_BIDIRECTIONAL);
> +	dma_buf_unmap_attachment_unlocked(import_attach, st, DMA_BIDIRECTIONAL);
>   out_detach:
> -	dma_buf_detach(dmabuf, import_attach);
> +	dma_buf_detach_unlocked(dmabuf, import_attach);
>   out_import:
>   	i915_gem_object_put(import_obj);
>   out_dmabuf:
> @@ -296,7 +296,7 @@ static int igt_dmabuf_import(void *arg)
>   		goto out_obj;
>   	}
>   
> -	err = dma_buf_vmap(dmabuf, &map);
> +	err = dma_buf_vmap_unlocked(dmabuf, &map);
>   	dma_map = err ? NULL : map.vaddr;
>   	if (!dma_map) {
>   		pr_err("dma_buf_vmap failed\n");
> @@ -337,7 +337,7 @@ static int igt_dmabuf_import(void *arg)
>   
>   	err = 0;
>   out_dma_map:
> -	dma_buf_vunmap(dmabuf, &map);
> +	dma_buf_vunmap_unlocked(dmabuf, &map);
>   out_obj:
>   	i915_gem_object_put(obj);
>   out_dmabuf:
> @@ -358,7 +358,7 @@ static int igt_dmabuf_import_ownership(void *arg)
>   	if (IS_ERR(dmabuf))
>   		return PTR_ERR(dmabuf);
>   
> -	err = dma_buf_vmap(dmabuf, &map);
> +	err = dma_buf_vmap_unlocked(dmabuf, &map);
>   	ptr = err ? NULL : map.vaddr;
>   	if (!ptr) {
>   		pr_err("dma_buf_vmap failed\n");
> @@ -367,7 +367,7 @@ static int igt_dmabuf_import_ownership(void *arg)
>   	}
>   
>   	memset(ptr, 0xc5, PAGE_SIZE);
> -	dma_buf_vunmap(dmabuf, &map);
> +	dma_buf_vunmap_unlocked(dmabuf, &map);
>   
>   	obj = to_intel_bo(i915_gem_prime_import(&i915->drm, dmabuf));
>   	if (IS_ERR(obj)) {
> @@ -418,7 +418,7 @@ static int igt_dmabuf_export_vmap(void *arg)
>   	}
>   	i915_gem_object_put(obj);
>   
> -	err = dma_buf_vmap(dmabuf, &map);
> +	err = dma_buf_vmap_unlocked(dmabuf, &map);
>   	ptr = err ? NULL : map.vaddr;
>   	if (!ptr) {
>   		pr_err("dma_buf_vmap failed\n");
> @@ -435,7 +435,7 @@ static int igt_dmabuf_export_vmap(void *arg)
>   	memset(ptr, 0xc5, dmabuf->size);
>   
>   	err = 0;
> -	dma_buf_vunmap(dmabuf, &map);
> +	dma_buf_vunmap_unlocked(dmabuf, &map);
>   out:
>   	dma_buf_put(dmabuf);
>   	return err;
> diff --git a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
> index 393f82e26927..a725a91c2ff9 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
> +++ b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
> @@ -119,13 +119,13 @@ struct drm_gem_object *omap_gem_prime_import(struct drm_device *dev,
>   		}
>   	}
>   
> -	attach = dma_buf_attach(dma_buf, dev->dev);
> +	attach = dma_buf_attach_unlocked(dma_buf, dev->dev);
>   	if (IS_ERR(attach))
>   		return ERR_CAST(attach);
>   
>   	get_dma_buf(dma_buf);
>   
> -	sgt = dma_buf_map_attachment(attach, DMA_TO_DEVICE);
> +	sgt = dma_buf_map_attachment_unlocked(attach, DMA_TO_DEVICE);
>   	if (IS_ERR(sgt)) {
>   		ret = PTR_ERR(sgt);
>   		goto fail_detach;
> @@ -142,9 +142,9 @@ struct drm_gem_object *omap_gem_prime_import(struct drm_device *dev,
>   	return obj;
>   
>   fail_unmap:
> -	dma_buf_unmap_attachment(attach, sgt, DMA_TO_DEVICE);
> +	dma_buf_unmap_attachment_unlocked(attach, sgt, DMA_TO_DEVICE);
>   fail_detach:
> -	dma_buf_detach(dma_buf, attach);
> +	dma_buf_detach_unlocked(dma_buf, attach);
>   	dma_buf_put(dma_buf);
>   
>   	return ERR_PTR(ret);
> diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c
> index 81991090adcc..bbfe196ff6f6 100644
> --- a/drivers/gpu/drm/tegra/gem.c
> +++ b/drivers/gpu/drm/tegra/gem.c
> @@ -78,15 +78,15 @@ static struct host1x_bo_mapping *tegra_bo_pin(struct device *dev, struct host1x_
>   	if (gem->import_attach) {
>   		struct dma_buf *buf = gem->import_attach->dmabuf;
>   
> -		map->attach = dma_buf_attach(buf, dev);
> +		map->attach = dma_buf_attach_unlocked(buf, dev);
>   		if (IS_ERR(map->attach)) {
>   			err = PTR_ERR(map->attach);
>   			goto free;
>   		}
>   
> -		map->sgt = dma_buf_map_attachment(map->attach, direction);
> +		map->sgt = dma_buf_map_attachment_unlocked(map->attach, direction);
>   		if (IS_ERR(map->sgt)) {
> -			dma_buf_detach(buf, map->attach);
> +			dma_buf_detach_unlocked(buf, map->attach);
>   			err = PTR_ERR(map->sgt);
>   			map->sgt = NULL;
>   			goto free;
> @@ -160,8 +160,9 @@ static struct host1x_bo_mapping *tegra_bo_pin(struct device *dev, struct host1x_
>   static void tegra_bo_unpin(struct host1x_bo_mapping *map)
>   {
>   	if (map->attach) {
> -		dma_buf_unmap_attachment(map->attach, map->sgt, map->direction);
> -		dma_buf_detach(map->attach->dmabuf, map->attach);
> +		dma_buf_unmap_attachment_unlocked(map->attach, map->sgt,
> +						  map->direction);
> +		dma_buf_detach_unlocked(map->attach->dmabuf, map->attach);
>   	} else {
>   		dma_unmap_sgtable(map->dev, map->sgt, map->direction, 0);
>   		sg_free_table(map->sgt);
> @@ -181,7 +182,7 @@ static void *tegra_bo_mmap(struct host1x_bo *bo)
>   	if (obj->vaddr) {
>   		return obj->vaddr;
>   	} else if (obj->gem.import_attach) {
> -		ret = dma_buf_vmap(obj->gem.import_attach->dmabuf, &map);
> +		ret = dma_buf_vmap_unlocked(obj->gem.import_attach->dmabuf, &map);
>   		return ret ? NULL : map.vaddr;
>   	} else {
>   		return vmap(obj->pages, obj->num_pages, VM_MAP,
> @@ -197,7 +198,7 @@ static void tegra_bo_munmap(struct host1x_bo *bo, void *addr)
>   	if (obj->vaddr)
>   		return;
>   	else if (obj->gem.import_attach)
> -		dma_buf_vunmap(obj->gem.import_attach->dmabuf, &map);
> +		dma_buf_vunmap_unlocked(obj->gem.import_attach->dmabuf, &map);
>   	else
>   		vunmap(addr);
>   }
> @@ -453,7 +454,7 @@ static struct tegra_bo *tegra_bo_import(struct drm_device *drm,
>   	if (IS_ERR(bo))
>   		return bo;
>   
> -	attach = dma_buf_attach(buf, drm->dev);
> +	attach = dma_buf_attach_unlocked(buf, drm->dev);
>   	if (IS_ERR(attach)) {
>   		err = PTR_ERR(attach);
>   		goto free;
> @@ -461,7 +462,7 @@ static struct tegra_bo *tegra_bo_import(struct drm_device *drm,
>   
>   	get_dma_buf(buf);
>   
> -	bo->sgt = dma_buf_map_attachment(attach, DMA_TO_DEVICE);
> +	bo->sgt = dma_buf_map_attachment_unlocked(attach, DMA_TO_DEVICE);
>   	if (IS_ERR(bo->sgt)) {
>   		err = PTR_ERR(bo->sgt);
>   		goto detach;
> @@ -479,9 +480,9 @@ static struct tegra_bo *tegra_bo_import(struct drm_device *drm,
>   
>   detach:
>   	if (!IS_ERR_OR_NULL(bo->sgt))
> -		dma_buf_unmap_attachment(attach, bo->sgt, DMA_TO_DEVICE);
> +		dma_buf_unmap_attachment_unlocked(attach, bo->sgt, DMA_TO_DEVICE);
>   
> -	dma_buf_detach(buf, attach);
> +	dma_buf_detach_unlocked(buf, attach);
>   	dma_buf_put(buf);
>   free:
>   	drm_gem_object_release(&bo->gem);
> @@ -508,8 +509,8 @@ void tegra_bo_free_object(struct drm_gem_object *gem)
>   		tegra_bo_iommu_unmap(tegra, bo);
>   
>   	if (gem->import_attach) {
> -		dma_buf_unmap_attachment(gem->import_attach, bo->sgt,
> -					 DMA_TO_DEVICE);
> +		dma_buf_unmap_attachment_unlocked(gem->import_attach, bo->sgt,
> +						  DMA_TO_DEVICE);
>   		drm_prime_gem_destroy(gem, NULL);
>   	} else {
>   		tegra_bo_free(gem->dev, bo);
> diff --git a/drivers/infiniband/core/umem_dmabuf.c b/drivers/infiniband/core/umem_dmabuf.c
> index fce80a4a5147..12b73e6ad054 100644
> --- a/drivers/infiniband/core/umem_dmabuf.c
> +++ b/drivers/infiniband/core/umem_dmabuf.c
> @@ -25,7 +25,8 @@ int ib_umem_dmabuf_map_pages(struct ib_umem_dmabuf *umem_dmabuf)
>   	if (umem_dmabuf->sgt)
>   		goto wait_fence;
>   
> -	sgt = dma_buf_map_attachment(umem_dmabuf->attach, DMA_BIDIRECTIONAL);
> +	sgt = dma_buf_map_attachment_unlocked(umem_dmabuf->attach,
> +					      DMA_BIDIRECTIONAL);
>   	if (IS_ERR(sgt))
>   		return PTR_ERR(sgt);
>   
> @@ -96,8 +97,8 @@ void ib_umem_dmabuf_unmap_pages(struct ib_umem_dmabuf *umem_dmabuf)
>   		umem_dmabuf->last_sg_trim = 0;
>   	}
>   
> -	dma_buf_unmap_attachment(umem_dmabuf->attach, umem_dmabuf->sgt,
> -				 DMA_BIDIRECTIONAL);
> +	dma_buf_unmap_attachment_unlocked(umem_dmabuf->attach, umem_dmabuf->sgt,
> +					  DMA_BIDIRECTIONAL);
>   
>   	umem_dmabuf->sgt = NULL;
>   }
> @@ -143,7 +144,7 @@ struct ib_umem_dmabuf *ib_umem_dmabuf_get(struct ib_device *device,
>   	if (!ib_umem_num_pages(umem))
>   		goto out_free_umem;
>   
> -	umem_dmabuf->attach = dma_buf_dynamic_attach(
> +	umem_dmabuf->attach = dma_buf_dynamic_attach_unlocked(
>   					dmabuf,
>   					device->dma_device,
>   					ops,
> @@ -222,7 +223,7 @@ void ib_umem_dmabuf_release(struct ib_umem_dmabuf *umem_dmabuf)
>   		dma_buf_unpin(umem_dmabuf->attach);
>   	dma_resv_unlock(dmabuf->resv);
>   
> -	dma_buf_detach(dmabuf, umem_dmabuf->attach);
> +	dma_buf_detach_unlocked(dmabuf, umem_dmabuf->attach);
>   	dma_buf_put(dmabuf);
>   	kfree(umem_dmabuf);
>   }
> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
> index 678b359717c4..de762dbdaf78 100644
> --- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
> +++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
> @@ -101,7 +101,7 @@ static void *vb2_dc_vaddr(struct vb2_buffer *vb, void *buf_priv)
>   	if (buf->db_attach) {
>   		struct iosys_map map;
>   
> -		if (!dma_buf_vmap(buf->db_attach->dmabuf, &map))
> +		if (!dma_buf_vmap_unlocked(buf->db_attach->dmabuf, &map))
>   			buf->vaddr = map.vaddr;
>   
>   		return buf->vaddr;
> @@ -711,7 +711,7 @@ static int vb2_dc_map_dmabuf(void *mem_priv)
>   	}
>   
>   	/* get the associated scatterlist for this buffer */
> -	sgt = dma_buf_map_attachment(buf->db_attach, buf->dma_dir);
> +	sgt = dma_buf_map_attachment_unlocked(buf->db_attach, buf->dma_dir);
>   	if (IS_ERR(sgt)) {
>   		pr_err("Error getting dmabuf scatterlist\n");
>   		return -EINVAL;
> @@ -722,7 +722,8 @@ static int vb2_dc_map_dmabuf(void *mem_priv)
>   	if (contig_size < buf->size) {
>   		pr_err("contiguous chunk is too small %lu/%lu\n",
>   		       contig_size, buf->size);
> -		dma_buf_unmap_attachment(buf->db_attach, sgt, buf->dma_dir);
> +		dma_buf_unmap_attachment_unlocked(buf->db_attach, sgt,
> +						  buf->dma_dir);
>   		return -EFAULT;
>   	}
>   
> @@ -750,10 +751,10 @@ static void vb2_dc_unmap_dmabuf(void *mem_priv)
>   	}
>   
>   	if (buf->vaddr) {
> -		dma_buf_vunmap(buf->db_attach->dmabuf, &map);
> +		dma_buf_vunmap_unlocked(buf->db_attach->dmabuf, &map);
>   		buf->vaddr = NULL;
>   	}
> -	dma_buf_unmap_attachment(buf->db_attach, sgt, buf->dma_dir);
> +	dma_buf_unmap_attachment_unlocked(buf->db_attach, sgt, buf->dma_dir);
>   
>   	buf->dma_addr = 0;
>   	buf->dma_sgt = NULL;
> @@ -768,7 +769,7 @@ static void vb2_dc_detach_dmabuf(void *mem_priv)
>   		vb2_dc_unmap_dmabuf(buf);
>   
>   	/* detach this attachment */
> -	dma_buf_detach(buf->db_attach->dmabuf, buf->db_attach);
> +	dma_buf_detach_unlocked(buf->db_attach->dmabuf, buf->db_attach);
>   	kfree(buf);
>   }
>   
> @@ -792,7 +793,7 @@ static void *vb2_dc_attach_dmabuf(struct vb2_buffer *vb, struct device *dev,
>   	buf->vb = vb;
>   
>   	/* create attachment for the dmabuf with the user device */
> -	dba = dma_buf_attach(dbuf, buf->dev);
> +	dba = dma_buf_attach_unlocked(dbuf, buf->dev);
>   	if (IS_ERR(dba)) {
>   		pr_err("failed to attach dmabuf\n");
>   		kfree(buf);
> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> index fa69158a65b1..39e11600304a 100644
> --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> @@ -309,7 +309,7 @@ static void *vb2_dma_sg_vaddr(struct vb2_buffer *vb, void *buf_priv)
>   
>   	if (!buf->vaddr) {
>   		if (buf->db_attach) {
> -			ret = dma_buf_vmap(buf->db_attach->dmabuf, &map);
> +			ret = dma_buf_vmap_unlocked(buf->db_attach->dmabuf, &map);
>   			buf->vaddr = ret ? NULL : map.vaddr;
>   		} else {
>   			buf->vaddr = vm_map_ram(buf->pages, buf->num_pages, -1);
> @@ -565,7 +565,7 @@ static int vb2_dma_sg_map_dmabuf(void *mem_priv)
>   	}
>   
>   	/* get the associated scatterlist for this buffer */
> -	sgt = dma_buf_map_attachment(buf->db_attach, buf->dma_dir);
> +	sgt = dma_buf_map_attachment_unlocked(buf->db_attach, buf->dma_dir);
>   	if (IS_ERR(sgt)) {
>   		pr_err("Error getting dmabuf scatterlist\n");
>   		return -EINVAL;
> @@ -594,10 +594,10 @@ static void vb2_dma_sg_unmap_dmabuf(void *mem_priv)
>   	}
>   
>   	if (buf->vaddr) {
> -		dma_buf_vunmap(buf->db_attach->dmabuf, &map);
> +		dma_buf_vunmap_unlocked(buf->db_attach->dmabuf, &map);
>   		buf->vaddr = NULL;
>   	}
> -	dma_buf_unmap_attachment(buf->db_attach, sgt, buf->dma_dir);
> +	dma_buf_unmap_attachment_unlocked(buf->db_attach, sgt, buf->dma_dir);
>   
>   	buf->dma_sgt = NULL;
>   }
> @@ -611,7 +611,7 @@ static void vb2_dma_sg_detach_dmabuf(void *mem_priv)
>   		vb2_dma_sg_unmap_dmabuf(buf);
>   
>   	/* detach this attachment */
> -	dma_buf_detach(buf->db_attach->dmabuf, buf->db_attach);
> +	dma_buf_detach_unlocked(buf->db_attach->dmabuf, buf->db_attach);
>   	kfree(buf);
>   }
>   
> @@ -633,7 +633,7 @@ static void *vb2_dma_sg_attach_dmabuf(struct vb2_buffer *vb, struct device *dev,
>   
>   	buf->dev = dev;
>   	/* create attachment for the dmabuf with the user device */
> -	dba = dma_buf_attach(dbuf, buf->dev);
> +	dba = dma_buf_attach_unlocked(dbuf, buf->dev);
>   	if (IS_ERR(dba)) {
>   		pr_err("failed to attach dmabuf\n");
>   		kfree(buf);
> diff --git a/drivers/media/common/videobuf2/videobuf2-vmalloc.c b/drivers/media/common/videobuf2/videobuf2-vmalloc.c
> index 948152f1596b..7831bf545874 100644
> --- a/drivers/media/common/videobuf2/videobuf2-vmalloc.c
> +++ b/drivers/media/common/videobuf2/videobuf2-vmalloc.c
> @@ -376,7 +376,7 @@ static int vb2_vmalloc_map_dmabuf(void *mem_priv)
>   	struct iosys_map map;
>   	int ret;
>   
> -	ret = dma_buf_vmap(buf->dbuf, &map);
> +	ret = dma_buf_vmap_unlocked(buf->dbuf, &map);
>   	if (ret)
>   		return -EFAULT;
>   	buf->vaddr = map.vaddr;
> @@ -389,7 +389,7 @@ static void vb2_vmalloc_unmap_dmabuf(void *mem_priv)
>   	struct vb2_vmalloc_buf *buf = mem_priv;
>   	struct iosys_map map = IOSYS_MAP_INIT_VADDR(buf->vaddr);
>   
> -	dma_buf_vunmap(buf->dbuf, &map);
> +	dma_buf_vunmap_unlocked(buf->dbuf, &map);
>   	buf->vaddr = NULL;
>   }
>   
> @@ -399,7 +399,7 @@ static void vb2_vmalloc_detach_dmabuf(void *mem_priv)
>   	struct iosys_map map = IOSYS_MAP_INIT_VADDR(buf->vaddr);
>   
>   	if (buf->vaddr)
> -		dma_buf_vunmap(buf->dbuf, &map);
> +		dma_buf_vunmap_unlocked(buf->dbuf, &map);
>   
>   	kfree(buf);
>   }
> diff --git a/drivers/media/platform/nvidia/tegra-vde/dmabuf-cache.c b/drivers/media/platform/nvidia/tegra-vde/dmabuf-cache.c
> index 69c346148070..58e4595f3a10 100644
> --- a/drivers/media/platform/nvidia/tegra-vde/dmabuf-cache.c
> +++ b/drivers/media/platform/nvidia/tegra-vde/dmabuf-cache.c
> @@ -38,8 +38,8 @@ static void tegra_vde_release_entry(struct tegra_vde_cache_entry *entry)
>   	if (entry->vde->domain)
>   		tegra_vde_iommu_unmap(entry->vde, entry->iova);
>   
> -	dma_buf_unmap_attachment(entry->a, entry->sgt, entry->dma_dir);
> -	dma_buf_detach(dmabuf, entry->a);
> +	dma_buf_unmap_attachment_unlocked(entry->a, entry->sgt, entry->dma_dir);
> +	dma_buf_detach_unlocked(dmabuf, entry->a);
>   	dma_buf_put(dmabuf);
>   
>   	list_del(&entry->list);
> @@ -95,14 +95,14 @@ int tegra_vde_dmabuf_cache_map(struct tegra_vde *vde,
>   		goto ref;
>   	}
>   
> -	attachment = dma_buf_attach(dmabuf, dev);
> +	attachment = dma_buf_attach_unlocked(dmabuf, dev);
>   	if (IS_ERR(attachment)) {
>   		dev_err(dev, "Failed to attach dmabuf\n");
>   		err = PTR_ERR(attachment);
>   		goto err_unlock;
>   	}
>   
> -	sgt = dma_buf_map_attachment(attachment, dma_dir);
> +	sgt = dma_buf_map_attachment_unlocked(attachment, dma_dir);
>   	if (IS_ERR(sgt)) {
>   		dev_err(dev, "Failed to get dmabufs sg_table\n");
>   		err = PTR_ERR(sgt);
> @@ -152,9 +152,9 @@ int tegra_vde_dmabuf_cache_map(struct tegra_vde *vde,
>   err_free:
>   	kfree(entry);
>   err_unmap:
> -	dma_buf_unmap_attachment(attachment, sgt, dma_dir);
> +	dma_buf_unmap_attachment_unlocked(attachment, sgt, dma_dir);
>   err_detach:
> -	dma_buf_detach(dmabuf, attachment);
> +	dma_buf_detach_unlocked(dmabuf, attachment);
>   err_unlock:
>   	mutex_unlock(&vde->map_lock);
>   
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 93ebd174d848..558e8056eb80 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -310,9 +310,9 @@ static void fastrpc_free_map(struct kref *ref)
>   				return;
>   			}
>   		}
> -		dma_buf_unmap_attachment(map->attach, map->table,
> -					 DMA_BIDIRECTIONAL);
> -		dma_buf_detach(map->buf, map->attach);
> +		dma_buf_unmap_attachment_unlocked(map->attach, map->table,
> +						  DMA_BIDIRECTIONAL);
> +		dma_buf_detach_unlocked(map->buf, map->attach);
>   		dma_buf_put(map->buf);
>   	}
>   
> @@ -719,14 +719,14 @@ static int fastrpc_map_create(struct fastrpc_user *fl, int fd,
>   		goto get_err;
>   	}
>   
> -	map->attach = dma_buf_attach(map->buf, sess->dev);
> +	map->attach = dma_buf_attach_unlocked(map->buf, sess->dev);
>   	if (IS_ERR(map->attach)) {
>   		dev_err(sess->dev, "Failed to attach dmabuf\n");
>   		err = PTR_ERR(map->attach);
>   		goto attach_err;
>   	}
>   
> -	map->table = dma_buf_map_attachment(map->attach, DMA_BIDIRECTIONAL);
> +	map->table = dma_buf_map_attachment_unlocked(map->attach, DMA_BIDIRECTIONAL);
>   	if (IS_ERR(map->table)) {
>   		err = PTR_ERR(map->table);
>   		goto map_err;
> @@ -763,7 +763,7 @@ static int fastrpc_map_create(struct fastrpc_user *fl, int fd,
>   	return 0;
>   
>   map_err:
> -	dma_buf_detach(map->buf, map->attach);
> +	dma_buf_detach_unlocked(map->buf, map->attach);
>   attach_err:
>   	dma_buf_put(map->buf);
>   get_err:
> diff --git a/drivers/xen/gntdev-dmabuf.c b/drivers/xen/gntdev-dmabuf.c
> index 940e5e9e8a54..5a50e2697e95 100644
> --- a/drivers/xen/gntdev-dmabuf.c
> +++ b/drivers/xen/gntdev-dmabuf.c
> @@ -592,7 +592,7 @@ dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv, struct device *dev,
>   	gntdev_dmabuf->priv = priv;
>   	gntdev_dmabuf->fd = fd;
>   
> -	attach = dma_buf_attach(dma_buf, dev);
> +	attach = dma_buf_attach_unlocked(dma_buf, dev);
>   	if (IS_ERR(attach)) {
>   		ret = ERR_CAST(attach);
>   		goto fail_free_obj;
> @@ -600,7 +600,7 @@ dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv, struct device *dev,
>   
>   	gntdev_dmabuf->u.imp.attach = attach;
>   
> -	sgt = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL);
> +	sgt = dma_buf_map_attachment_unlocked(attach, DMA_BIDIRECTIONAL);
>   	if (IS_ERR(sgt)) {
>   		ret = ERR_CAST(sgt);
>   		goto fail_detach;
> @@ -658,9 +658,9 @@ dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv, struct device *dev,
>   fail_end_access:
>   	dmabuf_imp_end_foreign_access(gntdev_dmabuf->u.imp.refs, count);
>   fail_unmap:
> -	dma_buf_unmap_attachment(attach, sgt, DMA_BIDIRECTIONAL);
> +	dma_buf_unmap_attachment_unlocked(attach, sgt, DMA_BIDIRECTIONAL);
>   fail_detach:
> -	dma_buf_detach(dma_buf, attach);
> +	dma_buf_detach_unlocked(dma_buf, attach);
>   fail_free_obj:
>   	dmabuf_imp_free_storage(gntdev_dmabuf);
>   fail_put:
> @@ -708,10 +708,10 @@ static int dmabuf_imp_release(struct gntdev_dmabuf_priv *priv, u32 fd)
>   	attach = gntdev_dmabuf->u.imp.attach;
>   
>   	if (gntdev_dmabuf->u.imp.sgt)
> -		dma_buf_unmap_attachment(attach, gntdev_dmabuf->u.imp.sgt,
> -					 DMA_BIDIRECTIONAL);
> +		dma_buf_unmap_attachment_unlocked(attach, gntdev_dmabuf->u.imp.sgt,
> +						  DMA_BIDIRECTIONAL);
>   	dma_buf = attach->dmabuf;
> -	dma_buf_detach(attach->dmabuf, attach);
> +	dma_buf_detach_unlocked(attach->dmabuf, attach);
>   	dma_buf_put(dma_buf);
>   
>   	dmabuf_imp_free_storage(gntdev_dmabuf);
> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> index 71731796c8c3..9ab09569dec1 100644
> --- a/include/linux/dma-buf.h
> +++ b/include/linux/dma-buf.h
> @@ -601,14 +601,16 @@ dma_buf_attachment_is_dynamic(struct dma_buf_attachment *attach)
>   	return !!attach->importer_ops;
>   }
>   
> -struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
> -					  struct device *dev);
> +struct dma_buf_attachment *dma_buf_attach_unlocked(struct dma_buf *dmabuf,
> +						   struct device *dev);
>   struct dma_buf_attachment *
> -dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev,
> -		       const struct dma_buf_attach_ops *importer_ops,
> -		       void *importer_priv);
> -void dma_buf_detach(struct dma_buf *dmabuf,
> -		    struct dma_buf_attachment *attach);
> +dma_buf_dynamic_attach_unlocked(struct dma_buf *dmabuf, struct device *dev,
> +				const struct dma_buf_attach_ops *importer_ops,
> +				void *importer_priv);
> +
> +void dma_buf_detach_unlocked(struct dma_buf *dmabuf,
> +			     struct dma_buf_attachment *attach);
> +
>   int dma_buf_pin(struct dma_buf_attachment *attach);
>   void dma_buf_unpin(struct dma_buf_attachment *attach);
>   
> @@ -618,18 +620,20 @@ int dma_buf_fd(struct dma_buf *dmabuf, int flags);
>   struct dma_buf *dma_buf_get(int fd);
>   void dma_buf_put(struct dma_buf *dmabuf);
>   
> -struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *,
> -					enum dma_data_direction);
> -void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct sg_table *,
> -				enum dma_data_direction);
> +struct sg_table *dma_buf_map_attachment_unlocked(struct dma_buf_attachment *,
> +						 enum dma_data_direction);
> +void dma_buf_unmap_attachment_unlocked(struct dma_buf_attachment *,
> +				       struct sg_table *,
> +				       enum dma_data_direction);
> +
>   void dma_buf_move_notify(struct dma_buf *dma_buf);
>   int dma_buf_begin_cpu_access(struct dma_buf *dma_buf,
>   			     enum dma_data_direction dir);
>   int dma_buf_end_cpu_access(struct dma_buf *dma_buf,
>   			   enum dma_data_direction dir);
>   
> -int dma_buf_mmap(struct dma_buf *, struct vm_area_struct *,
> -		 unsigned long);
> -int dma_buf_vmap(struct dma_buf *dmabuf, struct iosys_map *map);
> -void dma_buf_vunmap(struct dma_buf *dmabuf, struct iosys_map *map);
> +int dma_buf_mmap_unlocked(struct dma_buf *, struct vm_area_struct *,
> +			  unsigned long);
> +int dma_buf_vmap_unlocked(struct dma_buf *dmabuf, struct iosys_map *map);
> +void dma_buf_vunmap_unlocked(struct dma_buf *dmabuf, struct iosys_map *map);
>   #endif /* __DMA_BUF_H__ */


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v1 2/6] drm/gem: Take reservation lock for vmap/vunmap operations
  2022-07-15  0:52 ` [PATCH v1 2/6] drm/gem: Take reservation lock for vmap/vunmap operations Dmitry Osipenko
@ 2022-07-15  7:31   ` Christian König
  0 siblings, 0 replies; 18+ messages in thread
From: Christian König @ 2022-07-15  7:31 UTC (permalink / raw)
  To: Dmitry Osipenko, David Airlie, Gerd Hoffmann, Gurchetan Singh,
	Chia-I Wu, Daniel Vetter, Daniel Almeida, Gert Wollny,
	Gustavo Padovan, Daniel Stone, Tomeu Vizoso, Maarten Lankhorst,
	Maxime Ripard, Thomas Zimmermann, Rob Clark, Sumit Semwal, Pan,
	Xinhui, Thierry Reding, Tomasz Figa, Marek Szyprowski,
	Mauro Carvalho Chehab, Alex Deucher, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin,
	Thomas Hellström, Thomas Zimmermann
  Cc: dri-devel, linux-kernel, Dmitry Osipenko, linux-media,
	linaro-mm-sig, amd-gfx, intel-gfx, kernel, virtualization,
	spice-devel, linux-rdma, linux-arm-msm

Am 15.07.22 um 02:52 schrieb Dmitry Osipenko:
> The new common dma-buf locking convention will require buffer importers
> to hold the reservation lock around mapping operations. Make DRM GEM core
> to take the lock around the vmapping operations and update QXL and i915
> drivers to use the locked functions for the case where DRM core now holds
> the lock. This patch prepares DRM core and drivers to transition to the
> common dma-buf locking convention where vmapping of exported GEMs will
> be done under the held reservation lock.

Oh ^^ That looks like a bug fix to me!

At least drm_gem_ttm_vmap() and drm_gem_ttm_vunmap() already expected 
that they are called with the reservation lock held.

Otherwise you could mess up internal structures in the TTM buffer object 
while vmapping it.

I will take a deeper look at this.

Regards,
Christian.

>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> ---
>   drivers/gpu/drm/drm_client.c                 |  4 +--
>   drivers/gpu/drm/drm_gem.c                    | 28 ++++++++++++++++++++
>   drivers/gpu/drm/drm_gem_framebuffer_helper.c |  6 ++---
>   drivers/gpu/drm/drm_prime.c                  |  4 +--
>   drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c   |  2 +-
>   drivers/gpu/drm/qxl/qxl_object.c             | 17 ++++++------
>   drivers/gpu/drm/qxl/qxl_prime.c              |  4 +--
>   include/drm/drm_gem.h                        |  3 +++
>   8 files changed, 50 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
> index 2b230b4d6942..fbcb1e995384 100644
> --- a/drivers/gpu/drm/drm_client.c
> +++ b/drivers/gpu/drm/drm_client.c
> @@ -323,7 +323,7 @@ drm_client_buffer_vmap(struct drm_client_buffer *buffer,
>   	 * fd_install step out of the driver backend hooks, to make that
>   	 * final step optional for internal users.
>   	 */
> -	ret = drm_gem_vmap(buffer->gem, map);
> +	ret = drm_gem_vmap_unlocked(buffer->gem, map);
>   	if (ret)
>   		return ret;
>   
> @@ -345,7 +345,7 @@ void drm_client_buffer_vunmap(struct drm_client_buffer *buffer)
>   {
>   	struct iosys_map *map = &buffer->map;
>   
> -	drm_gem_vunmap(buffer->gem, map);
> +	drm_gem_vunmap_unlocked(buffer->gem, map);
>   }
>   EXPORT_SYMBOL(drm_client_buffer_vunmap);
>   
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index eb0c2d041f13..9769c33cad99 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1155,6 +1155,8 @@ void drm_gem_print_info(struct drm_printer *p, unsigned int indent,
>   
>   int drm_gem_pin(struct drm_gem_object *obj)
>   {
> +	dma_resv_assert_held(obj->resv);
> +
>   	if (obj->funcs->pin)
>   		return obj->funcs->pin(obj);
>   	else
> @@ -1163,6 +1165,8 @@ int drm_gem_pin(struct drm_gem_object *obj)
>   
>   void drm_gem_unpin(struct drm_gem_object *obj)
>   {
> +	dma_resv_assert_held(obj->resv);
> +
>   	if (obj->funcs->unpin)
>   		obj->funcs->unpin(obj);
>   }
> @@ -1171,6 +1175,8 @@ int drm_gem_vmap(struct drm_gem_object *obj, struct iosys_map *map)
>   {
>   	int ret;
>   
> +	dma_resv_assert_held(obj->resv);
> +
>   	if (!obj->funcs->vmap)
>   		return -EOPNOTSUPP;
>   
> @@ -1186,6 +1192,8 @@ EXPORT_SYMBOL(drm_gem_vmap);
>   
>   void drm_gem_vunmap(struct drm_gem_object *obj, struct iosys_map *map)
>   {
> +	dma_resv_assert_held(obj->resv);
> +
>   	if (iosys_map_is_null(map))
>   		return;
>   
> @@ -1197,6 +1205,26 @@ void drm_gem_vunmap(struct drm_gem_object *obj, struct iosys_map *map)
>   }
>   EXPORT_SYMBOL(drm_gem_vunmap);
>   
> +int drm_gem_vmap_unlocked(struct drm_gem_object *obj, struct iosys_map *map)
> +{
> +	int ret;
> +
> +	dma_resv_lock(obj->resv, NULL);
> +	ret = drm_gem_vmap(obj, map);
> +	dma_resv_unlock(obj->resv);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_gem_vmap_unlocked);
> +
> +void drm_gem_vunmap_unlocked(struct drm_gem_object *obj, struct iosys_map *map)
> +{
> +	dma_resv_lock(obj->resv, NULL);
> +	drm_gem_vunmap(obj, map);
> +	dma_resv_unlock(obj->resv);
> +}
> +EXPORT_SYMBOL(drm_gem_vunmap_unlocked);
> +
>   /**
>    * drm_gem_lock_reservations - Sets up the ww context and acquires
>    * the lock on an array of GEM objects.
> diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> index 880a4975507f..e35e224e6303 100644
> --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> @@ -354,7 +354,7 @@ int drm_gem_fb_vmap(struct drm_framebuffer *fb, struct iosys_map *map,
>   			ret = -EINVAL;
>   			goto err_drm_gem_vunmap;
>   		}
> -		ret = drm_gem_vmap(obj, &map[i]);
> +		ret = drm_gem_vmap_unlocked(obj, &map[i]);
>   		if (ret)
>   			goto err_drm_gem_vunmap;
>   	}
> @@ -376,7 +376,7 @@ int drm_gem_fb_vmap(struct drm_framebuffer *fb, struct iosys_map *map,
>   		obj = drm_gem_fb_get_obj(fb, i);
>   		if (!obj)
>   			continue;
> -		drm_gem_vunmap(obj, &map[i]);
> +		drm_gem_vunmap_unlocked(obj, &map[i]);
>   	}
>   	return ret;
>   }
> @@ -403,7 +403,7 @@ void drm_gem_fb_vunmap(struct drm_framebuffer *fb, struct iosys_map *map)
>   			continue;
>   		if (iosys_map_is_null(&map[i]))
>   			continue;
> -		drm_gem_vunmap(obj, &map[i]);
> +		drm_gem_vunmap_unlocked(obj, &map[i]);
>   	}
>   }
>   EXPORT_SYMBOL(drm_gem_fb_vunmap);
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index b75ef1756873..1bd234fd21a5 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -678,7 +678,7 @@ int drm_gem_dmabuf_vmap(struct dma_buf *dma_buf, struct iosys_map *map)
>   {
>   	struct drm_gem_object *obj = dma_buf->priv;
>   
> -	return drm_gem_vmap(obj, map);
> +	return drm_gem_vmap_unlocked(obj, map);
>   }
>   EXPORT_SYMBOL(drm_gem_dmabuf_vmap);
>   
> @@ -694,7 +694,7 @@ void drm_gem_dmabuf_vunmap(struct dma_buf *dma_buf, struct iosys_map *map)
>   {
>   	struct drm_gem_object *obj = dma_buf->priv;
>   
> -	drm_gem_vunmap(obj, map);
> +	drm_gem_vunmap_unlocked(obj, map);
>   }
>   EXPORT_SYMBOL(drm_gem_dmabuf_vunmap);
>   
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> index 5ecea7df98b1..cc54a5b1d6ae 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> @@ -72,7 +72,7 @@ static int i915_gem_dmabuf_vmap(struct dma_buf *dma_buf,
>   	struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf);
>   	void *vaddr;
>   
> -	vaddr = i915_gem_object_pin_map_unlocked(obj, I915_MAP_WB);
> +	vaddr = i915_gem_object_pin_map(obj, I915_MAP_WB);
>   	if (IS_ERR(vaddr))
>   		return PTR_ERR(vaddr);
>   
> diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
> index 695d9308d1f0..06a58dad5f5c 100644
> --- a/drivers/gpu/drm/qxl/qxl_object.c
> +++ b/drivers/gpu/drm/qxl/qxl_object.c
> @@ -168,9 +168,16 @@ int qxl_bo_vmap_locked(struct qxl_bo *bo, struct iosys_map *map)
>   		bo->map_count++;
>   		goto out;
>   	}
> -	r = ttm_bo_vmap(&bo->tbo, &bo->map);
> +
> +	r = __qxl_bo_pin(bo);
>   	if (r)
>   		return r;
> +
> +	r = ttm_bo_vmap(&bo->tbo, &bo->map);
> +	if (r) {
> +		__qxl_bo_unpin(bo);
> +		return r;
> +	}
>   	bo->map_count = 1;
>   
>   	/* TODO: Remove kptr in favor of map everywhere. */
> @@ -192,12 +199,6 @@ int qxl_bo_vmap(struct qxl_bo *bo, struct iosys_map *map)
>   	if (r)
>   		return r;
>   
> -	r = __qxl_bo_pin(bo);
> -	if (r) {
> -		qxl_bo_unreserve(bo);
> -		return r;
> -	}
> -
>   	r = qxl_bo_vmap_locked(bo, map);
>   	qxl_bo_unreserve(bo);
>   	return r;
> @@ -247,6 +248,7 @@ void qxl_bo_vunmap_locked(struct qxl_bo *bo)
>   		return;
>   	bo->kptr = NULL;
>   	ttm_bo_vunmap(&bo->tbo, &bo->map);
> +	__qxl_bo_unpin(bo);
>   }
>   
>   int qxl_bo_vunmap(struct qxl_bo *bo)
> @@ -258,7 +260,6 @@ int qxl_bo_vunmap(struct qxl_bo *bo)
>   		return r;
>   
>   	qxl_bo_vunmap_locked(bo);
> -	__qxl_bo_unpin(bo);
>   	qxl_bo_unreserve(bo);
>   	return 0;
>   }
> diff --git a/drivers/gpu/drm/qxl/qxl_prime.c b/drivers/gpu/drm/qxl/qxl_prime.c
> index 142d01415acb..9169c26357d3 100644
> --- a/drivers/gpu/drm/qxl/qxl_prime.c
> +++ b/drivers/gpu/drm/qxl/qxl_prime.c
> @@ -59,7 +59,7 @@ int qxl_gem_prime_vmap(struct drm_gem_object *obj, struct iosys_map *map)
>   	struct qxl_bo *bo = gem_to_qxl_bo(obj);
>   	int ret;
>   
> -	ret = qxl_bo_vmap(bo, map);
> +	ret = qxl_bo_vmap_locked(bo, map);
>   	if (ret < 0)
>   		return ret;
>   
> @@ -71,5 +71,5 @@ void qxl_gem_prime_vunmap(struct drm_gem_object *obj,
>   {
>   	struct qxl_bo *bo = gem_to_qxl_bo(obj);
>   
> -	qxl_bo_vunmap(bo);
> +	qxl_bo_vunmap_locked(bo);
>   }
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index 87cffc9efa85..bf3700415229 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -420,4 +420,7 @@ void drm_gem_unlock_reservations(struct drm_gem_object **objs, int count,
>   int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
>   			    u32 handle, u64 *offset);
>   
> +int drm_gem_vmap_unlocked(struct drm_gem_object *obj, struct iosys_map *map);
> +void drm_gem_vunmap_unlocked(struct drm_gem_object *obj, struct iosys_map *map);
> +
>   #endif /* __DRM_GEM_H__ */


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v1 1/6] dma-buf: Add _unlocked postfix to function names
  2022-07-15  7:19   ` Christian König
@ 2022-07-15  9:31     ` Dmitry Osipenko
  2022-07-15 11:04       ` Christian König
  0 siblings, 1 reply; 18+ messages in thread
From: Dmitry Osipenko @ 2022-07-15  9:31 UTC (permalink / raw)
  To: Christian König, David Airlie, Gerd Hoffmann,
	Gurchetan Singh, Chia-I Wu, Daniel Vetter, Daniel Almeida,
	Gert Wollny, Gustavo Padovan, Daniel Stone, Tomeu Vizoso,
	Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Rob Clark,
	Sumit Semwal, Pan, Xinhui, Thierry Reding, Tomasz Figa,
	Marek Szyprowski, Mauro Carvalho Chehab, Alex Deucher,
	Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin,
	Thomas Hellström
  Cc: dri-devel, linux-kernel, Dmitry Osipenko, linux-media,
	linaro-mm-sig, amd-gfx, intel-gfx, kernel, virtualization,
	spice-devel, linux-rdma, linux-arm-msm

On 7/15/22 10:19, Christian König wrote:
>> -struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment
>> *attach,
>> -                    enum dma_data_direction direction)
>> +struct sg_table *
>> +dma_buf_map_attachment_unlocked(struct dma_buf_attachment *attach,
>> +                enum dma_data_direction direction)
> 
> The locking state of mapping and unmapping operations depend on if the
> attachment is dynamic or not.
> 
> So this here is not a good idea at all since it suggests that the
> function is always called without holding the lock.

I had the same thought while was working on this patch and initially was
thinking about adding an "unlocked" alias to dma_buf_map_attachment().
In the end I decided that it will create even more confusion and it's
simpler just to rename this func here since there are only two drivers
using the dynamic mapping.

Do you have suggestions how to improve it?

-- 
Best regards,
Dmitry

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v1 1/6] dma-buf: Add _unlocked postfix to function names
  2022-07-15  9:31     ` Dmitry Osipenko
@ 2022-07-15 11:04       ` Christian König
  0 siblings, 0 replies; 18+ messages in thread
From: Christian König @ 2022-07-15 11:04 UTC (permalink / raw)
  To: Dmitry Osipenko, David Airlie, Gerd Hoffmann, Gurchetan Singh,
	Chia-I Wu, Daniel Vetter, Daniel Almeida, Gert Wollny,
	Gustavo Padovan, Daniel Stone, Tomeu Vizoso, Maarten Lankhorst,
	Maxime Ripard, Thomas Zimmermann, Rob Clark, Sumit Semwal, Pan,
	Xinhui, Thierry Reding, Tomasz Figa, Marek Szyprowski,
	Mauro Carvalho Chehab, Alex Deucher, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin,
	Thomas Hellström
  Cc: dri-devel, linux-kernel, Dmitry Osipenko, linux-media,
	linaro-mm-sig, amd-gfx, intel-gfx, kernel, virtualization,
	spice-devel, linux-rdma, linux-arm-msm

Am 15.07.22 um 11:31 schrieb Dmitry Osipenko:
> On 7/15/22 10:19, Christian König wrote:
>>> -struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment
>>> *attach,
>>> -                    enum dma_data_direction direction)
>>> +struct sg_table *
>>> +dma_buf_map_attachment_unlocked(struct dma_buf_attachment *attach,
>>> +                enum dma_data_direction direction)
>> The locking state of mapping and unmapping operations depend on if the
>> attachment is dynamic or not.
>>
>> So this here is not a good idea at all since it suggests that the
>> function is always called without holding the lock.
> I had the same thought while was working on this patch and initially was
> thinking about adding an "unlocked" alias to dma_buf_map_attachment().
> In the end I decided that it will create even more confusion and it's
> simpler just to rename this func here since there are only two drivers
> using the dynamic mapping.
>
> Do you have suggestions how to improve it?

Just keep it as it is. The ultimative goal is to switch all drivers over 
to the dynamic mapping or at least over to assume that the map/unmap 
callbacks are called with the lock held.

Regards,
Christian.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v1 0/6] Move all drivers to a common dma-buf locking convention
  2022-07-15  0:52 [PATCH v1 0/6] Move all drivers to a common dma-buf locking convention Dmitry Osipenko
                   ` (5 preceding siblings ...)
  2022-07-15  0:52 ` [PATCH v1 6/6] dma-buf: Remove internal lock Dmitry Osipenko
@ 2022-07-19  9:13 ` Tomasz Figa
  6 siblings, 0 replies; 18+ messages in thread
From: Tomasz Figa @ 2022-07-19  9:13 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: David Airlie, Gerd Hoffmann, Gurchetan Singh, Chia-I Wu,
	Daniel Vetter, Daniel Almeida, Gert Wollny, Gustavo Padovan,
	Daniel Stone, Tomeu Vizoso, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, Rob Clark, Sumit Semwal, Christian König,
	Pan, Xinhui, Thierry Reding, Marek Szyprowski,
	Mauro Carvalho Chehab, Alex Deucher, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin,
	Thomas Hellström, dri-devel, linux-kernel, Dmitry Osipenko,
	linux-media, linaro-mm-sig, amd-gfx, intel-gfx, kernel,
	virtualization, spice-devel, linux-rdma, linux-arm-msm

On Fri, Jul 15, 2022 at 9:53 AM Dmitry Osipenko
<dmitry.osipenko@collabora.com> wrote:
>
> Hello,
>
> This series moves all drivers to a dynamic dma-buf locking specification.
> From now on all dma-buf importers are made responsible for holding
> dma-buf's reservation lock around all operations performed over dma-bufs.
> This common locking convention allows us to utilize reservation lock more
> broadly around kernel without fearing of potential dead locks.
>
> This patchset passes all i915 selftests. It was also tested using VirtIO,
> Panfrost, Lima and Tegra drivers. I tested cases of display+GPU,
> display+V4L and GPU+V4L dma-buf sharing, which covers majority of kernel
> drivers since rest of the drivers share same or similar code paths.
>
> This is a continuation of [1] where Christian König asked to factor out
> the dma-buf locking changes into separate series.
>
> [1] https://lore.kernel.org/dri-devel/20220526235040.678984-1-dmitry.osipenko@collabora.com/
>
> Dmitry Osipenko (6):
>   dma-buf: Add _unlocked postfix to function names
>   drm/gem: Take reservation lock for vmap/vunmap operations
>   dma-buf: Move all dma-bufs to dynamic locking specification
>   dma-buf: Acquire wait-wound context on attachment
>   media: videobuf2: Stop using internal dma-buf lock
>   dma-buf: Remove internal lock
>
>  drivers/dma-buf/dma-buf.c                     | 198 +++++++++++-------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c   |   4 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c       |   4 +-
>  drivers/gpu/drm/armada/armada_gem.c           |  14 +-
>  drivers/gpu/drm/drm_client.c                  |   4 +-
>  drivers/gpu/drm/drm_gem.c                     |  28 +++
>  drivers/gpu/drm/drm_gem_cma_helper.c          |   6 +-
>  drivers/gpu/drm/drm_gem_framebuffer_helper.c  |   6 +-
>  drivers/gpu/drm/drm_gem_shmem_helper.c        |   6 +-
>  drivers/gpu/drm/drm_prime.c                   |  12 +-
>  drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c   |   6 +-
>  drivers/gpu/drm/exynos/exynos_drm_gem.c       |   2 +-
>  drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c    |  20 +-
>  .../gpu/drm/i915/gem/i915_gem_execbuffer.c    |   2 +-
>  drivers/gpu/drm/i915/gem/i915_gem_object.h    |   6 +-
>  .../drm/i915/gem/selftests/i915_gem_dmabuf.c  |  20 +-
>  drivers/gpu/drm/i915/i915_gem_evict.c         |   2 +-
>  drivers/gpu/drm/i915/i915_gem_ww.c            |  26 ++-
>  drivers/gpu/drm/i915/i915_gem_ww.h            |  15 +-
>  drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c     |   8 +-
>  drivers/gpu/drm/qxl/qxl_object.c              |  17 +-
>  drivers/gpu/drm/qxl/qxl_prime.c               |   4 +-
>  drivers/gpu/drm/tegra/gem.c                   |  27 +--
>  drivers/infiniband/core/umem_dmabuf.c         |  11 +-
>  .../common/videobuf2/videobuf2-dma-contig.c   |  26 +--
>  .../media/common/videobuf2/videobuf2-dma-sg.c |  23 +-
>  .../common/videobuf2/videobuf2-vmalloc.c      |  17 +-

For the videobuf2 changes:

Acked-by: Tomasz Figa <tfiga@chromium.org>

Best regards,
Tomasz

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v1 4/6] dma-buf: Acquire wait-wound context on attachment
  2022-07-15  6:59     ` Dmitry Osipenko
@ 2022-07-19 20:05       ` Dmitry Osipenko
  2022-07-20  8:29         ` Christian König
  0 siblings, 1 reply; 18+ messages in thread
From: Dmitry Osipenko @ 2022-07-19 20:05 UTC (permalink / raw)
  To: Christian König, David Airlie, Gerd Hoffmann,
	Gurchetan Singh, Chia-I Wu, Daniel Vetter, Daniel Almeida,
	Gert Wollny, Gustavo Padovan, Daniel Stone, Tomeu Vizoso,
	Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Rob Clark,
	Sumit Semwal, Pan, Xinhui, Thierry Reding, Tomasz Figa,
	Marek Szyprowski, Mauro Carvalho Chehab, Alex Deucher,
	Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin,
	Thomas Hellström
  Cc: dri-devel, linux-kernel, Dmitry Osipenko, linux-media,
	linaro-mm-sig, amd-gfx, intel-gfx, kernel, virtualization,
	spice-devel, linux-rdma, linux-arm-msm

On 7/15/22 09:59, Dmitry Osipenko wrote:
> On 7/15/22 09:50, Christian König wrote:
>> Am 15.07.22 um 02:52 schrieb Dmitry Osipenko:
>>> Intel i915 GPU driver uses wait-wound mutex to lock multiple GEMs on the
>>> attachment to the i915 dma-buf. In order to let all drivers utilize
>>> shared
>>> wait-wound context during attachment in a general way, make dma-buf
>>> core to
>>> acquire the ww context internally for the attachment operation and update
>>> i915 driver to use the importer's ww context instead of the internal one.
>>>
>>>  From now on all dma-buf exporters shall use the importer's ww context
>>> for
>>> the attachment operation.
>>>
>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>>> ---
>>>   drivers/dma-buf/dma-buf.c                     |  8 +++++-
>>>   drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c    |  2 +-
>>>   .../gpu/drm/i915/gem/i915_gem_execbuffer.c    |  2 +-
>>>   drivers/gpu/drm/i915/gem/i915_gem_object.h    |  6 ++---
>>>   drivers/gpu/drm/i915/i915_gem_evict.c         |  2 +-
>>>   drivers/gpu/drm/i915/i915_gem_ww.c            | 26 +++++++++++++++----
>>>   drivers/gpu/drm/i915/i915_gem_ww.h            | 15 +++++++++--
>>>   7 files changed, 47 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>>> index 0ee588276534..37545ecb845a 100644
>>> --- a/drivers/dma-buf/dma-buf.c
>>> +++ b/drivers/dma-buf/dma-buf.c
>>> @@ -807,6 +807,8 @@ static struct sg_table * __map_dma_buf(struct
>>> dma_buf_attachment *attach,
>>>    * Optionally this calls &dma_buf_ops.attach to allow
>>> device-specific attach
>>>    * functionality.
>>>    *
>>> + * Exporters shall use ww_ctx acquired by this function.
>>> + *
>>>    * Returns:
>>>    *
>>>    * A pointer to newly created &dma_buf_attachment on success, or a
>>> negative
>>> @@ -822,6 +824,7 @@ dma_buf_dynamic_attach_unlocked(struct dma_buf
>>> *dmabuf, struct device *dev,
>>>                   void *importer_priv)
>>>   {
>>>       struct dma_buf_attachment *attach;
>>> +    struct ww_acquire_ctx ww_ctx;
>>>       int ret;
>>>         if (WARN_ON(!dmabuf || !dev))
>>> @@ -841,7 +844,8 @@ dma_buf_dynamic_attach_unlocked(struct dma_buf
>>> *dmabuf, struct device *dev,
>>>       attach->importer_ops = importer_ops;
>>>       attach->importer_priv = importer_priv;
>>>   -    dma_resv_lock(dmabuf->resv, NULL);
>>> +    ww_acquire_init(&ww_ctx, &reservation_ww_class);
>>> +    dma_resv_lock(dmabuf->resv, &ww_ctx);
>>
>> That won't work like this. The core property of a WW context is that you
>> need to unwind all the locks and re-quire them with the contended one
>> first.
>>
>> When you statically lock the imported one here you can't do that any more.
> 
> You're right. I felt that something is missing here, but couldn't
> notice. I'll think more about this and enable
> CONFIG_DEBUG_WW_MUTEX_SLOWPATH. Thank you!
> 

Christian, do you think we could make an excuse for the attach()
callback and make the exporter responsible for taking the resv lock? It
will be inconsistent with the rest of the callbacks, where importer
takes the lock, but it will be the simplest and least invasive solution.
It's very messy to do a cross-driver ww locking, I don't think it's the
right approach.

-- 
Best regards,
Dmitry

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v1 4/6] dma-buf: Acquire wait-wound context on attachment
  2022-07-19 20:05       ` Dmitry Osipenko
@ 2022-07-20  8:29         ` Christian König
  2022-07-20 12:18           ` Dmitry Osipenko
  0 siblings, 1 reply; 18+ messages in thread
From: Christian König @ 2022-07-20  8:29 UTC (permalink / raw)
  To: Dmitry Osipenko, David Airlie, Gerd Hoffmann, Gurchetan Singh,
	Chia-I Wu, Daniel Vetter, Daniel Almeida, Gert Wollny,
	Gustavo Padovan, Daniel Stone, Tomeu Vizoso, Maarten Lankhorst,
	Maxime Ripard, Thomas Zimmermann, Rob Clark, Sumit Semwal, Pan,
	Xinhui, Thierry Reding, Tomasz Figa, Marek Szyprowski,
	Mauro Carvalho Chehab, Alex Deucher, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin,
	Thomas Hellström
  Cc: dri-devel, linux-kernel, Dmitry Osipenko, linux-media,
	linaro-mm-sig, amd-gfx, intel-gfx, kernel, virtualization,
	spice-devel, linux-rdma, linux-arm-msm

Am 19.07.22 um 22:05 schrieb Dmitry Osipenko:
> On 7/15/22 09:59, Dmitry Osipenko wrote:
>> On 7/15/22 09:50, Christian König wrote:
>>> Am 15.07.22 um 02:52 schrieb Dmitry Osipenko:
>>>> Intel i915 GPU driver uses wait-wound mutex to lock multiple GEMs on the
>>>> attachment to the i915 dma-buf. In order to let all drivers utilize
>>>> shared
>>>> wait-wound context during attachment in a general way, make dma-buf
>>>> core to
>>>> acquire the ww context internally for the attachment operation and update
>>>> i915 driver to use the importer's ww context instead of the internal one.
>>>>
>>>>   From now on all dma-buf exporters shall use the importer's ww context
>>>> for
>>>> the attachment operation.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>>>> ---
>>>>    drivers/dma-buf/dma-buf.c                     |  8 +++++-
>>>>    drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c    |  2 +-
>>>>    .../gpu/drm/i915/gem/i915_gem_execbuffer.c    |  2 +-
>>>>    drivers/gpu/drm/i915/gem/i915_gem_object.h    |  6 ++---
>>>>    drivers/gpu/drm/i915/i915_gem_evict.c         |  2 +-
>>>>    drivers/gpu/drm/i915/i915_gem_ww.c            | 26 +++++++++++++++----
>>>>    drivers/gpu/drm/i915/i915_gem_ww.h            | 15 +++++++++--
>>>>    7 files changed, 47 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>>>> index 0ee588276534..37545ecb845a 100644
>>>> --- a/drivers/dma-buf/dma-buf.c
>>>> +++ b/drivers/dma-buf/dma-buf.c
>>>> @@ -807,6 +807,8 @@ static struct sg_table * __map_dma_buf(struct
>>>> dma_buf_attachment *attach,
>>>>     * Optionally this calls &dma_buf_ops.attach to allow
>>>> device-specific attach
>>>>     * functionality.
>>>>     *
>>>> + * Exporters shall use ww_ctx acquired by this function.
>>>> + *
>>>>     * Returns:
>>>>     *
>>>>     * A pointer to newly created &dma_buf_attachment on success, or a
>>>> negative
>>>> @@ -822,6 +824,7 @@ dma_buf_dynamic_attach_unlocked(struct dma_buf
>>>> *dmabuf, struct device *dev,
>>>>                    void *importer_priv)
>>>>    {
>>>>        struct dma_buf_attachment *attach;
>>>> +    struct ww_acquire_ctx ww_ctx;
>>>>        int ret;
>>>>          if (WARN_ON(!dmabuf || !dev))
>>>> @@ -841,7 +844,8 @@ dma_buf_dynamic_attach_unlocked(struct dma_buf
>>>> *dmabuf, struct device *dev,
>>>>        attach->importer_ops = importer_ops;
>>>>        attach->importer_priv = importer_priv;
>>>>    -    dma_resv_lock(dmabuf->resv, NULL);
>>>> +    ww_acquire_init(&ww_ctx, &reservation_ww_class);
>>>> +    dma_resv_lock(dmabuf->resv, &ww_ctx);
>>> That won't work like this. The core property of a WW context is that you
>>> need to unwind all the locks and re-quire them with the contended one
>>> first.
>>>
>>> When you statically lock the imported one here you can't do that any more.
>> You're right. I felt that something is missing here, but couldn't
>> notice. I'll think more about this and enable
>> CONFIG_DEBUG_WW_MUTEX_SLOWPATH. Thank you!
>>
> Christian, do you think we could make an excuse for the attach()
> callback and make the exporter responsible for taking the resv lock? It
> will be inconsistent with the rest of the callbacks, where importer
> takes the lock, but it will be the simplest and least invasive solution.
> It's very messy to do a cross-driver ww locking, I don't think it's the
> right approach.

So to summarize the following calls will require that the caller hold 
the resv lock:
1. dma_buf_pin()/dma_buf_unpin()
2. dma_buf_map_attachment()/dma_buf_unmap_attachment()
3. dma_buf_vmap()/dma_buf_vunmap()
4. dma_buf_move_notify()

The following calls require that caller does not held the resv lock:
1. dma_buf_attach()/dma_buf_dynamic_attach()/dma_buf_detach()
2. dma_buf_export()/dma_buf_fd()
3. dma_buf_get()/dma_buf_put()
4. dma_buf_begin_cpu_access()/dma_buf_end_cpu_access()

If that's correct than that would work for me as well, but we should 
probably document this.

Or let me ask the other way around: What calls exactly do you need to 
change to solve your original issue? That was vmap/vunmap, wasn't it? If 
yes then let's concentrate on those for the moment.

Regards,
Christian.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v1 4/6] dma-buf: Acquire wait-wound context on attachment
  2022-07-20  8:29         ` Christian König
@ 2022-07-20 12:18           ` Dmitry Osipenko
  0 siblings, 0 replies; 18+ messages in thread
From: Dmitry Osipenko @ 2022-07-20 12:18 UTC (permalink / raw)
  To: Christian König, David Airlie, Gerd Hoffmann,
	Gurchetan Singh, Chia-I Wu, Daniel Vetter, Daniel Almeida,
	Gert Wollny, Gustavo Padovan, Daniel Stone, Tomeu Vizoso,
	Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Rob Clark,
	Sumit Semwal, Pan, Xinhui, Thierry Reding, Tomasz Figa,
	Marek Szyprowski, Mauro Carvalho Chehab, Alex Deucher,
	Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin,
	Thomas Hellström
  Cc: dri-devel, linux-kernel, Dmitry Osipenko, linux-media,
	linaro-mm-sig, amd-gfx, intel-gfx, kernel, virtualization,
	spice-devel, linux-rdma, linux-arm-msm

On 7/20/22 11:29, Christian König wrote:
> Am 19.07.22 um 22:05 schrieb Dmitry Osipenko:
>> On 7/15/22 09:59, Dmitry Osipenko wrote:
>>> On 7/15/22 09:50, Christian König wrote:
>>>> Am 15.07.22 um 02:52 schrieb Dmitry Osipenko:
>>>>> Intel i915 GPU driver uses wait-wound mutex to lock multiple GEMs
>>>>> on the
>>>>> attachment to the i915 dma-buf. In order to let all drivers utilize
>>>>> shared
>>>>> wait-wound context during attachment in a general way, make dma-buf
>>>>> core to
>>>>> acquire the ww context internally for the attachment operation and
>>>>> update
>>>>> i915 driver to use the importer's ww context instead of the
>>>>> internal one.
>>>>>
>>>>>   From now on all dma-buf exporters shall use the importer's ww
>>>>> context
>>>>> for
>>>>> the attachment operation.
>>>>>
>>>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>>>>> ---
>>>>>    drivers/dma-buf/dma-buf.c                     |  8 +++++-
>>>>>    drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c    |  2 +-
>>>>>    .../gpu/drm/i915/gem/i915_gem_execbuffer.c    |  2 +-
>>>>>    drivers/gpu/drm/i915/gem/i915_gem_object.h    |  6 ++---
>>>>>    drivers/gpu/drm/i915/i915_gem_evict.c         |  2 +-
>>>>>    drivers/gpu/drm/i915/i915_gem_ww.c            | 26
>>>>> +++++++++++++++----
>>>>>    drivers/gpu/drm/i915/i915_gem_ww.h            | 15 +++++++++--
>>>>>    7 files changed, 47 insertions(+), 14 deletions(-)
>>>>>
>>>>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>>>>> index 0ee588276534..37545ecb845a 100644
>>>>> --- a/drivers/dma-buf/dma-buf.c
>>>>> +++ b/drivers/dma-buf/dma-buf.c
>>>>> @@ -807,6 +807,8 @@ static struct sg_table * __map_dma_buf(struct
>>>>> dma_buf_attachment *attach,
>>>>>     * Optionally this calls &dma_buf_ops.attach to allow
>>>>> device-specific attach
>>>>>     * functionality.
>>>>>     *
>>>>> + * Exporters shall use ww_ctx acquired by this function.
>>>>> + *
>>>>>     * Returns:
>>>>>     *
>>>>>     * A pointer to newly created &dma_buf_attachment on success, or a
>>>>> negative
>>>>> @@ -822,6 +824,7 @@ dma_buf_dynamic_attach_unlocked(struct dma_buf
>>>>> *dmabuf, struct device *dev,
>>>>>                    void *importer_priv)
>>>>>    {
>>>>>        struct dma_buf_attachment *attach;
>>>>> +    struct ww_acquire_ctx ww_ctx;
>>>>>        int ret;
>>>>>          if (WARN_ON(!dmabuf || !dev))
>>>>> @@ -841,7 +844,8 @@ dma_buf_dynamic_attach_unlocked(struct dma_buf
>>>>> *dmabuf, struct device *dev,
>>>>>        attach->importer_ops = importer_ops;
>>>>>        attach->importer_priv = importer_priv;
>>>>>    -    dma_resv_lock(dmabuf->resv, NULL);
>>>>> +    ww_acquire_init(&ww_ctx, &reservation_ww_class);
>>>>> +    dma_resv_lock(dmabuf->resv, &ww_ctx);
>>>> That won't work like this. The core property of a WW context is that
>>>> you
>>>> need to unwind all the locks and re-quire them with the contended one
>>>> first.
>>>>
>>>> When you statically lock the imported one here you can't do that any
>>>> more.
>>> You're right. I felt that something is missing here, but couldn't
>>> notice. I'll think more about this and enable
>>> CONFIG_DEBUG_WW_MUTEX_SLOWPATH. Thank you!
>>>
>> Christian, do you think we could make an excuse for the attach()
>> callback and make the exporter responsible for taking the resv lock? It
>> will be inconsistent with the rest of the callbacks, where importer
>> takes the lock, but it will be the simplest and least invasive solution.
>> It's very messy to do a cross-driver ww locking, I don't think it's the
>> right approach.
> 
> So to summarize the following calls will require that the caller hold
> the resv lock:
> 1. dma_buf_pin()/dma_buf_unpin()
> 2. dma_buf_map_attachment()/dma_buf_unmap_attachment()
> 3. dma_buf_vmap()/dma_buf_vunmap()
> 4. dma_buf_move_notify()
> 
> The following calls require that caller does not held the resv lock:
> 1. dma_buf_attach()/dma_buf_dynamic_attach()/dma_buf_detach()
> 2. dma_buf_export()/dma_buf_fd()
> 3. dma_buf_get()/dma_buf_put()
> 4. dma_buf_begin_cpu_access()/dma_buf_end_cpu_access()
> 
> If that's correct than that would work for me as well, but we should
> probably document this.

Looks good, thank you. I'll try this variant.

> Or let me ask the other way around: What calls exactly do you need to
> change to solve your original issue? That was vmap/vunmap, wasn't it? If
> yes then let's concentrate on those for the moment.

Originally, Daniel Vetter asked to sort out the dma-buf lockings across
all drivers, so we could replace custom locks in DRM-SHMEM with the resv
lock, otherwise there were no guarantees that we won't have deadlocks in
the dma-buf code paths.

The vmap/vunmap is one of the paths that needs to be sorted out, there
is no particular issue with it, just need to specify the convention. The
mmaping was the other questionable path and we concluded that it's
better to prohibit dma-buf mappings for DRM entirely. Lastly, there is
i915 attach() that uses the ww locking.

-- 
Best regards,
Dmitry

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2022-07-20 12:18 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-15  0:52 [PATCH v1 0/6] Move all drivers to a common dma-buf locking convention Dmitry Osipenko
2022-07-15  0:52 ` [PATCH v1 1/6] dma-buf: Add _unlocked postfix to function names Dmitry Osipenko
2022-07-15  7:19   ` Christian König
2022-07-15  9:31     ` Dmitry Osipenko
2022-07-15 11:04       ` Christian König
2022-07-15  0:52 ` [PATCH v1 2/6] drm/gem: Take reservation lock for vmap/vunmap operations Dmitry Osipenko
2022-07-15  7:31   ` Christian König
2022-07-15  0:52 ` [PATCH v1 3/6] dma-buf: Move all dma-bufs to dynamic locking specification Dmitry Osipenko
2022-07-15  0:52 ` [PATCH v1 4/6] dma-buf: Acquire wait-wound context on attachment Dmitry Osipenko
2022-07-15  3:38   ` kernel test robot
2022-07-15  6:50   ` Christian König
2022-07-15  6:59     ` Dmitry Osipenko
2022-07-19 20:05       ` Dmitry Osipenko
2022-07-20  8:29         ` Christian König
2022-07-20 12:18           ` Dmitry Osipenko
2022-07-15  0:52 ` [PATCH v1 5/6] media: videobuf2: Stop using internal dma-buf lock Dmitry Osipenko
2022-07-15  0:52 ` [PATCH v1 6/6] dma-buf: Remove internal lock Dmitry Osipenko
2022-07-19  9:13 ` [PATCH v1 0/6] Move all drivers to a common dma-buf locking convention Tomasz Figa

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).