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

[Chris Wilson]

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. 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:

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