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@xxxxxxxx> > > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
