AMD General
> -----Original Message-----
> From: Koenig, Christian <[email protected]>
> Sent: Friday, August 14, 2026 7:30 PM
> To: Zhang, Jesse(Jie) <[email protected]>; [email protected]
> Cc: Deucher, Alexander <[email protected]>; Liang, Prike
> <[email protected]>; Khatri, Sunil <[email protected]>
> Subject: Re: [PATCH] drm/amdgpu/userq: lock and validate wptr BOs before
> reading their GPU offset on restore
>
>
>
> On 8/14/26 10:55, Jesse Zhang wrote:
> > amdgpu_userq_vm_validate_and_restore_queue() reads each queue's wptr
> > BO GPU offset via amdgpu_bo_gpu_offset() after only calling
> > amdgpu_ttm_alloc_gart() on it. But the wptr BOs are not part of this
> > VM (their reservation object is their own, not vm->root), so neither
> > amdgpu_vm_validate() nor amdgpu_userq_bo_validate() (which only
> > handles the VM's evicted list) covers them. As a result a wptr BO can
> > still be in TTM_PL_SYSTEM and unreserved when its offset is read,
> > tripping the
> > amdgpu_bo_gpu_offset() sanity checks from the restore worker:
> >
> > ------------[ cut here ]------------
> > WARNING: amdgpu_object.c:1486 at amdgpu_bo_gpu_offset+0x75/0xa0
> [amdgpu], CPU#3: kworker/3:1/116
> > Workqueue: events amdgpu_userq_restore_worker [amdgpu]
> > RIP: 0010:amdgpu_bo_gpu_offset+0x75/0xa0 [amdgpu]
> > Call Trace:
> > <TASK>
> > amdgpu_userq_vm_validate_and_restore_queue+0x629/0x960 [amdgpu]
> > amdgpu_userq_restore_worker+0xa6/0x180 [amdgpu]
> > process_scheduled_works+0xa6/0x460
> > worker_thread+0x13c/0x290
> > kthread+0xfb/0x140
> > ret_from_fork+0x1b6/0x2b0
> > ret_from_fork_asm+0x1a/0x30
> > </TASK>
> > ---[ end trace 0000000000000000 ]---
> > ------------[ cut here ]------------
> > WARNING: amdgpu_object.c:1485 at amdgpu_bo_gpu_offset+0x9a/0xa0
> [amdgpu], CPU#2: kworker/2:1/127
> > Workqueue: events amdgpu_userq_restore_worker [amdgpu]
> > RIP: 0010:amdgpu_bo_gpu_offset+0x9a/0xa0 [amdgpu]
> >
> > amdgpu_ttm_alloc_gart() only creates the GART mapping; it does not
> > migrate the BO out of system memory, so the offset read is bogus (the
> > queue would resume with a wrong wptr address).
> >
> > Lock each wptr BO into the drm_exec context and validate it into GTT
> > inside the drm_exec_until_all_locked() block, mirroring what the
> > create path (mes_userq_create_wptr_mapping) already does, so that the
> > offset read later is safe and correct.
> >
> > Signed-off-by: Jesse Zhang <[email protected]>
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 26
> > +++++++++++++++++++++++
> > 1 file changed, 26 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> > index 17cc48d87c4d..59aa4802c111 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> > @@ -1054,6 +1054,32 @@
> amdgpu_userq_vm_validate_and_restore_queue(struct amdgpu_userq_mgr
> *uq_mgr)
> > drm_exec_retry_on_contention(&exec);
> > if (unlikely(ret))
> > goto unlock_all;
> > +
> > + /*
> > + * The per-queue wptr BOs are not part of this VM (their resv is
> > + * their own, not vm->root), so the validation above does not
> > + * cover them.
>
> That's not correct.
>
> The WPTR BOs absolutely must be part of the VM or otherwise the MES/CP/SDMA
> FW wouldn't be able to read it.
Thanks Christian, you're right that “not part of this VM” is incorrect wording
— the WPTR BO is VM-mapped.
The issue I’m hitting is different: in restore, amdgpu_bo_gpu_offset() can be
called without the WPTR BO reservation lock held, which triggers WARN_ON and
races with BO state changes.
I’ll send v2 with corrected description and a narrower fix that only adds each
WPTR BO to the drm_exec lock set before reading/updating its GPU offset.
Thanks
Jesse
>
> > + Lock and validate each into GTT here so that
> > + * reading its GPU offset below is safe - matching what the
> > + * create path (mes_userq_create_wptr_mapping) does.
> > + */
> > + xa_for_each(&uq_mgr->userq_xa, tmp_key, queue) {
> > + struct ttm_operation_ctx wptr_ctx = { false, false };
> > +
> > + bo = queue->wptr_obj.obj;
> > + if (!bo)
> > + continue;
> > +
> > + ret = drm_exec_prepare_obj(&exec, &bo->tbo.base,
> > + TTM_NUM_MOVE_FENCES + 1);
> > + drm_exec_retry_on_contention(&exec);
> > + if (unlikely(ret))
> > + goto unlock_all;
> > +
> > + amdgpu_bo_placement_from_domain(bo, bo-
> >allowed_domains);
> > + ret = ttm_bo_validate(&bo->tbo, &bo->placement,
> > &wptr_ctx);
> > + if (unlikely(ret))
> > + goto unlock_all;
> > + }
>
> Clear NAK to that, this is clearly not correct.
>
> Regards,
> Christian.
>
> > }
> >
> > if (invalidated) {
