[Intel-gfx] [PATCH 1/2] drm/i915: Unconditionally flush any chipset buffers before execbuf

[Chris Wilson]

On Thu, Aug 18, 2016 at 04:59:35PM +0300, Mika Kuoppala wrote:
> Chris Wilson <chris at chris-wilson.co.uk> writes:
> 
> > If userspace is asynchronously streaming into the batch or other
> > execobjects, we may not flush those writes along with a change in cache
> > domain (as there is no change). Therefore those writes may end up in
> > internal chipset buffers and not visible to the GPU upon execution. We
> > must issue a flush command or otherwise we encounter incoherency in the
> > batchbuffers and the GPU executing invalid commands (i.e. hanging) quite
> > regularly.
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90841
> > Fixes: 1816f9236303 ("drm/i915: Support creation of unbound wc user...")
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Akash Goel <akash.goel at intel.com>
> > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> > Tested-by: Matti Hämäläinen <ccr at tnsp.org>
> > Cc: stable at vger.kernel.org
> > ---
> >  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 13 +++----------
> >  1 file changed, 3 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > index 699315304748..bce587abc601 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -1015,8 +1015,6 @@ i915_gem_execbuffer_move_to_gpu(struct drm_i915_gem_request *req,
> >  {
> >  	const unsigned int other_rings = eb_other_engines(req);
> >  	struct i915_vma *vma;
> > -	uint32_t flush_domains = 0;
> > -	bool flush_chipset = false;
> >  	int ret;
> >  
> >  	list_for_each_entry(vma, vmas, exec_list) {
> > @@ -1029,16 +1027,11 @@ i915_gem_execbuffer_move_to_gpu(struct drm_i915_gem_request *req,
> >  		}
> >  
> >  		if (obj->base.write_domain & I915_GEM_DOMAIN_CPU)
> > -			flush_chipset |= i915_gem_clflush_object(obj, false);
> > -
> > -		flush_domains |= obj->base.write_domain;
> > +			i915_gem_clflush_object(obj, false);
> >  	}
> >  
> > -	if (flush_chipset)
> > -		i915_gem_chipset_flush(req->engine->i915);
> > -
> > -	if (flush_domains & I915_GEM_DOMAIN_GTT)
> > -		wmb();
> 
> Was a bit worried about this vanishing.
> 
> But after chatting with Chris and also founding this:
> https://lwn.net/Articles/174655/
> [in I386 AND X86_64 SPECIFIC NOTES]
> 
> I am convinced that the uncached write to force the chipset_flush
> will be strong barrier enough.
> 
> Reviewed-by: Mika Kuoppala <mika.kuoppala at intel.com>

Fwiw, the v2 that was meant to be here puts the wmb() back into
i915_gem_chipset_flush() so that we always have at least an sfence. It
should be redundant, but it shouldn't be noticeable either.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre