On 5/21/26 17:08, Mikhail Gavrilov wrote:
> 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.
That commit message is a bit to long. It should describe the problem and the
solution and not necessary how to reproduce it.
>
> Fixes: 7b15fc2d1f1a ("drm/amdgpu: dump job ibs in the devcoredump")
> Suggested-by: Christian König <[email protected]>
> Signed-off-by: Mikhail Gavrilov <[email protected]>
> ---
> .../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;
It's probably a good idea to put the IB dumping into a separate function.
> + bool locked = false;
Drop that variable and handling, it is superflous.
> +
> + /*
> + * 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;
This should use goto error handling, when we can't find the VM we should abort
here.
> +
> + 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;
That's also an error, it could be that we just want to print the IB start
address in that case.
> +
> + abo = mapping->bo_va->base.bo;
> + r = drm_exec_lock_obj(&exec,
> &abo->tbo.base);
> + drm_exec_retry_on_contention(&exec);
> + if (r)
> + break;
Dito
> + }
> + if (r)
> + break;
And here as well.
> + }
> + if (vm && !r)
> + locked = true;
> + else
> + drm_exec_fini(&exec);
Don't call drm_exec_fini() here.
Regards,
Christian.
> + }
> +
> + 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;
