| Message ID | 1372960927-1112-1-git-send-email-ben@bwidawsk.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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! 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. > > 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? > > #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? > > 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
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
On Thu, Jul 04, 2013 at 11:02:03AM -0700, Ben Widawsky 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. > > 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> > --- Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
On Thu, Jul 04, 2013 at 11:02:03AM -0700, Ben Widawsky wrote: > +/* 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) One small note, an age of '0' means "old" as in it's likely to be evicted before buffers aged 3, 2 or 1. We don't use any other age yet, so it doesn't matter for now, but might in the future.
On Mon, Jul 15, 2013 at 03:23:00PM +0100, Damien Lespiau wrote: > On Thu, Jul 04, 2013 at 11:02:03AM -0700, Ben Widawsky wrote: > > +/* 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) > > One small note, an age of '0' means "old" as in it's likely to be > evicted before buffers aged 3, 2 or 1. We don't use any other age yet, > so it doesn't matter for now, but might in the future. > > -- > Damien > FWIW, I have no intention of using any ages in the kernel. We can pick 3 equally well. Maybe in a way off future if or when we decide to have the kernel try to track which cacheability to use for objects, we'll care. Daniel, would you mind adding a comment on merge? Damien is correct 3 is youngest, 0 is oldest.
On Mon, Jul 15, 2013 at 09:54:35AM -0700, Ben Widawsky wrote: > On Mon, Jul 15, 2013 at 03:23:00PM +0100, Damien Lespiau wrote: > > On Thu, Jul 04, 2013 at 11:02:03AM -0700, Ben Widawsky wrote: > > > +/* 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) > > > > One small note, an age of '0' means "old" as in it's likely to be > > evicted before buffers aged 3, 2 or 1. We don't use any other age yet, > > so it doesn't matter for now, but might in the future. > > > > -- > > Damien > > > FWIW, I have no intention of using any ages in the kernel. We can pick 3 > equally well. Maybe in a way off future if or when we decide to have the > kernel try to track which cacheability to use for objects, we'll care. > > Daniel, would you mind adding a comment on merge? Damien is correct 3 is > youngest, 0 is oldest. Done and merged, thanks for the patch. -Daniel
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)) #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) 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; }