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,
