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

On 7/8/26 17:27, 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().
> 
> v5:
> - Delay DRM PASID allocation until after VM initialization so that the
>   PASID is not published before the VM is fully initialized.
> 
> v4:
> - Allocate DRM PASIDs with fpriv directly.
> - Squash ownership registration and PASID lookup conversion.
> 
> Cc: Alex Deucher <[email protected]>
> Suggested-by: Christian König <[email protected]>
> Signed-off-by: Srinivasan Shanmugam <[email protected]>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 21 ++++++-------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 40 ++++++++++++++++---------
>  2 files changed, 37 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 0c973a3c73fa..43396c44e868 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -1376,11 +1376,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;
> @@ -1465,7 +1465,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);
> @@ -1488,22 +1488,23 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
>  		goto out_suspend;
>  	}
>  
> -	pasid = amdgpu_pasid_alloc(16, NULL);
> -	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);
> +	r = amdgpu_vm_init(adev, &fpriv->vm, fpriv->xcp_id, 0);

Please also remove the now unused pasid parameter from amdgpu_vm_init() if that isn't done in patch #3.

Let me check path #3 as well.

>  	if (r)
>  		goto error_pasid;
>  
> +	pasid = amdgpu_pasid_alloc(16, fpriv);
> +	if (pasid < 0) {
> +		dev_warn(adev->dev, "No more PASIDs available!");
> +		pasid = 0;
> +	}
> +	fpriv->vm.pasid = pasid;
> +
>  	drm_exec_init(&exec, DRM_EXEC_IGNORE_DUPLICATES, 0);
>  	drm_exec_until_all_locked(&exec) {
>  		r = amdgpu_vm_lock_pd(&fpriv->vm, &exec, 0);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 180d694323e8..11822bf832f8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2506,14 +2506,16 @@ amdgpu_vm_get_task_info_vm(struct amdgpu_vm *vm)
>  struct amdgpu_task_info *
>  amdgpu_vm_get_task_info_pasid(struct amdgpu_device *adev, u32 pasid)
>  {
> +	struct amdgpu_fpriv *fpriv;
>  	struct amdgpu_task_info *ti;
>  	struct amdgpu_vm *vm;
>  	unsigned long flags;
>  
> -	xa_lock_irqsave(&adev->vm_manager.pasids, flags);
> -	vm = xa_load(&adev->vm_manager.pasids, pasid);
> +	amdgpu_pasid_lock(&flags);
> +	fpriv = amdgpu_pasid_get_fpriv_locked(pasid);
> +	vm = fpriv ? &fpriv->vm : NULL;
>  	ti = amdgpu_vm_get_task_info_vm(vm);
> -	xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
> +	amdgpu_pasid_unlock(flags);
>  
>  	return ti;
>  }
> @@ -2938,14 +2940,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;
> @@ -2957,11 +2961,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;

Oh! Good catch! At least in theory that is a bug in the existing code.

We probably don't need to fix it in existing code because PASIDs are allocated cyclic, but that is definately better.

Regards,
Christian.

> +	}
> +	amdgpu_pasid_unlock(irqflags);
> +
>  	if (!vm) {
>  		drm_exec_unlock_obj(exec, &root->tbo.base);
>  		amdgpu_bo_unref(&root);
> @@ -3158,12 +3168,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
> @@ -3196,7 +3208,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,