On Wed, 30 Mar 2011 08:09:47 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> The series looks really good, only one quibble below.
> 
> On Tue, 29 Mar 2011 16:59:53 -0700, Eric Anholt <eric@anholt.net> wrote:
> > +int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> > +				    enum i915_cache_level cache_level)
> > +	if (cache_level == I915_CACHE_NONE) {
> > +		/* If we're coming frm LLC cached, then we haven't
> > +		 * actually been tracking whether the data is in the
> > +		 * CPU cache or not, since we only allow one bit set
> > +		 * in obj->write_domain.  Just set it to the CPU cache
> > +		 * for now.
> > +		 */
> > +		BUG_ON(obj->base.write_domain & I915_GEM_GPU_DOMAINS);
> > +		obj->base.write_domain = I915_GEM_DOMAIN_CPU;
> > +	}
> 
> [We can rearrange the code to convert the BUG_ON into a
>  if (WARN_ON()) return -EBUSY;.]
> 
> I think this is overkill, at least by my interpretation of the old BLT
> docs which imply that cache line invalidation (both CPU and GPU) is done
> for snooped PTEs on MI_FLUSH.

And what about a CPU write through the GTT?  Or the CPU writes to the
pages before we turned them into a BO and cleared their CPU write domain
flag without actually clflushing them?