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

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Wayne Boyer <wayne.boyer@intel.com>
Cc: intel-gfx@lists.freedesktop.org, Rodrigo Vivi <rodrigo.vivi@intel.com>
Subject: Re: [PATCH 4/5] drm/i915: Only set gem object L3 cache level for IVB devices
Date: Mon, 7 Dec 2015 21:28:34 +0200	[thread overview]
Message-ID: <20151207192834.GL4437@intel.com> (raw)
In-Reply-To: <1449514270-15171-5-git-send-email-wayne.boyer@intel.com>

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@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Wayne Boyer <wayne.boyer@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...

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

>  		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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx