RE: [PATCH] drm/amdgpu/userq: lock and validate wptr BOs before reading their GPU offset on restore

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) {