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

[Xiao, Shane]

[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 at gmail.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
>
>
>
> > -----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))

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
> > >