[PATCH] drm/amd/display: Implement functions to let DC allocate GPU memory

[PATCH] drm/amd/display: Implement functions to let DC allocate GPU memory

[Bhawanpreet Lakha]

From: Harry Wentland <harry.wentland@amd.com>

[Why]
DC needs to communicate with PM FW through GPU memory. In order
to do so we need to be able to allocate memory from within DC.

[How]
Call amdgpu_bo_create_kernel to allocate GPU memory and use a
list in amdgpu_display_manager to track our allocations so we
can clean them up later.

Signed-off-by: Harry Wentland <harry.wentland@amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 +
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  9 +++++
 .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 40 +++++++++++++++++--
 3 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index e490fc2486f7..83ec92a69cba 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -1017,6 +1017,8 @@ static int amdgpu_dm_init(struct amdgpu_device *adev)
 
 	init_data.soc_bounding_box = adev->dm.soc_bounding_box;
 
+	INIT_LIST_HEAD(&adev->dm.da_list);
+
 	/* Display Core create. */
 	adev->dm.dc = dc_create(&init_data);
 
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index 38bc0f88b29c..49137924a855 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -130,6 +130,13 @@ struct amdgpu_dm_backlight_caps {
 	bool aux_support;
 };
 
+struct dal_allocation {
+	struct list_head list;
+	struct amdgpu_bo *bo;
+	void *cpu_ptr;
+	u64 gpu_addr;
+};
+
 /**
  * struct amdgpu_display_manager - Central amdgpu display manager device
  *
@@ -350,6 +357,8 @@ struct amdgpu_display_manager {
 	 */
 	struct amdgpu_encoder mst_encoders[AMDGPU_DM_MAX_CRTC];
 	bool force_timing_sync;
+
+	struct list_head da_list;
 };
 
 enum dsc_clock_force_state {
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
index 3244a6ea7a65..5dc426e6e785 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
@@ -652,8 +652,31 @@ void *dm_helpers_allocate_gpu_mem(
 		size_t size,
 		long long *addr)
 {
-	// TODO
-	return NULL;
+	struct amdgpu_device *adev = ctx->driver_context;
+	struct dal_allocation *da;
+	u32 domain = (type == DC_MEM_ALLOC_TYPE_GART) ?
+		AMDGPU_GEM_DOMAIN_GTT : AMDGPU_GEM_DOMAIN_VRAM;
+	int ret;
+
+	da = kzalloc(sizeof(struct dal_allocation), GFP_KERNEL);
+	if (!da)
+		return NULL;
+
+	ret = amdgpu_bo_create_kernel(adev, size, PAGE_SIZE,
+				      domain, &da->bo,
+				      &da->gpu_addr, &da->cpu_ptr);
+
+	*addr = da->gpu_addr;
+
+	if (ret) {
+		kfree(da);
+		return NULL;
+	}
+
+	/* add da to list in dm */
+	list_add(&da->list, &adev->dm.da_list);
+
+	return da->cpu_ptr;
 }
 
 void dm_helpers_free_gpu_mem(
@@ -661,5 +684,16 @@ void dm_helpers_free_gpu_mem(
 		enum dc_gpu_mem_alloc_type type,
 		void *pvMem)
 {
-	// TODO
+	struct amdgpu_device *adev = ctx->driver_context;
+	struct dal_allocation *da;
+
+	/* walk the da list in DM */
+	list_for_each_entry(da, &adev->dm.da_list, list) {
+		if (pvMem == da->cpu_ptr) {
+			amdgpu_bo_free_kernel(&da->bo, &da->gpu_addr, &da->cpu_ptr);
+			list_del(&da->list);
+			kfree(da);
+			break;
+		}
+	}
 }
-- 
2.25.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: Implement functions to let DC allocate GPU memory

[Kazlauskas, Nicholas]

On 2021-01-19 3:40 p.m., Bhawanpreet Lakha wrote:
> From: Harry Wentland <harry.wentland@amd.com>
> 
> [Why]
> DC needs to communicate with PM FW through GPU memory. In order
> to do so we need to be able to allocate memory from within DC.
> 
> [How]
> Call amdgpu_bo_create_kernel to allocate GPU memory and use a
> list in amdgpu_display_manager to track our allocations so we
> can clean them up later.
> 
> Signed-off-by: Harry Wentland <harry.wentland@amd.com>

Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>

Regards,
Nicholas Kazlauskas

> ---
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 +
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  9 +++++
>   .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 40 +++++++++++++++++--
>   3 files changed, 48 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index e490fc2486f7..83ec92a69cba 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -1017,6 +1017,8 @@ static int amdgpu_dm_init(struct amdgpu_device *adev)
>   
>   	init_data.soc_bounding_box = adev->dm.soc_bounding_box;
>   
> +	INIT_LIST_HEAD(&adev->dm.da_list);
> +
>   	/* Display Core create. */
>   	adev->dm.dc = dc_create(&init_data);
>   
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index 38bc0f88b29c..49137924a855 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -130,6 +130,13 @@ struct amdgpu_dm_backlight_caps {
>   	bool aux_support;
>   };
>   
> +struct dal_allocation {
> +	struct list_head list;
> +	struct amdgpu_bo *bo;
> +	void *cpu_ptr;
> +	u64 gpu_addr;
> +};
> +
>   /**
>    * struct amdgpu_display_manager - Central amdgpu display manager device
>    *
> @@ -350,6 +357,8 @@ struct amdgpu_display_manager {
>   	 */
>   	struct amdgpu_encoder mst_encoders[AMDGPU_DM_MAX_CRTC];
>   	bool force_timing_sync;
> +
> +	struct list_head da_list;
>   };
>   
>   enum dsc_clock_force_state {
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> index 3244a6ea7a65..5dc426e6e785 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> @@ -652,8 +652,31 @@ void *dm_helpers_allocate_gpu_mem(
>   		size_t size,
>   		long long *addr)
>   {
> -	// TODO
> -	return NULL;
> +	struct amdgpu_device *adev = ctx->driver_context;
> +	struct dal_allocation *da;
> +	u32 domain = (type == DC_MEM_ALLOC_TYPE_GART) ?
> +		AMDGPU_GEM_DOMAIN_GTT : AMDGPU_GEM_DOMAIN_VRAM;
> +	int ret;
> +
> +	da = kzalloc(sizeof(struct dal_allocation), GFP_KERNEL);
> +	if (!da)
> +		return NULL;
> +
> +	ret = amdgpu_bo_create_kernel(adev, size, PAGE_SIZE,
> +				      domain, &da->bo,
> +				      &da->gpu_addr, &da->cpu_ptr);
> +
> +	*addr = da->gpu_addr;
> +
> +	if (ret) {
> +		kfree(da);
> +		return NULL;
> +	}
> +
> +	/* add da to list in dm */
> +	list_add(&da->list, &adev->dm.da_list);
> +
> +	return da->cpu_ptr;
>   }
>   
>   void dm_helpers_free_gpu_mem(
> @@ -661,5 +684,16 @@ void dm_helpers_free_gpu_mem(
>   		enum dc_gpu_mem_alloc_type type,
>   		void *pvMem)
>   {
> -	// TODO
> +	struct amdgpu_device *adev = ctx->driver_context;
> +	struct dal_allocation *da;
> +
> +	/* walk the da list in DM */
> +	list_for_each_entry(da, &adev->dm.da_list, list) {
> +		if (pvMem == da->cpu_ptr) {
> +			amdgpu_bo_free_kernel(&da->bo, &da->gpu_addr, &da->cpu_ptr);
> +			list_del(&da->list);
> +			kfree(da);
> +			break;
> +		}
> +	}
>   }
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm/amd/display: Implement functions to let DC allocate GPU memory

[Chen, Guchun]

[AMD Public Use]

+da = kzalloc(sizeof(struct dal_allocation), GFP_KERNEL);

This looks to be one coding style issue. It's better to modify it to kzalloc(sizeof(*da),...)

https://www.kernel.org/doc/html/latest/process/coding-style.html#allocating-memory

Regards,
Guchun

-----Original Message-----
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Bhawanpreet Lakha
Sent: Wednesday, January 20, 2021 4:41 AM
To: Deucher, Alexander <Alexander.Deucher@amd.com>
Cc: Wentland, Harry <Harry.Wentland@amd.com>; amd-gfx@lists.freedesktop.org
Subject: [PATCH] drm/amd/display: Implement functions to let DC allocate GPU memory

From: Harry Wentland <harry.wentland@amd.com>

[Why]
DC needs to communicate with PM FW through GPU memory. In order to do so we need to be able to allocate memory from within DC.

[How]
Call amdgpu_bo_create_kernel to allocate GPU memory and use a list in amdgpu_display_manager to track our allocations so we can clean them up later.

Signed-off-by: Harry Wentland <harry.wentland@amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 +  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  9 +++++  .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 40 +++++++++++++++++--
 3 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index e490fc2486f7..83ec92a69cba 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -1017,6 +1017,8 @@ static int amdgpu_dm_init(struct amdgpu_device *adev)
 
 	init_data.soc_bounding_box = adev->dm.soc_bounding_box;
 
+	INIT_LIST_HEAD(&adev->dm.da_list);
+
 	/* Display Core create. */
 	adev->dm.dc = dc_create(&init_data);
 
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index 38bc0f88b29c..49137924a855 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -130,6 +130,13 @@ struct amdgpu_dm_backlight_caps {
 	bool aux_support;
 };
 
+struct dal_allocation {
+	struct list_head list;
+	struct amdgpu_bo *bo;
+	void *cpu_ptr;
+	u64 gpu_addr;
+};
+
 /**
  * struct amdgpu_display_manager - Central amdgpu display manager device
  *
@@ -350,6 +357,8 @@ struct amdgpu_display_manager {
 	 */
 	struct amdgpu_encoder mst_encoders[AMDGPU_DM_MAX_CRTC];
 	bool force_timing_sync;
+
+	struct list_head da_list;
 };
 
 enum dsc_clock_force_state {
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
index 3244a6ea7a65..5dc426e6e785 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
@@ -652,8 +652,31 @@ void *dm_helpers_allocate_gpu_mem(
 		size_t size,
 		long long *addr)
 {
-	// TODO
-	return NULL;
+	struct amdgpu_device *adev = ctx->driver_context;
+	struct dal_allocation *da;
+	u32 domain = (type == DC_MEM_ALLOC_TYPE_GART) ?
+		AMDGPU_GEM_DOMAIN_GTT : AMDGPU_GEM_DOMAIN_VRAM;
+	int ret;
+
+	da = kzalloc(sizeof(struct dal_allocation), GFP_KERNEL);
+	if (!da)
+		return NULL;
+
+	ret = amdgpu_bo_create_kernel(adev, size, PAGE_SIZE,
+				      domain, &da->bo,
+				      &da->gpu_addr, &da->cpu_ptr);
+
+	*addr = da->gpu_addr;
+
+	if (ret) {
+		kfree(da);
+		return NULL;
+	}
+
+	/* add da to list in dm */
+	list_add(&da->list, &adev->dm.da_list);
+
+	return da->cpu_ptr;
 }
 
 void dm_helpers_free_gpu_mem(
@@ -661,5 +684,16 @@ void dm_helpers_free_gpu_mem(
 		enum dc_gpu_mem_alloc_type type,
 		void *pvMem)
 {
-	// TODO
+	struct amdgpu_device *adev = ctx->driver_context;
+	struct dal_allocation *da;
+
+	/* walk the da list in DM */
+	list_for_each_entry(da, &adev->dm.da_list, list) {
+		if (pvMem == da->cpu_ptr) {
+			amdgpu_bo_free_kernel(&da->bo, &da->gpu_addr, &da->cpu_ptr);
+			list_del(&da->list);
+			kfree(da);
+			break;
+		}
+	}
 }
--
2.25.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Cguchun.chen%40amd.com%7C85ba845fc320487d19cb08d8bcba7b46%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637466856421596284%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=kaDroqYYgq4ooRdvMDm93i%2BNbtvBGjdWKLd4Op1yemc%3D&amp;reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: Implement functions to let DC allocate GPU memory

[Christian König]

Am 19.01.21 um 21:40 schrieb Bhawanpreet Lakha:
> From: Harry Wentland <harry.wentland@amd.com>
>
> [Why]
> DC needs to communicate with PM FW through GPU memory. In order
> to do so we need to be able to allocate memory from within DC.
>
> [How]
> Call amdgpu_bo_create_kernel to allocate GPU memory and use a
> list in amdgpu_display_manager to track our allocations so we
> can clean them up later.

Well that looks like classic mid-layering to me with a horrible 
implementation of the free function.

Christian.

>
> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
> ---
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 +
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  9 +++++
>   .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 40 +++++++++++++++++--
>   3 files changed, 48 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index e490fc2486f7..83ec92a69cba 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -1017,6 +1017,8 @@ static int amdgpu_dm_init(struct amdgpu_device *adev)
>   
>   	init_data.soc_bounding_box = adev->dm.soc_bounding_box;
>   
> +	INIT_LIST_HEAD(&adev->dm.da_list);
> +
>   	/* Display Core create. */
>   	adev->dm.dc = dc_create(&init_data);
>   
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index 38bc0f88b29c..49137924a855 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -130,6 +130,13 @@ struct amdgpu_dm_backlight_caps {
>   	bool aux_support;
>   };
>   
> +struct dal_allocation {
> +	struct list_head list;
> +	struct amdgpu_bo *bo;
> +	void *cpu_ptr;
> +	u64 gpu_addr;
> +};
> +
>   /**
>    * struct amdgpu_display_manager - Central amdgpu display manager device
>    *
> @@ -350,6 +357,8 @@ struct amdgpu_display_manager {
>   	 */
>   	struct amdgpu_encoder mst_encoders[AMDGPU_DM_MAX_CRTC];
>   	bool force_timing_sync;
> +
> +	struct list_head da_list;
>   };
>   
>   enum dsc_clock_force_state {
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> index 3244a6ea7a65..5dc426e6e785 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> @@ -652,8 +652,31 @@ void *dm_helpers_allocate_gpu_mem(
>   		size_t size,
>   		long long *addr)
>   {
> -	// TODO
> -	return NULL;
> +	struct amdgpu_device *adev = ctx->driver_context;
> +	struct dal_allocation *da;
> +	u32 domain = (type == DC_MEM_ALLOC_TYPE_GART) ?
> +		AMDGPU_GEM_DOMAIN_GTT : AMDGPU_GEM_DOMAIN_VRAM;
> +	int ret;
> +
> +	da = kzalloc(sizeof(struct dal_allocation), GFP_KERNEL);
> +	if (!da)
> +		return NULL;
> +
> +	ret = amdgpu_bo_create_kernel(adev, size, PAGE_SIZE,
> +				      domain, &da->bo,
> +				      &da->gpu_addr, &da->cpu_ptr);
> +
> +	*addr = da->gpu_addr;
> +
> +	if (ret) {
> +		kfree(da);
> +		return NULL;
> +	}
> +
> +	/* add da to list in dm */
> +	list_add(&da->list, &adev->dm.da_list);
> +
> +	return da->cpu_ptr;
>   }
>   
>   void dm_helpers_free_gpu_mem(
> @@ -661,5 +684,16 @@ void dm_helpers_free_gpu_mem(
>   		enum dc_gpu_mem_alloc_type type,
>   		void *pvMem)
>   {
> -	// TODO
> +	struct amdgpu_device *adev = ctx->driver_context;
> +	struct dal_allocation *da;
> +
> +	/* walk the da list in DM */
> +	list_for_each_entry(da, &adev->dm.da_list, list) {
> +		if (pvMem == da->cpu_ptr) {
> +			amdgpu_bo_free_kernel(&da->bo, &da->gpu_addr, &da->cpu_ptr);
> +			list_del(&da->list);
> +			kfree(da);
> +			break;
> +		}
> +	}
>   }

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: Implement functions to let DC allocate GPU memory

[Kazlauskas, Nicholas]

On 2021-01-20 5:26 a.m., Christian König wrote:
> Am 19.01.21 um 21:40 schrieb Bhawanpreet Lakha:
>> From: Harry Wentland <harry.wentland@amd.com>
>>
>> [Why]
>> DC needs to communicate with PM FW through GPU memory. In order
>> to do so we need to be able to allocate memory from within DC.
>>
>> [How]
>> Call amdgpu_bo_create_kernel to allocate GPU memory and use a
>> list in amdgpu_display_manager to track our allocations so we
>> can clean them up later.
> 
> Well that looks like classic mid-layering to me with a horrible 
> implementation of the free function.
> 
> Christian.

FWIW this is only really used during device creation and destruction so 
the overhead of the free function isn't a considerable concern.

Does AMDGPU always need to know the GPU address for the allocation to 
free or should we work on fixing the callsites for this to pass it down?

We generally separate the CPU/GPU pointer but maybe it'd be best to have 
some sort of allocation object here that has both for DC's purposes.

Regards,
Nicholas Kazlauskas

> 
>>
>> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
>> ---
>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 +
>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  9 +++++
>>   .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 40 +++++++++++++++++--
>>   3 files changed, 48 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index e490fc2486f7..83ec92a69cba 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -1017,6 +1017,8 @@ static int amdgpu_dm_init(struct amdgpu_device 
>> *adev)
>>       init_data.soc_bounding_box = adev->dm.soc_bounding_box;
>> +    INIT_LIST_HEAD(&adev->dm.da_list);
>> +
>>       /* Display Core create. */
>>       adev->dm.dc = dc_create(&init_data);
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h 
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> index 38bc0f88b29c..49137924a855 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> @@ -130,6 +130,13 @@ struct amdgpu_dm_backlight_caps {
>>       bool aux_support;
>>   };
>> +struct dal_allocation {
>> +    struct list_head list;
>> +    struct amdgpu_bo *bo;
>> +    void *cpu_ptr;
>> +    u64 gpu_addr;
>> +};
>> +
>>   /**
>>    * struct amdgpu_display_manager - Central amdgpu display manager 
>> device
>>    *
>> @@ -350,6 +357,8 @@ struct amdgpu_display_manager {
>>        */
>>       struct amdgpu_encoder mst_encoders[AMDGPU_DM_MAX_CRTC];
>>       bool force_timing_sync;
>> +
>> +    struct list_head da_list;
>>   };
>>   enum dsc_clock_force_state {
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c 
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>> index 3244a6ea7a65..5dc426e6e785 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>> @@ -652,8 +652,31 @@ void *dm_helpers_allocate_gpu_mem(
>>           size_t size,
>>           long long *addr)
>>   {
>> -    // TODO
>> -    return NULL;
>> +    struct amdgpu_device *adev = ctx->driver_context;
>> +    struct dal_allocation *da;
>> +    u32 domain = (type == DC_MEM_ALLOC_TYPE_GART) ?
>> +        AMDGPU_GEM_DOMAIN_GTT : AMDGPU_GEM_DOMAIN_VRAM;
>> +    int ret;
>> +
>> +    da = kzalloc(sizeof(struct dal_allocation), GFP_KERNEL);
>> +    if (!da)
>> +        return NULL;
>> +
>> +    ret = amdgpu_bo_create_kernel(adev, size, PAGE_SIZE,
>> +                      domain, &da->bo,
>> +                      &da->gpu_addr, &da->cpu_ptr);
>> +
>> +    *addr = da->gpu_addr;
>> +
>> +    if (ret) {
>> +        kfree(da);
>> +        return NULL;
>> +    }
>> +
>> +    /* add da to list in dm */
>> +    list_add(&da->list, &adev->dm.da_list);
>> +
>> +    return da->cpu_ptr;
>>   }
>>   void dm_helpers_free_gpu_mem(
>> @@ -661,5 +684,16 @@ void dm_helpers_free_gpu_mem(
>>           enum dc_gpu_mem_alloc_type type,
>>           void *pvMem)
>>   {
>> -    // TODO
>> +    struct amdgpu_device *adev = ctx->driver_context;
>> +    struct dal_allocation *da;
>> +
>> +    /* walk the da list in DM */
>> +    list_for_each_entry(da, &adev->dm.da_list, list) {
>> +        if (pvMem == da->cpu_ptr) {
>> +            amdgpu_bo_free_kernel(&da->bo, &da->gpu_addr, &da->cpu_ptr);
>> +            list_del(&da->list);
>> +            kfree(da);
>> +            break;
>> +        }
>> +    }
>>   }
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Cnicholas.kazlauskas%40amd.com%7C65096f1a05bf4379c1cd08d8bd2dd5a1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637467351862623818%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=Krf3epZ%2FsAWOmHRhUSukqEjKJJBLwtKNEe7GWYKBG1w%3D&amp;reserved=0 
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: Implement functions to let DC allocate GPU memory

[Christian König]

Am 20.01.21 um 15:16 schrieb Kazlauskas, Nicholas:
> On 2021-01-20 5:26 a.m., Christian König wrote:
>> Am 19.01.21 um 21:40 schrieb Bhawanpreet Lakha:
>>> From: Harry Wentland <harry.wentland@amd.com>
>>>
>>> [Why]
>>> DC needs to communicate with PM FW through GPU memory. In order
>>> to do so we need to be able to allocate memory from within DC.
>>>
>>> [How]
>>> Call amdgpu_bo_create_kernel to allocate GPU memory and use a
>>> list in amdgpu_display_manager to track our allocations so we
>>> can clean them up later.
>>
>> Well that looks like classic mid-layering to me with a horrible 
>> implementation of the free function.
>>
>> Christian.
>
> FWIW this is only really used during device creation and destruction 
> so the overhead of the free function isn't a considerable concern.
>
> Does AMDGPU always need to know the GPU address for the allocation to 
> free or should we work on fixing the callsites for this to pass it down?

No, we only need to know the BO. The GPU address pointer is only passed 
in so that it can be set to NULL and the CPU address pointer is used to 
judge if we need to kunmap() or not.

> We generally separate the CPU/GPU pointer but maybe it'd be best to 
> have some sort of allocation object here that has both for DC's purposes.

You need to pass around either the BO pointer directly or a structure 
containing it, but using the CPU mapping address to determine the BO is 
a really no-go.

Regards,
Christian.

>
> Regards,
> Nicholas Kazlauskas
>
>>
>>>
>>> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
>>> ---
>>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 +
>>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  9 +++++
>>>   .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 40 
>>> +++++++++++++++++--
>>>   3 files changed, 48 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> index e490fc2486f7..83ec92a69cba 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> @@ -1017,6 +1017,8 @@ static int amdgpu_dm_init(struct amdgpu_device 
>>> *adev)
>>>       init_data.soc_bounding_box = adev->dm.soc_bounding_box;
>>> +    INIT_LIST_HEAD(&adev->dm.da_list);
>>> +
>>>       /* Display Core create. */
>>>       adev->dm.dc = dc_create(&init_data);
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h 
>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>> index 38bc0f88b29c..49137924a855 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>> @@ -130,6 +130,13 @@ struct amdgpu_dm_backlight_caps {
>>>       bool aux_support;
>>>   };
>>> +struct dal_allocation {
>>> +    struct list_head list;
>>> +    struct amdgpu_bo *bo;
>>> +    void *cpu_ptr;
>>> +    u64 gpu_addr;
>>> +};
>>> +
>>>   /**
>>>    * struct amdgpu_display_manager - Central amdgpu display manager 
>>> device
>>>    *
>>> @@ -350,6 +357,8 @@ struct amdgpu_display_manager {
>>>        */
>>>       struct amdgpu_encoder mst_encoders[AMDGPU_DM_MAX_CRTC];
>>>       bool force_timing_sync;
>>> +
>>> +    struct list_head da_list;
>>>   };
>>>   enum dsc_clock_force_state {
>>> diff --git 
>>> a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c 
>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>>> index 3244a6ea7a65..5dc426e6e785 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>>> @@ -652,8 +652,31 @@ void *dm_helpers_allocate_gpu_mem(
>>>           size_t size,
>>>           long long *addr)
>>>   {
>>> -    // TODO
>>> -    return NULL;
>>> +    struct amdgpu_device *adev = ctx->driver_context;
>>> +    struct dal_allocation *da;
>>> +    u32 domain = (type == DC_MEM_ALLOC_TYPE_GART) ?
>>> +        AMDGPU_GEM_DOMAIN_GTT : AMDGPU_GEM_DOMAIN_VRAM;
>>> +    int ret;
>>> +
>>> +    da = kzalloc(sizeof(struct dal_allocation), GFP_KERNEL);
>>> +    if (!da)
>>> +        return NULL;
>>> +
>>> +    ret = amdgpu_bo_create_kernel(adev, size, PAGE_SIZE,
>>> +                      domain, &da->bo,
>>> +                      &da->gpu_addr, &da->cpu_ptr);
>>> +
>>> +    *addr = da->gpu_addr;
>>> +
>>> +    if (ret) {
>>> +        kfree(da);
>>> +        return NULL;
>>> +    }
>>> +
>>> +    /* add da to list in dm */
>>> +    list_add(&da->list, &adev->dm.da_list);
>>> +
>>> +    return da->cpu_ptr;
>>>   }
>>>   void dm_helpers_free_gpu_mem(
>>> @@ -661,5 +684,16 @@ void dm_helpers_free_gpu_mem(
>>>           enum dc_gpu_mem_alloc_type type,
>>>           void *pvMem)
>>>   {
>>> -    // TODO
>>> +    struct amdgpu_device *adev = ctx->driver_context;
>>> +    struct dal_allocation *da;
>>> +
>>> +    /* walk the da list in DM */
>>> +    list_for_each_entry(da, &adev->dm.da_list, list) {
>>> +        if (pvMem == da->cpu_ptr) {
>>> +            amdgpu_bo_free_kernel(&da->bo, &da->gpu_addr, 
>>> &da->cpu_ptr);
>>> +            list_del(&da->list);
>>> +            kfree(da);
>>> +            break;
>>> +        }
>>> +    }
>>>   }
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Cnicholas.kazlauskas%40amd.com%7C65096f1a05bf4379c1cd08d8bd2dd5a1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637467351862623818%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=Krf3epZ%2FsAWOmHRhUSukqEjKJJBLwtKNEe7GWYKBG1w%3D&amp;reserved=0 
>>
>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx