On Thu, Mar 21, 2013 at 03:30:19PM +0000, Chris Wilson 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 > write the fence from each cpu taking care to serialise memory accesses > on each. The usual mb(), or even a mb() on each CPU is not enough to > ensure that access to the fenced region is coherent across the change in > fence register. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=62191 > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > Cc: stable@xxxxxxxxxxxxxxx > --- > drivers/gpu/drm/i915/i915_gem.c | 34 ++++++++++++++++++++++++++++++---- > 1 file changed, 30 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 19fc21b..fe34240 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2684,17 +2684,43 @@ static inline int fence_number(struct drm_i915_private *dev_priv, > return fence - dev_priv->fence_regs; > } > > +struct write_fence { > + struct drm_device *dev; > + struct drm_i915_gem_object *obj; > + int fence; > +}; > + > +static void i915_gem_write_fence__ipi(void *data) > +{ > + struct write_fence *args = data; > + i915_gem_write_fence(args->dev, args->fence, args->obj); > +} > + > 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 write_fence args = { > + .dev = obj->base.dev, > + .fence = fence_number(dev_priv, fence), > + .obj = enable ? obj : NULL, > + }; > + > + /* 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 write the fence from each cpu taking > + * care to serialise memory accesses on each. The usual mb(), > + * or even a mb() on each CPU is not enough to ensure that access > + * to the fenced region is coherent across the change in fence > + * register. > + */ > + if (!HAS_LLC(obj->base.dev) || > + on_each_cpu(i915_gem_write_fence__ipi, &args, 1) != 0) > + i915_gem_write_fence__ipi(&args); I think the if condition here is a notch to clever and hides the elefantentöter a bit too well. on_each_cpu always calls the given function unconditionally, even when the ipi function call fails, so if (!HAS_LLC) WARN_ON(on_each_cpu); else i915_gem_write_fence looks clearer to me. -Daniel > > if (enable) { > - obj->fence_reg = reg; > + obj->fence_reg = args.fence; > fence->obj = obj; > list_move_tail(&fence->lru_list, &dev_priv->mm.fence_list); > } else { > -- > 1.7.10.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html