[PATCH] drm/amdkfd: Correct the GFX12 memory type setting

[Xiao, Shane]

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Alex Deucher <alexdeucher at gmail.com>
> Sent: Friday, May 17, 2024 11:52 PM
> To: Xiao, Shane <shane.xiao at amd.com>
> Cc: amd-gfx at lists.freedesktop.org; Kuehling, Felix
> <Felix.Kuehling at amd.com>; Somasekharan, Sreekant
> <Sreekant.Somasekharan at amd.com>; Liu, Aaron <Aaron.Liu at amd.com>;
> Yao, Longlong <Longlong.Yao at amd.com>
> Subject: Re: [PATCH] drm/amdkfd: Correct the GFX12 memory type setting
>
> On Fri, May 17, 2024 at 3:07 AM Shane Xiao <shane.xiao at amd.com> 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 at amd.com>
> > Signed-off-by: Shane Xiao <shane.xiao at amd.com>
> > ---
> >  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
> >