On Tue, Oct 15, 2024 at 6:13 AM Sharma, Shashank
<shashank.sharma@xxxxxxx> wrote:
>
> Hello Alex,
>
> On 14/10/2024 22:29, Deucher, Alexander wrote:
>
> [AMD Official Use Only - AMD Internal Distribution Only]
>
> -----Original Message-----
> From: Sharma, Shashank <Shashank.Sharma@xxxxxxx>
> Sent: Thursday, October 10, 2024 2:08 PM
> To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Somalapuram, Amaranath <Amaranath.Somalapuram@xxxxxxx>; Deucher,
> Alexander <Alexander.Deucher@xxxxxxx>; Koenig, Christian
> <Christian.Koenig@xxxxxxx>; Yadav, Arvind <Arvind.Yadav@xxxxxxx>; Sharma,
> Shashank <Shashank.Sharma@xxxxxxx>
> Subject: [PATCH] drm/amdgpu: enable userqueue support for GFX12
>
> From: Somalapuram Amaranath <Amaranath.Somalapuram@xxxxxxx>
>
> This patch enables Usermode queue support across GFX, Compute and SDMA IPs
> on GFX12/SDMA7. It typically reuses Navi3X userqueue IP functions to create and
> destroy MQDs.
>
> I would like to make this more explicit. In mes_v11_0_userqueue.c, I would suggest splitting out any non-gfx11 specific code into some new helpers in mes_userqueue.c. E.g.,
>
> mes_v11_0_map_gtt_bo_to_gart() -> mes_userq_map_gtt_bo_to_gart()
> mes_v11_0_create_wptr_mapping() -> mes_userq_create_wptr_mapping()
> mes_v11_0_userq_map() -> mes_userq_map()
> mes_v11_0_userq_unmap() -> mes_userq_unmap()
> mes_v11_0_userq_mqd_destroy() -> mes_userq_mqd_destroy()
>
> Initial patch sets had all these functions named similar to 'mes_userq_* ' only, but later you recommended that we should have mention of _v11_0 to indicate the IP specific stuff, as there might be IP specific way of mapping/unmapping/creating and destroying the queues. So with this comment we might be going back to the same version.
Well that was before gfx12 support was on our radar. Generally, you
develop for the first generation and if there are things that you can
pull out into common code and share across generations, then you
should do that when you add support for the next generation.
>
> As of now, v12 UQ was tested using the the same methods as V11 UQs, and found it functional. We might need more information before taking this step.
You would need to verify that the V11 and V12 MQDs are the same.
EIther way, I would recommend making v11 and v12 variants the the
functions which populate the MQDs that the firmware uses. There is
alays a chance that the firmware may repurpose some of the fields for
different things that can lead to subtle bugs. At the end of the day,
I think we'll end up with a bunch of helpers in mes_userqueue.c and
then a function or two in mes_v11_0_userqueue.c and
mes_v12_0_userqueue.c. Alternatively, you could just put the helpers
in amdgpu_mes.c and the gfx11 and gfx12 specific functions in
mes_v11_0.c and mes_v12_0.c since it will probably only be a function
or two. You could even add a callback for the MQD specific changes
and add a wrapper like the other functions in amdgpu_mes.c and the
generic functions in mes_v11_0_userqueue.c. That would make
everything symmetric for mes managed queues.
Alex
>
> - Shashank
>
> However, mes_v11_userq_create_ctx_space() uses the v11 mqd structures which may not match the v12 structures. We should add a v12 implementation for any functions that use the v12 structures.
>
> Alex
>
> Cc: Alex Deucher <alexander.deucher@xxxxxxx>
> Cc: Christian Koenig <christian.koenig@xxxxxxx>
> Cc: Arvind Yadav <arvind.yadav@xxxxxxx>
> Signed-off-by: Somalapuram Amaranath <Amaranath.Somalapuram@xxxxxxx>
> Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxx>
> ---
> drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c | 5 +++++
> drivers/gpu/drm/amd/amdgpu/sdma_v7_0.c | 6 ++++++
> 2 files changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c
> b/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c
> index 9fec28d8a5fc..d511996c374d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c
> @@ -46,6 +46,7 @@
> #include "gfx_v12_0.h"
> #include "nbif_v6_3_1.h"
> #include "mes_v12_0.h"
> +#include "mes_v11_0_userqueue.h"
>
> #define GFX12_NUM_GFX_RINGS 1
> #define GFX12_MEC_HPD_SIZE 2048
> @@ -1335,6 +1336,10 @@ static int gfx_v12_0_sw_init(struct amdgpu_ip_block
> *ip_block)
> adev->gfx.mec.num_mec = 2;
> adev->gfx.mec.num_pipe_per_mec = 2;
> adev->gfx.mec.num_queue_per_pipe = 4;
> +#ifdef CONFIG_DRM_AMDGPU_NAVI3X_USERQ
> + adev->userq_funcs[AMDGPU_HW_IP_GFX] =
> &userq_mes_v11_0_funcs;
> + adev->userq_funcs[AMDGPU_HW_IP_COMPUTE] =
> &userq_mes_v11_0_funcs;
> +#endif
> break;
> default:
> adev->gfx.me.num_me = 1;
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v7_0.c
> b/drivers/gpu/drm/amd/amdgpu/sdma_v7_0.c
> index 24f24974ac1d..badcf38bb8b6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v7_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v7_0.c
> @@ -42,6 +42,7 @@
> #include "sdma_common.h"
> #include "sdma_v7_0.h"
> #include "v12_structs.h"
> +#include "mes_v11_0_userqueue.h"
>
> MODULE_FIRMWARE("amdgpu/sdma_7_0_0.bin");
> MODULE_FIRMWARE("amdgpu/sdma_7_0_1.bin");
> @@ -1317,6 +1318,11 @@ static int sdma_v7_0_sw_init(struct amdgpu_ip_block
> *ip_block)
> else
> DRM_ERROR("Failed to allocated memory for SDMA IP Dump\n");
>
> +#ifdef CONFIG_DRM_AMDGPU_NAVI3X_USERQ
> + adev->userq_funcs[AMDGPU_HW_IP_DMA] = &userq_mes_v11_0_funcs;
> #endif
> +
> +
> return r;
> }
>
> --
> 2.46.2
