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

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

[Arvind Yadav]

On older chips, the absolute doorbell offset within
the doorbell page is based on the queue ID.
KFD is using queue ID and doorbell size to get an
absolute doorbell offset in userspace.

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.

Arvind Yadav (1):
  drm/amdkfd: Fix unaligned doorbell absolute offset for gfx8

 drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

-- 
2.34.1

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

[Arvind Yadav]

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);
+
+	/* 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

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

[Joshi, Mukul]

[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?

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

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

[Felix Kuehling]

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?

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.

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

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

[Shashank Sharma]

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.

- 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

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

[Felix Kuehling]

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

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

[Shashank Sharma]

On 28/09/2023 20:53, Felix Kuehling wrote:
> 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.


Sounds good, will do that and send an update.

- Shashank

>
> 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