[Intel-gfx] [PATCH v3] drm/i915: Move to CPU domain in pwrite/pread

[Chris Wilson]

On Fri, Nov 14, 2014 at 08:35:57PM +0200, Ville Syrjälä wrote:
> On Fri, Nov 14, 2014 at 05:00:59PM +0000, Chris Wilson wrote:
> > On Wed, Nov 12, 2014 at 11:47:14PM +0200, ville.syrjala at linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > 
> > > Currently it's possible to get visible cache dirt on scanout on LLC
> > > machines when using pwrite on the future scanout bo if its cache_level
> > > is already NONE.
> > > 
> > > pwrite's "does this need clflush?" checks would decide that no clflush
> > > is necessary since the bo isn't currently pinned to the display and LLC
> > > makes everything else coherent. The subsequent set_cache_level(NONE)
> > > would also do nothing since cache_level is already correct. And hence
> > > no clflush will be performed and we flip to a bo which can still have
> > > dirty data in the caches.
> > > 
> > > To correctly track the cache dirtyness move the object to CPU write
> > > domain in pwrite. This cures the cache dirt since we explicitly flush
> > > the CPU write domain in the pin_to_display path.
> > > 
> > > Give pread the same treatment simply in the name of symmetry.
> > > 
> > > v2: Use trace_i915_gem_object_change_domain() and provide some kind
> > >     of commit message
> > > v3: Don't mark things as clean if we're not sure everything got
> > >     flushed (Chris)
> > 
> > I think we just want to be more conservative during clflushes after
> > pwrite:
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 557746b2b72b..e9f98531b9d2 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -75,7 +75,7 @@ static bool cpu_cache_is_coherent(struct drm_device *dev,
> >  
> >  static bool cpu_write_needs_clflush(struct drm_i915_gem_object *obj)
> >  {
> > -       if (!cpu_cache_is_coherent(obj->base.dev, obj->cache_level))
> > +       if (level != I915_CACHE_NONE)
> 
> You mean == ?

Indeed.
 
> And I guess you'd then have to consider WT as well.
> 
> It would mean we'd end up clflushing even when not strictly needed. But
> maybe that's acceptable.

Yes. I think it is better to err on clflushing too often and worry later
if someone batters us over the head with a profile. The only mistake I
know about, in this regard, is my pwrite of the sprite planes and the
mmap(wc) fixes that, or I can flag those planes as fenced which may be
preferrable by hw (I didn't think so at the time)? The much more frequent
use case (and the only other one that immediately springs to mind) will
not trigger the clflushes.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre