Re: [PATCH 14/14] drm/amdgpu: introduce doorbell bo in kernel

[Shashank Sharma <shashank.sharma@xxxxxxx>]

On 06/02/2023 17:57, Christian König wrote:
> Am 03.02.23 um 20:08 schrieb Shashank Sharma:
> > From: Alex Deucher <alexander.deucher@xxxxxxx>
> >
> > This patch does the following:
> > - Removes doorbell ptr from adev.
> > - Moves doorbell.num_doorbells into mman.num_doorbells.
> > - Introduces a kernel bo for doorbell management in form of 
> > mman.doorbell_kernel_bo.
> >    This bo holds the doorbell space now.
> > - Introduces mman.doorbell_bo_size: to save total doorbell memory size.
> > - Also introduces mman.doorbell_va to save CPU address of doorbell BO
> >    mapping. This ptr will be used now for doorbell read/write from 
> > doorbell BAR.
>
> Please don't call any CPU address VA, this is a term we usually use 
> with GPUVM.
>
> Rather use doorbell_cpu_addr for this.
>
> Christian.

Noted,

- Shashank

> > Signed-off-by: Alex Deucher <alexander.deucher@xxxxxxx>
> > Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxx>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu.h          |  1 -
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c   |  5 ++--
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c   | 16 ++++++-------
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell.h |  4 ----
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c      | 24 ++++++++++++++++----
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h      |  6 +++++
> >   6 files changed, 35 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > index e1c1a360614e..dd3875ae1ad3 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > @@ -834,7 +834,6 @@ struct amdgpu_device {
> >       spinlock_t audio_endpt_idx_lock;
> >       amdgpu_block_rreg_t        audio_endpt_rreg;
> >       amdgpu_block_wreg_t        audio_endpt_wreg;
> > -    struct amdgpu_doorbell        doorbell;
> >         /* clock/pll info */
> >       struct amdgpu_clock            clock;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> > index 28076da2258f..aea943e337df 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> > @@ -109,11 +109,10 @@ static void amdgpu_doorbell_get_kfd_info(struct 
> > amdgpu_device *adev,
> >           *aperture_base = adev->gmc.doorbell_aper_base;
> >           *aperture_size = 0;
> >           *start_offset = 0;
> > -    } else if (adev->gmc.doorbell_aper_size > 
> > adev->doorbell.num_doorbells *
> > -                        sizeof(u32)) {
> > +    } else if (adev->gmc.doorbell_aper_size > 
> > adev->mman.doorbell_kernel_bo_size) {
> >           *aperture_base = adev->gmc.doorbell_aper_base;
> >           *aperture_size = adev->gmc.doorbell_aper_size;
> > -        *start_offset = adev->doorbell.num_doorbells * sizeof(u32);
> > +        *start_offset = adev->mman.doorbell_kernel_bo_size;
> >       } else {
> >           *aperture_base = 0;
> >           *aperture_size = 0;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index 7c21ffe63ebc..9c3ce1ae66ea 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -593,8 +593,8 @@ u32 amdgpu_mm_rdoorbell(struct amdgpu_device 
> > *adev, u32 index)
> >       if (amdgpu_device_skip_hw_access(adev))
> >           return 0;
> >   -    if (index < adev->doorbell.num_doorbells) {
> > -        return readl(adev->mman.doorbell_aper_base_kaddr + index);
> > +    if (index < adev->mman.num_doorbells) {
> > +        return readl(adev->mman.doorbell_va + index);
> >       } else {
> >           DRM_ERROR("reading beyond doorbell aperture: 0x%08x!\n", 
> > index);
> >           return 0;
> > @@ -616,8 +616,8 @@ void amdgpu_mm_wdoorbell(struct amdgpu_device 
> > *adev, u32 index, u32 v)
> >       if (amdgpu_device_skip_hw_access(adev))
> >           return;
> >   -    if (index < adev->doorbell.num_doorbells) {
> > -        writel(v, adev->mman.doorbell_aper_base_kaddr + index);
> > +    if (index < adev->mman.num_doorbells) {
> > +        writel(v, adev->mman.doorbell_va + index);
> >       } else {
> >           DRM_ERROR("writing beyond doorbell aperture: 0x%08x!\n", 
> > index);
> >       }
> > @@ -637,8 +637,8 @@ u64 amdgpu_mm_rdoorbell64(struct amdgpu_device 
> > *adev, u32 index)
> >       if (amdgpu_device_skip_hw_access(adev))
> >           return 0;
> >   -    if (index < adev->doorbell.num_doorbells) {
> > -        return atomic64_read((atomic64_t 
> > *)(adev->mman.doorbell_aper_base_kaddr + index));
> > +    if (index < adev->mman.num_doorbells) {
> > +        return atomic64_read((atomic64_t *)(adev->mman.doorbell_va + 
> > index));
> >       } else {
> >           DRM_ERROR("reading beyond doorbell aperture: 0x%08x!\n", 
> > index);
> >           return 0;
> > @@ -660,8 +660,8 @@ void amdgpu_mm_wdoorbell64(struct amdgpu_device 
> > *adev, u32 index, u64 v)
> >       if (amdgpu_device_skip_hw_access(adev))
> >           return;
> >   -    if (index < adev->doorbell.num_doorbells) {
> > -        atomic64_set((atomic64_t 
> > *)(adev->mman.doorbell_aper_base_kaddr + index), v);
> > +    if (index < adev->mman.num_doorbells) {
> > +        atomic64_set((atomic64_t *)(adev->mman.doorbell_va + index), 
> > v);
> >       } else {
> >           DRM_ERROR("writing beyond doorbell aperture: 0x%08x!\n", 
> > index);
> >       }
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell.h 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell.h
> > index 464be28da4fb..97298a98561f 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell.h
> > @@ -24,10 +24,6 @@
> >   /*
> >    * GPU doorbell structures, functions & helpers
> >    */
> > -struct amdgpu_doorbell {
> > -    u32            num_doorbells;    /* Number of doorbells actually 
> > reserved for amdgpu. */
> > -};
> > -
> >   /* Reserved doorbells for amdgpu (including multimedia).
> >    * KFD can use all the rest in the 2M doorbell bar.
> >    * For asic before vega10, doorbell is 32-bit, so the
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > index e38720b91958..8f0537f16cc8 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > @@ -1742,7 +1742,7 @@ static int amdgpu_ttm_doorbell_init(struct 
> > amdgpu_device *adev)
> >       if (adev->asic_type < CHIP_BONAIRE) {
> >           adev->gmc.doorbell_aper_base = 0;
> >           adev->gmc.doorbell_aper_size = 0;
> > -        adev->doorbell.num_doorbells = 0;
> > +        adev->mman.num_doorbells = 0;
> >           adev->mman.doorbell_aper_base_kaddr = NULL;
> >           return 0;
> >       }
> > @@ -1759,13 +1759,13 @@ static int amdgpu_ttm_doorbell_init(struct 
> > amdgpu_device *adev)
> > adev->gmc.doorbell_aper_size);
> >         if (adev->enable_mes) {
> > -        adev->doorbell.num_doorbells =
> > +        adev->mman.num_doorbells =
> >               adev->gmc.doorbell_aper_size / sizeof(u32);
> >       } else {
> > -        adev->doorbell.num_doorbells =
> > +        adev->mman.num_doorbells =
> >               min_t(u32, adev->gmc.doorbell_aper_size / sizeof(u32),
> >                     adev->doorbell_index.max_assignment+1);
> > -        if (adev->doorbell.num_doorbells == 0)
> > +        if (adev->mman.num_doorbells == 0)
> >               return -EINVAL;
> >             /* For Vega, reserve and map two pages on doorbell BAR 
> > since SDMA
> > @@ -1775,7 +1775,7 @@ static int amdgpu_ttm_doorbell_init(struct 
> > amdgpu_device *adev)
> >            * the max num_doorbells should + 1 page (0x400 in dword)
> >            */
> >           if (adev->asic_type >= CHIP_VEGA10)
> > -            adev->doorbell.num_doorbells += 0x400;
> > +            adev->mman.num_doorbells += 0x400;
> >       }
> >         return 0;
> > @@ -1901,6 +1901,18 @@ int amdgpu_ttm_init(struct amdgpu_device *adev)
> >       if (r)
> >           return r;
> >   +    adev->mman.doorbell_kernel_bo_size = adev->mman.num_doorbells 
> > * sizeof(u32);
> > +    r = amdgpu_bo_create_kernel_at(adev,
> > +                      0,
> > +                      adev->mman.doorbell_kernel_bo_size,
> > +                      AMDGPU_GEM_DOMAIN_DOORBELL,
> > +                      &adev->mman.doorbell_kernel_bo,
> > +                      &adev->mman.doorbell_va);
> > +    if (r) {
> > +        DRM_ERROR("Failed to create doorbell BO\n");
> > +        return r;
> > +    }
> > +
> >       DRM_INFO("amdgpu: %uM of VRAM memory ready\n",
> >            (unsigned) (adev->gmc.real_vram_size / (1024 * 1024)));
> >   @@ -1992,6 +2004,8 @@ void amdgpu_ttm_fini(struct amdgpu_device *adev)
> >                         NULL, NULL);
> >       amdgpu_bo_free_kernel(&adev->mman.sdma_access_bo, NULL,
> >                       &adev->mman.sdma_access_ptr);
> > + amdgpu_bo_free_kernel(&adev->mman.doorbell_kernel_bo,
> > +                  NULL, &adev->mman.doorbell_va);
> >       amdgpu_ttm_fw_reserve_vram_fini(adev);
> >       amdgpu_ttm_drv_reserve_vram_fini(adev);
> >   diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> > index a492de079c2e..405294ca9834 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> > @@ -98,6 +98,12 @@ struct amdgpu_mman {
> >       /* PAGE_SIZE'd BO for process memory r/w over SDMA. */
> >       struct amdgpu_bo    *sdma_access_bo;
> >       void            *sdma_access_ptr;
> > +
> > +    /* doorbells reserved for the kernel driver */
> > +    u32            num_doorbells;    /* Number of doorbells actually 
> > reserved for amdgpu. */
> > +    uint64_t        doorbell_kernel_bo_size;
> > +    struct amdgpu_bo    *doorbell_kernel_bo;
> > +    void            *doorbell_va;
> >   };
> >     struct amdgpu_copy_mem {