[Intel-gfx] [PATCH v4] drm/i915: Split obj->cache_coherent to track r/w

[Chris Wilson]

Quoting Joonas Lahtinen (2017-08-15 15:34:38)
> On Fri, 2017-08-11 at 12:11 +0100, Chris Wilson wrote:
> > Another month, another story in the cache coherency saga. This time, we
> > come to the realisation that i915_gem_object_is_coherent() has been
> > reporting whether we can read from the target without requiring a cache
> > invalidate; but we were using it in places for testing whether we could
> > write into the object without requiring a cache flush. So split the
> > tracking into two, one to decide before reads, one after writes.
> > 
> > See commit e27ab73d17ef ("drm/i915: Mark CPU cache as dirty on every
> > transition for CPU writes") for the previous entry in this saga.
> > 
> > v2: Be verbose
> > v3: Remove unused function (i915_gem_object_is_coherent)
> > v4: Fix inverted coherency check prior to execbuf (from v2)
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101109
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101555
> > Testcase: igt/kms_mmap_write_crc
> > Testcase: igt/kms_pwrite_crc
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > Cc: Dongwon Kim <dongwon.kim at intel.com>
> > Cc: Matt Roper <matthew.d.roper at intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> > Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> > Tested-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > Acked-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> 
> <SNIP>
> 
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -1842,7 +1842,13 @@ static int eb_move_to_gpu(struct i915_execbuffer *eb)
> >                       eb->request->capture_list = capture;
> >               }
> >  
> > -             if (unlikely(obj->cache_dirty && !obj->cache_coherent)) {
> > +             /*
> > +              * If the GPU is not _reading_ through the CPU cache, we need
> > +              * to make sure that any writes (both previous GPU writes from
> > +              * before a change in snooping levels and normal CPU writes)
> > +              * caught in that cache are flushed to main memory.
> > +              */
> > +             if (unlikely(obj->cache_dirty & ~obj->cache_coherent)) {
> 
> I'd rather see this as "obj->cache_dirty && !(obj->cache_coherent &
> .._READ)" but if GCC is doing such a poor job, do add a comment here,
> then this is:

Added the comment, should tie nicely into any grepping if we need.
 
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>

Thanks, pushed. Now to poke you for some late execbuf patches...
-Chris