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

[Daniel Vetter <daniel@ffwll.ch>]

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@chris-wilson.co.uk>
> Cc: stable@vger.kernel.org
> ---
>  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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch