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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
>




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux