On 5/19/26 18:15, Mikhail Gavrilov wrote:

When dumping IB contents from a hung job, amdgpu_devcoredump_format() acquires the VM root PD's reservation lock via amdgpu_vm_lock_by_pasid() and then, for each IB referenced by the job, calls amdgpu_bo_reserve() on the BO that backs the IB. Both reservations are taken on reservation_ww_class_mutex objects but neither uses a ww_acquire_ctx, which trips lockdep:

WARNING: possible recursive locking detected

kworker/u128:0 is trying to acquire lock: ffff88838b16e1f0 (reservation_ww_class_mutex){+.+.}-{4:4}, at: amdgpu_devcoredump_format+0x1594/0x23f0 [amdgpu]

but task is already holding lock: ffff8882f82681f0 (reservation_ww_class_mutex){+.+.}-{4:4}, at: amdgpu_devcoredump_format+0x1594/0x23f0 [amdgpu]

Possible unsafe locking scenario: CPU0 ---- lock(reservation_ww_class_mutex); lock(reservation_ww_class_mutex);

*** DEADLOCK *** May be due to missing lock nesting notation

Workqueue: events_unbound amdgpu_devcoredump_deferred_work [amdgpu] Call Trace: __ww_mutex_lock.constprop.0 ww_mutex_lock amdgpu_bo_reserve amdgpu_devcoredump_format+0x1594 [amdgpu] amdgpu_devcoredump_deferred_work+0xea [amdgpu]

The two reservations are on different BOs in the captured trace, so the splat is a lockdep-correctness warning, not an observed deadlock. It becomes a real self-deadlock whenever the IB BO shares its dma_resv with the root PD (the always-valid case, see amdgpu_vm_is_bo_always_valid()): amdgpu_bo_reserve(abo) re-acquires the same ww_mutex without a ticket and blocks forever.

Fix it in two steps:

1. Collect per-IB BO references under the root PD's reservation, then release the root before locking the IB BOs. The walk over the VM mapping tree must remain under the root lock (mappings can be torn down without it), but the actual content copies do not.

2. Lock all the IB BOs together using drm_exec(9) with a single ww_acquire_ctx. DRM_EXEC_IGNORE_DUPLICATES handles the case where IB BOs share a dma_resv (e.g. always-valid BOs). Each lock attempt is now a top-level acquire under one ticket, with retry-on- contention handled by drm_exec; the recursive ww_mutex condition is gone.

The collect/lock/release logic is factored out into three small helpers (amdgpu_devcoredump_{collect,lock,release}_ib_refs) to keep the main function readable and within the kernel coding style indentation guideline.

This also fixes a BO refcount leak in the original code: when amdgpu_bo_reserve() failed, control jumped to free_ib_content without running amdgpu_bo_unref(). In the new structure the per-IB BO refs are released unconditionally in the cleanup helper.

Reproducer (~150 LoC libdrm_amdgpu): submit a single GFX IB containing PACKET3_INDIRECT_BUFFER chained at GPU VA 0 and wait for the fence. The TDR fires within ~10 s and the deferred coredump worker produces the splat above on every invocation.

v2: switch from per-IB amdgpu_bo_reserve() to drm_exec for the IB BO locking as suggested by Christian König; the snapshot approach for collecting BO references under the root PD's reservation is retained.

Fixes: 7b15fc2d1f1a ("drm/amdgpu: dump job ibs in the devcoredump") Suggested-by: Christian König christian.koenig@amd.com Signed-off-by: Mikhail Gavrilov mikhail.v.gavrilov@gmail.com

---

.../gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c | 187 ++++++++++++++---- 1 file changed, 148 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c index d386bc775d03..9ac958cf09fd 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c @@ -24,6 +24,7 @@ #include <generated/utsrelease.h> #include <linux/devcoredump.h> +#include <drm/drm_exec.h> #include "amdgpu_dev_coredump.h" #include "atom.h" @@ -207,6 +208,108 @@ static void amdgpu_devcoredump_fw_info(struct amdgpu_device *adev, } } +struct amdgpu_devcoredump_ib_ref {

• struct amdgpu_bo *bo;
• u64 offset;

+};

+/*

• • Walk the VM's mapping tree under the root PD's reservation to obtain the BO
• • that backs each IB and pin it with a refcount. The root PD reservation is
• • dropped before this function returns; the caller can then lock each IB BO
• • via drm_exec without nesting reservations on reservation_ww_class_mutex.
• • Returns an array of num_ibs entries (each ib_refs[i].bo may be NULL if its
• • mapping was not found), or NULL on allocation failure / VM lookup failure.
• • The caller must release the BO refs and free the array via
• • amdgpu_devcoredump_release_ib_refs().
• */

+static struct amdgpu_devcoredump_ib_ref * +amdgpu_devcoredump_collect_ib_refs(struct amdgpu_device *adev,

• 		   struct amdgpu_coredump_info *coredump)
  

+{

• struct amdgpu_devcoredump_ib_ref *ib_refs;
• struct amdgpu_bo_va_mapping *mapping;
• struct amdgpu_bo *root;
• struct amdgpu_vm *vm;
• u64 va_start;
• ib_refs = kcalloc(coredump->num_ibs, sizeof(*ib_refs), GFP_KERNEL);
• if (!ib_refs)

• return NULL;
  

• vm = amdgpu_vm_lock_by_pasid(adev, &root, coredump->pasid);
• if (!vm) {

• kfree(ib_refs);
  

• return NULL;
  

• }
• for (int i = 0; i < coredump->num_ibs; i++) {

• va_start = coredump->ibs[i].gpu_addr & AMDGPU_GMC_HOLE_MASK;
  

• mapping = amdgpu_vm_bo_lookup_mapping(vm, va_start / AMDGPU_GPU_PAGE_SIZE);
  

• if (!mapping)
  

• 	continue;
  

• ib_refs[i].bo = amdgpu_bo_ref(mapping->bo_va->base.bo);
  

• ib_refs[i].offset = va_start -
  

• 		    mapping->start * AMDGPU_GPU_PAGE_SIZE;
  

• }
• amdgpu_bo_unreserve(root);
• amdgpu_bo_unref(&root);
• return ib_refs;

+}

That whole infrastructure is superflous. You just need to modify amdgpu_vm_lock_by_pasid() to take a drm_exec object to lock the root BO.

Regards, Christian.

+static void +amdgpu_devcoredump_release_ib_refs(struct amdgpu_devcoredump_ib_ref *ib_refs,

• 		   int num_ibs)
  

+{

• if (!ib_refs)

• return;
  

• for (int i = 0; i < num_ibs; i++)

• if (ib_refs[i].bo)
  

• 	amdgpu_bo_unref(&ib_refs[i].bo);
  

• kfree(ib_refs);

+}

+/*

• • Lock all collected IB BOs together using a single drm_exec ticket. This
• • eliminates the nested ww_mutex acquire that lockdep flags as recursive
• • locking (and that becomes a real self-deadlock for IB BOs sharing their
• • dma_resv with the root PD).
• • Returns 0 if drm_exec was initialised and the BOs are locked; the caller
• • must call drm_exec_fini() on success. Returns non-zero on failure, in which
• • case drm_exec is already torn down.
• */

+static int +amdgpu_devcoredump_lock_ib_refs(struct drm_exec *exec,

• 		struct amdgpu_devcoredump_ib_ref *ib_refs,
  

• 		int num_ibs)
  

+{

• int r = 0;
• drm_exec_init(exec, DRM_EXEC_IGNORE_DUPLICATES, num_ibs);
• drm_exec_until_all_locked(exec) {

• r = 0;
  

• for (int i = 0; i < num_ibs; i++) {
  

• 	if (!ib_refs[i].bo)
  

• 		continue;
  

• 	r = drm_exec_lock_obj(exec, &ib_refs[i].bo->tbo.base);
  

• 	drm_exec_retry_on_contention(exec);
  

• 	if (r)
  

• 		break;
  

• }
  

• if (r)
  

• 	break;
  

• }
• if (r)

• drm_exec_fini(exec);
  

• return r;

+}

static ssize_t amdgpu_devcoredump_format(char *buffer, size_t count, struct amdgpu_coredump_info *coredump) { @@ -214,13 +317,9 @@ amdgpu_devcoredump_format(char *buffer, size_t count, struct amdgpu_coredump_inf struct drm_printer p; struct drm_print_iterator iter; struct amdgpu_vm_fault_info *fault_info;

• struct amdgpu_bo_va_mapping *mapping; struct amdgpu_ip_block *ip_block; struct amdgpu_res_cursor cursor;
• struct amdgpu_bo *abo, *root;
• uint64_t va_start, offset; struct amdgpu_ring *ring;
• struct amdgpu_vm *vm; u32 *ib_content; uint8_t *kptr; int ver, i, j, r;

@@ -343,43 +442,52 @@ amdgpu_devcoredump_format(char *buffer, size_t count, struct amdgpu_coredump_inf drm_printf(&p, "VRAM is lost due to GPU reset!\n"); if (coredump->num_ibs) {

• /* Don't try to lookup the VM or map the BOs when calculating the
  

•  * size required to store the devcoredump.
  

• struct amdgpu_devcoredump_ib_ref *ib_refs = NULL;
  

• struct drm_exec exec;
  

• bool ibs_locked = false;
  

• /*
  

•  * Collect the BO that backs each IB under the root PD's
  

•  * reservation, drop the root reservation, then lock all the
  

•  * IB BOs together in one drm_exec ticket. This avoids nesting
  

•  * amdgpu_bo_reserve() inside the root PD's reservation, which
  

•  * would be a recursive reservation_ww_class_mutex acquire
  

•  * without a ww_acquire_ctx (lockdep splat, and a real
  

•  * self-deadlock for always-valid BOs that share their dma_resv
  

•  * with the root PD).
  

•  *
  

•  * Skip lookup/locking entirely on the sizing pass: it does not
  

•  * write IB content, and the size estimate doesn't depend on
  

•  * whether the BOs are reachable.
  

  */

• if (sizing_pass)
  

• 	vm = NULL;
  

• else
  

• 	vm = amdgpu_vm_lock_by_pasid(adev, &root, coredump->pasid);
  

• if (!sizing_pass) {
  

• 	ib_refs = amdgpu_devcoredump_collect_ib_refs(adev, coredump);
  

• 	if (ib_refs) {
  

• 		r = amdgpu_devcoredump_lock_ib_refs(&exec, ib_refs,
  

• 						    coredump->num_ibs);
  

• 		if (!r)
  

• 			ibs_locked = true;
  

• 	}
  

• }
  

• for (int i = 0; i < coredump->num_ibs; i++) {
  

• 	struct amdgpu_bo *abo = ibs_locked ? ib_refs[i].bo : NULL;
  

• 	u64 offset = ibs_locked ? ib_refs[i].offset : 0;
  

• 	bool emit_content = sizing_pass;
  

• for (int i = 0; i < coredump->num_ibs && (sizing_pass || vm); i++) {
  ib_content = kvmalloc_array(coredump->ibs[i].ib_size_dw, 4,
  			    GFP_KERNEL);
  if (!ib_content)
  	continue;
  

• 	/* vm=NULL can only happen when 'sizing_pass' is true. Skip to the
  

• 	 * drm_printf() calls (ib_content doesn't need to be initialized
  

• 	 * as its content won't be written anywhere).
  

• 	 */
  

• 	if (!vm)
  

• 	if (!abo)
  	goto output_ib_content;
  

• 	va_start = coredump->ibs[i].gpu_addr & AMDGPU_GMC_HOLE_MASK;
  

• 	mapping = amdgpu_vm_bo_lookup_mapping(vm, va_start / AMDGPU_GPU_PAGE_SIZE);
  

• 	if (!mapping)
  

• 		goto free_ib_content;
  

• 	offset = va_start - (mapping->start * AMDGPU_GPU_PAGE_SIZE);
  

• 	abo = amdgpu_bo_ref(mapping->bo_va->base.bo);
  

• 	r = amdgpu_bo_reserve(abo, false);
  

• 	if (r)
  

• 		goto free_ib_content;
  

• if (abo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS) {
  	off = 0;
  

if (abo->tbo.resource->mem_type != TTM_PL_VRAM)

• 			goto unreserve_abo;
  

• 			goto output_ib_content;
  

amdgpu_res_first(abo->tbo.resource, offset, coredump->ibs[i].ib_size_dw * 4, @@ -391,12 +499,13 @@ amdgpu_devcoredump_format(char *buffer, size_t count, struct amdgpu_coredump_inf off += cursor.size; amdgpu_res_next(&cursor, cursor.size); }

• 		emit_content = true;
  } else {
  	r = ttm_bo_kmap(&abo->tbo, 0,
  			PFN_UP(abo->tbo.base.size),
  			&abo->kmap);
  	if (r)
  

• 			goto unreserve_abo;
  

• 			goto output_ib_content;
  

kptr = amdgpu_bo_kptr(abo); kptr += offset; @@ -404,23 +513,23 @@ amdgpu_devcoredump_format(char *buffer, size_t count, struct amdgpu_coredump_inf coredump->ibs[i].ib_size_dw * 4); amdgpu_bo_kunmap(abo);

• 		emit_content = true;
  }
  

output_ib_content: drm_printf(&p, "\nIB #%d 0x%llx %d dw\n", i, coredump->ibs[i].gpu_addr, coredump->ibs[i].ib_size_dw);

• 	for (int j = 0; j < coredump->ibs[i].ib_size_dw; j++)
  

• 		drm_printf(&p, "0x%08x\n", ib_content[j]);
  

-unreserve_abo:

• 	if (vm)
  

• 		amdgpu_bo_unreserve(abo);
  

-free_ib_content:

• 	if (emit_content) {
  

• 		for (int j = 0; j < coredump->ibs[i].ib_size_dw; j++)
  

• 			drm_printf(&p, "0x%08x\n", ib_content[j]);
  

• 	}
  kvfree(ib_content);
  

  }

• if (vm) {
  

• 	amdgpu_bo_unreserve(root);
  

• 	amdgpu_bo_unref(&root);
  

• }
  

• if (ibs_locked)
  

• 	drm_exec_fini(&exec);
  

• amdgpu_devcoredump_release_ib_refs(ib_refs, coredump->num_ibs);
  

  }

return count - iter.remain;

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 mikhail.v.gavrilov@gmail.com --- 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, + 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); + 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) { + amdgpu_bo_unref(root); + return NULL; + }

return vm; -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);

When dumping IB contents from a hung job, amdgpu_devcoredump_format() acquired the VM root PD's reservation via amdgpu_vm_lock_by_pasid() and then, for each IB, called amdgpu_bo_reserve() on the BO backing the IB. Both reservations are reservation_ww_class_mutex objects and neither used a ww_acquire_ctx, which trips lockdep:

WARNING: possible recursive locking detected -------------------------------------------- kworker/u128:0 is trying to acquire lock: ffff88838b16e1f0 (reservation_ww_class_mutex){+.+.}-{4:4}, at: amdgpu_devcoredump_format+0x1594/0x23f0 [amdgpu]

but task is already holding lock: ffff8882f82681f0 (reservation_ww_class_mutex){+.+.}-{4:4}, at: amdgpu_devcoredump_format+0x1594/0x23f0 [amdgpu]

Possible unsafe locking scenario: CPU0 ---- lock(reservation_ww_class_mutex); lock(reservation_ww_class_mutex);

*** DEADLOCK *** May be due to missing lock nesting notation

Workqueue: events_unbound amdgpu_devcoredump_deferred_work [amdgpu] Call Trace: __ww_mutex_lock.constprop.0 ww_mutex_lock amdgpu_bo_reserve amdgpu_devcoredump_format+0x1594 [amdgpu] amdgpu_devcoredump_deferred_work+0xea [amdgpu]

The two reservations are on different BOs in the captured trace, so the splat is a lockdep-correctness warning, not an observed deadlock. It becomes a real self-deadlock whenever the IB BO shares its dma_resv with the root PD (the always-valid case, see amdgpu_vm_is_bo_always_valid()): amdgpu_bo_reserve(abo) re-acquires the same ww_mutex without a ticket and blocks forever.

With amdgpu.gpu_recovery=0 the timeout handler refires every ~2 s and each invocation produces this splat, drowning the kernel ring buffer.

Now that amdgpu_vm_lock_by_pasid() takes a drm_exec context, lock the root PD and every IB BO together in a single drm_exec ticket. DRM_EXEC_IGNORE_DUPLICATES handles IB BOs that share a dma_resv (e.g. always-valid BOs, or two IBs backed by the same BO). Every lock is now a top-level acquire under one ww_acquire_ctx, so the recursive ww_mutex condition is gone, and the per-IB amdgpu_bo_reserve()/amdgpu_bo_unref() dance -- including a BO refcount leak on the amdgpu_bo_reserve() failure path -- is removed.

Reproducer (~150 LoC libdrm_amdgpu): submit a single GFX IB containing PACKET3_INDIRECT_BUFFER chained at GPU VA 0 and wait for the fence. The TDR fires within ~10 s and the deferred coredump worker produces the splat above on every invocation; with this change applied the splat is gone.

Fixes: 7b15fc2d1f1a ("drm/amdgpu: dump job ibs in the devcoredump") Suggested-by: Christian König christian.koenig@amd.com Signed-off-by: Mikhail Gavrilov mikhail.v.gavrilov@gmail.com --- .../gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c | 103 ++++++++++++------ 1 file changed, 71 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c index d386bc775d03..a9d8e03fad83 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c @@ -24,6 +24,7 @@

#include <generated/utsrelease.h> #include <linux/devcoredump.h> +#include <drm/drm_exec.h> #include "amdgpu_dev_coredump.h" #include "atom.h"

@@ -214,13 +215,9 @@ amdgpu_devcoredump_format(char *buffer, size_t count, struct amdgpu_coredump_inf struct drm_printer p; struct drm_print_iterator iter; struct amdgpu_vm_fault_info *fault_info; - struct amdgpu_bo_va_mapping *mapping; struct amdgpu_ip_block *ip_block; struct amdgpu_res_cursor cursor; - struct amdgpu_bo *abo, *root; - uint64_t va_start, offset; struct amdgpu_ring *ring; - struct amdgpu_vm *vm; u32 *ib_content; uint8_t *kptr; int ver, i, j, r; @@ -343,43 +340,84 @@ amdgpu_devcoredump_format(char *buffer, size_t count, struct amdgpu_coredump_inf drm_printf(&p, "VRAM is lost due to GPU reset!\n");

if (coredump->num_ibs) { - /* Don't try to lookup the VM or map the BOs when calculating the - * size required to store the devcoredump. + struct amdgpu_bo_va_mapping *mapping; + struct amdgpu_bo *root, *abo; + struct drm_exec exec; + struct amdgpu_vm *vm; + u64 va_start, offset; + bool locked = false; + + /* + * Lock the VM root PD and every IB BO together in a single + * drm_exec ticket. Reserving the IB BOs one by one while the + * root PD is held would be a recursive reservation_ww_class_mutex + * acquire without a ww_acquire_ctx, which trips lockdep and + * self-deadlocks for IB BOs that share their dma_resv with the + * root PD (always-valid BOs). + * + * Skip locking entirely on the sizing pass: it does not write + * IB content, so the size estimate doesn't depend on whether + * the BOs are reachable. */ - if (sizing_pass) - vm = NULL; - else - vm = amdgpu_vm_lock_by_pasid(adev, &root, coredump->pasid); + if (!sizing_pass) { + drm_exec_init(&exec, DRM_EXEC_IGNORE_DUPLICATES, + 1 + coredump->num_ibs); + drm_exec_until_all_locked(&exec) { + vm = amdgpu_vm_lock_by_pasid(adev, &root, + coredump->pasid, &exec); + drm_exec_retry_on_contention(&exec); + if (!vm) + break; + + for (int i = 0; i < coredump->num_ibs; i++) { + u64 pfn; + + va_start = coredump->ibs[i].gpu_addr & + AMDGPU_GMC_HOLE_MASK; + pfn = va_start / AMDGPU_GPU_PAGE_SIZE; + mapping = amdgpu_vm_bo_lookup_mapping(vm, pfn); + if (!mapping) + continue; + + abo = mapping->bo_va->base.bo; + r = drm_exec_lock_obj(&exec, &abo->tbo.base); + drm_exec_retry_on_contention(&exec); + if (r) + break; + } + if (r) + break; + } + if (vm && !r) + locked = true; + else + drm_exec_fini(&exec); + } + + for (int i = 0; i < coredump->num_ibs; i++) { + bool emit_content = sizing_pass;

- for (int i = 0; i < coredump->num_ibs && (sizing_pass || vm); i++) { ib_content = kvmalloc_array(coredump->ibs[i].ib_size_dw, 4, GFP_KERNEL); if (!ib_content) continue;

- /* vm=NULL can only happen when 'sizing_pass' is true. Skip to the - * drm_printf() calls (ib_content doesn't need to be initialized - * as its content won't be written anywhere). - */ - if (!vm) + if (!locked) goto output_ib_content;

va_start = coredump->ibs[i].gpu_addr & AMDGPU_GMC_HOLE_MASK; mapping = amdgpu_vm_bo_lookup_mapping(vm, va_start / AMDGPU_GPU_PAGE_SIZE); if (!mapping) - goto free_ib_content; + goto output_ib_content;

- offset = va_start - (mapping->start * AMDGPU_GPU_PAGE_SIZE); - abo = amdgpu_bo_ref(mapping->bo_va->base.bo); - r = amdgpu_bo_reserve(abo, false); - if (r) - goto free_ib_content; + abo = mapping->bo_va->base.bo; + offset = va_start - mapping->start * AMDGPU_GPU_PAGE_SIZE;

if (abo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS) { off = 0;

if (abo->tbo.resource->mem_type != TTM_PL_VRAM) - goto unreserve_abo; + goto output_ib_content;

amdgpu_res_first(abo->tbo.resource, offset, coredump->ibs[i].ib_size_dw * 4, @@ -391,12 +429,13 @@ amdgpu_devcoredump_format(char *buffer, size_t count, struct amdgpu_coredump_inf off += cursor.size; amdgpu_res_next(&cursor, cursor.size); } + emit_content = true; } else { r = ttm_bo_kmap(&abo->tbo, 0, PFN_UP(abo->tbo.base.size), &abo->kmap); if (r) - goto unreserve_abo; + goto output_ib_content;

kptr = amdgpu_bo_kptr(abo); kptr += offset; @@ -404,21 +443,21 @@ amdgpu_devcoredump_format(char *buffer, size_t count, struct amdgpu_coredump_inf coredump->ibs[i].ib_size_dw * 4);

amdgpu_bo_kunmap(abo); + emit_content = true; }

output_ib_content: drm_printf(&p, "\nIB #%d 0x%llx %d dw\n", i, coredump->ibs[i].gpu_addr, coredump->ibs[i].ib_size_dw); - for (int j = 0; j < coredump->ibs[i].ib_size_dw; j++) - drm_printf(&p, "0x%08x\n", ib_content[j]); -unreserve_abo: - if (vm) - amdgpu_bo_unreserve(abo); -free_ib_content: + if (emit_content) { + for (int j = 0; j < coredump->ibs[i].ib_size_dw; j++) + drm_printf(&p, "0x%08x\n", ib_content[j]); + } kvfree(ib_content); } - if (vm) { - amdgpu_bo_unreserve(root); + + if (locked) { + drm_exec_fini(&exec); amdgpu_bo_unref(&root); } }

On 5/21/26 12:43, 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 with drm_exec_lock_obj(). 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 drm_exec context holds its own reference on the locked root BO, so the helper no longer hands a root reference back to the caller: the root output parameter is dropped, and the transient reference taken across the PASID lookup is released before returning.

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 mikhail.v.gavrilov@gmail.com

drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 91 ++++++++++++++++---------- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 2 +- 2 files changed, 58 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index 9ba9de16a27a..591980907211 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -2950,47 +2950,56 @@ 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
  • @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. The drm_exec context holds
• • a reference on the root BO until it 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)
  

• 			  u32 pasid, struct drm_exec *exec)
  

{ unsigned long irqflags;

• 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;

• root = vm ? amdgpu_bo_ref(vm->root.bo) : NULL; xa_unlock_irqrestore(&adev->vm_manager.pasids, irqflags);

• if (!*root)

• if (!root) return NULL;

• r = amdgpu_bo_reserve(*root, true);
• if (r)

• goto error_unref;
  

• 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); vm = xa_load(&adev->vm_manager.pasids, pasid);

• if (vm && vm->root.bo != *root)

• if (vm && vm->root.bo != root) vm = NULL; xa_unlock_irqrestore(&adev->vm_manager.pasids, irqflags);

• if (!vm)

• goto error_unlock;
  

• if (!vm) {

• drm_exec_unlock_obj(exec, &root->tbo.base);
  

• amdgpu_bo_unref(&root);
  

• return NULL;
  

• }

• return vm;

-error_unlock:

• amdgpu_bo_unreserve(*root);

• /* The drm_exec context holds its own reference on the root BO. */
• amdgpu_bo_unref(&root);

-error_unref:

• amdgpu_bo_unref(root);
• return NULL;

• return vm;

} /** @@ -3012,33 +3021,49 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, u32 pasid, uint64_t ts, bool write_fault) { 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);

Make the last parameter 1 here since we are expecting to lock 1 object.

Not a must have, it will work without but it is just a little bit more optimal.

Apart from that Reviewed-by: Christian König christian.koenig@amd.com.

Thanks, Christian.

• drm_exec_until_all_locked(&exec) {

• vm = amdgpu_vm_lock_by_pasid(adev, 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)) {
  

• 	amdgpu_bo_unref(&root);
  

• 			     node_id, addr >> PAGE_SHIFT, ts, write_fault))
  return true;
  

• }
  

• 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, pasid, &exec);
  

• 	drm_exec_retry_on_contention(&exec);
  

• 	if (!vm)
  

• 		break;
  

• }
  

• if (!vm) {
  

• 	drm_exec_fini(&exec);
  return false;
  

• }
  

  }

addr /= AMDGPU_GPU_PAGE_SIZE; @@ -3062,7 +3087,7 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, u32 pasid, value = 0; }

• r = dma_resv_reserve_fences(root->tbo.base.resv, 1);

• r = dma_resv_reserve_fences(vm->root.bo->tbo.base.resv, 1); if (r) { pr_debug("failed %d to reserve fence slot\n", r); goto error_unlock;

@@ -3076,12 +3101,10 @@ 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);

• amdgpu_bo_unref(&root);
• return false;

} diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h index d083d7aab75c..0c6e3e0368c7 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h @@ -593,7 +593,7 @@ 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);
  

• 			  u32 pasid, struct drm_exec *exec);
  

void amdgpu_vm_set_task_info(struct amdgpu_vm *vm);

When dumping IB contents from a hung job, amdgpu_devcoredump_format() acquired the VM root PD's reservation via amdgpu_vm_lock_by_pasid() and then, for each IB, called amdgpu_bo_reserve() on the BO backing the IB. Both reservations are reservation_ww_class_mutex objects and neither used a ww_acquire_ctx, which trips lockdep:

WARNING: possible recursive locking detected -------------------------------------------- kworker/u128:0 is trying to acquire lock: ffff88838b16e1f0 (reservation_ww_class_mutex){+.+.}-{4:4}, at: amdgpu_devcoredump_format+0x1594/0x23f0 [amdgpu]

but task is already holding lock: ffff8882f82681f0 (reservation_ww_class_mutex){+.+.}-{4:4}, at: amdgpu_devcoredump_format+0x1594/0x23f0 [amdgpu]

Possible unsafe locking scenario: CPU0 ---- lock(reservation_ww_class_mutex); lock(reservation_ww_class_mutex);

*** DEADLOCK *** May be due to missing lock nesting notation

Workqueue: events_unbound amdgpu_devcoredump_deferred_work [amdgpu] Call Trace: __ww_mutex_lock.constprop.0 ww_mutex_lock amdgpu_bo_reserve amdgpu_devcoredump_format+0x1594 [amdgpu] amdgpu_devcoredump_deferred_work+0xea [amdgpu]

The two reservations are on different BOs in the captured trace, so the splat is a lockdep-correctness warning, not an observed deadlock. It becomes a real self-deadlock whenever the IB BO shares its dma_resv with the root PD (the always-valid case, see amdgpu_vm_is_bo_always_valid()): amdgpu_bo_reserve(abo) re-acquires the same ww_mutex without a ticket and blocks forever.

With amdgpu.gpu_recovery=0 the timeout handler refires every ~2 s and each invocation produces this splat, drowning the kernel ring buffer.

Now that amdgpu_vm_lock_by_pasid() takes a drm_exec context, lock the root PD and every IB BO together in a single drm_exec ticket. DRM_EXEC_IGNORE_DUPLICATES handles IB BOs that share a dma_resv (e.g. always-valid BOs, or two IBs backed by the same BO). Every lock is now a top-level acquire under one ww_acquire_ctx, so the recursive ww_mutex condition is gone, and the per-IB amdgpu_bo_reserve()/amdgpu_bo_unref() dance -- including a BO refcount leak on the amdgpu_bo_reserve() failure path -- is removed.

Reproducer (~150 LoC libdrm_amdgpu): submit a single GFX IB containing PACKET3_INDIRECT_BUFFER chained at GPU VA 0 and wait for the fence. The TDR fires within ~10 s and the deferred coredump worker produces the splat above on every invocation; with this change applied the splat is gone.

Fixes: 7b15fc2d1f1a ("drm/amdgpu: dump job ibs in the devcoredump") Suggested-by: Christian König christian.koenig@amd.com Signed-off-by: Mikhail Gavrilov mikhail.v.gavrilov@gmail.com --- .../gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c | 105 ++++++++++++------ 1 file changed, 71 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c index d386bc775d03..456ea9911d48 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c @@ -24,6 +24,7 @@

#include <generated/utsrelease.h> #include <linux/devcoredump.h> +#include <drm/drm_exec.h> #include "amdgpu_dev_coredump.h" #include "atom.h"

@@ -214,13 +215,9 @@ amdgpu_devcoredump_format(char *buffer, size_t count, struct amdgpu_coredump_inf struct drm_printer p; struct drm_print_iterator iter; struct amdgpu_vm_fault_info *fault_info; - struct amdgpu_bo_va_mapping *mapping; struct amdgpu_ip_block *ip_block; struct amdgpu_res_cursor cursor; - struct amdgpu_bo *abo, *root; - uint64_t va_start, offset; struct amdgpu_ring *ring; - struct amdgpu_vm *vm; u32 *ib_content; uint8_t *kptr; int ver, i, j, r; @@ -343,43 +340,84 @@ amdgpu_devcoredump_format(char *buffer, size_t count, struct amdgpu_coredump_inf drm_printf(&p, "VRAM is lost due to GPU reset!\n");

if (coredump->num_ibs) { - /* Don't try to lookup the VM or map the BOs when calculating the - * size required to store the devcoredump. + struct amdgpu_bo_va_mapping *mapping; + struct amdgpu_bo *abo; + struct drm_exec exec; + struct amdgpu_vm *vm; + u64 va_start, offset; + bool locked = false; + + /* + * Lock the VM root PD and every IB BO together in a single + * drm_exec ticket. Reserving the IB BOs one by one while the + * root PD is held would be a recursive reservation_ww_class_mutex + * acquire without a ww_acquire_ctx, which trips lockdep and + * self-deadlocks for IB BOs that share their dma_resv with the + * root PD (always-valid BOs). + * + * Skip locking entirely on the sizing pass: it does not write + * IB content, so the size estimate doesn't depend on whether + * the BOs are reachable. */ - if (sizing_pass) - vm = NULL; - else - vm = amdgpu_vm_lock_by_pasid(adev, &root, coredump->pasid); + if (!sizing_pass) { + drm_exec_init(&exec, DRM_EXEC_IGNORE_DUPLICATES, + 1 + coredump->num_ibs); + drm_exec_until_all_locked(&exec) { + vm = amdgpu_vm_lock_by_pasid(adev, coredump->pasid, + &exec); + drm_exec_retry_on_contention(&exec); + if (!vm) + break; + + for (int i = 0; i < coredump->num_ibs; i++) { + u64 pfn; + + va_start = coredump->ibs[i].gpu_addr & + AMDGPU_GMC_HOLE_MASK; + pfn = va_start / AMDGPU_GPU_PAGE_SIZE; + mapping = amdgpu_vm_bo_lookup_mapping(vm, pfn); + if (!mapping) + continue; + + abo = mapping->bo_va->base.bo; + r = drm_exec_lock_obj(&exec, &abo->tbo.base); + drm_exec_retry_on_contention(&exec); + if (r) + break; + } + if (r) + break; + } + if (vm && !r) + locked = true; + else + drm_exec_fini(&exec); + } + + for (int i = 0; i < coredump->num_ibs; i++) { + bool emit_content = sizing_pass;

- for (int i = 0; i < coredump->num_ibs && (sizing_pass || vm); i++) { ib_content = kvmalloc_array(coredump->ibs[i].ib_size_dw, 4, GFP_KERNEL); if (!ib_content) continue;

- /* vm=NULL can only happen when 'sizing_pass' is true. Skip to the - * drm_printf() calls (ib_content doesn't need to be initialized - * as its content won't be written anywhere). - */ - if (!vm) + if (!locked) goto output_ib_content;

va_start = coredump->ibs[i].gpu_addr & AMDGPU_GMC_HOLE_MASK; mapping = amdgpu_vm_bo_lookup_mapping(vm, va_start / AMDGPU_GPU_PAGE_SIZE); if (!mapping) - goto free_ib_content; + goto output_ib_content;

- offset = va_start - (mapping->start * AMDGPU_GPU_PAGE_SIZE); - abo = amdgpu_bo_ref(mapping->bo_va->base.bo); - r = amdgpu_bo_reserve(abo, false); - if (r) - goto free_ib_content; + abo = mapping->bo_va->base.bo; + offset = va_start - mapping->start * AMDGPU_GPU_PAGE_SIZE;

if (abo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS) { off = 0;

if (abo->tbo.resource->mem_type != TTM_PL_VRAM) - goto unreserve_abo; + goto output_ib_content;

amdgpu_res_first(abo->tbo.resource, offset, coredump->ibs[i].ib_size_dw * 4, @@ -391,12 +429,13 @@ amdgpu_devcoredump_format(char *buffer, size_t count, struct amdgpu_coredump_inf off += cursor.size; amdgpu_res_next(&cursor, cursor.size); } + emit_content = true; } else { r = ttm_bo_kmap(&abo->tbo, 0, PFN_UP(abo->tbo.base.size), &abo->kmap); if (r) - goto unreserve_abo; + goto output_ib_content;

kptr = amdgpu_bo_kptr(abo); kptr += offset; @@ -404,23 +443,21 @@ amdgpu_devcoredump_format(char *buffer, size_t count, struct amdgpu_coredump_inf coredump->ibs[i].ib_size_dw * 4);

amdgpu_bo_kunmap(abo); + emit_content = true; }

output_ib_content: drm_printf(&p, "\nIB #%d 0x%llx %d dw\n", i, coredump->ibs[i].gpu_addr, coredump->ibs[i].ib_size_dw); - for (int j = 0; j < coredump->ibs[i].ib_size_dw; j++) - drm_printf(&p, "0x%08x\n", ib_content[j]); -unreserve_abo: - if (vm) - amdgpu_bo_unreserve(abo); -free_ib_content: + if (emit_content) { + for (int j = 0; j < coredump->ibs[i].ib_size_dw; j++) + drm_printf(&p, "0x%08x\n", ib_content[j]); + } kvfree(ib_content); } - if (vm) { - amdgpu_bo_unreserve(root); - amdgpu_bo_unref(&root); - } + + if (locked) + drm_exec_fini(&exec); }

return count - iter.remain;

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 with drm_exec_lock_obj(). 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 drm_exec context holds its own reference on the locked root BO, so the helper no longer hands a root reference back to the caller: the root output parameter is dropped, and the transient reference taken across the PASID lookup is released before returning.

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.

Reviewed-by: Christian König christian.koenig@amd.com Signed-off-by: Mikhail Gavrilov mikhail.v.gavrilov@gmail.com --- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 91 ++++++++++++++++---------- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 2 +- 2 files changed, 58 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index fccd758b6699..b9d93b664daf 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -2950,47 +2950,56 @@ 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 * @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. The drm_exec context holds + * a reference on the root BO until it 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) + u32 pasid, struct drm_exec *exec) { unsigned long irqflags; + 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; + root = vm ? amdgpu_bo_ref(vm->root.bo) : NULL; xa_unlock_irqrestore(&adev->vm_manager.pasids, irqflags);

- if (!*root) + if (!root) return NULL;

- r = amdgpu_bo_reserve(*root, true); - if (r) - goto error_unref; + 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); vm = xa_load(&adev->vm_manager.pasids, pasid); - if (vm && vm->root.bo != *root) + if (vm && vm->root.bo != root) vm = NULL; xa_unlock_irqrestore(&adev->vm_manager.pasids, irqflags); - if (!vm) - goto error_unlock; + if (!vm) { + drm_exec_unlock_obj(exec, &root->tbo.base); + amdgpu_bo_unref(&root); + return NULL; + }

- return vm; -error_unlock: - amdgpu_bo_unreserve(*root); + /* The drm_exec context holds its own reference on the root BO. */ + amdgpu_bo_unref(&root);

-error_unref: - amdgpu_bo_unref(root); - return NULL; + return vm; }

/** @@ -3012,33 +3021,49 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, u32 pasid, uint64_t ts, bool write_fault) { 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, 1); + drm_exec_until_all_locked(&exec) { + vm = amdgpu_vm_lock_by_pasid(adev, 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)) { - amdgpu_bo_unref(&root); + node_id, addr >> PAGE_SHIFT, ts, write_fault)) return true; - } - 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, 1); + drm_exec_until_all_locked(&exec) { + vm = amdgpu_vm_lock_by_pasid(adev, pasid, &exec); + drm_exec_retry_on_contention(&exec); + if (!vm) + break; + } + if (!vm) { + drm_exec_fini(&exec); return false; + } }

addr /= AMDGPU_GPU_PAGE_SIZE; @@ -3062,7 +3087,7 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, u32 pasid, value = 0; }

- r = dma_resv_reserve_fences(root->tbo.base.resv, 1); + r = dma_resv_reserve_fences(vm->root.bo->tbo.base.resv, 1); if (r) { pr_debug("failed %d to reserve fence slot\n", r); goto error_unlock; @@ -3076,12 +3101,10 @@ 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);

- amdgpu_bo_unref(&root); - return false; }

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h index d083d7aab75c..0c6e3e0368c7 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h @@ -593,7 +593,7 @@ 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); + u32 pasid, struct drm_exec *exec);

void amdgpu_vm_set_task_info(struct amdgpu_vm *vm);