[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 at 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 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  | 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);
>