[v2] drm/i915: Use seqlock in engine stats

Commit Message

Tvrtko Ursulin Feb. 15, 2018, 1:45 p.m. UTC

From: Tvrtko Ursulin <[e-mail]>

We can convert engine stats from a spinlock to seqlock to ensure interrupt
processing is never even a tiny bit delayed by parallel readers.

There is a smidgen bit more cost on the write lock side, and an extremely
unlikely chance that readers will have to retry a few times in face of
heavy interrupt load.Bbut it should be extremely unlikely given how
lightweight read side section is compared to the interrupt processing side,
and also compared to the rest of the code paths which can lead into it.

v2:
Relax locking to reflect API usage is now from process context and
tasklet. (Chris Wilson)

Signed-off-by: Tvrtko Ursulin <[e-mail]>
Suggested-by: Chris Wilson <[e-mail]>
Cc: Chris Wilson <[e-mail]>
---
Note the call path to intel_engine_context_out from
execlists_cancel_requests which is not from the tasklet, but happens to
run with interrupts disabled. So should work buty perhaps not the most
obvious.
---
 drivers/gpu/drm/i915/intel_engine_cs.c  | 22 ++++++++++------------
 drivers/gpu/drm/i915/intel_ringbuffer.h | 16 +++++++---------
 2 files changed, 17 insertions(+), 21 deletions(-)

Patch

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index f3c5100d629e..92a6e82304f9 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -246,7 +246,7 @@  intel_engine_setup(struct drm_i915_private *dev_priv,
 	/* Nothing to do here, execute in order of dependencies */
 	engine->schedule = NULL;
 
-	spin_lock_init(&engine->stats.lock);
+	seqlock_init(&engine->stats.lock);
 
 	ATOMIC_INIT_NOTIFIER_HEAD(&engine->context_status_notifier);
 
@@ -1968,14 +1968,13 @@  intel_engine_lookup_user(struct drm_i915_private *i915, u8 class, u8 instance)
 int intel_enable_engine_stats(struct intel_engine_cs *engine)
 {
 	struct intel_engine_execlists *execlists = &engine->execlists;
-	unsigned long flags;
 	int err = 0;
 
 	if (!intel_engine_supports_stats(engine))
 		return -ENODEV;
 
 	tasklet_disable(&execlists->tasklet);
-	spin_lock_irqsave(&engine->stats.lock, flags);
+	write_seqlock_bh(&engine->stats.lock);
 
 	if (unlikely(engine->stats.enabled == ~0)) {
 		err = -EBUSY;
@@ -1999,7 +1998,7 @@  int intel_enable_engine_stats(struct intel_engine_cs *engine)
 	}
 
 unlock:
-	spin_unlock_irqrestore(&engine->stats.lock, flags);
+	write_sequnlock_bh(&engine->stats.lock);
 	tasklet_enable(&execlists->tasklet);
 
 	return err;
@@ -2028,12 +2027,13 @@  static ktime_t __intel_engine_get_busy_time(struct intel_engine_cs *engine)
  */
 ktime_t intel_engine_get_busy_time(struct intel_engine_cs *engine)
 {
+	unsigned int seq;
 	ktime_t total;
-	unsigned long flags;
 
-	spin_lock_irqsave(&engine->stats.lock, flags);
-	total = __intel_engine_get_busy_time(engine);
-	spin_unlock_irqrestore(&engine->stats.lock, flags);
+	do {
+		seq = read_seqbegin(&engine->stats.lock);
+		total = __intel_engine_get_busy_time(engine);
+	} while (read_seqretry(&engine->stats.lock, seq));
 
 	return total;
 }
@@ -2046,18 +2046,16 @@  ktime_t intel_engine_get_busy_time(struct intel_engine_cs *engine)
  */
 void intel_disable_engine_stats(struct intel_engine_cs *engine)
 {
-	unsigned long flags;
-
 	if (!intel_engine_supports_stats(engine))
 		return;
 
-	spin_lock_irqsave(&engine->stats.lock, flags);
+	write_seqlock_bh(&engine->stats.lock);
 	WARN_ON_ONCE(engine->stats.enabled == 0);
 	if (--engine->stats.enabled == 0) {
 		engine->stats.total = __intel_engine_get_busy_time(engine);
 		engine->stats.active = 0;
 	}
-	spin_unlock_irqrestore(&engine->stats.lock, flags);
+	write_sequnlock_bh(&engine->stats.lock);
 }
 
 #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 51523ad049de..68f273cde012 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -3,6 +3,8 @@ 
 #define _INTEL_RINGBUFFER_H_
 
 #include <linux/hashtable.h>
+#include <linux/seqlock.h>
+
 #include "i915_gem_batch_pool.h"
 #include "i915_gem_request.h"
 #include "i915_gem_timeline.h"
@@ -567,7 +569,7 @@  struct intel_engine_cs {
 		/**
 		 * @lock: Lock protecting the below fields.
 		 */
-		spinlock_t lock;
+		seqlock_t lock;
 		/**
 		 * @enabled: Reference count indicating number of listeners.
 		 */
@@ -1014,12 +1016,10 @@  intel_engine_lookup_user(struct drm_i915_private *i915, u8 class, u8 instance);
 
 static inline void intel_engine_context_in(struct intel_engine_cs *engine)
 {
-	unsigned long flags;
-
 	if (READ_ONCE(engine->stats.enabled) == 0)
 		return;
 
-	spin_lock_irqsave(&engine->stats.lock, flags);
+	write_seqlock(&engine->stats.lock);
 
 	if (engine->stats.enabled > 0) {
 		if (engine->stats.active++ == 0)
@@ -1027,17 +1027,15 @@  static inline void intel_engine_context_in(struct intel_engine_cs *engine)
 		GEM_BUG_ON(engine->stats.active == 0);
 	}
 
-	spin_unlock_irqrestore(&engine->stats.lock, flags);
+	write_sequnlock(&engine->stats.lock);
 }
 
 static inline void intel_engine_context_out(struct intel_engine_cs *engine)
 {
-	unsigned long flags;
-
 	if (READ_ONCE(engine->stats.enabled) == 0)
 		return;
 
-	spin_lock_irqsave(&engine->stats.lock, flags);
+	write_seqlock(&engine->stats.lock);
 
 	if (engine->stats.enabled > 0) {
 		ktime_t last;
@@ -1064,7 +1062,7 @@  static inline void intel_engine_context_out(struct intel_engine_cs *engine)
 		}
 	}
 
-	spin_unlock_irqrestore(&engine->stats.lock, flags);
+	write_sequnlock(&engine->stats.lock);
 }
 
 int intel_enable_engine_stats(struct intel_engine_cs *engine);