| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <0d546c7c-2302-8d51-e37d-da47d9f5a39f@amd.com>
Date: Wed, 4 Oct 2023 22:47:10 +0530
From: "Yadav, Arvind" <arvyadav@....com>
To: Felix Kuehling <felix.kuehling@....com>,
Arvind Yadav <Arvind.Yadav@....com>, Christian.Koenig@....com,
alexander.deucher@....com, shashank.sharma@....com,
Mukul.Joshi@....com, Xinhui.Pan@....com, airlied@...il.com,
daniel@...ll.ch
Cc: amd-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] drm/amdkfd: get doorbell's absolute offset based
on the db size
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@....com>
>> Cc: Alex Deucher <alexander.deucher@....com>
>> Signed-off-by: Shashank Sharma <shashank.sharma@....com>
>> Signed-off-by: Arvind Yadav <Arvind.Yadav@....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);
Powered by blists - more mailing lists