Re: [PATCH 2/2] drm/i915: Use Write-Through cacheing for the display plane on Iris

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Jul 30, 2013 at 06:36:15PM +0100, Chris Wilson wrote:
> On Tue, Jul 30, 2013 at 08:19:28PM +0300, Ville Syrjälä wrote:
> > On Tue, Jul 30, 2013 at 05:58:36PM +0100, Chris Wilson wrote:
> > > Haswell GT3e has the unique feature of supporting Write-Through cacheing
> > > of objects within the eLLC. The purpose of this is to enable the display
> > > plane to remain coherent whilst objects lie resident in the eLLC - so
> > > that we in theory get the best of both worlds, perfect display and fast
> > > access.
> > 
> > The description here talks about eLLC only, but you set the PTE for
> > WT in LLC/eLLC both.
> 
> s/eLLC/LLC/
> 
> For some reason, I keep telling myself that it is a magic property of
> the eLLC otherwise why wouldn't they do it for all LLC!
> 
> > > -	ret = i915_gem_object_set_cache_level(obj, I915_CACHE_NONE);
> > > +	ret = i915_gem_object_set_cache_level(obj,
> > > +					      HAS_WT(obj->base.dev) ? I915_CACHE_WT : I915_CACHE_NONE);
> > 
> > Don't we need to tweak the write domain like we do for UC to make sure
> > already dirty lines get flushed from caches?
> 
> You need to explicitly do the flush, which gets ugly.

Ah right, because i915_gem_clflush_object() checks the cache_level.

> I choose to ignore
> the problem as unlike the LLC -> UC transition, I haven't spotted any
> dirt. However, it doesn't look too bad if we replace it like so:

Hmm, and what about CPU access in general? CPU doesn't know about the
WT policy, so I'm assuming we'd need to flush after all CPU writes.

> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index ac1b9cd..0e089e2 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3362,27 +3362,32 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
>  		i915_gem_obj_ggtt_set_color(obj, cache_level);
>  	}
>  
> -	if (cache_level == I915_CACHE_NONE) {
> -		u32 old_read_domains, old_write_domain;
> -
> +	if (cache_level == I915_CACHE_NONE ||
> +	    cache_level == I915_CACHE_WT) {

>  		/* If we're coming from LLC cached, then we haven't
>  		 * actually been tracking whether the data is in the
>  		 * CPU cache or not, since we only allow one bit set
>  		 * in obj->write_domain and have been skipping the clflushes.
> -		 * Just set it to the CPU cache for now.
> +		 * Do them now.
>  		 */
> -		WARN_ON(obj->base.write_domain & ~I915_GEM_DOMAIN_CPU);
> -		WARN_ON(obj->base.read_domains & ~I915_GEM_DOMAIN_CPU);
> +		if (obj->base.read_domains & I915_GEM_DOMAIN_CPU) {
> +			u32 old_read_domains, old_write_domain;
>  
> -		old_read_domains = obj->base.read_domains;
> -		old_write_domain = obj->base.write_domain;
> +			BUG_ON(obj->pages == NULL);
>  
> -		obj->base.read_domains = I915_GEM_DOMAIN_CPU;
> -		obj->base.write_domain = I915_GEM_DOMAIN_CPU;
> +			trace_i915_gem_object_clflush(obj);
> +			drm_clflush_sg(obj->pages);
>  
> -		trace_i915_gem_object_change_domain(obj,
> -						    old_read_domains,
> -						    old_write_domain);
> +			old_read_domains = obj->base.read_domains;
> +			old_write_domain = obj->base.write_domain;
> +
> +			obj->base.read_domains &= ~I915_GEM_DOMAIN_CPU;
> +			obj->base.write_domain &= ~I915_GEM_DOMAIN_CPU;
> +
> +			trace_i915_gem_object_change_domain(obj,
> +							    old_read_domains,
> +							    old_write_domain);
> +		}
>  	}
>  
>  	obj->cache_level = cache_level;
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Ville Syrjälä
Intel OTC