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

["Christian König" <christian.koenig@amd.com>]

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.

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

And BTW please no comment after structure members.

Christian.

> +	uint64_t		doorbell_gpu_addr;
> +	struct amdgpu_bo	*doorbell_kernel_bo;
> +	u32			*doorbell_cpu_addr;
>   };
>   
>   struct amdgpu_copy_mem {