Re: [PATCH 07/16] drm/amdgpu: add helper to create doorbell pages

[Shashank Sharma <shashank.sharma@amd.com>]

On 30/03/2023 16:55, Alex Deucher wrote:
> On Thu, Mar 30, 2023 at 10:34 AM Shashank Sharma
> <shashank.sharma@amd.com> wrote:
>>
>> On 30/03/2023 16:15, Luben Tuikov wrote:
>>> On 2023-03-30 10:04, Shashank Sharma wrote:
>>>> On 30/03/2023 15:42, Luben Tuikov wrote:
>>>>> On 2023-03-29 11:47, Shashank Sharma wrote:
>>>>>> From: Shashank Sharma <contactshashanksharma@gmail.com>
>>>>>>
>>>>>> This patch adds helper functions to create and free doorbell
>>>>>> pages for kernel objects.
>>>>>>
>>>>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>>>>> Cc: Christian Koenig <christian.koenig@amd.com>
>>>>>> Signed-off-by: Shashank Sharma <shashank.sharma@amd.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell.h  | 41 ++++++++++++++++
>>>>>>     .../gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c  | 49 +++++++++++++++++++
>>>>>>     2 files changed, 90 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell.h
>>>>>> index f9c3b77bf65d..6581b78fe438 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell.h
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell.h
>>>>>> @@ -27,6 +27,24 @@
>>>>>>     /*
>>>>>>      * GPU doorbell structures, functions & helpers
>>>>>>      */
>>>>>> +
>>>>>> +/* Structure to hold doorbell pages from PCI doorbell BAR */
>>>>>> +struct amdgpu_doorbell_obj {
>>>>> In the comment you say "Structure to hold ...";
>>>>> it is a C structure, but then in the name of a function we see "obj".
>>>>> (Object is something which is defined like in memory, i.e. it exists, not
>>>>> something which is only declared.)
>>>>> This is just a declaration of a structure, not an object per se.
>>>>> I'd call it "struct amdgpu_doorbell_struct" or just "struct amdgpu_doorbell".
>>>> It is similar to struct amdgpu buffer object (struct amdgpu_bo), and
>>>> many more existing structure.
>>> The amdpgu_bo is very different than a structure describing a doorbell.
>>> The doorbell description isn't really "an object". I understand
>>> the enthusiasm, but it is really not "an object". It's just a doorbell
>>> description. :-)
>> amdgpu_bo is page of ttm_memory with additional information,
>>
>> amdgpu_doorbell_obj is a page of ttm_doorbells with additional information
>>
>> (it is not just one doorbell description)
>>
>> I don't see a problem here.
> I find the new API confusing.  I would expect to see
> amdgpu_bo_create_kernel(...DOORBELL...), amdgpu_bo_reserve(),
> amdgpu_bo_kmap(), etc.  That makes it consistent with the other
> resource pools that we manage in ttm.

I am assuming here you are talking about why do we need 
amdgpu_doorbell_page_create()/free() API here.

The wrappers here allow us to do additional book keeping work.

For example:

- We need to validate kernel doorbell writes, which means we need the 
range of kernel doorbells.

- There are 3 kernel doorbell pages, for KGD, KFD and MES. These are non 
consecutive pages.

- While we do doorbell_write in kernel, we need to check if this index 
is correct, which means:

kgd_doobrell_min < index < kgd_doorbell_max

kfd_doobrell_min < index < kgd_doorbell_max

mes_doobrell_min < index < kgd_doorbell_max

- which means we need start/end limits set at every object.

- we have to either do this work at each place where we want to call 
amdgpu_bo_create(DOORBELL)

   which means KFD, KGD and MES all places (which will look irrelevant 
in the context)

  or we can do this in one place, which is the doorbell wrapper API.


Please see patch 10 for this range check.


- Shashank


now kfd is setting start/end limits by calling 
amdgpu_get_doorbell_index() function.

>
> Alex
>
>> - Shashank
>>
>>> Regards,
>>> Luben
>>>
>>>> - Shashank
>>>>
>>>>> Then in the definition, you can call it an object/objects, if you'd like,
>>>>> like "struct amdgpu_doorbell *doorb_object[];" then you can say
>>>>> "db = doorb_object[i]";
>>>>>
>>>>> Regards,
>>>>> Luben
>>>>>
>>>>>> +  struct amdgpu_bo *bo;
>>>>>> +  uint64_t gpu_addr;
>>>>>> +  uint32_t *cpu_addr;
>>>>>> +  uint32_t size;
>>>>>> +
>>>>>> +  /* First index in this object */
>>>>>> +  uint32_t start;
>>>>>> +
>>>>>> +  /* Last index in this object */
>>>>>> +  uint32_t end;
>>>>>> +
>>>>>> +  /* bitmap for dynamic doorbell allocation from this object */
>>>>>> +  unsigned long *doorbell_bitmap;
>>>>>> +};
>>>>>> +
>>>>>>     struct amdgpu_doorbell {
>>>>>>             /* doorbell mmio */
>>>>>>             resource_size_t         base;
>>>>>> @@ -328,6 +346,29 @@ int amdgpu_device_doorbell_init(struct amdgpu_device *adev);
>>>>>>      */
>>>>>>     void amdgpu_device_doorbell_fini(struct amdgpu_device *adev);
>>>>>>
>>>>>> +/**
>>>>>> + * amdgpu_doorbell_free_page - Free a doorbell page
>>>>>> + *
>>>>>> + * @adev: amdgpu_device pointer
>>>>>> + *
>>>>>> + * @db_age: previously allocated doobell page details
>>>>>> + *
>>>>>> + */
>>>>>> +void amdgpu_doorbell_free_page(struct amdgpu_device *adev,
>>>>>> +                          struct amdgpu_doorbell_obj *db_obj);
>>>>>> +
>>>>>> +/**
>>>>>> + * amdgpu_doorbell_alloc_page - create a page from doorbell pool
>>>>>> + *
>>>>>> + * @adev: amdgpu_device pointer
>>>>>> + *
>>>>>> + * @db_age: doobell page structure to fill details with
>>>>>> + *
>>>>>> + * returns 0 on success, else error number
>>>>>> + */
>>>>>> +int amdgpu_doorbell_alloc_page(struct amdgpu_device *adev,
>>>>>> +                          struct amdgpu_doorbell_obj *db_obj);
>>>>>> +
>>>>>>     #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 1aea92363fd3..8be15b82b545 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c
>>>>>> @@ -111,6 +111,55 @@ void amdgpu_mm_wdoorbell64(struct amdgpu_device *adev, u32 index, u64 v)
>>>>>>             }
>>>>>>     }
>>>>>>
>>>>>> +/**
>>>>>> + * amdgpu_doorbell_free_page - Free a doorbell page
>>>>>> + *
>>>>>> + * @adev: amdgpu_device pointer
>>>>>> + *
>>>>>> + * @db_age: previously allocated doobell page details
>>>>>> + *
>>>>>> + */
>>>>>> +void amdgpu_doorbell_free_page(struct amdgpu_device *adev,
>>>>>> +                                  struct amdgpu_doorbell_obj *db_obj)
>>>>>> +{
>>>>>> +  amdgpu_bo_free_kernel(&db_obj->bo,
>>>>>> +                        &db_obj->gpu_addr,
>>>>>> +                        (void **)&db_obj->cpu_addr);
>>>>>> +
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>> + * amdgpu_doorbell_alloc_page - create a page from doorbell pool
>>>>>> + *
>>>>>> + * @adev: amdgpu_device pointer
>>>>>> + *
>>>>>> + * @db_age: doobell page structure to fill details with
>>>>>> + *
>>>>>> + * returns 0 on success, else error number
>>>>>> + */
>>>>>> +int amdgpu_doorbell_alloc_page(struct amdgpu_device *adev,
>>>>>> +                          struct amdgpu_doorbell_obj *db_obj)
>>>>>> +{
>>>>>> +  int r;
>>>>>> +
>>>>>> +  db_obj->size = ALIGN(db_obj->size, PAGE_SIZE);
>>>>>> +
>>>>>> +  r = amdgpu_bo_create_kernel(adev,
>>>>>> +                              db_obj->size,
>>>>>> +                              PAGE_SIZE,
>>>>>> +                              AMDGPU_GEM_DOMAIN_DOORBELL,
>>>>>> +                              &db_obj->bo,
>>>>>> +                              &db_obj->gpu_addr,
>>>>>> +                              (void **)&db_obj->cpu_addr);
>>>>>> +
>>>>>> +  if (r) {
>>>>>> +          DRM_ERROR("Failed to create doorbell BO, err=%d\n", r);
>>>>>> +          return r;
>>>>>> +  }
>>>>>> +
>>>>>> +  return 0;
>>>>>> +}
>>>>>> +
>>>>>>     /*
>>>>>>      * GPU doorbell aperture helpers function.
>>>>>>      */