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 <[email protected]>
> Suggested-by: Christian König <[email protected]>
> Signed-off-by: Srinivasan Shanmugam <[email protected]>
> ---
> 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,
