[PATCH 1/2] drm/amdgpu: free pasid early before converting a vm

[Felix Kuehling]

On 2021-06-23 8:25 a.m., Nirmoy Das wrote:
> VM code should not be responsible for freeing pasid as pasid
> gets allocated outside of VM code, before initializing a vm.
>
> Signed-off-by: Nirmoy Das <nirmoy.das at amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 6 ++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c           | 5 -----
>   2 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index f96598279593..42e22b1fdfee 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -1287,6 +1287,12 @@ int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct kgd_dev *kgd,
>   	if (avm->process_info)
>   		return -EINVAL;
>   
> +	/* Free the original amdgpu allocated pasid,
> +	 * will be replaced with kfd allocated pasid
> +	 */
> +	if (avm->pasid)
> +		amdgpu_pasid_free(avm->pasid);
> +

If amdgpu_vm_make_compute fails, amdgpu_driver_postclose_kms will try to 
free the PASID a second time because vm->pasid still points to it. You 
could fix that by moving the amdgpu_pasid_free after a successful 
amdgpu_vm_make_compute call. But that still means you're making an 
assumption about the side effect of amdgpu_vm_make_compute on vm->pasid. 
IMHO, splitting the amdgpu_pasid_free and the vm->pasid = 0 into two 
different functions is a mistake.

Regards,
   Felix


>   	/* Convert VM into a compute VM */
>   	ret = amdgpu_vm_make_compute(adev, avm, pasid);
>   	if (ret)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 63975bda8e76..be841d62a1d4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -3094,11 +3094,6 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   		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);
> -
> -		/* Free the original amdgpu allocated pasid
> -		 * Will be replaced with kfd allocated pasid
> -		 */
> -		amdgpu_pasid_free(vm->pasid);
>   		vm->pasid = 0;
>   	}
>