[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 at gmail.com>; amd-gfx at lists.freedesktop.org
Cc: Zeng, Oak <Oak.Zeng at 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 at 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);