Messages in this thread | | | Date | Thu, 28 Sep 2023 14:53:29 -0400 | Subject | Re: [PATCH v2 1/1] drm/amdkfd: Fix unaligned doorbell absolute offset for gfx8 | From | 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
| |