Re: [PATCH v6 03/11] drm/amdgpu: rework the userq doorbell object destroy

["Christian König" <christian.koenig@amd.com>]

On 15.07.25 10:07, Liang, Prike wrote:
> [Public]
> 
> Regards,
>       Prike
> 
>> -----Original Message-----
>> From: Koenig, Christian <Christian.Koenig@amd.com>
>> Sent: Friday, July 11, 2025 8:01 PM
>> To: Liang, Prike <Prike.Liang@amd.com>; amd-gfx@lists.freedesktop.org
>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
>> Subject: Re: [PATCH v6 03/11] drm/amdgpu: rework the userq doorbell object
>> destroy
>>
>> On 11.07.25 11:39, Prike Liang wrote:
>>> This patch aims to unify and destroy the userq doorbell objects at
>>> mes_userq_mqd_destroy(), and this change will also help with unpinning
>>> and destroying the userq doorbell objects for amdgpu_userq_mgr_fini()
>>> during releasing the drm files.
>>>
>>> Signed-off-by: Prike Liang <Prike.Liang@amd.com>
>>> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
>>> ---
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c  | 6 ------
>>> drivers/gpu/drm/amd/amdgpu/mes_userqueue.c | 7 +++++++
>>>  2 files changed, 7 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> index 3d2a7f8946cf..15e833b1b3e3 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>> @@ -312,12 +312,6 @@ amdgpu_userq_destroy(struct drm_file *filp, int
>> queue_id)
>>>             return -EINVAL;
>>>     }
>>>     amdgpu_userq_wait_for_last_fence(uq_mgr, queue);
>>> -   r = amdgpu_bo_reserve(queue->db_obj.obj, true);
>>> -   if (!r) {
>>> -           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);
>>>     /*TODO: It requires a reset for userq hw unmap error*/
>>>     if (unlikely(r != AMDGPU_USERQ_STATE_UNMAPPED)) { diff --git
>>> a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
>>> b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
>>> index 1457fb49a794..15aa1ca67a11 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
>>> @@ -336,6 +336,13 @@ 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 (!amdgpu_bo_reserve(queue->db_obj.obj, true)) {
>>> +           amdgpu_bo_unpin(queue->db_obj.obj);
>>> +           amdgpu_bo_unreserve(queue->db_obj.obj);
>>> +           amdgpu_userq_destroy_object(uq_mgr, &queue->db_obj);
>>> +   }
>>> +
>>
>> That makes no sense to do here. The pinning isn't done in mes_userq_mqd_create()
>> either.
> Yes, but the doorbell BO is pinned by amdgpu_userq_get_doorbell_index(), which is still
> Invoked during userq BOs creation phase. This patch wants to free the doorbell object like some
> other userq objects at the unified place of mes_userq_mqd_destroy().

Yeah and exactly that is not a good idea.

The doorbell object is provided by userspace and not allocated by the kernel like the MQD.

So destroying it here makes no sense at all. You are most likely messing up the doorbell reference count with that.

>> In general we should avoid pinning the MQD in the first place, that buffer needs to be
>> fences instead.
> If here not pin the userq doorbell BO, then will the doorbell index be changed when the doorbell
> BO is moved?

Correct, yes. The doorbell index needs to be updated on each resume of the userqueue.

We haven't implemented that yet since we weren't sure if the MES FW could handle that (and because the eviction fences wasn't ready at that time).

Regards,
Christian.

> 
>> Regards,
>> Christian.
>>
>>>     kfree(queue->userq_prop);
>>>     amdgpu_userq_destroy_object(uq_mgr, &queue->mqd);  }
>