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

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

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@chris-wilson.co.uk>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Dongwon Kim <dongwon.kim@intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Tested-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Acked-by: Maarten Lankhorst <maarten.lankhorst@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:

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx