[AMD Official Use Only - AMD Internal Distribution Only]
> -----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))
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
> >
