Re: [PATCH 21/25] drm/i915: Convert trace-irq to the breadcrumb waiter

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 25/06/16 11:13, Chris Wilson wrote:
If we convert the tracing over from direct use of ring->irq_get() and
over to the breadcrumb infrastructure, we only have a single user of the
ring->irq_get and so we will be able to simplify the driver routines
(eliminating the redundant validation and irq refcounting).

Process context is preferred over softirq (or even hardirq) for a couple
of reasons:

  - we already utilize process context to have fast wakeup of a single
    client (i.e. the client waiting for the GPU inspects the seqno for
    itself following an interrupt to avoid the overhead of a context
    switch before it returns to userspace)

  - engine->irq_seqno() is not suitable for use from an softirq/hardirq
    context as we may require long waits (100-250us) to ensure the seqno
    write is posted before we read it from the CPU

A signaling framework is a requirement for enabling dma-fences.

v2: Move to a signaling framework based upon the waiter.
v3: Track the first-signal to avoid having to walk the rbtree everytime.
v4: Mark the signaler thread as RT priority to reduce latency in the
indirect wakeups.
v5: Make failure to allocate the thread fatal.

Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
---
  drivers/gpu/drm/i915/i915_drv.h          |   8 --
  drivers/gpu/drm/i915/i915_gem.c          |   9 +-
  drivers/gpu/drm/i915/i915_trace.h        |   2 +-
  drivers/gpu/drm/i915/intel_breadcrumbs.c | 192 ++++++++++++++++++++++++++++++-
  drivers/gpu/drm/i915/intel_ringbuffer.h  |   8 +-
  5 files changed, 201 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d09b96d193a5..b7089cedb80c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3965,14 +3965,6 @@ wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
  			    schedule_timeout_uninterruptible(remaining_jiffies);
  	}
  }
-
-static inline void i915_trace_irq_get(struct intel_engine_cs *engine,
-				      struct drm_i915_gem_request *req)
-{
-	if (engine->trace_irq_req == NULL && engine->irq_get(engine))
-		i915_gem_request_assign(&engine->trace_irq_req, req);
-}
-
  static inline bool __i915_request_irq_complete(struct drm_i915_gem_request *req)
  {
  	struct intel_engine_cs *engine = req->engine;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index fd59b30a024d..e6daa1ee4d2d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2745,7 +2745,8 @@ i915_gem_init_seqno(struct drm_i915_private *dev_priv, u32 seqno)

  	/* If the seqno wraps around, we need to clear the breadcrumb rbtree */
  	if (!i915_seqno_passed(seqno, dev_priv->next_seqno)) {
-		while (intel_kick_waiters(dev_priv))
+		while (intel_kick_waiters(dev_priv) ||
+		       intel_kick_signalers(dev_priv))
  			yield();
  	}

@@ -3210,12 +3211,6 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *engine)
  		i915_gem_object_retire__read(obj, engine->id);
  	}

-	if (unlikely(engine->trace_irq_req &&
-		     i915_gem_request_completed(engine->trace_irq_req))) {
-		engine->irq_put(engine);
-		i915_gem_request_assign(&engine->trace_irq_req, NULL);
-	}
-
  	WARN_ON(i915_verify_lists(engine->dev));
  }

diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
index 3d13fde95fdf..f59cf07184ae 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -490,7 +490,7 @@ TRACE_EVENT(i915_gem_ring_dispatch,
  			   __entry->ring = req->engine->id;
  			   __entry->seqno = req->seqno;
  			   __entry->flags = flags;
-			   i915_trace_irq_get(req->engine, req);
+			   intel_engine_enable_signaling(req);
  			   ),

  	    TP_printk("dev=%u, ring=%u, seqno=%u, flags=%x",
diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index 84c2a449dd0e..bc0e9e8b7871 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -22,6 +22,8 @@
   *
   */

+#include <linux/kthread.h>
+
  #include "i915_drv.h"

  static void intel_breadcrumbs_fake_irq(unsigned long data)
@@ -255,6 +257,15 @@ static inline bool chain_wakeup(struct rb_node *rb, int priority)
  	return rb && to_wait(rb)->tsk->prio <= priority;
  }

+static inline int wakeup_priority(struct intel_breadcrumbs *b,
+				  struct task_struct *tsk)
+{
+	if (tsk == b->signaler)
+		return INT_MIN;
+	else
+		return tsk->prio;
+}
+
  void intel_engine_remove_wait(struct intel_engine_cs *engine,
  			      struct intel_wait *wait)
  {
@@ -273,8 +284,8 @@ void intel_engine_remove_wait(struct intel_engine_cs *engine,
  		goto out_unlock;

  	if (b->first_wait == wait) {
+		const int priority = wakeup_priority(b, wait->tsk);
  		struct rb_node *next;
-		const int priority = wait->tsk->prio;

  		GEM_BUG_ON(b->tasklet != wait->tsk);

@@ -340,15 +351,176 @@ out_unlock:
  	spin_unlock(&b->lock);
  }

+struct signal {
+	struct rb_node node;
+	struct intel_wait wait;
+	struct drm_i915_gem_request *request;
+};
+
+static bool signal_complete(struct signal *signal)
+{
+	if (signal == NULL)
+		return false;
+
+	/* If another process served as the bottom-half it may have already
+	 * signalled that this wait is already completed.
+	 */
+	if (intel_wait_complete(&signal->wait))
+		return true;
+
+	/* Carefully check if the request is complete, giving time for the
+	 * seqno to be visible or if the GPU hung.
+	 */
+	if (__i915_request_irq_complete(signal->request))
+		return true;
+
+	return false;
+}
+
+static struct signal *to_signal(struct rb_node *rb)
+{
+	return container_of(rb, struct signal, node);
+}
+
+static void signaler_set_rtpriority(void)
+{
+	 struct sched_param param = { .sched_priority = 1 };
+	 sched_setscheduler_nocheck(current, SCHED_FIFO, &param);
+}
+
+static int intel_breadcrumbs_signaler(void *arg)
+{
+	struct intel_engine_cs *engine = arg;
+	struct intel_breadcrumbs *b = &engine->breadcrumbs;
+	struct signal *signal;
+
+	/* Install ourselves with high priority to reduce signalling latency */
+	signaler_set_rtpriority();
+
+	do {
+		set_current_state(TASK_INTERRUPTIBLE);
+
+		/* We are either woken up by the interrupt bottom-half,
+		 * or by a client adding a new signaller. In both cases,
+		 * the GPU seqno may have advanced beyond our oldest signal.
+		 * If it has, propagate the signal, remove the waiter and
+		 * check again with the next oldest signal. Otherwise we
+		 * need to wait for a new interrupt from the GPU or for
+		 * a new client.
+		 */
+		signal = READ_ONCE(b->first_signal);
+		if (signal_complete(signal)) {
+			/* Wake up all other completed waiters and select the
+			 * next bottom-half for the next user interrupt.
+			 */
+			intel_engine_remove_wait(engine, &signal->wait);
+
+			i915_gem_request_unreference(signal->request);
+
+			/* Find the next oldest signal. Note that as we have
+			 * not been holding the lock, another client may
+			 * have installed an even older signal than the one
+			 * we just completed - so double check we are still
+			 * the oldest before picking the next one.
+			 */
+			spin_lock(&b->lock);
+			if (signal == b->first_signal)
+				b->first_signal = rb_next(&signal->node);
+			rb_erase(&signal->node, &b->signals);
+			spin_unlock(&b->lock);
+
+			kfree(signal);
+		} else {
+			if (kthread_should_stop())
+				break;
+
+			schedule();
+		}
+	} while (1);
+	__set_current_state(TASK_RUNNING);
+
+	return 0;
+}
+
+int intel_engine_enable_signaling(struct drm_i915_gem_request *request)
+{
+	struct intel_engine_cs *engine = request->engine;
+	struct intel_breadcrumbs *b = &engine->breadcrumbs;
+	struct rb_node *parent, **p;
+	struct signal *signal;
+	bool first, wakeup;
+
+	signal = kmalloc(sizeof(*signal), GFP_ATOMIC);
+	if (unlikely(!signal))
+		return -ENOMEM;
+
+	signal->wait.tsk = b->signaler;
+	signal->wait.seqno = request->seqno;
+
+	signal->request = i915_gem_request_reference(request);
+
+	/* First add ourselves into the list of waiters, but register our
+	 * bottom-half as the signaller thread. As per usual, only the oldest
+	 * waiter (not just signaller) is tasked as the bottom-half waking
+	 * up all completed waiters after the user interrupt.
+	 *
+	 * If we are the oldest waiter, enable the irq (after which we
+	 * must double check that the seqno did not complete).
+	 */
+	wakeup = intel_engine_add_wait(engine, &signal->wait);
+
+	/* Now insert ourselves into the retirement ordered list of signals
+	 * on this engine. We track the oldest seqno as that will be the
+	 * first signal to complete.
+	 */
+	spin_lock(&b->lock);
+	parent = NULL;
+	first = true;
+	p = &b->signals.rb_node;
+	while (*p) {
+		parent = *p;
+		if (i915_seqno_passed(signal->wait.seqno,
+				      to_signal(parent)->wait.seqno)) {
+			p = &parent->rb_right;
+			first = false;
+		} else
+			p = &parent->rb_left;
+	}
+	rb_link_node(&signal->node, parent, p);
+	rb_insert_color(&signal->node, &b->signals);
+	if (first)
+		smp_store_mb(b->first_signal, signal);
+	spin_unlock(&b->lock);
+
+	if (wakeup)
+		wake_up_process(b->signaler);
+
+	return 0;
+}
+
  int intel_engine_init_breadcrumbs(struct intel_engine_cs *engine)
  {
  	struct intel_breadcrumbs *b = &engine->breadcrumbs;
+	struct task_struct *tsk;

  	spin_lock_init(&b->lock);
  	setup_timer(&b->fake_irq,
  		    intel_breadcrumbs_fake_irq,
  		    (unsigned long)engine);

+	/* Spawn a thread to provide a common bottom-half for all signals.
+	 * As this is an asynchronous interface we cannot steal the current
+	 * task for handling the bottom-half to the user interrupt, therefore
+	 * we create a thread to do the coherent seqno dance after the
+	 * interrupt and then signal the waitqueue (via the dma-buf/fence).
+	 */
+	tsk = kthread_run(intel_breadcrumbs_signaler, engine,
+			  "irq/i915:%d", engine->id);
+	if (IS_ERR(tsk))
+		return PTR_ERR(tsk);
+
+	b->signaler = tsk;
+
  	return 0;
  }

@@ -356,6 +528,9 @@ void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine)
  {
  	struct intel_breadcrumbs *b = &engine->breadcrumbs;

+	if (!IS_ERR_OR_NULL(b->signaler))
+		kthread_stop(b->signaler);
+
  	del_timer_sync(&b->fake_irq);
  }

@@ -377,3 +552,18 @@ unsigned intel_kick_waiters(struct drm_i915_private *i915)

  	return mask;
  }
+
+unsigned intel_kick_signalers(struct drm_i915_private *i915)
+{
+	struct intel_engine_cs *engine;
+	unsigned mask = 0;
+
+	for_each_engine(engine, i915) {
+		if (unlikely(READ_ONCE(engine->breadcrumbs.first_signal))) {
+			wake_up_process(engine->breadcrumbs.signaler);
+			mask |= intel_engine_flag(engine);
+		}
+	}
+
+	return mask;
+}
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 4d6c2b773a64..c05b45727f7d 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -141,6 +141,8 @@ struct  i915_ctx_workarounds {
  	struct drm_i915_gem_object *obj;
  };

+struct drm_i915_gem_request;
+
  struct intel_engine_cs {
  	struct drm_i915_private *i915;
  	const char	*name;
@@ -179,8 +181,11 @@ struct intel_engine_cs {
  	struct intel_breadcrumbs {
  		spinlock_t lock; /* protects the lists of requests */
  		struct rb_root waiters; /* sorted by retirement, priority */
+		struct rb_root signals; /* sorted by retirement */
  		struct intel_wait *first_wait; /* oldest waiter by retirement */
  		struct task_struct *tasklet; /* bh for user interrupts */
+		struct task_struct *signaler; /* used for fence signalling */
+		void *first_signal;
  		struct timer_list fake_irq; /* used after a missed interrupt */
  		bool irq_enabled;
  		bool rpm_wakelock;
@@ -199,7 +204,6 @@ struct intel_engine_cs {
  	unsigned irq_refcount; /* protected by dev_priv->irq_lock */
  	bool		irq_posted;
  	u32		irq_enable_mask;	/* bitmask to enable ring interrupt */
-	struct drm_i915_gem_request *trace_irq_req;
  	bool __must_check (*irq_get)(struct intel_engine_cs *ring);
  	void		(*irq_put)(struct intel_engine_cs *ring);

@@ -540,6 +544,7 @@ bool intel_engine_add_wait(struct intel_engine_cs *engine,
  			   struct intel_wait *wait);
  void intel_engine_remove_wait(struct intel_engine_cs *engine,
  			      struct intel_wait *wait);
+int intel_engine_enable_signaling(struct drm_i915_gem_request *request);
  static inline bool intel_engine_has_waiter(struct intel_engine_cs *engine)
  {
  	return READ_ONCE(engine->breadcrumbs.tasklet);
@@ -563,5 +568,6 @@ static inline bool intel_engine_wakeup(struct intel_engine_cs *engine)
  void intel_engine_enable_fake_irq(struct intel_engine_cs *engine);
  void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine);
  unsigned intel_kick_waiters(struct drm_i915_private *i915);
+unsigned intel_kick_signalers(struct drm_i915_private *i915);

  #endif /* _INTEL_RINGBUFFER_H_ */


Looks good to me.

Only thing I would possibly change is the thread name to not mislead anyone into thinking this is the main and only i915 irq activity. Looks to me it should just be called something like i915/signaller:%u or similar.

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

Regards,

Tvrtko


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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux