Re: [PATCH v2 8/8] drm/amdgpu: start using kernel doorbell bo

[Christian König <christian.koenig@xxxxxxx>]

Am 14.02.23 um 20:28 schrieb Shashank Sharma:
> On 14/02/2023 19:40, Christian König wrote:
> > Am 14.02.23 um 17:15 schrieb Shashank Sharma:
> > > From: Shashank Sharma <contactshashanksharma@xxxxxxxxx>
> > >
> > > This patch does the following:
> > >
> > > - Adds new variables like mman.doorbell_bo_size/gpu_addr/cpu_addr.
> > >    The cpu_addr ptr will be used now for doorbell read/write from
> > >    doorbell BAR.
> > > - Adjusts the existing code to use kernel doorbell BO's size and its
> > >    cpu_address.
> > >
> > > Cc: Alex Deucher <alexander.deucher@xxxxxxx>
> > > Cc: Christian Koenig <christian.koenig@xxxxxxx>
> > > Signed-off-by: Alex Deucher <alexander.deucher@xxxxxxx>
> > > Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxx>
> >
> > Maybe squash this one together with the previous patch.
>
> I just split it from the last patch in this series, thought it was too 
> scattered and might not be
>
> easy to review :D

Yeah, ok good point as well :D

Christian.

> > But see below.
> >
> > > ---
> > >   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c   |  5 ++-
> > >   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c   | 33 
> > > +++++++++-----------
> > >   drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell.h |  1 -
> > >   3 files changed, 16 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c 
> > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> > > index 0493c64e9d0a..87f486f522ae 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->doorbell.doorbell_aper_base;
> > >           *aperture_size = 0;
> > >           *start_offset = 0;
> > > -    } else if (adev->doorbell.doorbell_aper_size > 
> > > adev->doorbell.num_doorbells *
> > > -                        sizeof(u32)) {
> > > +    } else if (adev->doorbell.doorbell_aper_size > 
> > > adev->mman.doorbell_kernel_bo_size) {
> > >           *aperture_base = adev->doorbell.doorbell_aper_base;
> > >           *aperture_size = adev->doorbell.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 43c1b67c2778..fde199434579 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > @@ -596,8 +596,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_kernel_doorbells) {
> > > +        return readl(adev->mman.doorbell_cpu_addr + index);
> > >       } else {
> > >           DRM_ERROR("reading beyond doorbell aperture: 0x%08x!\n", 
> > > index);
> > >           return 0;
> > > @@ -619,8 +619,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_kernel_doorbells) {
> > > +        writel(v, adev->mman.doorbell_cpu_addr + index);
> > >       } else {
> > >           DRM_ERROR("writing beyond doorbell aperture: 0x%08x!\n", 
> > > index);
> > >       }
> > > @@ -640,8 +640,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_kernel_doorbells) {
> > > +        return atomic64_read((atomic64_t 
> > > *)(adev->mman.doorbell_cpu_addr + index));
> > >       } else {
> > >           DRM_ERROR("reading beyond doorbell aperture: 0x%08x!\n", 
> > > index);
> > >           return 0;
> > > @@ -663,8 +663,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_kernel_doorbells) {
> > > +        atomic64_set((atomic64_t *)(adev->mman.doorbell_cpu_addr + 
> > > index), v);
> > >       } else {
> > >           DRM_ERROR("writing beyond doorbell aperture: 0x%08x!\n", 
> > > index);
> > >       }
> > > @@ -1037,7 +1037,7 @@ static int amdgpu_device_doorbell_init(struct 
> > > amdgpu_device *adev)
> > >       if (adev->asic_type < CHIP_BONAIRE) {
> > >           adev->doorbell.doorbell_aper_base = 0;
> > >           adev->doorbell.doorbell_aper_size = 0;
> > > -        adev->doorbell.num_doorbells = 0;
> > > +        adev->mman.num_kernel_doorbells = 0;
> > >           adev->mman.doorbell_aper_base_kaddr = NULL;
> > >           return 0;
> > >       }
> > > @@ -1052,13 +1052,13 @@ static int 
> > > amdgpu_device_doorbell_init(struct amdgpu_device *adev)
> > >       adev->doorbell.doorbell_aper_size = 
> > > pci_resource_len(adev->pdev, 2);
> > >         if (adev->enable_mes) {
> > > -        adev->doorbell.num_doorbells =
> > > +        adev->mman.num_kernel_doorbells =
> > >               adev->doorbell.doorbell_aper_size / sizeof(u32);
> > >       } else {
> > > -        adev->doorbell.num_doorbells =
> > > +        adev->mman.num_kernel_doorbells =
> > >               min_t(u32, adev->doorbell.doorbell_aper_size / 
> > > sizeof(u32),
> > >                     adev->doorbell_index.max_assignment+1);
> > > -        if (adev->doorbell.num_doorbells == 0)
> > > +        if (adev->mman.num_kernel_doorbells == 0)
> > >               return -EINVAL;
> > >             /* For Vega, reserve and map two pages on doorbell BAR 
> > > since SDMA
> > > @@ -1068,15 +1068,10 @@ static int 
> > > amdgpu_device_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_kernel_doorbells += 0x400;
> > >       }
> > >   -    adev->mman.doorbell_aper_base_kaddr = 
> > > ioremap(adev->doorbell.doorbell_aper_base,
> > > -                              adev->doorbell.num_doorbells *
> > > -                              sizeof(u32));
> > > -    if (adev->mman.doorbell_aper_base_kaddr == NULL)
> > > -        return -ENOMEM;
> > > -
> > > +    adev->mman.doorbell_kernel_bo_size = 
> > > adev->mman.num_kernel_doorbells * sizeof(u32);
> >
> > I would just keep the kernel_bo_size around and make the 
> > num_kernel_doorbells a local variable.
> Noted,
>
> - Shashank
>
> > Christian.
> >
> > >       return 0;
> > >   }
> > >   diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell.h 
> > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell.h
> > > index 526b6b4a86dd..7bdff4f926ad 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell.h
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell.h
> > > @@ -28,7 +28,6 @@ struct amdgpu_doorbell {
> > >       /* doorbell mmio */
> > >       resource_size_t        doorbell_aper_base;
> > >       resource_size_t        doorbell_aper_size;
> > > -    u32            num_doorbells;    /* Number of doorbells 
> > > actually reserved for amdgpu. */
> > >   };
> > >     /* Reserved doorbells for amdgpu (including multimedia).