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

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

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@gmail.com>
>>>
>>> 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@amd.com>
>>> Cc: Christian Koenig <christian.koenig@amd.com>
>>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>>> Signed-off-by: Shashank Sharma <shashank.sharma@amd.com>
>>> ---
>>>   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 {
>>