From mboxrd@z Thu Jan  1 00:00:00 1970
From: Daniel Vetter <daniel@ffwll.ch>
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Workaround incoherence between
 fences and LLC across multiple CPUs
Date: Fri, 22 Mar 2013 12:36:32 +0100
Message-ID: <20130322113632.GS9021@phenom.ffwll.local>
References: <1363879819-6943-1-git-send-email-chris@chris-wilson.co.uk>
Mime-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Transfer-Encoding: QUOTED-PRINTABLE
Return-path: <stable-owner@vger.kernel.org>
Content-Disposition: inline
In-Reply-To: <1363879819-6943-1-git-send-email-chris@chris-wilson.co.uk>
Sender: stable-owner@vger.kernel.org
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: intel-gfx@lists.freedesktop.org, stable@vger.kernel.org
List-Id: intel-gfx@lists.freedesktop.org

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 updat=
e
> to the fence register we need to take extreme measures on SNB+, and
> write the fence from each cpu taking care to serialise memory accesse=
s
> 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.
>=20
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=3D62191
> 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(-)
>=20
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i=
915_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_i91=
5_private *dev_priv,
>  	return fence - dev_priv->fence_regs;
>  }
> =20
> +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 =3D 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 =3D obj->base.dev->dev_private;
> -	int reg =3D fence_number(dev_priv, fence);
> -
> -	i915_gem_write_fence(obj->base.dev, reg, enable ? obj : NULL);
> +	struct write_fence args =3D {
> +		.dev =3D obj->base.dev,
> +		.fence =3D fence_number(dev_priv, fence),
> +		.obj =3D 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) !=3D 0)
> +		i915_gem_write_fence__ipi(&args);

I think the if condition here is a notch to clever and hides the
elefantent=F6ter a bit too well. on_each_cpu always calls the given fun=
ction
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

> =20
>  	if (enable) {
> -		obj->fence_reg =3D reg;
> +		obj->fence_reg =3D args.fence;
>  		fence->obj =3D obj;
>  		list_move_tail(&fence->lru_list, &dev_priv->mm.fence_list);
>  	} else {
> --=20
> 1.7.10.4
>=20
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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