[Intel-gfx] [PATCH] drm/i915: Performed deferred clflush inside set-cache-level

[Chris Wilson]

On Tue, Jan 13, 2015 at 10:23:55PM +0200, Ville Syrjälä wrote:
> On Tue, Jan 13, 2015 at 01:32:52PM +0000, Chris Wilson wrote:
> > Currently we are hitting the WARN inside
> > i915_gem_object_set_cache_level() as we can now have an unbound object
> > in the GTT write domain (due to 43566dedde54f9 "drm/i915: Broaden
> > application of set-domain(GTT)"). To avoid the warning, we need to track
> > when we elided the clflush on a cacheable object and then evict the
> > cache for the object when we move the object out of a cacheable domain.
> > 
> > Reported-by: Jani Nikula <jani.nikula at linux.intel.com>
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Jani Nikula <jani.nikula at linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h |  1 +
> >  drivers/gpu/drm/i915/i915_gem.c | 32 +++++++++-----------------------
> >  2 files changed, 10 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index bf18a5238887..7070482000cd 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1998,6 +1998,7 @@ struct drm_i915_gem_object {
> >  	 */
> >  	unsigned long gt_ro:1;
> >  	unsigned int cache_level:3;
> > +	unsigned int cache_dirty:1;
> >  
> >  	unsigned int has_dma_mapping:1;
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 9bafe50d3df7..aa089e7c31bf 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3639,11 +3639,14 @@ i915_gem_clflush_object(struct drm_i915_gem_object *obj,
> >  	 * snooping behaviour occurs naturally as the result of our domain
> >  	 * tracking.
> >  	 */
> > -	if (!force && cpu_cache_is_coherent(obj->base.dev, obj->cache_level))
> > +	if (!force && cpu_cache_is_coherent(obj->base.dev, obj->cache_level)) {
> > +		obj->cache_dirty = true;
> 
> Hmm. This would mark the cache dirty even when just moving to the CPU read
> domain. A subsequent set_cache_level might then flush a bit needlessly.
> But maybe that's rare enoguh to ignore?

Right, we err on the side of doing an extra clflush on cache-level
transitions. I thought keeping the rule as simple as always do the
clflush if we have skipped any clflushes was a good idea. And yes,
moving dirty objects into uncached is rare, roughly once every 10s on a
bad day.
 
> I suppose we could follow this up with another patch to fix
> kms_pwrite_crc using cache_dirty as well. But actually that would
> require doing the clflush even when the cache_level didn't change,
> which might aggravate the needless flushing issue somewhat. Or
> perhaps we should just add another cache_dirty dependent flush to
> i915_gem_object_pin_to_display_plane() to deal with that particular
> problem?

I like the idea. I think it will help document the requirements if done
well.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre