[PATCH] drm/amdgpu: Update the impelmentation of AMDGPU_PTE_MTYPE_GFX12

[Xiao, Shane]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Alex,

I have changed the macro AMDGPU_PTE_MTYPE_GFX12 to clear mtype bit before setting.
Add one parameter for this macro, and some related code needs to be changed.

I'm not sure whether that's the ideal way to do it, but if it is, I'll implement it for all other generations.
If this is not ideal way, could you please give me some other advice?

Best Regards,
Shane


> -----Original Message-----
> From: Xiao, Shane <shane.xiao at amd.com>
> Sent: Monday, May 20, 2024 5:14 PM
> To: amd-gfx at lists.freedesktop.org; alexdeucher at gmail.com; Kuehling, Felix
> <Felix.Kuehling at amd.com>; Somasekharan, Sreekant
> <Sreekant.Somasekharan at amd.com>
> Cc: Liu, Aaron <Aaron.Liu at amd.com>; Xiao, Shane <shane.xiao at amd.com>;
> Yao, Longlong <Longlong.Yao at amd.com>
> Subject: [PATCH] drm/amdgpu: Update the impelmentation of
> AMDGPU_PTE_MTYPE_GFX12
>
> This patch changes the implementation of AMDGPU_PTE_MTYPE_GFX12,
> clear the bits before setting the new one.
> This fixed the potential issue that GFX12 setting memory to NC.
>
> v2: Clear mtype field before setting the new one (Alex)
>
> Signed-off-by: longlyao <Longlong.Yao at amd.com>
> Signed-off-by: Shane Xiao <shane.xiao at amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |  7 +++++--
> drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c | 23 +++++++++++------------
>  2 files changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index bc71b44387b2..99b246e82ed6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -116,8 +116,11 @@ struct amdgpu_mem_stats;
>  #define AMDGPU_PTE_PRT_FLAG(adev)    \
>       ((amdgpu_ip_version((adev), GC_HWIP, 0) >= IP_VERSION(12, 0, 0)) ?
> AMDGPU_PTE_PRT_GFX12 : AMDGPU_PTE_PRT)
>
> -#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, mtype) \
> +     ((flags) & ((~AMDGPU_PTE_MTYPE_GFX12_MASK)) |   \
> +       AMDGPU_PTE_MTYPE_GFX12_SHIFT(mtype))
>
>  #define AMDGPU_PTE_IS_PTE            (1ULL << 63)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c
> b/drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c
> index e2c6ec3cc4f3..f2d331d0181f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v12_0.c
> @@ -461,17 +461,17 @@ static uint64_t gmc_v12_0_map_mtype(struct
> amdgpu_device *adev, uint32_t flags)  {
>       switch (flags) {
>       case AMDGPU_VM_MTYPE_DEFAULT:
> -             return AMDGPU_PTE_MTYPE_GFX12(MTYPE_NC);
> +             return AMDGPU_PTE_MTYPE_GFX12(0ULL,MTYPE_NC);
>       case AMDGPU_VM_MTYPE_NC:
> -             return AMDGPU_PTE_MTYPE_GFX12(MTYPE_NC);
> +             return AMDGPU_PTE_MTYPE_GFX12(0ULL,MTYPE_NC);
>       case AMDGPU_VM_MTYPE_WC:
> -             return AMDGPU_PTE_MTYPE_GFX12(MTYPE_WC);
> +             return AMDGPU_PTE_MTYPE_GFX12(0ULL,MTYPE_WC);
>       case AMDGPU_VM_MTYPE_CC:
> -             return AMDGPU_PTE_MTYPE_GFX12(MTYPE_CC);
> +             return AMDGPU_PTE_MTYPE_GFX12(0ULL,MTYPE_CC);
>       case AMDGPU_VM_MTYPE_UC:
> -             return AMDGPU_PTE_MTYPE_GFX12(MTYPE_UC);
> +             return AMDGPU_PTE_MTYPE_GFX12(0ULL,MTYPE_UC);
>       default:
> -             return AMDGPU_PTE_MTYPE_GFX12(MTYPE_NC);
> +             return AMDGPU_PTE_MTYPE_GFX12(0ULL,MTYPE_NC);
>       }
>  }
>
> @@ -509,8 +509,8 @@ static void gmc_v12_0_get_vm_pte(struct
> amdgpu_device *adev,
>       *flags &= ~AMDGPU_PTE_EXECUTABLE;
>       *flags |= mapping->flags & AMDGPU_PTE_EXECUTABLE;
>
> -     *flags &= ~AMDGPU_PTE_MTYPE_GFX12_MASK;
> -     *flags |= (mapping->flags & AMDGPU_PTE_MTYPE_GFX12_MASK);
> +     *flags = AMDGPU_PTE_MTYPE_GFX12(*flags, (mapping->flags &
>       \
> +                      AMDGPU_PTE_MTYPE_GFX12_MASK) >>
> AMDGPU_PTE_MTYPE_GFX12_SHIFT);

This is not correct. I will correct this change in next version.

>
>       if (mapping->flags & AMDGPU_PTE_PRT_GFX12) {
>               *flags |= AMDGPU_PTE_PRT_GFX12;
> @@ -524,8 +524,7 @@ static void gmc_v12_0_get_vm_pte(struct
> amdgpu_device *adev,
>
>       if (bo->flags & (AMDGPU_GEM_CREATE_COHERENT |
>                              AMDGPU_GEM_CREATE_UNCACHED))
> -             *flags = (*flags & ~AMDGPU_PTE_MTYPE_GFX12_MASK) |
> -                      AMDGPU_PTE_MTYPE_GFX12(MTYPE_UC);
> +             *flags = AMDGPU_PTE_MTYPE_GFX12(*flags, MTYPE_UC);
>
>       bo_adev = amdgpu_ttm_adev(bo->tbo.bdev);
>       coherent = bo->flags & AMDGPU_GEM_CREATE_COHERENT; @@ -
> 534,7 +533,7 @@ 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 = AMDGPU_PTE_MTYPE_GFX12(*flags, MTYPE_NC);
>
>  }
>
> @@ -707,7 +706,7 @@ static int gmc_v12_0_gart_init(struct amdgpu_device
> *adev)
>               return r;
>
>       adev->gart.table_size = adev->gart.num_gpu_pages * 8;
> -     adev->gart.gart_pte_flags =
> AMDGPU_PTE_MTYPE_GFX12(MTYPE_UC) |
> +     adev->gart.gart_pte_flags = AMDGPU_PTE_MTYPE_GFX12(0ULL,
> MTYPE_UC) |
>                                   AMDGPU_PTE_EXECUTABLE |
>                                   AMDGPU_PTE_IS_PTE;
>
> --
> 2.25.1