Re: [PATCH v3 2/2] drm/amdkfd: get doorbell's absolute offset based on the db size

["Yadav, Arvind" <arvyadav@amd.com>]

On 10/4/2023 10:29 PM, Felix Kuehling wrote:
>
> On 2023-10-04 12:16, Arvind Yadav wrote:
>> This patch is to align the absolute doorbell offset
>> based on the doorbell's size. So that doorbell offset
>> will be aligned for both 32 bit and 64 bit.
>>
>> v2:
>> - Addressed the review comment from Felix.
>> v3:
>> - Adding doorbell_size as parameter to get db absolute offset.
>>
>> Cc: Christian Koenig <christian.koenig@amd.com>
>> Cc: Alex Deucher <alexander.deucher@amd.com>
>> Signed-off-by: Shashank Sharma <shashank.sharma@amd.com>
>> Signed-off-by: Arvind Yadav <Arvind.Yadav@amd.com>
>
> The final result looks good to me. But please squash the two patches 
> into one. The first patch on its own breaks the build, and that's 
> something we don't want to commit to the branch history as it makes 
> tracking regressions (e.g. with git bisect) very hard or impossible.
>
> More nit-picks inline.
Sure, we can have one patch.
>
>
>> ---
>>   .../gpu/drm/amd/amdkfd/kfd_device_queue_manager.c   |  6 +++++-
>>   drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c           | 13 +++++++++++--
>>   .../gpu/drm/amd/amdkfd/kfd_process_queue_manager.c  |  4 +++-
>>   3 files changed, 19 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c 
>> b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
>> index 0d3d538b64eb..690ff131fe4b 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
>> @@ -346,6 +346,7 @@ static int allocate_doorbell(struct 
>> qcm_process_device *qpd,
>>                    uint32_t const *restore_id)
>>   {
>>       struct kfd_node *dev = qpd->dqm->dev;
>> +    uint32_t doorbell_size;
>>         if (!KFD_IS_SOC15(dev)) {
>>           /* On pre-SOC15 chips we need to use the queue ID to
>> @@ -405,9 +406,12 @@ static int allocate_doorbell(struct 
>> qcm_process_device *qpd,
>>           }
>>       }
>>   +    doorbell_size = dev->kfd->device_info.doorbell_size;
>> +
>>       q->properties.doorbell_off = 
>> amdgpu_doorbell_index_on_bar(dev->adev,
>>                                     qpd->proc_doorbells,
>> -                                  q->doorbell_id);
>> +                                  q->doorbell_id,
>> +                                  doorbell_size);
>
> You don't need a local variable for doorbell size that's only used 
> once. Just pass dev->kfd->device_info.doorbell_size directly.
>
I have used local variable to make the code cleaner but I will remove 
local variable.
>
>>       return 0;
>>   }
>>   diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c 
>> b/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c
>> index 7b38537c7c99..59dd76c4b138 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c
>> @@ -161,7 +161,10 @@ void __iomem *kfd_get_kernel_doorbell(struct 
>> kfd_dev *kfd,
>>       if (inx >= KFD_MAX_NUM_OF_QUEUES_PER_PROCESS)
>>           return NULL;
>>   -    *doorbell_off = amdgpu_doorbell_index_on_bar(kfd->adev, 
>> kfd->doorbells, inx);
>> +    *doorbell_off = amdgpu_doorbell_index_on_bar(kfd->adev,
>> +                             kfd->doorbells,
>> +                             inx,
>> + kfd->device_info.doorbell_size);
>>       inx *= 2;
>>         pr_debug("Get kernel queue doorbell\n"
>> @@ -233,6 +236,7 @@ phys_addr_t kfd_get_process_doorbells(struct 
>> kfd_process_device *pdd)
>>   {
>>       struct amdgpu_device *adev = pdd->dev->adev;
>>       uint32_t first_db_index;
>> +    uint32_t doorbell_size;
>>         if (!pdd->qpd.proc_doorbells) {
>>           if (kfd_alloc_process_doorbells(pdd->dev->kfd, pdd))
>> @@ -240,7 +244,12 @@ phys_addr_t kfd_get_process_doorbells(struct 
>> kfd_process_device *pdd)
>>               return 0;
>>       }
>>   -    first_db_index = amdgpu_doorbell_index_on_bar(adev, 
>> pdd->qpd.proc_doorbells, 0);
>> +    doorbell_size = pdd->dev->kfd->device_info.doorbell_size;
>> +
>> +    first_db_index = amdgpu_doorbell_index_on_bar(adev,
>> +                              pdd->qpd.proc_doorbells,
>> +                              0,
>> +                              doorbell_size);
>
> Same as above, no local variable needed.
Noted,
>
>
>>       return adev->doorbell.base + first_db_index * sizeof(uint32_t);
>>   }
>>   diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c 
>> b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
>> index adb5e4bdc0b2..010cd8e8e6a1 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
>> @@ -375,9 +375,11 @@ int pqm_create_queue(struct 
>> process_queue_manager *pqm,
>>            * relative doorbell index = Absolute doorbell index -
>>            * absolute index of first doorbell in the page.
>>            */
>> +        uint32_t doorbell_size = 
>> pdd->dev->kfd->device_info.doorbell_size;
>>           uint32_t first_db_index = 
>> amdgpu_doorbell_index_on_bar(pdd->dev->adev,
>> pdd->qpd.proc_doorbells,
>> -                                       0);
>> +                                       0,
>> +                                       doorbell_size);
>
> No local variable needed.
>
Noted,

Thanks
~Arvind

> Regards,
>   Felix
>
>
>>             *p_doorbell_offset_in_process = (q->properties.doorbell_off
>>                           - first_db_index) * sizeof(uint32_t);