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

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

[Jesse Zhang]

On resume, amdgpu_userq_vm_validate_and_restore_queue() updates each queue's
wptr GPU address via amdgpu_bo_gpu_offset().

WPTR BOs are VM-mapped, but each BO has its own reservation object and is not
implicitly covered by the VM validation path here. This can leave offset reads
without proper BO locking/placement state and trigger WARN_ONs.
  ------------[ 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]

Add each queue's WPTR BO to the drm_exec ww context and validate it to its
allowed placement before the later offset update.

Signed-off-by: Jesse Zhang <Jesse.Zhang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 24 +++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
index 17cc48d87c4d..ab8fc14a235b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
@@ -1054,6 +1054,30 @@ 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;
+
+		/*
+		 * WPTR BOs are VM-mapped, but each BO has its own reservation
+		 * object. Lock them into this drm_exec ww context so the later
+		 * amdgpu_bo_gpu_offset() reads are done with the BO resv locked.
+		 */
+		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;
+		}
 	}
 
 	if (invalidated) {
-- 
2.49.0

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

[Christian König]

On 8/17/26 09:05, Jesse Zhang wrote:
> On resume, amdgpu_userq_vm_validate_and_restore_queue() updates each queue's
> wptr GPU address via amdgpu_bo_gpu_offset().
> 
> WPTR BOs are VM-mapped, but each BO has its own reservation object and is not
> implicitly covered by the VM validation path here. This can leave offset reads
> without proper BO locking/placement state and trigger WARN_ONs.
>   ------------[ 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]
> 
> Add each queue's WPTR BO to the drm_exec ww context and validate it to its
> allowed placement before the later offset update.
> 
> Signed-off-by: Jesse Zhang <Jesse.Zhang@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 24 +++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> index 17cc48d87c4d..ab8fc14a235b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> @@ -1054,6 +1054,30 @@ 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;
> +
> +		/*
> +		 * WPTR BOs are VM-mapped, but each BO has its own reservation
> +		 * object. Lock them into this drm_exec ww context so the later
> +		 * amdgpu_bo_gpu_offset() reads are done with the BO resv locked.
> +		 */
> +		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;
> +		}

Mhm, something is very wrong here and that is clearly not the right solution.

At this point all BOs which are part of the VM should already be locked and validated and that include the WPTR BOs.

So you are just working around a bug somewhere else.

Regards,
Christian.

>  	}
>  
>  	if (invalidated) {

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

[Jesse Zhang]

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 <Jesse.Zhang@amd.com>
---
 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. 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;
+		}
 	}
 
 	if (invalidated) {
-- 
2.49.0

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

[Christian König]

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 <Jesse.Zhang@amd.com>
> ---
>  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.

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

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

[Zhang, Jesse(Jie)]

AMD General

> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Friday, August 14, 2026 7:30 PM
> To: Zhang, Jesse(Jie) <Jesse.Zhang@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Liang, Prike
> <Prike.Liang@amd.com>; Khatri, Sunil <Sunil.Khatri@amd.com>
> 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 <Jesse.Zhang@amd.com>
> > ---
> >  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) {