[Intel-gfx] [PATCH] drm/i915: Revoke fenced GTT mmapings across GPU reset

[Chris Wilson]

On Wed, Jan 04, 2017 at 03:36:16PM +0000, Tvrtko Ursulin wrote:
> 
> On 04/01/2017 14:51, Chris Wilson wrote:
> >During the fence registers are clobbered by a GPU reset. Hence if
> 
> During the reset I suppose, although it would sound still a bit
> redundant. So maybe "Since GPU reset clobbers the fence registers,
> if there is concurrent..."

I was just going to leave it as "The fence registers are clobbered by
a GPU reset. If..."

> >there is concurrent user access to a fenced region via a GTT mmaping,
> >the access will not be fenced during the reset (until we restore the
> >fences afterwards). In order to prevent invalid access during the reset,
> >before we clobber the fences we must invalidate the GTT mmapings. Access
> >to the mmap will then be forced to fault the page, and in handling the
> >fault, i915_gem_fault() will take the struct_mutex and wait upon the
> >reset to complete.
> >
> >Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99274
> >Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> >---
> > drivers/gpu/drm/i915/i915_drv.c           |  3 ++-
> > drivers/gpu/drm/i915/i915_drv.h           |  4 +++-
> > drivers/gpu/drm/i915/i915_gem.c           |  7 ++++++-
> > drivers/gpu/drm/i915/i915_gem_fence_reg.c | 24 ++++++++++++++++++++++++
> > 4 files changed, 35 insertions(+), 3 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >index 72e29fcb1638..39463fcab593 100644
> >--- a/drivers/gpu/drm/i915/i915_drv.c
> >+++ b/drivers/gpu/drm/i915/i915_drv.c
> >@@ -1780,6 +1780,7 @@ void i915_reset(struct drm_i915_private *dev_priv)
> > 	error->reset_count++;
> >
> > 	pr_notice("drm/i915: Resetting chip after gpu hang\n");
> >+	i915_gem_reset_prepare(dev_priv);
> >
> > 	disable_engines_irq(dev_priv);
> > 	ret = intel_gpu_reset(dev_priv, ALL_ENGINES);
> >@@ -1793,7 +1794,7 @@ void i915_reset(struct drm_i915_private *dev_priv)
> > 		goto error;
> > 	}
> >
> >-	i915_gem_reset(dev_priv);
> >+	i915_gem_reset_finish(dev_priv);
> > 	intel_overlay_reset(dev_priv);
> >
> > 	/* Ok, now get things going again... */
> >diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >index c035b6576efa..f16986cfa127 100644
> >--- a/drivers/gpu/drm/i915/i915_drv.h
> >+++ b/drivers/gpu/drm/i915/i915_drv.h
> >@@ -3355,7 +3355,8 @@ static inline u32 i915_reset_count(struct i915_gpu_error *error)
> > 	return READ_ONCE(error->reset_count);
> > }
> >
> >-void i915_gem_reset(struct drm_i915_private *dev_priv);
> >+void i915_gem_reset_prepare(struct drm_i915_private *dev_priv);
> >+void i915_gem_reset_finish(struct drm_i915_private *dev_priv);
> > void i915_gem_set_wedged(struct drm_i915_private *dev_priv);
> >
> > void i915_gem_clflush_init(struct drm_i915_private *i915);
> >@@ -3426,6 +3427,7 @@ i915_vm_to_ppgtt(struct i915_address_space *vm)
> > int __must_check i915_vma_get_fence(struct i915_vma *vma);
> > int __must_check i915_vma_put_fence(struct i915_vma *vma);
> >
> >+void i915_gem_revoke_fences(struct drm_i915_private *dev_priv);
> > void i915_gem_restore_fences(struct drm_i915_private *dev_priv);
> >
> > void i915_gem_detect_bit_6_swizzle(struct drm_i915_private *dev_priv);
> >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >index 73987014a7bd..019ea7cb13b5 100644
> >--- a/drivers/gpu/drm/i915/i915_gem.c
> >+++ b/drivers/gpu/drm/i915/i915_gem.c
> >@@ -2715,6 +2715,11 @@ static void reset_request(struct drm_i915_gem_request *request)
> > 	memset(vaddr + head, 0, request->postfix - head);
> > }
> >
> >+void i915_gem_reset_prepare(struct drm_i915_private *dev_priv)
> >+{
> >+	i915_gem_revoke_fences(dev_priv);
> >+}
> >+
> > static void i915_gem_reset_engine(struct intel_engine_cs *engine)
> > {
> > 	struct drm_i915_gem_request *request;
> >@@ -2780,7 +2785,7 @@ static void i915_gem_reset_engine(struct intel_engine_cs *engine)
> > 	spin_unlock_irqrestore(&engine->timeline->lock, flags);
> > }
> >
> >-void i915_gem_reset(struct drm_i915_private *dev_priv)
> >+void i915_gem_reset_finish(struct drm_i915_private *dev_priv)
> > {
> > 	struct intel_engine_cs *engine;
> > 	enum intel_engine_id id;
> >diff --git a/drivers/gpu/drm/i915/i915_gem_fence_reg.c b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
> >index 67835d7b39c7..447abb1105ab 100644
> >--- a/drivers/gpu/drm/i915/i915_gem_fence_reg.c
> >+++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
> >@@ -358,6 +358,30 @@ i915_vma_get_fence(struct i915_vma *vma)
> > }
> >
> > /**
> >+ * i915_gem_revoke_fences - revoke fence state
> >+ * @dev_priv: i915 device private
> >+ *
> >+ * Clears the hw fence state and removes all GTT mmappings via the fence. This
> 
> The function doesn't clear the hw fence state - reset does that, no?

The first version did, then realised all that was required was zapping
the mmap.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre