[PATCH 0/6] Enable KFD to use render node BO mappings

[PATCH 0/6] Enable KFD to use render node BO mappings

[Felix Kuehling]

Rebased on latest amd-staging-drm-next. This is meant to be the final
review of this series, assuming no more issues are found.

This patch series enables KFD to interoperate more closely with DRM render
nodes. ROCm user mode already uses DRM render nodes to create its GPU VM
contexts and to CPU-map its GEM buffer objects. This patch series adds an
API to let KFD export its BOs as DMABufs, so they can be imported into
the DRM render nodes. This enables more flexible virtual memory mappings
using DRM_IOCTL_AMDGPU_GEM_VA.

Patches 1 and 2 deal with the exporting and importing of DMABufs.

The remaining patches let KFD validate and update GPUVM mappings managed
through render nodes.

The user mode side of this patch series can be seen in libhsakmt and
KFDTest where we improve integration with libdrm (initializing
amdgpu_device instances) to enable DMABuf imports into the render nodes
representing KFD GPU VM contexts. KFDTest is modified to test evictions
and validations of BOs mapped through amdgpu_bo_va_op:
https://github.com/fxkamd/ROCT-Thunk-Interface/commits/fxkamd/dmabuf

As a consequence, applications using Mesa and ROCm in the same process on
the same GPU will now share a single render node FD and GPUVM address
space.

The DMABuf export API will also be used later for upstream IPC and RDMA
implementations.

Felix Kuehling (6):
  drm/amdgpu: Generalize KFD dmabuf import
  drm/amdkfd: Implement DMA buf fd export from KFD
  drm/amdkfd: Improve amdgpu_vm_handle_moved
  drm/amdgpu: Attach eviction fence on alloc
  drm/amdgpu: update mappings not managed by KFD
  drm/amdgpu: Do bo_va ref counting for KFD BOs

 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h    |   2 +
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 196 ++++++++++++------
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c        |   6 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c   |   2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        |  18 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |   3 +-
 drivers/gpu/drm/amd/amdkfd/kfd_chardev.c      |  55 +++++
 include/uapi/linux/kfd_ioctl.h                |  14 +-
 8 files changed, 219 insertions(+), 77 deletions(-)

-- 
2.34.1

[PATCH 1/6] drm/amdgpu: Generalize KFD dmabuf import

[Felix Kuehling]

Use proper amdgpu_gem_prime_import function to handle all kinds of
imports. Remember the dmabuf reference to enable proper multi-GPU
attachment to multiple VMs without erroneously re-exporting the
underlying BO multiple times.

Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 38 ++++++++++---------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 6f236ded5f12..e13c3493b786 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -2209,30 +2209,27 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
 	struct amdgpu_bo *bo;
 	int ret;
 
-	if (dma_buf->ops != &amdgpu_dmabuf_ops)
-		/* Can't handle non-graphics buffers */
-		return -EINVAL;
-
-	obj = dma_buf->priv;
-	if (drm_to_adev(obj->dev) != adev)
-		/* Can't handle buffers from other devices */
-		return -EINVAL;
+	obj = amdgpu_gem_prime_import(adev_to_drm(adev), dma_buf);
+	if (IS_ERR(obj))
+		return PTR_ERR(obj);
 
 	bo = gem_to_amdgpu_bo(obj);
 	if (!(bo->preferred_domains & (AMDGPU_GEM_DOMAIN_VRAM |
-				    AMDGPU_GEM_DOMAIN_GTT)))
+				    AMDGPU_GEM_DOMAIN_GTT))) {
 		/* Only VRAM and GTT BOs are supported */
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err_put_obj;
+	}
 
 	*mem = kzalloc(sizeof(struct kgd_mem), GFP_KERNEL);
-	if (!*mem)
-		return -ENOMEM;
+	if (!*mem) {
+		ret = -ENOMEM;
+		goto err_put_obj;
+	}
 
 	ret = drm_vma_node_allow(&obj->vma_node, drm_priv);
-	if (ret) {
-		kfree(*mem);
-		return ret;
-	}
+	if (ret)
+		goto err_free_mem;
 
 	if (size)
 		*size = amdgpu_bo_size(bo);
@@ -2249,7 +2246,8 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
 		| KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE
 		| KFD_IOC_ALLOC_MEM_FLAGS_EXECUTABLE;
 
-	drm_gem_object_get(&bo->tbo.base);
+	get_dma_buf(dma_buf);
+	(*mem)->dmabuf = dma_buf;
 	(*mem)->bo = bo;
 	(*mem)->va = va;
 	(*mem)->domain = (bo->preferred_domains & AMDGPU_GEM_DOMAIN_VRAM) ?
@@ -2261,6 +2259,12 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
 	(*mem)->is_imported = true;
 
 	return 0;
+
+err_free_mem:
+	kfree(*mem);
+err_put_obj:
+	drm_gem_object_put(obj);
+	return ret;
 }
 
 /* Evict a userptr BO by stopping the queues if necessary
-- 
2.34.1

[PATCH 2/6] drm/amdkfd: Implement DMA buf fd export from KFD

[Felix Kuehling]

Exports a DMA buf fd of a given KFD buffer handle. This is intended for
being able to import KFD BOs into GEM contexts to leverage the
amdgpu_bo_va API for more flexible virtual address mappings. It will
also be used for the new upstreamable RDMA solution coming to UCX and
RCCL.

The corresponding user mode change (Thunk API and kfdtest) is here:
https://github.com/fxkamd/ROCT-Thunk-Interface/commits/fxkamd/dmabuf

Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h    |  2 +
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 45 +++++++++++----
 drivers/gpu/drm/amd/amdkfd/kfd_chardev.c      | 55 +++++++++++++++++++
 include/uapi/linux/kfd_ioctl.h                | 14 ++++-
 4 files changed, 104 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
index 333780491867..01ba3589b60a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
@@ -308,6 +308,8 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
 				      uint64_t va, void *drm_priv,
 				      struct kgd_mem **mem, uint64_t *size,
 				      uint64_t *mmap_offset);
+int amdgpu_amdkfd_gpuvm_export_dmabuf(struct kgd_mem *mem,
+				      struct dma_buf **dmabuf);
 int amdgpu_amdkfd_get_tile_config(struct amdgpu_device *adev,
 				struct tile_config *config);
 void amdgpu_amdkfd_ras_poison_consumption_handler(struct amdgpu_device *adev,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index e13c3493b786..5645103beed0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -710,6 +710,21 @@ kfd_mem_dmaunmap_attachment(struct kgd_mem *mem,
 	}
 }
 
+static int kfd_mem_export_dmabuf(struct kgd_mem *mem)
+{
+	if (!mem->dmabuf) {
+		struct dma_buf *ret = amdgpu_gem_prime_export(
+			&mem->bo->tbo.base,
+			mem->alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE ?
+				DRM_RDWR : 0);
+		if (IS_ERR(ret))
+			return PTR_ERR(ret);
+		mem->dmabuf = ret;
+	}
+
+	return 0;
+}
+
 static int
 kfd_mem_attach_dmabuf(struct amdgpu_device *adev, struct kgd_mem *mem,
 		      struct amdgpu_bo **bo)
@@ -717,16 +732,9 @@ kfd_mem_attach_dmabuf(struct amdgpu_device *adev, struct kgd_mem *mem,
 	struct drm_gem_object *gobj;
 	int ret;
 
-	if (!mem->dmabuf) {
-		mem->dmabuf = amdgpu_gem_prime_export(&mem->bo->tbo.base,
-			mem->alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE ?
-				DRM_RDWR : 0);
-		if (IS_ERR(mem->dmabuf)) {
-			ret = PTR_ERR(mem->dmabuf);
-			mem->dmabuf = NULL;
-			return ret;
-		}
-	}
+	ret = kfd_mem_export_dmabuf(mem);
+	if (ret)
+		return ret;
 
 	gobj = amdgpu_gem_prime_import(adev_to_drm(adev), mem->dmabuf);
 	if (IS_ERR(gobj))
@@ -2267,6 +2275,23 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
 	return ret;
 }
 
+int amdgpu_amdkfd_gpuvm_export_dmabuf(struct kgd_mem *mem,
+				      struct dma_buf **dma_buf)
+{
+	int ret;
+
+	mutex_lock(&mem->lock);
+	ret = kfd_mem_export_dmabuf(mem);
+	if (ret)
+		goto out;
+
+	get_dma_buf(mem->dmabuf);
+	*dma_buf = mem->dmabuf;
+out:
+	mutex_unlock(&mem->lock);
+	return ret;
+}
+
 /* Evict a userptr BO by stopping the queues if necessary
  *
  * Runs in MMU notifier, may be in RECLAIM_FS context. This means it
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index f79b8e964140..bcf2263927d6 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -1572,6 +1572,58 @@ static int kfd_ioctl_import_dmabuf(struct file *filep,
 	return r;
 }
 
+static int kfd_ioctl_export_dmabuf(struct file *filep,
+				   struct kfd_process *p, void *data)
+{
+	struct kfd_ioctl_export_dmabuf_args *args = data;
+	struct kfd_process_device *pdd;
+	struct dma_buf *dmabuf;
+	struct kfd_dev *dev;
+	void *mem;
+	int ret = 0;
+
+	dev = kfd_device_by_id(GET_GPU_ID(args->handle));
+	if (!dev)
+		return -EINVAL;
+
+	mutex_lock(&p->mutex);
+
+	pdd = kfd_get_process_device_data(dev, p);
+	if (!pdd) {
+		ret = -EINVAL;
+		goto err_unlock;
+	}
+
+	mem = kfd_process_device_translate_handle(pdd,
+						GET_IDR_HANDLE(args->handle));
+	if (!mem) {
+		ret = -EINVAL;
+		goto err_unlock;
+	}
+
+	ret = amdgpu_amdkfd_gpuvm_export_dmabuf(mem, &dmabuf);
+	mutex_unlock(&p->mutex);
+	if (ret)
+		goto err_out;
+
+	ret = dma_buf_fd(dmabuf, args->flags);
+	if (ret < 0) {
+		dma_buf_put(dmabuf);
+		goto err_out;
+	}
+	/* dma_buf_fd assigns the reference count to the fd, no need to
+	 * put the reference here.
+	 */
+	args->dmabuf_fd = ret;
+
+	return 0;
+
+err_unlock:
+	mutex_unlock(&p->mutex);
+err_out:
+	return ret;
+}
+
 /* Handle requests for watching SMI events */
 static int kfd_ioctl_smi_events(struct file *filep,
 				struct kfd_process *p, void *data)
@@ -2754,6 +2806,9 @@ static const struct amdkfd_ioctl_desc amdkfd_ioctls[] = {
 
 	AMDKFD_IOCTL_DEF(AMDKFD_IOC_AVAILABLE_MEMORY,
 			kfd_ioctl_get_available_memory, 0),
+
+	AMDKFD_IOCTL_DEF(AMDKFD_IOC_EXPORT_DMABUF,
+				kfd_ioctl_export_dmabuf, 0),
 };
 
 #define AMDKFD_CORE_IOCTL_COUNT	ARRAY_SIZE(amdkfd_ioctls)
diff --git a/include/uapi/linux/kfd_ioctl.h b/include/uapi/linux/kfd_ioctl.h
index 42b60198b6c5..2da5c3ad71bd 100644
--- a/include/uapi/linux/kfd_ioctl.h
+++ b/include/uapi/linux/kfd_ioctl.h
@@ -37,9 +37,10 @@
  * - 1.9 - Add available memory ioctl
  * - 1.10 - Add SMI profiler event log
  * - 1.11 - Add unified memory for ctx save/restore area
+ * - 1.12 - Add DMA buf export ioctl
  */
 #define KFD_IOCTL_MAJOR_VERSION 1
-#define KFD_IOCTL_MINOR_VERSION 11
+#define KFD_IOCTL_MINOR_VERSION 12
 
 struct kfd_ioctl_get_version_args {
 	__u32 major_version;	/* from KFD */
@@ -463,6 +464,12 @@ struct kfd_ioctl_import_dmabuf_args {
 	__u32 dmabuf_fd;	/* to KFD */
 };
 
+struct kfd_ioctl_export_dmabuf_args {
+	__u64 handle;		/* to KFD */
+	__u32 flags;		/* to KFD */
+	__u32 dmabuf_fd;	/* from KFD */
+};
+
 /*
  * KFD SMI(System Management Interface) events
  */
@@ -877,7 +884,10 @@ struct kfd_ioctl_set_xnack_mode_args {
 #define AMDKFD_IOC_AVAILABLE_MEMORY		\
 		AMDKFD_IOWR(0x23, struct kfd_ioctl_get_available_memory_args)
 
+#define AMDKFD_IOC_EXPORT_DMABUF		\
+		AMDKFD_IOWR(0x24, struct kfd_ioctl_export_dmabuf_args)
+
 #define AMDKFD_COMMAND_START		0x01
-#define AMDKFD_COMMAND_END		0x24
+#define AMDKFD_COMMAND_END		0x25
 
 #endif
-- 
2.34.1

[PATCH 3/6] drm/amdkfd: Improve amdgpu_vm_handle_moved

[Felix Kuehling]

Let amdgpu_vm_handle_moved update all BO VA mappings of BOs reserved by
the caller. This will be useful for handling extra BO VA mappings in
KFD VMs that are managed through the render node API.

Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
Reviewed-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c      |  6 +++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c      | 18 +++++++++++++-----
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h      |  3 ++-
 4 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 275da612cd87..a80d2557edb2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -1121,6 +1121,10 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
 			return r;
 	}
 
+	/* TODO: Is this loop still needed, or could this be handled by
+	 * amdgpu_vm_handle_moved, now that it can handle all BOs that are
+	 * reserved under p->ticket?
+	 */
 	amdgpu_bo_list_for_each_entry(e, p->bo_list) {
 		/* ignore duplicates */
 		bo = ttm_to_amdgpu_bo(e->tv.bo);
@@ -1140,7 +1144,7 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
 			return r;
 	}
 
-	r = amdgpu_vm_handle_moved(adev, vm);
+	r = amdgpu_vm_handle_moved(adev, vm, &p->ticket);
 	if (r)
 		return r;
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
index 271e30e34d93..23a213e4ab2c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
@@ -404,7 +404,7 @@ amdgpu_dma_buf_move_notify(struct dma_buf_attachment *attach)
 
 		r = amdgpu_vm_clear_freed(adev, vm, NULL);
 		if (!r)
-			r = amdgpu_vm_handle_moved(adev, vm);
+			r = amdgpu_vm_handle_moved(adev, vm, ticket);
 
 		if (r && r != -EBUSY)
 			DRM_ERROR("Failed to invalidate VM page tables (%d))\n",
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index dc379dc22c77..75dae2850e75 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1286,11 +1286,12 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
  * PTs have to be reserved!
  */
 int amdgpu_vm_handle_moved(struct amdgpu_device *adev,
-			   struct amdgpu_vm *vm)
+			   struct amdgpu_vm *vm,
+			   struct ww_acquire_ctx *ticket)
 {
 	struct amdgpu_bo_va *bo_va;
 	struct dma_resv *resv;
-	bool clear;
+	bool clear, unlock;
 	int r;
 
 	spin_lock(&vm->status_lock);
@@ -1313,17 +1314,24 @@ int amdgpu_vm_handle_moved(struct amdgpu_device *adev,
 		spin_unlock(&vm->status_lock);
 
 		/* Try to reserve the BO to avoid clearing its ptes */
-		if (!amdgpu_vm_debug && dma_resv_trylock(resv))
+		if (!amdgpu_vm_debug && dma_resv_trylock(resv)) {
 			clear = false;
+			unlock = true;
+		/* The caller is already holding the reservation lock */
+		} else if (ticket && dma_resv_locking_ctx(resv) == ticket) {
+			clear = false;
+			unlock = false;
 		/* Somebody else is using the BO right now */
-		else
+		} else {
 			clear = true;
+			unlock = false;
+		}
 
 		r = amdgpu_vm_bo_update(adev, bo_va, clear);
 		if (r)
 			return r;
 
-		if (!clear)
+		if (unlock)
 			dma_resv_unlock(resv);
 		spin_lock(&vm->status_lock);
 	}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 094bb4807303..03a3314e5b43 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -400,7 +400,8 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
 			  struct amdgpu_vm *vm,
 			  struct dma_fence **fence);
 int amdgpu_vm_handle_moved(struct amdgpu_device *adev,
-			   struct amdgpu_vm *vm);
+			   struct amdgpu_vm *vm,
+			   struct ww_acquire_ctx *ticket);
 void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base *base,
 			    struct amdgpu_vm *vm, struct amdgpu_bo *bo);
 int amdgpu_vm_update_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
-- 
2.34.1

[PATCH 4/6] drm/amdgpu: Attach eviction fence on alloc

[Felix Kuehling]

Instead of attaching the eviction fence when a KFD BO is first mapped,
attach it when it is allocated or imported. This in preparation to allow
KFD BOs to be mapped using the render node API.

Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 63 ++++++++++---------
 1 file changed, 32 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 5645103beed0..79213f476493 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -360,6 +360,24 @@ static int amdgpu_amdkfd_bo_validate(struct amdgpu_bo *bo, uint32_t domain,
 	return ret;
 }
 
+static int amdgpu_amdkfd_bo_validate_and_fence(struct amdgpu_bo *bo,
+					       uint32_t domain,
+					       struct dma_fence *fence)
+{
+	int ret = amdgpu_bo_reserve(bo, false);
+
+	if (ret)
+		return ret;
+
+	ret = amdgpu_amdkfd_bo_validate(bo, domain, true);
+	if (!ret)
+		dma_resv_add_fence(bo->tbo.base.resv, fence,
+				   DMA_RESV_USAGE_BOOKKEEP);
+	amdgpu_bo_unreserve(bo);
+
+	return ret;
+}
+
 static int amdgpu_amdkfd_validate_vm_bo(void *_unused, struct amdgpu_bo *bo)
 {
 	return amdgpu_amdkfd_bo_validate(bo, bo->allowed_domains, false);
@@ -1720,6 +1738,11 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
 		}
 		bo->allowed_domains = AMDGPU_GEM_DOMAIN_GTT;
 		bo->preferred_domains = AMDGPU_GEM_DOMAIN_GTT;
+	} else {
+		ret = amdgpu_amdkfd_bo_validate_and_fence(bo, domain,
+				&avm->process_info->eviction_fence->base);
+		if (ret)
+			goto err_validate_bo;
 	}
 
 	if (offset)
@@ -1729,6 +1752,7 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
 
 allocate_init_user_pages_failed:
 err_pin_bo:
+err_validate_bo:
 	remove_kgd_mem_from_kfd_bo_list(*mem, avm->process_info);
 	drm_vma_node_revoke(&gobj->vma_node, drm_priv);
 err_node_allow:
@@ -1804,10 +1828,6 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(
 	if (unlikely(ret))
 		return ret;
 
-	/* The eviction fence should be removed by the last unmap.
-	 * TODO: Log an error condition if the bo still has the eviction fence
-	 * attached
-	 */
 	amdgpu_amdkfd_remove_eviction_fence(mem->bo,
 					process_info->eviction_fence);
 	pr_debug("Release VA 0x%llx - 0x%llx\n", mem->va,
@@ -1931,19 +1951,6 @@ int amdgpu_amdkfd_gpuvm_map_memory_to_gpu(
 	if (unlikely(ret))
 		goto out_unreserve;
 
-	if (mem->mapped_to_gpu_memory == 0 &&
-	    !amdgpu_ttm_tt_get_usermm(bo->tbo.ttm)) {
-		/* Validate BO only once. The eviction fence gets added to BO
-		 * the first time it is mapped. Validate will wait for all
-		 * background evictions to complete.
-		 */
-		ret = amdgpu_amdkfd_bo_validate(bo, domain, true);
-		if (ret) {
-			pr_debug("Validate failed\n");
-			goto out_unreserve;
-		}
-	}
-
 	list_for_each_entry(entry, &mem->attachments, list) {
 		if (entry->bo_va->base.vm != avm || entry->is_mapped)
 			continue;
@@ -1970,10 +1977,6 @@ int amdgpu_amdkfd_gpuvm_map_memory_to_gpu(
 			 mem->mapped_to_gpu_memory);
 	}
 
-	if (!amdgpu_ttm_tt_get_usermm(bo->tbo.ttm) && !bo->tbo.pin_count)
-		dma_resv_add_fence(bo->tbo.base.resv,
-				   &avm->process_info->eviction_fence->base,
-				   DMA_RESV_USAGE_BOOKKEEP);
 	ret = unreserve_bo_and_vms(&ctx, false, false);
 
 	goto out;
@@ -1990,7 +1993,6 @@ int amdgpu_amdkfd_gpuvm_unmap_memory_from_gpu(
 		struct amdgpu_device *adev, struct kgd_mem *mem, void *drm_priv)
 {
 	struct amdgpu_vm *avm = drm_priv_to_vm(drm_priv);
-	struct amdkfd_process_info *process_info = avm->process_info;
 	unsigned long bo_size = mem->bo->tbo.base.size;
 	struct kfd_mem_attachment *entry;
 	struct bo_vm_reservation_context ctx;
@@ -2031,15 +2033,6 @@ int amdgpu_amdkfd_gpuvm_unmap_memory_from_gpu(
 			 mem->mapped_to_gpu_memory);
 	}
 
-	/* If BO is unmapped from all VMs, unfence it. It can be evicted if
-	 * required.
-	 */
-	if (mem->mapped_to_gpu_memory == 0 &&
-	    !amdgpu_ttm_tt_get_usermm(mem->bo->tbo.ttm) &&
-	    !mem->bo->tbo.pin_count)
-		amdgpu_amdkfd_remove_eviction_fence(mem->bo,
-						process_info->eviction_fence);
-
 unreserve_out:
 	unreserve_bo_and_vms(&ctx, false, false);
 out:
@@ -2266,8 +2259,16 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
 	amdgpu_sync_create(&(*mem)->sync);
 	(*mem)->is_imported = true;
 
+	ret = amdgpu_amdkfd_bo_validate_and_fence(bo, (*mem)->domain,
+				&avm->process_info->eviction_fence->base);
+	if (ret)
+		goto err_remove_mem;
+
 	return 0;
 
+err_remove_mem:
+	remove_kgd_mem_from_kfd_bo_list(*mem, avm->process_info);
+	drm_vma_node_revoke(&obj->vma_node, drm_priv);
 err_free_mem:
 	kfree(*mem);
 err_put_obj:
-- 
2.34.1

[PATCH 5/6] drm/amdgpu: update mappings not managed by KFD

[Felix Kuehling]

When restoring after an eviction, use amdgpu_vm_handle_moved to update
BO VA mappings in KFD VMs that are not managed through the KFD API. This
should allow using the render node API to create more flexible memory
mappings in KFD VMs.

v2: Sync with pd fence after all page table updates
v3: Update comments, remove TODOs that are no longer applicable

Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 28 +++++++++++++++----
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 79213f476493..df08e84f01d7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -2728,12 +2728,6 @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void *info, struct dma_fence **ef)
 	if (ret)
 		goto validate_map_fail;
 
-	ret = process_sync_pds_resv(process_info, &sync_obj);
-	if (ret) {
-		pr_debug("Memory eviction: Failed to sync to PD BO moving fence. Try again\n");
-		goto validate_map_fail;
-	}
-
 	/* Validate BOs and map them to GPUVM (update VM page tables). */
 	list_for_each_entry(mem, &process_info->kfd_bo_list,
 			    validate_list.head) {
@@ -2781,6 +2775,19 @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void *info, struct dma_fence **ef)
 	if (failed_size)
 		pr_debug("0x%lx/0x%lx in system\n", failed_size, total_size);
 
+	/* Update mappings not managed by KFD */
+	list_for_each_entry(peer_vm, &process_info->vm_list_head,
+			vm_list_node) {
+		struct amdgpu_device *adev = amdgpu_ttm_adev(
+			peer_vm->root.bo->tbo.bdev);
+
+		ret = amdgpu_vm_handle_moved(adev, peer_vm, &ctx.ticket);
+		if (ret) {
+			pr_debug("Memory eviction: handle moved failed. Try again\n");
+			goto validate_map_fail;
+		}
+	}
+
 	/* Update page directories */
 	ret = process_update_pds(process_info, &sync_obj);
 	if (ret) {
@@ -2788,6 +2795,15 @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void *info, struct dma_fence **ef)
 		goto validate_map_fail;
 	}
 
+	/* Sync with fences on all the page tables. They implicitly depend on any
+	 * move fences from amdgpu_vm_handle_moved above.
+	 */
+	ret = process_sync_pds_resv(process_info, &sync_obj);
+	if (ret) {
+		pr_debug("Memory eviction: Failed to sync to PD BO moving fence. Try again\n");
+		goto validate_map_fail;
+	}
+
 	/* Wait for validate and PT updates to finish */
 	amdgpu_sync_wait(&sync_obj, false);
 
-- 
2.34.1

[PATCH 6/6] drm/amdgpu: Do bo_va ref counting for KFD BOs

[Felix Kuehling]

This is needed to correctly handle BOs imported into the GEM API, which
would otherwise get added twice to the same VM.

Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 28 +++++++++++++++----
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index df08e84f01d7..8b5ba2e04a79 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -370,9 +370,17 @@ static int amdgpu_amdkfd_bo_validate_and_fence(struct amdgpu_bo *bo,
 		return ret;
 
 	ret = amdgpu_amdkfd_bo_validate(bo, domain, true);
-	if (!ret)
-		dma_resv_add_fence(bo->tbo.base.resv, fence,
-				   DMA_RESV_USAGE_BOOKKEEP);
+	if (ret)
+		goto unreserve_out;
+
+	ret = dma_resv_reserve_fences(bo->tbo.base.resv, 1);
+	if (ret)
+		goto unreserve_out;
+
+	dma_resv_add_fence(bo->tbo.base.resv, fence,
+			   DMA_RESV_USAGE_BOOKKEEP);
+
+unreserve_out:
 	amdgpu_bo_unreserve(bo);
 
 	return ret;
@@ -785,6 +793,7 @@ static int kfd_mem_attach(struct amdgpu_device *adev, struct kgd_mem *mem,
 	uint64_t va = mem->va;
 	struct kfd_mem_attachment *attachment[2] = {NULL, NULL};
 	struct amdgpu_bo *bo[2] = {NULL, NULL};
+	struct amdgpu_bo_va *bo_va;
 	bool same_hive = false;
 	int i, ret;
 
@@ -871,7 +880,12 @@ static int kfd_mem_attach(struct amdgpu_device *adev, struct kgd_mem *mem,
 			pr_debug("Unable to reserve BO during memory attach");
 			goto unwind;
 		}
-		attachment[i]->bo_va = amdgpu_vm_bo_add(adev, vm, bo[i]);
+		bo_va = amdgpu_vm_bo_find(vm, bo[i]);
+		if (!bo_va)
+			bo_va = amdgpu_vm_bo_add(adev, vm, bo[i]);
+		else
+			++bo_va->ref_count;
+		attachment[i]->bo_va = bo_va;
 		amdgpu_bo_unreserve(bo[i]);
 		if (unlikely(!attachment[i]->bo_va)) {
 			ret = -ENOMEM;
@@ -895,7 +909,8 @@ static int kfd_mem_attach(struct amdgpu_device *adev, struct kgd_mem *mem,
 			continue;
 		if (attachment[i]->bo_va) {
 			amdgpu_bo_reserve(bo[i], true);
-			amdgpu_vm_bo_del(adev, attachment[i]->bo_va);
+			if (--attachment[i]->bo_va->ref_count == 0)
+				amdgpu_vm_bo_del(adev, attachment[i]->bo_va);
 			amdgpu_bo_unreserve(bo[i]);
 			list_del(&attachment[i]->list);
 		}
@@ -912,7 +927,8 @@ static void kfd_mem_detach(struct kfd_mem_attachment *attachment)
 
 	pr_debug("\t remove VA 0x%llx in entry %p\n",
 			attachment->va, attachment);
-	amdgpu_vm_bo_del(attachment->adev, attachment->bo_va);
+	if (--attachment->bo_va->ref_count == 0)
+		amdgpu_vm_bo_del(attachment->adev, attachment->bo_va);
 	drm_gem_object_put(&bo->tbo.base);
 	list_del(&attachment->list);
 	kfree(attachment);
-- 
2.34.1

Re: [PATCH 0/6] Enable KFD to use render node BO mappings

[Christian König]

Am 12.01.23 um 02:31 schrieb Felix Kuehling:
> Rebased on latest amd-staging-drm-next. This is meant to be the final
> review of this series, assuming no more issues are found.
>
> This patch series enables KFD to interoperate more closely with DRM render
> nodes. ROCm user mode already uses DRM render nodes to create its GPU VM
> contexts and to CPU-map its GEM buffer objects. This patch series adds an
> API to let KFD export its BOs as DMABufs, so they can be imported into
> the DRM render nodes. This enables more flexible virtual memory mappings
> using DRM_IOCTL_AMDGPU_GEM_VA.
>
> Patches 1 and 2 deal with the exporting and importing of DMABufs.
>
> The remaining patches let KFD validate and update GPUVM mappings managed
> through render nodes.
>
> The user mode side of this patch series can be seen in libhsakmt and
> KFDTest where we improve integration with libdrm (initializing
> amdgpu_device instances) to enable DMABuf imports into the render nodes
> representing KFD GPU VM contexts. KFDTest is modified to test evictions
> and validations of BOs mapped through amdgpu_bo_va_op:
> https://github.com/fxkamd/ROCT-Thunk-Interface/commits/fxkamd/dmabuf
>
> As a consequence, applications using Mesa and ROCm in the same process on
> the same GPU will now share a single render node FD and GPUVM address
> space.
>
> The DMABuf export API will also be used later for upstream IPC and RDMA
> implementations.

Nice, I don't have time to check everything in detail but at least from 
a high level skimming over this I can't see anything obvious wrong.

Feel free to add an Acked-by: Christian König <christian.koenig@amd.com> 
to the series.

Christian.

>
> Felix Kuehling (6):
>    drm/amdgpu: Generalize KFD dmabuf import
>    drm/amdkfd: Implement DMA buf fd export from KFD
>    drm/amdkfd: Improve amdgpu_vm_handle_moved
>    drm/amdgpu: Attach eviction fence on alloc
>    drm/amdgpu: update mappings not managed by KFD
>    drm/amdgpu: Do bo_va ref counting for KFD BOs
>
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h    |   2 +
>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 196 ++++++++++++------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c        |   6 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c   |   2 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        |  18 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |   3 +-
>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c      |  55 +++++
>   include/uapi/linux/kfd_ioctl.h                |  14 +-
>   8 files changed, 219 insertions(+), 77 deletions(-)
>

Re: [PATCH 1/6] drm/amdgpu: Generalize KFD dmabuf import

[Chen, Xiaogang]

On 1/11/2023 7:31 PM, Felix Kuehling wrote:
> Use proper amdgpu_gem_prime_import function to handle all kinds of
> imports. Remember the dmabuf reference to enable proper multi-GPU
> attachment to multiple VMs without erroneously re-exporting the
> underlying BO multiple times.
>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 38 ++++++++++---------
>   1 file changed, 21 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index 6f236ded5f12..e13c3493b786 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -2209,30 +2209,27 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
>   	struct amdgpu_bo *bo;
>   	int ret;
>   
> -	if (dma_buf->ops != &amdgpu_dmabuf_ops)
> -		/* Can't handle non-graphics buffers */
> -		return -EINVAL;
> -
> -	obj = dma_buf->priv;
> -	if (drm_to_adev(obj->dev) != adev)
> -		/* Can't handle buffers from other devices */
> -		return -EINVAL;
> +	obj = amdgpu_gem_prime_import(adev_to_drm(adev), dma_buf);
> +	if (IS_ERR(obj))
> +		return PTR_ERR(obj);
>   
>   	bo = gem_to_amdgpu_bo(obj);
>   	if (!(bo->preferred_domains & (AMDGPU_GEM_DOMAIN_VRAM |
> -				    AMDGPU_GEM_DOMAIN_GTT)))
> +				    AMDGPU_GEM_DOMAIN_GTT))) {
>   		/* Only VRAM and GTT BOs are supported */
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto err_put_obj;
> +	}
>   
>   	*mem = kzalloc(sizeof(struct kgd_mem), GFP_KERNEL);
> -	if (!*mem)
> -		return -ENOMEM;
> +	if (!*mem) {
> +		ret = -ENOMEM;
> +		goto err_put_obj;
> +	}
>   
>   	ret = drm_vma_node_allow(&obj->vma_node, drm_priv);
> -	if (ret) {
> -		kfree(*mem);
> -		return ret;
> -	}
> +	if (ret)
> +		goto err_free_mem;
>   
>   	if (size)
>   		*size = amdgpu_bo_size(bo);

Hi Felix:

I have a question when using amdgpu_gem_prime_import. It will allow 
importing a dmabuf to different gpus, then can we still call 
amdgpu_bo_mmap_offset on the generated bo if 
amdgpu_amdkfd_gpuvm_import_dmabuf also ask mmap_offset?

> @@ -2249,7 +2246,8 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
>   		| KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE
>   		| KFD_IOC_ALLOC_MEM_FLAGS_EXECUTABLE;
>   
> -	drm_gem_object_get(&bo->tbo.base);
> +	get_dma_buf(dma_buf);
> +	(*mem)->dmabuf = dma_buf;
>   	(*mem)->bo = bo;
>   	(*mem)->va = va;
>   	(*mem)->domain = (bo->preferred_domains & AMDGPU_GEM_DOMAIN_VRAM) ?
> @@ -2261,6 +2259,12 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
>   	(*mem)->is_imported = true;
>   
>   	return 0;
> +
> +err_free_mem:
> +	kfree(*mem);
> +err_put_obj:
> +	drm_gem_object_put(obj);
> +	return ret;
>   }
>   
>   /* Evict a userptr BO by stopping the queues if necessary

Re: [PATCH 2/6] drm/amdkfd: Implement DMA buf fd export from KFD

[Chen, Xiaogang]

Reviewed-by: Xiaogang Chen <Xiaoganng.Chen@amd.com>

Regards

Xiaogang

On 1/11/2023 7:31 PM, Felix Kuehling wrote:
> Exports a DMA buf fd of a given KFD buffer handle. This is intended for
> being able to import KFD BOs into GEM contexts to leverage the
> amdgpu_bo_va API for more flexible virtual address mappings. It will
> also be used for the new upstreamable RDMA solution coming to UCX and
> RCCL.
>
> The corresponding user mode change (Thunk API and kfdtest) is here:
> https://github.com/fxkamd/ROCT-Thunk-Interface/commits/fxkamd/dmabuf
>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h    |  2 +
>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 45 +++++++++++----
>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c      | 55 +++++++++++++++++++
>   include/uapi/linux/kfd_ioctl.h                | 14 ++++-
>   4 files changed, 104 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> index 333780491867..01ba3589b60a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> @@ -308,6 +308,8 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
>   				      uint64_t va, void *drm_priv,
>   				      struct kgd_mem **mem, uint64_t *size,
>   				      uint64_t *mmap_offset);
> +int amdgpu_amdkfd_gpuvm_export_dmabuf(struct kgd_mem *mem,
> +				      struct dma_buf **dmabuf);
>   int amdgpu_amdkfd_get_tile_config(struct amdgpu_device *adev,
>   				struct tile_config *config);
>   void amdgpu_amdkfd_ras_poison_consumption_handler(struct amdgpu_device *adev,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index e13c3493b786..5645103beed0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -710,6 +710,21 @@ kfd_mem_dmaunmap_attachment(struct kgd_mem *mem,
>   	}
>   }
>   
> +static int kfd_mem_export_dmabuf(struct kgd_mem *mem)
> +{
> +	if (!mem->dmabuf) {
> +		struct dma_buf *ret = amdgpu_gem_prime_export(
> +			&mem->bo->tbo.base,
> +			mem->alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE ?
> +				DRM_RDWR : 0);
> +		if (IS_ERR(ret))
> +			return PTR_ERR(ret);
> +		mem->dmabuf = ret;
> +	}
> +
> +	return 0;
> +}
> +
>   static int
>   kfd_mem_attach_dmabuf(struct amdgpu_device *adev, struct kgd_mem *mem,
>   		      struct amdgpu_bo **bo)
> @@ -717,16 +732,9 @@ kfd_mem_attach_dmabuf(struct amdgpu_device *adev, struct kgd_mem *mem,
>   	struct drm_gem_object *gobj;
>   	int ret;
>   
> -	if (!mem->dmabuf) {
> -		mem->dmabuf = amdgpu_gem_prime_export(&mem->bo->tbo.base,
> -			mem->alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE ?
> -				DRM_RDWR : 0);
> -		if (IS_ERR(mem->dmabuf)) {
> -			ret = PTR_ERR(mem->dmabuf);
> -			mem->dmabuf = NULL;
> -			return ret;
> -		}
> -	}
> +	ret = kfd_mem_export_dmabuf(mem);
> +	if (ret)
> +		return ret;
>   
>   	gobj = amdgpu_gem_prime_import(adev_to_drm(adev), mem->dmabuf);
>   	if (IS_ERR(gobj))
> @@ -2267,6 +2275,23 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
>   	return ret;
>   }
>   
> +int amdgpu_amdkfd_gpuvm_export_dmabuf(struct kgd_mem *mem,
> +				      struct dma_buf **dma_buf)
> +{
> +	int ret;
> +
> +	mutex_lock(&mem->lock);
> +	ret = kfd_mem_export_dmabuf(mem);
> +	if (ret)
> +		goto out;
> +
> +	get_dma_buf(mem->dmabuf);
> +	*dma_buf = mem->dmabuf;
> +out:
> +	mutex_unlock(&mem->lock);
> +	return ret;
> +}
> +
>   /* Evict a userptr BO by stopping the queues if necessary
>    *
>    * Runs in MMU notifier, may be in RECLAIM_FS context. This means it
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index f79b8e964140..bcf2263927d6 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -1572,6 +1572,58 @@ static int kfd_ioctl_import_dmabuf(struct file *filep,
>   	return r;
>   }
>   
> +static int kfd_ioctl_export_dmabuf(struct file *filep,
> +				   struct kfd_process *p, void *data)
> +{
> +	struct kfd_ioctl_export_dmabuf_args *args = data;
> +	struct kfd_process_device *pdd;
> +	struct dma_buf *dmabuf;
> +	struct kfd_dev *dev;
> +	void *mem;
> +	int ret = 0;
> +
> +	dev = kfd_device_by_id(GET_GPU_ID(args->handle));
> +	if (!dev)
> +		return -EINVAL;
> +
> +	mutex_lock(&p->mutex);
> +
> +	pdd = kfd_get_process_device_data(dev, p);
> +	if (!pdd) {
> +		ret = -EINVAL;
> +		goto err_unlock;
> +	}
> +
> +	mem = kfd_process_device_translate_handle(pdd,
> +						GET_IDR_HANDLE(args->handle));
> +	if (!mem) {
> +		ret = -EINVAL;
> +		goto err_unlock;
> +	}
> +
> +	ret = amdgpu_amdkfd_gpuvm_export_dmabuf(mem, &dmabuf);
> +	mutex_unlock(&p->mutex);
> +	if (ret)
> +		goto err_out;
> +
> +	ret = dma_buf_fd(dmabuf, args->flags);
> +	if (ret < 0) {
> +		dma_buf_put(dmabuf);
> +		goto err_out;
> +	}
> +	/* dma_buf_fd assigns the reference count to the fd, no need to
> +	 * put the reference here.
> +	 */
> +	args->dmabuf_fd = ret;
> +
> +	return 0;
> +
> +err_unlock:
> +	mutex_unlock(&p->mutex);
> +err_out:
> +	return ret;
> +}
> +
>   /* Handle requests for watching SMI events */
>   static int kfd_ioctl_smi_events(struct file *filep,
>   				struct kfd_process *p, void *data)
> @@ -2754,6 +2806,9 @@ static const struct amdkfd_ioctl_desc amdkfd_ioctls[] = {
>   
>   	AMDKFD_IOCTL_DEF(AMDKFD_IOC_AVAILABLE_MEMORY,
>   			kfd_ioctl_get_available_memory, 0),
> +
> +	AMDKFD_IOCTL_DEF(AMDKFD_IOC_EXPORT_DMABUF,
> +				kfd_ioctl_export_dmabuf, 0),
>   };
>   
>   #define AMDKFD_CORE_IOCTL_COUNT	ARRAY_SIZE(amdkfd_ioctls)
> diff --git a/include/uapi/linux/kfd_ioctl.h b/include/uapi/linux/kfd_ioctl.h
> index 42b60198b6c5..2da5c3ad71bd 100644
> --- a/include/uapi/linux/kfd_ioctl.h
> +++ b/include/uapi/linux/kfd_ioctl.h
> @@ -37,9 +37,10 @@
>    * - 1.9 - Add available memory ioctl
>    * - 1.10 - Add SMI profiler event log
>    * - 1.11 - Add unified memory for ctx save/restore area
> + * - 1.12 - Add DMA buf export ioctl
>    */
>   #define KFD_IOCTL_MAJOR_VERSION 1
> -#define KFD_IOCTL_MINOR_VERSION 11
> +#define KFD_IOCTL_MINOR_VERSION 12
>   
>   struct kfd_ioctl_get_version_args {
>   	__u32 major_version;	/* from KFD */
> @@ -463,6 +464,12 @@ struct kfd_ioctl_import_dmabuf_args {
>   	__u32 dmabuf_fd;	/* to KFD */
>   };
>   
> +struct kfd_ioctl_export_dmabuf_args {
> +	__u64 handle;		/* to KFD */
> +	__u32 flags;		/* to KFD */
> +	__u32 dmabuf_fd;	/* from KFD */
> +};
> +
>   /*
>    * KFD SMI(System Management Interface) events
>    */
> @@ -877,7 +884,10 @@ struct kfd_ioctl_set_xnack_mode_args {
>   #define AMDKFD_IOC_AVAILABLE_MEMORY		\
>   		AMDKFD_IOWR(0x23, struct kfd_ioctl_get_available_memory_args)
>   
> +#define AMDKFD_IOC_EXPORT_DMABUF		\
> +		AMDKFD_IOWR(0x24, struct kfd_ioctl_export_dmabuf_args)
> +
>   #define AMDKFD_COMMAND_START		0x01
> -#define AMDKFD_COMMAND_END		0x24
> +#define AMDKFD_COMMAND_END		0x25
>   
>   #endif

Re: [PATCH 3/6] drm/amdkfd: Improve amdgpu_vm_handle_moved

[Chen, Xiaogang]

Acked-by: Xiaogang Chen

Regards

Xiaogang

On 1/11/2023 7:31 PM, Felix Kuehling wrote:
> Let amdgpu_vm_handle_moved update all BO VA mappings of BOs reserved by
> the caller. This will be useful for handling extra BO VA mappings in
> KFD VMs that are managed through the render node API.
>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> Reviewed-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c      |  6 +++++-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c |  2 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c      | 18 +++++++++++++-----
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h      |  3 ++-
>   4 files changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 275da612cd87..a80d2557edb2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -1121,6 +1121,10 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
>   			return r;
>   	}
>   
> +	/* TODO: Is this loop still needed, or could this be handled by
> +	 * amdgpu_vm_handle_moved, now that it can handle all BOs that are
> +	 * reserved under p->ticket?
> +	 */
>   	amdgpu_bo_list_for_each_entry(e, p->bo_list) {
>   		/* ignore duplicates */
>   		bo = ttm_to_amdgpu_bo(e->tv.bo);
> @@ -1140,7 +1144,7 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
>   			return r;
>   	}
>   
> -	r = amdgpu_vm_handle_moved(adev, vm);
> +	r = amdgpu_vm_handle_moved(adev, vm, &p->ticket);
>   	if (r)
>   		return r;
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> index 271e30e34d93..23a213e4ab2c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> @@ -404,7 +404,7 @@ amdgpu_dma_buf_move_notify(struct dma_buf_attachment *attach)
>   
>   		r = amdgpu_vm_clear_freed(adev, vm, NULL);
>   		if (!r)
> -			r = amdgpu_vm_handle_moved(adev, vm);
> +			r = amdgpu_vm_handle_moved(adev, vm, ticket);
>   
>   		if (r && r != -EBUSY)
>   			DRM_ERROR("Failed to invalidate VM page tables (%d))\n",
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index dc379dc22c77..75dae2850e75 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1286,11 +1286,12 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
>    * PTs have to be reserved!
>    */
>   int amdgpu_vm_handle_moved(struct amdgpu_device *adev,
> -			   struct amdgpu_vm *vm)
> +			   struct amdgpu_vm *vm,
> +			   struct ww_acquire_ctx *ticket)
>   {
>   	struct amdgpu_bo_va *bo_va;
>   	struct dma_resv *resv;
> -	bool clear;
> +	bool clear, unlock;
>   	int r;
>   
>   	spin_lock(&vm->status_lock);
> @@ -1313,17 +1314,24 @@ int amdgpu_vm_handle_moved(struct amdgpu_device *adev,
>   		spin_unlock(&vm->status_lock);
>   
>   		/* Try to reserve the BO to avoid clearing its ptes */
> -		if (!amdgpu_vm_debug && dma_resv_trylock(resv))
> +		if (!amdgpu_vm_debug && dma_resv_trylock(resv)) {
>   			clear = false;
> +			unlock = true;
> +		/* The caller is already holding the reservation lock */
> +		} else if (ticket && dma_resv_locking_ctx(resv) == ticket) {
> +			clear = false;
> +			unlock = false;
>   		/* Somebody else is using the BO right now */
> -		else
> +		} else {
>   			clear = true;
> +			unlock = false;
> +		}
>   
>   		r = amdgpu_vm_bo_update(adev, bo_va, clear);
>   		if (r)
>   			return r;
>   
> -		if (!clear)
> +		if (unlock)
>   			dma_resv_unlock(resv);
>   		spin_lock(&vm->status_lock);
>   	}
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 094bb4807303..03a3314e5b43 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -400,7 +400,8 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
>   			  struct amdgpu_vm *vm,
>   			  struct dma_fence **fence);
>   int amdgpu_vm_handle_moved(struct amdgpu_device *adev,
> -			   struct amdgpu_vm *vm);
> +			   struct amdgpu_vm *vm,
> +			   struct ww_acquire_ctx *ticket);
>   void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base *base,
>   			    struct amdgpu_vm *vm, struct amdgpu_bo *bo);
>   int amdgpu_vm_update_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,

Re: [PATCH 4/6] drm/amdgpu: Attach eviction fence on alloc

[Chen, Xiaogang]

Reviewed-by: Xiaogang Chen <Xiaoganng.Chen@amd.com>

Regards

Xiaogang

On 1/11/2023 7:31 PM, Felix Kuehling wrote:
> Instead of attaching the eviction fence when a KFD BO is first mapped,
> attach it when it is allocated or imported. This in preparation to allow
> KFD BOs to be mapped using the render node API.
>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 63 ++++++++++---------
>   1 file changed, 32 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index 5645103beed0..79213f476493 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -360,6 +360,24 @@ static int amdgpu_amdkfd_bo_validate(struct amdgpu_bo *bo, uint32_t domain,
>   	return ret;
>   }
>   
> +static int amdgpu_amdkfd_bo_validate_and_fence(struct amdgpu_bo *bo,
> +					       uint32_t domain,
> +					       struct dma_fence *fence)
> +{
> +	int ret = amdgpu_bo_reserve(bo, false);
> +
> +	if (ret)
> +		return ret;
> +
> +	ret = amdgpu_amdkfd_bo_validate(bo, domain, true);
> +	if (!ret)
> +		dma_resv_add_fence(bo->tbo.base.resv, fence,
> +				   DMA_RESV_USAGE_BOOKKEEP);
> +	amdgpu_bo_unreserve(bo);
> +
> +	return ret;
> +}
> +
>   static int amdgpu_amdkfd_validate_vm_bo(void *_unused, struct amdgpu_bo *bo)
>   {
>   	return amdgpu_amdkfd_bo_validate(bo, bo->allowed_domains, false);
> @@ -1720,6 +1738,11 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
>   		}
>   		bo->allowed_domains = AMDGPU_GEM_DOMAIN_GTT;
>   		bo->preferred_domains = AMDGPU_GEM_DOMAIN_GTT;
> +	} else {
> +		ret = amdgpu_amdkfd_bo_validate_and_fence(bo, domain,
> +				&avm->process_info->eviction_fence->base);
> +		if (ret)
> +			goto err_validate_bo;
>   	}
>   
>   	if (offset)
> @@ -1729,6 +1752,7 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
>   
>   allocate_init_user_pages_failed:
>   err_pin_bo:
> +err_validate_bo:
>   	remove_kgd_mem_from_kfd_bo_list(*mem, avm->process_info);
>   	drm_vma_node_revoke(&gobj->vma_node, drm_priv);
>   err_node_allow:
> @@ -1804,10 +1828,6 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(
>   	if (unlikely(ret))
>   		return ret;
>   
> -	/* The eviction fence should be removed by the last unmap.
> -	 * TODO: Log an error condition if the bo still has the eviction fence
> -	 * attached
> -	 */
>   	amdgpu_amdkfd_remove_eviction_fence(mem->bo,
>   					process_info->eviction_fence);
>   	pr_debug("Release VA 0x%llx - 0x%llx\n", mem->va,
> @@ -1931,19 +1951,6 @@ int amdgpu_amdkfd_gpuvm_map_memory_to_gpu(
>   	if (unlikely(ret))
>   		goto out_unreserve;
>   
> -	if (mem->mapped_to_gpu_memory == 0 &&
> -	    !amdgpu_ttm_tt_get_usermm(bo->tbo.ttm)) {
> -		/* Validate BO only once. The eviction fence gets added to BO
> -		 * the first time it is mapped. Validate will wait for all
> -		 * background evictions to complete.
> -		 */
> -		ret = amdgpu_amdkfd_bo_validate(bo, domain, true);
> -		if (ret) {
> -			pr_debug("Validate failed\n");
> -			goto out_unreserve;
> -		}
> -	}
> -
>   	list_for_each_entry(entry, &mem->attachments, list) {
>   		if (entry->bo_va->base.vm != avm || entry->is_mapped)
>   			continue;
> @@ -1970,10 +1977,6 @@ int amdgpu_amdkfd_gpuvm_map_memory_to_gpu(
>   			 mem->mapped_to_gpu_memory);
>   	}
>   
> -	if (!amdgpu_ttm_tt_get_usermm(bo->tbo.ttm) && !bo->tbo.pin_count)
> -		dma_resv_add_fence(bo->tbo.base.resv,
> -				   &avm->process_info->eviction_fence->base,
> -				   DMA_RESV_USAGE_BOOKKEEP);
>   	ret = unreserve_bo_and_vms(&ctx, false, false);
>   
>   	goto out;
> @@ -1990,7 +1993,6 @@ int amdgpu_amdkfd_gpuvm_unmap_memory_from_gpu(
>   		struct amdgpu_device *adev, struct kgd_mem *mem, void *drm_priv)
>   {
>   	struct amdgpu_vm *avm = drm_priv_to_vm(drm_priv);
> -	struct amdkfd_process_info *process_info = avm->process_info;
>   	unsigned long bo_size = mem->bo->tbo.base.size;
>   	struct kfd_mem_attachment *entry;
>   	struct bo_vm_reservation_context ctx;
> @@ -2031,15 +2033,6 @@ int amdgpu_amdkfd_gpuvm_unmap_memory_from_gpu(
>   			 mem->mapped_to_gpu_memory);
>   	}
>   
> -	/* If BO is unmapped from all VMs, unfence it. It can be evicted if
> -	 * required.
> -	 */
> -	if (mem->mapped_to_gpu_memory == 0 &&
> -	    !amdgpu_ttm_tt_get_usermm(mem->bo->tbo.ttm) &&
> -	    !mem->bo->tbo.pin_count)
> -		amdgpu_amdkfd_remove_eviction_fence(mem->bo,
> -						process_info->eviction_fence);
> -
>   unreserve_out:
>   	unreserve_bo_and_vms(&ctx, false, false);
>   out:
> @@ -2266,8 +2259,16 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
>   	amdgpu_sync_create(&(*mem)->sync);
>   	(*mem)->is_imported = true;
>   
> +	ret = amdgpu_amdkfd_bo_validate_and_fence(bo, (*mem)->domain,
> +				&avm->process_info->eviction_fence->base);
> +	if (ret)
> +		goto err_remove_mem;
> +
>   	return 0;
>   
> +err_remove_mem:
> +	remove_kgd_mem_from_kfd_bo_list(*mem, avm->process_info);
> +	drm_vma_node_revoke(&obj->vma_node, drm_priv);
>   err_free_mem:
>   	kfree(*mem);
>   err_put_obj:

Re: [PATCH 5/6] drm/amdgpu: update mappings not managed by KFD

[Chen, Xiaogang]

Reviewed-by: Xiaogang Chen <Xiaoganng.Chen@amd.com>

Regards

Xiaogang

On 1/11/2023 7:31 PM, Felix Kuehling wrote:
> When restoring after an eviction, use amdgpu_vm_handle_moved to update
> BO VA mappings in KFD VMs that are not managed through the KFD API. This
> should allow using the render node API to create more flexible memory
> mappings in KFD VMs.
>
> v2: Sync with pd fence after all page table updates
> v3: Update comments, remove TODOs that are no longer applicable
>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 28 +++++++++++++++----
>   1 file changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index 79213f476493..df08e84f01d7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -2728,12 +2728,6 @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void *info, struct dma_fence **ef)
>   	if (ret)
>   		goto validate_map_fail;
>   
> -	ret = process_sync_pds_resv(process_info, &sync_obj);
> -	if (ret) {
> -		pr_debug("Memory eviction: Failed to sync to PD BO moving fence. Try again\n");
> -		goto validate_map_fail;
> -	}
> -
>   	/* Validate BOs and map them to GPUVM (update VM page tables). */
>   	list_for_each_entry(mem, &process_info->kfd_bo_list,
>   			    validate_list.head) {
> @@ -2781,6 +2775,19 @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void *info, struct dma_fence **ef)
>   	if (failed_size)
>   		pr_debug("0x%lx/0x%lx in system\n", failed_size, total_size);
>   
> +	/* Update mappings not managed by KFD */
> +	list_for_each_entry(peer_vm, &process_info->vm_list_head,
> +			vm_list_node) {
> +		struct amdgpu_device *adev = amdgpu_ttm_adev(
> +			peer_vm->root.bo->tbo.bdev);
> +
> +		ret = amdgpu_vm_handle_moved(adev, peer_vm, &ctx.ticket);
> +		if (ret) {
> +			pr_debug("Memory eviction: handle moved failed. Try again\n");
> +			goto validate_map_fail;
> +		}
> +	}
> +
>   	/* Update page directories */
>   	ret = process_update_pds(process_info, &sync_obj);
>   	if (ret) {
> @@ -2788,6 +2795,15 @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void *info, struct dma_fence **ef)
>   		goto validate_map_fail;
>   	}
>   
> +	/* Sync with fences on all the page tables. They implicitly depend on any
> +	 * move fences from amdgpu_vm_handle_moved above.
> +	 */
> +	ret = process_sync_pds_resv(process_info, &sync_obj);
> +	if (ret) {
> +		pr_debug("Memory eviction: Failed to sync to PD BO moving fence. Try again\n");
> +		goto validate_map_fail;
> +	}
> +
>   	/* Wait for validate and PT updates to finish */
>   	amdgpu_sync_wait(&sync_obj, false);
>   

Re: [PATCH 1/6] drm/amdgpu: Generalize KFD dmabuf import

[Felix Kuehling]

On 2023-01-12 17:41, Chen, Xiaogang wrote:
>
> On 1/11/2023 7:31 PM, Felix Kuehling wrote:
>> Use proper amdgpu_gem_prime_import function to handle all kinds of
>> imports. Remember the dmabuf reference to enable proper multi-GPU
>> attachment to multiple VMs without erroneously re-exporting the
>> underlying BO multiple times.
>>
>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
>> ---
>>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 38 ++++++++++---------
>>   1 file changed, 21 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> index 6f236ded5f12..e13c3493b786 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> @@ -2209,30 +2209,27 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct 
>> amdgpu_device *adev,
>>       struct amdgpu_bo *bo;
>>       int ret;
>>   -    if (dma_buf->ops != &amdgpu_dmabuf_ops)
>> -        /* Can't handle non-graphics buffers */
>> -        return -EINVAL;
>> -
>> -    obj = dma_buf->priv;
>> -    if (drm_to_adev(obj->dev) != adev)
>> -        /* Can't handle buffers from other devices */
>> -        return -EINVAL;
>> +    obj = amdgpu_gem_prime_import(adev_to_drm(adev), dma_buf);
>> +    if (IS_ERR(obj))
>> +        return PTR_ERR(obj);
>>         bo = gem_to_amdgpu_bo(obj);
>>       if (!(bo->preferred_domains & (AMDGPU_GEM_DOMAIN_VRAM |
>> -                    AMDGPU_GEM_DOMAIN_GTT)))
>> +                    AMDGPU_GEM_DOMAIN_GTT))) {
>>           /* Only VRAM and GTT BOs are supported */
>> -        return -EINVAL;
>> +        ret = -EINVAL;
>> +        goto err_put_obj;
>> +    }
>>         *mem = kzalloc(sizeof(struct kgd_mem), GFP_KERNEL);
>> -    if (!*mem)
>> -        return -ENOMEM;
>> +    if (!*mem) {
>> +        ret = -ENOMEM;
>> +        goto err_put_obj;
>> +    }
>>         ret = drm_vma_node_allow(&obj->vma_node, drm_priv);
>> -    if (ret) {
>> -        kfree(*mem);
>> -        return ret;
>> -    }
>> +    if (ret)
>> +        goto err_free_mem;
>>         if (size)
>>           *size = amdgpu_bo_size(bo);
>
> Hi Felix:
>
> I have a question when using amdgpu_gem_prime_import. It will allow 
> importing a dmabuf to different gpus, then can we still call 
> amdgpu_bo_mmap_offset on the generated bo if 
> amdgpu_amdkfd_gpuvm_import_dmabuf also ask mmap_offset?

The mmap  offset comes from drm_vma_node_offset_addr. The DRM VMA 
address is allocated when ttm_bo_init_reserved calls drm_vma_offset_add, 
so there should be no problem querying the mmap_offset. Whether mmapping 
of an imported BO is actually supported is a different question. As far 
as I can see, it should be possible. That said, currently ROCm 
(libhsakmt) uses only original BOs for CPU mappings, not imported BOs.

Regards,
   Felix


>
>> @@ -2249,7 +2246,8 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct 
>> amdgpu_device *adev,
>>           | KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE
>>           | KFD_IOC_ALLOC_MEM_FLAGS_EXECUTABLE;
>>   -    drm_gem_object_get(&bo->tbo.base);
>> +    get_dma_buf(dma_buf);
>> +    (*mem)->dmabuf = dma_buf;
>>       (*mem)->bo = bo;
>>       (*mem)->va = va;
>>       (*mem)->domain = (bo->preferred_domains & 
>> AMDGPU_GEM_DOMAIN_VRAM) ?
>> @@ -2261,6 +2259,12 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct 
>> amdgpu_device *adev,
>>       (*mem)->is_imported = true;
>>         return 0;
>> +
>> +err_free_mem:
>> +    kfree(*mem);
>> +err_put_obj:
>> +    drm_gem_object_put(obj);
>> +    return ret;
>>   }
>>     /* Evict a userptr BO by stopping the queues if necessary

Re: [PATCH 6/6] drm/amdgpu: Do bo_va ref counting for KFD BOs

[Chen, Xiaogang]

Reviewed-by: Xiaogang Chen <Xiaoganng.Chen@amd.com>

Regards

Xiaogang

On 1/11/2023 7:31 PM, Felix Kuehling wrote:
> This is needed to correctly handle BOs imported into the GEM API, which
> would otherwise get added twice to the same VM.
>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 28 +++++++++++++++----
>   1 file changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index df08e84f01d7..8b5ba2e04a79 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -370,9 +370,17 @@ static int amdgpu_amdkfd_bo_validate_and_fence(struct amdgpu_bo *bo,
>   		return ret;
>   
>   	ret = amdgpu_amdkfd_bo_validate(bo, domain, true);
> -	if (!ret)
> -		dma_resv_add_fence(bo->tbo.base.resv, fence,
> -				   DMA_RESV_USAGE_BOOKKEEP);
> +	if (ret)
> +		goto unreserve_out;
> +
> +	ret = dma_resv_reserve_fences(bo->tbo.base.resv, 1);
> +	if (ret)
> +		goto unreserve_out;
> +
> +	dma_resv_add_fence(bo->tbo.base.resv, fence,
> +			   DMA_RESV_USAGE_BOOKKEEP);
> +
> +unreserve_out:
>   	amdgpu_bo_unreserve(bo);
>   
>   	return ret;
> @@ -785,6 +793,7 @@ static int kfd_mem_attach(struct amdgpu_device *adev, struct kgd_mem *mem,
>   	uint64_t va = mem->va;
>   	struct kfd_mem_attachment *attachment[2] = {NULL, NULL};
>   	struct amdgpu_bo *bo[2] = {NULL, NULL};
> +	struct amdgpu_bo_va *bo_va;
>   	bool same_hive = false;
>   	int i, ret;
>   
> @@ -871,7 +880,12 @@ static int kfd_mem_attach(struct amdgpu_device *adev, struct kgd_mem *mem,
>   			pr_debug("Unable to reserve BO during memory attach");
>   			goto unwind;
>   		}
> -		attachment[i]->bo_va = amdgpu_vm_bo_add(adev, vm, bo[i]);
> +		bo_va = amdgpu_vm_bo_find(vm, bo[i]);
> +		if (!bo_va)
> +			bo_va = amdgpu_vm_bo_add(adev, vm, bo[i]);
> +		else
> +			++bo_va->ref_count;
> +		attachment[i]->bo_va = bo_va;
>   		amdgpu_bo_unreserve(bo[i]);
>   		if (unlikely(!attachment[i]->bo_va)) {
>   			ret = -ENOMEM;
> @@ -895,7 +909,8 @@ static int kfd_mem_attach(struct amdgpu_device *adev, struct kgd_mem *mem,
>   			continue;
>   		if (attachment[i]->bo_va) {
>   			amdgpu_bo_reserve(bo[i], true);
> -			amdgpu_vm_bo_del(adev, attachment[i]->bo_va);
> +			if (--attachment[i]->bo_va->ref_count == 0)
> +				amdgpu_vm_bo_del(adev, attachment[i]->bo_va);
>   			amdgpu_bo_unreserve(bo[i]);
>   			list_del(&attachment[i]->list);
>   		}
> @@ -912,7 +927,8 @@ static void kfd_mem_detach(struct kfd_mem_attachment *attachment)
>   
>   	pr_debug("\t remove VA 0x%llx in entry %p\n",
>   			attachment->va, attachment);
> -	amdgpu_vm_bo_del(attachment->adev, attachment->bo_va);
> +	if (--attachment->bo_va->ref_count == 0)
> +		amdgpu_vm_bo_del(attachment->adev, attachment->bo_va);
>   	drm_gem_object_put(&bo->tbo.base);
>   	list_del(&attachment->list);
>   	kfree(attachment);

Re: [PATCH 1/6] drm/amdgpu: Generalize KFD dmabuf import

[Chen, Xiaogang]

On 1/13/2023 4:26 PM, Felix Kuehling wrote:
> On 2023-01-12 17:41, Chen, Xiaogang wrote:
>>
>> On 1/11/2023 7:31 PM, Felix Kuehling wrote:
>>> Use proper amdgpu_gem_prime_import function to handle all kinds of
>>> imports. Remember the dmabuf reference to enable proper multi-GPU
>>> attachment to multiple VMs without erroneously re-exporting the
>>> underlying BO multiple times.
>>>
>>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
>>> ---
>>>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 38 
>>> ++++++++++---------
>>>   1 file changed, 21 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>> index 6f236ded5f12..e13c3493b786 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>> @@ -2209,30 +2209,27 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct 
>>> amdgpu_device *adev,
>>>       struct amdgpu_bo *bo;
>>>       int ret;
>>>   -    if (dma_buf->ops != &amdgpu_dmabuf_ops)
>>> -        /* Can't handle non-graphics buffers */
>>> -        return -EINVAL;
>>> -
>>> -    obj = dma_buf->priv;
>>> -    if (drm_to_adev(obj->dev) != adev)
>>> -        /* Can't handle buffers from other devices */
>>> -        return -EINVAL;
>>> +    obj = amdgpu_gem_prime_import(adev_to_drm(adev), dma_buf);
>>> +    if (IS_ERR(obj))
>>> +        return PTR_ERR(obj);
>>>         bo = gem_to_amdgpu_bo(obj);
>>>       if (!(bo->preferred_domains & (AMDGPU_GEM_DOMAIN_VRAM |
>>> -                    AMDGPU_GEM_DOMAIN_GTT)))
>>> +                    AMDGPU_GEM_DOMAIN_GTT))) {
>>>           /* Only VRAM and GTT BOs are supported */
>>> -        return -EINVAL;
>>> +        ret = -EINVAL;
>>> +        goto err_put_obj;
>>> +    }
>>>         *mem = kzalloc(sizeof(struct kgd_mem), GFP_KERNEL);
>>> -    if (!*mem)
>>> -        return -ENOMEM;
>>> +    if (!*mem) {
>>> +        ret = -ENOMEM;
>>> +        goto err_put_obj;
>>> +    }
>>>         ret = drm_vma_node_allow(&obj->vma_node, drm_priv);
>>> -    if (ret) {
>>> -        kfree(*mem);
>>> -        return ret;
>>> -    }
>>> +    if (ret)
>>> +        goto err_free_mem;
>>>         if (size)
>>>           *size = amdgpu_bo_size(bo);
>>
>> Hi Felix:
>>
>> I have a question when using amdgpu_gem_prime_import. It will allow 
>> importing a dmabuf to different gpus, then can we still call 
>> amdgpu_bo_mmap_offset on the generated bo if 
>> amdgpu_amdkfd_gpuvm_import_dmabuf also ask mmap_offset?
>
> The mmap  offset comes from drm_vma_node_offset_addr. The DRM VMA 
> address is allocated when ttm_bo_init_reserved calls 
> drm_vma_offset_add, so there should be no problem querying the 
> mmap_offset. Whether mmapping of an imported BO is actually supported 
> is a different question. As far as I can see, it should be possible. 
> That said, currently ROCm (libhsakmt) uses only original BOs for CPU 
> mappings, not imported BOs.
>
> Regards,
>   Felix

The mmap_offset is actually not returned to user space. I just wonder if 
here we should get mmap_offset of imported vram buffer if allow bo be 
imported to difference gpus. If a vram buffer is imported to same gpu 
device amdgpu_bo_mmap_offset is ok, otherwise I think 
amdgpu_bo_mmap_offset would not give correct mmap_offset for the device 
that the buffer is  imported to.

Maybe we should remove mmap_offset parameter of 
amdgpu_amdkfd_gpuvm_import_dmabuf since we do not return it to user 
space anyway. With that:

Reviewed-by: Xiaogang Chen <Xiaoganng.Chen@amd.com>

Regards

Xiaogang


>
>
>>
>>> @@ -2249,7 +2246,8 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct 
>>> amdgpu_device *adev,
>>>           | KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE
>>>           | KFD_IOC_ALLOC_MEM_FLAGS_EXECUTABLE;
>>>   -    drm_gem_object_get(&bo->tbo.base);
>>> +    get_dma_buf(dma_buf);
>>> +    (*mem)->dmabuf = dma_buf;
>>>       (*mem)->bo = bo;
>>>       (*mem)->va = va;
>>>       (*mem)->domain = (bo->preferred_domains & 
>>> AMDGPU_GEM_DOMAIN_VRAM) ?
>>> @@ -2261,6 +2259,12 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct 
>>> amdgpu_device *adev,
>>>       (*mem)->is_imported = true;
>>>         return 0;
>>> +
>>> +err_free_mem:
>>> +    kfree(*mem);
>>> +err_put_obj:
>>> +    drm_gem_object_put(obj);
>>> +    return ret;
>>>   }
>>>     /* Evict a userptr BO by stopping the queues if necessary

Re: [PATCH 1/6] drm/amdgpu: Generalize KFD dmabuf import

[Felix Kuehling]

On 2023-01-13 18:00, Chen, Xiaogang wrote:
>
> On 1/13/2023 4:26 PM, Felix Kuehling wrote:
>> On 2023-01-12 17:41, Chen, Xiaogang wrote:
>>>
>>> On 1/11/2023 7:31 PM, Felix Kuehling wrote:
>>>> Use proper amdgpu_gem_prime_import function to handle all kinds of
>>>> imports. Remember the dmabuf reference to enable proper multi-GPU
>>>> attachment to multiple VMs without erroneously re-exporting the
>>>> underlying BO multiple times.
>>>>
>>>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
>>>> ---
>>>>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 38 
>>>> ++++++++++---------
>>>>   1 file changed, 21 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>> index 6f236ded5f12..e13c3493b786 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>> @@ -2209,30 +2209,27 @@ int 
>>>> amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
>>>>       struct amdgpu_bo *bo;
>>>>       int ret;
>>>>   -    if (dma_buf->ops != &amdgpu_dmabuf_ops)
>>>> -        /* Can't handle non-graphics buffers */
>>>> -        return -EINVAL;
>>>> -
>>>> -    obj = dma_buf->priv;
>>>> -    if (drm_to_adev(obj->dev) != adev)
>>>> -        /* Can't handle buffers from other devices */
>>>> -        return -EINVAL;
>>>> +    obj = amdgpu_gem_prime_import(adev_to_drm(adev), dma_buf);
>>>> +    if (IS_ERR(obj))
>>>> +        return PTR_ERR(obj);
>>>>         bo = gem_to_amdgpu_bo(obj);
>>>>       if (!(bo->preferred_domains & (AMDGPU_GEM_DOMAIN_VRAM |
>>>> -                    AMDGPU_GEM_DOMAIN_GTT)))
>>>> +                    AMDGPU_GEM_DOMAIN_GTT))) {
>>>>           /* Only VRAM and GTT BOs are supported */
>>>> -        return -EINVAL;
>>>> +        ret = -EINVAL;
>>>> +        goto err_put_obj;
>>>> +    }
>>>>         *mem = kzalloc(sizeof(struct kgd_mem), GFP_KERNEL);
>>>> -    if (!*mem)
>>>> -        return -ENOMEM;
>>>> +    if (!*mem) {
>>>> +        ret = -ENOMEM;
>>>> +        goto err_put_obj;
>>>> +    }
>>>>         ret = drm_vma_node_allow(&obj->vma_node, drm_priv);
>>>> -    if (ret) {
>>>> -        kfree(*mem);
>>>> -        return ret;
>>>> -    }
>>>> +    if (ret)
>>>> +        goto err_free_mem;
>>>>         if (size)
>>>>           *size = amdgpu_bo_size(bo);
>>>
>>> Hi Felix:
>>>
>>> I have a question when using amdgpu_gem_prime_import. It will allow 
>>> importing a dmabuf to different gpus, then can we still call 
>>> amdgpu_bo_mmap_offset on the generated bo if 
>>> amdgpu_amdkfd_gpuvm_import_dmabuf also ask mmap_offset?
>>
>> The mmap  offset comes from drm_vma_node_offset_addr. The DRM VMA 
>> address is allocated when ttm_bo_init_reserved calls 
>> drm_vma_offset_add, so there should be no problem querying the 
>> mmap_offset. Whether mmapping of an imported BO is actually supported 
>> is a different question. As far as I can see, it should be possible. 
>> That said, currently ROCm (libhsakmt) uses only original BOs for CPU 
>> mappings, not imported BOs.
>>
>> Regards,
>>   Felix
>
> The mmap_offset is actually not returned to user space. I just wonder 
> if here we should get mmap_offset of imported vram buffer if allow bo 
> be imported to difference gpus. If a vram buffer is imported to same 
> gpu device amdgpu_bo_mmap_offset is ok, otherwise I think 
> amdgpu_bo_mmap_offset would not give correct mmap_offset for the 
> device that the buffer is  imported to.

When the BO is imported into the same GPU, you get a reference to the 
same BO, so the imported BO has the same mmap_offset as the original BO.

When the BO is imported into a different GPU, it is a new BO with a new 
mmap_offset. I don't think this is incorrect. mmapping the memory with 
that new offset should still work. The imported BO is created with 
ttm_bo_type_sg, and AFAICT ttm_bo_vm.c supports mapping of SG BOs.

Regards,
   Felix


>
> Maybe we should remove mmap_offset parameter of 
> amdgpu_amdkfd_gpuvm_import_dmabuf since we do not return it to user 
> space anyway. With that:
>
> Reviewed-by: Xiaogang Chen <Xiaoganng.Chen@amd.com>
>
> Regards
>
> Xiaogang
>
>
>>
>>
>>>
>>>> @@ -2249,7 +2246,8 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct 
>>>> amdgpu_device *adev,
>>>>           | KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE
>>>>           | KFD_IOC_ALLOC_MEM_FLAGS_EXECUTABLE;
>>>>   -    drm_gem_object_get(&bo->tbo.base);
>>>> +    get_dma_buf(dma_buf);
>>>> +    (*mem)->dmabuf = dma_buf;
>>>>       (*mem)->bo = bo;
>>>>       (*mem)->va = va;
>>>>       (*mem)->domain = (bo->preferred_domains & 
>>>> AMDGPU_GEM_DOMAIN_VRAM) ?
>>>> @@ -2261,6 +2259,12 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct 
>>>> amdgpu_device *adev,
>>>>       (*mem)->is_imported = true;
>>>>         return 0;
>>>> +
>>>> +err_free_mem:
>>>> +    kfree(*mem);
>>>> +err_put_obj:
>>>> +    drm_gem_object_put(obj);
>>>> +    return ret;
>>>>   }
>>>>     /* Evict a userptr BO by stopping the queues if necessary

Re: [PATCH 1/6] drm/amdgpu: Generalize KFD dmabuf import

[Christian König]

Am 14.01.23 um 00:15 schrieb Felix Kuehling:
> On 2023-01-13 18:00, Chen, Xiaogang wrote:
>>
>> On 1/13/2023 4:26 PM, Felix Kuehling wrote:
>>> On 2023-01-12 17:41, Chen, Xiaogang wrote:
>>>>
>>>> On 1/11/2023 7:31 PM, Felix Kuehling wrote:
>>>>> Use proper amdgpu_gem_prime_import function to handle all kinds of
>>>>> imports. Remember the dmabuf reference to enable proper multi-GPU
>>>>> attachment to multiple VMs without erroneously re-exporting the
>>>>> underlying BO multiple times.
>>>>>
>>>>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
>>>>> ---
>>>>>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 38 
>>>>> ++++++++++---------
>>>>>   1 file changed, 21 insertions(+), 17 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c 
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>>> index 6f236ded5f12..e13c3493b786 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>>> @@ -2209,30 +2209,27 @@ int 
>>>>> amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
>>>>>       struct amdgpu_bo *bo;
>>>>>       int ret;
>>>>>   -    if (dma_buf->ops != &amdgpu_dmabuf_ops)
>>>>> -        /* Can't handle non-graphics buffers */
>>>>> -        return -EINVAL;
>>>>> -
>>>>> -    obj = dma_buf->priv;
>>>>> -    if (drm_to_adev(obj->dev) != adev)
>>>>> -        /* Can't handle buffers from other devices */
>>>>> -        return -EINVAL;
>>>>> +    obj = amdgpu_gem_prime_import(adev_to_drm(adev), dma_buf);
>>>>> +    if (IS_ERR(obj))
>>>>> +        return PTR_ERR(obj);
>>>>>         bo = gem_to_amdgpu_bo(obj);
>>>>>       if (!(bo->preferred_domains & (AMDGPU_GEM_DOMAIN_VRAM |
>>>>> -                    AMDGPU_GEM_DOMAIN_GTT)))
>>>>> +                    AMDGPU_GEM_DOMAIN_GTT))) {
>>>>>           /* Only VRAM and GTT BOs are supported */
>>>>> -        return -EINVAL;
>>>>> +        ret = -EINVAL;
>>>>> +        goto err_put_obj;
>>>>> +    }
>>>>>         *mem = kzalloc(sizeof(struct kgd_mem), GFP_KERNEL);
>>>>> -    if (!*mem)
>>>>> -        return -ENOMEM;
>>>>> +    if (!*mem) {
>>>>> +        ret = -ENOMEM;
>>>>> +        goto err_put_obj;
>>>>> +    }
>>>>>         ret = drm_vma_node_allow(&obj->vma_node, drm_priv);
>>>>> -    if (ret) {
>>>>> -        kfree(*mem);
>>>>> -        return ret;
>>>>> -    }
>>>>> +    if (ret)
>>>>> +        goto err_free_mem;
>>>>>         if (size)
>>>>>           *size = amdgpu_bo_size(bo);
>>>>
>>>> Hi Felix:
>>>>
>>>> I have a question when using amdgpu_gem_prime_import. It will allow 
>>>> importing a dmabuf to different gpus, then can we still call 
>>>> amdgpu_bo_mmap_offset on the generated bo if 
>>>> amdgpu_amdkfd_gpuvm_import_dmabuf also ask mmap_offset?
>>>
>>> The mmap  offset comes from drm_vma_node_offset_addr. The DRM VMA 
>>> address is allocated when ttm_bo_init_reserved calls 
>>> drm_vma_offset_add, so there should be no problem querying the 
>>> mmap_offset. Whether mmapping of an imported BO is actually 
>>> supported is a different question. As far as I can see, it should be 
>>> possible. That said, currently ROCm (libhsakmt) uses only original 
>>> BOs for CPU mappings, not imported BOs.
>>>
>>> Regards,
>>>   Felix
>>
>> The mmap_offset is actually not returned to user space. I just wonder 
>> if here we should get mmap_offset of imported vram buffer if allow bo 
>> be imported to difference gpus. If a vram buffer is imported to same 
>> gpu device amdgpu_bo_mmap_offset is ok, otherwise I think 
>> amdgpu_bo_mmap_offset would not give correct mmap_offset for the 
>> device that the buffer is  imported to.
>
> When the BO is imported into the same GPU, you get a reference to the 
> same BO, so the imported BO has the same mmap_offset as the original BO.
>
> When the BO is imported into a different GPU, it is a new BO with a 
> new mmap_offset.

That won't work.

> I don't think this is incorrect.

No, this is completely incorrect. It mixes up the reverse tracking of 
mappings and might crash the system. This is the reason why we can't 
mmap() imported BOs.

> mmapping the memory with that new offset should still work. The 
> imported BO is created with ttm_bo_type_sg, and AFAICT ttm_bo_vm.c 
> supports mapping of SG BOs.

Actually it shouldn't. This can go boom really easily.

When you have imported a BO the only correct way of to mmap() it is to 
do so on the original exporter.

Regards,
Christian.

>
> Regards,
>   Felix
>
>
>>
>> Maybe we should remove mmap_offset parameter of 
>> amdgpu_amdkfd_gpuvm_import_dmabuf since we do not return it to user 
>> space anyway. With that:
>>
>> Reviewed-by: Xiaogang Chen <Xiaoganng.Chen@amd.com>
>>
>> Regards
>>
>> Xiaogang
>>
>>
>>>
>>>
>>>>
>>>>> @@ -2249,7 +2246,8 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct 
>>>>> amdgpu_device *adev,
>>>>>           | KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE
>>>>>           | KFD_IOC_ALLOC_MEM_FLAGS_EXECUTABLE;
>>>>>   -    drm_gem_object_get(&bo->tbo.base);
>>>>> +    get_dma_buf(dma_buf);
>>>>> +    (*mem)->dmabuf = dma_buf;
>>>>>       (*mem)->bo = bo;
>>>>>       (*mem)->va = va;
>>>>>       (*mem)->domain = (bo->preferred_domains & 
>>>>> AMDGPU_GEM_DOMAIN_VRAM) ?
>>>>> @@ -2261,6 +2259,12 @@ int 
>>>>> amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
>>>>>       (*mem)->is_imported = true;
>>>>>         return 0;
>>>>> +
>>>>> +err_free_mem:
>>>>> +    kfree(*mem);
>>>>> +err_put_obj:
>>>>> +    drm_gem_object_put(obj);
>>>>> +    return ret;
>>>>>   }
>>>>>     /* Evict a userptr BO by stopping the queues if necessary

Re: [PATCH 1/6] drm/amdgpu: Generalize KFD dmabuf import

[Felix Kuehling]

Am 2023-01-15 um 11:43 schrieb Christian König:
>
>
> Am 14.01.23 um 00:15 schrieb Felix Kuehling:
>> On 2023-01-13 18:00, Chen, Xiaogang wrote:
>>>
>>> On 1/13/2023 4:26 PM, Felix Kuehling wrote:
>>>> On 2023-01-12 17:41, Chen, Xiaogang wrote:
>>>>>
>>>>> On 1/11/2023 7:31 PM, Felix Kuehling wrote:
>>>>>> Use proper amdgpu_gem_prime_import function to handle all kinds of
>>>>>> imports. Remember the dmabuf reference to enable proper multi-GPU
>>>>>> attachment to multiple VMs without erroneously re-exporting the
>>>>>> underlying BO multiple times.
>>>>>>
>>>>>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
>>>>>> ---
>>>>>>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 38 
>>>>>> ++++++++++---------
>>>>>>   1 file changed, 21 insertions(+), 17 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c 
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>>>> index 6f236ded5f12..e13c3493b786 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>>>>>> @@ -2209,30 +2209,27 @@ int 
>>>>>> amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
>>>>>>       struct amdgpu_bo *bo;
>>>>>>       int ret;
>>>>>>   -    if (dma_buf->ops != &amdgpu_dmabuf_ops)
>>>>>> -        /* Can't handle non-graphics buffers */
>>>>>> -        return -EINVAL;
>>>>>> -
>>>>>> -    obj = dma_buf->priv;
>>>>>> -    if (drm_to_adev(obj->dev) != adev)
>>>>>> -        /* Can't handle buffers from other devices */
>>>>>> -        return -EINVAL;
>>>>>> +    obj = amdgpu_gem_prime_import(adev_to_drm(adev), dma_buf);
>>>>>> +    if (IS_ERR(obj))
>>>>>> +        return PTR_ERR(obj);
>>>>>>         bo = gem_to_amdgpu_bo(obj);
>>>>>>       if (!(bo->preferred_domains & (AMDGPU_GEM_DOMAIN_VRAM |
>>>>>> -                    AMDGPU_GEM_DOMAIN_GTT)))
>>>>>> +                    AMDGPU_GEM_DOMAIN_GTT))) {
>>>>>>           /* Only VRAM and GTT BOs are supported */
>>>>>> -        return -EINVAL;
>>>>>> +        ret = -EINVAL;
>>>>>> +        goto err_put_obj;
>>>>>> +    }
>>>>>>         *mem = kzalloc(sizeof(struct kgd_mem), GFP_KERNEL);
>>>>>> -    if (!*mem)
>>>>>> -        return -ENOMEM;
>>>>>> +    if (!*mem) {
>>>>>> +        ret = -ENOMEM;
>>>>>> +        goto err_put_obj;
>>>>>> +    }
>>>>>>         ret = drm_vma_node_allow(&obj->vma_node, drm_priv);
>>>>>> -    if (ret) {
>>>>>> -        kfree(*mem);
>>>>>> -        return ret;
>>>>>> -    }
>>>>>> +    if (ret)
>>>>>> +        goto err_free_mem;
>>>>>>         if (size)
>>>>>>           *size = amdgpu_bo_size(bo);
>>>>>
>>>>> Hi Felix:
>>>>>
>>>>> I have a question when using amdgpu_gem_prime_import. It will 
>>>>> allow importing a dmabuf to different gpus, then can we still call 
>>>>> amdgpu_bo_mmap_offset on the generated bo if 
>>>>> amdgpu_amdkfd_gpuvm_import_dmabuf also ask mmap_offset?
>>>>
>>>> The mmap  offset comes from drm_vma_node_offset_addr. The DRM VMA 
>>>> address is allocated when ttm_bo_init_reserved calls 
>>>> drm_vma_offset_add, so there should be no problem querying the 
>>>> mmap_offset. Whether mmapping of an imported BO is actually 
>>>> supported is a different question. As far as I can see, it should 
>>>> be possible. That said, currently ROCm (libhsakmt) uses only 
>>>> original BOs for CPU mappings, not imported BOs.
>>>>
>>>> Regards,
>>>>   Felix
>>>
>>> The mmap_offset is actually not returned to user space. I just 
>>> wonder if here we should get mmap_offset of imported vram buffer if 
>>> allow bo be imported to difference gpus. If a vram buffer is 
>>> imported to same gpu device amdgpu_bo_mmap_offset is ok, otherwise I 
>>> think amdgpu_bo_mmap_offset would not give correct mmap_offset for 
>>> the device that the buffer is imported to.
>>
>> When the BO is imported into the same GPU, you get a reference to the 
>> same BO, so the imported BO has the same mmap_offset as the original BO.
>>
>> When the BO is imported into a different GPU, it is a new BO with a 
>> new mmap_offset.
>
> That won't work.
>
>> I don't think this is incorrect.
>
> No, this is completely incorrect. It mixes up the reverse tracking of 
> mappings and might crash the system.

I don't understand that. The imported BO is a different BO with a 
different mmap offset in a different device file. I don't see how that 
messes with the tracking of mappings.


> This is the reason why we can't mmap() imported BOs.

I don't see anything preventing that. For userptr BOs, there is this 
code in amdgpu_gem_object_mmap:

         if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm))
                 return -EPERM;

I don't see anything like this preventing mmapping of imported dmabuf 
BOs. What am I missing?


>
>> mmapping the memory with that new offset should still work. The 
>> imported BO is created with ttm_bo_type_sg, and AFAICT ttm_bo_vm.c 
>> supports mapping of SG BOs.
>
> Actually it shouldn't. This can go boom really easily.

OK. I don't think we're doing this, but after Xiaogang raised the 
question I went looking through the code whether it's theoretically 
possible. I didn't find anything in the code that says that mmapping 
imported dmabufs would be prohibited or even dangerous. On the contrary, 
I found that ttm_bo_vm explicitly supports mmapping SG BOs.


>
> When you have imported a BO the only correct way of to mmap() it is to 
> do so on the original exporter.

That seems sensible, and this is what we do today. That said, if 
mmapping an imported BO is dangerous, I'm missing a mechanism to protect 
against this. It could be as simple as setting 
AMDGPU_GEM_CREATE_NO_CPU_ACCESS in amdgpu_dma_buf_create_obj.

Regards,
   Felix


>
> Regards,
> Christian.
>
>>
>> Regards,
>>   Felix
>>
>>
>>>
>>> Maybe we should remove mmap_offset parameter of 
>>> amdgpu_amdkfd_gpuvm_import_dmabuf since we do not return it to user 
>>> space anyway. With that:
>>>
>>> Reviewed-by: Xiaogang Chen <Xiaoganng.Chen@amd.com>
>>>
>>> Regards
>>>
>>> Xiaogang
>>>
>>>
>>>>
>>>>
>>>>>
>>>>>> @@ -2249,7 +2246,8 @@ int 
>>>>>> amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
>>>>>>           | KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE
>>>>>>           | KFD_IOC_ALLOC_MEM_FLAGS_EXECUTABLE;
>>>>>>   -    drm_gem_object_get(&bo->tbo.base);
>>>>>> +    get_dma_buf(dma_buf);
>>>>>> +    (*mem)->dmabuf = dma_buf;
>>>>>>       (*mem)->bo = bo;
>>>>>>       (*mem)->va = va;
>>>>>>       (*mem)->domain = (bo->preferred_domains & 
>>>>>> AMDGPU_GEM_DOMAIN_VRAM) ?
>>>>>> @@ -2261,6 +2259,12 @@ int 
>>>>>> amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
>>>>>>       (*mem)->is_imported = true;
>>>>>>         return 0;
>>>>>> +
>>>>>> +err_free_mem:
>>>>>> +    kfree(*mem);
>>>>>> +err_put_obj:
>>>>>> +    drm_gem_object_put(obj);
>>>>>> +    return ret;
>>>>>>   }
>>>>>>     /* Evict a userptr BO by stopping the queues if necessary
>

Re: [PATCH 1/6] drm/amdgpu: Generalize KFD dmabuf import

[Christian König]

[SNIP]
>>> When the BO is imported into the same GPU, you get a reference to 
>>> the same BO, so the imported BO has the same mmap_offset as the 
>>> original BO.
>>>
>>> When the BO is imported into a different GPU, it is a new BO with a 
>>> new mmap_offset.
>>
>> That won't work.
>>
>>> I don't think this is incorrect.
>>
>> No, this is completely incorrect. It mixes up the reverse tracking of 
>> mappings and might crash the system.
>
> I don't understand that. The imported BO is a different BO with a 
> different mmap offset in a different device file. I don't see how that 
> messes with the tracking of mappings.

The tracking keeps note which piece of information is accessible through 
which address space object and offset. I you suddenly have two address 
spaces and offsets pointing to the same piece of information that won't 
work any more.

>
>> This is the reason why we can't mmap() imported BOs.
>
> I don't see anything preventing that. For userptr BOs, there is this 
> code in amdgpu_gem_object_mmap:
>
>         if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm))
>                 return -EPERM;
>
> I don't see anything like this preventing mmapping of imported dmabuf 
> BOs. What am I missing?
>

At some point I really need to make a big presentation about all this 
stuff, we had the same discussion multiple times now :)

It's the same reason why you can't mmap() VRAM through the kfd node: 
Each file can have only one address space object associated with it.

See dma_buf_mmap() and vma_set_file() how this is worked around in DMA-buf.

>>
>>> mmapping the memory with that new offset should still work. The 
>>> imported BO is created with ttm_bo_type_sg, and AFAICT ttm_bo_vm.c 
>>> supports mapping of SG BOs.
>>
>> Actually it shouldn't. This can go boom really easily.
>
> OK. I don't think we're doing this, but after Xiaogang raised the 
> question I went looking through the code whether it's theoretically 
> possible. I didn't find anything in the code that says that mmapping 
> imported dmabufs would be prohibited or even dangerous. On the 
> contrary, I found that ttm_bo_vm explicitly supports mmapping SG BOs.
>
>
>>
>> When you have imported a BO the only correct way of to mmap() it is 
>> to do so on the original exporter.
>
> That seems sensible, and this is what we do today. That said, if 
> mmapping an imported BO is dangerous, I'm missing a mechanism to 
> protect against this. It could be as simple as setting 
> AMDGPU_GEM_CREATE_NO_CPU_ACCESS in amdgpu_dma_buf_create_obj.

At least for the GEM mmap() handler this is double checked very early by 
looking at obj->import_attach and then either rejecting it or 
redirecting the request to the DMA-buf file instead.

We probably need something similar when stuff is mapped through the KFD 
node. But I think we don't do that any more for "normal" BOs anyway, 
don't we?

Regards,
Christian.

>
> Regards,
>   Felix

Re: [PATCH 1/6] drm/amdgpu: Generalize KFD dmabuf import

[Felix Kuehling]

Am 2023-01-16 um 06:42 schrieb Christian König:
> [SNIP]
>>>> When the BO is imported into the same GPU, you get a reference to 
>>>> the same BO, so the imported BO has the same mmap_offset as the 
>>>> original BO.
>>>>
>>>> When the BO is imported into a different GPU, it is a new BO with a 
>>>> new mmap_offset.
>>>
>>> That won't work.
>>>
>>>> I don't think this is incorrect.
>>>
>>> No, this is completely incorrect. It mixes up the reverse tracking 
>>> of mappings and might crash the system.
>>
>> I don't understand that. The imported BO is a different BO with a 
>> different mmap offset in a different device file. I don't see how 
>> that messes with the tracking of mappings.
>
> The tracking keeps note which piece of information is accessible 
> through which address space object and offset. I you suddenly have two 
> address spaces and offsets pointing to the same piece of information 
> that won't work any more.

How do you identify a "piece of information". I don't think it's the 
physical page. VRAM doesn't even have struct pages. I think it's the BO 
that's being tracked. With a dmabuf import you have a second BO aliasing 
the same physical pages as the original BO. Then those two BOs are seen 
as two distinct "pieces of information" that can each have their own 
mapping.


>
>>
>>> This is the reason why we can't mmap() imported BOs.
>>
>> I don't see anything preventing that. For userptr BOs, there is this 
>> code in amdgpu_gem_object_mmap:
>>
>>         if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm))
>>                 return -EPERM;
>>
>> I don't see anything like this preventing mmapping of imported dmabuf 
>> BOs. What am I missing?
>>
>
> At some point I really need to make a big presentation about all this 
> stuff, we had the same discussion multiple times now :)
>
> It's the same reason why you can't mmap() VRAM through the kfd node: 
> Each file can have only one address space object associated with it.

I remember that. We haven't used KFD to mmap BOs for a long time for 
that reason.


>
> See dma_buf_mmap() and vma_set_file() how this is worked around in 
> DMA-buf.

These are for mmapping memory through the dmabuf fd. I'm not sure that's 
a good example. drm_gem_prime_mmap creates a temporary struct file and 
struct drm_file that are destroyed immediately after calling 
obj->dev->driver->fops->mmap. I think that would break any reverse mapping.


>
>>>
>>>> mmapping the memory with that new offset should still work. The 
>>>> imported BO is created with ttm_bo_type_sg, and AFAICT ttm_bo_vm.c 
>>>> supports mapping of SG BOs.
>>>
>>> Actually it shouldn't. This can go boom really easily.
>>
>> OK. I don't think we're doing this, but after Xiaogang raised the 
>> question I went looking through the code whether it's theoretically 
>> possible. I didn't find anything in the code that says that mmapping 
>> imported dmabufs would be prohibited or even dangerous. On the 
>> contrary, I found that ttm_bo_vm explicitly supports mmapping SG BOs.
>>
>>
>>>
>>> When you have imported a BO the only correct way of to mmap() it is 
>>> to do so on the original exporter.
>>
>> That seems sensible, and this is what we do today. That said, if 
>> mmapping an imported BO is dangerous, I'm missing a mechanism to 
>> protect against this. It could be as simple as setting 
>> AMDGPU_GEM_CREATE_NO_CPU_ACCESS in amdgpu_dma_buf_create_obj.
>
> At least for the GEM mmap() handler this is double checked very early 
> by looking at obj->import_attach and then either rejecting it or 
> redirecting the request to the DMA-buf file instead.

Can you point me at where this check is? I see a check for 
obj->import_attach in drm_gem_dumb_map_offset. But I can't see how this 
function is called in amdgpu. I don't think it is used at all.


>
> We probably need something similar when stuff is mapped through the 
> KFD node. But I think we don't do that any more for "normal" BOs 
> anyway, don't we?

Correct, we don't map BOs through the KFD device file. The only mappings 
we still use it for are:

  * Doorbells on APUs
  * Events page on APUs
  * MMIO page for HDP flushing

The code for mmapping regular BOs through /dev/kfd was never upstream.

Regards,
   Felix


>
> Regards,
> Christian.
>
>>
>> Regards,
>>   Felix
>

Re: [PATCH 1/6] drm/amdgpu: Generalize KFD dmabuf import

[Christian König]

Am 16.01.23 um 15:52 schrieb Felix Kuehling:
> Am 2023-01-16 um 06:42 schrieb Christian König:
>> [SNIP]
>>>>> When the BO is imported into the same GPU, you get a reference to 
>>>>> the same BO, so the imported BO has the same mmap_offset as the 
>>>>> original BO.
>>>>>
>>>>> When the BO is imported into a different GPU, it is a new BO with 
>>>>> a new mmap_offset.
>>>>
>>>> That won't work.
>>>>
>>>>> I don't think this is incorrect.
>>>>
>>>> No, this is completely incorrect. It mixes up the reverse tracking 
>>>> of mappings and might crash the system.
>>>
>>> I don't understand that. The imported BO is a different BO with a 
>>> different mmap offset in a different device file. I don't see how 
>>> that messes with the tracking of mappings.
>>
>> The tracking keeps note which piece of information is accessible 
>> through which address space object and offset. I you suddenly have 
>> two address spaces and offsets pointing to the same piece of 
>> information that won't work any more.
>
> How do you identify a "piece of information". I don't think it's the 
> physical page. VRAM doesn't even have struct pages. I think it's the 
> BO that's being tracked.

Both struct page as well as pfn. Essentially everything you can give 
vm_insert_pfn() or vm_insert_page() as parameter.

> With a dmabuf import you have a second BO aliasing the same physical 
> pages as the original BO.

No, exactly that's wrong. You have a BO which contains a bunch of dma 
addresses. The pages or whatever lies behind those as backing store are 
intentionally hidden from the importer.

Daniel even started mangling the page pointer to prevent people from 
using them: 
https://elixir.bootlin.com/linux/latest/source/drivers/dma-buf/dma-buf.c#L769

> Then those two BOs are seen as two distinct "pieces of information" 
> that can each have their own mapping.

Well I need to correct myself: Creating a "fake" offset for the BO is 
ok, but we should just never ever mmap() through two different address 
spaces.

>
>>>
>>>> This is the reason why we can't mmap() imported BOs.
>>>
>>> I don't see anything preventing that. For userptr BOs, there is this 
>>> code in amdgpu_gem_object_mmap:
>>>
>>>         if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm))
>>>                 return -EPERM;
>>>
>>> I don't see anything like this preventing mmapping of imported 
>>> dmabuf BOs. What am I missing?
>>>
>>
>> At some point I really need to make a big presentation about all this 
>> stuff, we had the same discussion multiple times now :)
>>
>> It's the same reason why you can't mmap() VRAM through the kfd node: 
>> Each file can have only one address space object associated with it.
>
> I remember that. We haven't used KFD to mmap BOs for a long time for 
> that reason.
>
>
>>
>> See dma_buf_mmap() and vma_set_file() how this is worked around in 
>> DMA-buf.
>
> These are for mmapping memory through the dmabuf fd. I'm not sure 
> that's a good example. drm_gem_prime_mmap creates a temporary struct 
> file and struct drm_file that are destroyed immediately after calling 
> obj->dev->driver->fops->mmap. I think that would break any reverse 
> mapping.

I was hoping we have removed that code by now. Today obj->funcs->mmap 
should be used and not the hack with temporary creating an file/fpriv 
any more.

>>>>
>>>>> mmapping the memory with that new offset should still work. The 
>>>>> imported BO is created with ttm_bo_type_sg, and AFAICT ttm_bo_vm.c 
>>>>> supports mapping of SG BOs.
>>>>
>>>> Actually it shouldn't. This can go boom really easily.
>>>
>>> OK. I don't think we're doing this, but after Xiaogang raised the 
>>> question I went looking through the code whether it's theoretically 
>>> possible. I didn't find anything in the code that says that mmapping 
>>> imported dmabufs would be prohibited or even dangerous. On the 
>>> contrary, I found that ttm_bo_vm explicitly supports mmapping SG BOs.
>>>
>>>
>>>>
>>>> When you have imported a BO the only correct way of to mmap() it is 
>>>> to do so on the original exporter.
>>>
>>> That seems sensible, and this is what we do today. That said, if 
>>> mmapping an imported BO is dangerous, I'm missing a mechanism to 
>>> protect against this. It could be as simple as setting 
>>> AMDGPU_GEM_CREATE_NO_CPU_ACCESS in amdgpu_dma_buf_create_obj.
>>
>> At least for the GEM mmap() handler this is double checked very early 
>> by looking at obj->import_attach and then either rejecting it or 
>> redirecting the request to the DMA-buf file instead.
>
> Can you point me at where this check is? I see a check for 
> obj->import_attach in drm_gem_dumb_map_offset. But I can't see how 
> this function is called in amdgpu. I don't think it is used at all.

Uff, good question! @Thomas and @Dmitry: I clearly remember that one of 
you guys was involved in the DRM/GEM mmap cleanup and DMA-buf with 
workarounds for the KFD and DMA-buf.

What was the final solution to this? I can't find it of hand any more.

>>
>> We probably need something similar when stuff is mapped through the 
>> KFD node. But I think we don't do that any more for "normal" BOs 
>> anyway, don't we?
>
> Correct, we don't map BOs through the KFD device file. The only 
> mappings we still use it for are:
>
>  * Doorbells on APUs
>  * Events page on APUs
>  * MMIO page for HDP flushing
>
> The code for mmapping regular BOs through /dev/kfd was never upstream.

Good, that's probably much less problematic.

Regards,
Christian.


>
> Regards,
>   Felix
>
>
>>
>> Regards,
>> Christian.
>>
>>>
>>> Regards,
>>>   Felix
>>

RE: [PATCH 1/6] drm/amdgpu: Generalize KFD dmabuf import

[Errabolu, Ramesh]

[AMD Official Use Only - General]

A minor comment, unrelated to the patch. The comments are inline.

Regards,
Ramesh

-----Original Message-----
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Felix Kuehling
Sent: Thursday, January 12, 2023 7:02 AM
To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
Cc: Chen, Xiaogang <Xiaogang.Chen@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>
Subject: [PATCH 1/6] drm/amdgpu: Generalize KFD dmabuf import

Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.


Use proper amdgpu_gem_prime_import function to handle all kinds of imports. Remember the dmabuf reference to enable proper multi-GPU attachment to multiple VMs without erroneously re-exporting the underlying BO multiple times.

Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 38 ++++++++++---------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 6f236ded5f12..e13c3493b786 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -2209,30 +2209,27 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
        struct amdgpu_bo *bo;
        int ret;

-       if (dma_buf->ops != &amdgpu_dmabuf_ops)
-               /* Can't handle non-graphics buffers */
-               return -EINVAL;
-
-       obj = dma_buf->priv;
-       if (drm_to_adev(obj->dev) != adev)
-               /* Can't handle buffers from other devices */
-               return -EINVAL;
+       obj = amdgpu_gem_prime_import(adev_to_drm(adev), dma_buf);
+       if (IS_ERR(obj))
+               return PTR_ERR(obj);

        bo = gem_to_amdgpu_bo(obj);
        if (!(bo->preferred_domains & (AMDGPU_GEM_DOMAIN_VRAM |
-                                   AMDGPU_GEM_DOMAIN_GTT)))
+                                   AMDGPU_GEM_DOMAIN_GTT))) {
                /* Only VRAM and GTT BOs are supported */
-               return -EINVAL;
+               ret = -EINVAL;
+               goto err_put_obj;
+       }

        *mem = kzalloc(sizeof(struct kgd_mem), GFP_KERNEL);
-       if (!*mem)
-               return -ENOMEM;
+       if (!*mem) {
+               ret = -ENOMEM;
+               goto err_put_obj;
+       }

        ret = drm_vma_node_allow(&obj->vma_node, drm_priv);
-       if (ret) {
-               kfree(*mem);
-               return ret;
-       }
+       if (ret)
+               goto err_free_mem;

        if (size)
                *size = amdgpu_bo_size(bo); @@ -2249,7 +2246,8 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
                | KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE
                | KFD_IOC_ALLOC_MEM_FLAGS_EXECUTABLE;
Ramesh: Is it correct to assign WRITE & EXECUTABLE permissions to alloc_flags variable.

-       drm_gem_object_get(&bo->tbo.base);
+       get_dma_buf(dma_buf);
+       (*mem)->dmabuf = dma_buf;
        (*mem)->bo = bo;
        (*mem)->va = va;
        (*mem)->domain = (bo->preferred_domains & AMDGPU_GEM_DOMAIN_VRAM) ?
@@ -2261,6 +2259,12 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
        (*mem)->is_imported = true;

        return 0;
+
+err_free_mem:
+       kfree(*mem);
+err_put_obj:
+       drm_gem_object_put(obj);
+       return ret;
 }

 /* Evict a userptr BO by stopping the queues if necessary
--
2.34.1

RE: [PATCH 4/6] drm/amdgpu: Attach eviction fence on alloc

[Errabolu, Ramesh]

[AMD Official Use Only - General]

Comment inline.

Regards,
Ramesh

-----Original Message-----
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Felix Kuehling
Sent: Thursday, January 12, 2023 7:02 AM
To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
Cc: Chen, Xiaogang <Xiaogang.Chen@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>
Subject: [PATCH 4/6] drm/amdgpu: Attach eviction fence on alloc

Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.


Instead of attaching the eviction fence when a KFD BO is first mapped, attach it when it is allocated or imported. This in preparation to allow KFD BOs to be mapped using the render node API.

Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 63 ++++++++++---------
 1 file changed, 32 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 5645103beed0..79213f476493 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -360,6 +360,24 @@ static int amdgpu_amdkfd_bo_validate(struct amdgpu_bo *bo, uint32_t domain,
        return ret;
 }

+static int amdgpu_amdkfd_bo_validate_and_fence(struct amdgpu_bo *bo,
+                                              uint32_t domain,
+                                              struct dma_fence *fence) 
+{
+       int ret = amdgpu_bo_reserve(bo, false);
+
+       if (ret)
+               return ret;
+
+       ret = amdgpu_amdkfd_bo_validate(bo, domain, true);
+       if (!ret)
Ramesh: Should space for fences be reserved before adding one.

+               dma_resv_add_fence(bo->tbo.base.resv, fence,
+                                  DMA_RESV_USAGE_BOOKKEEP);
+       amdgpu_bo_unreserve(bo);
+
+       return ret;
+}
+
 static int amdgpu_amdkfd_validate_vm_bo(void *_unused, struct amdgpu_bo *bo)  {
        return amdgpu_amdkfd_bo_validate(bo, bo->allowed_domains, false); @@ -1720,6 +1738,11 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
                }
                bo->allowed_domains = AMDGPU_GEM_DOMAIN_GTT;
                bo->preferred_domains = AMDGPU_GEM_DOMAIN_GTT;
+       } else {
+               ret = amdgpu_amdkfd_bo_validate_and_fence(bo, domain,
+                               &avm->process_info->eviction_fence->base);
+               if (ret)
+                       goto err_validate_bo;
        }

        if (offset)
@@ -1729,6 +1752,7 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(

 allocate_init_user_pages_failed:
 err_pin_bo:
+err_validate_bo:
        remove_kgd_mem_from_kfd_bo_list(*mem, avm->process_info);
        drm_vma_node_revoke(&gobj->vma_node, drm_priv);
 err_node_allow:
@@ -1804,10 +1828,6 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(
        if (unlikely(ret))
                return ret;

-       /* The eviction fence should be removed by the last unmap.
-        * TODO: Log an error condition if the bo still has the eviction fence
-        * attached
-        */
        amdgpu_amdkfd_remove_eviction_fence(mem->bo,
                                        process_info->eviction_fence);
Ramesh: Is it correct to call remove_eviction() unconditionally? This procedure applies to GTT and VRAM BO's only. Furthermore, the fence on these BO's has already been removed in the unmap_memory() call.

        pr_debug("Release VA 0x%llx - 0x%llx\n", mem->va, @@ -1931,19 +1951,6 @@ int amdgpu_amdkfd_gpuvm_map_memory_to_gpu(
        if (unlikely(ret))
                goto out_unreserve;

-       if (mem->mapped_to_gpu_memory == 0 &&
-           !amdgpu_ttm_tt_get_usermm(bo->tbo.ttm)) {
-               /* Validate BO only once. The eviction fence gets added to BO
-                * the first time it is mapped. Validate will wait for all
-                * background evictions to complete.
-                */
-               ret = amdgpu_amdkfd_bo_validate(bo, domain, true);
-               if (ret) {
-                       pr_debug("Validate failed\n");
-                       goto out_unreserve;
-               }
-       }
-
        list_for_each_entry(entry, &mem->attachments, list) {
                if (entry->bo_va->base.vm != avm || entry->is_mapped)
                        continue;
@@ -1970,10 +1977,6 @@ int amdgpu_amdkfd_gpuvm_map_memory_to_gpu(
                         mem->mapped_to_gpu_memory);
        }

-       if (!amdgpu_ttm_tt_get_usermm(bo->tbo.ttm) && !bo->tbo.pin_count)
-               dma_resv_add_fence(bo->tbo.base.resv,
-                                  &avm->process_info->eviction_fence->base,
-                                  DMA_RESV_USAGE_BOOKKEEP);
        ret = unreserve_bo_and_vms(&ctx, false, false);

        goto out;
@@ -1990,7 +1993,6 @@ int amdgpu_amdkfd_gpuvm_unmap_memory_from_gpu(
                struct amdgpu_device *adev, struct kgd_mem *mem, void *drm_priv)  {
        struct amdgpu_vm *avm = drm_priv_to_vm(drm_priv);
-       struct amdkfd_process_info *process_info = avm->process_info;
        unsigned long bo_size = mem->bo->tbo.base.size;
        struct kfd_mem_attachment *entry;
        struct bo_vm_reservation_context ctx; @@ -2031,15 +2033,6 @@ int amdgpu_amdkfd_gpuvm_unmap_memory_from_gpu(
                         mem->mapped_to_gpu_memory);
        }

-       /* If BO is unmapped from all VMs, unfence it. It can be evicted if
-        * required.
-        */
-       if (mem->mapped_to_gpu_memory == 0 &&
-           !amdgpu_ttm_tt_get_usermm(mem->bo->tbo.ttm) &&
-           !mem->bo->tbo.pin_count)
-               amdgpu_amdkfd_remove_eviction_fence(mem->bo,
-                                               process_info->eviction_fence);
-
 unreserve_out:
        unreserve_bo_and_vms(&ctx, false, false);
 out:
@@ -2266,8 +2259,16 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
        amdgpu_sync_create(&(*mem)->sync);
        (*mem)->is_imported = true;

+       ret = amdgpu_amdkfd_bo_validate_and_fence(bo, (*mem)->domain,
+                               &avm->process_info->eviction_fence->base);
+       if (ret)
+               goto err_remove_mem;
+
        return 0;

+err_remove_mem:
+       remove_kgd_mem_from_kfd_bo_list(*mem, avm->process_info);
+       drm_vma_node_revoke(&obj->vma_node, drm_priv);
 err_free_mem:
        kfree(*mem);
 err_put_obj:
--
2.34.1

RE: [PATCH 6/6] drm/amdgpu: Do bo_va ref counting for KFD BOs

[Errabolu, Ramesh]

[AMD Official Use Only - General]

Comment inline.

Regards,
Ramesh

-----Original Message-----
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Chen, Xiaogang
Sent: Saturday, January 14, 2023 4:28 AM
To: Kuehling, Felix <Felix.Kuehling@amd.com>; amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
Cc: Koenig, Christian <Christian.Koenig@amd.com>
Subject: Re: [PATCH 6/6] drm/amdgpu: Do bo_va ref counting for KFD BOs

Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.


Reviewed-by: Xiaogang Chen <Xiaoganng.Chen@amd.com>

Regards

Xiaogang

On 1/11/2023 7:31 PM, Felix Kuehling wrote:
> This is needed to correctly handle BOs imported into the GEM API, 
> which would otherwise get added twice to the same VM.
>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 28 +++++++++++++++----
>   1 file changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index df08e84f01d7..8b5ba2e04a79 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -370,9 +370,17 @@ static int amdgpu_amdkfd_bo_validate_and_fence(struct amdgpu_bo *bo,
>               return ret;
>
>       ret = amdgpu_amdkfd_bo_validate(bo, domain, true);
> -     if (!ret)
> -             dma_resv_add_fence(bo->tbo.base.resv, fence,
> -                                DMA_RESV_USAGE_BOOKKEEP);
> +     if (ret)
> +             goto unreserve_out;
> +
> +     ret = dma_resv_reserve_fences(bo->tbo.base.resv, 1);
Ramesh: Could this stmt to reserve space for fence be applied in patch 4.

> +     if (ret)
> +             goto unreserve_out;
> +
> +     dma_resv_add_fence(bo->tbo.base.resv, fence,
> +                        DMA_RESV_USAGE_BOOKKEEP);
> +
> +unreserve_out:
>       amdgpu_bo_unreserve(bo);
>
>       return ret;
> @@ -785,6 +793,7 @@ static int kfd_mem_attach(struct amdgpu_device *adev, struct kgd_mem *mem,
>       uint64_t va = mem->va;
>       struct kfd_mem_attachment *attachment[2] = {NULL, NULL};
>       struct amdgpu_bo *bo[2] = {NULL, NULL};
> +     struct amdgpu_bo_va *bo_va;
>       bool same_hive = false;
>       int i, ret;
>
> @@ -871,7 +880,12 @@ static int kfd_mem_attach(struct amdgpu_device *adev, struct kgd_mem *mem,
>                       pr_debug("Unable to reserve BO during memory attach");
>                       goto unwind;
>               }
> -             attachment[i]->bo_va = amdgpu_vm_bo_add(adev, vm, bo[i]);
> +             bo_va = amdgpu_vm_bo_find(vm, bo[i]);
> +             if (!bo_va)
> +                     bo_va = amdgpu_vm_bo_add(adev, vm, bo[i]);
> +             else
> +                     ++bo_va->ref_count;
> +             attachment[i]->bo_va = bo_va;
>               amdgpu_bo_unreserve(bo[i]);
>               if (unlikely(!attachment[i]->bo_va)) {
>                       ret = -ENOMEM;
> @@ -895,7 +909,8 @@ static int kfd_mem_attach(struct amdgpu_device *adev, struct kgd_mem *mem,
>                       continue;
>               if (attachment[i]->bo_va) {
>                       amdgpu_bo_reserve(bo[i], true);
> -                     amdgpu_vm_bo_del(adev, attachment[i]->bo_va);
> +                     if (--attachment[i]->bo_va->ref_count == 0)
> +                             amdgpu_vm_bo_del(adev, 
> + attachment[i]->bo_va);
>                       amdgpu_bo_unreserve(bo[i]);
>                       list_del(&attachment[i]->list);
>               }
> @@ -912,7 +927,8 @@ static void kfd_mem_detach(struct 
> kfd_mem_attachment *attachment)
>
>       pr_debug("\t remove VA 0x%llx in entry %p\n",
>                       attachment->va, attachment);
> -     amdgpu_vm_bo_del(attachment->adev, attachment->bo_va);
> +     if (--attachment->bo_va->ref_count == 0)
> +             amdgpu_vm_bo_del(attachment->adev, attachment->bo_va);
>       drm_gem_object_put(&bo->tbo.base);
>       list_del(&attachment->list);
>       kfree(attachment);

Re: [PATCH 1/6] drm/amdgpu: Generalize KFD dmabuf import

[Felix Kuehling]

On 2023-01-16 17:04, Errabolu, Ramesh wrote:
> [AMD Official Use Only - General]
>
> A minor comment, unrelated to the patch. The comments are inline.
>
> Regards,
> Ramesh
>
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Felix Kuehling
> Sent: Thursday, January 12, 2023 7:02 AM
> To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Cc: Chen, Xiaogang <Xiaogang.Chen@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>
> Subject: [PATCH 1/6] drm/amdgpu: Generalize KFD dmabuf import
>
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> Use proper amdgpu_gem_prime_import function to handle all kinds of imports. Remember the dmabuf reference to enable proper multi-GPU attachment to multiple VMs without erroneously re-exporting the underlying BO multiple times.
>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 38 ++++++++++---------
>   1 file changed, 21 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index 6f236ded5f12..e13c3493b786 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -2209,30 +2209,27 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
>          struct amdgpu_bo *bo;
>          int ret;
>
> -       if (dma_buf->ops != &amdgpu_dmabuf_ops)
> -               /* Can't handle non-graphics buffers */
> -               return -EINVAL;
> -
> -       obj = dma_buf->priv;
> -       if (drm_to_adev(obj->dev) != adev)
> -               /* Can't handle buffers from other devices */
> -               return -EINVAL;
> +       obj = amdgpu_gem_prime_import(adev_to_drm(adev), dma_buf);
> +       if (IS_ERR(obj))
> +               return PTR_ERR(obj);
>
>          bo = gem_to_amdgpu_bo(obj);
>          if (!(bo->preferred_domains & (AMDGPU_GEM_DOMAIN_VRAM |
> -                                   AMDGPU_GEM_DOMAIN_GTT)))
> +                                   AMDGPU_GEM_DOMAIN_GTT))) {
>                  /* Only VRAM and GTT BOs are supported */
> -               return -EINVAL;
> +               ret = -EINVAL;
> +               goto err_put_obj;
> +       }
>
>          *mem = kzalloc(sizeof(struct kgd_mem), GFP_KERNEL);
> -       if (!*mem)
> -               return -ENOMEM;
> +       if (!*mem) {
> +               ret = -ENOMEM;
> +               goto err_put_obj;
> +       }
>
>          ret = drm_vma_node_allow(&obj->vma_node, drm_priv);
> -       if (ret) {
> -               kfree(*mem);
> -               return ret;
> -       }
> +       if (ret)
> +               goto err_free_mem;
>
>          if (size)
>                  *size = amdgpu_bo_size(bo); @@ -2249,7 +2246,8 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
>                  | KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE
>                  | KFD_IOC_ALLOC_MEM_FLAGS_EXECUTABLE;
> Ramesh: Is it correct to assign WRITE & EXECUTABLE permissions to alloc_flags variable.

These flags affect GPUVM mappings of the BO. If we didn't set these 
flags, the imported BO would be read-only. The existing graphics-interop 
API (struct kfd_ioctl_import_dmabuf_args) doesn't have a way for user 
mode to provide these flags. Changing this would break the API.

Regards,
   Felix


>
> -       drm_gem_object_get(&bo->tbo.base);
> +       get_dma_buf(dma_buf);
> +       (*mem)->dmabuf = dma_buf;
>          (*mem)->bo = bo;
>          (*mem)->va = va;
>          (*mem)->domain = (bo->preferred_domains & AMDGPU_GEM_DOMAIN_VRAM) ?
> @@ -2261,6 +2259,12 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
>          (*mem)->is_imported = true;
>
>          return 0;
> +
> +err_free_mem:
> +       kfree(*mem);
> +err_put_obj:
> +       drm_gem_object_put(obj);
> +       return ret;
>   }
>
>   /* Evict a userptr BO by stopping the queues if necessary
> --
> 2.34.1

Re: [PATCH 4/6] drm/amdgpu: Attach eviction fence on alloc

[Felix Kuehling]

On 2023-01-16 17:11, Errabolu, Ramesh wrote:
> [AMD Official Use Only - General]
>
> Comment inline.
>
> Regards,
> Ramesh
>
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Felix Kuehling
> Sent: Thursday, January 12, 2023 7:02 AM
> To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Cc: Chen, Xiaogang <Xiaogang.Chen@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>
> Subject: [PATCH 4/6] drm/amdgpu: Attach eviction fence on alloc
>
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> Instead of attaching the eviction fence when a KFD BO is first mapped, attach it when it is allocated or imported. This in preparation to allow KFD BOs to be mapped using the render node API.
>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 63 ++++++++++---------
>   1 file changed, 32 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index 5645103beed0..79213f476493 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -360,6 +360,24 @@ static int amdgpu_amdkfd_bo_validate(struct amdgpu_bo *bo, uint32_t domain,
>          return ret;
>   }
>
> +static int amdgpu_amdkfd_bo_validate_and_fence(struct amdgpu_bo *bo,
> +                                              uint32_t domain,
> +                                              struct dma_fence *fence)
> +{
> +       int ret = amdgpu_bo_reserve(bo, false);
> +
> +       if (ret)
> +               return ret;
> +
> +       ret = amdgpu_amdkfd_bo_validate(bo, domain, true);
> +       if (!ret)
> Ramesh: Should space for fences be reserved before adding one.

Yes, I probably should. I think I fixed this in the later patch because 
I didn't see the problem until I fixed those other issues.


>
> +               dma_resv_add_fence(bo->tbo.base.resv, fence,
> +                                  DMA_RESV_USAGE_BOOKKEEP);
> +       amdgpu_bo_unreserve(bo);
> +
> +       return ret;
> +}
> +
>   static int amdgpu_amdkfd_validate_vm_bo(void *_unused, struct amdgpu_bo *bo)  {
>          return amdgpu_amdkfd_bo_validate(bo, bo->allowed_domains, false);
> @@ -1720,6 +1738,11 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
>                  }
>                  bo->allowed_domains = AMDGPU_GEM_DOMAIN_GTT;
>                  bo->preferred_domains = AMDGPU_GEM_DOMAIN_GTT;
> +       } else {
> +               ret = amdgpu_amdkfd_bo_validate_and_fence(bo, domain,
> +                               &avm->process_info->eviction_fence->base);
> +               if (ret)
> +                       goto err_validate_bo;
>          }
>
>          if (offset)
> @@ -1729,6 +1752,7 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
>
>   allocate_init_user_pages_failed:
>   err_pin_bo:
> +err_validate_bo:
>          remove_kgd_mem_from_kfd_bo_list(*mem, avm->process_info);
>          drm_vma_node_revoke(&gobj->vma_node, drm_priv);
>   err_node_allow:
> @@ -1804,10 +1828,6 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(
>          if (unlikely(ret))
>                  return ret;
>
> -       /* The eviction fence should be removed by the last unmap.
> -        * TODO: Log an error condition if the bo still has the eviction fence
> -        * attached
> -        */
>          amdgpu_amdkfd_remove_eviction_fence(mem->bo,
>                                          process_info->eviction_fence);
> Ramesh: Is it correct to call remove_eviction() unconditionally? This procedure applies to GTT and VRAM BO's only. Furthermore, the fence on these BO's has already been removed in the unmap_memory() call.

This patch removes the amdgpu_amdkfd_remove_eviction_fence from 
amdgpu_amdkfd_gpuvm_unmap_memory_from_gpu. So we definitely need to do 
this here.

amdgpu_amdkfd_remove_eviction_fence uses dma_resv_replace_fences. If the 
specified fence is not found in the BO's reservation object, it is a 
no-op. So calling this unconditionally is safe.

Regards,
   Felix


>
>          pr_debug("Release VA 0x%llx - 0x%llx\n", mem->va,
> @@ -1931,19 +1951,6 @@ int amdgpu_amdkfd_gpuvm_map_memory_to_gpu(
>          if (unlikely(ret))
>                  goto out_unreserve;
>
> -       if (mem->mapped_to_gpu_memory == 0 &&
> -           !amdgpu_ttm_tt_get_usermm(bo->tbo.ttm)) {
> -               /* Validate BO only once. The eviction fence gets added to BO
> -                * the first time it is mapped. Validate will wait for all
> -                * background evictions to complete.
> -                */
> -               ret = amdgpu_amdkfd_bo_validate(bo, domain, true);
> -               if (ret) {
> -                       pr_debug("Validate failed\n");
> -                       goto out_unreserve;
> -               }
> -       }
> -
>          list_for_each_entry(entry, &mem->attachments, list) {
>                  if (entry->bo_va->base.vm != avm || entry->is_mapped)
>                          continue;
> @@ -1970,10 +1977,6 @@ int amdgpu_amdkfd_gpuvm_map_memory_to_gpu(
>                           mem->mapped_to_gpu_memory);
>          }
>
> -       if (!amdgpu_ttm_tt_get_usermm(bo->tbo.ttm) && !bo->tbo.pin_count)
> -               dma_resv_add_fence(bo->tbo.base.resv,
> -                                  &avm->process_info->eviction_fence->base,
> -                                  DMA_RESV_USAGE_BOOKKEEP);
>          ret = unreserve_bo_and_vms(&ctx, false, false);
>
>          goto out;
> @@ -1990,7 +1993,6 @@ int amdgpu_amdkfd_gpuvm_unmap_memory_from_gpu(
>                  struct amdgpu_device *adev, struct kgd_mem *mem, void *drm_priv)  {
>          struct amdgpu_vm *avm = drm_priv_to_vm(drm_priv);
> -       struct amdkfd_process_info *process_info = avm->process_info;
>          unsigned long bo_size = mem->bo->tbo.base.size;
>          struct kfd_mem_attachment *entry;
>          struct bo_vm_reservation_context ctx; @@ -2031,15 +2033,6 @@ int amdgpu_amdkfd_gpuvm_unmap_memory_from_gpu(
>                           mem->mapped_to_gpu_memory);
>          }
>
> -       /* If BO is unmapped from all VMs, unfence it. It can be evicted if
> -        * required.
> -        */
> -       if (mem->mapped_to_gpu_memory == 0 &&
> -           !amdgpu_ttm_tt_get_usermm(mem->bo->tbo.ttm) &&
> -           !mem->bo->tbo.pin_count)
> -               amdgpu_amdkfd_remove_eviction_fence(mem->bo,
> -                                               process_info->eviction_fence);
> -
>   unreserve_out:
>          unreserve_bo_and_vms(&ctx, false, false);
>   out:
> @@ -2266,8 +2259,16 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
>          amdgpu_sync_create(&(*mem)->sync);
>          (*mem)->is_imported = true;
>
> +       ret = amdgpu_amdkfd_bo_validate_and_fence(bo, (*mem)->domain,
> +                               &avm->process_info->eviction_fence->base);
> +       if (ret)
> +               goto err_remove_mem;
> +
>          return 0;
>
> +err_remove_mem:
> +       remove_kgd_mem_from_kfd_bo_list(*mem, avm->process_info);
> +       drm_vma_node_revoke(&obj->vma_node, drm_priv);
>   err_free_mem:
>          kfree(*mem);
>   err_put_obj:
> --
> 2.34.1

Re: [PATCH 6/6] drm/amdgpu: Do bo_va ref counting for KFD BOs

[Felix Kuehling]

On 2023-01-16 17:12, Errabolu, Ramesh wrote:
> [AMD Official Use Only - General]
>
> Comment inline.
>
> Regards,
> Ramesh
>
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Chen, Xiaogang
> Sent: Saturday, January 14, 2023 4:28 AM
> To: Kuehling, Felix <Felix.Kuehling@amd.com>; amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Cc: Koenig, Christian <Christian.Koenig@amd.com>
> Subject: Re: [PATCH 6/6] drm/amdgpu: Do bo_va ref counting for KFD BOs
>
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> Reviewed-by: Xiaogang Chen <Xiaoganng.Chen@amd.com>
>
> Regards
>
> Xiaogang
>
> On 1/11/2023 7:31 PM, Felix Kuehling wrote:
>> This is needed to correctly handle BOs imported into the GEM API,
>> which would otherwise get added twice to the same VM.
>>
>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
>> ---
>>    .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 28 +++++++++++++++----
>>    1 file changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> index df08e84f01d7..8b5ba2e04a79 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> @@ -370,9 +370,17 @@ static int amdgpu_amdkfd_bo_validate_and_fence(struct amdgpu_bo *bo,
>>                return ret;
>>
>>        ret = amdgpu_amdkfd_bo_validate(bo, domain, true);
>> -     if (!ret)
>> -             dma_resv_add_fence(bo->tbo.base.resv, fence,
>> -                                DMA_RESV_USAGE_BOOKKEEP);
>> +     if (ret)
>> +             goto unreserve_out;
>> +
>> +     ret = dma_resv_reserve_fences(bo->tbo.base.resv, 1);
> Ramesh: Could this stmt to reserve space for fence be applied in patch 4.

Done. Thanks for pointing this out.

Regards,
   Felix


>
>> +     if (ret)
>> +             goto unreserve_out;
>> +
>> +     dma_resv_add_fence(bo->tbo.base.resv, fence,
>> +                        DMA_RESV_USAGE_BOOKKEEP);
>> +
>> +unreserve_out:
>>        amdgpu_bo_unreserve(bo);
>>
>>        return ret;
>> @@ -785,6 +793,7 @@ static int kfd_mem_attach(struct amdgpu_device *adev, struct kgd_mem *mem,
>>        uint64_t va = mem->va;
>>        struct kfd_mem_attachment *attachment[2] = {NULL, NULL};
>>        struct amdgpu_bo *bo[2] = {NULL, NULL};
>> +     struct amdgpu_bo_va *bo_va;
>>        bool same_hive = false;
>>        int i, ret;
>>
>> @@ -871,7 +880,12 @@ static int kfd_mem_attach(struct amdgpu_device *adev, struct kgd_mem *mem,
>>                        pr_debug("Unable to reserve BO during memory attach");
>>                        goto unwind;
>>                }
>> -             attachment[i]->bo_va = amdgpu_vm_bo_add(adev, vm, bo[i]);
>> +             bo_va = amdgpu_vm_bo_find(vm, bo[i]);
>> +             if (!bo_va)
>> +                     bo_va = amdgpu_vm_bo_add(adev, vm, bo[i]);
>> +             else
>> +                     ++bo_va->ref_count;
>> +             attachment[i]->bo_va = bo_va;
>>                amdgpu_bo_unreserve(bo[i]);
>>                if (unlikely(!attachment[i]->bo_va)) {
>>                        ret = -ENOMEM;
>> @@ -895,7 +909,8 @@ static int kfd_mem_attach(struct amdgpu_device *adev, struct kgd_mem *mem,
>>                        continue;
>>                if (attachment[i]->bo_va) {
>>                        amdgpu_bo_reserve(bo[i], true);
>> -                     amdgpu_vm_bo_del(adev, attachment[i]->bo_va);
>> +                     if (--attachment[i]->bo_va->ref_count == 0)
>> +                             amdgpu_vm_bo_del(adev,
>> + attachment[i]->bo_va);
>>                        amdgpu_bo_unreserve(bo[i]);
>>                        list_del(&attachment[i]->list);
>>                }
>> @@ -912,7 +927,8 @@ static void kfd_mem_detach(struct
>> kfd_mem_attachment *attachment)
>>
>>        pr_debug("\t remove VA 0x%llx in entry %p\n",
>>                        attachment->va, attachment);
>> -     amdgpu_vm_bo_del(attachment->adev, attachment->bo_va);
>> +     if (--attachment->bo_va->ref_count == 0)
>> +             amdgpu_vm_bo_del(attachment->adev, attachment->bo_va);
>>        drm_gem_object_put(&bo->tbo.base);
>>        list_del(&attachment->list);
>>        kfree(attachment);

Re: [PATCH 1/6] drm/amdgpu: Generalize KFD dmabuf import

[Dmitry Osipenko]

16.01.2023 18:11, Christian König пишет:
> 
>>>>>
>>>>>> mmapping the memory with that new offset should still work. The
>>>>>> imported BO is created with ttm_bo_type_sg, and AFAICT ttm_bo_vm.c
>>>>>> supports mapping of SG BOs.
>>>>>
>>>>> Actually it shouldn't. This can go boom really easily.
>>>>
>>>> OK. I don't think we're doing this, but after Xiaogang raised the
>>>> question I went looking through the code whether it's theoretically
>>>> possible. I didn't find anything in the code that says that mmapping
>>>> imported dmabufs would be prohibited or even dangerous. On the
>>>> contrary, I found that ttm_bo_vm explicitly supports mmapping SG BOs.
>>>>
>>>>
>>>>>
>>>>> When you have imported a BO the only correct way of to mmap() it is
>>>>> to do so on the original exporter.
>>>>
>>>> That seems sensible, and this is what we do today. That said, if
>>>> mmapping an imported BO is dangerous, I'm missing a mechanism to
>>>> protect against this. It could be as simple as setting
>>>> AMDGPU_GEM_CREATE_NO_CPU_ACCESS in amdgpu_dma_buf_create_obj.
>>>
>>> At least for the GEM mmap() handler this is double checked very early
>>> by looking at obj->import_attach and then either rejecting it or
>>> redirecting the request to the DMA-buf file instead.
>>
>> Can you point me at where this check is? I see a check for
>> obj->import_attach in drm_gem_dumb_map_offset. But I can't see how
>> this function is called in amdgpu. I don't think it is used at all.
> 
> Uff, good question! @Thomas and @Dmitry: I clearly remember that one of
> you guys was involved in the DRM/GEM mmap cleanup and DMA-buf with
> workarounds for the KFD and DMA-buf.
> 
> What was the final solution to this? I can't find it of hand any more.

I was looking at it. The AMDGPU indeed allows to map imported GEMs, but
then touching the mapped area by CPU results in a bus fault. You,
Christian, suggested that this an AMDGPU bug that should be fixed by
prohibiting the mapping in the first place and I was going to fix it,
but then the plan changed from prohibiting the mapping into fixing it.

The first proposal was to make DRM core to handle the dma-buf mappings
for all drivers universally [1]. Then we decided that will be better to
prohibit mapping of imported GEMs [2]. In the end, Rob Clark argued that
better to implement the [1], otherwise current userspace (Android) will
be broken if mapping will be prohibited.

The last question was about the cache syncing of imported dma-bufs, how
to ensure that drivers will do the cache maintenance/syncing properly.
Rob suggested that it should be a problem for drivers and not for DRM core.

I was going to re-send the [1], but other things were getting priority.
It's good that you reminded me about it :) I may re-send it sometime
soon if there are no new objections.

[1] https://patchwork.freedesktop.org/patch/487481/

[2]
https://lore.kernel.org/all/20220701090240.1896131-1-dmitry.osipenko@collabora.com/

Re: [PATCH 1/6] drm/amdgpu: Generalize KFD dmabuf import

[Daniel Vetter]

On Tue, Jan 17, 2023 at 04:06:05AM +0300, Dmitry Osipenko wrote:
> 16.01.2023 18:11, Christian König пишет:
> > 
> >>>>>
> >>>>>> mmapping the memory with that new offset should still work. The
> >>>>>> imported BO is created with ttm_bo_type_sg, and AFAICT ttm_bo_vm.c
> >>>>>> supports mapping of SG BOs.
> >>>>>
> >>>>> Actually it shouldn't. This can go boom really easily.
> >>>>
> >>>> OK. I don't think we're doing this, but after Xiaogang raised the
> >>>> question I went looking through the code whether it's theoretically
> >>>> possible. I didn't find anything in the code that says that mmapping
> >>>> imported dmabufs would be prohibited or even dangerous. On the
> >>>> contrary, I found that ttm_bo_vm explicitly supports mmapping SG BOs.
> >>>>
> >>>>
> >>>>>
> >>>>> When you have imported a BO the only correct way of to mmap() it is
> >>>>> to do so on the original exporter.
> >>>>
> >>>> That seems sensible, and this is what we do today. That said, if
> >>>> mmapping an imported BO is dangerous, I'm missing a mechanism to
> >>>> protect against this. It could be as simple as setting
> >>>> AMDGPU_GEM_CREATE_NO_CPU_ACCESS in amdgpu_dma_buf_create_obj.
> >>>
> >>> At least for the GEM mmap() handler this is double checked very early
> >>> by looking at obj->import_attach and then either rejecting it or
> >>> redirecting the request to the DMA-buf file instead.
> >>
> >> Can you point me at where this check is? I see a check for
> >> obj->import_attach in drm_gem_dumb_map_offset. But I can't see how
> >> this function is called in amdgpu. I don't think it is used at all.
> > 
> > Uff, good question! @Thomas and @Dmitry: I clearly remember that one of
> > you guys was involved in the DRM/GEM mmap cleanup and DMA-buf with
> > workarounds for the KFD and DMA-buf.
> > 
> > What was the final solution to this? I can't find it of hand any more.
> 
> I was looking at it. The AMDGPU indeed allows to map imported GEMs, but
> then touching the mapped area by CPU results in a bus fault. You,
> Christian, suggested that this an AMDGPU bug that should be fixed by
> prohibiting the mapping in the first place and I was going to fix it,
> but then the plan changed from prohibiting the mapping into fixing it.
> 
> The first proposal was to make DRM core to handle the dma-buf mappings
> for all drivers universally [1]. Then we decided that will be better to
> prohibit mapping of imported GEMs [2]. In the end, Rob Clark argued that
> better to implement the [1], otherwise current userspace (Android) will
> be broken if mapping will be prohibited.
> 
> The last question was about the cache syncing of imported dma-bufs, how
> to ensure that drivers will do the cache maintenance/syncing properly.
> Rob suggested that it should be a problem for drivers and not for DRM core.
> 
> I was going to re-send the [1], but other things were getting priority.
> It's good that you reminded me about it :) I may re-send it sometime
> soon if there are no new objections.
> 
> [1] https://patchwork.freedesktop.org/patch/487481/
> 
> [2]
> https://lore.kernel.org/all/20220701090240.1896131-1-dmitry.osipenko@collabora.com/

Hm I still don't like allowing this in general, because in general it just
doesn't work.

I think more like a per-driver opt-in or something might be needed, so
that drivers which "know" that it's ok to just mmap without coherency can
allow that. Allowing this in general essentially gives up on the entire
idea of dma-buf cache flushing completely.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/6] drm/amdgpu: Generalize KFD dmabuf import

[Daniel Vetter]

On Tue, Jan 17, 2023 at 04:06:05AM +0300, Dmitry Osipenko wrote:
> 16.01.2023 18:11, Christian König пишет:
> > 
> >>>>>
> >>>>>> mmapping the memory with that new offset should still work. The
> >>>>>> imported BO is created with ttm_bo_type_sg, and AFAICT ttm_bo_vm.c
> >>>>>> supports mapping of SG BOs.
> >>>>>
> >>>>> Actually it shouldn't. This can go boom really easily.
> >>>>
> >>>> OK. I don't think we're doing this, but after Xiaogang raised the
> >>>> question I went looking through the code whether it's theoretically
> >>>> possible. I didn't find anything in the code that says that mmapping
> >>>> imported dmabufs would be prohibited or even dangerous. On the
> >>>> contrary, I found that ttm_bo_vm explicitly supports mmapping SG BOs.
> >>>>
> >>>>
> >>>>>
> >>>>> When you have imported a BO the only correct way of to mmap() it is
> >>>>> to do so on the original exporter.
> >>>>
> >>>> That seems sensible, and this is what we do today. That said, if
> >>>> mmapping an imported BO is dangerous, I'm missing a mechanism to
> >>>> protect against this. It could be as simple as setting
> >>>> AMDGPU_GEM_CREATE_NO_CPU_ACCESS in amdgpu_dma_buf_create_obj.
> >>>
> >>> At least for the GEM mmap() handler this is double checked very early
> >>> by looking at obj->import_attach and then either rejecting it or
> >>> redirecting the request to the DMA-buf file instead.
> >>
> >> Can you point me at where this check is? I see a check for
> >> obj->import_attach in drm_gem_dumb_map_offset. But I can't see how
> >> this function is called in amdgpu. I don't think it is used at all.
> > 
> > Uff, good question! @Thomas and @Dmitry: I clearly remember that one of
> > you guys was involved in the DRM/GEM mmap cleanup and DMA-buf with
> > workarounds for the KFD and DMA-buf.
> > 
> > What was the final solution to this? I can't find it of hand any more.
> 
> I was looking at it. The AMDGPU indeed allows to map imported GEMs, but
> then touching the mapped area by CPU results in a bus fault. You,
> Christian, suggested that this an AMDGPU bug that should be fixed by
> prohibiting the mapping in the first place and I was going to fix it,
> but then the plan changed from prohibiting the mapping into fixing it.
> 
> The first proposal was to make DRM core to handle the dma-buf mappings
> for all drivers universally [1]. Then we decided that will be better to
> prohibit mapping of imported GEMs [2]. In the end, Rob Clark argued that
> better to implement the [1], otherwise current userspace (Android) will
> be broken if mapping will be prohibited.
> 
> The last question was about the cache syncing of imported dma-bufs, how
> to ensure that drivers will do the cache maintenance/syncing properly.
> Rob suggested that it should be a problem for drivers and not for DRM core.
> 
> I was going to re-send the [1], but other things were getting priority.
> It's good that you reminded me about it :) I may re-send it sometime
> soon if there are no new objections.
> 
> [1] https://patchwork.freedesktop.org/patch/487481/
> 
> [2]
> https://lore.kernel.org/all/20220701090240.1896131-1-dmitry.osipenko@collabora.com/

Hm I still don't like allowing this in general, because in general it just
doesn't work.

I think more like a per-driver opt-in or something might be needed, so
that drivers which "know" that it's ok to just mmap without coherency can
allow that. Allowing this in general essentially gives up on the entire
idea of dma-buf cache flushing completely.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 1/6] drm/amdgpu: Generalize KFD dmabuf import

[Felix Kuehling]

Use proper amdgpu_gem_prime_import function to handle all kinds of
imports. Remember the dmabuf reference to enable proper multi-GPU
attachment to multiple VMs without erroneously re-exporting the
underlying BO multiple times.

Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 38 ++++++++++---------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 3a763916a5a1..67c832ae498b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -2183,30 +2183,27 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
 	struct amdgpu_bo *bo;
 	int ret;
 
-	if (dma_buf->ops != &amdgpu_dmabuf_ops)
-		/* Can't handle non-graphics buffers */
-		return -EINVAL;
-
-	obj = dma_buf->priv;
-	if (drm_to_adev(obj->dev) != adev)
-		/* Can't handle buffers from other devices */
-		return -EINVAL;
+	obj = amdgpu_gem_prime_import(adev_to_drm(adev), dma_buf);
+	if (IS_ERR(obj))
+		return PTR_ERR(obj);
 
 	bo = gem_to_amdgpu_bo(obj);
 	if (!(bo->preferred_domains & (AMDGPU_GEM_DOMAIN_VRAM |
-				    AMDGPU_GEM_DOMAIN_GTT)))
+				    AMDGPU_GEM_DOMAIN_GTT))) {
 		/* Only VRAM and GTT BOs are supported */
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err_put_obj;
+	}
 
 	*mem = kzalloc(sizeof(struct kgd_mem), GFP_KERNEL);
-	if (!*mem)
-		return -ENOMEM;
+	if (!*mem) {
+		ret = -ENOMEM;
+		goto err_put_obj;
+	}
 
 	ret = drm_vma_node_allow(&obj->vma_node, drm_priv);
-	if (ret) {
-		kfree(mem);
-		return ret;
-	}
+	if (ret)
+		goto err_free_mem;
 
 	if (size)
 		*size = amdgpu_bo_size(bo);
@@ -2223,7 +2220,8 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
 		| KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE
 		| KFD_IOC_ALLOC_MEM_FLAGS_EXECUTABLE;
 
-	drm_gem_object_get(&bo->tbo.base);
+	get_dma_buf(dma_buf);
+	(*mem)->dmabuf = dma_buf;
 	(*mem)->bo = bo;
 	(*mem)->va = va;
 	(*mem)->domain = (bo->preferred_domains & AMDGPU_GEM_DOMAIN_VRAM) ?
@@ -2235,6 +2233,12 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
 	(*mem)->is_imported = true;
 
 	return 0;
+
+err_free_mem:
+	kfree(mem);
+err_put_obj:
+	drm_gem_object_put(obj);
+	return ret;
 }
 
 /* Evict a userptr BO by stopping the queues if necessary
-- 
2.32.0