[Intel-gfx] [RFCv5 2/2] drm/i915: Introduce private PAT management

[Joonas Lahtinen]

On Tue, 2017-08-29 at 12:23 +0100, Chris Wilson wrote:
> Quoting Wang, Zhi A (2017-08-29 12:13:26)
> > Hi Chris:
> >     There is mapping between i915 cache level -> PPAT index. Currently it's:
> > 
> > static gen8_pte_t gen8_pte_encode(dma_addr_t addr,
> >                                   enum i915_cache_level level)
> > {
> > ...
> >         switch (level) {
> >         case I915_CACHE_NONE:
> >                 pte |= PPAT_UNCACHED_INDEX;
> >                 break;
> >         case I915_CACHE_WT:
> >                 pte |= PPAT_DISPLAY_ELLC_INDEX;
> >                 break;
> >         default:
> >                 pte |= PPAT_CACHED_INDEX;
> >                 break;
> >         }
> > ...
> > 
> > According to bspec, the PPAT index filled in the page table is calculated as:
> > 
> > PPAT index = 4 * PAT + 2 * PCD + PWT
> > 
> > In the i915_gem_gtt.c
> > 
> > #define PPAT_UNCACHED_INDEX             (_PAGE_PWT | _PAGE_PCD)     // PPAT INDEX =  1 + 2 * 1 = 3
> > #define PPAT_CACHED_PDE_INDEX           0 /* WB LLC */                  //  PPAT INDEX = 0
> > #define PPAT_CACHED_INDEX               _PAGE_PAT /* WB LLCeLLC */      // PPAT INDEX = 4 * 1 = 4
> > #define PPAT_DISPLAY_ELLC_INDEX         _PAGE_PCD /* WT eLLC */           // PPAT INDEX = 2 * 1 = 2
> > 
> > Actually the PPAT index used by i915 are: 0 2 3 4, which has already been reserved in the RFC patches.
> 
> So we can use these values in alloc_ppat, right? Still very concerned
> about the hole -- it kind of implies there is an entry we should be
> using but have forgotten!
> 
> > > > @@ -2864,17 +3038,22 @@ static void bdw_setup_private_ppat(struct drm_i915_private *dev_priv)
> > > >                  * So we can still hold onto all our assumptions wrt cpu
> > > >                  * clflushing on LLC machines.
> > > >                  */
> > > > -               pat = GEN8_PPAT(0, GEN8_PPAT_UC);
> > > > +               alloc_ppat_entry(ppat, 0, GEN8_PPAT_UC);
> > > > +               return;
> > > > +       }
> > > >  
> > > > -       /* XXX: spec defines this as 2 distinct registers. It's unclear if a 64b
> > > > -        * write would work. */
> > > > -       I915_WRITE(GEN8_PRIVATE_PAT_LO, pat);
> > > > -       I915_WRITE(GEN8_PRIVATE_PAT_HI, pat >> 32);
> > > > +       alloc_ppat_entry(ppat, 0, GEN8_PPAT_WB | GEN8_PPAT_LLC);     /* for normal objects, no eLLC */
> > > > +       alloc_ppat_entry(ppat, 2, GEN8_PPAT_WT | GEN8_PPAT_LLCELLC); /* for scanout with eLLC */
> > > > +       alloc_ppat_entry(ppat, 3, GEN8_PPAT_UC);                     /* Uncached objects, mostly for scanout */
> > > > +       alloc_ppat_entry(ppat, 4, GEN8_PPAT_WB | GEN8_PPAT_LLCELLC | 
> 
> /* See gen8_pte_encode() for the mapping from cache-level to ppat */
> alloc_ppage_entry(ppat, PPAT_CACHED_PDE_INDEX, GEN8_PPAT_WB | GEN8_PPAT_LLC);
> alloc_ppage_entry(ppat, PPAT_DISPLAY_ELLC_INDEX, GEN8_PPAT_WT | GEN8_PPAT_LLCELLC);
> alloc_ppage_entry(ppat, PPAT_UNCACHED_INDEX, GEN8_PPAT_UC);
> alloc_ppage_entry(ppat, PPAT_CACHED_INDEX, GEN8_PPAT_WB | GEN8_PPAT_LLCELLC | GEN8_PPAT_AGE(0));
> 

+1 on that idea as follow-up patch which should do these entry changes.

I'd first make sure this patch leaves all actual register writes in
equal state as they were before the patch, just the code is
restructured. Bisecting will be easier if the actual added holes will
be brought in by separate patch.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation