Re: [PATCH v2 1/1] drm/amdkfd: Fix unaligned doorbell absolute offset for gfx8

[Felix Kuehling <felix.kuehling@amd.com>]

On 2023-09-28 11:38, Shashank Sharma wrote:
> Hello Felix, Mukul,
>
> On 28/09/2023 17:30, Felix Kuehling wrote:
>> On 2023-09-28 10:30, Joshi, Mukul wrote:
>>> [AMD Official Use Only - General]
>>>
>>>> -----Original Message-----
>>>> From: Yadav, Arvind <Arvind.Yadav@amd.com>
>>>> Sent: Thursday, September 28, 2023 5:54 AM
>>>> To: Koenig, Christian <Christian.Koenig@amd.com>; Deucher, Alexander
>>>> <Alexander.Deucher@amd.com>; Sharma, Shashank
>>>> <Shashank.Sharma@amd.com>; Kuehling, Felix <Felix.Kuehling@amd.com>;
>>>> Joshi, Mukul <Mukul.Joshi@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>;
>>>> airlied@gmail.com; daniel@ffwll.ch
>>>> Cc: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; 
>>>> linux-
>>>> kernel@vger.kernel.org; Yadav, Arvind <Arvind.Yadav@amd.com>; Koenig,
>>>> Christian <Christian.Koenig@amd.com>
>>>> Subject: [PATCH v2 1/1] drm/amdkfd: Fix unaligned doorbell absolute 
>>>> offset
>>>> for gfx8
>>>>
>>>> This patch is to adjust the absolute doorbell offset against the 
>>>> doorbell id
>>>> considering the doorbell size of 32/64 bit.
>>>>
>>>> v2:
>>>> - Addressed the review comment from Felix.
>>>>
>>>> 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>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c | 9 ++++++++-
>>>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>>>
>>>> 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..c54c4392d26e 100644
>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
>>>> @@ -407,7 +407,14 @@ static int allocate_doorbell(struct
>>>> qcm_process_device *qpd,
>>>>
>>>>        q->properties.doorbell_off = amdgpu_doorbell_index_on_bar(dev-
>>>>> adev,
>>>>                                                                  qpd-
>>>>> proc_doorbells,
>>>> -                                                               q-
>>>>> doorbell_id);
>>>> +                                                               0);
>>>> +
>>> It looks like amdgpu_doorbell_index_on_bar() works only for 64-bit 
>>> doorbells.
>>> Shouldn't it work for both 32-bit and 64-bit doorbells considering 
>>> this is common
>>> doorbell manager code?
>
>
> Yes, You are right that the calculations to find a particular doorbell 
> in the doorbell page considers a doorbell width of 64-bit.
>
>>
>> I could see this argument going either way. KFD is the only one that 
>> cares about managing doorbells for user mode queues on GFXv8 GPUs. 
>> This is not a use case that amdgpu cares about. So I'm OK with KFD 
>> doing its own address calculations to make sure doorbells continue to 
>> work on GFXv8.
>>
>> It may not be worth adding complexity to the common doorbell manager 
>> code to support legacy GPUs with 32-bit doorbells.
>
>
> I was thinking about adding an additional input parameter which will 
> indicate if the doorbell width is 32-bit vs 64-bit (like 
> is_doorbell_64_bit), and doorbell manager can alter the multiplier 
> while calculating the final offset. Please let me know if that will 
> work for both the cases.

Yes, that would work for KFD because we already have the doorbell size 
in our device-info structure. Instead of making it a boolean flag, you 
could make it a doorbell_size parameter, in byte or dword units to 
simplify the pointer math.

Regards,
   Felix


>
> - Shashank
>
>>
>>
>> Regards,
>>   Felix
>>
>>
>>>
>>> Thanks,
>>> Mukul
>>>
>>>> +     /* Adjust the absolute doorbell offset against the doorbell id
>>>> considering
>>>> +      * the doorbell size of 32/64 bit.
>>>> +      */
>>>> +     q->properties.doorbell_off += q->doorbell_id *
>>>> + dev->kfd->device_info.doorbell_size / 4;
>>>> +
>>>>        return 0;
>>>>   }
>>>>
>>>> -- 
>>>> 2.34.1