[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@xxxxxxx>
Sent: Friday, February 7, 2025 9:51 AM
To: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
Cc: SHANMUGAM, SRINIVASAN <SRINIVASAN.SHANMUGAM@xxxxxxx>; cao, lin <lin.cao@xxxxxxx>; Chen, JingWen (Wayne) <JingWen.Chen2@xxxxxxx>; Liu, Shaoyun <Shaoyun.Liu@xxxxxxx>
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@xxxxxxx>
>
> 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@xxxxxxx>
> Cc: Jingwen Chen <Jingwen.Chen2@xxxxxxx>
> Cc: Christian König <christian.koenig@xxxxxxx>
> Cc: Alex Deucher <alexander.deucher@xxxxxxx>
> Suggested-by: Shaoyun Liu <shaoyun.liu@xxxxxxx>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@xxxxxxx>
> Signed-off-by: Alex Deucher <alexander.deucher@xxxxxxx>
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@xxxxxxx> 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;
> }
>
