Re: [Intel-gfx] [PATCH] drm/i915: Workaround incoherence between fences and LLC across multiple CPUs

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Apr 05, 2013 at 09:03:27AM -0700, Jesse Barnes wrote:
> On Thu,  4 Apr 2013 21:31:03 +0100
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > In order to fully serialize access to the fenced region and the update
> > to the fence register we need to take extreme measures on SNB+, and
> > manually flush writes to memory prior to writing the fence register in
> > conjunction with the memory barriers placed around the register write.
> > 
> > Fixes i-g-t/gem_fence_thrash
> > 
> > v2: Bring a bigger gun
> > v3: Switch the bigger gun for heavier bullets (Arjan van de Ven)
> > v4: Remove changes for working generations.
> > v5: Reduce to a per-cpu wbinvd() call prior to updating the fences.
> > v6: Rewrite comments to ellide forgotten history.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=62191
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> > Tested-by: Jon Bloomfield <jon.bloomfield@intel.com> (v2)
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c |   28 +++++++++++++++++++++++-----
> >  1 file changed, 23 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index fa4ea1a..8f7739e 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -2689,17 +2689,35 @@ static inline int fence_number(struct drm_i915_private *dev_priv,
> >  	return fence - dev_priv->fence_regs;
> >  }
> >  
> > +static void i915_gem_write_fence__ipi(void *data)
> > +{
> > +	wbinvd();
> > +}
> > +
> >  static void i915_gem_object_update_fence(struct drm_i915_gem_object *obj,
> >  					 struct drm_i915_fence_reg *fence,
> >  					 bool enable)
> >  {
> > -	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
> > -	int reg = fence_number(dev_priv, fence);
> > -
> > -	i915_gem_write_fence(obj->base.dev, reg, enable ? obj : NULL);
> > +	struct drm_device *dev = obj->base.dev;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	int fence_reg = fence_number(dev_priv, fence);
> > +
> > +	/* In order to fully serialize access to the fenced region and
> > +	 * the update to the fence register we need to take extreme
> > +	 * measures on SNB+. In theory, the write to the fence register
> > +	 * flushes all memory transactions before, and coupled with the
> > +	 * mb() placed around the register write we serialise all memory
> > +	 * operations with respect to the changes in the tiler. Yet, on
> > +	 * SNB+ we need to take a step further and emit an explicit wbinvd()
> > +	 * on each processor in order to manually flush all memory
> > +	 * transactions before updating the fence register.
> > +	 */
> > +	if (HAS_LLC(obj->base.dev))
> > +		on_each_cpu(i915_gem_write_fence__ipi, NULL, 1);
> > +	i915_gem_write_fence(dev, fence_reg, enable ? obj : NULL);
> >  
> >  	if (enable) {
> > -		obj->fence_reg = reg;
> > +		obj->fence_reg = fence_reg;
> >  		fence->obj = obj;
> >  		list_move_tail(&fence->lru_list, &dev_priv->mm.fence_list);
> >  	} else {
> 
> I almost wish x86 just gave up and went fully weakly ordered.  At least
> then we'd know we need barriers everywhere, rather than just in
> mystical places.
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Queued for -next, thanks for the patch. I am a bit unhappy that the
testcase doesn't work too well and can't reliably reproduce this on all
affected machines. But I've tried a bit to improve things and it's very
fickle. I guess we just have to accept this :(
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch