Re: [PATCH 1/5] drm/i915/hsw: Set correct Haswell PTE encodings.

[Ben Widawsky <benjamin.widawsky@intel.com>]

On Fri, Jul 12, 2013 at 09:00:31PM -0300, Rodrigo Vivi wrote:
> Hi Ben,
> 
> sorry for taking so long to look at your patches.
> Well, since I changed my TI password I'm not able to see bspec
> anymore, so I couldn't verify many things that I'm going to ask many
> questions.
> If the answer is on spec or any other doc please send me in pvt!

I think since we haven't yet published HSW docs, it's pretty fair of you
to ask, and for me to address it publicly.

> 
> 
> On Thu, Jul 4, 2013 at 3:02 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
> > From: Ben Widawsky <benjamin.widawsky@intel.com>
> >
> > The cacheability controls have changed, and the bits have been
> > rearranged in general.
> >
> > v2: Remove comments for snb/ivb cache leves, that's a separate change.
> >
> > v3: Resolve conflicts due to patch series reordering.
> >
> > v4: Rebased on top of Kenneth Graunke's ->pet_encode refactoring.
> 
> pet like in leper?! :P
> just kidding never mind.

heh, fixedi locally, thanks.
> 
> >
> > v5: Removed eLLC bits for separate patch.
> >
> > In the internal repository this was:
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_gtt.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index 66929ea..42262d0 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -33,6 +33,7 @@
> >
> >  /* PPGTT stuff */
> >  #define GEN6_GTT_ADDR_ENCODE(addr)     ((addr) | (((addr) >> 28) & 0xff0))
> > +#define HSW_GTT_ADDR_ENCODE(addr)      ((addr) | (((addr) >> 28) & 0x7f0))
> 
> why not masking bit 10 anymore?

You're asking the wrong guy... The HW just supports one less address
bit. The bit gets used for some of the cacheability flags later in the
series.

> 
> >
> >  #define GEN6_PDE_VALID                 (1 << 0)
> >  /* gen6+ has bit 11-4 for physical addr bit 39-32 */
> > @@ -44,6 +45,14 @@
> >  #define GEN6_PTE_CACHE_LLC             (2 << 1)
> >  #define GEN6_PTE_CACHE_LLC_MLC         (3 << 1)
> >  #define GEN6_PTE_ADDR_ENCODE(addr)     GEN6_GTT_ADDR_ENCODE(addr)
> > +#define HSW_PTE_ADDR_ENCODE(addr)      HSW_GTT_ADDR_ENCODE(addr)
> > +
> > +/* Cacheability Control is a 4-bit value. The low three bits are stored in *
> > + * bits 3:1 of the PTE, while the fourth bit is stored in bit 11 of the PTE.
> > + */
> > +#define HSW_CACHEABILITY_CONTROL(bits) ((((bits) & 0x7) << 1) | \
> > +                                        (((bits) & 0x8) << (11 - 3)))
> > +#define HSW_WB_LLC_AGE0                        HSW_CACHEABILITY_CONTROL(0x3)
> 
> where did you get that? and are you really using that in any other patch?

It's in a table in the spec. Unfortunately there is no formula defined
in the spec, but I came up with this one (actually my original formula
had a bug, fixed by Ken).

Not sure what you mean by, "using that in any other patch." I'm not sure
why is that relevant, it's used below.

> 
> >
> >  static gen6_gtt_pte_t gen6_pte_encode(dma_addr_t addr,
> >                                       enum i915_cache_level level)
> > @@ -92,10 +101,10 @@ static gen6_gtt_pte_t hsw_pte_encode(dma_addr_t addr,
> >                                      enum i915_cache_level level)
> >  {
> >         gen6_gtt_pte_t pte = GEN6_PTE_VALID;
> > -       pte |= GEN6_PTE_ADDR_ENCODE(addr);
> > +       pte |= HSW_PTE_ADDR_ENCODE(addr);
> >
> >         if (level != I915_CACHE_NONE)
> > -               pte |= GEN6_PTE_CACHE_LLC;
> > +               pte |= HSW_WB_LLC_AGE0;
> >
> >         return pte;
> >  }
> > --
> > 1.8.3
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> --
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br

-- 
Ben Widawsky, Intel Open Source Technology Center