Re: [PATCH v2 7/8] drm/amdgpu: create doorbell kernel object

[Shashank Sharma <shashank.sharma@xxxxxxx>]

On 14/02/2023 20:26, Shashank Sharma wrote:
> On 14/02/2023 19:35, Christian König wrote:
> > Am 14.02.23 um 17:15 schrieb Shashank Sharma:
> > > From: Shashank Sharma <contactshashanksharma@xxxxxxxxx>
> > >
> > > This patch does the following:
> > > - Initializes TTM range management for domain DOORBELL.
> > > - Introduces a kernel bo for doorbell management in form of 
> > > mman.doorbell_kernel_bo.
> > >    This bo holds the kernel doorbell space now.
> > > - Removes ioremapping of doorbell-kernel memory, as its not required 
> > > now.
> > >
> > > V2:
> > > - Addressed review comments from Christian:
> > >      - do not use kernel_create_at(0), use kernel_create() instead.
> > >      - do not use ttm_resource_manager, use range_manager instead.
> > >      - do not ioremap doorbell, TTM will do that.
> > > - Split one big patch into 2
> > >
> > > 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>
> > > ---
> > >   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 22 ++++++++++++++++++++++
> > >   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h |  7 +++++++
> > >   2 files changed, 29 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
> > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > > index e9dc24191fc8..086e83c17c0f 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > > @@ -1879,12 +1879,32 @@ int amdgpu_ttm_init(struct amdgpu_device *adev)
> > >           return r;
> > >       }
> > >   +    r = amdgpu_ttm_init_on_chip(adev, AMDGPU_PL_DOORBELL, 
> > > adev->doorbell.doorbell_aper_size);
> > > +    if (r) {
> > > +        DRM_ERROR("Failed initializing oa heap.\n");
> > > +        return r;
> > > +    }
> > > +
> > >       if (amdgpu_bo_create_kernel(adev, PAGE_SIZE, PAGE_SIZE,
> > >                   AMDGPU_GEM_DOMAIN_GTT,
> > >                   &adev->mman.sdma_access_bo, NULL,
> > >                   &adev->mman.sdma_access_ptr))
> > >           DRM_WARN("Debug VRAM access will use slowpath MM access\n");
> > >   +    /* Create a doorbell BO for kernel usages */
> > > +    r = amdgpu_bo_create_kernel(adev,
> > > +                    adev->mman.doorbell_kernel_bo_size,
> > > +                    PAGE_SIZE,
> > > +                    AMDGPU_GEM_DOMAIN_DOORBELL,
> > > +                    &adev->mman.doorbell_kernel_bo,
> > > +                    &adev->mman.doorbell_gpu_addr,
> > > +                    (void **)&adev->mman.doorbell_cpu_addr);
> > > +
> > > +    if (r) {
> > > +        DRM_ERROR("Failed to create doorbell BO, err=%d\n", r);
> > > +        return r;
> > > +    }
> > > +
> >
> > I would even move this before the SDMA VRAM buffer since the later is 
> > only nice to have while the doorbell is mandatory to have.
> Agree,
> > >       return 0;
> > >   }
> > >   @@ -1908,6 +1928,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, (void **)&adev->mman.doorbell_cpu_addr);
> > >       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 9cf5d8419965..50748ff1dd3c 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> > > @@ -97,6 +97,13 @@ 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_kernel_doorbells;    /* Number of doorbells 
> > > actually reserved for kernel */
> > > +    uint64_t        doorbell_kernel_bo_size;
> >
> > That looks like duplicated information. We should only keep either 
> > the number of kernel doorbells or the kernel doorbell bo size around, 
> > not both.
> Yeah, agree. I can remove one of these two.

On a second thought, while doing some experiments with doorbells I 
realized that we might want to keep both of these, as:

num_kernel_doorbell = actual doorbells reserved for kernel,

doorbell_kernel_bo_size = max (PAGE_SIZE,  num_kernel_doorbell* sizeof(u32))

I will have to update the code to reflect that as well.


- Shashank

> > And BTW please no comment after structure members.
> Noted,
>
> - Shashank
>
> > Christian.
> >
> > > +    uint64_t        doorbell_gpu_addr;
> > > +    struct amdgpu_bo    *doorbell_kernel_bo;
> > > +    u32            *doorbell_cpu_addr;
> > >   };
> > >     struct amdgpu_copy_mem {