[PATCH 1/2] drm/amdgpu/mes11: fix set_hw_resources_1 calculation

[PATCH 1/2] drm/amdgpu/mes11: fix set_hw_resources_1 calculation

[Alex Deucher]

It's GPU page size not CPU page size.  In most cases they
are the same, but not always.  This can lead to overallocation
on systems with larger pages.

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/mes_v11_0.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c b/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c
index bf51f3dcc130e..e862a3febe2b2 100644
--- a/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c
@@ -743,7 +743,7 @@ static int mes_v11_0_set_hw_resources(struct amdgpu_mes *mes)
 
 static int mes_v11_0_set_hw_resources_1(struct amdgpu_mes *mes)
 {
-	int size = 128 * PAGE_SIZE;
+	int size = 128 * AMDGPU_GPU_PAGE_SIZE;
 	int ret = 0;
 	struct amdgpu_device *adev = mes->adev;
 	union MESAPI_SET_HW_RESOURCES_1 mes_set_hw_res_pkt;
-- 
2.48.1

[PATCH V3 2/2] drm/amdgpu/mes: Add cleaner shader fence address handling in MES for GFX11

[Alex Deucher]

From: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>

This commit introduces enhancements to the handling of the cleaner
shader fence in the AMDGPU MES driver:

- The MES (Microcode Execution Scheduler) now sends a PM4 packet to the
  KIQ (Kernel Interface Queue) to request the cleaner shader, ensuring
  that requests are handled in a controlled manner and avoiding the
  race conditions.
- The CP (Compute Processor) firmware has been updated to use a private
  bus for accessing specific registers, avoiding unnecessary operations
  that could lead to issues in VF (Virtual Function) mode.
- The cleaner shader fence memory address is now set correctly in the
  `mes_set_hw_res_pkt` structure, allowing for proper synchronization of
  the cleaner shader execution. This is done by calculating the address
  using the write-back memory base address and the cleaner fence offset.

Cc: lin cao <lin.cao@amd.com>
Cc: Jingwen Chen <Jingwen.Chen2@amd.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Suggested-by: Shaoyun Liu <shaoyun.liu@amd.com>
Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/mes_v11_0.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c b/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c
index e862a3febe2b2..e22d0ee6d8a3a 100644
--- a/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c
@@ -754,7 +754,7 @@ static int mes_v11_0_set_hw_resources_1(struct amdgpu_mes *mes)
 	mes_set_hw_res_pkt.header.dwsize = API_FRAME_SIZE_IN_DWORDS;
 	mes_set_hw_res_pkt.enable_mes_info_ctx = 1;
 
-	ret = amdgpu_bo_create_kernel(adev, size, PAGE_SIZE,
+	ret = amdgpu_bo_create_kernel(adev, size + AMDGPU_GPU_PAGE_SIZE, PAGE_SIZE,
 				AMDGPU_GEM_DOMAIN_VRAM,
 				&mes->resource_1,
 				&mes->resource_1_gpu_addr,
@@ -765,7 +765,10 @@ static int mes_v11_0_set_hw_resources_1(struct amdgpu_mes *mes)
 	}
 
 	mes_set_hw_res_pkt.mes_info_ctx_mc_addr = mes->resource_1_gpu_addr;
-	mes_set_hw_res_pkt.mes_info_ctx_size = mes->resource_1->tbo.base.size;
+	mes_set_hw_res_pkt.mes_info_ctx_size = size;
+	mes_set_hw_res_pkt.cleaner_shader_fence_mc_addr =
+		mes->resource_1_gpu_addr + size;
+
 	return mes_v11_0_submit_pkt_and_poll_completion(mes,
 			&mes_set_hw_res_pkt, sizeof(mes_set_hw_res_pkt),
 			offsetof(union MESAPI_SET_HW_RESOURCES_1, api_status));
@@ -1632,12 +1635,10 @@ static int mes_v11_0_hw_init(struct amdgpu_ip_block *ip_block)
 	if (r)
 		goto failure;
 
-	if (amdgpu_sriov_is_mes_info_enable(adev)) {
-		r = mes_v11_0_set_hw_resources_1(&adev->mes);
-		if (r) {
-			DRM_ERROR("failed mes_v11_0_set_hw_resources_1, r=%d\n", r);
-			goto failure;
-		}
+	r = mes_v11_0_set_hw_resources_1(&adev->mes);
+	if (r) {
+		DRM_ERROR("failed mes_v11_0_set_hw_resources_1, r=%d\n", r);
+		goto failure;
 	}
 
 	r = mes_v11_0_query_sched_status(&adev->mes);
@@ -1665,10 +1666,9 @@ static int mes_v11_0_hw_init(struct amdgpu_ip_block *ip_block)
 static int mes_v11_0_hw_fini(struct amdgpu_ip_block *ip_block)
 {
 	struct amdgpu_device *adev = ip_block->adev;
-	if (amdgpu_sriov_is_mes_info_enable(adev)) {
-		amdgpu_bo_free_kernel(&adev->mes.resource_1, &adev->mes.resource_1_gpu_addr,
-					&adev->mes.resource_1_addr);
-	}
+
+	amdgpu_bo_free_kernel(&adev->mes.resource_1, &adev->mes.resource_1_gpu_addr,
+			      &adev->mes.resource_1_addr);
 	return 0;
 }
 
-- 
2.48.1

Re: [PATCH V3 2/2] drm/amdgpu/mes: Add cleaner shader fence address handling in MES for GFX11

[Christian König]

Am 07.02.25 um 15:43 schrieb Alex Deucher:
> From: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
>
> This commit introduces enhancements to the handling of the cleaner
> shader fence in the AMDGPU MES driver:
>
> - The MES (Microcode Execution Scheduler) now sends a PM4 packet to the
>    KIQ (Kernel Interface Queue) to request the cleaner shader, ensuring
>    that requests are handled in a controlled manner and avoiding the
>    race conditions.
> - The CP (Compute Processor) firmware has been updated to use a private
>    bus for accessing specific registers, avoiding unnecessary operations
>    that could lead to issues in VF (Virtual Function) mode.
> - The cleaner shader fence memory address is now set correctly in the
>    `mes_set_hw_res_pkt` structure, allowing for proper synchronization of
>    the cleaner shader execution. This is done by calculating the address
>    using the write-back memory base address and the cleaner fence offset.
>
> Cc: lin cao <lin.cao@amd.com>
> Cc: Jingwen Chen <Jingwen.Chen2@amd.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Suggested-by: Shaoyun Liu <shaoyun.liu@amd.com>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>

Yeah that makes much more sense.

I'm really wondering why the MES hasn't allocated that in it's own 
memory in the first place?

Does the MES expect the kernel driver to wait for that fence value?

Anyway Reviewed-by: Christian König <christian.koenig@amd.com> for now.

Regards,
Christian.

> ---
>   drivers/gpu/drm/amd/amdgpu/mes_v11_0.c | 24 ++++++++++++------------
>   1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c b/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c
> index e862a3febe2b2..e22d0ee6d8a3a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c
> @@ -754,7 +754,7 @@ static int mes_v11_0_set_hw_resources_1(struct amdgpu_mes *mes)
>   	mes_set_hw_res_pkt.header.dwsize = API_FRAME_SIZE_IN_DWORDS;
>   	mes_set_hw_res_pkt.enable_mes_info_ctx = 1;
>   
> -	ret = amdgpu_bo_create_kernel(adev, size, PAGE_SIZE,
> +	ret = amdgpu_bo_create_kernel(adev, size + AMDGPU_GPU_PAGE_SIZE, PAGE_SIZE,
>   				AMDGPU_GEM_DOMAIN_VRAM,
>   				&mes->resource_1,
>   				&mes->resource_1_gpu_addr,
> @@ -765,7 +765,10 @@ static int mes_v11_0_set_hw_resources_1(struct amdgpu_mes *mes)
>   	}
>   
>   	mes_set_hw_res_pkt.mes_info_ctx_mc_addr = mes->resource_1_gpu_addr;
> -	mes_set_hw_res_pkt.mes_info_ctx_size = mes->resource_1->tbo.base.size;
> +	mes_set_hw_res_pkt.mes_info_ctx_size = size;
> +	mes_set_hw_res_pkt.cleaner_shader_fence_mc_addr =
> +		mes->resource_1_gpu_addr + size;
> +
>   	return mes_v11_0_submit_pkt_and_poll_completion(mes,
>   			&mes_set_hw_res_pkt, sizeof(mes_set_hw_res_pkt),
>   			offsetof(union MESAPI_SET_HW_RESOURCES_1, api_status));
> @@ -1632,12 +1635,10 @@ static int mes_v11_0_hw_init(struct amdgpu_ip_block *ip_block)
>   	if (r)
>   		goto failure;
>   
> -	if (amdgpu_sriov_is_mes_info_enable(adev)) {
> -		r = mes_v11_0_set_hw_resources_1(&adev->mes);
> -		if (r) {
> -			DRM_ERROR("failed mes_v11_0_set_hw_resources_1, r=%d\n", r);
> -			goto failure;
> -		}
> +	r = mes_v11_0_set_hw_resources_1(&adev->mes);
> +	if (r) {
> +		DRM_ERROR("failed mes_v11_0_set_hw_resources_1, r=%d\n", r);
> +		goto failure;
>   	}
>   
>   	r = mes_v11_0_query_sched_status(&adev->mes);
> @@ -1665,10 +1666,9 @@ static int mes_v11_0_hw_init(struct amdgpu_ip_block *ip_block)
>   static int mes_v11_0_hw_fini(struct amdgpu_ip_block *ip_block)
>   {
>   	struct amdgpu_device *adev = ip_block->adev;
> -	if (amdgpu_sriov_is_mes_info_enable(adev)) {
> -		amdgpu_bo_free_kernel(&adev->mes.resource_1, &adev->mes.resource_1_gpu_addr,
> -					&adev->mes.resource_1_addr);
> -	}
> +
> +	amdgpu_bo_free_kernel(&adev->mes.resource_1, &adev->mes.resource_1_gpu_addr,
> +			      &adev->mes.resource_1_addr);
>   	return 0;
>   }
>   

RE: [PATCH V3 2/2] drm/amdgpu/mes: Add cleaner shader fence address handling in MES for GFX11

[Liu, Shaoyun]

[AMD Official Use Only - AMD Internal Distribution Only]

I'm not sure why there is no mes.resource_1_gpu_addr  for mes v12 , probably sriov team still not support it . But we do need this cleaner_fence_gpu_addr for both mes v11 and  v12 .  What MES need is a GPU addr it can update a 32 bit value on it ,an extra page do seems a overkill for this.  I still think allcoate a WB in amdgpu_mes_init and free it in amdgpu_mes_fini is a better solution and  it can  be  generic for v11 and v12 .  The original design is for windows , and  I think they need to sync between their KMD driver and  FW for the cleaner shader submission, so the KMD need to be able to check the fence .

Regards
Shaoyun.liu

-----Original Message-----
From: Koenig, Christian <Christian.Koenig@amd.com>
Sent: Friday, February 7, 2025 9:51 AM
To: Deucher, Alexander <Alexander.Deucher@amd.com>; amd-gfx@lists.freedesktop.org
Cc: SHANMUGAM, SRINIVASAN <SRINIVASAN.SHANMUGAM@amd.com>; cao, lin <lin.cao@amd.com>; Chen, JingWen (Wayne) <JingWen.Chen2@amd.com>; Liu, Shaoyun <Shaoyun.Liu@amd.com>
Subject: Re: [PATCH V3 2/2] drm/amdgpu/mes: Add cleaner shader fence address handling in MES for GFX11

Am 07.02.25 um 15:43 schrieb Alex Deucher:
> From: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
>
> This commit introduces enhancements to the handling of the cleaner
> shader fence in the AMDGPU MES driver:
>
> - The MES (Microcode Execution Scheduler) now sends a PM4 packet to the
>    KIQ (Kernel Interface Queue) to request the cleaner shader, ensuring
>    that requests are handled in a controlled manner and avoiding the
>    race conditions.
> - The CP (Compute Processor) firmware has been updated to use a private
>    bus for accessing specific registers, avoiding unnecessary operations
>    that could lead to issues in VF (Virtual Function) mode.
> - The cleaner shader fence memory address is now set correctly in the
>    `mes_set_hw_res_pkt` structure, allowing for proper synchronization of
>    the cleaner shader execution. This is done by calculating the address
>    using the write-back memory base address and the cleaner fence offset.
>
> Cc: lin cao <lin.cao@amd.com>
> Cc: Jingwen Chen <Jingwen.Chen2@amd.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Suggested-by: Shaoyun Liu <shaoyun.liu@amd.com>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>

Yeah that makes much more sense.

I'm really wondering why the MES hasn't allocated that in it's own memory in the first place?

Does the MES expect the kernel driver to wait for that fence value?

Anyway Reviewed-by: Christian König <christian.koenig@amd.com> for now.

Regards,
Christian.

> ---
>   drivers/gpu/drm/amd/amdgpu/mes_v11_0.c | 24 ++++++++++++------------
>   1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c
> b/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c
> index e862a3febe2b2..e22d0ee6d8a3a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c
> @@ -754,7 +754,7 @@ static int mes_v11_0_set_hw_resources_1(struct amdgpu_mes *mes)
>       mes_set_hw_res_pkt.header.dwsize = API_FRAME_SIZE_IN_DWORDS;
>       mes_set_hw_res_pkt.enable_mes_info_ctx = 1;
>
> -     ret = amdgpu_bo_create_kernel(adev, size, PAGE_SIZE,
> +     ret = amdgpu_bo_create_kernel(adev, size + AMDGPU_GPU_PAGE_SIZE,
> +PAGE_SIZE,
>                               AMDGPU_GEM_DOMAIN_VRAM,
>                               &mes->resource_1,
>                               &mes->resource_1_gpu_addr,
> @@ -765,7 +765,10 @@ static int mes_v11_0_set_hw_resources_1(struct amdgpu_mes *mes)
>       }
>
>       mes_set_hw_res_pkt.mes_info_ctx_mc_addr = mes->resource_1_gpu_addr;
> -     mes_set_hw_res_pkt.mes_info_ctx_size = mes->resource_1->tbo.base.size;
> +     mes_set_hw_res_pkt.mes_info_ctx_size = size;
> +     mes_set_hw_res_pkt.cleaner_shader_fence_mc_addr =
> +             mes->resource_1_gpu_addr + size;
> +
>       return mes_v11_0_submit_pkt_and_poll_completion(mes,
>                       &mes_set_hw_res_pkt, sizeof(mes_set_hw_res_pkt),
>                       offsetof(union MESAPI_SET_HW_RESOURCES_1, api_status)); @@
> -1632,12 +1635,10 @@ static int mes_v11_0_hw_init(struct amdgpu_ip_block *ip_block)
>       if (r)
>               goto failure;
>
> -     if (amdgpu_sriov_is_mes_info_enable(adev)) {
> -             r = mes_v11_0_set_hw_resources_1(&adev->mes);
> -             if (r) {
> -                     DRM_ERROR("failed mes_v11_0_set_hw_resources_1, r=%d\n", r);
> -                     goto failure;
> -             }
> +     r = mes_v11_0_set_hw_resources_1(&adev->mes);
> +     if (r) {
> +             DRM_ERROR("failed mes_v11_0_set_hw_resources_1, r=%d\n", r);
> +             goto failure;
>       }
>
>       r = mes_v11_0_query_sched_status(&adev->mes);
> @@ -1665,10 +1666,9 @@ static int mes_v11_0_hw_init(struct amdgpu_ip_block *ip_block)
>   static int mes_v11_0_hw_fini(struct amdgpu_ip_block *ip_block)
>   {
>       struct amdgpu_device *adev = ip_block->adev;
> -     if (amdgpu_sriov_is_mes_info_enable(adev)) {
> -             amdgpu_bo_free_kernel(&adev->mes.resource_1, &adev->mes.resource_1_gpu_addr,
> -                                     &adev->mes.resource_1_addr);
> -     }
> +
> +     amdgpu_bo_free_kernel(&adev->mes.resource_1, &adev->mes.resource_1_gpu_addr,
> +                           &adev->mes.resource_1_addr);
>       return 0;
>   }
>