[AMD Official Use Only - AMD Internal Distribution Only] > -----Original Message----- > From: Xiao, Shane > Sent: Monday, May 20, 2024 4:21 PM > To: Alex Deucher <alexdeucher@xxxxxxxxx> > Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx; Kuehling, Felix > <Felix.Kuehling@xxxxxxx>; Somasekharan, Sreekant > <Sreekant.Somasekharan@xxxxxxx>; Liu, Aaron <Aaron.Liu@xxxxxxx>; > Yao, Longlong <Longlong.Yao@xxxxxxx> > Subject: RE: [PATCH] drm/amdkfd: Correct the GFX12 memory type setting > > > > > -----Original Message----- > > From: Alex Deucher <alexdeucher@xxxxxxxxx> > > Sent: Friday, May 17, 2024 11:52 PM > > To: Xiao, Shane <shane.xiao@xxxxxxx> > > Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx; Kuehling, Felix > > <Felix.Kuehling@xxxxxxx>; Somasekharan, Sreekant > > <Sreekant.Somasekharan@xxxxxxx>; Liu, Aaron <Aaron.Liu@xxxxxxx>; > Yao, > > Longlong <Longlong.Yao@xxxxxxx> > > Subject: Re: [PATCH] drm/amdkfd: Correct the GFX12 memory type setting > > > > On Fri, May 17, 2024 at 3:07 AM Shane Xiao <shane.xiao@xxxxxxx> > wrote: > > > > > > This patch fixes the GFX12 memory type to NC. Since the Memory type > > > can be overwritten by the previous operations, the GFX12 MTYPE bits > > > need to be clear before setting to NC. > > > > > > Signed-off-by: longlyao <Longlong.Yao@xxxxxxx> > > > Signed-off-by: Shane Xiao <shane.xiao@xxxxxxx> > > > --- > > > drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c > > > b/drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c > > > index e2c6ec3cc4f3..6246d1dc0d30 100644 > > > --- a/drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c > > > +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c > > > @@ -534,7 +534,8 @@ static void gmc_v12_0_get_vm_pte(struct > > > amdgpu_device *adev, > > > > > > /* WA for HW bug */ > > > if (is_system || ((bo_adev != adev) && coherent)) > > > - *flags |= AMDGPU_PTE_MTYPE_GFX12(MTYPE_NC); > > > + *flags |= (*flags & ~AMDGPU_PTE_MTYPE_GFX12_MASK) | > > > + AMDGPU_PTE_MTYPE_GFX12(MTYPE_NC); > > > > Maybe we should make the AMDGPU_PTE_MTYPE_GFX12() macro clear the > > current field before setting the new one? That would align with the > > similar register field macros. > > The AMDGPU_PTE_MTYPE_GFX12() macro is used mtype as input, if we align > with Register field setting, we need use uint64 flags as input. > > If we do like below: > > -#define AMDGPU_PTE_MTYPE_GFX12(a) ((uint64_t)(a) << 54) > -#define AMDGPU_PTE_MTYPE_GFX12_MASK > AMDGPU_PTE_MTYPE_GFX12(3ULL) > +#define AMDGPU_PTE_MTYPE_GFX12_SHIFT(mtype) ((uint64_t)(mytype) > << 54) > +#define AMDGPU_PTE_MTYPE_GFX12_MASK > AMDGPU_PTE_MTYPE_GFX12_SHIFT(3ULL) > +#define AMDGPU_PTE_MTYPE_GFX12(flags) \ > + ((flags) & ((~AMDGPU_PTE_MTYPE_GFX12_MASK)) | (flags)) Just igore my comment before. We need both flags and MTYPE as input. I will send V2 version for this. > > We need to change the memory type setting from > AMDGPU_PTE_MTYPE_GFX12(MTYPE_NC) to > AMDGPU_PTE_MTYPE_GFX12(AMDGPU_PTE_MTYPE_GFX12_SHIFT(MTYPE_ > NC)). > This makes the code look more complicated. > > > Another way is to add one more macro AMDGPU_UPDATE/SET_PTE > _MTYPE_GFX12. > #define AMDGPU_ UPDATE_PTE_MTYPE_GFX12(flags) \ > ((flags) & ((~AMDGPU_PTE_MTYPE_GFX12_MASK)) | (flags)) > > Which of the above two methods do you think is more suitable? > If neither of these methods are suitable, do you have any other suggestions? > > > Best Regards, > Shane > > > > > Alex > > > > > > > > } > > > > > > -- > > > 2.25.1 > > >
