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

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

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.
>
> 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 {
>