|
[AMD Official Use Only - AMD Internal Distribution Only]
Thanks Alex,
I have created an integration branch for GFX12, and cleaned up and ported these patches. We will finish testing and start the code review of these patches soon.
Regards
Shashank
From: Alex Deucher <alexdeucher@xxxxxxxxx>
Sent: Friday, October 18, 2024 9:18 PM To: Sharma, Shashank <Shashank.Sharma@xxxxxxx> Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx <amd-gfx@xxxxxxxxxxxxxxxxxxxxx>; Somalapuram, Amaranath <Amaranath.Somalapuram@xxxxxxx>; Koenig, Christian <Christian.Koenig@xxxxxxx>; Yadav, Arvind <Arvind.Yadav@xxxxxxx> Subject: Re: [PATCH] drm/amdgpu: enable userqueue support for GFX12 On Tue, Oct 15, 2024 at 12:37 PM Sharma, Shashank
<shashank.sharma@xxxxxxx> wrote: > > > On 15/10/2024 16:58, Alex Deucher wrote: > > 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. > Noted, > >> 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 > > Thanks, Noted, I would do the recommended changes. A took a longer look at the changes and it turns out I could move all of the IP specific stuff into the IP specific code. I hacked together the attached untested patches to clean this up. Alex > > - Shashank > > >> - 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 |
