Re: [PATCH] drm/i915: Drop the overzealous warning from i915_gem_set_cache_level

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Aug 12, 2013 at 11:46:17AM +0100, Chris Wilson wrote:
> By our earlier reckoning, move from a snooped/llc setting to an uncached
> setting, leaves the CPU cache in a consistent state irrespective of our
> domain tracking - so we can forgo the warning about the lack of
> invalidation. Similarly for any writes posted to the snooped CPU domain,
> we know will be safely clflushed to the uncached PTEs after forcing the
> domain change.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 925c77d..1d3e57e 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3520,7 +3520,6 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
>  		 * Just set it to the CPU cache for now.
>  		 */
>  		WARN_ON(obj->base.write_domain & ~I915_GEM_DOMAIN_CPU);
> -		WARN_ON(obj->base.read_domains & ~I915_GEM_DOMAIN_CPU);

AFAICS this can only be reached by stolen objs starting in GTT read
domain. Normally set_cache_level checks if the object is bound and
then calls finish_gtt, and unbind also calls finish_gtt, and GPU
domain is handled in a similar way. So I don't see that we can end up
here any other way. Based on that, both WARNs seem rather pointless
actually.

Then again I'm not really sure what we gain from setting stolen objs
to GTT read domain initially.

The write domain check might make a bit of sense, except for the fact
that finish_gtt/gpu clears it just before.

Thinking about this stuff a bit, I think I actually came up with a
scenario where we would currently fail to invalidate the CPU cache
between non-snooped GPU/GTT access and CPU access:

1. make bo non-snooped w/ pin_display=true (wd=0, rd|=gtt)
2. set to CPU read domain (wd=0 rd|=cpu)
3. set to GTT (or GPU) write domain (wd=gtt, rd=gtt) -> CPU cache is stale after this point
4. make bo snooped -> pin_display=true still so we directly set (wd=cpu, rd=cpu)
5. set to CPU domain -> CPU cache is still stale

>  		old_read_domains = obj->base.read_domains;
>  		old_write_domain = obj->base.write_domain;
> -- 
> 1.8.4.rc2

-- 
Ville Syrjälä
Intel OTC