[Intel-gfx] [PATCH] drm/i915: Performed deferred clflush inside set-cache-level

[Daniel Vetter]

On Thu, Jan 15, 2015 at 08:54:54AM +0000, Chris Wilson wrote:
> On Wed, Jan 14, 2015 at 08:46:09PM +0100, Daniel Vetter wrote:
> > > + if (obj->cache_dirty &&
> > > +    obj->base.write_domain != I915_GEM_DOMAIN_CPU &&
> > > +    cpu_write_needs_clflush(obj) &&
> > > +    i915_gem_clflush_object(obj, true))
> > 
> > Imo hiding the actual action in the if condition like this is a bit too
> > evil.
> 
> Split it out into 2 ifs:
> 
> if (cache_dirty && !not-in-cpu-cache && needs_clflush)
>    if (i915_gem_clflush_object(obj, true))
>        i915_gem_chipset_flush();

With that applied this is Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> 
> Moving the chipset_flush around can tidy this up (at the expense of some
> brain power to only do the flush when required).

Wrt cleanups I think more clarity would come from pushing the decision
whether the clflush should be forced (with the bool force argument for
both clflush_object and flush_cpu_write_domain) and replace it with a bool
write. The clflush_object could switch between cpu_cache_is_coherent and
cpu_write_needs_clflush. Probably we could even inline the later.

Or do I miss something?
-Daniel

> 
> > Also, can we please have a testcase to at lest exercise the
> > codepath? It sounds like a real functional tests using crc is a bit more
> > work, but just poking at the WARN_ON would be good already.
> 
> Testcase: igt/gem_mmap_wc/set-cache-level
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch