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