[PATCH] drm/i915: Always call fence-lost prior to removing the fence

[PATCH] drm/i915: Always call fence-lost prior to removing the fence

[Chris Wilson]

There is a minute window for a race between put-fence removing the fence
and for a new transaction by an external party on the GTT mmap. That is
we must zap the mmap prior to removing the fence and not afterwards.

Fixes regression from
commit 61050808bb019ebea966b7b5bfd357aaf219fb51
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Tue Apr 17 15:31:31 2012 +0100

    drm/i915: Refactor put_fence() to use the common fence writing routine

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: stable@vger.kernel.org # regression introduced in v3.5
---
 drivers/gpu/drm/i915/i915_gem.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a92d431..c9f9a92 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2124,11 +2124,11 @@ static void i915_gem_reset_fences(struct drm_device *dev)
 	for (i = 0; i < dev_priv->num_fence_regs; i++) {
 		struct drm_i915_fence_reg *reg = &dev_priv->fence_regs[i];
 
-		i915_gem_write_fence(dev, i, NULL);
-
 		if (reg->obj)
 			i915_gem_object_fence_lost(reg->obj);
 
+		i915_gem_write_fence(dev, i, NULL);
+
 		reg->pin_count = 0;
 		reg->obj = NULL;
 		INIT_LIST_HEAD(&reg->lru_list);
@@ -2760,10 +2760,10 @@ i915_gem_object_put_fence(struct drm_i915_gem_object *obj)
 	if (obj->fence_reg == I915_FENCE_REG_NONE)
 		return 0;
 
+	i915_gem_object_fence_lost(obj);
 	i915_gem_object_update_fence(obj,
 				     &dev_priv->fence_regs[obj->fence_reg],
 				     false);
-	i915_gem_object_fence_lost(obj);
 
 	return 0;
 }
-- 
1.7.10.4

[PATCH] drm/i915: Always call fence-lost prior to removing the fence

[Chris Wilson]

There is a minute window for a race between put-fence removing the fence
and for a new transaction by an external party on the GTT mmap. That is
we must zap the mmap prior to removing the fence and not afterwards.

Fixes regression from
commit 61050808bb019ebea966b7b5bfd357aaf219fb51
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Tue Apr 17 15:31:31 2012 +0100

    drm/i915: Refactor put_fence() to use the common fence writing routine

v2: Remember the fence to remove with a local variable (gcc)

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: stable@vger.kernel.org # regression introduced in v3.5
---
 drivers/gpu/drm/i915/i915_gem.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a92d431..85b5f56 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2124,11 +2124,11 @@ static void i915_gem_reset_fences(struct drm_device *dev)
 	for (i = 0; i < dev_priv->num_fence_regs; i++) {
 		struct drm_i915_fence_reg *reg = &dev_priv->fence_regs[i];
 
-		i915_gem_write_fence(dev, i, NULL);
-
 		if (reg->obj)
 			i915_gem_object_fence_lost(reg->obj);
 
+		i915_gem_write_fence(dev, i, NULL);
+
 		reg->pin_count = 0;
 		reg->obj = NULL;
 		INIT_LIST_HEAD(&reg->lru_list);
@@ -2751,6 +2751,7 @@ int
 i915_gem_object_put_fence(struct drm_i915_gem_object *obj)
 {
 	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
+	struct drm_i915_fence_reg *fence;
 	int ret;
 
 	ret = i915_gem_object_wait_fence(obj);
@@ -2760,10 +2761,10 @@ i915_gem_object_put_fence(struct drm_i915_gem_object *obj)
 	if (obj->fence_reg == I915_FENCE_REG_NONE)
 		return 0;
 
-	i915_gem_object_update_fence(obj,
-				     &dev_priv->fence_regs[obj->fence_reg],
-				     false);
+	fence = &dev_priv->fence_regs[obj->fence_reg];
+
 	i915_gem_object_fence_lost(obj);
+	i915_gem_object_update_fence(obj, fence, false);
 
 	return 0;
 }
-- 
1.7.10.4

Re: [Intel-gfx] [PATCH] drm/i915: Always call fence-lost prior to removing the fence

[Imre Deak]

On Tue, 2013-03-26 at 11:29 +0000, Chris Wilson wrote:
> There is a minute window for a race between put-fence removing the fence
> and for a new transaction by an external party on the GTT mmap. That is
> we must zap the mmap prior to removing the fence and not afterwards.
> 
> Fixes regression from
> commit 61050808bb019ebea966b7b5bfd357aaf219fb51
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Tue Apr 17 15:31:31 2012 +0100
> 
>     drm/i915: Refactor put_fence() to use the common fence writing routine
> 
> v2: Remember the fence to remove with a local variable (gcc)
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: stable@vger.kernel.org # regression introduced in v3.5

Reviewed-by: Imre Deak <imre.deak@intel.com>

Re: [Intel-gfx] [PATCH] drm/i915: Always call fence-lost prior to removing the fence

[Daniel Vetter]

On Tue, Mar 26, 2013 at 04:25:58PM +0200, Imre Deak wrote:
> On Tue, 2013-03-26 at 11:29 +0000, Chris Wilson wrote:
> > There is a minute window for a race between put-fence removing the fence
> > and for a new transaction by an external party on the GTT mmap. That is
> > we must zap the mmap prior to removing the fence and not afterwards.
> > 
> > Fixes regression from
> > commit 61050808bb019ebea966b7b5bfd357aaf219fb51
> > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > Date:   Tue Apr 17 15:31:31 2012 +0100
> > 
> >     drm/i915: Refactor put_fence() to use the common fence writing routine
> > 
> > v2: Remember the fence to remove with a local variable (gcc)
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: stable@vger.kernel.org # regression introduced in v3.5
> 
> Reviewed-by: Imre Deak <imre.deak@intel.com>

stable rules say "no theoretical races", and I think we don't even have a
testcase for this. Hence queued for -next and dropped cc: stable, thanks
for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch