On 5/20/26 17:17, Mikhail Gavrilov wrote:
> amdgpu_vm_lock_by_pasid() looks up a VM by PASID and reserves its root
> PD with a bare amdgpu_bo_reserve(), returning the still-reserved root to
> the caller. A caller that then needs to reserve further BOs (for example
> the devcoredump IB dump) ends up nesting reservation_ww_class_mutex
> acquires without a ww_acquire_ctx, which lockdep flags as recursive
> locking.
>
> Convert the helper to take a drm_exec context and lock the root PD via
> amdgpu_vm_lock_pd() instead. Callers now run it inside a
> drm_exec_until_all_locked() loop and can lock additional BOs in the same
> ww ticket, so there is no nested ww_mutex acquire.
>
> The only existing caller, amdgpu_vm_handle_fault(), is updated
> accordingly. Its is_compute_context path, which previously dropped the
> root reservation around svm_range_restore_pages() and re-took it, now
> finalises the drm_exec context and re-initialises a fresh one; behaviour
> is otherwise unchanged.
>
> No functional change intended for the page-fault path.
>
> Signed-off-by: Mikhail Gavrilov <[email protected]>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 72 ++++++++++++++++++--------
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 3 +-
> 2 files changed, 51 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 9ba9de16a27a..3a22670b733f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2950,14 +2950,22 @@ int amdgpu_vm_ioctl(struct drm_device *dev, void
> *data, struct drm_file *filp)
> }
>
> /**
> - * amdgpu_vm_lock_by_pasid - return an amdgpu_vm and its root bo from a
> pasid, if possible.
> + * amdgpu_vm_lock_by_pasid - look up a VM by PASID and lock its root PD
> * @adev: amdgpu device pointer
> - * @root: root BO of the VM
> + * @root: out: reference to the VM's root BO, dropped by the caller
> * @pasid: PASID of the VM
> - * The caller needs to unreserve and unref the root bo on success.
> + * @exec: drm_exec context to lock the root PD in
> + *
> + * Must be called from within a drm_exec_until_all_locked() loop; the caller
> + * runs drm_exec_retry_on_contention() afterwards and drops the *root
> + * reference once the drm_exec context is finalised.
> + *
> + * Return: the VM on success, or NULL if the PASID has no VM, the VM is being
> + * torn down, or locking the root PD failed.
> */
> struct amdgpu_vm *amdgpu_vm_lock_by_pasid(struct amdgpu_device *adev,
> - struct amdgpu_bo **root, u32 pasid)
> + struct amdgpu_bo **root, u32 pasid,
I think we can drop the root parameter now, the exec reference should be
sufficient.
> + struct drm_exec *exec)
> {
> unsigned long irqflags;
> struct amdgpu_vm *vm;
> @@ -2971,9 +2979,11 @@ struct amdgpu_vm *amdgpu_vm_lock_by_pasid(struct
> amdgpu_device *adev,
> if (!*root)
> return NULL;
>
> - r = amdgpu_bo_reserve(*root, true);
> - if (r)
> - goto error_unref;
> + r = amdgpu_vm_lock_pd(vm, exec, 0);
amdgpu_vm_lock_pd() can't be used here since we can't gurantee that the VM
pointer wouldn't go away.
Just do:
r = drm_exec_lock_obj(exec, root->tbo.base);
> + if (r) {
> + amdgpu_bo_unref(root);
> + return NULL;
> + }
>
> /* Double check that the VM still exists */
> xa_lock_irqsave(&adev->vm_manager.pasids, irqflags);
> @@ -2981,16 +2991,12 @@ struct amdgpu_vm *amdgpu_vm_lock_by_pasid(struct
> amdgpu_device *adev,
> if (vm && vm->root.bo != *root)
> vm = NULL;
> xa_unlock_irqrestore(&adev->vm_manager.pasids, irqflags);
> - if (!vm)
> - goto error_unlock;
> + if (!vm) {
We should cleanup with drm_exec_unlock_obj() here, same as it was before.
> + amdgpu_bo_unref(root);
> + return NULL;
> + }
>
> return vm;
We can drop the extra reference on the root BO before returning the VM now
since the drm_exec object holds one as well.
Apart from that this looks like a really nice cleanup to me.
Thanks,
Christian.
> -error_unlock:
> - amdgpu_bo_unreserve(*root);
> -
> -error_unref:
> - amdgpu_bo_unref(root);
> - return NULL;
> }
>
> /**
> @@ -3013,20 +3019,32 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device
> *adev, u32 pasid,
> {
> bool is_compute_context = false;
> struct amdgpu_bo *root;
> + struct drm_exec exec;
> uint64_t value, flags;
> struct amdgpu_vm *vm;
> int r;
>
> - vm = amdgpu_vm_lock_by_pasid(adev, &root, pasid);
> - if (!vm)
> + drm_exec_init(&exec, 0, 0);
> + drm_exec_until_all_locked(&exec) {
> + vm = amdgpu_vm_lock_by_pasid(adev, &root, pasid, &exec);
> + drm_exec_retry_on_contention(&exec);
> + if (!vm)
> + break;
> + }
> + if (!vm) {
> + drm_exec_fini(&exec);
> return false;
> + }
>
> is_compute_context = vm->is_compute_context;
>
> if (is_compute_context) {
> - /* Unreserve root since svm_range_restore_pages might try to
> reserve it. */
> - /* TODO: rework svm_range_restore_pages so that this isn't
> necessary. */
> - amdgpu_bo_unreserve(root);
> + /* Release the root PD lock since svm_range_restore_pages
> + * might try to take it.
> + * TODO: rework svm_range_restore_pages so that this isn't
> + * necessary.
> + */
> + drm_exec_fini(&exec);
>
> if (!svm_range_restore_pages(adev, pasid, vmid,
> node_id, addr >> PAGE_SHIFT, ts,
> write_fault)) {
> @@ -3036,9 +3054,17 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device
> *adev, u32 pasid,
> amdgpu_bo_unref(&root);
>
> /* Re-acquire the VM lock, could be that the VM was freed in
> between. */
> - vm = amdgpu_vm_lock_by_pasid(adev, &root, pasid);
> - if (!vm)
> + drm_exec_init(&exec, 0, 0);
> + drm_exec_until_all_locked(&exec) {
> + vm = amdgpu_vm_lock_by_pasid(adev, &root, pasid, &exec);
> + drm_exec_retry_on_contention(&exec);
> + if (!vm)
> + break;
> + }
> + if (!vm) {
> + drm_exec_fini(&exec);
> return false;
> + }
> }
>
> addr /= AMDGPU_GPU_PAGE_SIZE;
> @@ -3076,7 +3102,7 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device *adev,
> u32 pasid,
> r = amdgpu_vm_update_pdes(adev, vm, true);
>
> error_unlock:
> - amdgpu_bo_unreserve(root);
> + drm_exec_fini(&exec);
> if (r < 0)
> dev_err(adev->dev, "Can't handle page fault (%d)\n", r);
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index d083d7aab75c..af292c2fc521 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -593,7 +593,8 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device *adev,
> u32 pasid,
> bool write_fault);
>
> struct amdgpu_vm *amdgpu_vm_lock_by_pasid(struct amdgpu_device *adev,
> - struct amdgpu_bo **root, u32 pasid);
> + struct amdgpu_bo **root, u32 pasid,
> + struct drm_exec *exec);
>
> void amdgpu_vm_set_task_info(struct amdgpu_vm *vm);
>
