[Public]
Regards,
Prike
> -----Original Message-----
> From: Koenig, Christian <christian.koe...@amd.com>
> Sent: Tuesday, July 15, 2025 8:18 PM
> To: Liang, Prike <prike.li...@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <alexander.deuc...@amd.com>
> Subject: Re: [PATCH v6 06/11] drm/amdgpu: track the userq bo va for its obj
> management
>
> On 15.07.25 14:05, Liang, Prike wrote:
> > [Public]
> >
> > Regards,
> > Prike
> >
> >> -----Original Message-----
> >> From: Koenig, Christian <christian.koe...@amd.com>
> >> Sent: Friday, July 11, 2025 8:11 PM
> >> To: Liang, Prike <prike.li...@amd.com>; amd-gfx@lists.freedesktop.org
> >> Cc: Deucher, Alexander <alexander.deuc...@amd.com>
> >> Subject: Re: [PATCH v6 06/11] drm/amdgpu: track the userq bo va for
> >> its obj management
> >>
> >>
> >>
> >> On 11.07.25 11:39, Prike Liang wrote:
> >>> The user queue object destroy requires ensuring its VA keeps mapping
> >>> prior to the queue being destroyed.
> >>> Otherwise, it seems a bug in the user space or VA freed wrongly, and
> >>> the kernel driver should report an invalidated error to the user
> >>> IOCLT request.
> >>>
> >>> Signed-off-by: Prike Liang <prike.li...@amd.com>
> >>> ---
> >>> drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 15 +++++++++++++++
> >>> 1 file changed, 15 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> >>> index 2856c2506bee..81fbb00b6d91 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> >>> @@ -510,12 +510,24 @@ amdgpu_userq_destroy(struct drm_file *filp,
> >>> int
> >> queue_id)
> >>> return -EINVAL;
> >>> }
> >>> amdgpu_userq_wait_for_last_fence(uq_mgr, queue);
> >>> +
> >>> + /*
> >>> + * At this point the userq obj va should be mapped,
> >>> + * otherwise will return error to user.
> >>> + */
> >>> + if (!amdgpu_userq_buffer_vas_mapped(&fpriv->vm, queue)) {
> >>> + drm_warn(adev_to_drm(uq_mgr->adev), "the userq obj va
> >>> + shouldn't
> >> be umapped here\n");
> >>> + r = -EINVAL;
> >>> + }
> >>> +
> >>
> >> That is still not something we can do.
> >>
> >> Destroying an userque can't fail in any way.
> > Yes, the userq destroy will continue performing in this invalid case.
> > Can we keep this part for detecting this invalid destroy case?
>
> No, exactly that's the point there is no such thing as an invalid destroy
> case.
>
> Perfectly valid to destroy the queue no matter what state we are in.
>
> The only invalid operation would be trying to destroy a queue which doesn't
> exists in
> the first place.
We might need a specific error code to detect invalid case of the VA unmapped
before destroying,
otherwise, it's difficult to identify and detect the test results for the IGT
negative test.
> Regards,
> Christian.
>
> > Furthermore, it looks like this error code will not affect the destroy
> > request at userspace since the Mesa driver doesn't check the userq destroy
> > return
> value.
> >
> >> Regards,
> >> Christian.
> >>
> >>> r = amdgpu_userq_unmap_helper(uq_mgr, queue);
> >>> /*TODO: It requires a reset for userq hw unmap error*/
> >>> if (unlikely(r != AMDGPU_USERQ_STATE_UNMAPPED)) {
> >>> drm_warn(adev_to_drm(uq_mgr->adev), "trying to destroy a
> >>> HW
> >> mapping userq\n");
> >>> r = -ETIMEDOUT;
> >>> }
> >>> +
> >>> + amdgpu_userq_buffer_vas_put(&fpriv->vm, queue);
> >>> amdgpu_userq_cleanup(uq_mgr, queue, queue_id);
> >>> mutex_unlock(&uq_mgr->userq_mutex);
> >>>
> >>> @@ -636,6 +648,9 @@ amdgpu_userq_create(struct drm_file *filp, union
> >> drm_amdgpu_userq *args)
> >>> goto unlock;
> >>> }
> >>>
> >>> + /* refer to the userq objects vm bo*/
> >>> + amdgpu_userq_buffer_vas_get(queue->vm, queue);
> >>> +
> >>> qid = idr_alloc(&uq_mgr->userq_idr, queue, 1,
> >> AMDGPU_MAX_USERQ_COUNT, GFP_KERNEL);
> >>> if (qid < 0) {
> >>> drm_file_err(uq_mgr->file, "Failed to allocate a queue
> >>> id\n");
> >
