Intel-GFX Archive on lore.kernel.org
 help / color / Atom feed
From: Daniel Vetter <daniel.vetter@ffwll.ch>
To: Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Subject: [PATCH 6/6] drm/i915: create a race-free reset detection
Date: Wed, 14 Nov 2012 17:14:08 +0100
Message-ID: <1352909648-21514-7-git-send-email-daniel.vetter@ffwll.ch> (raw)
In-Reply-To: <1352909648-21514-1-git-send-email-daniel.vetter@ffwll.ch>

With the previous patch the state transition handling of the reset
code itself is now (hopefully) race free and solid. But that still
leaves out everyone else - with the various lock-free wait paths
we have there's the possibility that the reset happens between the
point where we read the seqno we should wait on and the actual wait.

And if __wait_seqno then never sees the RESET_IN_PROGRESS state, we'll
happily wait for a seqno which will in all likelyhood never signal.

In practice this is not a big problem since the X server gets
constantly interrupted, and can then submit more work (hopefully) to
unblock everyone else: As soon as a new seqno write lands, all waiters
will unblock. But running the i-g-t reset testcase ZZ_hangman can
expose this race, especially on slower hw with fewer cpu cores.

Now looking forward to ARB_robustness and friends that's not the best
possible behaviour, hence this patch adds a reset_counter to be able
to detect any reset, even if a given thread never observed the
in-progress state.

The important part is to correctly order things:
- The write side needs to increment the counter after any seqno gets
  reset.  Hence we need to do that at the end of the reset work, and
  again wake everyone up. We also need to place a barrier in between
  any possible seqno changes and the counter increment, since any
  unlock operations only guarantee that nothing leaks out, but not
  that at later load operation gets moved ahead.
- On the read side we need to ensure that no reset can sneak in and
  invalidate the seqno. In all cases we can use the one-sided barrier
  that unlock operations guarantee (of the lock protecting the
  respective seqno/ring pair) to ensure correct ordering. Hence it is
  sufficient to place the atomic read before the mutex/spin_unlock and
  no additional barriers are required.

The end-result of all this is that we need to wake up everyone twice
in a reset operation:
- First, before the reset starts, to get any lockholders of the locks,
  so that the reset can proceed.
- Second, after the reset is completed, to allow waiters to properly
  and reliably detect the reset condition and bail out.

I admit that this entire reset_counter thing smells a bit like
overkill, but I think it's justified since it makes it really explicit
what the bail-out condition is. And we need a reset counter anyway to
implement ARB_robustness, and imo with finer-grained locking on the
horizont this is the most resilient scheme I could think of.

v2: Drop spurious change in the wait_for_error EXIT_COND - we only
need to wait until we leave the reset-in-progress wedged state.

v3: Don't play tricks with barriers in the throttle ioctl, the
spin_unlock is barrier enough.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h | 13 +++++++++++++
 drivers/gpu/drm/i915/i915_gem.c | 34 +++++++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/i915_irq.c | 16 ++++++++++++++++
 3 files changed, 58 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 423541b..b5ad2a8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -739,6 +739,19 @@ struct i915_gpu_error {
 #define I915_WEDGED		2
 
 	/**
+	 * Reset counter
+	 *
+	 * This is used by the wait_seqno code to race-free noticed that a reset
+	 * event happened and that it needs to restart the entire ioctl (since
+	 * most likely the seqno it waited for won't ever signal anytime soon).
+	 *
+	 * This is important for lock-free wait paths, where no contended lock
+	 * naturally enforces the correct ordering between the bail-out of the
+	 * waiter and the gpu reset work code.
+	 */
+	atomic_t reset_counter;
+
+	/**
 	 * Waitqueue to signal when the reset has completed. Used by clients
 	 * that wait for dev_priv->mm.wedged to settle.
 	 */
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6112a4f..f2c031b 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -975,13 +975,22 @@ i915_gem_check_olr(struct intel_ring_buffer *ring, u32 seqno)
  * __wait_seqno - wait until execution of seqno has finished
  * @ring: the ring expected to report seqno
  * @seqno: duh!
+ * @reset_counter: reset sequence associated with the given seqno
  * @interruptible: do an interruptible wait (normally yes)
  * @timeout: in - how long to wait (NULL forever); out - how much time remaining
  *
+ * Note: It is of utmost importance that the passed in seqno and reset_counter
+ * values have been read by the caller in an smb safe manner. Where read-side
+ * locks are involved, it is sufficient to read the reset_counter before
+ * unlocking the lock that protects the seqno. For lockless tricks, the
+ * reset_counter _must_ be read before, and an appropriate smb_rmb must be
+ * inserted.
+ *
  * Returns 0 if the seqno was found within the alloted time. Else returns the
  * errno with remaining time filled in timeout argument.
  */
 static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno,
+			unsigned reset_counter,
 			bool interruptible, struct timespec *timeout)
 {
 	drm_i915_private_t *dev_priv = ring->dev->dev_private;
@@ -1011,7 +1020,8 @@ static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno,
 
 #define EXIT_COND \
 	(i915_seqno_passed(ring->get_seqno(ring, false), seqno) || \
-	atomic_read(&dev_priv->gpu_error.wedged))
+	atomic_read(&dev_priv->gpu_error.wedged) || \
+	(reset_counter != atomic_read(&dev_priv->gpu_error.reset_counter)))
 	do {
 		if (interruptible)
 			end = wait_event_interruptible_timeout(ring->irq_queue,
@@ -1021,6 +1031,11 @@ static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno,
 			end = wait_event_timeout(ring->irq_queue, EXIT_COND,
 						 timeout_jiffies);
 
+		/* We need to check whether any gpu reset happened in between
+		 * the caller grabbing the seqno and now. */
+		if (reset_counter != atomic_read(&dev_priv->gpu_error.reset_counter))
+			end = -EAGAIN;
+
 		ret = i915_gem_check_wedge(&dev_priv->gpu_error, interruptible);
 		if (ret)
 			end = ret;
@@ -1075,7 +1090,9 @@ i915_wait_seqno(struct intel_ring_buffer *ring, uint32_t seqno)
 	if (ret)
 		return ret;
 
-	return __wait_seqno(ring, seqno, interruptible, NULL);
+	return __wait_seqno(ring, seqno,
+			    atomic_read(&dev_priv->gpu_error.reset_counter),
+			    interruptible, NULL);
 }
 
 /**
@@ -1122,6 +1139,7 @@ i915_gem_object_wait_rendering__nonblocking(struct drm_i915_gem_object *obj,
 	struct drm_device *dev = obj->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_ring_buffer *ring = obj->ring;
+	unsigned reset_counter;
 	u32 seqno;
 	int ret;
 
@@ -1140,8 +1158,9 @@ i915_gem_object_wait_rendering__nonblocking(struct drm_i915_gem_object *obj,
 	if (ret)
 		return ret;
 
+	reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
 	mutex_unlock(&dev->struct_mutex);
-	ret = __wait_seqno(ring, seqno, true, NULL);
+	ret = __wait_seqno(ring, seqno, reset_counter, true, NULL);
 	mutex_lock(&dev->struct_mutex);
 
 	i915_gem_retire_requests_ring(ring);
@@ -2273,10 +2292,12 @@ i915_gem_object_flush_active(struct drm_i915_gem_object *obj)
 int
 i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
 {
+	drm_i915_private_t *dev_priv = dev->dev_private;
 	struct drm_i915_gem_wait *args = data;
 	struct drm_i915_gem_object *obj;
 	struct intel_ring_buffer *ring = NULL;
 	struct timespec timeout_stack, *timeout = NULL;
+	unsigned reset_counter;
 	u32 seqno = 0;
 	int ret = 0;
 
@@ -2317,9 +2338,10 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
 	}
 
 	drm_gem_object_unreference(&obj->base);
+	reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
 	mutex_unlock(&dev->struct_mutex);
 
-	ret = __wait_seqno(ring, seqno, true, timeout);
+	ret = __wait_seqno(ring, seqno, reset_counter, true, timeout);
 	if (timeout) {
 		WARN_ON(!timespec_valid(timeout));
 		args->timeout_ns = timespec_to_ns(timeout);
@@ -3384,6 +3406,7 @@ i915_gem_ring_throttle(struct drm_device *dev, struct drm_file *file)
 	unsigned long recent_enough = jiffies - msecs_to_jiffies(20);
 	struct drm_i915_gem_request *request;
 	struct intel_ring_buffer *ring = NULL;
+	unsigned reset_counter;
 	u32 seqno = 0;
 	int ret;
 
@@ -3403,12 +3426,13 @@ i915_gem_ring_throttle(struct drm_device *dev, struct drm_file *file)
 		ring = request->ring;
 		seqno = request->seqno;
 	}
+	reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
 	spin_unlock(&file_priv->mm.lock);
 
 	if (seqno == 0)
 		return 0;
 
-	ret = __wait_seqno(ring, seqno, true, NULL);
+	ret = __wait_seqno(ring, seqno, reset_counter, true, NULL);
 	if (ret == 0)
 		queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work, 0);
 
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 73d46f4..87f5031 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -843,9 +843,11 @@ static void i915_error_work_func(struct work_struct *work)
 	drm_i915_private_t *dev_priv = container_of(error, drm_i915_private_t,
 						    gpu_error);
 	struct drm_device *dev = dev_priv->dev;
+	struct intel_ring_buffer *ring;
 	char *error_event[] = { "ERROR=1", NULL };
 	char *reset_event[] = { "RESET=1", NULL };
 	char *reset_done_event[] = { "ERROR=0", NULL };
+	int i;
 
 	kobject_uevent_env(&dev->primary->kdev.kobj, KOBJ_CHANGE, error_event);
 
@@ -861,6 +863,20 @@ static void i915_error_work_func(struct work_struct *work)
 				   I915_WEDGED);
 		}
 
+		/*
+		 * After all the gem state is reset, increment the reset counter
+		 * and wake up everyone waiting for the reset to complete.
+		 *
+		 * Since unlock operations are a one-sided barrier only, we need
+		 * to insert a barrier here to order any seqno updates before
+		 * the counter increment.
+		 */
+		smp_mb__before_atomic_inc();
+		atomic_inc(&dev_priv->gpu_error.reset_counter);
+
+		for_each_ring(ring, dev_priv, i)
+			wake_up_all(&ring->irq_queue);
+
 		wake_up_all(&dev_priv->gpu_error.reset_queue);
 	}
 }
-- 
1.7.11.4

  parent reply index

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-11-14 16:14 [PATCH 0/6] robustify reset transitions Daniel Vetter
2012-11-14 16:14 ` [PATCH 1/6] drm/i915: move dev_priv->mm out of line Daniel Vetter
2012-12-04 16:31   ` Damien Lespiau
2012-11-14 16:14 ` [PATCH 2/6] drm/i915: extract hangcheck/reset/error_state state into substruct Daniel Vetter
2012-12-04 17:20   ` Damien Lespiau
2012-11-14 16:14 ` [PATCH 3/6] drm/i915: move wedged to the other gpu error handling stuff Daniel Vetter
2012-12-04 17:24   ` Damien Lespiau
2012-11-14 16:14 ` [PATCH 4/6] drm/i915: fix reset handling in the throttle ioctl Daniel Vetter
2012-12-05 14:08   ` Damien Lespiau
2012-11-14 16:14 ` [PATCH 5/6] drm/i915: clear up wedged transitions Daniel Vetter
2012-11-14 16:14 ` Daniel Vetter [this message]
2012-11-15 16:17 ` [PATCH 1/2] " Daniel Vetter
2012-11-15 16:17   ` [PATCH 2/2] drm/i915: create a race-free reset detection Daniel Vetter
2012-12-05 16:35     ` Damien Lespiau
2012-12-06  8:01       ` [PATCH] " Daniel Vetter
2012-12-06 15:23       ` [PATCH] drm/i915: clarify concurrent hang detect/gpu reset consistency Daniel Vetter
2013-01-18 20:48       ` [PATCH 2/2] drm/i915: create a race-free reset detection Daniel Vetter
2013-01-21 12:06         ` Damien Lespiau
2013-01-21 19:15           ` Daniel Vetter
2012-12-05 14:54   ` [PATCH 1/2] drm/i915: clear up wedged transitions Damien Lespiau
2012-12-05 16:38   ` Damien Lespiau
2012-12-05 17:14     ` Daniel Vetter
2014-09-03 20:26   ` Chris Wilson
2014-09-04  6:03     ` Daniel Vetter
2014-09-04  6:11       ` Chris Wilson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1352909648-21514-7-git-send-email-daniel.vetter@ffwll.ch \
    --to=daniel.vetter@ffwll.ch \
    --cc=intel-gfx@lists.freedesktop.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Intel-GFX Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/intel-gfx/0 intel-gfx/git/0.git
	git clone --mirror https://lore.kernel.org/intel-gfx/1 intel-gfx/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 intel-gfx intel-gfx/ https://lore.kernel.org/intel-gfx \
		intel-gfx@lists.freedesktop.org
	public-inbox-index intel-gfx

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.freedesktop.lists.intel-gfx


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git