diff mbox

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

Message ID 20180426074716.7352-1-tvrtko.ursulin@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tvrtko Ursulin April 26, 2018, 7:47 a.m. UTC 
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

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. But 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. Furthermore, writer is the ones doing the real, latency sensitive
work, while readers are only informative.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_engine_cs.c  | 19 ++++++++++---------
 drivers/gpu/drm/i915/intel_ringbuffer.h | 11 ++++++-----
 2 files changed, 16 insertions(+), 14 deletions(-)

Comments

Tvrtko Ursulin April 26, 2018, 10:26 a.m. UTC | #1 
On 26/04/2018 09:16, Patchwork wrote:
> == Series Details ==
> 
> Series: drm/i915: Use seqlock in engine stats (rev3)
> URL   : https://patchwork.freedesktop.org/series/38347/
> State : success
> 
> == Summary ==
> 
> = CI Bug Log - changes from CI_DRM_4100 -> Patchwork_8806 =
> 
> == Summary - SUCCESS ==
> 
>    No regressions found.
> 
>    External URL: https://patchwork.freedesktop.org/api/1.0/series/38347/revisions/3/mbox/
> 
> == Known issues ==
> 
>    Here are the changes found in Patchwork_8806 that come from known issues:
> 
>    === IGT changes ===
> 
>      ==== Possible fixes ====
> 
>      igt@debugfs_test@read_all_entries:
>        fi-snb-2520m:       INCOMPLETE (fdo#103713) -> PASS
> 
>      igt@kms_flip@basic-flip-vs-dpms:
>        fi-bxt-dsi:         INCOMPLETE (fdo#103927) -> PASS
> 
>      
>    fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
>    fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
> 
> 
> == Participating hosts (37 -> 35) ==
> 
>    Missing    (2): fi-ilk-m540 fi-skl-6700hq
> 
> 
> == Build changes ==
> 
>      * Linux: CI_DRM_4100 -> Patchwork_8806
> 
>    CI_DRM_4100: 05cc701ce9d22b01f2f4afa9fddd521b931ed163 @ git://anongit.freedesktop.org/gfx-ci/linux
>    IGT_4449: 0350f0e7f6a0e07281445fc3082aa70419f4aac7 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
>    Patchwork_8806: 9ce2e374546c8bd06ae6e97f8b5b7b6cdf8e7f0e @ git://anongit.freedesktop.org/gfx-ci/linux
>    piglit_4449: ad8992d3fb27fd604b9ab15e7963c42421ced85c @ git://anongit.freedesktop.org/piglit
> 
> 
> == Linux commits ==
> 
> 9ce2e374546c drm/i915: Use seqlock in engine stats

Pushed, thanks for the suggestion and review!

Regards,

Tvrtko
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 58be7fac5b8c..ac009f10c948 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -306,7 +306,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);
 
@@ -1481,7 +1481,7 @@  int intel_enable_engine_stats(struct intel_engine_cs *engine)
 		return -ENODEV;
 
 	tasklet_disable(&execlists->tasklet);
-	spin_lock_irqsave(&engine->stats.lock, flags);
+	write_seqlock_irqsave(&engine->stats.lock, flags);
 
 	if (unlikely(engine->stats.enabled == ~0)) {
 		err = -EBUSY;
@@ -1505,7 +1505,7 @@  int intel_enable_engine_stats(struct intel_engine_cs *engine)
 	}
 
 unlock:
-	spin_unlock_irqrestore(&engine->stats.lock, flags);
+	write_sequnlock_irqrestore(&engine->stats.lock, flags);
 	tasklet_enable(&execlists->tasklet);
 
 	return err;
@@ -1534,12 +1534,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;
 }
@@ -1557,13 +1558,13 @@  void intel_disable_engine_stats(struct intel_engine_cs *engine)
 	if (!intel_engine_supports_stats(engine))
 		return;
 
-	spin_lock_irqsave(&engine->stats.lock, flags);
+	write_seqlock_irqsave(&engine->stats.lock, flags);
 	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_irqrestore(&engine->stats.lock, flags);
 }
 
 #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 c5e27905b0e1..24af3f1088ba 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -3,6 +3,7 @@ 
 #define _INTEL_RINGBUFFER_H_
 
 #include <linux/hashtable.h>
+#include <linux/seqlock.h>
 
 #include "i915_gem_batch_pool.h"
 #include "i915_gem_timeline.h"
@@ -595,7 +596,7 @@  struct intel_engine_cs {
 		/**
 		 * @lock: Lock protecting the below fields.
 		 */
-		spinlock_t lock;
+		seqlock_t lock;
 		/**
 		 * @enabled: Reference count indicating number of listeners.
 		 */
@@ -1064,7 +1065,7 @@  static inline void intel_engine_context_in(struct intel_engine_cs *engine)
 	if (READ_ONCE(engine->stats.enabled) == 0)
 		return;
 
-	spin_lock_irqsave(&engine->stats.lock, flags);
+	write_seqlock_irqsave(&engine->stats.lock, flags);
 
 	if (engine->stats.enabled > 0) {
 		if (engine->stats.active++ == 0)
@@ -1072,7 +1073,7 @@  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_irqrestore(&engine->stats.lock, flags);
 }
 
 static inline void intel_engine_context_out(struct intel_engine_cs *engine)
@@ -1082,7 +1083,7 @@  static inline void intel_engine_context_out(struct intel_engine_cs *engine)
 	if (READ_ONCE(engine->stats.enabled) == 0)
 		return;
 
-	spin_lock_irqsave(&engine->stats.lock, flags);
+	write_seqlock_irqsave(&engine->stats.lock, flags);
 
 	if (engine->stats.enabled > 0) {
 		ktime_t last;
@@ -1109,7 +1110,7 @@  static inline void intel_engine_context_out(struct intel_engine_cs *engine)
 		}
 	}
 
-	spin_unlock_irqrestore(&engine->stats.lock, flags);
+	write_sequnlock_irqrestore(&engine->stats.lock, flags);
 }
 
 int intel_enable_engine_stats(struct intel_engine_cs *engine);