[PATCH 11/16] drm/amdgpu: get absolute offset from doorbell index

[Shashank Sharma]

On 30/03/2023 19:18, Luben Tuikov wrote:
> On 2023-03-29 11:47, Shashank Sharma wrote:
>> This patch adds a helper function which converts a doorbell's
>> relative index in a BO to an absolute doorbell offset in the
>> doorbell BAR.
>>
>> Cc: Alex Deucher <alexander.deucher at amd.com>
>> Cc: Christian Koenig <christian.koenig at amd.com>
>> Signed-off-by: Shashank Sharma <shashank.sharma at amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell.h  | 15 +++++++++++
>>   .../gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c  | 26 +++++++++++++++++++
>>   2 files changed, 41 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell.h
>> index 10a9bb10e974..3481e9d83879 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell.h
>> @@ -383,6 +383,21 @@ int amdgpu_doorbell_alloc_page(struct amdgpu_device *adev,
>>    */
>>   int amdgpu_doorbell_create_kernel_doorbells(struct amdgpu_device *adev);
>>   
>> +/**
>> + * amdgpu_doorbell_index_on_bar - Find doorbell's absolute offset in BAR
>> + *
>> + * @adev: amdgpu_device pointer
>> + *
>> + * @db_bo: doorbell object's bo
>> + *
>> + * @db_index: doorbell relative index in this doorbell object
>> + *
>> + * returns doorbell's absolute index in BAR
>> + */
>> +uint32_t amdgpu_doorbell_index_on_bar(struct amdgpu_device *adev,
>> +				       struct amdgpu_bo *db_bo,
>> +				       uint32_t doorbell_index);
>> +
> Two things:
> 1. No kernel doc for function declarations--this should go to where
> the function is defined. (This also removes redundancy.)
>
> 2. No empty lines around function arguments in kernel doc. See this about
> the format of function documentation:
> https://www.kernel.org/doc/html/v4.12/doc-guide/kernel-doc.html#function-documentation

Noted, agreed.

>>   #define RDOORBELL32(index) amdgpu_mm_rdoorbell(adev, (index))
>>   #define WDOORBELL32(index, v) amdgpu_mm_wdoorbell(adev, (index), (v))
>>   #define RDOORBELL64(index) amdgpu_mm_rdoorbell64(adev, (index))
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c
>> index 81713b2c28e1..c263bae6b0c4 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c
>> @@ -130,6 +130,32 @@ void amdgpu_mm_wdoorbell64(struct amdgpu_device *adev, u32 index, u64 v)
>>   	}
>>   }
>>   
>> +/**
>> + * amdgpu_doorbell_index_on_bar - Find doorbell's absolute offset in BAR
>> + *
>> + * @adev: amdgpu_device pointer
>> + *
>> + * @db_bo: doorbell object's bo
>> + *
>> + * @db_index: doorbell relative index in this doorbell object
>> + *
>> + * returns doorbell's absolute index in BAR
>> + */
>> +uint32_t amdgpu_doorbell_index_on_bar(struct amdgpu_device *adev,
>> +				       struct amdgpu_bo *db_bo,
>> +				       uint32_t doorbell_index)
>> +{
>> +	int db_bo_offset;
>> +
>> +	db_bo_offset = amdgpu_bo_gpu_offset_no_check(db_bo);
> amdgpu_bo_gpu_offset_no_check() returns u64. Perhaps use u64,
> or u32 (which is what this function returns) and cast it down.

Yeah, makes sense, will do it.

- Shashank

>> +
>> +	/*
>> +	 * doorbell index granularity is maintained at 32 bit
>> +	 * but doorbell's size is 64-bit, so index * 2
>> +	 */
>> +	return db_bo_offset / sizeof(u32) + doorbell_index * 2;
> Perhaps add this inside the comment:
> * (db_bo_offset + doorbell_index * 8) / sizeof(u32),
> which seems clearer to me. But leave the return as is.
> Regards,
> Luben
>
>> +}
>> +
>>   /**
>>    * amdgpu_doorbell_free_page - Free a doorbell page
>>    *