Re: [PATCH 1/2] drm/amdgpu: clean up userq iVA mapping after removing userq from MES

["Khatri, Sunil" <sukhatri@amd.com>]

[-- Attachment #1: Type: text/plain, Size: 3641 bytes --]


On 14-05-2026 06:12 pm, Prike Liang wrote:
> User queue destroy removed the tracked queue VA mappings before removing
> the HW queue from MES. If the queue still had active waves, HW like as TCP
> could continue accessing queue backing memory after the VM mappings were
> removed, resulting in gfxhub page faults.
>
> So, that needs to move queue VA cleanup after HW queue unmap. Meanwhile,
> if MES fails to remove the queue, then need to run reset recovery before
> freeing queue resources.
>
> Signed-off-by: Prike Liang<Prike.Liang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 43 +++++++++++++++++------
>   1 file changed, 32 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> index 83aee0810513..2e3edb6dd506 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> @@ -625,8 +625,7 @@ amdgpu_userq_destroy(struct amdgpu_userq_mgr *uq_mgr, struct amdgpu_usermode_que
>   	struct amdgpu_device *adev = uq_mgr->adev;
>   	struct amdgpu_fpriv *fpriv = uq_mgr_to_fpriv(uq_mgr);
>   	struct amdgpu_vm *vm = &fpriv->vm;
> -
> -	int r = 0;
> +	int r = 0, tmp;
>   
>   	trace_amdgpu_userq_destroy_start(queue);
>   
> @@ -635,15 +634,6 @@ amdgpu_userq_destroy(struct amdgpu_userq_mgr *uq_mgr, struct amdgpu_usermode_que
>   	/* Cancel any pending hang detection work and cleanup */
>   	cancel_delayed_work_sync(&queue->hang_detect_work);
>   
> -	r = amdgpu_bo_reserve(vm->root.bo, false);
> -	if (r) {
> -		drm_file_err(uq_mgr->file, "Failed to reserve root bo during userqueue destroy\n");
> -		trace_amdgpu_userq_destroy_end(queue, r);
> -		return r;
> -	}
> -	amdgpu_userq_buffer_vas_list_cleanup(adev, queue);
> -	amdgpu_bo_unreserve(vm->root.bo);
> -
>   	mutex_lock(&uq_mgr->userq_mutex);
>   	amdgpu_userq_wait_for_last_fence(queue);
>   
> @@ -651,6 +641,37 @@ amdgpu_userq_destroy(struct amdgpu_userq_mgr *uq_mgr, struct amdgpu_usermode_que
>   	debugfs_remove_recursive(queue->debugfs_queue);
>   #endif
>   	r = amdgpu_userq_unmap_helper(queue);
> +
> +	if (r) {
> +		drm_file_err(uq_mgr->file,
> +			     "Failed to unmap userqueue during destroy, ret=%d\n",
> +			     r);
> +		amdgpu_userq_fence_driver_force_completion(queue);
We should be calling this before amdgpu_userq_unmap_helper because we 
are in tear down and we want to finish all the work and their 
corresponding fences and if unmap fails or not we should be cleaning up 
the resources.
> +		amdgpu_reset_domain_schedule(uq_mgr->adev->reset_domain,
> +					     &uq_mgr->reset_work);

reset at this stage during tear down seems like a bad idea, we are 
infact waiting for if any reset thread if already scheduled.

regards
Sunil khatri

> +		flush_work(&uq_mgr->reset_work);
> +	}
> +	mutex_unlock(&uq_mgr->userq_mutex);
> +	/*
> +	 * Drop the queue VA mappings only after the HW queue is removed (or
> +	 * reset recovery has run). Removing the mappings first lets active TCP
> +	 * waves fault on queue backing memory while MES is still trying to
> +	 * process REMOVE_QUEUE.
> +	 */
> +	tmp = amdgpu_bo_reserve(vm->root.bo, false);
> +	if (tmp) {
> +		drm_file_err(uq_mgr->file,
> +			     "Failed to reserve root bo during userqueue destroy\n");
> +		if (!r)
> +			r = tmp;
> +	} else {
> +		tmp = amdgpu_userq_buffer_vas_list_cleanup(adev, queue);
> +		amdgpu_bo_unreserve(vm->root.bo);
> +		if (tmp && !r)
> +			r = tmp;
> +	}
> +
> +	mutex_lock(&uq_mgr->userq_mutex);
>   	atomic_dec(&uq_mgr->userq_count[queue->queue_type]);
>   	amdgpu_userq_cleanup(queue);
>   	mutex_unlock(&uq_mgr->userq_mutex);

[-- Attachment #2: Type: text/html, Size: 4426 bytes --]