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

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

On to, 2017-08-10 at 17:20 +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 step in this saga.
> 
> 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>

<SNIP>

> +++ b/drivers/gpu/drm/i915/i915_gem_object.c
> @@ -0,0 +1,44 @@
> +/*
> + * Copyright © 2017 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + */
> +
> +#include "i915_drv.h"
> +#include "i915_gem_object.h"
> +

Kerneldoc for the two following functions needed.

> +static inline bool
> +i915_gem_object_is_coherent(struct drm_i915_gem_object *obj, bool write)
> +{
> +	if (obj->cache_level != I915_CACHE_NONE)
> +		return true;
> +
> +	return !write && HAS_LLC(to_i915(obj->base.dev));
> +}
> +
> +void i915_gem_object_set_cache_coherency(struct drm_i915_gem_object *obj,
> +					 unsigned int cache_level)
> +{
> +	obj->cache_level = cache_level;
> +	obj->cache_coherent_r = i915_gem_object_is_coherent(obj, false);
> +	obj->cache_coherent_w = i915_gem_object_is_coherent(obj, true);
> +	obj->cache_dirty = !obj->cache_coherent_w;
> +}
> 

Other than that, it seems OK. Although READ / WRITE might be more
explicit than true / false.

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