Re: [PATCH v4 2/3] drm/amdgpu: Resolve VM through DRM PASID ownership

["Christian König" <christian.koenig@amd.com>]

On 7/3/26 08:18, Srinivasan Shanmugam wrote:
> Allocate DRM PASIDs with fpriv and resolve VM lookup users through:
> 
> 	PASID -> fpriv -> VM
> 
> This preserves the root BO reference and revalidation flow in
> amdgpu_vm_lock_by_pasid().
> 
> v4:
> - Allocate DRM PASIDs with fpriv directly.
> - Squash ownership registration and PASID lookup conversion.
> 
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Suggested-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 16 +++++-----
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 40 ++++++++++++++++---------
>  2 files changed, 34 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 4610d6889e9b..087ca7783d1b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -1375,11 +1375,11 @@ int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>  
>  		memset(&gpuvm_fault, 0, sizeof(gpuvm_fault));
>  
> -		xa_lock_irqsave(&adev->vm_manager.pasids, flags);
> +		amdgpu_pasid_lock(&flags);
>  		gpuvm_fault.addr = vm->fault_info.addr;
>  		gpuvm_fault.status = vm->fault_info.status;
>  		gpuvm_fault.vmhub = vm->fault_info.vmhub;
> -		xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
> +		amdgpu_pasid_unlock(flags);
>  
>  		return copy_to_user(out, &gpuvm_fault,
>  				    min((size_t)size, sizeof(gpuvm_fault))) ? -EFAULT : 0;
> @@ -1464,7 +1464,7 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
>  	struct amdgpu_device *adev = drm_to_adev(dev);
>  	struct amdgpu_fpriv *fpriv;
>  	struct drm_exec exec;
> -	int r, pasid;
> +	int r, pasid = 0;
>  
>  	/* Ensure IB tests are run on ring */
>  	flush_delayed_work(&adev->delayed_init_work);
> @@ -1487,16 +1487,16 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
>  		goto out_suspend;
>  	}
>  
> -	pasid = amdgpu_pasid_alloc(16, NULL);
> +	r = amdgpu_xcp_open_device(adev, fpriv, file_priv);
> +	if (r)
> +		goto error_pasid;
> +
> +	pasid = amdgpu_pasid_alloc(16, fpriv);

That needs to come even later, e.g. after the VM is initialized.

Otherwise we would run into a bunch of trouble should there be any stale entries for this PASID in the interrupt ring buffers.

Apart from that looks good to me,
Christian.

>  	if (pasid < 0) {
>  		dev_warn(adev->dev, "No more PASIDs available!");
>  		pasid = 0;
>  	}
>  
> -	r = amdgpu_xcp_open_device(adev, fpriv, file_priv);
> -	if (r)
> -		goto error_pasid;
> -
>  	amdgpu_debugfs_vm_init(file_priv);
>  
>  	r = amdgpu_vm_init(adev, &fpriv->vm, fpriv->xcp_id, pasid);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 32719f31b6c9..9092ff227a55 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2463,12 +2463,14 @@ static void amdgpu_vm_destroy_task_info(struct kref *kref)
>  static inline struct amdgpu_vm *
>  amdgpu_vm_get_vm_from_pasid(struct amdgpu_device *adev, u32 pasid)
>  {
> +	struct amdgpu_fpriv *fpriv;
>  	struct amdgpu_vm *vm;
>  	unsigned long flags;
>  
> -	xa_lock_irqsave(&adev->vm_manager.pasids, flags);
> -	vm = xa_load(&adev->vm_manager.pasids, pasid);
> -	xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
> +	amdgpu_pasid_lock(&flags);
> +	fpriv = amdgpu_pasid_get_fpriv_locked(pasid);
> +	vm = fpriv ? &fpriv->vm : NULL;
> +	amdgpu_pasid_unlock(flags);
>  
>  	return vm;
>  }
> @@ -2943,14 +2945,16 @@ struct amdgpu_vm *amdgpu_vm_lock_by_pasid(struct amdgpu_device *adev,
>  					  u32 pasid, struct drm_exec *exec)
>  {
>  	unsigned long irqflags;
> +	struct amdgpu_fpriv *fpriv;
>  	struct amdgpu_bo *root;
>  	struct amdgpu_vm *vm;
>  	int r;
>  
> -	xa_lock_irqsave(&adev->vm_manager.pasids, irqflags);
> -	vm = xa_load(&adev->vm_manager.pasids, pasid);
> -	root = vm ? amdgpu_bo_ref(vm->root.bo) : NULL;
> -	xa_unlock_irqrestore(&adev->vm_manager.pasids, irqflags);
> +	amdgpu_pasid_lock(&irqflags);
> +	fpriv = amdgpu_pasid_get_fpriv_locked(pasid);
> +	vm = fpriv ? &fpriv->vm : NULL;
> +	root = vm && vm->root.bo ? amdgpu_bo_ref(vm->root.bo) : NULL;
> +	amdgpu_pasid_unlock(irqflags);
>  
>  	if (!root)
>  		return NULL;
> @@ -2962,11 +2966,17 @@ struct amdgpu_vm *amdgpu_vm_lock_by_pasid(struct amdgpu_device *adev,
>  	}
>  
>  	/* Double check that the VM still exists */
> -	xa_lock_irqsave(&adev->vm_manager.pasids, irqflags);
> -	vm = xa_load(&adev->vm_manager.pasids, pasid);
> -	if (vm && vm->root.bo != root)
> +	amdgpu_pasid_lock(&irqflags);
> +	fpriv = amdgpu_pasid_get_fpriv_locked(pasid);
> +	if (!fpriv) {
>  		vm = NULL;
> -	xa_unlock_irqrestore(&adev->vm_manager.pasids, irqflags);
> +	} else {
> +		vm = &fpriv->vm;
> +		if (vm->root.bo != root)
> +			vm = NULL;
> +	}
> +	amdgpu_pasid_unlock(irqflags);
> +
>  	if (!vm) {
>  		drm_exec_unlock_obj(exec, &root->tbo.base);
>  		amdgpu_bo_unref(&root);
> @@ -3163,12 +3173,14 @@ void amdgpu_vm_update_fault_cache(struct amdgpu_device *adev,
>  				  uint32_t status,
>  				  unsigned int vmhub)
>  {
> +	struct amdgpu_fpriv *fpriv;
>  	struct amdgpu_vm *vm;
>  	unsigned long flags;
>  
> -	xa_lock_irqsave(&adev->vm_manager.pasids, flags);
> +	amdgpu_pasid_lock(&flags);
>  
> -	vm = xa_load(&adev->vm_manager.pasids, pasid);
> +	fpriv = amdgpu_pasid_get_fpriv_locked(pasid);
> +	vm = fpriv ? &fpriv->vm : NULL;
>  	/* Don't update the fault cache if status is 0.  In the multiple
>  	 * fault case, subsequent faults will return a 0 status which is
>  	 * useless for userspace and replaces the useful fault status, so
> @@ -3201,7 +3213,7 @@ void amdgpu_vm_update_fault_cache(struct amdgpu_device *adev,
>  			WARN_ONCE(1, "Invalid vmhub %u\n", vmhub);
>  		}
>  	}
> -	xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
> +	amdgpu_pasid_unlock(flags);
>  }
>  
>  void amdgpu_vm_print_task_info(struct amdgpu_device *adev,