Re: [PATCH 3/3] drm/i915: Enable GTT caching on gen8

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, May 21, 2015 at 01:18:44PM -0700, Jesse Barnes wrote:
> On 05/19/2015 10:32 AM, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > GTT caching was disabled by default on gen8 due to not working with
> > big pages. Some information suggests that it got fixed, but still
> > GTT caching has been left disabled by default. Or could be it just
> > meant that the default was changed to off, and hence the problem
> > got solved.
> > 
> > Enable GTT caching in the hopes of some performance increase.
> > Whether or not the big pages issue has been fixed is irrelevant
> > at this stage since we don't use big pages.
> > 
> > This gives me a 1-2% improvement in xonotic on my BSW. Haven't tried
> > BDW, but supposedly it has larger TLBs so might not benefit as much.
> > On HSW GTT caching is enabled by default.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h |  2 ++
> >  drivers/gpu/drm/i915/intel_pm.c | 13 +++++++++++++
> >  2 files changed, 15 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 84af255..90640d5 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -1461,6 +1461,8 @@ enum skl_disp_power_wells {
> >  #define RING_HWS_PGA(base)	((base)+0x80)
> >  #define RING_HWS_PGA_GEN6(base)	((base)+0x2080)
> >  
> > +#define HSW_GTT_CACHE_EN	0x4024
> > +#define   GTT_CACHE_EN_ALL	0xF0007FFF
> >  #define GEN7_WR_WATERMARK	0x4028
> >  #define GEN7_GFX_PRIO_CTRL	0x402C
> >  #define ARB_MODE		0x4030
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 5ec56b6..58517a50 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -6205,6 +6205,13 @@ static void broadwell_init_clock_gating(struct drm_device *dev)
> >  	I915_WRITE(GEN8_L3SQCREG1, BDW_WA_L3SQCREG1_DEFAULT);
> >  	I915_WRITE(GEN7_MISCCPCTL, misccpctl);
> >  
> > +	/*
> > +	 * WaGttCachingOffByDefault:bdw
> > +	 * GTT cache may not work with big pages, so if those
> > +	 * are ever enabled GTT cache may need to be disabled.
> > +	 */
> > +	I915_WRITE(HSW_GTT_CACHE_EN, GTT_CACHE_EN_ALL);
> > +
> >  	lpt_init_clock_gating(dev);
> >  }
> >  
> > @@ -6480,6 +6487,12 @@ static void cherryview_init_clock_gating(struct drm_device *dev)
> >  	/* WaDisableSDEUnitClockGating:chv */
> >  	I915_WRITE(GEN8_UCGCTL6, I915_READ(GEN8_UCGCTL6) |
> >  		   GEN8_SDEUNIT_CLOCK_GATE_DISABLE);
> > +
> > +	/*
> > +	 * GTT cache may not work with big pages, so if those
> > +	 * are ever enabled GTT cache may need to be disabled.
> > +	 */
> > +	I915_WRITE(HSW_GTT_CACHE_EN, GTT_CACHE_EN_ALL);
> >  }
> >  
> >  static void g4x_init_clock_gating(struct drm_device *dev)
> > 
> 
> Looks ok to me; I guess testing will be the real review here.

Just aside: If you think the real test for a patch is the real world imo
an important part of the review work is to make sure we do have that
testing coverage. We have a few igts that specifically exercise gtt tlb
issues (from previous generations), so I think we're covered here.

> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Merged all three, thanks for patches&review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx