On 6/5/25 15:15, Liang, Prike wrote:
> [Public]
>
>> -----Original Message-----
>> From: Koenig, Christian <christian.koe...@amd.com>
>> Sent: Monday, June 2, 2025 7:05 PM
>> To: Liang, Prike <prike.li...@amd.com>; amd-gfx@lists.freedesktop.org
>> Cc: Deucher, Alexander <alexander.deuc...@amd.com>; Lazar, Lijo
>> <lijo.la...@amd.com>
>> Subject: Re: [PATCH 3/9] drm/amdgpu: wait for the user objects done before
>> destroying
>>
>> On 5/30/25 09:54, Prike Liang wrote:
>>> The userq buffer should complete its attached work before being
>>> destroyed.
>>
>> That doesn't make sense at all. We should wait for the requested signal
>> fences, but
>> apart from that it is the responsibility for userspace to make sure that
>> queues are
>> only destroyed after they are idle.
>>
>> If the kernel does this it should *never* wait for anything ongoing, it
>> should only
>> forcefully unmap the queue and destroy it.
> [Prike] Yeah, generally, the userq destroy IOCTL request should be performed
> after Mesa tears the window context down by executing amdgpu_winsys_destroy().
> But if there's an improper case where kernel tries to destroy the userq BOs
> attached fences somehow haven't signaled, then it's safe to force destroy the
> userq buffer object with ongoing work?
What do you mean with attached fences? We need to wait for the userq fence
before destroying the queue, but apart from that it shouldn't matter what
fences are attached where.
Regards,
Christian.
>
>> Regards,
>> Christian.
>>
>>>
>>> Signed-off-by: Prike Liang <prike.li...@amd.com>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 3 ++-
>>> drivers/gpu/drm/amd/amdgpu/mes_userqueue.c | 3 +++
>>> 2 files changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> index 8eea0e1e1b6a..f45585bd5872 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> @@ -217,6 +217,8 @@ int amdgpu_userq_create_object(struct
>>> amdgpu_userq_mgr *uq_mgr, void amdgpu_userq_destroy_object(struct
>> amdgpu_userq_mgr *uq_mgr,
>>> struct amdgpu_userq_obj *userq_obj) {
>>> + amdgpu_bo_sync_wait(userq_obj->obj,
>> AMDGPU_FENCE_OWNER_UNDEFINED,
>>> + false);
>>> amdgpu_bo_kunmap(userq_obj->obj);
>>> amdgpu_bo_unref(&userq_obj->obj);
>>> }
>>> @@ -317,7 +319,6 @@ amdgpu_userq_destroy(struct drm_file *filp, int
>>> queue_id)
>>> amdgpu_bo_unpin(queue->db_obj.obj);
>>> amdgpu_bo_unreserve(queue->db_obj.obj);
>>> }
>>> - amdgpu_bo_unref(&queue->db_obj.obj);
>>> r = amdgpu_userq_unmap_helper(uq_mgr, queue);
>>> if (r != AMDGPU_USERQ_STATE_UNMAPPED) {
>>> drm_dbg_driver(adev_to_drm(uq_mgr->adev), "Can't unmap the
>> queue
>>> for destroying.\n"); diff --git
>>> a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
>>> b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
>>> index 1457fb49a794..b46e67b179fc 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
>>> @@ -336,6 +336,9 @@ mes_userq_mqd_destroy(struct amdgpu_userq_mgr
>> *uq_mgr,
>>> struct amdgpu_usermode_queue *queue) {
>>> amdgpu_userq_destroy_object(uq_mgr, &queue->fw_obj);
>>> + if (queue->db_obj.obj->tbo.pin_count)
>>> + amdgpu_bo_unpin(queue->db_obj.obj);
>>> + amdgpu_userq_destroy_object(uq_mgr, &queue->db_obj);
>>> kfree(queue->userq_prop);
>>> amdgpu_userq_destroy_object(uq_mgr, &queue->mqd); }
>
