[PATCH 1/2] drm/amdgpu: Set pasid for compute vm

[PATCH 1/2] drm/amdgpu: Set pasid for compute vm

[Oak Zeng]

To make a amdgpu vm to a compute vm, the old pasid will be freed and
replaced with a pasid managed by kfd. Kfd can't reuse original pasid
allocated by amdgpu because kfd uses different pasid policy with amdgpu.
For example, all graphic devices share one same pasid in a process.

Signed-off-by: Oak Zeng <Oak.Zeng@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h       |  6 ++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 10 +++----
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c           | 33 +++++++++++++++++++++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h           |  2 +-
 drivers/gpu/drm/amd/amdkfd/kfd_process.c         |  4 +--
 drivers/gpu/drm/amd/include/kgd_kfd_interface.h  |  5 ++--
 6 files changed, 43 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
index a8418a3..e1f6454 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
@@ -155,11 +155,11 @@ uint64_t amdgpu_amdkfd_get_vram_usage(struct kgd_dev *kgd);
 	})
 
 /* GPUVM API */
-int amdgpu_amdkfd_gpuvm_create_process_vm(struct kgd_dev *kgd, void **vm,
-					void **process_info,
+int amdgpu_amdkfd_gpuvm_create_process_vm(struct kgd_dev *kgd, unsigned int pasid,
+					void **vm, void **process_info,
 					struct dma_fence **ef);
 int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct kgd_dev *kgd,
-					struct file *filp,
+					struct file *filp, unsigned int pasid,
 					void **vm, void **process_info,
 					struct dma_fence **ef);
 void amdgpu_amdkfd_gpuvm_destroy_cb(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 7eadc58..0980a1f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -1003,8 +1003,8 @@ static int init_kfd_vm(struct amdgpu_vm *vm, void **process_info,
 	return ret;
 }
 
-int amdgpu_amdkfd_gpuvm_create_process_vm(struct kgd_dev *kgd, void **vm,
-					  void **process_info,
+int amdgpu_amdkfd_gpuvm_create_process_vm(struct kgd_dev *kgd, unsigned int pasid,
+					  void **vm, void **process_info,
 					  struct dma_fence **ef)
 {
 	struct amdgpu_device *adev = get_amdgpu_device(kgd);
@@ -1016,7 +1016,7 @@ int amdgpu_amdkfd_gpuvm_create_process_vm(struct kgd_dev *kgd, void **vm,
 		return -ENOMEM;
 
 	/* Initialize AMDGPU part of the VM */
-	ret = amdgpu_vm_init(adev, new_vm, AMDGPU_VM_CONTEXT_COMPUTE, 0);
+	ret = amdgpu_vm_init(adev, new_vm, AMDGPU_VM_CONTEXT_COMPUTE, pasid);
 	if (ret) {
 		pr_err("Failed init vm ret %d\n", ret);
 		goto amdgpu_vm_init_fail;
@@ -1039,7 +1039,7 @@ int amdgpu_amdkfd_gpuvm_create_process_vm(struct kgd_dev *kgd, void **vm,
 }
 
 int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct kgd_dev *kgd,
-					   struct file *filp,
+					   struct file *filp, unsigned int pasid,
 					   void **vm, void **process_info,
 					   struct dma_fence **ef)
 {
@@ -1054,7 +1054,7 @@ int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct kgd_dev *kgd,
 		return -EINVAL;
 
 	/* Convert VM into a compute VM */
-	ret = amdgpu_vm_make_compute(adev, avm);
+	ret = amdgpu_vm_make_compute(adev, avm, pasid);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 662aec5..d8a99f4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2690,7 +2690,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
  * Returns:
  * 0 for success, -errno for errors.
  */
-int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
+int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm, unsigned int pasid)
 {
 	bool pte_support_ats = (adev->asic_type == CHIP_RAVEN);
 	int r;
@@ -2702,7 +2702,20 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 	/* Sanity checks */
 	if (!RB_EMPTY_ROOT(&vm->va.rb_root) || vm->root.entries) {
 		r = -EINVAL;
-		goto error;
+		goto unreserve_bo;
+	}
+
+	if (pasid) {
+		unsigned long flags;
+
+		spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
+		r = idr_alloc(&adev->vm_manager.pasid_idr, vm, pasid, pasid + 1,
+			      GFP_ATOMIC);
+		spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
+
+		if (r == -ENOSPC)
+			goto unreserve_bo;
+		r = 0;
 	}
 
 	/* Check if PD needs to be reinitialized and do it before
@@ -2713,7 +2726,7 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 			       adev->vm_manager.root_level,
 			       pte_support_ats);
 		if (r)
-			goto error;
+			goto free_idr;
 	}
 
 	/* Update VM state */
@@ -2732,10 +2745,22 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 		idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
 		spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
 
+		/* Free the original amdgpu allocated pasid
+		 * Will be replaced with kfd allocated pasid
+		 */
+		amdgpu_pasid_free(vm->pasid);
 		vm->pasid = 0;
 	}
 
-error:
+	if (pasid)
+		vm->pasid = pasid;
+
+	goto unreserve_bo;
+
+free_idr:
+	if (pasid)
+		idr_remove(&adev->vm_manager.pasid_idr, pasid);
+unreserve_bo:
 	amdgpu_bo_unreserve(vm->root.base.bo);
 	return r;
 }
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 1162c2b..c68945d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -294,7 +294,7 @@ void amdgpu_vm_manager_init(struct amdgpu_device *adev);
 void amdgpu_vm_manager_fini(struct amdgpu_device *adev);
 int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 		   int vm_context, unsigned int pasid);
-int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm);
+int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm, unsigned int pasid);
 void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm);
 bool amdgpu_vm_pasid_fault_credit(struct amdgpu_device *adev,
 				  unsigned int pasid);
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
index 1d80b4f..5f559c5 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
@@ -685,11 +685,11 @@ int kfd_process_device_init_vm(struct kfd_process_device *pdd,
 
 	if (drm_file)
 		ret = dev->kfd2kgd->acquire_process_vm(
-			dev->kgd, drm_file,
+			dev->kgd, drm_file, p->pasid,
 			&pdd->vm, &p->kgd_process_info, &p->ef);
 	else
 		ret = dev->kfd2kgd->create_process_vm(
-			dev->kgd, &pdd->vm, &p->kgd_process_info, &p->ef);
+			dev->kgd, p->pasid, &pdd->vm, &p->kgd_process_info, &p->ef);
 	if (ret) {
 		pr_err("Failed to create process VM object\n");
 		return ret;
diff --git a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
index 5733fbe..1632869 100644
--- a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
+++ b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
@@ -346,10 +346,11 @@ struct kfd2kgd_calls {
 			struct kfd_cu_info *cu_info);
 	uint64_t (*get_vram_usage)(struct kgd_dev *kgd);
 
-	int (*create_process_vm)(struct kgd_dev *kgd, void **vm,
+	int (*create_process_vm)(struct kgd_dev *kgd, unsigned int pasid, void **vm,
 			void **process_info, struct dma_fence **ef);
 	int (*acquire_process_vm)(struct kgd_dev *kgd, struct file *filp,
-			void **vm, void **process_info, struct dma_fence **ef);
+			unsigned int pasid, void **vm, void **process_info,
+			struct dma_fence **ef);
 	void (*destroy_process_vm)(struct kgd_dev *kgd, void *vm);
 	uint32_t (*get_process_page_dir)(void *vm);
 	void (*set_vm_context_page_table_base)(struct kgd_dev *kgd,
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/2] drm/amdkfd: Release an acquired process vm

[Oak Zeng]

For compute vm acquired from amdgpu, vm.pasid is managed
by kfd. Decouple pasid from such vm on process destroy
to avoid duplicate pasid release.

Signed-off-by: Oak Zeng <Oak.Zeng@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h        |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 26 +++++++++++++++++++++++
 drivers/gpu/drm/amd/amdkfd/kfd_process.c          |  4 +++-
 drivers/gpu/drm/amd/include/kgd_kfd_interface.h   |  1 +
 7 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
index e1f6454..b414889 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
@@ -165,6 +165,7 @@ int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct kgd_dev *kgd,
 void amdgpu_amdkfd_gpuvm_destroy_cb(struct amdgpu_device *adev,
 				struct amdgpu_vm *vm);
 void amdgpu_amdkfd_gpuvm_destroy_process_vm(struct kgd_dev *kgd, void *vm);
+void amdgpu_amdkfd_gpuvm_release_process_vm(struct kgd_dev *kgd, void *vm);
 uint32_t amdgpu_amdkfd_gpuvm_get_process_page_dir(void *vm);
 int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
 		struct kgd_dev *kgd, uint64_t va, uint64_t size,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
index ea79908..03a83d3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
@@ -204,6 +204,7 @@ static const struct kfd2kgd_calls kfd2kgd = {
 	.create_process_vm = amdgpu_amdkfd_gpuvm_create_process_vm,
 	.acquire_process_vm = amdgpu_amdkfd_gpuvm_acquire_process_vm,
 	.destroy_process_vm = amdgpu_amdkfd_gpuvm_destroy_process_vm,
+	.release_process_vm = amdgpu_amdkfd_gpuvm_release_process_vm,
 	.get_process_page_dir = amdgpu_amdkfd_gpuvm_get_process_page_dir,
 	.set_vm_context_page_table_base = set_vm_context_page_table_base,
 	.alloc_memory_of_gpu = amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
index 19dd665..b2b9c5f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
@@ -164,6 +164,7 @@ static const struct kfd2kgd_calls kfd2kgd = {
 	.create_process_vm = amdgpu_amdkfd_gpuvm_create_process_vm,
 	.acquire_process_vm = amdgpu_amdkfd_gpuvm_acquire_process_vm,
 	.destroy_process_vm = amdgpu_amdkfd_gpuvm_destroy_process_vm,
+	.release_process_vm = amdgpu_amdkfd_gpuvm_release_process_vm,
 	.get_process_page_dir = amdgpu_amdkfd_gpuvm_get_process_page_dir,
 	.set_vm_context_page_table_base = set_vm_context_page_table_base,
 	.alloc_memory_of_gpu = amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
index 1db60aa..3722bbd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
@@ -201,6 +201,7 @@ static const struct kfd2kgd_calls kfd2kgd = {
 	.create_process_vm = amdgpu_amdkfd_gpuvm_create_process_vm,
 	.acquire_process_vm = amdgpu_amdkfd_gpuvm_acquire_process_vm,
 	.destroy_process_vm = amdgpu_amdkfd_gpuvm_destroy_process_vm,
+	.release_process_vm = amdgpu_amdkfd_gpuvm_release_process_vm,
 	.get_process_page_dir = amdgpu_amdkfd_gpuvm_get_process_page_dir,
 	.set_vm_context_page_table_base = set_vm_context_page_table_base,
 	.alloc_memory_of_gpu = amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 0980a1f..6fd839c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -1117,6 +1117,32 @@ void amdgpu_amdkfd_gpuvm_destroy_process_vm(struct kgd_dev *kgd, void *vm)
 	kfree(vm);
 }
 
+void amdgpu_amdkfd_gpuvm_release_process_vm(struct kgd_dev *kgd, void *vm)
+{
+	struct amdgpu_device *adev = get_amdgpu_device(kgd);
+        struct amdgpu_vm *avm = (struct amdgpu_vm *)vm;
+
+        if (WARN_ON(!vm))
+                return;
+
+        pr_debug("Releasing process vm %p\n", vm);
+
+        /* The original pasid of amdgpu vm has already been
+         * released during making a amdgpu vm to a compute vm
+         * The current pasid is managed by kfd and will be
+         * released on kfd process destroy. Set amdgpu pasid
+         * to 0 to avoid duplicate release.
+         */
+	if (avm->pasid) {
+		unsigned long flags;
+
+		spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
+		idr_remove(&adev->vm_manager.pasid_idr, avm->pasid);
+		spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
+	}
+	avm->pasid = 0;
+}
+
 uint32_t amdgpu_amdkfd_gpuvm_get_process_page_dir(void *vm)
 {
 	struct amdgpu_vm *avm = (struct amdgpu_vm *)vm;
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
index 5f559c5..7afeaa1 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
@@ -320,8 +320,10 @@ static void kfd_process_destroy_pdds(struct kfd_process *p)
 		pr_debug("Releasing pdd (topology id %d) for process (pasid %d)\n",
 				pdd->dev->id, p->pasid);
 
-		if (pdd->drm_file)
+		if (pdd->drm_file) {
+			pdd->dev->kfd2kgd->release_process_vm(pdd->dev->kgd, pdd->vm);
 			fput(pdd->drm_file);
+		}
 		else if (pdd->vm)
 			pdd->dev->kfd2kgd->destroy_process_vm(
 				pdd->dev->kgd, pdd->vm);
diff --git a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
index 1632869..8fdd032 100644
--- a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
+++ b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
@@ -352,6 +352,7 @@ struct kfd2kgd_calls {
 			unsigned int pasid, void **vm, void **process_info,
 			struct dma_fence **ef);
 	void (*destroy_process_vm)(struct kgd_dev *kgd, void *vm);
+	void (*release_process_vm)(struct kgd_dev *kgd, void *vm);
 	uint32_t (*get_process_page_dir)(void *vm);
 	void (*set_vm_context_page_table_base)(struct kgd_dev *kgd,
 			uint32_t vmid, uint32_t page_table_base);
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdkfd: Release an acquired process vm

[Christian König]

Am 27.08.2018 um 22:45 schrieb Oak Zeng:
> For compute vm acquired from amdgpu, vm.pasid is managed
> by kfd. Decouple pasid from such vm on process destroy
> to avoid duplicate pasid release.
>
> Signed-off-by: Oak Zeng <Oak.Zeng@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h        |  1 +
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c |  1 +
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c |  1 +
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c |  1 +
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 26 +++++++++++++++++++++++
>   drivers/gpu/drm/amd/amdkfd/kfd_process.c          |  4 +++-
>   drivers/gpu/drm/amd/include/kgd_kfd_interface.h   |  1 +
>   7 files changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> index e1f6454..b414889 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> @@ -165,6 +165,7 @@ int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct kgd_dev *kgd,
>   void amdgpu_amdkfd_gpuvm_destroy_cb(struct amdgpu_device *adev,
>   				struct amdgpu_vm *vm);
>   void amdgpu_amdkfd_gpuvm_destroy_process_vm(struct kgd_dev *kgd, void *vm);
> +void amdgpu_amdkfd_gpuvm_release_process_vm(struct kgd_dev *kgd, void *vm);
>   uint32_t amdgpu_amdkfd_gpuvm_get_process_page_dir(void *vm);
>   int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
>   		struct kgd_dev *kgd, uint64_t va, uint64_t size,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
> index ea79908..03a83d3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
> @@ -204,6 +204,7 @@ static const struct kfd2kgd_calls kfd2kgd = {
>   	.create_process_vm = amdgpu_amdkfd_gpuvm_create_process_vm,
>   	.acquire_process_vm = amdgpu_amdkfd_gpuvm_acquire_process_vm,
>   	.destroy_process_vm = amdgpu_amdkfd_gpuvm_destroy_process_vm,
> +	.release_process_vm = amdgpu_amdkfd_gpuvm_release_process_vm,
>   	.get_process_page_dir = amdgpu_amdkfd_gpuvm_get_process_page_dir,
>   	.set_vm_context_page_table_base = set_vm_context_page_table_base,
>   	.alloc_memory_of_gpu = amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
> index 19dd665..b2b9c5f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
> @@ -164,6 +164,7 @@ static const struct kfd2kgd_calls kfd2kgd = {
>   	.create_process_vm = amdgpu_amdkfd_gpuvm_create_process_vm,
>   	.acquire_process_vm = amdgpu_amdkfd_gpuvm_acquire_process_vm,
>   	.destroy_process_vm = amdgpu_amdkfd_gpuvm_destroy_process_vm,
> +	.release_process_vm = amdgpu_amdkfd_gpuvm_release_process_vm,
>   	.get_process_page_dir = amdgpu_amdkfd_gpuvm_get_process_page_dir,
>   	.set_vm_context_page_table_base = set_vm_context_page_table_base,
>   	.alloc_memory_of_gpu = amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
> index 1db60aa..3722bbd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
> @@ -201,6 +201,7 @@ static const struct kfd2kgd_calls kfd2kgd = {
>   	.create_process_vm = amdgpu_amdkfd_gpuvm_create_process_vm,
>   	.acquire_process_vm = amdgpu_amdkfd_gpuvm_acquire_process_vm,
>   	.destroy_process_vm = amdgpu_amdkfd_gpuvm_destroy_process_vm,
> +	.release_process_vm = amdgpu_amdkfd_gpuvm_release_process_vm,
>   	.get_process_page_dir = amdgpu_amdkfd_gpuvm_get_process_page_dir,
>   	.set_vm_context_page_table_base = set_vm_context_page_table_base,
>   	.alloc_memory_of_gpu = amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index 0980a1f..6fd839c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -1117,6 +1117,32 @@ void amdgpu_amdkfd_gpuvm_destroy_process_vm(struct kgd_dev *kgd, void *vm)
>   	kfree(vm);
>   }
>   
> +void amdgpu_amdkfd_gpuvm_release_process_vm(struct kgd_dev *kgd, void *vm)
> +{
> +	struct amdgpu_device *adev = get_amdgpu_device(kgd);
> +        struct amdgpu_vm *avm = (struct amdgpu_vm *)vm;
> +
> +        if (WARN_ON(!vm))
> +                return;
> +
> +        pr_debug("Releasing process vm %p\n", vm);
> +
> +        /* The original pasid of amdgpu vm has already been
> +         * released during making a amdgpu vm to a compute vm
> +         * The current pasid is managed by kfd and will be
> +         * released on kfd process destroy. Set amdgpu pasid
> +         * to 0 to avoid duplicate release.
> +         */
> +	if (avm->pasid) {
> +		unsigned long flags;
> +
> +		spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
> +		idr_remove(&adev->vm_manager.pasid_idr, avm->pasid);
> +		spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
> +	}
> +	avm->pasid = 0;

Can we factor this out into a helper inside amdgpu_vm.c?

Would be nice to not manipulate VM internals here.

Christian.

> +}
> +
>   uint32_t amdgpu_amdkfd_gpuvm_get_process_page_dir(void *vm)
>   {
>   	struct amdgpu_vm *avm = (struct amdgpu_vm *)vm;
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> index 5f559c5..7afeaa1 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> @@ -320,8 +320,10 @@ static void kfd_process_destroy_pdds(struct kfd_process *p)
>   		pr_debug("Releasing pdd (topology id %d) for process (pasid %d)\n",
>   				pdd->dev->id, p->pasid);
>   
> -		if (pdd->drm_file)
> +		if (pdd->drm_file) {
> +			pdd->dev->kfd2kgd->release_process_vm(pdd->dev->kgd, pdd->vm);
>   			fput(pdd->drm_file);
> +		}
>   		else if (pdd->vm)
>   			pdd->dev->kfd2kgd->destroy_process_vm(
>   				pdd->dev->kgd, pdd->vm);
> diff --git a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
> index 1632869..8fdd032 100644
> --- a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
> +++ b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
> @@ -352,6 +352,7 @@ struct kfd2kgd_calls {
>   			unsigned int pasid, void **vm, void **process_info,
>   			struct dma_fence **ef);
>   	void (*destroy_process_vm)(struct kgd_dev *kgd, void *vm);
> +	void (*release_process_vm)(struct kgd_dev *kgd, void *vm);
>   	uint32_t (*get_process_page_dir)(void *vm);
>   	void (*set_vm_context_page_table_base)(struct kgd_dev *kgd,
>   			uint32_t vmid, uint32_t page_table_base);

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdkfd: Release an acquired process vm

[Oak]

Good point. I also planned to do it the way you suggested.

Sent from my iPad

On 2018-08-28, at 7:21 AM, Christian König <ckoenig.leichtzumerken@gmail.com> wrote:

> Am 27.08.2018 um 22:45 schrieb Oak Zeng:
>> For compute vm acquired from amdgpu, vm.pasid is managed
>> by kfd. Decouple pasid from such vm on process destroy
>> to avoid duplicate pasid release.
>> 
>> Signed-off-by: Oak Zeng <Oak.Zeng@amd.com>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h        |  1 +
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c |  1 +
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c |  1 +
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c |  1 +
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 26 +++++++++++++++++++++++
>>  drivers/gpu/drm/amd/amdkfd/kfd_process.c          |  4 +++-
>>  drivers/gpu/drm/amd/include/kgd_kfd_interface.h   |  1 +
>>  7 files changed, 34 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
>> index e1f6454..b414889 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
>> @@ -165,6 +165,7 @@ int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct kgd_dev *kgd,
>>  void amdgpu_amdkfd_gpuvm_destroy_cb(struct amdgpu_device *adev,
>>                  struct amdgpu_vm *vm);
>>  void amdgpu_amdkfd_gpuvm_destroy_process_vm(struct kgd_dev *kgd, void *vm);
>> +void amdgpu_amdkfd_gpuvm_release_process_vm(struct kgd_dev *kgd, void *vm);
>>  uint32_t amdgpu_amdkfd_gpuvm_get_process_page_dir(void *vm);
>>  int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
>>          struct kgd_dev *kgd, uint64_t va, uint64_t size,
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
>> index ea79908..03a83d3 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
>> @@ -204,6 +204,7 @@ static const struct kfd2kgd_calls kfd2kgd = {
>>      .create_process_vm = amdgpu_amdkfd_gpuvm_create_process_vm,
>>      .acquire_process_vm = amdgpu_amdkfd_gpuvm_acquire_process_vm,
>>      .destroy_process_vm = amdgpu_amdkfd_gpuvm_destroy_process_vm,
>> +    .release_process_vm = amdgpu_amdkfd_gpuvm_release_process_vm,
>>      .get_process_page_dir = amdgpu_amdkfd_gpuvm_get_process_page_dir,
>>      .set_vm_context_page_table_base = set_vm_context_page_table_base,
>>      .alloc_memory_of_gpu = amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu,
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
>> index 19dd665..b2b9c5f 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
>> @@ -164,6 +164,7 @@ static const struct kfd2kgd_calls kfd2kgd = {
>>      .create_process_vm = amdgpu_amdkfd_gpuvm_create_process_vm,
>>      .acquire_process_vm = amdgpu_amdkfd_gpuvm_acquire_process_vm,
>>      .destroy_process_vm = amdgpu_amdkfd_gpuvm_destroy_process_vm,
>> +    .release_process_vm = amdgpu_amdkfd_gpuvm_release_process_vm,
>>      .get_process_page_dir = amdgpu_amdkfd_gpuvm_get_process_page_dir,
>>      .set_vm_context_page_table_base = set_vm_context_page_table_base,
>>      .alloc_memory_of_gpu = amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu,
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
>> index 1db60aa..3722bbd 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
>> @@ -201,6 +201,7 @@ static const struct kfd2kgd_calls kfd2kgd = {
>>      .create_process_vm = amdgpu_amdkfd_gpuvm_create_process_vm,
>>      .acquire_process_vm = amdgpu_amdkfd_gpuvm_acquire_process_vm,
>>      .destroy_process_vm = amdgpu_amdkfd_gpuvm_destroy_process_vm,
>> +    .release_process_vm = amdgpu_amdkfd_gpuvm_release_process_vm,
>>      .get_process_page_dir = amdgpu_amdkfd_gpuvm_get_process_page_dir,
>>      .set_vm_context_page_table_base = set_vm_context_page_table_base,
>>      .alloc_memory_of_gpu = amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu,
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> index 0980a1f..6fd839c 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> @@ -1117,6 +1117,32 @@ void amdgpu_amdkfd_gpuvm_destroy_process_vm(struct kgd_dev *kgd, void *vm)
>>      kfree(vm);
>>  }
>>  +void amdgpu_amdkfd_gpuvm_release_process_vm(struct kgd_dev *kgd, void *vm)
>> +{
>> +    struct amdgpu_device *adev = get_amdgpu_device(kgd);
>> +        struct amdgpu_vm *avm = (struct amdgpu_vm *)vm;
>> +
>> +        if (WARN_ON(!vm))
>> +                return;
>> +
>> +        pr_debug("Releasing process vm %p\n", vm);
>> +
>> +        /* The original pasid of amdgpu vm has already been
>> +         * released during making a amdgpu vm to a compute vm
>> +         * The current pasid is managed by kfd and will be
>> +         * released on kfd process destroy. Set amdgpu pasid
>> +         * to 0 to avoid duplicate release.
>> +         */
>> +    if (avm->pasid) {
>> +        unsigned long flags;
>> +
>> +        spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>> +        idr_remove(&adev->vm_manager.pasid_idr, avm->pasid);
>> +        spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>> +    }
>> +    avm->pasid = 0;
> 
> Can we factor this out into a helper inside amdgpu_vm.c?
> 
> Would be nice to not manipulate VM internals here.
> 
> Christian.
> 
>> +}
>> +
>>  uint32_t amdgpu_amdkfd_gpuvm_get_process_page_dir(void *vm)
>>  {
>>      struct amdgpu_vm *avm = (struct amdgpu_vm *)vm;
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>> index 5f559c5..7afeaa1 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>> @@ -320,8 +320,10 @@ static void kfd_process_destroy_pdds(struct kfd_process *p)
>>          pr_debug("Releasing pdd (topology id %d) for process (pasid %d)\n",
>>                  pdd->dev->id, p->pasid);
>>  -        if (pdd->drm_file)
>> +        if (pdd->drm_file) {
>> +            pdd->dev->kfd2kgd->release_process_vm(pdd->dev->kgd, pdd->vm);
>>              fput(pdd->drm_file);
>> +        }
>>          else if (pdd->vm)
>>              pdd->dev->kfd2kgd->destroy_process_vm(
>>                  pdd->dev->kgd, pdd->vm);
>> diff --git a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
>> index 1632869..8fdd032 100644
>> --- a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
>> +++ b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
>> @@ -352,6 +352,7 @@ struct kfd2kgd_calls {
>>              unsigned int pasid, void **vm, void **process_info,
>>              struct dma_fence **ef);
>>      void (*destroy_process_vm)(struct kgd_dev *kgd, void *vm);
>> +    void (*release_process_vm)(struct kgd_dev *kgd, void *vm);
>>      uint32_t (*get_process_page_dir)(void *vm);
>>      void (*set_vm_context_page_table_base)(struct kgd_dev *kgd,
>>              uint32_t vmid, uint32_t page_table_base);
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdkfd: Release an acquired process vm

[Christian König]

Am 28.08.2018 um 19:34 schrieb Oak Zeng:
> For compute vm acquired from amdgpu, vm.pasid is managed
> by kfd. Decouple pasid from such vm on process destroy
> to avoid duplicate pasid release.
>
> Signed-off-by: Oak Zeng <Oak.Zeng@amd.com>

One minor nit pick below, apart from that Acked-by: Christian König 
<christian.koenig@amd.com>

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h        |  1 +
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c |  1 +
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c |  1 +
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c |  1 +
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 19 +++++++++++++++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c            | 20 ++++++++++++++++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h            |  1 +
>   drivers/gpu/drm/amd/amdkfd/kfd_process.c          |  4 +++-
>   drivers/gpu/drm/amd/include/kgd_kfd_interface.h   |  1 +
>   9 files changed, 48 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> index e1f6454..b414889 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> @@ -165,6 +165,7 @@ int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct kgd_dev *kgd,
>   void amdgpu_amdkfd_gpuvm_destroy_cb(struct amdgpu_device *adev,
>   				struct amdgpu_vm *vm);
>   void amdgpu_amdkfd_gpuvm_destroy_process_vm(struct kgd_dev *kgd, void *vm);
> +void amdgpu_amdkfd_gpuvm_release_process_vm(struct kgd_dev *kgd, void *vm);
>   uint32_t amdgpu_amdkfd_gpuvm_get_process_page_dir(void *vm);
>   int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
>   		struct kgd_dev *kgd, uint64_t va, uint64_t size,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
> index ea79908..03a83d3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
> @@ -204,6 +204,7 @@ static const struct kfd2kgd_calls kfd2kgd = {
>   	.create_process_vm = amdgpu_amdkfd_gpuvm_create_process_vm,
>   	.acquire_process_vm = amdgpu_amdkfd_gpuvm_acquire_process_vm,
>   	.destroy_process_vm = amdgpu_amdkfd_gpuvm_destroy_process_vm,
> +	.release_process_vm = amdgpu_amdkfd_gpuvm_release_process_vm,
>   	.get_process_page_dir = amdgpu_amdkfd_gpuvm_get_process_page_dir,
>   	.set_vm_context_page_table_base = set_vm_context_page_table_base,
>   	.alloc_memory_of_gpu = amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
> index 19dd665..b2b9c5f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
> @@ -164,6 +164,7 @@ static const struct kfd2kgd_calls kfd2kgd = {
>   	.create_process_vm = amdgpu_amdkfd_gpuvm_create_process_vm,
>   	.acquire_process_vm = amdgpu_amdkfd_gpuvm_acquire_process_vm,
>   	.destroy_process_vm = amdgpu_amdkfd_gpuvm_destroy_process_vm,
> +	.release_process_vm = amdgpu_amdkfd_gpuvm_release_process_vm,
>   	.get_process_page_dir = amdgpu_amdkfd_gpuvm_get_process_page_dir,
>   	.set_vm_context_page_table_base = set_vm_context_page_table_base,
>   	.alloc_memory_of_gpu = amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
> index 1db60aa..3722bbd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
> @@ -201,6 +201,7 @@ static const struct kfd2kgd_calls kfd2kgd = {
>   	.create_process_vm = amdgpu_amdkfd_gpuvm_create_process_vm,
>   	.acquire_process_vm = amdgpu_amdkfd_gpuvm_acquire_process_vm,
>   	.destroy_process_vm = amdgpu_amdkfd_gpuvm_destroy_process_vm,
> +	.release_process_vm = amdgpu_amdkfd_gpuvm_release_process_vm,
>   	.get_process_page_dir = amdgpu_amdkfd_gpuvm_get_process_page_dir,
>   	.set_vm_context_page_table_base = set_vm_context_page_table_base,
>   	.alloc_memory_of_gpu = amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index 0980a1f..bc4413f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -1117,6 +1117,25 @@ void amdgpu_amdkfd_gpuvm_destroy_process_vm(struct kgd_dev *kgd, void *vm)
>   	kfree(vm);
>   }
>   
> +void amdgpu_amdkfd_gpuvm_release_process_vm(struct kgd_dev *kgd, void *vm)
> +{
> +	struct amdgpu_device *adev = get_amdgpu_device(kgd);
> +        struct amdgpu_vm *avm = (struct amdgpu_vm *)vm;
> +
> +	if (WARN_ON(!kgd || !vm))
> +                return;
> +
> +        pr_debug("Releasing process vm %p\n", vm);
> +
> +        /* The original pasid of amdgpu vm has already been
> +         * released during making a amdgpu vm to a compute vm
> +         * The current pasid is managed by kfd and will be
> +         * released on kfd process destroy. Set amdgpu pasid
> +         * to 0 to avoid duplicate release.
> +         */
> +	amdgpu_vm_release_compute(adev, avm);
> +}
> +
>   uint32_t amdgpu_amdkfd_gpuvm_get_process_page_dir(void *vm)
>   {
>   	struct amdgpu_vm *avm = (struct amdgpu_vm *)vm;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index d8a99f4..d93f2f8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2766,6 +2766,26 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm, uns
>   }
>   
>   /**
> + * amdgpu_vm_release_compute - release a compute vm
> + * @adev: amdgpu_device pointer
> + * @vm: a vm turned into compute vm by calling amdgpu_vm_make_compute
> + *
> + * This is a correspondant of amdgpu_vm_make_compute. It decouples compute
> + * pasid from vm. Compute should stop use of vm after this call.
> + */
> +void amdgpu_vm_release_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
> +{
> +	if (vm->pasid) {
> +		unsigned long flags;
> +
> +		spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
> +		idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
> +		spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
> +	}
> +	vm->pasid = 0;
> +}
> +
> +/**
>    * amdgpu_vm_free_levels - free PD/PT levels
>    *
>    * @adev: amdgpu device structure
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index c68945d..bd55516 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -295,6 +295,7 @@ void amdgpu_vm_manager_fini(struct amdgpu_device *adev);
>   int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   		   int vm_context, unsigned int pasid);
>   int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm, unsigned int pasid);
> +void amdgpu_vm_release_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm);
>   void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm);
>   bool amdgpu_vm_pasid_fault_credit(struct amdgpu_device *adev,
>   				  unsigned int pasid);
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> index 5f559c5..7afeaa1 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> @@ -320,8 +320,10 @@ static void kfd_process_destroy_pdds(struct kfd_process *p)
>   		pr_debug("Releasing pdd (topology id %d) for process (pasid %d)\n",
>   				pdd->dev->id, p->pasid);
>   
> -		if (pdd->drm_file)
> +		if (pdd->drm_file) {
> +			pdd->dev->kfd2kgd->release_process_vm(pdd->dev->kgd, pdd->vm);
>   			fput(pdd->drm_file);
> +		}
>   		else if (pdd->vm)

Coding style says when one branch of an if uses {} all other branches 
should do as well. In other words use "} else if (..) {" here.

Christian.

>   			pdd->dev->kfd2kgd->destroy_process_vm(
>   				pdd->dev->kgd, pdd->vm);
> diff --git a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
> index 1632869..8fdd032 100644
> --- a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
> +++ b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
> @@ -352,6 +352,7 @@ struct kfd2kgd_calls {
>   			unsigned int pasid, void **vm, void **process_info,
>   			struct dma_fence **ef);
>   	void (*destroy_process_vm)(struct kgd_dev *kgd, void *vm);
> +	void (*release_process_vm)(struct kgd_dev *kgd, void *vm);
>   	uint32_t (*get_process_page_dir)(void *vm);
>   	void (*set_vm_context_page_table_base)(struct kgd_dev *kgd,
>   			uint32_t vmid, uint32_t page_table_base);

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/2] drm/amdkfd: Release an acquired process vm

[Oak Zeng]

For compute vm acquired from amdgpu, vm.pasid is managed
by kfd. Decouple pasid from such vm on process destroy
to avoid duplicate pasid release.

Signed-off-by: Oak Zeng <Oak.Zeng@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h        |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 19 +++++++++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c            | 20 ++++++++++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h            |  1 +
 drivers/gpu/drm/amd/amdkfd/kfd_process.c          |  4 +++-
 drivers/gpu/drm/amd/include/kgd_kfd_interface.h   |  1 +
 9 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
index e1f6454..b414889 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
@@ -165,6 +165,7 @@ int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct kgd_dev *kgd,
 void amdgpu_amdkfd_gpuvm_destroy_cb(struct amdgpu_device *adev,
 				struct amdgpu_vm *vm);
 void amdgpu_amdkfd_gpuvm_destroy_process_vm(struct kgd_dev *kgd, void *vm);
+void amdgpu_amdkfd_gpuvm_release_process_vm(struct kgd_dev *kgd, void *vm);
 uint32_t amdgpu_amdkfd_gpuvm_get_process_page_dir(void *vm);
 int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
 		struct kgd_dev *kgd, uint64_t va, uint64_t size,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
index ea79908..03a83d3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
@@ -204,6 +204,7 @@ static const struct kfd2kgd_calls kfd2kgd = {
 	.create_process_vm = amdgpu_amdkfd_gpuvm_create_process_vm,
 	.acquire_process_vm = amdgpu_amdkfd_gpuvm_acquire_process_vm,
 	.destroy_process_vm = amdgpu_amdkfd_gpuvm_destroy_process_vm,
+	.release_process_vm = amdgpu_amdkfd_gpuvm_release_process_vm,
 	.get_process_page_dir = amdgpu_amdkfd_gpuvm_get_process_page_dir,
 	.set_vm_context_page_table_base = set_vm_context_page_table_base,
 	.alloc_memory_of_gpu = amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
index 19dd665..b2b9c5f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
@@ -164,6 +164,7 @@ static const struct kfd2kgd_calls kfd2kgd = {
 	.create_process_vm = amdgpu_amdkfd_gpuvm_create_process_vm,
 	.acquire_process_vm = amdgpu_amdkfd_gpuvm_acquire_process_vm,
 	.destroy_process_vm = amdgpu_amdkfd_gpuvm_destroy_process_vm,
+	.release_process_vm = amdgpu_amdkfd_gpuvm_release_process_vm,
 	.get_process_page_dir = amdgpu_amdkfd_gpuvm_get_process_page_dir,
 	.set_vm_context_page_table_base = set_vm_context_page_table_base,
 	.alloc_memory_of_gpu = amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
index 1db60aa..3722bbd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
@@ -201,6 +201,7 @@ static const struct kfd2kgd_calls kfd2kgd = {
 	.create_process_vm = amdgpu_amdkfd_gpuvm_create_process_vm,
 	.acquire_process_vm = amdgpu_amdkfd_gpuvm_acquire_process_vm,
 	.destroy_process_vm = amdgpu_amdkfd_gpuvm_destroy_process_vm,
+	.release_process_vm = amdgpu_amdkfd_gpuvm_release_process_vm,
 	.get_process_page_dir = amdgpu_amdkfd_gpuvm_get_process_page_dir,
 	.set_vm_context_page_table_base = set_vm_context_page_table_base,
 	.alloc_memory_of_gpu = amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 0980a1f..bc4413f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -1117,6 +1117,25 @@ void amdgpu_amdkfd_gpuvm_destroy_process_vm(struct kgd_dev *kgd, void *vm)
 	kfree(vm);
 }
 
+void amdgpu_amdkfd_gpuvm_release_process_vm(struct kgd_dev *kgd, void *vm)
+{
+	struct amdgpu_device *adev = get_amdgpu_device(kgd);
+        struct amdgpu_vm *avm = (struct amdgpu_vm *)vm;
+
+	if (WARN_ON(!kgd || !vm))
+                return;
+
+        pr_debug("Releasing process vm %p\n", vm);
+
+        /* The original pasid of amdgpu vm has already been
+         * released during making a amdgpu vm to a compute vm
+         * The current pasid is managed by kfd and will be
+         * released on kfd process destroy. Set amdgpu pasid
+         * to 0 to avoid duplicate release.
+         */
+	amdgpu_vm_release_compute(adev, avm);
+}
+
 uint32_t amdgpu_amdkfd_gpuvm_get_process_page_dir(void *vm)
 {
 	struct amdgpu_vm *avm = (struct amdgpu_vm *)vm;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index d8a99f4..d93f2f8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2766,6 +2766,26 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm, uns
 }
 
 /**
+ * amdgpu_vm_release_compute - release a compute vm
+ * @adev: amdgpu_device pointer
+ * @vm: a vm turned into compute vm by calling amdgpu_vm_make_compute
+ *
+ * This is a correspondant of amdgpu_vm_make_compute. It decouples compute
+ * pasid from vm. Compute should stop use of vm after this call.
+ */
+void amdgpu_vm_release_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
+{
+	if (vm->pasid) {
+		unsigned long flags;
+
+		spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
+		idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
+		spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
+	}
+	vm->pasid = 0;
+}
+
+/**
  * amdgpu_vm_free_levels - free PD/PT levels
  *
  * @adev: amdgpu device structure
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index c68945d..bd55516 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -295,6 +295,7 @@ void amdgpu_vm_manager_fini(struct amdgpu_device *adev);
 int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 		   int vm_context, unsigned int pasid);
 int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm, unsigned int pasid);
+void amdgpu_vm_release_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm);
 void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm);
 bool amdgpu_vm_pasid_fault_credit(struct amdgpu_device *adev,
 				  unsigned int pasid);
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
index 5f559c5..7afeaa1 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
@@ -320,8 +320,10 @@ static void kfd_process_destroy_pdds(struct kfd_process *p)
 		pr_debug("Releasing pdd (topology id %d) for process (pasid %d)\n",
 				pdd->dev->id, p->pasid);
 
-		if (pdd->drm_file)
+		if (pdd->drm_file) {
+			pdd->dev->kfd2kgd->release_process_vm(pdd->dev->kgd, pdd->vm);
 			fput(pdd->drm_file);
+		}
 		else if (pdd->vm)
 			pdd->dev->kfd2kgd->destroy_process_vm(
 				pdd->dev->kgd, pdd->vm);
diff --git a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
index 1632869..8fdd032 100644
--- a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
+++ b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
@@ -352,6 +352,7 @@ struct kfd2kgd_calls {
 			unsigned int pasid, void **vm, void **process_info,
 			struct dma_fence **ef);
 	void (*destroy_process_vm)(struct kgd_dev *kgd, void *vm);
+	void (*release_process_vm)(struct kgd_dev *kgd, void *vm);
 	uint32_t (*get_process_page_dir)(void *vm);
 	void (*set_vm_context_page_table_base)(struct kgd_dev *kgd,
 			uint32_t vmid, uint32_t page_table_base);
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 2/2] drm/amdkfd: Release an acquired process vm

[Zeng, Oak]

Thank you Felix. That is a great point. I will update patch. 

Thanks,
Oak

-----Original Message-----
From: Kuehling, Felix 
Sent: Monday, August 27, 2018 4:05 PM
To: Oak Zeng <zengshanjun@gmail.com>; amd-gfx@lists.freedesktop.org
Cc: Zeng, Oak <Oak.Zeng@amd.com>
Subject: Re: [PATCH 2/2] drm/amdkfd: Release an acquired process vm

On 2018-08-27 03:25 PM, Oak Zeng wrote:
> For compute vm acquired from amdgpu, vm.pasid is managed by kfd. 
> Decouple pasid from such vm on process destroy to avoid duplicate 
> pasid release.
>
> Signed-off-by: Oak Zeng <Oak.Zeng@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h        |  1 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c |  1 +  
> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c |  1 +  
> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c |  1 +  
> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 18 ++++++++++++++++++
>  drivers/gpu/drm/amd/amdkfd/kfd_process.c          |  4 +++-
>  drivers/gpu/drm/amd/include/kgd_kfd_interface.h   |  1 +
>  7 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> index 2895ef5..5827734 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> @@ -166,6 +166,7 @@ int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct 
> kgd_dev *kgd,  void amdgpu_amdkfd_gpuvm_destroy_cb(struct amdgpu_device *adev,
>  				struct amdgpu_vm *vm);
>  void amdgpu_amdkfd_gpuvm_destroy_process_vm(struct kgd_dev *kgd, void 
> *vm);
> +void amdgpu_amdkfd_gpuvm_release_process_vm(void *vm);
>  uint32_t amdgpu_amdkfd_gpuvm_get_process_page_dir(void *vm);  int 
> amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
>  		struct kgd_dev *kgd, uint64_t va, uint64_t size, diff --git 
> a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
> index ea79908..03a83d3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
> @@ -204,6 +204,7 @@ static const struct kfd2kgd_calls kfd2kgd = {
>  	.create_process_vm = amdgpu_amdkfd_gpuvm_create_process_vm,
>  	.acquire_process_vm = amdgpu_amdkfd_gpuvm_acquire_process_vm,
>  	.destroy_process_vm = amdgpu_amdkfd_gpuvm_destroy_process_vm,
> +	.release_process_vm = amdgpu_amdkfd_gpuvm_release_process_vm,
>  	.get_process_page_dir = amdgpu_amdkfd_gpuvm_get_process_page_dir,
>  	.set_vm_context_page_table_base = set_vm_context_page_table_base,
>  	.alloc_memory_of_gpu = amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
> index 19dd665..b2b9c5f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
> @@ -164,6 +164,7 @@ static const struct kfd2kgd_calls kfd2kgd = {
>  	.create_process_vm = amdgpu_amdkfd_gpuvm_create_process_vm,
>  	.acquire_process_vm = amdgpu_amdkfd_gpuvm_acquire_process_vm,
>  	.destroy_process_vm = amdgpu_amdkfd_gpuvm_destroy_process_vm,
> +	.release_process_vm = amdgpu_amdkfd_gpuvm_release_process_vm,
>  	.get_process_page_dir = amdgpu_amdkfd_gpuvm_get_process_page_dir,
>  	.set_vm_context_page_table_base = set_vm_context_page_table_base,
>  	.alloc_memory_of_gpu = amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
> index 1db60aa..3722bbd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
> @@ -201,6 +201,7 @@ static const struct kfd2kgd_calls kfd2kgd = {
>  	.create_process_vm = amdgpu_amdkfd_gpuvm_create_process_vm,
>  	.acquire_process_vm = amdgpu_amdkfd_gpuvm_acquire_process_vm,
>  	.destroy_process_vm = amdgpu_amdkfd_gpuvm_destroy_process_vm,
> +	.release_process_vm = amdgpu_amdkfd_gpuvm_release_process_vm,
>  	.get_process_page_dir = amdgpu_amdkfd_gpuvm_get_process_page_dir,
>  	.set_vm_context_page_table_base = set_vm_context_page_table_base,
>  	.alloc_memory_of_gpu = amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index d60940e..7d1cdcd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -1118,6 +1118,24 @@ void amdgpu_amdkfd_gpuvm_destroy_process_vm(struct kgd_dev *kgd, void *vm)
>  	kfree(vm);
>  }
>  
> +void amdgpu_amdkfd_gpuvm_release_process_vm(void *vm) {
> +        struct amdgpu_vm *avm = (struct amdgpu_vm *)vm;
> +
> +        if (WARN_ON(!vm))
> +                return;       
> +
> +        pr_debug("Releasing process vm %p\n", vm);
> +
> +        /* The original pasid of amdgpu vm has already been
> +         * released during making a amdgpu vm to a compute vm
> +         * The current pasid is managed by kfd and will be
> +         * released on kfd process destroy. Set amdgpu pasid
> +         * to 0 to avoid duplicate release.
> +         */
> +        avm->pasid = 0;

Don't you also need to call idr_remove(&adev->vm_manager.pasid_idr,
avm->pasid) here? After setting avm->pasid to 0, amdgpu_vm_fini won't
take care of this any more.

Regards,
  Felix

> +}
> +
>  uint32_t amdgpu_amdkfd_gpuvm_get_process_page_dir(void *vm)  {
>  	struct amdgpu_vm *avm = (struct amdgpu_vm *)vm; diff --git 
> a/drivers/gpu/drm/amd/amdkfd/kfd_process.c 
> b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> index 571edd8..a716422 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> @@ -320,8 +320,10 @@ static void kfd_process_destroy_pdds(struct kfd_process *p)
>  		pr_debug("Releasing pdd (topology id %d) for process (pasid %d)\n",
>  				pdd->dev->id, p->pasid);
>  
> -		if (pdd->drm_file)
> +		if (pdd->drm_file) {
> +			pdd->dev->kfd2kgd->release_process_vm(pdd->vm);
>  			fput(pdd->drm_file);
> +		}
>  		else if (pdd->vm)
>  			pdd->dev->kfd2kgd->destroy_process_vm(
>  				pdd->dev->kgd, pdd->vm);
> diff --git a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h 
> b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
> index d040241..8e6d6bf 100644
> --- a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
> +++ b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
> @@ -352,6 +352,7 @@ struct kfd2kgd_calls {
>  			void **vm, void **process_info, unsigned int pasid,
>  			struct dma_fence **ef);
>  	void (*destroy_process_vm)(struct kgd_dev *kgd, void *vm);
> +	void (*release_process_vm)(void *vm);
>  	uint32_t (*get_process_page_dir)(void *vm);
>  	void (*set_vm_context_page_table_base)(struct kgd_dev *kgd,
>  			uint32_t vmid, uint32_t page_table_base);

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdkfd: Release an acquired process vm

[Felix Kuehling]

On 2018-08-27 03:25 PM, Oak Zeng wrote:
> For compute vm acquired from amdgpu, vm.pasid is managed
> by kfd. Decouple pasid from such vm on process destroy
> to avoid duplicate pasid release.
>
> Signed-off-by: Oak Zeng <Oak.Zeng@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h        |  1 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c |  1 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c |  1 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c |  1 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 18 ++++++++++++++++++
>  drivers/gpu/drm/amd/amdkfd/kfd_process.c          |  4 +++-
>  drivers/gpu/drm/amd/include/kgd_kfd_interface.h   |  1 +
>  7 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> index 2895ef5..5827734 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> @@ -166,6 +166,7 @@ int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct kgd_dev *kgd,
>  void amdgpu_amdkfd_gpuvm_destroy_cb(struct amdgpu_device *adev,
>  				struct amdgpu_vm *vm);
>  void amdgpu_amdkfd_gpuvm_destroy_process_vm(struct kgd_dev *kgd, void *vm);
> +void amdgpu_amdkfd_gpuvm_release_process_vm(void *vm);
>  uint32_t amdgpu_amdkfd_gpuvm_get_process_page_dir(void *vm);
>  int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
>  		struct kgd_dev *kgd, uint64_t va, uint64_t size,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
> index ea79908..03a83d3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
> @@ -204,6 +204,7 @@ static const struct kfd2kgd_calls kfd2kgd = {
>  	.create_process_vm = amdgpu_amdkfd_gpuvm_create_process_vm,
>  	.acquire_process_vm = amdgpu_amdkfd_gpuvm_acquire_process_vm,
>  	.destroy_process_vm = amdgpu_amdkfd_gpuvm_destroy_process_vm,
> +	.release_process_vm = amdgpu_amdkfd_gpuvm_release_process_vm,
>  	.get_process_page_dir = amdgpu_amdkfd_gpuvm_get_process_page_dir,
>  	.set_vm_context_page_table_base = set_vm_context_page_table_base,
>  	.alloc_memory_of_gpu = amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
> index 19dd665..b2b9c5f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
> @@ -164,6 +164,7 @@ static const struct kfd2kgd_calls kfd2kgd = {
>  	.create_process_vm = amdgpu_amdkfd_gpuvm_create_process_vm,
>  	.acquire_process_vm = amdgpu_amdkfd_gpuvm_acquire_process_vm,
>  	.destroy_process_vm = amdgpu_amdkfd_gpuvm_destroy_process_vm,
> +	.release_process_vm = amdgpu_amdkfd_gpuvm_release_process_vm,
>  	.get_process_page_dir = amdgpu_amdkfd_gpuvm_get_process_page_dir,
>  	.set_vm_context_page_table_base = set_vm_context_page_table_base,
>  	.alloc_memory_of_gpu = amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
> index 1db60aa..3722bbd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
> @@ -201,6 +201,7 @@ static const struct kfd2kgd_calls kfd2kgd = {
>  	.create_process_vm = amdgpu_amdkfd_gpuvm_create_process_vm,
>  	.acquire_process_vm = amdgpu_amdkfd_gpuvm_acquire_process_vm,
>  	.destroy_process_vm = amdgpu_amdkfd_gpuvm_destroy_process_vm,
> +	.release_process_vm = amdgpu_amdkfd_gpuvm_release_process_vm,
>  	.get_process_page_dir = amdgpu_amdkfd_gpuvm_get_process_page_dir,
>  	.set_vm_context_page_table_base = set_vm_context_page_table_base,
>  	.alloc_memory_of_gpu = amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index d60940e..7d1cdcd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -1118,6 +1118,24 @@ void amdgpu_amdkfd_gpuvm_destroy_process_vm(struct kgd_dev *kgd, void *vm)
>  	kfree(vm);
>  }
>  
> +void amdgpu_amdkfd_gpuvm_release_process_vm(void *vm)
> +{
> +        struct amdgpu_vm *avm = (struct amdgpu_vm *)vm; 
> +
> +        if (WARN_ON(!vm))
> +                return;       
> +
> +        pr_debug("Releasing process vm %p\n", vm);
> +
> +        /* The original pasid of amdgpu vm has already been
> +         * released during making a amdgpu vm to a compute vm
> +         * The current pasid is managed by kfd and will be
> +         * released on kfd process destroy. Set amdgpu pasid
> +         * to 0 to avoid duplicate release.
> +         */
> +        avm->pasid = 0;

Don't you also need to call idr_remove(&adev->vm_manager.pasid_idr,
avm->pasid) here? After setting avm->pasid to 0, amdgpu_vm_fini won't
take care of this any more.

Regards,
  Felix

> +}
> +
>  uint32_t amdgpu_amdkfd_gpuvm_get_process_page_dir(void *vm)
>  {
>  	struct amdgpu_vm *avm = (struct amdgpu_vm *)vm;
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> index 571edd8..a716422 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> @@ -320,8 +320,10 @@ static void kfd_process_destroy_pdds(struct kfd_process *p)
>  		pr_debug("Releasing pdd (topology id %d) for process (pasid %d)\n",
>  				pdd->dev->id, p->pasid);
>  
> -		if (pdd->drm_file)
> +		if (pdd->drm_file) {
> +			pdd->dev->kfd2kgd->release_process_vm(pdd->vm);
>  			fput(pdd->drm_file);
> +		}
>  		else if (pdd->vm)
>  			pdd->dev->kfd2kgd->destroy_process_vm(
>  				pdd->dev->kgd, pdd->vm);
> diff --git a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
> index d040241..8e6d6bf 100644
> --- a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
> +++ b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
> @@ -352,6 +352,7 @@ struct kfd2kgd_calls {
>  			void **vm, void **process_info, unsigned int pasid,
>  			struct dma_fence **ef);
>  	void (*destroy_process_vm)(struct kgd_dev *kgd, void *vm);
> +	void (*release_process_vm)(void *vm);
>  	uint32_t (*get_process_page_dir)(void *vm);
>  	void (*set_vm_context_page_table_base)(struct kgd_dev *kgd,
>  			uint32_t vmid, uint32_t page_table_base);

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/2] drm/amdkfd: Release an acquired process vm

[Oak Zeng]

For compute vm acquired from amdgpu, vm.pasid is managed
by kfd. Decouple pasid from such vm on process destroy
to avoid duplicate pasid release.

Signed-off-by: Oak Zeng <Oak.Zeng@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h        |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 18 ++++++++++++++++++
 drivers/gpu/drm/amd/amdkfd/kfd_process.c          |  4 +++-
 drivers/gpu/drm/amd/include/kgd_kfd_interface.h   |  1 +
 7 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
index 2895ef5..5827734 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
@@ -166,6 +166,7 @@ int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct kgd_dev *kgd,
 void amdgpu_amdkfd_gpuvm_destroy_cb(struct amdgpu_device *adev,
 				struct amdgpu_vm *vm);
 void amdgpu_amdkfd_gpuvm_destroy_process_vm(struct kgd_dev *kgd, void *vm);
+void amdgpu_amdkfd_gpuvm_release_process_vm(void *vm);
 uint32_t amdgpu_amdkfd_gpuvm_get_process_page_dir(void *vm);
 int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
 		struct kgd_dev *kgd, uint64_t va, uint64_t size,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
index ea79908..03a83d3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v7.c
@@ -204,6 +204,7 @@ static const struct kfd2kgd_calls kfd2kgd = {
 	.create_process_vm = amdgpu_amdkfd_gpuvm_create_process_vm,
 	.acquire_process_vm = amdgpu_amdkfd_gpuvm_acquire_process_vm,
 	.destroy_process_vm = amdgpu_amdkfd_gpuvm_destroy_process_vm,
+	.release_process_vm = amdgpu_amdkfd_gpuvm_release_process_vm,
 	.get_process_page_dir = amdgpu_amdkfd_gpuvm_get_process_page_dir,
 	.set_vm_context_page_table_base = set_vm_context_page_table_base,
 	.alloc_memory_of_gpu = amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
index 19dd665..b2b9c5f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v8.c
@@ -164,6 +164,7 @@ static const struct kfd2kgd_calls kfd2kgd = {
 	.create_process_vm = amdgpu_amdkfd_gpuvm_create_process_vm,
 	.acquire_process_vm = amdgpu_amdkfd_gpuvm_acquire_process_vm,
 	.destroy_process_vm = amdgpu_amdkfd_gpuvm_destroy_process_vm,
+	.release_process_vm = amdgpu_amdkfd_gpuvm_release_process_vm,
 	.get_process_page_dir = amdgpu_amdkfd_gpuvm_get_process_page_dir,
 	.set_vm_context_page_table_base = set_vm_context_page_table_base,
 	.alloc_memory_of_gpu = amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
index 1db60aa..3722bbd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v9.c
@@ -201,6 +201,7 @@ static const struct kfd2kgd_calls kfd2kgd = {
 	.create_process_vm = amdgpu_amdkfd_gpuvm_create_process_vm,
 	.acquire_process_vm = amdgpu_amdkfd_gpuvm_acquire_process_vm,
 	.destroy_process_vm = amdgpu_amdkfd_gpuvm_destroy_process_vm,
+	.release_process_vm = amdgpu_amdkfd_gpuvm_release_process_vm,
 	.get_process_page_dir = amdgpu_amdkfd_gpuvm_get_process_page_dir,
 	.set_vm_context_page_table_base = set_vm_context_page_table_base,
 	.alloc_memory_of_gpu = amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index d60940e..7d1cdcd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -1118,6 +1118,24 @@ void amdgpu_amdkfd_gpuvm_destroy_process_vm(struct kgd_dev *kgd, void *vm)
 	kfree(vm);
 }
 
+void amdgpu_amdkfd_gpuvm_release_process_vm(void *vm)
+{
+        struct amdgpu_vm *avm = (struct amdgpu_vm *)vm; 
+
+        if (WARN_ON(!vm))
+                return;       
+
+        pr_debug("Releasing process vm %p\n", vm);
+
+        /* The original pasid of amdgpu vm has already been
+         * released during making a amdgpu vm to a compute vm
+         * The current pasid is managed by kfd and will be
+         * released on kfd process destroy. Set amdgpu pasid
+         * to 0 to avoid duplicate release.
+         */
+        avm->pasid = 0;
+}
+
 uint32_t amdgpu_amdkfd_gpuvm_get_process_page_dir(void *vm)
 {
 	struct amdgpu_vm *avm = (struct amdgpu_vm *)vm;
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
index 571edd8..a716422 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
@@ -320,8 +320,10 @@ static void kfd_process_destroy_pdds(struct kfd_process *p)
 		pr_debug("Releasing pdd (topology id %d) for process (pasid %d)\n",
 				pdd->dev->id, p->pasid);
 
-		if (pdd->drm_file)
+		if (pdd->drm_file) {
+			pdd->dev->kfd2kgd->release_process_vm(pdd->vm);
 			fput(pdd->drm_file);
+		}
 		else if (pdd->vm)
 			pdd->dev->kfd2kgd->destroy_process_vm(
 				pdd->dev->kgd, pdd->vm);
diff --git a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
index d040241..8e6d6bf 100644
--- a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
+++ b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
@@ -352,6 +352,7 @@ struct kfd2kgd_calls {
 			void **vm, void **process_info, unsigned int pasid,
 			struct dma_fence **ef);
 	void (*destroy_process_vm)(struct kgd_dev *kgd, void *vm);
+	void (*release_process_vm)(void *vm);
 	uint32_t (*get_process_page_dir)(void *vm);
 	void (*set_vm_context_page_table_base)(struct kgd_dev *kgd,
 			uint32_t vmid, uint32_t page_table_base);
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx