On 6/6/25 09:19, Liang, Prike wrote:
> [Public]
>
>> From: Koenig, Christian <christian.koe...@amd.com>
>> Sent: Thursday, June 5, 2025 9:38 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 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.
> [Prike] The userq destroy function already waits for its last fence using
> amdgpu_userq_wait_for_last_fence().
> I mean, is it necessary to also wait for the reservation fences of the
> userq's MQD, doorbell, and WPTR buffer
> objects to be signaled by using amdgpu_bo_sync_wait() before destroying such
> BOs?
No absolutely not.
Regards,
Christian.
>
>> 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); }
>>>
>
