[Intel-gfx] [PATCH v3] drm/i915 : Avoid superfluous invalidation of CPU cache lines

[Chris Wilson]

On Tue, Dec 01, 2015 at 02:34:41PM +0200, Ville Syrjälä wrote:
> On Mon, Nov 30, 2015 at 12:41:05PM +0530, akash.goel at intel.com wrote:
> > @@ -3982,7 +3983,21 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write)
> >  
> >  	/* Flush the CPU cache if it's still invalid. */
> >  	if ((obj->base.read_domains & I915_GEM_DOMAIN_CPU) == 0) {
> > -		i915_gem_clflush_object(obj, false);
> > +		/* If an object is moved out of the CPU domain following a
> > +		 * CPU write and before a GPU or GTT write, we will clflush
> > +		 * it out of the CPU cache, and mark the cache as clean.
> > +		 * After clflushing we know that this object cannot be in the
> > +		 * CPU cache, nor can it be speculatively loaded into the CPU
> > +		 * cache as our objects are page-aligned (& speculation cannot
> > +		 * cross page boundaries). Whilst this flag is set, we know
> > +		 * that any future access to the object's pages will miss the
> > +		 * stale cache and have to be serviced from main memory, i.e.
> > +		 * we do not need another clflush to invalidate the CPU cache
> > +		 * in preparing to read from the object.
> > +		 */
> > +		if (!obj->cache_clean)
> > +			i915_gem_clflush_object(obj, false);
> > +		obj->cache_clean = false;
> 
> Having the comment here talk about moving stuff out of the cpu domain
> made me think there's a bug here (false vs. true). But actually this
> code moves it into the cpu domain so it's actually fine, I wonder if
> there's a better place for the comment (eg. where we do set
> cache_clean=true)?

I thought it made more sense here because this is where we playing the
trick to avoid the clflush.

Hmm, would s/If an object/When the object/ and
s/cache_clean/cache_flushed/ suffice?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre