[Intel-gfx] [PATCH 4/5] drm/i915: Only set gem object L3 cache level for IVB devices

[Imre Deak]

On Mon, 2015-12-07 at 21:28 +0200, Ville Syrjälä wrote:
> On Mon, Dec 07, 2015 at 10:51:09AM -0800, Wayne Boyer wrote:
> > Do some further clean up based on the initial review of
> > drm/i915: Separate cherryview from valleyview.
> > 
> > In this case, in i915_gem_alloc_context_obj() only call
> > i915_gem_object_set_cache_level() for Ivy Bridge devices
> > since later platforms don't have L3 control bits in the PTE.
> > 
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > Signed-off-by: Wayne Boyer <wayne.boyer at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_context.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
> > b/drivers/gpu/drm/i915/i915_gem_context.c
> > index 4b1161d..e4de433 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -185,12 +185,10 @@ i915_gem_alloc_context_obj(struct drm_device
> > *dev, size_t size)
> >  	/*
> >  	 * Try to make the context utilize L3 as well as LLC.
> >  	 *
> > -	 * On VLV we don't have L3 controls in the PTEs so we
> > -	 * shouldn't touch the cache level, especially as that
> > -	 * would make the object snooped which might have a
> > -	 * negative performance impact.
> > +	 * This is only applicable for Ivy Bridge devices since
> > +	 * later platforms don't have L3 control bits in the PTE.
> >  	 */
> > -	if (INTEL_INFO(dev)->gen >= 7 && !IS_VALLEYVIEW(dev) &&
> > !IS_CHERRYVIEW(dev)) {
> > +	if (IS_IVYBRIDGE(dev)) {
> 
> This would actually change the snoop setting on BXT, but that
> should be fine since BXT still has the GTT -> PAT 0 thing which
> means we get snooping anyway IIRC.
> 
> Imre, we discussed this stuff at some point, and I hope I'm not
> forgetting something crucial here that would break BXT. Probably best
> if you sanity check my thinking here...

Yes, AFAIU it doesn't matter what level we set, because of the PAT
mapping constraint you describe above. Atm we also don't use this
function on BXT, b/c of the different codepaths for LRC vs. ringbuffer
mode. But that could/should be unified at one point, so yea it's better
to keep things working for BXT too here. In the corresponding
intel_lr_context_deferred_alloc() we don't set the cache level, which
also agrees with the above, so I think this change is fine.

> We should probably mention this PAT 0 business in a comment somewhere
> here since it's a very easy thing to miss.

Yep, a comment about this would be in order.

> 
> >  		ret = i915_gem_object_set_cache_level(obj,
> > I915_CACHE_L3_LLC);
> >  		/* Failure shouldn't ever happen this early */
> >  		if (WARN_ON(ret)) {
> > -- 
> > 2.6.3
>