[Intel-xe] [PATCH] Different platforms has different PAT encoding in PTE and PDE format, add correct PAT encoding for pre-Xe2 platforms (XELP, XEHPC, XELPG).
Matthew Brost
matthew.brost at intel.com
Fri Aug 11 20:38:20 UTC 2023
On Fri, Aug 11, 2023 at 01:30:12PM -0700, Matt Roper wrote:
> On Wed, Aug 09, 2023 at 12:26:30PM -0600, Lucas De Marchi wrote:
> >
> > As Ashutosh said, please take a look on kernel documentation on how to
> > write proper commit messages.
> >
> > https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
> >
> > It's a good idea to check checkpatch on the patch before submitting too.
> > Yes, sometimes we forget, but it's good to create the habit.
> >
> > On Wed, Aug 09, 2023 at 05:37:30PM +0530, Ravi Kumar Vodapalli wrote:
> > > Bspec: 45101, 71582
> > > Signed-off-by: Ravi Kumar Vodapalli <ravi.kumar.vodapalli at intel.com>
> > > ---
> > > drivers/gpu/drm/xe/xe_device.c | 3 ++
> > > drivers/gpu/drm/xe/xe_device_types.h | 5 ++
> > > drivers/gpu/drm/xe/xe_pat.c | 76 ++++++++++++++++++++++++++++
> > > drivers/gpu/drm/xe/xe_pat.h | 25 +++++++++
> > > drivers/gpu/drm/xe/xe_pt.c | 28 +++-------
> > > drivers/gpu/drm/xe/xe_pt_types.h | 3 +-
> > > 6 files changed, 119 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> > > index 766df07de979..0675a60f17f9 100644
> > > --- a/drivers/gpu/drm/xe/xe_device.c
> > > +++ b/drivers/gpu/drm/xe/xe_device.c
> > > @@ -34,6 +34,7 @@
> > > #include "xe_vm.h"
> > > #include "xe_vm_madvise.h"
> > > #include "xe_wait_user_fence.h"
> > > +#include "xe_pat.h"
> >
> > all the includes are alphabetically sorted. Do not simply append here.
> >
> > >
> > > #ifdef CONFIG_LOCKDEP
> > > struct lockdep_map xe_device_mem_access_lockdep_map = {
> > > @@ -292,6 +293,8 @@ int xe_device_probe(struct xe_device *xe)
> > > goto err_irq_shutdown;
> > > }
> > >
> > > + xe_pte_pat_init(xe);
> > > +
> > > err = xe_mmio_probe_vram(xe);
> > > if (err)
> > > goto err_irq_shutdown;
> > > diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> > > index bfedcc7571b0..3c64834c32fc 100644
> > > --- a/drivers/gpu/drm/xe/xe_device_types.h
> > > +++ b/drivers/gpu/drm/xe/xe_device_types.h
> > > @@ -23,6 +23,8 @@
> > > #include "intel_display_device.h"
> > > #endif
> > >
> > > +#include "xe_pt_types.h"
> > > +
> > > struct xe_ggtt;
> > >
> > > #define XE_BO_INVALID_OFFSET LONG_MAX
> > > @@ -470,6 +472,9 @@ struct xe_device {
> > > u32 lvds_channel_mode;
> > > } params;
> > > #endif
> > > + const u32 *pte_pat_table;
> > > + u64 (*ppgtt_pte_encode)(struct xe_device *xe, u64 pte_pat, enum xe_cache_level cache);
> > > + u64 (*ppgtt_pde_encode)(struct xe_device *xe, u64 pde_pat, enum xe_cache_level cache);
> >
> > A few lines above we have:
> >
> > /* private: */
> >
> > #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> > /*
> > * Any fields below this point are the ones used by display. *
> > They are temporarily added here so xe_device can be desguised as *
> > drm_i915_private during build. After cleanup these should go away, *
> > migrating to the right sub-structs */
> >
> >
> > You need to find a proper place in the structure to add new fields. Do
> > not simply append. However I'll defer to Matt Roper (Cc'ed) if xe_device
> > is the correct place or if this should be on the gt level. Anyway, it
> > should probably be a substruct "ops" or "funcs" for this vfunc approach.
>
> I think it might make more sense to keep this closer to what it's
> operating on, in a more object-oriented sense. Since these vfuncs are
> specific to PPGTT handling, I think 'xe_vm' might be a better spot for
Agree.
Also the ops should defined as a struct rather than individual ops too.
See 'const struct xe_ring_ops *ring_ops' in xe_exec_queue_types.h".
The pte_pat_table, ppgtt_pte_encode, ppgtt_pde_encode, probably all could in a single struct.
Matthew Brost
> them than 'xe_device.' We could also drop the 'ppgtt_' prefix and
> assign the function pointers inside xe_vm_create().
>
>
> Matt
>
> >
> > > };
> > >
> > > /**
> > > diff --git a/drivers/gpu/drm/xe/xe_pat.c b/drivers/gpu/drm/xe/xe_pat.c
> > > index b56a65779d26..8f4c33d0f7dc 100644
> > > --- a/drivers/gpu/drm/xe/xe_pat.c
> > > +++ b/drivers/gpu/drm/xe/xe_pat.c
> > > @@ -62,6 +62,24 @@ static const u32 mtl_pat_table[] = {
> > > [4] = MTL_PAT_0_WB | MTL_3_COH_2W,
> > > };
> > >
> > > +static const u32 xelp_pte_pat_table[] = {
> > > + [XE_CACHE_NONE] = XELP_PAT_UNCACHE,
> > > + [XE_CACHE_WT] = XELP_PAT_WT_CACHE,
> > > + [XE_CACHE_WB] = XELP_PAT_WB_CACHE,
> > > +};
> > > +
> > > +static const u32 xehpc_pte_pat_table[] = {
> > > + [XE_CACHE_NONE] = XEHPC_PAT_CLOS0_UNCACHE,
> > > + [XE_CACHE_WT] = XEHPC_PAT_CLOS0_WT_CACHE,
> > > + [XE_CACHE_WB] = XEHPC_PAT_CLOS0_WB_CACHE,
> > > +};
> > > +
> > > +static const u32 xelpg_pte_pat_table[] = {
> > > + [XE_CACHE_NONE] = XELPG_PAT_UNCACHE,
> > > + [XE_CACHE_WT] = XELPG_PAT_WT_CACHE,
> > > + [XE_CACHE_WB] = XELPG_PAT_WB_CACHE,
> > > +};
> >
> > you added XE_CACHE_LAST but then did nothing with that. Did you mean to
> > add a static assert to make sure we don't have arrays with fewer
> > elements than XE_CACHE_LAST?
> >
> > > +
> > > static void program_pat(struct xe_gt *gt, const u32 table[], int n_entries)
> > > {
> > > for (int i = 0; i < n_entries; i++) {
> > > @@ -111,3 +129,61 @@ void xe_pat_init(struct xe_gt *gt)
> > > GRAPHICS_VER(xe), GRAPHICS_VERx100(xe) % 100);
> > > }
> > > }
> > > +
> > > +u32 xe_pat_get_index(struct xe_device *xe, enum xe_cache_level cache)
> > > +{
> > > + return xe->pte_pat_table[cache];
> > > +}
> > > +
> > > +static u64 xelp_ppgtt_pde_encode_pat(struct xe_device *xe, u64 pde_pat, enum xe_cache_level cache)
> > > +{
> > > + u32 pat_index = xe_pat_get_index(xe, cache);
> > > +
> > > + pde_pat &= ~(XELP_PDE_PAT_MASK);
> > > +
> > > + if (pat_index & BIT(0))
> > > + pde_pat |= BIT(3);
> > > +
> > > + if (pat_index & BIT(1))
> > > + pde_pat |= BIT(4);
> > > +
> > > + if (pat_index & BIT(2))
> > > + pde_pat |= BIT(12);
> > > +}
> > > +
> > > +static u64 xelp_ppgtt_pte_encode_pat(struct xe_device *xe, u64 pte_pat, enum xe_cache_level cache)
> > > +{
> > > + u32 pat_index = xe_pat_get_index(xe, cache);
> > > +
> > > + pte_pat &= ~(XELP_PTE_PAT_MASK);
> > > +
> > > + if (pat_index & BIT(0))
> > > + pte_pat |= BIT(3);
> > > +
> > > + if (pat_index & BIT(1))
> > > + pte_pat |= BIT(4);
> > > +
> > > + if (pat_index & BIT(2))
> > > + pte_pat |= BIT(7);
> > > +
> > > + return pte_pat;
> > > +}
> > > +
> > > +void xe_pte_pat_init(struct xe_device *xe)
> >
> > any non-static function in this file should be xe_pat_* (there a few
> > exceptions, but this is usually the rule).
> >
> > > +{
> > > + if (GRAPHICS_VERx100(xe) >= 1270) {
> > > + xe->pte_pat_table = xelpg_pte_pat_table;
> > > + xe->ppgtt_pte_encode = xelp_ppgtt_pte_encode_pat;
> > > + xe->ppgtt_pde_encode = xelp_ppgtt_pde_encode_pat;
> >
> > setting those don't belong to this file. They should be in the relevant
> > file that cares about ppgtt (i.e. xe_pt.c).
> >
> > Also you are missing the necessary changes to xe_ggtt.c that should also
> > encode the bits.
> >
> > > + } else if (GRAPHICS_VERx100(xe) >= 1260) {
> > > + xe->pte_pat_table = xehpc_pte_pat_table;
> > > + xe->ppgtt_pte_encode = xelp_ppgtt_pte_encode_pat;
> > > + xe->ppgtt_pde_encode = xelp_ppgtt_pde_encode_pat;
> > > + } else {
> > > + xe->pte_pat_table = xelp_pte_pat_table;
> > > + xe->ppgtt_pte_encode = xelp_ppgtt_pte_encode_pat;
> > > + xe->ppgtt_pde_encode = xelp_ppgtt_pde_encode_pat;
> > > + }
> > > +}
> > > diff --git a/drivers/gpu/drm/xe/xe_pat.h b/drivers/gpu/drm/xe/xe_pat.h
> > > index 659de4008131..1138ac78943f 100644
> > > --- a/drivers/gpu/drm/xe/xe_pat.h
> > > +++ b/drivers/gpu/drm/xe/xe_pat.h
> > > @@ -6,8 +6,33 @@
> > > #ifndef _XE_PAT_H_
> > > #define _XE_PAT_H_
> > >
> > > +#include "xe_device_types.h"
> >
> > why? we don't add includes, particularly in headers, that aren't used.
> >
> > > +
> > > +#define XELP_PTE_PAT_MASK (BIT_ULL(7) | BIT_ULL(4) | BIT_ULL(3))
> > > +#define XELP_PDE_PAT_MASK (BIT_ULL(12) | BIT_ULL(4) | BIT_ULL(3))
> > > +#define XELP_PAT_WB_CACHE 0
> > > +#define XELP_PAT_WC_CACHE 1
> > > +#define XELP_PAT_WT_CACHE 2
> > > +#define XELP_PAT_UNCACHE 3
> > > +
> > > +#define XEHPC_PAT_CLOS0_UNCACHE 0
> > > +#define XEHPC_PAT_CLOS0_WC_CACHE 1
> > > +#define XEHPC_PAT_CLOS0_WT_CACHE 2
> > > +#define XEHPC_PAT_CLOS0_WB_CACHE 3
> > > +#define XEHPC_PAT_CLOS1_WT_CACHE 4
> > > +#define XEHPC_PAT_CLOS1_WB_CACHE 5
> > > +#define XEHPC_PAT_CLOS2_WT_CACHE 6
> > > +#define XEHPC_PAT_CLOS2_WB_CACHE 7
> > > +
> > > +#define XELPG_PAT_WB_CACHE 0
> > > +#define XELPG_PAT_WT_CACHE 1
> > > +#define XELPG_PAT_UNCACHE 2
> > > +#define XELPG_PAT_1_WAY_WB_CACHE 3
> > > +#define XELPG_PAT_2_WAY_WB_CACHE 4
> >
> > only define what you use.
> >
> > > +
> > > struct xe_gt;
> > >
> > > void xe_pat_init(struct xe_gt *gt);
> > > +void xe_pte_pat_init(struct xe_device *xe);
> > >
> > > #endif
> > > diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
> > > index 5709518e314b..eccb0942ff46 100644
> > > --- a/drivers/gpu/drm/xe/xe_pt.c
> > > +++ b/drivers/gpu/drm/xe/xe_pt.c
> > > @@ -58,19 +58,16 @@ static struct xe_pt *xe_pt_entry(struct xe_pt_dir *pt_dir, unsigned int index)
> > > * Return: An encoded page directory entry. No errors.
> > > */
> > > u64 xe_pde_encode(struct xe_bo *bo, u64 bo_offset,
> > > - const enum xe_cache_level level)
> > > + const enum xe_cache_level cache)
> > > {
> > > u64 pde;
> > > + struct xe_vm *vm = bo->vm;
> > > + struct xe_device *xe = vm->xe;
> > >
> > > pde = xe_bo_addr(bo, bo_offset, XE_PAGE_SIZE);
> > > pde |= XE_PAGE_PRESENT | XE_PAGE_RW;
> > >
> > > - /* FIXME: I don't think the PPAT handling is correct for MTL */
> > > -
> > > - if (level != XE_CACHE_NONE)
> > > - pde |= PPAT_CACHED_PDE;
> > > - else
> > > - pde |= PPAT_UNCACHED;
> > > + pde = xe->ppgtt_pde_encode(xe, pde, cache);
> > >
> > > return pde;
> > > }
> > > @@ -78,6 +75,9 @@ u64 xe_pde_encode(struct xe_bo *bo, u64 bo_offset,
> > > static u64 __pte_encode(u64 pte, enum xe_cache_level cache,
> > > struct xe_vma *vma, u32 pt_level)
> > > {
> > > + struct xe_vm *vm = xe_vma_vm(vma);
> > > + struct xe_device *xe = vm->xe;
> > > +
> > > pte |= XE_PAGE_PRESENT | XE_PAGE_RW;
> > >
> > > if (unlikely(vma && xe_vma_read_only(vma)))
> > > @@ -86,19 +86,7 @@ static u64 __pte_encode(u64 pte, enum xe_cache_level cache,
> > > if (unlikely(vma && xe_vma_is_null(vma)))
> > > pte |= XE_PTE_NULL;
> > >
> > > - /* FIXME: I don't think the PPAT handling is correct for MTL */
> > > -
> > > - switch (cache) {
> > > - case XE_CACHE_NONE:
> > > - pte |= PPAT_UNCACHED;
> > > - break;
> > > - case XE_CACHE_WT:
> > > - pte |= PPAT_DISPLAY_ELLC;
> > > - break;
> > > - default:
> > > - pte |= PPAT_CACHED;
> > > - break;
> > > - }
> > > + pte = xe->ppgtt_pte_encode(xe, pte, cache);
> > >
> > > if (pt_level == 1)
> > > pte |= XE_PDE_PS_2M;
> > > diff --git a/drivers/gpu/drm/xe/xe_pt_types.h b/drivers/gpu/drm/xe/xe_pt_types.h
> > > index 2ed64c0a4485..2a94ecbaa844 100644
> > > --- a/drivers/gpu/drm/xe/xe_pt_types.h
> > > +++ b/drivers/gpu/drm/xe/xe_pt_types.h
> > > @@ -9,9 +9,10 @@
> > > #include "xe_pt_walk.h"
> > >
> > > enum xe_cache_level {
> > > - XE_CACHE_NONE,
> > > + XE_CACHE_NONE = 0,
> > > XE_CACHE_WT,
> > > XE_CACHE_WB,
> > > + XE_CACHE_LAST,
> >
> > I can't make sense of these changes here
> >
> > Lucas De Marchi
> >
> > > };
> > >
> > > #define XE_VM_MAX_LEVEL 4
> > > --
> > > 2.25.1
> > >
>
> --
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation
More information about the Intel-xe
mailing list