Re: [PATCH v3] drm/i915 : Avoid superfluous invalidation of CPU cache lines

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

On Tue, Dec 01, 2015 at 01:09:33PM +0000, Chris Wilson wrote:
> 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@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?

Maybe 'When the object is eventually moved out...' ?

That extra word might convey more clearly that's it's not talking
about moving it out right now.

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