public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH v3 1/5] drm/i915: Report both waiters and success from intel_engine_wakeup()
@ 2017-02-24 18:01 Chris Wilson
  2017-02-24 18:01 ` [PATCH v3 2/5] drm/i915: Signal first fence from irq handler if complete Chris Wilson
                   ` (6 more replies)
  0 siblings, 7 replies; 15+ messages in thread
From: Chris Wilson @ 2017-02-24 18:01 UTC (permalink / raw)
  To: intel-gfx

The two users of the return value from intel_engine_wakeup() are
expecting different results. In the breadcrumbs hangcheck, we are using
it to determine whether wake_up_process() detected the waiter was
currently running (and if so we presume that it hasn't yet missed the
interrupt). However, in the fake_irq path, we are using the return value
as a check as to whether there are any waiters, and so we may
incorrectly stop the fake-irq if that waiter was currently running.

To handle the two different needs, return both bits of information!

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_breadcrumbs.c | 28 +++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_ringbuffer.h  | 26 +++-----------------------
 2 files changed, 30 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index 027c93e34c97..64e1b0c2d8b6 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -26,6 +26,32 @@
 
 #include "i915_drv.h"
 
+unsigned int intel_engine_wakeup(struct intel_engine_cs *engine)
+{
+	unsigned int ret = 0;
+
+	/* Note that for this not to dangerously chase a dangling pointer,
+	 * we must hold the rcu_read_lock here.
+	 *
+	 * Also note that tsk is likely to be in !TASK_RUNNING state so an
+	 * early test for tsk->state != TASK_RUNNING before wake_up_process()
+	 * is unlikely to be beneficial.
+	 */
+	if (intel_engine_has_waiter(engine)) {
+		struct task_struct *tsk;
+
+		ret = ENGINE_WAKEUP_WAITER;
+
+		rcu_read_lock();
+		tsk = rcu_dereference(engine->breadcrumbs.irq_seqno_bh);
+		if (tsk && !wake_up_process(tsk))
+			ret |= ENGINE_WAKEUP_ACTIVE;
+		rcu_read_unlock();
+	}
+
+	return ret;
+}
+
 static unsigned long wait_timeout(void)
 {
 	return round_jiffies_up(jiffies + DRM_I915_HANGCHECK_JIFFIES);
@@ -49,7 +75,7 @@ static void intel_breadcrumbs_hangcheck(unsigned long data)
 	 * to process the pending interrupt (e.g, low priority task on a loaded
 	 * system) and wait until it sleeps before declaring a missed interrupt.
 	 */
-	if (!intel_engine_wakeup(engine)) {
+	if (intel_engine_wakeup(engine) & ENGINE_WAKEUP_ACTIVE) {
 		mod_timer(&b->hangcheck, wait_timeout());
 		return;
 	}
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 0f29e07a9581..7d753dc1b89d 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -642,29 +642,9 @@ static inline bool intel_engine_has_waiter(const struct intel_engine_cs *engine)
 	return rcu_access_pointer(engine->breadcrumbs.irq_seqno_bh);
 }
 
-static inline bool intel_engine_wakeup(const struct intel_engine_cs *engine)
-{
-	bool wakeup = false;
-
-	/* Note that for this not to dangerously chase a dangling pointer,
-	 * we must hold the rcu_read_lock here.
-	 *
-	 * Also note that tsk is likely to be in !TASK_RUNNING state so an
-	 * early test for tsk->state != TASK_RUNNING before wake_up_process()
-	 * is unlikely to be beneficial.
-	 */
-	if (intel_engine_has_waiter(engine)) {
-		struct task_struct *tsk;
-
-		rcu_read_lock();
-		tsk = rcu_dereference(engine->breadcrumbs.irq_seqno_bh);
-		if (tsk)
-			wakeup = wake_up_process(tsk);
-		rcu_read_unlock();
-	}
-
-	return wakeup;
-}
+unsigned int intel_engine_wakeup(struct intel_engine_cs *engine);
+#define ENGINE_WAKEUP_WAITER BIT(0)
+#define ENGINE_WAKEUP_ACTIVE BIT(1)
 
 void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine);
 void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine);
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v3 2/5] drm/i915: Signal first fence from irq handler if complete
  2017-02-24 18:01 [PATCH v3 1/5] drm/i915: Report both waiters and success from intel_engine_wakeup() Chris Wilson
@ 2017-02-24 18:01 ` Chris Wilson
  2017-02-25 10:05   ` [PATCH v4] " Chris Wilson
  2017-02-24 18:01 ` [PATCH v3 3/5] drm/i915: Defer enabling hangcheck to the first fake breadcrumb interrupt Chris Wilson
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 15+ messages in thread
From: Chris Wilson @ 2017-02-24 18:01 UTC (permalink / raw)
  To: intel-gfx

As execlists and other non-semaphore multi-engine devices coordinate
between engines using interrupts, we can shave off a few 10s of
microsecond of scheduling latency by doing the fence signaling from the
interrupt as opposed to a RT kthread. (Realistically the delay adds
about 1% to an individual cross-engine workload.) We only signal the
first fence in order to limit the amount of work we move into the
interrupt handler. We also have to remember that our breadcrumbs may be
unordered with respect to the interrupt and so we still require the
waiter process to perform some heavyweight coherency fixups, as well as
traversing the tree of waiters.

v2: No need for early exit in irq handler - it breaks the flow between
patches and prevents the tracepoint

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h          | 13 ++++++-------
 drivers/gpu/drm/i915/i915_gem_request.c  |  2 +-
 drivers/gpu/drm/i915/i915_gem_request.h  |  2 ++
 drivers/gpu/drm/i915/i915_irq.c          | 32 +++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_breadcrumbs.c | 32 ++++++++------------------------
 drivers/gpu/drm/i915/intel_ringbuffer.h  |  8 ++++----
 6 files changed, 50 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 56caab35f216..16c5e7a37eba 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -4063,9 +4063,9 @@ __i915_request_irq_complete(const struct drm_i915_gem_request *req)
 	 * is woken.
 	 */
 	if (engine->irq_seqno_barrier &&
-	    rcu_access_pointer(engine->breadcrumbs.irq_seqno_bh) == current &&
 	    test_and_clear_bit(ENGINE_IRQ_BREADCRUMB, &engine->irq_posted)) {
-		struct task_struct *tsk;
+		struct intel_breadcrumbs *b = &engine->breadcrumbs;
+		unsigned long flags;
 
 		/* The ordering of irq_posted versus applying the barrier
 		 * is crucial. The clearing of the current irq_posted must
@@ -4087,17 +4087,16 @@ __i915_request_irq_complete(const struct drm_i915_gem_request *req)
 		 * the seqno before we believe it coherent since they see
 		 * irq_posted == false but we are still running).
 		 */
-		rcu_read_lock();
-		tsk = rcu_dereference(engine->breadcrumbs.irq_seqno_bh);
-		if (tsk && tsk != current)
+		spin_lock_irqsave(&b->lock, flags);
+		if (b->first_wait && b->first_wait->tsk != current)
 			/* Note that if the bottom-half is changed as we
 			 * are sending the wake-up, the new bottom-half will
 			 * be woken by whomever made the change. We only have
 			 * to worry about when we steal the irq-posted for
 			 * ourself.
 			 */
-			wake_up_process(tsk);
-		rcu_read_unlock();
+			wake_up_process(b->first_wait->tsk);
+		spin_unlock_irqrestore(&b->lock, flags);
 
 		if (__i915_gem_request_completed(req, seqno))
 			return true;
diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
index fbfeb5f8d069..77c3ee7a3fd0 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/i915_gem_request.c
@@ -1083,7 +1083,7 @@ long i915_wait_request(struct drm_i915_gem_request *req,
 	if (flags & I915_WAIT_LOCKED)
 		add_wait_queue(errq, &reset);
 
-	intel_wait_init(&wait);
+	intel_wait_init(&wait, req);
 
 restart:
 	do {
diff --git a/drivers/gpu/drm/i915/i915_gem_request.h b/drivers/gpu/drm/i915/i915_gem_request.h
index 5f73d8c0a38a..0efee879df23 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.h
+++ b/drivers/gpu/drm/i915/i915_gem_request.h
@@ -32,10 +32,12 @@
 
 struct drm_file;
 struct drm_i915_gem_object;
+struct drm_i915_gem_request;
 
 struct intel_wait {
 	struct rb_node node;
 	struct task_struct *tsk;
+	struct drm_i915_gem_request *request;
 	u32 seqno;
 };
 
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index bc70e2c451b2..7607aba35f26 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1033,12 +1033,38 @@ static void ironlake_rps_change_irq_handler(struct drm_i915_private *dev_priv)
 
 static void notify_ring(struct intel_engine_cs *engine)
 {
-	bool waiters;
+	struct drm_i915_gem_request *rq = NULL;
+	struct intel_wait *wait;
 
 	atomic_inc(&engine->irq_count);
 	set_bit(ENGINE_IRQ_BREADCRUMB, &engine->irq_posted);
-	waiters = intel_engine_wakeup(engine);
-	trace_intel_engine_notify(engine, waiters);
+
+	spin_lock(&engine->breadcrumbs.lock);
+	wait = engine->breadcrumbs.first_wait;
+	if (wait) {
+		/* We use a callback from the dma-fence to submit
+		 * requests after waiting on our own requests. To
+		 * ensure minimum delay in queuing the next request to
+		 * hardware, signal the fence now rather than wait for
+		 * the signaler to be woken up. We still wake up the
+		 * waiter in order to handle the irq-seqno coherency
+		 * issues (we may receive the interrupt before the
+		 * seqno is written, see __i915_request_irq_complete())
+		 * and to handle coalescing of multiple seqno updates
+		 * and many waiters.
+		 */
+		if (i915_seqno_passed(intel_engine_get_seqno(engine),
+				      wait->seqno))
+			rq = wait->request;
+
+		wake_up_process(wait->tsk);
+	}
+	spin_unlock(&engine->breadcrumbs.lock);
+
+	if (rq) /* rq is protected by RCU, so safe from hard-irq context */
+		dma_fence_signal(&rq->fence);
+
+	trace_intel_engine_notify(engine, wait);
 }
 
 static void vlv_c0_read(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index 64e1b0c2d8b6..ba5f72fb4890 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -28,26 +28,18 @@
 
 unsigned int intel_engine_wakeup(struct intel_engine_cs *engine)
 {
+	struct intel_wait *wait;
+	unsigned long flags;
 	unsigned int ret = 0;
 
-	/* Note that for this not to dangerously chase a dangling pointer,
-	 * we must hold the rcu_read_lock here.
-	 *
-	 * Also note that tsk is likely to be in !TASK_RUNNING state so an
-	 * early test for tsk->state != TASK_RUNNING before wake_up_process()
-	 * is unlikely to be beneficial.
-	 */
-	if (intel_engine_has_waiter(engine)) {
-		struct task_struct *tsk;
-
+	spin_lock_irqsave(&engine->breadcrumbs.lock, flags);
+	wait = engine->breadcrumbs.first_wait;
+	if (wait) {
 		ret = ENGINE_WAKEUP_WAITER;
-
-		rcu_read_lock();
-		tsk = rcu_dereference(engine->breadcrumbs.irq_seqno_bh);
-		if (tsk && !wake_up_process(tsk))
+		if (!wake_up_process(wait->tsk))
 			ret |= ENGINE_WAKEUP_ACTIVE;
-		rcu_read_unlock();
 	}
+	spin_unlock_irqrestore(&engine->breadcrumbs.lock, flags);
 
 	return ret;
 }
@@ -301,7 +293,6 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
 	}
 	rb_link_node(&wait->node, parent, p);
 	rb_insert_color(&wait->node, &b->waiters);
-	GEM_BUG_ON(!first && !rcu_access_pointer(b->irq_seqno_bh));
 
 	if (completed) {
 		struct rb_node *next = rb_next(completed);
@@ -310,7 +301,6 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
 		if (next && next != &wait->node) {
 			GEM_BUG_ON(first);
 			b->first_wait = to_wait(next);
-			rcu_assign_pointer(b->irq_seqno_bh, b->first_wait->tsk);
 			/* As there is a delay between reading the current
 			 * seqno, processing the completed tasks and selecting
 			 * the next waiter, we may have missed the interrupt
@@ -337,7 +327,6 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
 	if (first) {
 		GEM_BUG_ON(rb_first(&b->waiters) != &wait->node);
 		b->first_wait = wait;
-		rcu_assign_pointer(b->irq_seqno_bh, wait->tsk);
 		/* After assigning ourselves as the new bottom-half, we must
 		 * perform a cursory check to prevent a missed interrupt.
 		 * Either we miss the interrupt whilst programming the hardware,
@@ -348,7 +337,6 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
 		 */
 		__intel_breadcrumbs_enable_irq(b);
 	}
-	GEM_BUG_ON(!rcu_access_pointer(b->irq_seqno_bh));
 	GEM_BUG_ON(!b->first_wait);
 	GEM_BUG_ON(rb_first(&b->waiters) != &b->first_wait->node);
 
@@ -396,8 +384,6 @@ static void __intel_engine_remove_wait(struct intel_engine_cs *engine,
 		const int priority = wakeup_priority(b, wait->tsk);
 		struct rb_node *next;
 
-		GEM_BUG_ON(rcu_access_pointer(b->irq_seqno_bh) != wait->tsk);
-
 		/* We are the current bottom-half. Find the next candidate,
 		 * the first waiter in the queue on the remaining oldest
 		 * request. As multiple seqnos may complete in the time it
@@ -439,13 +425,11 @@ static void __intel_engine_remove_wait(struct intel_engine_cs *engine,
 			 * exception rather than a seqno completion.
 			 */
 			b->first_wait = to_wait(next);
-			rcu_assign_pointer(b->irq_seqno_bh, b->first_wait->tsk);
 			if (b->first_wait->seqno != wait->seqno)
 				__intel_breadcrumbs_enable_irq(b);
 			wake_up_process(b->first_wait->tsk);
 		} else {
 			b->first_wait = NULL;
-			rcu_assign_pointer(b->irq_seqno_bh, NULL);
 			__intel_breadcrumbs_disable_irq(b);
 		}
 	} else {
@@ -459,7 +443,6 @@ static void __intel_engine_remove_wait(struct intel_engine_cs *engine,
 	GEM_BUG_ON(b->first_wait == wait);
 	GEM_BUG_ON(rb_first(&b->waiters) !=
 		   (b->first_wait ? &b->first_wait->node : NULL));
-	GEM_BUG_ON(!rcu_access_pointer(b->irq_seqno_bh) ^ RB_EMPTY_ROOT(&b->waiters));
 }
 
 void intel_engine_remove_wait(struct intel_engine_cs *engine,
@@ -621,6 +604,7 @@ void intel_engine_enable_signaling(struct drm_i915_gem_request *request)
 		return;
 
 	request->signaling.wait.tsk = b->signaler;
+	request->signaling.wait.request = request;
 	request->signaling.wait.seqno = seqno;
 	i915_gem_request_get(request);
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 7d753dc1b89d..974ec11d2b1d 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -235,8 +235,6 @@ struct intel_engine_cs {
 	 * the overhead of waking that client is much preferred.
 	 */
 	struct intel_breadcrumbs {
-		struct task_struct __rcu *irq_seqno_bh; /* bh for interrupts */
-
 		spinlock_t lock; /* protects the lists of requests; irqsafe */
 		struct rb_root waiters; /* sorted by retirement, priority */
 		struct rb_root signals; /* sorted by retirement */
@@ -582,9 +580,11 @@ static inline u32 intel_hws_seqno_address(struct intel_engine_cs *engine)
 /* intel_breadcrumbs.c -- user interrupt bottom-half for waiters */
 int intel_engine_init_breadcrumbs(struct intel_engine_cs *engine);
 
-static inline void intel_wait_init(struct intel_wait *wait)
+static inline void intel_wait_init(struct intel_wait *wait,
+				   struct drm_i915_gem_request *rq)
 {
 	wait->tsk = current;
+	wait->request = rq;
 }
 
 static inline void intel_wait_init_for_seqno(struct intel_wait *wait, u32 seqno)
@@ -639,7 +639,7 @@ void intel_engine_cancel_signaling(struct drm_i915_gem_request *request);
 
 static inline bool intel_engine_has_waiter(const struct intel_engine_cs *engine)
 {
-	return rcu_access_pointer(engine->breadcrumbs.irq_seqno_bh);
+	return READ_ONCE(engine->breadcrumbs.first_wait);
 }
 
 unsigned int intel_engine_wakeup(struct intel_engine_cs *engine);
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v3 3/5] drm/i915: Defer enabling hangcheck to the first fake breadcrumb interrupt
  2017-02-24 18:01 [PATCH v3 1/5] drm/i915: Report both waiters and success from intel_engine_wakeup() Chris Wilson
  2017-02-24 18:01 ` [PATCH v3 2/5] drm/i915: Signal first fence from irq handler if complete Chris Wilson
@ 2017-02-24 18:01 ` Chris Wilson
  2017-02-27 10:15   ` Tvrtko Ursulin
  2017-02-24 18:01 ` [PATCH v3 4/5] drm/i915: Delay disabling the user interrupt for breadcrumbs Chris Wilson
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 15+ messages in thread
From: Chris Wilson @ 2017-02-24 18:01 UTC (permalink / raw)
  To: intel-gfx

By deferring hangcheck to the fake breadcrumb interrupt, we can simply
the enabling procedure slightly - as by enabling the fake, we then
enable the hangcheck. By always enabling the hangcheck from each fake
interrupt (it will be a no-op for an already queued hangcheck), it will
make restoring the breadcrumbs after a reset simpler in the next patch.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_breadcrumbs.c | 36 ++++++++++++++++----------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index ba5f72fb4890..d7511e89c8ab 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -75,17 +75,6 @@ static void intel_breadcrumbs_hangcheck(unsigned long data)
 	DRM_DEBUG("Hangcheck timer elapsed... %s idle\n", engine->name);
 	set_bit(engine->id, &engine->i915->gpu_error.missed_irq_rings);
 	mod_timer(&engine->breadcrumbs.fake_irq, jiffies + 1);
-
-	/* Ensure that even if the GPU hangs, we get woken up.
-	 *
-	 * However, note that if no one is waiting, we never notice
-	 * a gpu hang. Eventually, we will have to wait for a resource
-	 * held by the GPU and so trigger a hangcheck. In the most
-	 * pathological case, this will be upon memory starvation! To
-	 * prevent this, we also queue the hangcheck from the retire
-	 * worker.
-	 */
-	i915_queue_hangcheck(engine->i915);
 }
 
 static void intel_breadcrumbs_fake_irq(unsigned long data)
@@ -99,8 +88,21 @@ static void intel_breadcrumbs_fake_irq(unsigned long data)
 	 * every jiffie in order to kick the oldest waiter to do the
 	 * coherent seqno check.
 	 */
-	if (intel_engine_wakeup(engine))
-		mod_timer(&engine->breadcrumbs.fake_irq, jiffies + 1);
+	if (!intel_engine_wakeup(engine))
+		return;
+
+	mod_timer(&engine->breadcrumbs.fake_irq, jiffies + 1);
+
+	/* Ensure that even if the GPU hangs, we get woken up.
+	 *
+	 * However, note that if no one is waiting, we never notice
+	 * a gpu hang. Eventually, we will have to wait for a resource
+	 * held by the GPU and so trigger a hangcheck. In the most
+	 * pathological case, this will be upon memory starvation! To
+	 * prevent this, we also queue the hangcheck from the retire
+	 * worker.
+	 */
+	i915_queue_hangcheck(engine->i915);
 }
 
 static void irq_enable(struct intel_engine_cs *engine)
@@ -179,13 +181,11 @@ static void __intel_breadcrumbs_enable_irq(struct intel_breadcrumbs *b)
 		b->irq_enabled = true;
 	}
 
-	if (!b->irq_enabled || use_fake_irq(b)) {
+	/* Ensure we never sleep indefinitely */
+	if (!b->irq_enabled || use_fake_irq(b))
 		mod_timer(&b->fake_irq, jiffies + 1);
-		i915_queue_hangcheck(i915);
-	} else {
-		/* Ensure we never sleep indefinitely */
+	else
 		mod_timer(&b->hangcheck, wait_timeout());
-	}
 }
 
 static void __intel_breadcrumbs_disable_irq(struct intel_breadcrumbs *b)
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v3 4/5] drm/i915: Delay disabling the user interrupt for breadcrumbs
  2017-02-24 18:01 [PATCH v3 1/5] drm/i915: Report both waiters and success from intel_engine_wakeup() Chris Wilson
  2017-02-24 18:01 ` [PATCH v3 2/5] drm/i915: Signal first fence from irq handler if complete Chris Wilson
  2017-02-24 18:01 ` [PATCH v3 3/5] drm/i915: Defer enabling hangcheck to the first fake breadcrumb interrupt Chris Wilson
@ 2017-02-24 18:01 ` Chris Wilson
  2017-02-27 10:31   ` Tvrtko Ursulin
  2017-02-24 18:01 ` [PATCH v3 5/5] drm/i915: Simplify intel_engine_wakeup() Chris Wilson
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 15+ messages in thread
From: Chris Wilson @ 2017-02-24 18:01 UTC (permalink / raw)
  To: intel-gfx

A significant cost in setting up a wait is the overhead of enabling the
interrupt. As we disable the interrupt whenever the queue of waiters is
empty, if we are frequently waiting on alternating batches, we end up
re-enabling the interrupt on a frequent basis. We do want to disable the
interrupt during normal operations as under high load it may add several
thousand interrupts/s - we have been known in the past to occupy whole
cores with our interrupt handler after accidentally leaving user
interrupts enabled. As a compromise, leave the interrupt enabled until
the next IRQ, or the system is idle. This gives a small window for a
waiter to keep the interrupt active and not be delayed by having to
re-enable the interrupt.

v2: Restore hangcheck/missed-irq detection for continuations
v3: Be more careful restoring the hangcheck timer after reset
v4: Be more careful restoring the fake irq after reset (if required!)
v5: Redo changes to intel_engine_wakeup()

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c          |   4 +-
 drivers/gpu/drm/i915/i915_irq.c          |   2 +
 drivers/gpu/drm/i915/intel_breadcrumbs.c | 137 ++++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_ringbuffer.h  |   7 +-
 4 files changed, 97 insertions(+), 53 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 01dbba3813c7..0ad080984877 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2988,8 +2988,10 @@ i915_gem_idle_work_handler(struct work_struct *work)
 	if (wait_for(intel_execlists_idle(dev_priv), 10))
 		DRM_ERROR("Timeout waiting for engines to idle\n");
 
-	for_each_engine(engine, dev_priv, id)
+	for_each_engine(engine, dev_priv, id) {
+		intel_engine_disarm_breadcrumbs(engine);
 		i915_gem_batch_pool_fini(&engine->batch_pool);
+	}
 
 	GEM_BUG_ON(!dev_priv->gt.awake);
 	dev_priv->gt.awake = false;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 7607aba35f26..ba0bb33e12ed 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1058,6 +1058,8 @@ static void notify_ring(struct intel_engine_cs *engine)
 			rq = wait->request;
 
 		wake_up_process(wait->tsk);
+	} else {
+		__intel_engine_disarm_breadcrumbs(engine);
 	}
 	spin_unlock(&engine->breadcrumbs.lock);
 
diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index d7511e89c8ab..8e83be343057 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -36,8 +36,8 @@ unsigned int intel_engine_wakeup(struct intel_engine_cs *engine)
 	wait = engine->breadcrumbs.first_wait;
 	if (wait) {
 		ret = ENGINE_WAKEUP_WAITER;
-		if (!wake_up_process(wait->tsk))
-			ret |= ENGINE_WAKEUP_ACTIVE;
+		if (wake_up_process(wait->tsk))
+			ret |= ENGINE_WAKEUP_ASLEEP;
 	}
 	spin_unlock_irqrestore(&engine->breadcrumbs.lock, flags);
 
@@ -54,7 +54,7 @@ static void intel_breadcrumbs_hangcheck(unsigned long data)
 	struct intel_engine_cs *engine = (struct intel_engine_cs *)data;
 	struct intel_breadcrumbs *b = &engine->breadcrumbs;
 
-	if (!b->irq_enabled)
+	if (!b->irq_armed)
 		return;
 
 	if (b->hangcheck_interrupts != atomic_read(&engine->irq_count)) {
@@ -67,7 +67,7 @@ static void intel_breadcrumbs_hangcheck(unsigned long data)
 	 * to process the pending interrupt (e.g, low priority task on a loaded
 	 * system) and wait until it sleeps before declaring a missed interrupt.
 	 */
-	if (intel_engine_wakeup(engine) & ENGINE_WAKEUP_ACTIVE) {
+	if (!(intel_engine_wakeup(engine) & ENGINE_WAKEUP_ASLEEP)) {
 		mod_timer(&b->hangcheck, wait_timeout());
 		return;
 	}
@@ -80,6 +80,9 @@ static void intel_breadcrumbs_hangcheck(unsigned long data)
 static void intel_breadcrumbs_fake_irq(unsigned long data)
 {
 	struct intel_engine_cs *engine = (struct intel_engine_cs *)data;
+	struct intel_breadcrumbs *b = &engine->breadcrumbs;
+	struct intel_wait *wait;
+	unsigned long flags;
 
 	/*
 	 * The timer persists in case we cannot enable interrupts,
@@ -88,10 +91,18 @@ static void intel_breadcrumbs_fake_irq(unsigned long data)
 	 * every jiffie in order to kick the oldest waiter to do the
 	 * coherent seqno check.
 	 */
-	if (!intel_engine_wakeup(engine))
+
+	spin_lock_irqsave(&b->lock, flags);
+	wait = b->first_wait;
+	if (wait)
+		wake_up_process(wait->tsk);
+	else
+		__intel_engine_disarm_breadcrumbs(engine);
+	spin_unlock_irqrestore(&b->lock, flags);
+	if (!b->irq_armed)
 		return;
 
-	mod_timer(&engine->breadcrumbs.fake_irq, jiffies + 1);
+	mod_timer(&b->fake_irq, jiffies + 1);
 
 	/* Ensure that even if the GPU hangs, we get woken up.
 	 *
@@ -127,6 +138,40 @@ static void irq_disable(struct intel_engine_cs *engine)
 	spin_unlock(&engine->i915->irq_lock);
 }
 
+void __intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
+{
+	struct intel_breadcrumbs *b = &engine->breadcrumbs;
+
+	assert_spin_locked(&b->lock);
+
+	if (b->irq_enabled) {
+		irq_disable(engine);
+		b->irq_enabled = false;
+	}
+
+	b->irq_armed = false;
+}
+
+void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
+{
+	struct intel_breadcrumbs *b = &engine->breadcrumbs;
+	struct intel_wait *wait;
+	unsigned long flags;
+
+	if (!b->irq_armed)
+		return;
+
+	spin_lock_irqsave(&b->lock, flags);
+
+	wait = b->first_wait;
+	if (wait && wake_up_process(wait->tsk))
+		set_bit(engine->id, &engine->i915->gpu_error.missed_irq_rings);
+
+	__intel_engine_disarm_breadcrumbs(engine);
+
+	spin_unlock_irqrestore(&b->lock, flags);
+}
+
 static bool use_fake_irq(const struct intel_breadcrumbs *b)
 {
 	const struct intel_engine_cs *engine =
@@ -144,6 +189,15 @@ static bool use_fake_irq(const struct intel_breadcrumbs *b)
 	return atomic_read(&engine->irq_count) == b->hangcheck_interrupts;
 }
 
+static void enable_fake_irq(struct intel_breadcrumbs *b)
+{
+	/* Ensure we never sleep indefinitely */
+	if (!b->irq_enabled || use_fake_irq(b))
+		mod_timer(&b->fake_irq, jiffies + 1);
+	else
+		mod_timer(&b->hangcheck, wait_timeout());
+}
+
 static void __intel_breadcrumbs_enable_irq(struct intel_breadcrumbs *b)
 {
 	struct intel_engine_cs *engine =
@@ -151,9 +205,17 @@ static void __intel_breadcrumbs_enable_irq(struct intel_breadcrumbs *b)
 	struct drm_i915_private *i915 = engine->i915;
 
 	assert_spin_locked(&b->lock);
-	if (b->rpm_wakelock)
+	if (b->irq_armed)
 		return;
 
+	/* The breadcrumb irq will be disarmed on the interrupt after the
+	 * waiters are signaled. This gives us a single interrupt window in
+	 * which we can add a new waiter and avoid the cost of re-enabling
+	 * the irq.
+	 */
+	b->irq_armed = true;
+	GEM_BUG_ON(b->irq_enabled);
+
 	if (I915_SELFTEST_ONLY(b->mock)) {
 		/* For our mock objects we want to avoid interaction
 		 * with the real hardware (which is not set up). So
@@ -162,17 +224,15 @@ static void __intel_breadcrumbs_enable_irq(struct intel_breadcrumbs *b)
 		 * implementation to call intel_engine_wakeup()
 		 * itself when it wants to simulate a user interrupt,
 		 */
-		b->rpm_wakelock = true;
 		return;
 	}
 
 	/* Since we are waiting on a request, the GPU should be busy
-	 * and should have its own rpm reference. For completeness,
-	 * record an rpm reference for ourselves to cover the
-	 * interrupt we unmask.
+	 * and should have its own rpm reference. This is tracked
+	 * by i915->gt.awake, we can forgo holding our own wakref
+	 * for the interrupt as before i915->gt.awake is released (when
+	 * the driver is idle) we disarm the breadcrumbs.
 	 */
-	intel_runtime_pm_get_noresume(i915);
-	b->rpm_wakelock = true;
 
 	/* No interrupts? Kick the waiter every jiffie! */
 	if (intel_irqs_enabled(i915)) {
@@ -181,34 +241,7 @@ static void __intel_breadcrumbs_enable_irq(struct intel_breadcrumbs *b)
 		b->irq_enabled = true;
 	}
 
-	/* Ensure we never sleep indefinitely */
-	if (!b->irq_enabled || use_fake_irq(b))
-		mod_timer(&b->fake_irq, jiffies + 1);
-	else
-		mod_timer(&b->hangcheck, wait_timeout());
-}
-
-static void __intel_breadcrumbs_disable_irq(struct intel_breadcrumbs *b)
-{
-	struct intel_engine_cs *engine =
-		container_of(b, struct intel_engine_cs, breadcrumbs);
-
-	assert_spin_locked(&b->lock);
-	if (!b->rpm_wakelock)
-		return;
-
-	if (I915_SELFTEST_ONLY(b->mock)) {
-		b->rpm_wakelock = false;
-		return;
-	}
-
-	if (b->irq_enabled) {
-		irq_disable(engine);
-		b->irq_enabled = false;
-	}
-
-	intel_runtime_pm_put(engine->i915);
-	b->rpm_wakelock = false;
+	enable_fake_irq(b);
 }
 
 static inline struct intel_wait *to_wait(struct rb_node *node)
@@ -430,7 +463,6 @@ static void __intel_engine_remove_wait(struct intel_engine_cs *engine,
 			wake_up_process(b->first_wait->tsk);
 		} else {
 			b->first_wait = NULL;
-			__intel_breadcrumbs_disable_irq(b);
 		}
 	} else {
 		GEM_BUG_ON(rb_first(&b->waiters) == &wait->node);
@@ -722,15 +754,20 @@ void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
 	cancel_fake_irq(engine);
 	spin_lock_irq(&b->lock);
 
-	__intel_breadcrumbs_disable_irq(b);
-	if (intel_engine_has_waiter(engine)) {
-		__intel_breadcrumbs_enable_irq(b);
-		if (test_bit(ENGINE_IRQ_BREADCRUMB, &engine->irq_posted))
-			wake_up_process(b->first_wait->tsk);
-	} else {
-		/* sanitize the IMR and unmask any auxiliary interrupts */
+	if (b->irq_enabled)
+		irq_enable(engine);
+	else
 		irq_disable(engine);
-	}
+
+	/* The engine is currently idle (we haven't started it yet), there
+	 * is no possibility for a missed interrupt as we enabled the irq
+	 * and so we can clear the immediate wakeup (until a real interrupt
+	 * arrives for the waiter).
+	 */
+	clear_bit(ENGINE_IRQ_BREADCRUMB, &engine->irq_posted);
+
+	if (b->irq_armed)
+		enable_fake_irq(b);
 
 	spin_unlock_irq(&b->lock);
 }
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 974ec11d2b1d..3dd6eee4a08b 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -246,8 +246,8 @@ struct intel_engine_cs {
 
 		unsigned int hangcheck_interrupts;
 
+		bool irq_armed : 1;
 		bool irq_enabled : 1;
-		bool rpm_wakelock : 1;
 		I915_SELFTEST_DECLARE(bool mock : 1);
 	} breadcrumbs;
 
@@ -644,7 +644,10 @@ static inline bool intel_engine_has_waiter(const struct intel_engine_cs *engine)
 
 unsigned int intel_engine_wakeup(struct intel_engine_cs *engine);
 #define ENGINE_WAKEUP_WAITER BIT(0)
-#define ENGINE_WAKEUP_ACTIVE BIT(1)
+#define ENGINE_WAKEUP_ASLEEP BIT(1)
+
+void __intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine);
+void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine);
 
 void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine);
 void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine);
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v3 5/5] drm/i915: Simplify intel_engine_wakeup()
  2017-02-24 18:01 [PATCH v3 1/5] drm/i915: Report both waiters and success from intel_engine_wakeup() Chris Wilson
                   ` (2 preceding siblings ...)
  2017-02-24 18:01 ` [PATCH v3 4/5] drm/i915: Delay disabling the user interrupt for breadcrumbs Chris Wilson
@ 2017-02-24 18:01 ` Chris Wilson
  2017-02-27 10:34   ` Tvrtko Ursulin
  2017-02-24 18:52 ` ✗ Fi.CI.BAT: warning for series starting with [v3,1/5] drm/i915: Report both waiters and success from intel_engine_wakeup() Patchwork
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 15+ messages in thread
From: Chris Wilson @ 2017-02-24 18:01 UTC (permalink / raw)
  To: intel-gfx

Now the only use of the return value is ask whether or not we actually
woke a process up. With a single condition to report, we can go back to
using a boolean.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_breadcrumbs.c | 13 +++++--------
 drivers/gpu/drm/i915/intel_ringbuffer.h  |  4 +---
 2 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index 8e83be343057..84034ee827af 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -26,19 +26,16 @@
 
 #include "i915_drv.h"
 
-unsigned int intel_engine_wakeup(struct intel_engine_cs *engine)
+bool intel_engine_wakeup(struct intel_engine_cs *engine)
 {
 	struct intel_wait *wait;
 	unsigned long flags;
-	unsigned int ret = 0;
+	bool ret = false;
 
 	spin_lock_irqsave(&engine->breadcrumbs.lock, flags);
 	wait = engine->breadcrumbs.first_wait;
-	if (wait) {
-		ret = ENGINE_WAKEUP_WAITER;
-		if (wake_up_process(wait->tsk))
-			ret |= ENGINE_WAKEUP_ASLEEP;
-	}
+	if (wait && wake_up_process(wait->tsk))
+		ret = true;
 	spin_unlock_irqrestore(&engine->breadcrumbs.lock, flags);
 
 	return ret;
@@ -67,7 +64,7 @@ static void intel_breadcrumbs_hangcheck(unsigned long data)
 	 * to process the pending interrupt (e.g, low priority task on a loaded
 	 * system) and wait until it sleeps before declaring a missed interrupt.
 	 */
-	if (!(intel_engine_wakeup(engine) & ENGINE_WAKEUP_ASLEEP)) {
+	if (!intel_engine_wakeup(engine)) {
 		mod_timer(&b->hangcheck, wait_timeout());
 		return;
 	}
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 3dd6eee4a08b..007628231ec3 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -642,9 +642,7 @@ static inline bool intel_engine_has_waiter(const struct intel_engine_cs *engine)
 	return READ_ONCE(engine->breadcrumbs.first_wait);
 }
 
-unsigned int intel_engine_wakeup(struct intel_engine_cs *engine);
-#define ENGINE_WAKEUP_WAITER BIT(0)
-#define ENGINE_WAKEUP_ASLEEP BIT(1)
+bool intel_engine_wakeup(struct intel_engine_cs *engine);
 
 void __intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine);
 void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine);
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* ✗ Fi.CI.BAT: warning for series starting with [v3,1/5] drm/i915: Report both waiters and success from intel_engine_wakeup()
  2017-02-24 18:01 [PATCH v3 1/5] drm/i915: Report both waiters and success from intel_engine_wakeup() Chris Wilson
                   ` (3 preceding siblings ...)
  2017-02-24 18:01 ` [PATCH v3 5/5] drm/i915: Simplify intel_engine_wakeup() Chris Wilson
@ 2017-02-24 18:52 ` Patchwork
  2017-02-25 10:52 ` ✓ Fi.CI.BAT: success for series starting with [v3,1/5] drm/i915: Report both waiters and success from intel_engine_wakeup() (rev2) Patchwork
  2017-02-27 10:03 ` [PATCH v3 1/5] drm/i915: Report both waiters and success from intel_engine_wakeup() Tvrtko Ursulin
  6 siblings, 0 replies; 15+ messages in thread
From: Patchwork @ 2017-02-24 18:52 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: series starting with [v3,1/5] drm/i915: Report both waiters and success from intel_engine_wakeup()
URL   : https://patchwork.freedesktop.org/series/20218/
State : warning

== Summary ==

Series 20218v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/20218/revisions/1/mbox/

Test gem_exec_parallel:
        Subgroup basic:
                pass       -> DMESG-WARN (fi-skl-6700hq)

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11 
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19 
fi-bxt-t5700     total:108  pass:95   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16 
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16 
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50 
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18 
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18 
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18 
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10 
fi-skl-6700hq    total:278  pass:260  dwarn:1   dfail:0   fail:0   skip:17 
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18 
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10 
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28 
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29 

9243ada504db810aa40b0e8f5e00d46871c78149 drm-tip: 2017y-02m-24d-17h-52m-18s UTC integration manifest
b751707 drm/i915: Simplify intel_engine_wakeup()
7b5ea88 drm/i915: Delay disabling the user interrupt for breadcrumbs
e6b5d79 drm/i915: Defer enabling hangcheck to the first fake breadcrumb interrupt
3f3e9ad drm/i915: Signal first fence from irq handler if complete
459ae66 drm/i915: Report both waiters and success from intel_engine_wakeup()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3963/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v4] drm/i915: Signal first fence from irq handler if complete
  2017-02-24 18:01 ` [PATCH v3 2/5] drm/i915: Signal first fence from irq handler if complete Chris Wilson
@ 2017-02-25 10:05   ` Chris Wilson
  2017-02-27 10:04     ` Tvrtko Ursulin
  0 siblings, 1 reply; 15+ messages in thread
From: Chris Wilson @ 2017-02-25 10:05 UTC (permalink / raw)
  To: intel-gfx

As execlists and other non-semaphore multi-engine devices coordinate
between engines using interrupts, we can shave off a few 10s of
microsecond of scheduling latency by doing the fence signaling from the
interrupt as opposed to a RT kthread. (Realistically the delay adds
about 1% to an individual cross-engine workload.) We only signal the
first fence in order to limit the amount of work we move into the
interrupt handler. We also have to remember that our breadcrumbs may be
unordered with respect to the interrupt and so we still require the
waiter process to perform some heavyweight coherency fixups, as well as
traversing the tree of waiters.

v2: No need for early exit in irq handler - it breaks the flow between
patches and prevents the tracepoint
v3: Restore rcu hold across irq signaling of request

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h          | 13 ++++++------
 drivers/gpu/drm/i915/i915_gem_request.c  |  2 +-
 drivers/gpu/drm/i915/i915_gem_request.h  |  2 ++
 drivers/gpu/drm/i915/i915_irq.c          | 36 +++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_breadcrumbs.c | 32 +++++++---------------------
 drivers/gpu/drm/i915/intel_ringbuffer.h  |  8 +++----
 6 files changed, 54 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 56caab35f216..16c5e7a37eba 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -4063,9 +4063,9 @@ __i915_request_irq_complete(const struct drm_i915_gem_request *req)
 	 * is woken.
 	 */
 	if (engine->irq_seqno_barrier &&
-	    rcu_access_pointer(engine->breadcrumbs.irq_seqno_bh) == current &&
 	    test_and_clear_bit(ENGINE_IRQ_BREADCRUMB, &engine->irq_posted)) {
-		struct task_struct *tsk;
+		struct intel_breadcrumbs *b = &engine->breadcrumbs;
+		unsigned long flags;
 
 		/* The ordering of irq_posted versus applying the barrier
 		 * is crucial. The clearing of the current irq_posted must
@@ -4087,17 +4087,16 @@ __i915_request_irq_complete(const struct drm_i915_gem_request *req)
 		 * the seqno before we believe it coherent since they see
 		 * irq_posted == false but we are still running).
 		 */
-		rcu_read_lock();
-		tsk = rcu_dereference(engine->breadcrumbs.irq_seqno_bh);
-		if (tsk && tsk != current)
+		spin_lock_irqsave(&b->lock, flags);
+		if (b->first_wait && b->first_wait->tsk != current)
 			/* Note that if the bottom-half is changed as we
 			 * are sending the wake-up, the new bottom-half will
 			 * be woken by whomever made the change. We only have
 			 * to worry about when we steal the irq-posted for
 			 * ourself.
 			 */
-			wake_up_process(tsk);
-		rcu_read_unlock();
+			wake_up_process(b->first_wait->tsk);
+		spin_unlock_irqrestore(&b->lock, flags);
 
 		if (__i915_gem_request_completed(req, seqno))
 			return true;
diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
index fbfeb5f8d069..77c3ee7a3fd0 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/i915_gem_request.c
@@ -1083,7 +1083,7 @@ long i915_wait_request(struct drm_i915_gem_request *req,
 	if (flags & I915_WAIT_LOCKED)
 		add_wait_queue(errq, &reset);
 
-	intel_wait_init(&wait);
+	intel_wait_init(&wait, req);
 
 restart:
 	do {
diff --git a/drivers/gpu/drm/i915/i915_gem_request.h b/drivers/gpu/drm/i915/i915_gem_request.h
index 5f73d8c0a38a..0efee879df23 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.h
+++ b/drivers/gpu/drm/i915/i915_gem_request.h
@@ -32,10 +32,12 @@
 
 struct drm_file;
 struct drm_i915_gem_object;
+struct drm_i915_gem_request;
 
 struct intel_wait {
 	struct rb_node node;
 	struct task_struct *tsk;
+	struct drm_i915_gem_request *request;
 	u32 seqno;
 };
 
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index bc70e2c451b2..bc520ee8d6fe 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1033,12 +1033,42 @@ static void ironlake_rps_change_irq_handler(struct drm_i915_private *dev_priv)
 
 static void notify_ring(struct intel_engine_cs *engine)
 {
-	bool waiters;
+	struct drm_i915_gem_request *rq = NULL;
+	struct intel_wait *wait;
 
 	atomic_inc(&engine->irq_count);
 	set_bit(ENGINE_IRQ_BREADCRUMB, &engine->irq_posted);
-	waiters = intel_engine_wakeup(engine);
-	trace_intel_engine_notify(engine, waiters);
+
+	rcu_read_lock();
+
+	spin_lock(&engine->breadcrumbs.lock);
+	wait = engine->breadcrumbs.first_wait;
+	if (wait) {
+		/* We use a callback from the dma-fence to submit
+		 * requests after waiting on our own requests. To
+		 * ensure minimum delay in queuing the next request to
+		 * hardware, signal the fence now rather than wait for
+		 * the signaler to be woken up. We still wake up the
+		 * waiter in order to handle the irq-seqno coherency
+		 * issues (we may receive the interrupt before the
+		 * seqno is written, see __i915_request_irq_complete())
+		 * and to handle coalescing of multiple seqno updates
+		 * and many waiters.
+		 */
+		if (i915_seqno_passed(intel_engine_get_seqno(engine),
+				      wait->seqno))
+			rq = wait->request;
+
+		wake_up_process(wait->tsk);
+	}
+	spin_unlock(&engine->breadcrumbs.lock);
+
+	if (rq)
+		dma_fence_signal(&rq->fence);
+
+	rcu_read_unlock();
+
+	trace_intel_engine_notify(engine, wait);
 }
 
 static void vlv_c0_read(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index 64e1b0c2d8b6..ba5f72fb4890 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -28,26 +28,18 @@
 
 unsigned int intel_engine_wakeup(struct intel_engine_cs *engine)
 {
+	struct intel_wait *wait;
+	unsigned long flags;
 	unsigned int ret = 0;
 
-	/* Note that for this not to dangerously chase a dangling pointer,
-	 * we must hold the rcu_read_lock here.
-	 *
-	 * Also note that tsk is likely to be in !TASK_RUNNING state so an
-	 * early test for tsk->state != TASK_RUNNING before wake_up_process()
-	 * is unlikely to be beneficial.
-	 */
-	if (intel_engine_has_waiter(engine)) {
-		struct task_struct *tsk;
-
+	spin_lock_irqsave(&engine->breadcrumbs.lock, flags);
+	wait = engine->breadcrumbs.first_wait;
+	if (wait) {
 		ret = ENGINE_WAKEUP_WAITER;
-
-		rcu_read_lock();
-		tsk = rcu_dereference(engine->breadcrumbs.irq_seqno_bh);
-		if (tsk && !wake_up_process(tsk))
+		if (!wake_up_process(wait->tsk))
 			ret |= ENGINE_WAKEUP_ACTIVE;
-		rcu_read_unlock();
 	}
+	spin_unlock_irqrestore(&engine->breadcrumbs.lock, flags);
 
 	return ret;
 }
@@ -301,7 +293,6 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
 	}
 	rb_link_node(&wait->node, parent, p);
 	rb_insert_color(&wait->node, &b->waiters);
-	GEM_BUG_ON(!first && !rcu_access_pointer(b->irq_seqno_bh));
 
 	if (completed) {
 		struct rb_node *next = rb_next(completed);
@@ -310,7 +301,6 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
 		if (next && next != &wait->node) {
 			GEM_BUG_ON(first);
 			b->first_wait = to_wait(next);
-			rcu_assign_pointer(b->irq_seqno_bh, b->first_wait->tsk);
 			/* As there is a delay between reading the current
 			 * seqno, processing the completed tasks and selecting
 			 * the next waiter, we may have missed the interrupt
@@ -337,7 +327,6 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
 	if (first) {
 		GEM_BUG_ON(rb_first(&b->waiters) != &wait->node);
 		b->first_wait = wait;
-		rcu_assign_pointer(b->irq_seqno_bh, wait->tsk);
 		/* After assigning ourselves as the new bottom-half, we must
 		 * perform a cursory check to prevent a missed interrupt.
 		 * Either we miss the interrupt whilst programming the hardware,
@@ -348,7 +337,6 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
 		 */
 		__intel_breadcrumbs_enable_irq(b);
 	}
-	GEM_BUG_ON(!rcu_access_pointer(b->irq_seqno_bh));
 	GEM_BUG_ON(!b->first_wait);
 	GEM_BUG_ON(rb_first(&b->waiters) != &b->first_wait->node);
 
@@ -396,8 +384,6 @@ static void __intel_engine_remove_wait(struct intel_engine_cs *engine,
 		const int priority = wakeup_priority(b, wait->tsk);
 		struct rb_node *next;
 
-		GEM_BUG_ON(rcu_access_pointer(b->irq_seqno_bh) != wait->tsk);
-
 		/* We are the current bottom-half. Find the next candidate,
 		 * the first waiter in the queue on the remaining oldest
 		 * request. As multiple seqnos may complete in the time it
@@ -439,13 +425,11 @@ static void __intel_engine_remove_wait(struct intel_engine_cs *engine,
 			 * exception rather than a seqno completion.
 			 */
 			b->first_wait = to_wait(next);
-			rcu_assign_pointer(b->irq_seqno_bh, b->first_wait->tsk);
 			if (b->first_wait->seqno != wait->seqno)
 				__intel_breadcrumbs_enable_irq(b);
 			wake_up_process(b->first_wait->tsk);
 		} else {
 			b->first_wait = NULL;
-			rcu_assign_pointer(b->irq_seqno_bh, NULL);
 			__intel_breadcrumbs_disable_irq(b);
 		}
 	} else {
@@ -459,7 +443,6 @@ static void __intel_engine_remove_wait(struct intel_engine_cs *engine,
 	GEM_BUG_ON(b->first_wait == wait);
 	GEM_BUG_ON(rb_first(&b->waiters) !=
 		   (b->first_wait ? &b->first_wait->node : NULL));
-	GEM_BUG_ON(!rcu_access_pointer(b->irq_seqno_bh) ^ RB_EMPTY_ROOT(&b->waiters));
 }
 
 void intel_engine_remove_wait(struct intel_engine_cs *engine,
@@ -621,6 +604,7 @@ void intel_engine_enable_signaling(struct drm_i915_gem_request *request)
 		return;
 
 	request->signaling.wait.tsk = b->signaler;
+	request->signaling.wait.request = request;
 	request->signaling.wait.seqno = seqno;
 	i915_gem_request_get(request);
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 7d753dc1b89d..974ec11d2b1d 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -235,8 +235,6 @@ struct intel_engine_cs {
 	 * the overhead of waking that client is much preferred.
 	 */
 	struct intel_breadcrumbs {
-		struct task_struct __rcu *irq_seqno_bh; /* bh for interrupts */
-
 		spinlock_t lock; /* protects the lists of requests; irqsafe */
 		struct rb_root waiters; /* sorted by retirement, priority */
 		struct rb_root signals; /* sorted by retirement */
@@ -582,9 +580,11 @@ static inline u32 intel_hws_seqno_address(struct intel_engine_cs *engine)
 /* intel_breadcrumbs.c -- user interrupt bottom-half for waiters */
 int intel_engine_init_breadcrumbs(struct intel_engine_cs *engine);
 
-static inline void intel_wait_init(struct intel_wait *wait)
+static inline void intel_wait_init(struct intel_wait *wait,
+				   struct drm_i915_gem_request *rq)
 {
 	wait->tsk = current;
+	wait->request = rq;
 }
 
 static inline void intel_wait_init_for_seqno(struct intel_wait *wait, u32 seqno)
@@ -639,7 +639,7 @@ void intel_engine_cancel_signaling(struct drm_i915_gem_request *request);
 
 static inline bool intel_engine_has_waiter(const struct intel_engine_cs *engine)
 {
-	return rcu_access_pointer(engine->breadcrumbs.irq_seqno_bh);
+	return READ_ONCE(engine->breadcrumbs.first_wait);
 }
 
 unsigned int intel_engine_wakeup(struct intel_engine_cs *engine);
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* ✓ Fi.CI.BAT: success for series starting with [v3,1/5] drm/i915: Report both waiters and success from intel_engine_wakeup() (rev2)
  2017-02-24 18:01 [PATCH v3 1/5] drm/i915: Report both waiters and success from intel_engine_wakeup() Chris Wilson
                   ` (4 preceding siblings ...)
  2017-02-24 18:52 ` ✗ Fi.CI.BAT: warning for series starting with [v3,1/5] drm/i915: Report both waiters and success from intel_engine_wakeup() Patchwork
@ 2017-02-25 10:52 ` Patchwork
  2017-02-27 10:03 ` [PATCH v3 1/5] drm/i915: Report both waiters and success from intel_engine_wakeup() Tvrtko Ursulin
  6 siblings, 0 replies; 15+ messages in thread
From: Patchwork @ 2017-02-25 10:52 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: series starting with [v3,1/5] drm/i915: Report both waiters and success from intel_engine_wakeup() (rev2)
URL   : https://patchwork.freedesktop.org/series/20218/
State : success

== Summary ==

Series 20218v2 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/20218/revisions/2/mbox/

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11 
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19 
fi-bxt-t5700     total:108  pass:95   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16 
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16 
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50 
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18 
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18 
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18 
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10 
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17 
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18 
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10 
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28 
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29 

9243ada504db810aa40b0e8f5e00d46871c78149 drm-tip: 2017y-02m-24d-17h-52m-18s UTC integration manifest
eddbd13 drm/i915: Simplify intel_engine_wakeup()
d1d6a4c drm/i915: Delay disabling the user interrupt for breadcrumbs
d8091d0 drm/i915: Defer enabling hangcheck to the first fake breadcrumb interrupt
5c85935 drm/i915: Signal first fence from irq handler if complete
6091bcd drm/i915: Report both waiters and success from intel_engine_wakeup()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3973/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 1/5] drm/i915: Report both waiters and success from intel_engine_wakeup()
  2017-02-24 18:01 [PATCH v3 1/5] drm/i915: Report both waiters and success from intel_engine_wakeup() Chris Wilson
                   ` (5 preceding siblings ...)
  2017-02-25 10:52 ` ✓ Fi.CI.BAT: success for series starting with [v3,1/5] drm/i915: Report both waiters and success from intel_engine_wakeup() (rev2) Patchwork
@ 2017-02-27 10:03 ` Tvrtko Ursulin
  2017-02-27 10:35   ` Chris Wilson
  6 siblings, 1 reply; 15+ messages in thread
From: Tvrtko Ursulin @ 2017-02-27 10:03 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx


On 24/02/2017 18:01, Chris Wilson wrote:
> The two users of the return value from intel_engine_wakeup() are
> expecting different results. In the breadcrumbs hangcheck, we are using
> it to determine whether wake_up_process() detected the waiter was
> currently running (and if so we presume that it hasn't yet missed the
> interrupt). However, in the fake_irq path, we are using the return value
> as a check as to whether there are any waiters, and so we may
> incorrectly stop the fake-irq if that waiter was currently running.
>
> To handle the two different needs, return both bits of information!
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_breadcrumbs.c | 28 +++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_ringbuffer.h  | 26 +++-----------------------
>  2 files changed, 30 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index 027c93e34c97..64e1b0c2d8b6 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -26,6 +26,32 @@
>
>  #include "i915_drv.h"
>
> +unsigned int intel_engine_wakeup(struct intel_engine_cs *engine)

How come you decided to un-inline it? Changes shouldn't have made a big 
difference to its size I would have thought.

> +{
> +	unsigned int ret = 0;

Optional bikeshed - maybe call it res or something, since ret is usually 
used for < 0 || 0.

> +
> +	/* Note that for this not to dangerously chase a dangling pointer,
> +	 * we must hold the rcu_read_lock here.
> +	 *
> +	 * Also note that tsk is likely to be in !TASK_RUNNING state so an
> +	 * early test for tsk->state != TASK_RUNNING before wake_up_process()
> +	 * is unlikely to be beneficial.
> +	 */
> +	if (intel_engine_has_waiter(engine)) {
> +		struct task_struct *tsk;
> +
> +		ret = ENGINE_WAKEUP_WAITER;
> +
> +		rcu_read_lock();
> +		tsk = rcu_dereference(engine->breadcrumbs.irq_seqno_bh);
> +		if (tsk && !wake_up_process(tsk))
> +			ret |= ENGINE_WAKEUP_ACTIVE;
> +		rcu_read_unlock();
> +	}
> +
> +	return ret;
> +}
> +
>  static unsigned long wait_timeout(void)
>  {
>  	return round_jiffies_up(jiffies + DRM_I915_HANGCHECK_JIFFIES);
> @@ -49,7 +75,7 @@ static void intel_breadcrumbs_hangcheck(unsigned long data)
>  	 * to process the pending interrupt (e.g, low priority task on a loaded
>  	 * system) and wait until it sleeps before declaring a missed interrupt.
>  	 */
> -	if (!intel_engine_wakeup(engine)) {
> +	if (intel_engine_wakeup(engine) & ENGINE_WAKEUP_ACTIVE) {
>  		mod_timer(&b->hangcheck, wait_timeout());
>  		return;
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 0f29e07a9581..7d753dc1b89d 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -642,29 +642,9 @@ static inline bool intel_engine_has_waiter(const struct intel_engine_cs *engine)
>  	return rcu_access_pointer(engine->breadcrumbs.irq_seqno_bh);
>  }
>
> -static inline bool intel_engine_wakeup(const struct intel_engine_cs *engine)
> -{
> -	bool wakeup = false;
> -
> -	/* Note that for this not to dangerously chase a dangling pointer,
> -	 * we must hold the rcu_read_lock here.
> -	 *
> -	 * Also note that tsk is likely to be in !TASK_RUNNING state so an
> -	 * early test for tsk->state != TASK_RUNNING before wake_up_process()
> -	 * is unlikely to be beneficial.
> -	 */
> -	if (intel_engine_has_waiter(engine)) {
> -		struct task_struct *tsk;
> -
> -		rcu_read_lock();
> -		tsk = rcu_dereference(engine->breadcrumbs.irq_seqno_bh);
> -		if (tsk)
> -			wakeup = wake_up_process(tsk);
> -		rcu_read_unlock();
> -	}
> -
> -	return wakeup;
> -}
> +unsigned int intel_engine_wakeup(struct intel_engine_cs *engine);
> +#define ENGINE_WAKEUP_WAITER BIT(0)
> +#define ENGINE_WAKEUP_ACTIVE BIT(1)
>
>  void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine);
>  void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine);
>

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4] drm/i915: Signal first fence from irq handler if complete
  2017-02-25 10:05   ` [PATCH v4] " Chris Wilson
@ 2017-02-27 10:04     ` Tvrtko Ursulin
  0 siblings, 0 replies; 15+ messages in thread
From: Tvrtko Ursulin @ 2017-02-27 10:04 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx


On 25/02/2017 10:05, Chris Wilson wrote:
> As execlists and other non-semaphore multi-engine devices coordinate
> between engines using interrupts, we can shave off a few 10s of
> microsecond of scheduling latency by doing the fence signaling from the
> interrupt as opposed to a RT kthread. (Realistically the delay adds
> about 1% to an individual cross-engine workload.) We only signal the
> first fence in order to limit the amount of work we move into the
> interrupt handler. We also have to remember that our breadcrumbs may be
> unordered with respect to the interrupt and so we still require the
> waiter process to perform some heavyweight coherency fixups, as well as
> traversing the tree of waiters.
>
> v2: No need for early exit in irq handler - it breaks the flow between
> patches and prevents the tracepoint
> v3: Restore rcu hold across irq signaling of request
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h          | 13 ++++++------
>  drivers/gpu/drm/i915/i915_gem_request.c  |  2 +-
>  drivers/gpu/drm/i915/i915_gem_request.h  |  2 ++
>  drivers/gpu/drm/i915/i915_irq.c          | 36 +++++++++++++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_breadcrumbs.c | 32 +++++++---------------------
>  drivers/gpu/drm/i915/intel_ringbuffer.h  |  8 +++----
>  6 files changed, 54 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 56caab35f216..16c5e7a37eba 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -4063,9 +4063,9 @@ __i915_request_irq_complete(const struct drm_i915_gem_request *req)
>  	 * is woken.
>  	 */
>  	if (engine->irq_seqno_barrier &&
> -	    rcu_access_pointer(engine->breadcrumbs.irq_seqno_bh) == current &&
>  	    test_and_clear_bit(ENGINE_IRQ_BREADCRUMB, &engine->irq_posted)) {
> -		struct task_struct *tsk;
> +		struct intel_breadcrumbs *b = &engine->breadcrumbs;
> +		unsigned long flags;
>
>  		/* The ordering of irq_posted versus applying the barrier
>  		 * is crucial. The clearing of the current irq_posted must
> @@ -4087,17 +4087,16 @@ __i915_request_irq_complete(const struct drm_i915_gem_request *req)
>  		 * the seqno before we believe it coherent since they see
>  		 * irq_posted == false but we are still running).
>  		 */
> -		rcu_read_lock();
> -		tsk = rcu_dereference(engine->breadcrumbs.irq_seqno_bh);
> -		if (tsk && tsk != current)
> +		spin_lock_irqsave(&b->lock, flags);
> +		if (b->first_wait && b->first_wait->tsk != current)
>  			/* Note that if the bottom-half is changed as we
>  			 * are sending the wake-up, the new bottom-half will
>  			 * be woken by whomever made the change. We only have
>  			 * to worry about when we steal the irq-posted for
>  			 * ourself.
>  			 */
> -			wake_up_process(tsk);
> -		rcu_read_unlock();
> +			wake_up_process(b->first_wait->tsk);
> +		spin_unlock_irqrestore(&b->lock, flags);
>
>  		if (__i915_gem_request_completed(req, seqno))
>  			return true;
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
> index fbfeb5f8d069..77c3ee7a3fd0 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.c
> +++ b/drivers/gpu/drm/i915/i915_gem_request.c
> @@ -1083,7 +1083,7 @@ long i915_wait_request(struct drm_i915_gem_request *req,
>  	if (flags & I915_WAIT_LOCKED)
>  		add_wait_queue(errq, &reset);
>
> -	intel_wait_init(&wait);
> +	intel_wait_init(&wait, req);
>
>  restart:
>  	do {
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.h b/drivers/gpu/drm/i915/i915_gem_request.h
> index 5f73d8c0a38a..0efee879df23 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.h
> +++ b/drivers/gpu/drm/i915/i915_gem_request.h
> @@ -32,10 +32,12 @@
>
>  struct drm_file;
>  struct drm_i915_gem_object;
> +struct drm_i915_gem_request;
>
>  struct intel_wait {
>  	struct rb_node node;
>  	struct task_struct *tsk;
> +	struct drm_i915_gem_request *request;
>  	u32 seqno;
>  };
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index bc70e2c451b2..bc520ee8d6fe 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1033,12 +1033,42 @@ static void ironlake_rps_change_irq_handler(struct drm_i915_private *dev_priv)
>
>  static void notify_ring(struct intel_engine_cs *engine)
>  {
> -	bool waiters;
> +	struct drm_i915_gem_request *rq = NULL;
> +	struct intel_wait *wait;
>
>  	atomic_inc(&engine->irq_count);
>  	set_bit(ENGINE_IRQ_BREADCRUMB, &engine->irq_posted);
> -	waiters = intel_engine_wakeup(engine);
> -	trace_intel_engine_notify(engine, waiters);
> +
> +	rcu_read_lock();
> +
> +	spin_lock(&engine->breadcrumbs.lock);
> +	wait = engine->breadcrumbs.first_wait;
> +	if (wait) {
> +		/* We use a callback from the dma-fence to submit
> +		 * requests after waiting on our own requests. To
> +		 * ensure minimum delay in queuing the next request to
> +		 * hardware, signal the fence now rather than wait for
> +		 * the signaler to be woken up. We still wake up the
> +		 * waiter in order to handle the irq-seqno coherency
> +		 * issues (we may receive the interrupt before the
> +		 * seqno is written, see __i915_request_irq_complete())
> +		 * and to handle coalescing of multiple seqno updates
> +		 * and many waiters.
> +		 */
> +		if (i915_seqno_passed(intel_engine_get_seqno(engine),
> +				      wait->seqno))
> +			rq = wait->request;
> +
> +		wake_up_process(wait->tsk);
> +	}
> +	spin_unlock(&engine->breadcrumbs.lock);
> +
> +	if (rq)
> +		dma_fence_signal(&rq->fence);
> +
> +	rcu_read_unlock();
> +
> +	trace_intel_engine_notify(engine, wait);
>  }
>
>  static void vlv_c0_read(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index 64e1b0c2d8b6..ba5f72fb4890 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -28,26 +28,18 @@
>
>  unsigned int intel_engine_wakeup(struct intel_engine_cs *engine)
>  {
> +	struct intel_wait *wait;
> +	unsigned long flags;
>  	unsigned int ret = 0;
>
> -	/* Note that for this not to dangerously chase a dangling pointer,
> -	 * we must hold the rcu_read_lock here.
> -	 *
> -	 * Also note that tsk is likely to be in !TASK_RUNNING state so an
> -	 * early test for tsk->state != TASK_RUNNING before wake_up_process()
> -	 * is unlikely to be beneficial.
> -	 */
> -	if (intel_engine_has_waiter(engine)) {
> -		struct task_struct *tsk;
> -
> +	spin_lock_irqsave(&engine->breadcrumbs.lock, flags);
> +	wait = engine->breadcrumbs.first_wait;
> +	if (wait) {
>  		ret = ENGINE_WAKEUP_WAITER;
> -
> -		rcu_read_lock();
> -		tsk = rcu_dereference(engine->breadcrumbs.irq_seqno_bh);
> -		if (tsk && !wake_up_process(tsk))
> +		if (!wake_up_process(wait->tsk))
>  			ret |= ENGINE_WAKEUP_ACTIVE;
> -		rcu_read_unlock();
>  	}
> +	spin_unlock_irqrestore(&engine->breadcrumbs.lock, flags);
>
>  	return ret;
>  }
> @@ -301,7 +293,6 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
>  	}
>  	rb_link_node(&wait->node, parent, p);
>  	rb_insert_color(&wait->node, &b->waiters);
> -	GEM_BUG_ON(!first && !rcu_access_pointer(b->irq_seqno_bh));
>
>  	if (completed) {
>  		struct rb_node *next = rb_next(completed);
> @@ -310,7 +301,6 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
>  		if (next && next != &wait->node) {
>  			GEM_BUG_ON(first);
>  			b->first_wait = to_wait(next);
> -			rcu_assign_pointer(b->irq_seqno_bh, b->first_wait->tsk);
>  			/* As there is a delay between reading the current
>  			 * seqno, processing the completed tasks and selecting
>  			 * the next waiter, we may have missed the interrupt
> @@ -337,7 +327,6 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
>  	if (first) {
>  		GEM_BUG_ON(rb_first(&b->waiters) != &wait->node);
>  		b->first_wait = wait;
> -		rcu_assign_pointer(b->irq_seqno_bh, wait->tsk);
>  		/* After assigning ourselves as the new bottom-half, we must
>  		 * perform a cursory check to prevent a missed interrupt.
>  		 * Either we miss the interrupt whilst programming the hardware,
> @@ -348,7 +337,6 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
>  		 */
>  		__intel_breadcrumbs_enable_irq(b);
>  	}
> -	GEM_BUG_ON(!rcu_access_pointer(b->irq_seqno_bh));
>  	GEM_BUG_ON(!b->first_wait);
>  	GEM_BUG_ON(rb_first(&b->waiters) != &b->first_wait->node);
>
> @@ -396,8 +384,6 @@ static void __intel_engine_remove_wait(struct intel_engine_cs *engine,
>  		const int priority = wakeup_priority(b, wait->tsk);
>  		struct rb_node *next;
>
> -		GEM_BUG_ON(rcu_access_pointer(b->irq_seqno_bh) != wait->tsk);
> -
>  		/* We are the current bottom-half. Find the next candidate,
>  		 * the first waiter in the queue on the remaining oldest
>  		 * request. As multiple seqnos may complete in the time it
> @@ -439,13 +425,11 @@ static void __intel_engine_remove_wait(struct intel_engine_cs *engine,
>  			 * exception rather than a seqno completion.
>  			 */
>  			b->first_wait = to_wait(next);
> -			rcu_assign_pointer(b->irq_seqno_bh, b->first_wait->tsk);
>  			if (b->first_wait->seqno != wait->seqno)
>  				__intel_breadcrumbs_enable_irq(b);
>  			wake_up_process(b->first_wait->tsk);
>  		} else {
>  			b->first_wait = NULL;
> -			rcu_assign_pointer(b->irq_seqno_bh, NULL);
>  			__intel_breadcrumbs_disable_irq(b);
>  		}
>  	} else {
> @@ -459,7 +443,6 @@ static void __intel_engine_remove_wait(struct intel_engine_cs *engine,
>  	GEM_BUG_ON(b->first_wait == wait);
>  	GEM_BUG_ON(rb_first(&b->waiters) !=
>  		   (b->first_wait ? &b->first_wait->node : NULL));
> -	GEM_BUG_ON(!rcu_access_pointer(b->irq_seqno_bh) ^ RB_EMPTY_ROOT(&b->waiters));
>  }
>
>  void intel_engine_remove_wait(struct intel_engine_cs *engine,
> @@ -621,6 +604,7 @@ void intel_engine_enable_signaling(struct drm_i915_gem_request *request)
>  		return;
>
>  	request->signaling.wait.tsk = b->signaler;
> +	request->signaling.wait.request = request;
>  	request->signaling.wait.seqno = seqno;
>  	i915_gem_request_get(request);
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 7d753dc1b89d..974ec11d2b1d 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -235,8 +235,6 @@ struct intel_engine_cs {
>  	 * the overhead of waking that client is much preferred.
>  	 */
>  	struct intel_breadcrumbs {
> -		struct task_struct __rcu *irq_seqno_bh; /* bh for interrupts */
> -
>  		spinlock_t lock; /* protects the lists of requests; irqsafe */
>  		struct rb_root waiters; /* sorted by retirement, priority */
>  		struct rb_root signals; /* sorted by retirement */
> @@ -582,9 +580,11 @@ static inline u32 intel_hws_seqno_address(struct intel_engine_cs *engine)
>  /* intel_breadcrumbs.c -- user interrupt bottom-half for waiters */
>  int intel_engine_init_breadcrumbs(struct intel_engine_cs *engine);
>
> -static inline void intel_wait_init(struct intel_wait *wait)
> +static inline void intel_wait_init(struct intel_wait *wait,
> +				   struct drm_i915_gem_request *rq)
>  {
>  	wait->tsk = current;
> +	wait->request = rq;
>  }
>
>  static inline void intel_wait_init_for_seqno(struct intel_wait *wait, u32 seqno)
> @@ -639,7 +639,7 @@ void intel_engine_cancel_signaling(struct drm_i915_gem_request *request);
>
>  static inline bool intel_engine_has_waiter(const struct intel_engine_cs *engine)
>  {
> -	return rcu_access_pointer(engine->breadcrumbs.irq_seqno_bh);
> +	return READ_ONCE(engine->breadcrumbs.first_wait);
>  }
>
>  unsigned int intel_engine_wakeup(struct intel_engine_cs *engine);
>

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 3/5] drm/i915: Defer enabling hangcheck to the first fake breadcrumb interrupt
  2017-02-24 18:01 ` [PATCH v3 3/5] drm/i915: Defer enabling hangcheck to the first fake breadcrumb interrupt Chris Wilson
@ 2017-02-27 10:15   ` Tvrtko Ursulin
  0 siblings, 0 replies; 15+ messages in thread
From: Tvrtko Ursulin @ 2017-02-27 10:15 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx


On 24/02/2017 18:01, Chris Wilson wrote:
> By deferring hangcheck to the fake breadcrumb interrupt, we can simply
> the enabling procedure slightly - as by enabling the fake, we then
> enable the hangcheck. By always enabling the hangcheck from each fake
> interrupt (it will be a no-op for an already queued hangcheck), it will
> make restoring the breadcrumbs after a reset simpler in the next patch.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_breadcrumbs.c | 36 ++++++++++++++++----------------
>  1 file changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index ba5f72fb4890..d7511e89c8ab 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -75,17 +75,6 @@ static void intel_breadcrumbs_hangcheck(unsigned long data)
>  	DRM_DEBUG("Hangcheck timer elapsed... %s idle\n", engine->name);
>  	set_bit(engine->id, &engine->i915->gpu_error.missed_irq_rings);
>  	mod_timer(&engine->breadcrumbs.fake_irq, jiffies + 1);
> -
> -	/* Ensure that even if the GPU hangs, we get woken up.
> -	 *
> -	 * However, note that if no one is waiting, we never notice
> -	 * a gpu hang. Eventually, we will have to wait for a resource
> -	 * held by the GPU and so trigger a hangcheck. In the most
> -	 * pathological case, this will be upon memory starvation! To
> -	 * prevent this, we also queue the hangcheck from the retire
> -	 * worker.
> -	 */
> -	i915_queue_hangcheck(engine->i915);
>  }
>
>  static void intel_breadcrumbs_fake_irq(unsigned long data)
> @@ -99,8 +88,21 @@ static void intel_breadcrumbs_fake_irq(unsigned long data)
>  	 * every jiffie in order to kick the oldest waiter to do the
>  	 * coherent seqno check.
>  	 */
> -	if (intel_engine_wakeup(engine))
> -		mod_timer(&engine->breadcrumbs.fake_irq, jiffies + 1);
> +	if (!intel_engine_wakeup(engine))
> +		return;
> +
> +	mod_timer(&engine->breadcrumbs.fake_irq, jiffies + 1);
> +
> +	/* Ensure that even if the GPU hangs, we get woken up.
> +	 *
> +	 * However, note that if no one is waiting, we never notice
> +	 * a gpu hang. Eventually, we will have to wait for a resource
> +	 * held by the GPU and so trigger a hangcheck. In the most
> +	 * pathological case, this will be upon memory starvation! To
> +	 * prevent this, we also queue the hangcheck from the retire
> +	 * worker.
> +	 */
> +	i915_queue_hangcheck(engine->i915);
>  }
>
>  static void irq_enable(struct intel_engine_cs *engine)
> @@ -179,13 +181,11 @@ static void __intel_breadcrumbs_enable_irq(struct intel_breadcrumbs *b)
>  		b->irq_enabled = true;
>  	}
>
> -	if (!b->irq_enabled || use_fake_irq(b)) {
> +	/* Ensure we never sleep indefinitely */
> +	if (!b->irq_enabled || use_fake_irq(b))
>  		mod_timer(&b->fake_irq, jiffies + 1);
> -		i915_queue_hangcheck(i915);
> -	} else {
> -		/* Ensure we never sleep indefinitely */
> +	else
>  		mod_timer(&b->hangcheck, wait_timeout());
> -	}
>  }
>
>  static void __intel_breadcrumbs_disable_irq(struct intel_breadcrumbs *b)
>

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 4/5] drm/i915: Delay disabling the user interrupt for breadcrumbs
  2017-02-24 18:01 ` [PATCH v3 4/5] drm/i915: Delay disabling the user interrupt for breadcrumbs Chris Wilson
@ 2017-02-27 10:31   ` Tvrtko Ursulin
  2017-02-27 11:02     ` Chris Wilson
  0 siblings, 1 reply; 15+ messages in thread
From: Tvrtko Ursulin @ 2017-02-27 10:31 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx


On 24/02/2017 18:01, Chris Wilson wrote:
> A significant cost in setting up a wait is the overhead of enabling the
> interrupt. As we disable the interrupt whenever the queue of waiters is
> empty, if we are frequently waiting on alternating batches, we end up
> re-enabling the interrupt on a frequent basis. We do want to disable the
> interrupt during normal operations as under high load it may add several
> thousand interrupts/s - we have been known in the past to occupy whole
> cores with our interrupt handler after accidentally leaving user
> interrupts enabled. As a compromise, leave the interrupt enabled until
> the next IRQ, or the system is idle. This gives a small window for a
> waiter to keep the interrupt active and not be delayed by having to
> re-enable the interrupt.
>
> v2: Restore hangcheck/missed-irq detection for continuations
> v3: Be more careful restoring the hangcheck timer after reset
> v4: Be more careful restoring the fake irq after reset (if required!)
> v5: Redo changes to intel_engine_wakeup()
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c          |   4 +-
>  drivers/gpu/drm/i915/i915_irq.c          |   2 +
>  drivers/gpu/drm/i915/intel_breadcrumbs.c | 137 ++++++++++++++++++++-----------
>  drivers/gpu/drm/i915/intel_ringbuffer.h  |   7 +-
>  4 files changed, 97 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 01dbba3813c7..0ad080984877 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2988,8 +2988,10 @@ i915_gem_idle_work_handler(struct work_struct *work)
>  	if (wait_for(intel_execlists_idle(dev_priv), 10))
>  		DRM_ERROR("Timeout waiting for engines to idle\n");
>
> -	for_each_engine(engine, dev_priv, id)
> +	for_each_engine(engine, dev_priv, id) {
> +		intel_engine_disarm_breadcrumbs(engine);
>  		i915_gem_batch_pool_fini(&engine->batch_pool);
> +	}
>
>  	GEM_BUG_ON(!dev_priv->gt.awake);
>  	dev_priv->gt.awake = false;
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 7607aba35f26..ba0bb33e12ed 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1058,6 +1058,8 @@ static void notify_ring(struct intel_engine_cs *engine)
>  			rq = wait->request;
>
>  		wake_up_process(wait->tsk);
> +	} else {
> +		__intel_engine_disarm_breadcrumbs(engine);
>  	}
>  	spin_unlock(&engine->breadcrumbs.lock);
>
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index d7511e89c8ab..8e83be343057 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -36,8 +36,8 @@ unsigned int intel_engine_wakeup(struct intel_engine_cs *engine)
>  	wait = engine->breadcrumbs.first_wait;
>  	if (wait) {
>  		ret = ENGINE_WAKEUP_WAITER;
> -		if (!wake_up_process(wait->tsk))
> -			ret |= ENGINE_WAKEUP_ACTIVE;
> +		if (wake_up_process(wait->tsk))
> +			ret |= ENGINE_WAKEUP_ASLEEP;

Why did you want to reverse the bit and all?

>  	}
>  	spin_unlock_irqrestore(&engine->breadcrumbs.lock, flags);
>
> @@ -54,7 +54,7 @@ static void intel_breadcrumbs_hangcheck(unsigned long data)
>  	struct intel_engine_cs *engine = (struct intel_engine_cs *)data;
>  	struct intel_breadcrumbs *b = &engine->breadcrumbs;
>
> -	if (!b->irq_enabled)
> +	if (!b->irq_armed)
>  		return;
>
>  	if (b->hangcheck_interrupts != atomic_read(&engine->irq_count)) {
> @@ -67,7 +67,7 @@ static void intel_breadcrumbs_hangcheck(unsigned long data)
>  	 * to process the pending interrupt (e.g, low priority task on a loaded
>  	 * system) and wait until it sleeps before declaring a missed interrupt.
>  	 */
> -	if (intel_engine_wakeup(engine) & ENGINE_WAKEUP_ACTIVE) {
> +	if (!(intel_engine_wakeup(engine) & ENGINE_WAKEUP_ASLEEP)) {
>  		mod_timer(&b->hangcheck, wait_timeout());
>  		return;
>  	}
> @@ -80,6 +80,9 @@ static void intel_breadcrumbs_hangcheck(unsigned long data)
>  static void intel_breadcrumbs_fake_irq(unsigned long data)
>  {
>  	struct intel_engine_cs *engine = (struct intel_engine_cs *)data;
> +	struct intel_breadcrumbs *b = &engine->breadcrumbs;
> +	struct intel_wait *wait;
> +	unsigned long flags;
>
>  	/*
>  	 * The timer persists in case we cannot enable interrupts,
> @@ -88,10 +91,18 @@ static void intel_breadcrumbs_fake_irq(unsigned long data)
>  	 * every jiffie in order to kick the oldest waiter to do the
>  	 * coherent seqno check.
>  	 */
> -	if (!intel_engine_wakeup(engine))
> +
> +	spin_lock_irqsave(&b->lock, flags);
> +	wait = b->first_wait;
> +	if (wait)
> +		wake_up_process(wait->tsk);
> +	else
> +		__intel_engine_disarm_breadcrumbs(engine);
> +	spin_unlock_irqrestore(&b->lock, flags);

Would it be worth restructuring the intel_engine_wakeup in the top level 
funtion which takes the spinlock and calls 
__intel_engine_wakeup(_locked) which does the rest, which could then be 
used here?

> +	if (!b->irq_armed)
>  		return;
>
> -	mod_timer(&engine->breadcrumbs.fake_irq, jiffies + 1);
> +	mod_timer(&b->fake_irq, jiffies + 1);
>
>  	/* Ensure that even if the GPU hangs, we get woken up.
>  	 *
> @@ -127,6 +138,40 @@ static void irq_disable(struct intel_engine_cs *engine)
>  	spin_unlock(&engine->i915->irq_lock);
>  }
>
> +void __intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
> +{
> +	struct intel_breadcrumbs *b = &engine->breadcrumbs;
> +
> +	assert_spin_locked(&b->lock);
> +
> +	if (b->irq_enabled) {
> +		irq_disable(engine);
> +		b->irq_enabled = false;
> +	}
> +
> +	b->irq_armed = false;
> +}
> +
> +void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
> +{
> +	struct intel_breadcrumbs *b = &engine->breadcrumbs;
> +	struct intel_wait *wait;
> +	unsigned long flags;
> +
> +	if (!b->irq_armed)
> +		return;
> +
> +	spin_lock_irqsave(&b->lock, flags);
> +
> +	wait = b->first_wait;
> +	if (wait && wake_up_process(wait->tsk))
> +		set_bit(engine->id, &engine->i915->gpu_error.missed_irq_rings);
> +
> +	__intel_engine_disarm_breadcrumbs(engine);
> +
> +	spin_unlock_irqrestore(&b->lock, flags);

__intel_engine_wakeup could be used here as well.

> +}
> +
>  static bool use_fake_irq(const struct intel_breadcrumbs *b)
>  {
>  	const struct intel_engine_cs *engine =
> @@ -144,6 +189,15 @@ static bool use_fake_irq(const struct intel_breadcrumbs *b)
>  	return atomic_read(&engine->irq_count) == b->hangcheck_interrupts;
>  }
>
> +static void enable_fake_irq(struct intel_breadcrumbs *b)
> +{
> +	/* Ensure we never sleep indefinitely */
> +	if (!b->irq_enabled || use_fake_irq(b))
> +		mod_timer(&b->fake_irq, jiffies + 1);
> +	else
> +		mod_timer(&b->hangcheck, wait_timeout());
> +}
> +
>  static void __intel_breadcrumbs_enable_irq(struct intel_breadcrumbs *b)
>  {
>  	struct intel_engine_cs *engine =
> @@ -151,9 +205,17 @@ static void __intel_breadcrumbs_enable_irq(struct intel_breadcrumbs *b)
>  	struct drm_i915_private *i915 = engine->i915;
>
>  	assert_spin_locked(&b->lock);
> -	if (b->rpm_wakelock)
> +	if (b->irq_armed)
>  		return;
>
> +	/* The breadcrumb irq will be disarmed on the interrupt after the
> +	 * waiters are signaled. This gives us a single interrupt window in
> +	 * which we can add a new waiter and avoid the cost of re-enabling
> +	 * the irq.
> +	 */
> +	b->irq_armed = true;
> +	GEM_BUG_ON(b->irq_enabled);
> +
>  	if (I915_SELFTEST_ONLY(b->mock)) {
>  		/* For our mock objects we want to avoid interaction
>  		 * with the real hardware (which is not set up). So
> @@ -162,17 +224,15 @@ static void __intel_breadcrumbs_enable_irq(struct intel_breadcrumbs *b)
>  		 * implementation to call intel_engine_wakeup()
>  		 * itself when it wants to simulate a user interrupt,
>  		 */
> -		b->rpm_wakelock = true;
>  		return;
>  	}
>
>  	/* Since we are waiting on a request, the GPU should be busy
> -	 * and should have its own rpm reference. For completeness,
> -	 * record an rpm reference for ourselves to cover the
> -	 * interrupt we unmask.
> +	 * and should have its own rpm reference. This is tracked
> +	 * by i915->gt.awake, we can forgo holding our own wakref
> +	 * for the interrupt as before i915->gt.awake is released (when
> +	 * the driver is idle) we disarm the breadcrumbs.
>  	 */
> -	intel_runtime_pm_get_noresume(i915);
> -	b->rpm_wakelock = true;
>
>  	/* No interrupts? Kick the waiter every jiffie! */
>  	if (intel_irqs_enabled(i915)) {
> @@ -181,34 +241,7 @@ static void __intel_breadcrumbs_enable_irq(struct intel_breadcrumbs *b)
>  		b->irq_enabled = true;
>  	}
>
> -	/* Ensure we never sleep indefinitely */
> -	if (!b->irq_enabled || use_fake_irq(b))
> -		mod_timer(&b->fake_irq, jiffies + 1);
> -	else
> -		mod_timer(&b->hangcheck, wait_timeout());
> -}
> -
> -static void __intel_breadcrumbs_disable_irq(struct intel_breadcrumbs *b)
> -{
> -	struct intel_engine_cs *engine =
> -		container_of(b, struct intel_engine_cs, breadcrumbs);
> -
> -	assert_spin_locked(&b->lock);
> -	if (!b->rpm_wakelock)
> -		return;
> -
> -	if (I915_SELFTEST_ONLY(b->mock)) {
> -		b->rpm_wakelock = false;
> -		return;
> -	}
> -
> -	if (b->irq_enabled) {
> -		irq_disable(engine);
> -		b->irq_enabled = false;
> -	}
> -
> -	intel_runtime_pm_put(engine->i915);
> -	b->rpm_wakelock = false;
> +	enable_fake_irq(b);
>  }
>
>  static inline struct intel_wait *to_wait(struct rb_node *node)
> @@ -430,7 +463,6 @@ static void __intel_engine_remove_wait(struct intel_engine_cs *engine,
>  			wake_up_process(b->first_wait->tsk);
>  		} else {
>  			b->first_wait = NULL;
> -			__intel_breadcrumbs_disable_irq(b);
>  		}
>  	} else {
>  		GEM_BUG_ON(rb_first(&b->waiters) == &wait->node);
> @@ -722,15 +754,20 @@ void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
>  	cancel_fake_irq(engine);
>  	spin_lock_irq(&b->lock);
>
> -	__intel_breadcrumbs_disable_irq(b);
> -	if (intel_engine_has_waiter(engine)) {
> -		__intel_breadcrumbs_enable_irq(b);
> -		if (test_bit(ENGINE_IRQ_BREADCRUMB, &engine->irq_posted))
> -			wake_up_process(b->first_wait->tsk);
> -	} else {
> -		/* sanitize the IMR and unmask any auxiliary interrupts */
> +	if (b->irq_enabled)
> +		irq_enable(engine);
> +	else
>  		irq_disable(engine);
> -	}
> +
> +	/* The engine is currently idle (we haven't started it yet), there
> +	 * is no possibility for a missed interrupt as we enabled the irq
> +	 * and so we can clear the immediate wakeup (until a real interrupt
> +	 * arrives for the waiter).
> +	 */
> +	clear_bit(ENGINE_IRQ_BREADCRUMB, &engine->irq_posted);

Who set this bit coming here after a reset? Sounds like it would be 
worth adding to the comment.

> +
> +	if (b->irq_armed)
> +		enable_fake_irq(b);
>
>  	spin_unlock_irq(&b->lock);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 974ec11d2b1d..3dd6eee4a08b 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -246,8 +246,8 @@ struct intel_engine_cs {
>
>  		unsigned int hangcheck_interrupts;
>
> +		bool irq_armed : 1;
>  		bool irq_enabled : 1;
> -		bool rpm_wakelock : 1;
>  		I915_SELFTEST_DECLARE(bool mock : 1);
>  	} breadcrumbs;
>
> @@ -644,7 +644,10 @@ static inline bool intel_engine_has_waiter(const struct intel_engine_cs *engine)
>
>  unsigned int intel_engine_wakeup(struct intel_engine_cs *engine);
>  #define ENGINE_WAKEUP_WAITER BIT(0)
> -#define ENGINE_WAKEUP_ACTIVE BIT(1)
> +#define ENGINE_WAKEUP_ASLEEP BIT(1)
> +
> +void __intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine);
> +void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine);
>
>  void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine);
>  void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine);
>

Looks okay, just not sure why you couldn't have left 
ENGINE_WAKEUP_ACTIVE for a smaller diff. And if you would agree or not 
the consolidation of intel_engine_wakup open coded instances makes sense.

Regards,

Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 5/5] drm/i915: Simplify intel_engine_wakeup()
  2017-02-24 18:01 ` [PATCH v3 5/5] drm/i915: Simplify intel_engine_wakeup() Chris Wilson
@ 2017-02-27 10:34   ` Tvrtko Ursulin
  0 siblings, 0 replies; 15+ messages in thread
From: Tvrtko Ursulin @ 2017-02-27 10:34 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx


On 24/02/2017 18:01, Chris Wilson wrote:
> Now the only use of the return value is ask whether or not we actually
> woke a process up. With a single condition to report, we can go back to
> using a boolean.

Argh! :) I guess it depends on whether you keep or not the open coded 
intel_engine_wakeup's in the previous patch.

Regards,

Tvrtko

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_breadcrumbs.c | 13 +++++--------
>  drivers/gpu/drm/i915/intel_ringbuffer.h  |  4 +---
>  2 files changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index 8e83be343057..84034ee827af 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -26,19 +26,16 @@
>
>  #include "i915_drv.h"
>
> -unsigned int intel_engine_wakeup(struct intel_engine_cs *engine)
> +bool intel_engine_wakeup(struct intel_engine_cs *engine)
>  {
>  	struct intel_wait *wait;
>  	unsigned long flags;
> -	unsigned int ret = 0;
> +	bool ret = false;
>
>  	spin_lock_irqsave(&engine->breadcrumbs.lock, flags);
>  	wait = engine->breadcrumbs.first_wait;
> -	if (wait) {
> -		ret = ENGINE_WAKEUP_WAITER;
> -		if (wake_up_process(wait->tsk))
> -			ret |= ENGINE_WAKEUP_ASLEEP;
> -	}
> +	if (wait && wake_up_process(wait->tsk))
> +		ret = true;
>  	spin_unlock_irqrestore(&engine->breadcrumbs.lock, flags);
>
>  	return ret;
> @@ -67,7 +64,7 @@ static void intel_breadcrumbs_hangcheck(unsigned long data)
>  	 * to process the pending interrupt (e.g, low priority task on a loaded
>  	 * system) and wait until it sleeps before declaring a missed interrupt.
>  	 */
> -	if (!(intel_engine_wakeup(engine) & ENGINE_WAKEUP_ASLEEP)) {
> +	if (!intel_engine_wakeup(engine)) {
>  		mod_timer(&b->hangcheck, wait_timeout());
>  		return;
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 3dd6eee4a08b..007628231ec3 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -642,9 +642,7 @@ static inline bool intel_engine_has_waiter(const struct intel_engine_cs *engine)
>  	return READ_ONCE(engine->breadcrumbs.first_wait);
>  }
>
> -unsigned int intel_engine_wakeup(struct intel_engine_cs *engine);
> -#define ENGINE_WAKEUP_WAITER BIT(0)
> -#define ENGINE_WAKEUP_ASLEEP BIT(1)
> +bool intel_engine_wakeup(struct intel_engine_cs *engine);
>
>  void __intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine);
>  void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine);
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 1/5] drm/i915: Report both waiters and success from intel_engine_wakeup()
  2017-02-27 10:03 ` [PATCH v3 1/5] drm/i915: Report both waiters and success from intel_engine_wakeup() Tvrtko Ursulin
@ 2017-02-27 10:35   ` Chris Wilson
  0 siblings, 0 replies; 15+ messages in thread
From: Chris Wilson @ 2017-02-27 10:35 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: intel-gfx

On Mon, Feb 27, 2017 at 10:03:32AM +0000, Tvrtko Ursulin wrote:
> 
> On 24/02/2017 18:01, Chris Wilson wrote:
> >The two users of the return value from intel_engine_wakeup() are
> >expecting different results. In the breadcrumbs hangcheck, we are using
> >it to determine whether wake_up_process() detected the waiter was
> >currently running (and if so we presume that it hasn't yet missed the
> >interrupt). However, in the fake_irq path, we are using the return value
> >as a check as to whether there are any waiters, and so we may
> >incorrectly stop the fake-irq if that waiter was currently running.
> >
> >To handle the two different needs, return both bits of information!
> >
> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> >---
> > drivers/gpu/drm/i915/intel_breadcrumbs.c | 28 +++++++++++++++++++++++++++-
> > drivers/gpu/drm/i915/intel_ringbuffer.h  | 26 +++-----------------------
> > 2 files changed, 30 insertions(+), 24 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >index 027c93e34c97..64e1b0c2d8b6 100644
> >--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >@@ -26,6 +26,32 @@
> >
> > #include "i915_drv.h"
> >
> >+unsigned int intel_engine_wakeup(struct intel_engine_cs *engine)
> 
> How come you decided to un-inline it? Changes shouldn't have made a
> big difference to its size I would have thought.

Mainly in the later patches we move this away from any important paths.

> >+{
> >+	unsigned int ret = 0;
> 
> Optional bikeshed - maybe call it res or something, since ret is
> usually used for < 0 || 0.

Been adopting int err for those, but result is better than ret.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 4/5] drm/i915: Delay disabling the user interrupt for breadcrumbs
  2017-02-27 10:31   ` Tvrtko Ursulin
@ 2017-02-27 11:02     ` Chris Wilson
  0 siblings, 0 replies; 15+ messages in thread
From: Chris Wilson @ 2017-02-27 11:02 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: intel-gfx

On Mon, Feb 27, 2017 at 10:31:36AM +0000, Tvrtko Ursulin wrote:
> 
> On 24/02/2017 18:01, Chris Wilson wrote:
> >A significant cost in setting up a wait is the overhead of enabling the
> >interrupt. As we disable the interrupt whenever the queue of waiters is
> >empty, if we are frequently waiting on alternating batches, we end up
> >re-enabling the interrupt on a frequent basis. We do want to disable the
> >interrupt during normal operations as under high load it may add several
> >thousand interrupts/s - we have been known in the past to occupy whole
> >cores with our interrupt handler after accidentally leaving user
> >interrupts enabled. As a compromise, leave the interrupt enabled until
> >the next IRQ, or the system is idle. This gives a small window for a
> >waiter to keep the interrupt active and not be delayed by having to
> >re-enable the interrupt.
> >
> >v2: Restore hangcheck/missed-irq detection for continuations
> >v3: Be more careful restoring the hangcheck timer after reset
> >v4: Be more careful restoring the fake irq after reset (if required!)
> >v5: Redo changes to intel_engine_wakeup()
> >
> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> >---
> > drivers/gpu/drm/i915/i915_gem.c          |   4 +-
> > drivers/gpu/drm/i915/i915_irq.c          |   2 +
> > drivers/gpu/drm/i915/intel_breadcrumbs.c | 137 ++++++++++++++++++++-----------
> > drivers/gpu/drm/i915/intel_ringbuffer.h  |   7 +-
> > 4 files changed, 97 insertions(+), 53 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >index 01dbba3813c7..0ad080984877 100644
> >--- a/drivers/gpu/drm/i915/i915_gem.c
> >+++ b/drivers/gpu/drm/i915/i915_gem.c
> >@@ -2988,8 +2988,10 @@ i915_gem_idle_work_handler(struct work_struct *work)
> > 	if (wait_for(intel_execlists_idle(dev_priv), 10))
> > 		DRM_ERROR("Timeout waiting for engines to idle\n");
> >
> >-	for_each_engine(engine, dev_priv, id)
> >+	for_each_engine(engine, dev_priv, id) {
> >+		intel_engine_disarm_breadcrumbs(engine);
> > 		i915_gem_batch_pool_fini(&engine->batch_pool);
> >+	}
> >
> > 	GEM_BUG_ON(!dev_priv->gt.awake);
> > 	dev_priv->gt.awake = false;
> >diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> >index 7607aba35f26..ba0bb33e12ed 100644
> >--- a/drivers/gpu/drm/i915/i915_irq.c
> >+++ b/drivers/gpu/drm/i915/i915_irq.c
> >@@ -1058,6 +1058,8 @@ static void notify_ring(struct intel_engine_cs *engine)
> > 			rq = wait->request;
> >
> > 		wake_up_process(wait->tsk);
> >+	} else {
> >+		__intel_engine_disarm_breadcrumbs(engine);
> > 	}
> > 	spin_unlock(&engine->breadcrumbs.lock);
> >
> >diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >index d7511e89c8ab..8e83be343057 100644
> >--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >@@ -36,8 +36,8 @@ unsigned int intel_engine_wakeup(struct intel_engine_cs *engine)
> > 	wait = engine->breadcrumbs.first_wait;
> > 	if (wait) {
> > 		ret = ENGINE_WAKEUP_WAITER;
> >-		if (!wake_up_process(wait->tsk))
> >-			ret |= ENGINE_WAKEUP_ACTIVE;
> >+		if (wake_up_process(wait->tsk))
> >+			ret |= ENGINE_WAKEUP_ASLEEP;
> 
> Why did you want to reverse the bit and all?

Because we need to keep the hangcheck alive whilst there is no waiter -
only when we declare the GT as idle do we disarm.

> > 	}
> > 	spin_unlock_irqrestore(&engine->breadcrumbs.lock, flags);
> >
> >@@ -54,7 +54,7 @@ static void intel_breadcrumbs_hangcheck(unsigned long data)
> > 	struct intel_engine_cs *engine = (struct intel_engine_cs *)data;
> > 	struct intel_breadcrumbs *b = &engine->breadcrumbs;
> >
> >-	if (!b->irq_enabled)
> >+	if (!b->irq_armed)
> > 		return;
> >
> > 	if (b->hangcheck_interrupts != atomic_read(&engine->irq_count)) {
> >@@ -67,7 +67,7 @@ static void intel_breadcrumbs_hangcheck(unsigned long data)
> > 	 * to process the pending interrupt (e.g, low priority task on a loaded
> > 	 * system) and wait until it sleeps before declaring a missed interrupt.
> > 	 */
> >-	if (intel_engine_wakeup(engine) & ENGINE_WAKEUP_ACTIVE) {
> >+	if (!(intel_engine_wakeup(engine) & ENGINE_WAKEUP_ASLEEP)) {
> > 		mod_timer(&b->hangcheck, wait_timeout());
> > 		return;
> > 	}

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2017-02-27 11:02 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-02-24 18:01 [PATCH v3 1/5] drm/i915: Report both waiters and success from intel_engine_wakeup() Chris Wilson
2017-02-24 18:01 ` [PATCH v3 2/5] drm/i915: Signal first fence from irq handler if complete Chris Wilson
2017-02-25 10:05   ` [PATCH v4] " Chris Wilson
2017-02-27 10:04     ` Tvrtko Ursulin
2017-02-24 18:01 ` [PATCH v3 3/5] drm/i915: Defer enabling hangcheck to the first fake breadcrumb interrupt Chris Wilson
2017-02-27 10:15   ` Tvrtko Ursulin
2017-02-24 18:01 ` [PATCH v3 4/5] drm/i915: Delay disabling the user interrupt for breadcrumbs Chris Wilson
2017-02-27 10:31   ` Tvrtko Ursulin
2017-02-27 11:02     ` Chris Wilson
2017-02-24 18:01 ` [PATCH v3 5/5] drm/i915: Simplify intel_engine_wakeup() Chris Wilson
2017-02-27 10:34   ` Tvrtko Ursulin
2017-02-24 18:52 ` ✗ Fi.CI.BAT: warning for series starting with [v3,1/5] drm/i915: Report both waiters and success from intel_engine_wakeup() Patchwork
2017-02-25 10:52 ` ✓ Fi.CI.BAT: success for series starting with [v3,1/5] drm/i915: Report both waiters and success from intel_engine_wakeup() (rev2) Patchwork
2017-02-27 10:03 ` [PATCH v3 1/5] drm/i915: Report both waiters and success from intel_engine_wakeup() Tvrtko Ursulin
2017-02-27 10:35   ` Chris Wilson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox