[drm/amdgpu]: user queue doorbell allocation for IP reqs
Sharma, Shashank
shashank.sharma at amd.com
Fri Jan 3 06:43:51 UTC 2025
On 03/01/2025 07:34, Saleemkhan Jamadar wrote:
> Hi Shashank,
>
> Replied inline [Saleem]
>
>
> Regards,
>
> Salem
>
> On 02/01/25 18:58, Sharma, Shashank wrote:
>> + (amd-gfx)
>>
>> On 01/01/2025 07:03, Saleemkhan Jamadar wrote:
>>> #resending patch
>>>
>>> From 79cd62f882197505dbf9c489ead2b0bcab98209f Mon Sep 17 00:00:00 2001
>>> From: Saleemkhan Jamadar <saleemkhan.jamadar at amd.com>
>>> Date: Wed, 18 Dec 2024 19:30:22 +0530
>>> Subject: [PATCH] drm/amdgpu: user queue doorbell allocation for IP reqs
>>>
>>> Currenlty doorbell allocation handles only 64 bit db size.
>>>
>>> In case of VCN we need to allocated AGDB and per instance
>>> doorbell.VCN/UMSCH doorbell size is 32 bit and offset
>>> calculated is from specific range from the allocated page.
>>>
>>> With these changes individual IP can provide specific reqs
>>> for db allocation.
>>>
>>> Signed-off-by: Saleemkhan Jamadar <saleemkhan.jamadar at amd.com>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c | 41
>>> ++++++++++++++-----
>>> .../gpu/drm/amd/include/amdgpu_userqueue.h | 12 ++++++
>>> 2 files changed, 42 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
>>> index cba51bdf2e2c..4fff15e0d838 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
>>> @@ -163,18 +163,17 @@ void amdgpu_userqueue_destroy_object(struct
>>> amdgpu_userq_mgr *uq_mgr,
>>> amdgpu_bo_unref(&userq_obj->obj);
>>> }
>>> -static uint64_t
>>> +uint64_t
>>> amdgpu_userqueue_get_doorbell_index(struct amdgpu_userq_mgr *uq_mgr,
>>> - struct amdgpu_usermode_queue *queue,
>>> - struct drm_file *filp,
>>> - uint32_t doorbell_offset)
>>> + struct amdgpu_db_info *db_info,
>>> + struct drm_file *filp)
>>> {
>>> uint64_t index;
>>> struct drm_gem_object *gobj;
>>> - struct amdgpu_userq_obj *db_obj = &queue->db_obj;
>>> - int r;
>>> + struct amdgpu_userq_obj *db_obj = db_info->db_obj;
>>> + int r, db_size;
>>> - gobj = drm_gem_object_lookup(filp, queue->doorbell_handle);
>>> + gobj = drm_gem_object_lookup(filp, db_info->doorbell_handle);
>>> if (gobj == NULL) {
>>> DRM_ERROR("Can't find GEM object for doorbell\n");
>>> return -EINVAL;
>>> @@ -196,8 +195,23 @@ amdgpu_userqueue_get_doorbell_index(struct
>>> amdgpu_userq_mgr *uq_mgr,
>>> goto unpin_bo;
>>> }
>>> + switch (db_info->queue_type) {
>>> + case AMDGPU_HW_IP_GFX:
>>> + case AMDGPU_HW_IP_COMPUTE:
>>> + case AMDGPU_HW_IP_DMA:
>>> + db_size = sizeof(u64);
>>> + break;
>>> + case AMDGPU_HW_IP_VCN_ENC:
>>> + db_size = sizeof(u32);
>>> + db_info->doorbell_offset += AMDGPU_NAVI10_DOORBELL64_VCN0_1
>>> << 1;
>>> + break;
>>> + default:
>>> + DRM_WARN("User queues not supported for IP (%d )\n",
>>> db_info->queue_type);
>>> + return -EINVAL;
>>> + }
>>> +
>>> index = amdgpu_doorbell_index_on_bar(uq_mgr->adev, db_obj->obj,
>>> - doorbell_offset, sizeof(u64));
>>> + db_info->doorbell_offset, db_size);
>>> DRM_DEBUG_DRIVER("[Usermode queues] doorbell index=%lld\n",
>>> index);
>>> amdgpu_bo_unreserve(db_obj->obj);
>>> return index;
>>> @@ -242,6 +256,7 @@ amdgpu_userqueue_create(struct drm_file *filp,
>>> union drm_amdgpu_userq *args)
>>> struct amdgpu_device *adev = uq_mgr->adev;
>>> const struct amdgpu_userq_funcs *uq_funcs;
>>> struct amdgpu_usermode_queue *queue;
>>> + struct amdgpu_db_info db_info;
>>> uint64_t index;
>>> int qid, r = 0;
>>> @@ -269,19 +284,23 @@ amdgpu_userqueue_create(struct drm_file
>>> *filp, union drm_amdgpu_userq *args)
>>> goto unlock;
>>> }
>>> queue->doorbell_handle = args->in.doorbell_handle;
>>> - queue->doorbell_index = args->in.doorbell_offset;
>> Nack, this is going to break mapping/unmapping for GFX UQ. Nothing
>> should be removed from the queue structure, as all of it is accounted
>> for.
> [Saleem]:This assignment is not used anywhere, moreover it gets
> overwritten with mapped doorbell index below.
I see that your patch is removing the mapped doorbell index from the
queue as well, whereas the mapped index of the doorbell is required to
map a queue in the MES (see amdgpu_userqueue_map() function). So that's
wrong interpretation of the code that its not being used anywhere.
>>> queue->queue_type = args->in.ip_type;
>>> queue->vm = &fpriv->vm;
>>> + db_info.queue_type = queue->queue_type;
>>> + db_info.doorbell_handle = queue->doorbell_handle;
>>> + db_info.db_obj = &queue->db_obj;
>>> + db_info.doorbell_offset = args->in.doorbell_offset;
>>> +
>> I can see that all the information you are adding in struct db_info
>> is coming from queue only, then why to add a new structure here at
>> all ? Instead, you can just do this in function
>> [Saleem]: In previous impl you can see that db_obj is fetched form
>> queue structure. In case of vcn we have mulpitle doorbell to be
>> allocated, to accomodate this and to avoid the another level of check
>> to identify which doorbell is being allocated this db_info (structure
>> used locally) helps. db_info shares data for which allocation needs
>> to done.
Nope, there is no difference in information available in the queue->* vs
db_info->*, so its duplication of code for no apparent reason, so it
doesn't make sense to use this here in the base/IP independent UQ file.
Just update the doorbell offset as suggested before in the queue->*
itself, and then if you want to have this structure, you can introduce
it in the vcn specific implementation (vcn_db_info*) and move this code
which you added here into the vcn_userqueueu* file.
- Shashank
>>
>> amdgpu_userqueue_get_doorbell_index():
>>
>> db_size = sizeof(u62);
>>
>> switch(queue->queue_type) {
>> case VCN:
>> queue->doorbell_offset += AMDGPU_NAVI10_DOORBELL64_VCN0_1 << 1;
>> db_size = sizeof(u32);
>> break;
>> }
>> [Saleem]: This change I can make.
>> <same doorbell offset calculation as usual>
>>
>> - Shashank
>>
>>> /* Convert relative doorbell offset into absolute doorbell
>>> index */
>>> - index = amdgpu_userqueue_get_doorbell_index(uq_mgr, queue,
>>> filp, args->in.doorbell_offset);
>>> + index = amdgpu_userqueue_get_doorbell_index(uq_mgr, &db_info,
>>> filp);
>>> if (index == (uint64_t)-EINVAL) {
>>> DRM_ERROR("Failed to get doorbell for queue\n");
>>> kfree(queue);
>>> goto unlock;
>>> }
>>> - queue->doorbell_index = index;
>>> + queue->doorbell_index = index;
>>> xa_init_flags(&queue->fence_drv_xa, XA_FLAGS_ALLOC);
>>> r = amdgpu_userq_fence_driver_alloc(adev, queue);
>>> if (r) {
>>> diff --git a/drivers/gpu/drm/amd/include/amdgpu_userqueue.h
>>> b/drivers/gpu/drm/amd/include/amdgpu_userqueue.h
>>> index 2bf28f3454cb..3d54601c6a24 100644
>>> --- a/drivers/gpu/drm/amd/include/amdgpu_userqueue.h
>>> +++ b/drivers/gpu/drm/amd/include/amdgpu_userqueue.h
>>> @@ -39,6 +39,13 @@ struct amdgpu_userq_obj {
>>> struct amdgpu_bo *obj;
>>> };
>>> +struct amdgpu_db_info {
>>> + uint64_t doorbell_handle;
>>> + uint32_t queue_type;
>>> + uint32_t doorbell_offset;
>>> + struct amdgpu_userq_obj *db_obj;
>>> +};
>>> +
>>> struct amdgpu_usermode_queue {
>>> int queue_type;
>>> uint8_t queue_active;
>>> @@ -93,4 +100,9 @@ void amdgpu_userqueue_destroy_object(struct
>>> amdgpu_userq_mgr *uq_mgr,
>>> void amdgpu_userqueue_suspend(struct amdgpu_userq_mgr *uq_mgr);
>>> int amdgpu_userqueue_active(struct amdgpu_userq_mgr *uq_mgr);
>>> +
>>> +uint64_t amdgpu_userqueue_get_doorbell_index(struct
>>> amdgpu_userq_mgr *uq_mgr,
>>> + struct amdgpu_db_info *db_info,
>>> + struct drm_file *filp);
>>> +
>>> #endif
More information about the amd-gfx
mailing list