Re: [PATCH v3 06/12] drm/amdgpu: create kernel doorbell pages

[Shashank Sharma <shashank.sharma@xxxxxxx>]

On 21/06/2023 11:13, Christian König wrote:
> Am 21.06.23 um 11:10 schrieb Shashank Sharma:
> > Hey Christian,
> >
> > On 21/06/2023 10:32, Christian König wrote:
> > > Am 20.06.23 um 19:16 schrieb Shashank Sharma:
> > > > This patch:
> > > > - creates a doorbell page for graphics driver usages.
> > > > - adds a few new varlables in adev->doorbell structure to
> > > >    keep track of kernel's doorbell-bo.
> > > > - removes the adev->doorbell.ptr variable, replaces it with
> > > >    kernel-doorbell-bo's cpu address.
> > > >
> > > > V2: - Create doorbell BO directly, no wrappe functions (Alex)
> > > >      - no additional doorbell structure (Alex, Christian)
> > > >      - Use doorbell_cpu_ptr, remove ioremap (Christian, Alex)
> > > >      - Allocate one extra page of doorbells for MES (Alex)
> > > >
> > > > Cc: Alex Deucher <alexander.deucher@xxxxxxx>
> > > > Cc: Christian Koenig <christian.koenig@xxxxxxx>
> > > > Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxx>
> > > > ---
> > > >   drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell.h  |  8 ++-
> > > >   .../gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c  | 56 
> > > > ++++++++++++++-----
> > > >   drivers/gpu/drm/amd/amdgpu/amdgpu_mes.h       |  3 +
> > > >   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c       |  7 +++
> > > >   4 files changed, 60 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell.h 
> > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell.h
> > > > index 783e2b8b086c..002899edb9d7 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell.h
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell.h
> > > > @@ -31,10 +31,15 @@ struct amdgpu_doorbell {
> > > >       /* doorbell mmio */
> > > >       resource_size_t        base;
> > > >       resource_size_t        size;
> > > > -    u32 __iomem        *ptr;
> > > >         /* Number of doorbells reserved for amdgpu kernel driver */
> > > >       u32 num_kernel_doorbells;
> > > > +
> > > > +    /* Kernel doorbells */
> > > > +    struct amdgpu_bo *kernel_doorbells;
> > > > +
> > > > +    /* For CPU access of doorbells */
> > > > +    uint32_t *cpu_addr;
> > > >   };
> > > >     /* Reserved doorbells for amdgpu (including multimedia).
> > > > @@ -311,6 +316,7 @@ void amdgpu_mm_wdoorbell64(struct amdgpu_device 
> > > > *adev, u32 index, u64 v);
> > > >    */
> > > >   int amdgpu_doorbell_init(struct amdgpu_device *adev);
> > > >   void amdgpu_doorbell_fini(struct amdgpu_device *adev);
> > > > +int amdgpu_doorbell_create_kernel_doorbells(struct amdgpu_device 
> > > > *adev);
> > > >     #define RDOORBELL32(index) amdgpu_mm_rdoorbell(adev, (index))
> > > >   #define WDOORBELL32(index, v) amdgpu_mm_wdoorbell(adev, (index), 
> > > > (v))
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c 
> > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c
> > > > index eb113e38f5e9..118f4bed32fd 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c
> > > > @@ -39,7 +39,7 @@ u32 amdgpu_mm_rdoorbell(struct amdgpu_device 
> > > > *adev, u32 index)
> > > >           return 0;
> > > >         if (index < adev->doorbell.num_kernel_doorbells)
> > > > -        return readl(adev->doorbell.ptr + index);
> > > > +        return readl(adev->doorbell.cpu_addr + index);
> > > >         DRM_ERROR("reading beyond doorbell aperture: 0x%08x!\n", 
> > > > index);
> > > >       return 0;
> > > > @@ -61,7 +61,7 @@ void amdgpu_mm_wdoorbell(struct amdgpu_device 
> > > > *adev, u32 index, u32 v)
> > > >           return;
> > > >         if (index < adev->doorbell.num_kernel_doorbells)
> > > > -        writel(v, adev->doorbell.ptr + index);
> > > > +        writel(v, adev->doorbell.cpu_addr + index);
> > > >       else
> > > >           DRM_ERROR("writing beyond doorbell aperture: 0x%08x!\n", 
> > > > index);
> > > >   }
> > > > @@ -81,7 +81,7 @@ u64 amdgpu_mm_rdoorbell64(struct amdgpu_device 
> > > > *adev, u32 index)
> > > >           return 0;
> > > >         if (index < adev->doorbell.num_kernel_doorbells)
> > > > -        return atomic64_read((atomic64_t *)(adev->doorbell.ptr + 
> > > > index));
> > > > +        return atomic64_read((atomic64_t 
> > > > *)(adev->doorbell.cpu_addr + index));
> > > >         DRM_ERROR("reading beyond doorbell aperture: 0x%08x!\n", 
> > > > index);
> > > >       return 0;
> > > > @@ -103,11 +103,47 @@ void amdgpu_mm_wdoorbell64(struct 
> > > > amdgpu_device *adev, u32 index, u64 v)
> > > >           return;
> > > >         if (index < adev->doorbell.num_kernel_doorbells)
> > > > -        atomic64_set((atomic64_t *)(adev->doorbell.ptr + index), v);
> > > > +        atomic64_set((atomic64_t *)(adev->doorbell.cpu_addr + 
> > > > index), v);
> > > >       else
> > > >           DRM_ERROR("writing beyond doorbell aperture: 0x%08x!\n", 
> > > > index);
> > > >   }
> > > >   +/**
> > > > + * amdgpu_doorbell_create_kernel_doorbells - Create kernel 
> > > > doorbells for graphics
> > > > + *
> > > > + * @adev: amdgpu_device pointer
> > > > + *
> > > > + * Creates doorbells for graphics driver usages.
> > > > + * returns 0 on success, error otherwise.
> > > > + */
> > > > +int amdgpu_doorbell_create_kernel_doorbells(struct amdgpu_device 
> > > > *adev)
> > > > +{
> > > > +    int r;
> > > > +    int size;
> > > > +
> > > > +    /* Reserve first num_kernel_doorbells (page-aligned) for 
> > > > kernel ops */
> > > > +    size = ALIGN(adev->doorbell.num_kernel_doorbells * 
> > > > sizeof(u32), PAGE_SIZE);
> > > > +
> > > > +    /* Allocate an extra page for MES kernel usages (ring test) */
> > > > +    adev->mes.db_start_dw_offset = size / sizeof(u32);
> > > > +    size += PAGE_SIZE;
> > >
> > > This is initialized but not used, so I suspect it will temporary 
> > > break the MES test? Or is the MES just implicitly using the correct 
> > > offset somehow?
> >
> > I am not sure if I understand this comment properly, can you please 
> > elaborate ?
> >
> > If you are talking about 'size', its just used below while creating 
> > the doorbell BO.
> >
> > If you mean adev->mes.db_start_dw_offset, its being used in 
> > amdgpu_mes_kernel_doorbell_get/free and amdgpu_mes_doorbell_init
>
> I was talking about adev->mes.db_start_dw_offset. But using this in 
> amdgpu_mes_kernel_doorbell_get/free comes in a later patch, doesn't 
> it? If yes, doesn't that break the MES test?
Ah, I get it now, and you are right, I though it would be better for 
reviewer to see all KGD kernel level pages being created in the same 
patch, but now I feel it would have been far better to do this in MES 
kernel specific patch.

I will move this code in MES kernel doorbells page.

- Shashank

> Christian.
>
> > So both of the variables are being used.
> >
> > - Shashank
> >
> > > Apart from that it looks good to me,
> > > Christian.
> > >
> > > > +
> > > > +    r = amdgpu_bo_create_kernel(adev,
> > > > +                    size,
> > > > +                    PAGE_SIZE,
> > > > +                    AMDGPU_GEM_DOMAIN_DOORBELL,
> > > > + &adev->doorbell.kernel_doorbells,
> > > > +                    NULL,
> > > > +                    (void **)&adev->doorbell.cpu_addr);
> > > > +    if (r) {
> > > > +        DRM_ERROR("Failed to allocate kernel doorbells, err=%d\n", 
> > > > r);
> > > > +        return r;
> > > > +    }
> > > > +
> > > > +    adev->doorbell.num_kernel_doorbells = size / sizeof(u32);
> > > > +    return 0;
> > > > +}
> > > > +
> > > >   /*
> > > >    * GPU doorbell aperture helpers function.
> > > >    */
> > > > @@ -127,7 +163,6 @@ int amdgpu_doorbell_init(struct amdgpu_device 
> > > > *adev)
> > > >           adev->doorbell.base = 0;
> > > >           adev->doorbell.size = 0;
> > > >           adev->doorbell.num_kernel_doorbells = 0;
> > > > -        adev->doorbell.ptr = NULL;
> > > >           return 0;
> > > >       }
> > > >   @@ -156,12 +191,6 @@ int amdgpu_doorbell_init(struct 
> > > > amdgpu_device *adev)
> > > >       if (adev->asic_type >= CHIP_VEGA10)
> > > >           adev->doorbell.num_kernel_doorbells += 0x400;
> > > >   -    adev->doorbell.ptr = ioremap(adev->doorbell.base,
> > > > -                     adev->doorbell.num_kernel_doorbells *
> > > > -                     sizeof(u32));
> > > > -    if (adev->doorbell.ptr == NULL)
> > > > -        return -ENOMEM;
> > > > -
> > > >       return 0;
> > > >   }
> > > >   @@ -174,6 +203,7 @@ int amdgpu_doorbell_init(struct amdgpu_device 
> > > > *adev)
> > > >    */
> > > >   void amdgpu_doorbell_fini(struct amdgpu_device *adev)
> > > >   {
> > > > -    iounmap(adev->doorbell.ptr);
> > > > -    adev->doorbell.ptr = NULL;
> > > > + amdgpu_bo_free_kernel(&adev->doorbell.kernel_doorbells,
> > > > +                  NULL,
> > > > +                  (void **)&adev->doorbell.cpu_addr);
> > > >   }
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.h 
> > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.h
> > > > index 547ec35691fa..a403418d5eac 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.h
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.h
> > > > @@ -128,6 +128,9 @@ struct amdgpu_mes {
> > > >       int                             (*kiq_hw_init)(struct 
> > > > amdgpu_device *adev);
> > > >       int                             (*kiq_hw_fini)(struct 
> > > > amdgpu_device *adev);
> > > >   +    /* MES doorbells */
> > > > +    uint32_t            db_start_dw_offset;
> > > > +
> > > >       /* ip specific functions */
> > > >       const struct amdgpu_mes_funcs   *funcs;
> > > >   };
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
> > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > > > index 6d1587bbda52..c48e97646541 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > > > @@ -1854,6 +1854,13 @@ int amdgpu_ttm_init(struct amdgpu_device *adev)
> > > >           return r;
> > > >       }
> > > >   +    /* Create a boorbell page for kernel usages */
> > > > +    r = amdgpu_doorbell_create_kernel_doorbells(adev);
> > > > +    if (r) {
> > > > +        DRM_ERROR("Failed to initialize kernel doorbells.\n");
> > > > +        return r;
> > > > +    }
> > > > +
> > > >       /* Initialize preemptible memory pool */
> > > >       r = amdgpu_preempt_mgr_init(adev);
> > > >       if (r) {