All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Wang, Zhi A" <zhi.a.wang@intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"intel-gvt-dev@lists.freedesktop.org"
	<intel-gvt-dev@lists.freedesktop.org>
Cc: "Vivi, Rodrigo" <rodrigo.vivi@intel.com>,
	"Widawsky, Benjamin" <benjamin.widawsky@intel.com>
Subject: Re: [RFCv5 2/2] drm/i915: Introduce private PAT management
Date: Tue, 29 Aug 2017 11:13:26 +0000	[thread overview]
Message-ID: <F3B0350DF4CB6849A642218320DE483D7D6036AE@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <150400319112.8572.378713713200582721@mail.alporthouse.com>

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.

The reason I want to reserve that PPAT entries for host is to keep the mapping between I915_CACHE_* and PPAT_*_INDEX unchanged.

Thanks,
Zhi.

-----Original Message-----
From: Chris Wilson [mailto:chris@chris-wilson.co.uk] 
Sent: Tuesday, August 29, 2017 1:40 PM
To: Wang, Zhi A <zhi.a.wang@intel.com>; intel-gfx@lists.freedesktop.org; intel-gvt-dev@lists.freedesktop.org
Cc: joonas.lahtinen@linux.intel.com; zhenyuw@linux.intel.com; Wang, Zhi A <zhi.a.wang@intel.com>; Widawsky, Benjamin <benjamin.widawsky@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>
Subject: Re: [RFCv5 2/2] drm/i915: Introduce private PAT management

Quoting Chris Wilson (2017-08-29 11:05:21)
> Quoting Zhi Wang (2017-08-29 09:00:51)
> > +static void cnl_setup_private_ppat(struct intel_ppat *ppat) {
> > +       ppat->max_entries = 8;
> > +       ppat->update = cnl_private_pat_update;
> > +       ppat->match = bdw_private_pat_match;
> > +       ppat->dummy_value = GEN8_PPAT_LLCELLC | GEN8_PPAT_AGE(3);
> > +
> >         /* XXX: spec is unclear if this is still needed for CNL+ */
> > -       if (!USES_PPGTT(dev_priv)) {
> > -               I915_WRITE(GEN10_PAT_INDEX(0), GEN8_PPAT_UC);
> > +       if (!USES_PPGTT(ppat->i915)) {
> > +               alloc_ppat_entry(ppat, 0, GEN8_PPAT_UC);
> >                 return;
> >         }
> >  
> > -       I915_WRITE(GEN10_PAT_INDEX(0), GEN8_PPAT_WB | GEN8_PPAT_LLC);
> > -       I915_WRITE(GEN10_PAT_INDEX(1), GEN8_PPAT_WC | GEN8_PPAT_LLCELLC);
> > -       I915_WRITE(GEN10_PAT_INDEX(2), GEN8_PPAT_WT | GEN8_PPAT_LLCELLC);
> > -       I915_WRITE(GEN10_PAT_INDEX(3), GEN8_PPAT_UC);
> > -       I915_WRITE(GEN10_PAT_INDEX(4), GEN8_PPAT_LLCELLC | GEN8_PPAT_AGE(0));
> > -       I915_WRITE(GEN10_PAT_INDEX(5), GEN8_PPAT_LLCELLC | GEN8_PPAT_AGE(1));
> > -       I915_WRITE(GEN10_PAT_INDEX(6), GEN8_PPAT_LLCELLC | GEN8_PPAT_AGE(2));
> > -       I915_WRITE(GEN10_PAT_INDEX(7), GEN8_PPAT_LLCELLC | GEN8_PPAT_AGE(3));
> > +       alloc_ppat_entry(ppat, 0, GEN8_PPAT_WB | GEN8_PPAT_LLC);
> > +       alloc_ppat_entry(ppat, 2, GEN8_PPAT_WT | GEN8_PPAT_LLCELLC);
> > +       alloc_ppat_entry(ppat, 3, GEN8_PPAT_UC);
> > +       alloc_ppat_entry(ppat, 4, GEN8_PPAT_LLCELLC | 
> > + GEN8_PPAT_AGE(0));
> >  }
> >  
> >  /* The GGTT and PPGTT need a private PPAT setup in order to handle cacheability
> >   * bits. When using advanced contexts each context stores its own PAT, but
> >   * writing this data shouldn't be harmful even in those cases. */ 
> > -static void bdw_setup_private_ppat(struct drm_i915_private 
> > *dev_priv)
> > +static void bdw_setup_private_ppat(struct intel_ppat *ppat)
> >  {
> > -       u64 pat;
> > -
> > -       pat = GEN8_PPAT(0, GEN8_PPAT_WB | GEN8_PPAT_LLC)     | /* for normal objects, no eLLC */
> > -             GEN8_PPAT(1, GEN8_PPAT_WC | GEN8_PPAT_LLCELLC) | /* for something pointing to ptes? */
> > -             GEN8_PPAT(2, GEN8_PPAT_WT | GEN8_PPAT_LLCELLC) | /* for scanout with eLLC */
> > -             GEN8_PPAT(3, GEN8_PPAT_UC)                     | /* Uncached objects, mostly for scanout */
> > -             GEN8_PPAT(4, GEN8_PPAT_WB | GEN8_PPAT_LLCELLC | GEN8_PPAT_AGE(0)) |
> > -             GEN8_PPAT(5, GEN8_PPAT_WB | GEN8_PPAT_LLCELLC | GEN8_PPAT_AGE(1)) |
> > -             GEN8_PPAT(6, GEN8_PPAT_WB | GEN8_PPAT_LLCELLC | GEN8_PPAT_AGE(2)) |
> > -             GEN8_PPAT(7, GEN8_PPAT_WB | GEN8_PPAT_LLCELLC | GEN8_PPAT_AGE(3));
> > +       ppat->max_entries = 8;
> > +       ppat->update = bdw_private_pat_update;
> > +       ppat->match = bdw_private_pat_match;
> > +       ppat->dummy_value = GEN8_PPAT_WB | GEN8_PPAT_LLCELLC | 
> > + GEN8_PPAT_AGE(3);
> >  
> > -       if (!USES_PPGTT(dev_priv))
> > +       if (!USES_PPGTT(dev_priv)) {
> >                 /* Spec: "For GGTT, there is NO pat_sel[2:0] from the entry,
> >                  * so RTL will always use the value corresponding to
> >                  * pat_sel = 000".
> > @@ -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 | 
> > + GEN8_PPAT_AGE(0));
> >  }
> >  
> > -static void chv_setup_private_ppat(struct drm_i915_private 
> > *dev_priv)
> > +static void chv_setup_private_ppat(struct intel_ppat *ppat)
> >  {
> > -       u64 pat;
> > +       ppat->max_entries = 8;
> > +       ppat->update = bdw_private_pat_update;
> > +       ppat->match = chv_private_pat_match;
> > +       ppat->dummy_value = CHV_PPAT_SNOOP;
> >  
> >         /*
> >          * Map WB on BDW to snooped on CHV.
> > @@ -2894,17 +3073,11 @@ static void chv_setup_private_ppat(struct drm_i915_private *dev_priv)
> >          * Which means we must set the snoop bit in PAT entry 0
> >          * in order to keep the global status page working.
> >          */
> > -       pat = GEN8_PPAT(0, CHV_PPAT_SNOOP) |
> > -             GEN8_PPAT(1, 0) |
> > -             GEN8_PPAT(2, 0) |
> > -             GEN8_PPAT(3, 0) |
> > -             GEN8_PPAT(4, CHV_PPAT_SNOOP) |
> > -             GEN8_PPAT(5, CHV_PPAT_SNOOP) |
> > -             GEN8_PPAT(6, CHV_PPAT_SNOOP) |
> > -             GEN8_PPAT(7, CHV_PPAT_SNOOP);
> >  
> > -       I915_WRITE(GEN8_PRIVATE_PAT_LO, pat);
> > -       I915_WRITE(GEN8_PRIVATE_PAT_HI, pat >> 32);
> > +       alloc_ppat_entry(ppat, 0, CHV_PPAT_SNOOP);
> > +       alloc_ppat_entry(ppat, 2, 0);
> > +       alloc_ppat_entry(ppat, 3, 0);
> > +       alloc_ppat_entry(ppat, 4, CHV_PPAT_SNOOP);
> >  }
> 
> 1 is dropped in all cases?
> 
> The current ABI is that we reserve (0, 1, 2) for userspace. 1 means 
> follow-PTE and is especially important.

Ignore the ABI concern, getting my tables confused. But the question still remains why do we need to reserve these in particular?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

  reply	other threads:[~2017-08-29 11:13 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-29  8:00 [RFCv5 1/2] drm/i915: Factor out setup_private_pat() Zhi Wang
2017-08-29  8:00 ` [RFCv5 2/2] drm/i915: Introduce private PAT management Zhi Wang
2017-08-29  9:37   ` Joonas Lahtinen
2017-08-29 11:18     ` Wang, Zhi A
2017-08-29 17:54     ` Wang, Zhi A
2017-08-29 18:14       ` Chris Wilson
2017-08-29 18:19         ` Wang, Zhi A
2017-08-30 11:09           ` Joonas Lahtinen
2017-08-29 10:05   ` Chris Wilson
2017-08-29 10:39     ` Chris Wilson
2017-08-29 11:13       ` Wang, Zhi A [this message]
2017-08-29 11:23         ` Chris Wilson
2017-08-29 11:31           ` Wang, Zhi A
2017-08-29 11:38           ` Joonas Lahtinen
2017-08-29 12:12   ` Chris Wilson
2017-08-29 12:18   ` Chris Wilson
2017-08-29 12:41     ` Wang, Zhi A
2017-08-29  8:19 ` ✓ Fi.CI.BAT: success for series starting with [RFCv5,1/2] drm/i915: Factor out setup_private_pat() Patchwork
2017-08-29  9:39 ` ✗ Fi.CI.IGT: failure " Patchwork

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=F3B0350DF4CB6849A642218320DE483D7D6036AE@SHSMSX104.ccr.corp.intel.com \
    --to=zhi.a.wang@intel.com \
    --cc=benjamin.widawsky@intel.com \
    --cc=chris@chris-wilson.co.uk \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=intel-gvt-dev@lists.freedesktop.org \
    --cc=rodrigo.vivi@intel.com \
    --subject='Re: [RFCv5 2/2] drm/i915: Introduce private PAT management' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.