On Mon, May 20, 2024 at 5:37 AM Xiao, Shane <shane.xiao@xxxxxxx> wrote:
>
> [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?
Yeah, this isn't as clean as I thought it would be. Either approach
is fine with me, although I suppose with this one, it's easier to
guarantee correctness.
Alex
>
> Best Regards,
> Shane
>
>
> > -----Original Message-----
> > From: Xiao, Shane <shane.xiao@xxxxxxx>
> > Sent: Monday, May 20, 2024 5:14 PM
> > To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx; alexdeucher@xxxxxxxxx; Kuehling, Felix
> > <Felix.Kuehling@xxxxxxx>; Somasekharan, Sreekant
> > <Sreekant.Somasekharan@xxxxxxx>
> > Cc: Liu, Aaron <Aaron.Liu@xxxxxxx>; Xiao, Shane <shane.xiao@xxxxxxx>;
> > Yao, Longlong <Longlong.Yao@xxxxxxx>
> > 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@xxxxxxx>
> > Signed-off-by: Shane Xiao <shane.xiao@xxxxxxx>
> > ---
> > 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
>
