* [PATCH] drm/i915: Use seqlock in engine stats
@ 2018-02-15 11:13 Tvrtko Ursulin
2018-02-15 11:47 ` Chris Wilson
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Tvrtko Ursulin @ 2018-02-15 11:13 UTC (permalink / raw)
To: Intel-gfx
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.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.
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>
---
drivers/gpu/drm/i915/intel_engine_cs.c | 19 ++++++++++---------
drivers/gpu/drm/i915/intel_ringbuffer.h | 12 +++++++-----
2 files changed, 17 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index f3c5100d629e..5a9dda74cd9e 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);
@@ -1975,7 +1975,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;
@@ -1999,7 +1999,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;
@@ -2028,12 +2028,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;
}
@@ -2051,13 +2052,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 51523ad049de..a947a5842f5b 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.
*/
@@ -1019,7 +1021,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)
@@ -1027,7 +1029,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)
@@ -1037,7 +1039,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;
@@ -1064,7 +1066,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);
--
2.14.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: Use seqlock in engine stats
2018-02-15 11:13 [PATCH] drm/i915: Use seqlock in engine stats Tvrtko Ursulin
@ 2018-02-15 11:47 ` Chris Wilson
2018-02-15 11:59 ` Tvrtko Ursulin
2018-02-15 11:47 ` ✓ Fi.CI.BAT: success for " Patchwork
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Chris Wilson @ 2018-02-15 11:47 UTC (permalink / raw)
To: Tvrtko Ursulin, Intel-gfx
Quoting Tvrtko Ursulin (2018-02-15 11:13:33)
> 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.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.
>
> 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>
> ---
> @@ -2028,12 +2028,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));
Hmm, only the reader is inside a hard-irq context right? (Thanks perf!)
I was hoping we could avoid disabling irqs around the writer. Now that
requires some coffee...
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Use seqlock in engine stats
2018-02-15 11:13 [PATCH] drm/i915: Use seqlock in engine stats Tvrtko Ursulin
2018-02-15 11:47 ` Chris Wilson
@ 2018-02-15 11:47 ` Patchwork
2018-02-15 14:50 ` ✓ Fi.CI.BAT: success for drm/i915: Use seqlock in engine stats (rev2) Patchwork
2018-02-15 23:50 ` ✗ Fi.CI.IGT: failure " Patchwork
3 siblings, 0 replies; 11+ messages in thread
From: Patchwork @ 2018-02-15 11:47 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Use seqlock in engine stats
URL : https://patchwork.freedesktop.org/series/38347/
State : success
== Summary ==
Series 38347v1 drm/i915: Use seqlock in engine stats
https://patchwork.freedesktop.org/api/1.0/series/38347/revisions/1/mbox/
Test kms_chamelium:
Subgroup dp-edid-read:
pass -> FAIL (fi-kbl-7500u) fdo#102505
fdo#102505 https://bugs.freedesktop.org/show_bug.cgi?id=102505
fi-bdw-5557u total:288 pass:267 dwarn:0 dfail:0 fail:0 skip:21 time:418s
fi-bdw-gvtdvm total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:424s
fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:375s
fi-bsw-n3050 total:288 pass:242 dwarn:0 dfail:0 fail:0 skip:46 time:484s
fi-bwr-2160 total:288 pass:183 dwarn:0 dfail:0 fail:0 skip:105 time:286s
fi-bxt-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:480s
fi-bxt-j4205 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:482s
fi-byt-j1900 total:288 pass:253 dwarn:0 dfail:0 fail:0 skip:35 time:470s
fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:458s
fi-cfl-s2 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:562s
fi-elk-e7500 total:288 pass:229 dwarn:0 dfail:0 fail:0 skip:59 time:417s
fi-gdg-551 total:288 pass:179 dwarn:0 dfail:0 fail:1 skip:108 time:281s
fi-glk-1 total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:510s
fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:391s
fi-ilk-650 total:288 pass:228 dwarn:0 dfail:0 fail:0 skip:60 time:414s
fi-ivb-3520m total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:449s
fi-kbl-7500u total:288 pass:262 dwarn:1 dfail:0 fail:1 skip:24 time:452s
fi-kbl-7560u total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:496s
fi-kbl-r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:500s
fi-pnv-d510 total:288 pass:222 dwarn:1 dfail:0 fail:0 skip:65 time:589s
fi-skl-6260u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:430s
fi-skl-6600u total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:511s
fi-skl-6700hq total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:527s
fi-skl-6700k2 total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:490s
fi-skl-6770hq total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:473s
fi-skl-guc total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:419s
fi-skl-gvtdvm total:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:430s
fi-snb-2520m total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:517s
fi-snb-2600 total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:398s
Blacklisted hosts:
fi-glk-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:468s
fi-kbl-7567u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:451s
bd16af128e78b302b3034fa85626cd15dcf5f038 drm-tip: 2018y-02m-15d-00h-22m-40s UTC integration manifest
36343aa25d03 drm/i915: Use seqlock in engine stats
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8046/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: Use seqlock in engine stats
2018-02-15 11:47 ` Chris Wilson
@ 2018-02-15 11:59 ` Tvrtko Ursulin
2018-02-15 12:04 ` Chris Wilson
0 siblings, 1 reply; 11+ messages in thread
From: Tvrtko Ursulin @ 2018-02-15 11:59 UTC (permalink / raw)
To: Chris Wilson, Tvrtko Ursulin, Intel-gfx
On 15/02/2018 11:47, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-02-15 11:13:33)
>> 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.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.
>>
>> 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>
>> ---
>> @@ -2028,12 +2028,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));
>
> Hmm, only the reader is inside a hard-irq context right? (Thanks perf!)
>
> I was hoping we could avoid disabling irqs around the writer. Now that
> requires some coffee...
Oh yeah.. it was irqsave only because of perf. I am pretty sure you're
right and I can downgrade write locking to _bh - after the recent
changes we got a writer in process context (enable/disable), and a
writer in tasklet context (context in/out).
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: Use seqlock in engine stats
2018-02-15 11:59 ` Tvrtko Ursulin
@ 2018-02-15 12:04 ` Chris Wilson
2018-02-15 13:45 ` [PATCH v2] " Tvrtko Ursulin
0 siblings, 1 reply; 11+ messages in thread
From: Chris Wilson @ 2018-02-15 12:04 UTC (permalink / raw)
To: Tvrtko Ursulin, Tvrtko Ursulin, Intel-gfx
Quoting Tvrtko Ursulin (2018-02-15 11:59:06)
>
> On 15/02/2018 11:47, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2018-02-15 11:13:33)
> >> 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.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.
> >>
> >> 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>
> >> ---
> >> @@ -2028,12 +2028,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));
> >
> > Hmm, only the reader is inside a hard-irq context right? (Thanks perf!)
> >
> > I was hoping we could avoid disabling irqs around the writer. Now that
> > requires some coffee...
>
> Oh yeah.. it was irqsave only because of perf. I am pretty sure you're
> right and I can downgrade write locking to _bh - after the recent
> changes we got a writer in process context (enable/disable), and a
> writer in tasklet context (context in/out).
First look says that the seqlock has lockdep magic to tell us if we are
wrong. (Or at least we need to be creative ;)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] drm/i915: Use seqlock in engine stats
2018-02-15 12:04 ` Chris Wilson
@ 2018-02-15 13:45 ` Tvrtko Ursulin
0 siblings, 0 replies; 11+ messages in thread
From: Tvrtko Ursulin @ 2018-02-15 13:45 UTC (permalink / raw)
To: Intel-gfx
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.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 <tvrtko.ursulin@intel.com>
Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
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(-)
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);
--
2.14.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 11+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Use seqlock in engine stats (rev2)
2018-02-15 11:13 [PATCH] drm/i915: Use seqlock in engine stats Tvrtko Ursulin
2018-02-15 11:47 ` Chris Wilson
2018-02-15 11:47 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2018-02-15 14:50 ` Patchwork
2018-02-15 23:50 ` ✗ Fi.CI.IGT: failure " Patchwork
3 siblings, 0 replies; 11+ messages in thread
From: Patchwork @ 2018-02-15 14:50 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Use seqlock in engine stats (rev2)
URL : https://patchwork.freedesktop.org/series/38347/
State : success
== Summary ==
Series 38347v2 drm/i915: Use seqlock in engine stats
https://patchwork.freedesktop.org/api/1.0/series/38347/revisions/2/mbox/
Test gem_mmap_gtt:
Subgroup basic-small-bo-tiledx:
pass -> FAIL (fi-gdg-551) fdo#102575
Test kms_pipe_crc_basic:
Subgroup suspend-read-crc-pipe-b:
pass -> INCOMPLETE (fi-snb-2520m) fdo#103713
incomplete -> PASS (fi-hsw-4770) fdo#103375
fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
fi-bdw-5557u total:288 pass:267 dwarn:0 dfail:0 fail:0 skip:21 time:417s
fi-bdw-gvtdvm total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:424s
fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:376s
fi-bsw-n3050 total:288 pass:242 dwarn:0 dfail:0 fail:0 skip:46 time:485s
fi-bwr-2160 total:288 pass:183 dwarn:0 dfail:0 fail:0 skip:105 time:287s
fi-bxt-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:482s
fi-bxt-j4205 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:485s
fi-byt-j1900 total:288 pass:253 dwarn:0 dfail:0 fail:0 skip:35 time:469s
fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:457s
fi-cfl-s2 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:563s
fi-elk-e7500 total:288 pass:229 dwarn:0 dfail:0 fail:0 skip:59 time:413s
fi-gdg-551 total:288 pass:179 dwarn:0 dfail:0 fail:1 skip:108 time:282s
fi-glk-1 total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:512s
fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:390s
fi-ilk-650 total:288 pass:228 dwarn:0 dfail:0 fail:0 skip:60 time:410s
fi-ivb-3520m total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:455s
fi-kbl-7500u total:288 pass:263 dwarn:1 dfail:0 fail:0 skip:24 time:457s
fi-kbl-7560u total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:500s
fi-kbl-7567u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:456s
fi-kbl-r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:506s
fi-pnv-d510 total:288 pass:222 dwarn:1 dfail:0 fail:0 skip:65 time:586s
fi-skl-6260u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:428s
fi-skl-6600u total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:504s
fi-skl-6700hq total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:526s
fi-skl-6700k2 total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:492s
fi-skl-6770hq total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:480s
fi-skl-guc total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:415s
fi-skl-gvtdvm total:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:433s
fi-snb-2520m total:245 pass:211 dwarn:0 dfail:0 fail:0 skip:33
fi-snb-2600 total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:400s
Blacklisted hosts:
fi-glk-dsi total:288 pass:257 dwarn:0 dfail:0 fail:1 skip:30 time:467s
47dbb4971216e1285bb8f8b9bb5a56da144d2fe4 drm-tip: 2018y-02m-15d-13h-53m-14s UTC integration manifest
7a0565729491 drm/i915: Use seqlock in engine stats
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8049/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* ✗ Fi.CI.IGT: failure for drm/i915: Use seqlock in engine stats (rev2)
2018-02-15 11:13 [PATCH] drm/i915: Use seqlock in engine stats Tvrtko Ursulin
` (2 preceding siblings ...)
2018-02-15 14:50 ` ✓ Fi.CI.BAT: success for drm/i915: Use seqlock in engine stats (rev2) Patchwork
@ 2018-02-15 23:50 ` Patchwork
2018-02-16 10:13 ` Chris Wilson
3 siblings, 1 reply; 11+ messages in thread
From: Patchwork @ 2018-02-15 23:50 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Use seqlock in engine stats (rev2)
URL : https://patchwork.freedesktop.org/series/38347/
State : failure
== Summary ==
Test perf_pmu:
Subgroup busy-start-vcs0:
pass -> DMESG-WARN (shard-apl)
Subgroup busy-idle-check-all-vecs0:
pass -> DMESG-WARN (shard-apl)
Subgroup render-node-busy-idle-vcs0:
pass -> DMESG-WARN (shard-apl)
Subgroup most-busy-idle-check-all-bcs0:
pass -> DMESG-WARN (shard-apl)
Subgroup busy-idle-check-all-bcs0:
pass -> DMESG-WARN (shard-apl)
Subgroup most-busy-check-all-bcs0:
pass -> DMESG-WARN (shard-apl)
Subgroup render-node-busy-bcs0:
pass -> DMESG-WARN (shard-apl)
Subgroup busy-idle-vecs0:
pass -> DMESG-WARN (shard-apl)
Test kms_sysfs_edid_timing:
pass -> WARN (shard-snb)
warn -> PASS (shard-apl) fdo#100047
Test kms_rotation_crc:
Subgroup primary-rotation-270:
pass -> FAIL (shard-apl)
Subgroup primary-rotation-90:
pass -> FAIL (shard-apl) fdo#103925
Subgroup cursor-rotation-180:
pass -> FAIL (shard-apl)
Test pm_rc6_residency:
Subgroup rc6-accuracy:
pass -> SKIP (shard-snb)
Test gem_eio:
Subgroup in-flight-contexts:
pass -> FAIL (shard-hsw) fdo#104676
Test kms_cursor_legacy:
Subgroup short-flip-after-cursor-atomic-transitions-varying-size:
skip -> PASS (shard-snb) fdo#102670
Test kms_draw_crc:
Subgroup draw-method-xrgb2101010-mmap-cpu-untiled:
skip -> PASS (shard-snb)
Test kms_properties:
Subgroup crtc-properties-legacy:
pass -> SKIP (shard-snb)
Test kms_frontbuffer_tracking:
Subgroup fbc-1p-primscrn-pri-shrfb-draw-mmap-cpu:
pass -> SKIP (shard-snb) fdo#103167
Subgroup fbc-suspend:
incomplete -> PASS (shard-hsw) fdo#103540
Test gem_exec_suspend:
Subgroup basic-s3-devices:
pass -> INCOMPLETE (shard-hsw)
Test kms_flip:
Subgroup plain-flip-ts-check-interruptible:
pass -> FAIL (shard-hsw) fdo#100368
Test kms_plane_lowres:
Subgroup pipe-a-tiling-x:
pass -> FAIL (shard-apl) fdo#103166
fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
fdo#104676 https://bugs.freedesktop.org/show_bug.cgi?id=104676
fdo#102670 https://bugs.freedesktop.org/show_bug.cgi?id=102670
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#103540 https://bugs.freedesktop.org/show_bug.cgi?id=103540
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
shard-apl total:3336 pass:1723 dwarn:9 dfail:0 fail:23 skip:1580 time:11212s
shard-hsw total:3377 pass:1724 dwarn:1 dfail:0 fail:12 skip:1638 time:11299s
shard-snb total:3427 pass:1344 dwarn:1 dfail:0 fail:12 skip:2069 time:6681s
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8049/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: ✗ Fi.CI.IGT: failure for drm/i915: Use seqlock in engine stats (rev2)
2018-02-15 23:50 ` ✗ Fi.CI.IGT: failure " Patchwork
@ 2018-02-16 10:13 ` Chris Wilson
2018-02-19 9:25 ` Tvrtko Ursulin
0 siblings, 1 reply; 11+ messages in thread
From: Chris Wilson @ 2018-02-16 10:13 UTC (permalink / raw)
To: Patchwork, Tvrtko Ursulin; +Cc: intel-gfx
Quoting Patchwork (2018-02-15 23:50:52)
> == Series Details ==
>
> Series: drm/i915: Use seqlock in engine stats (rev2)
> URL : https://patchwork.freedesktop.org/series/38347/
> State : failure
>
> == Summary ==
>
> Test perf_pmu:
> Subgroup busy-start-vcs0:
> pass -> DMESG-WARN (shard-apl)
> Subgroup busy-idle-check-all-vecs0:
> pass -> DMESG-WARN (shard-apl)
> Subgroup render-node-busy-idle-vcs0:
> pass -> DMESG-WARN (shard-apl)
> Subgroup most-busy-idle-check-all-bcs0:
> pass -> DMESG-WARN (shard-apl)
> Subgroup busy-idle-check-all-bcs0:
> pass -> DMESG-WARN (shard-apl)
> Subgroup most-busy-check-all-bcs0:
> pass -> DMESG-WARN (shard-apl)
> Subgroup render-node-busy-bcs0:
> pass -> DMESG-WARN (shard-apl)
> Subgroup busy-idle-vecs0:
> pass -> DMESG-WARN (shard-apl)
Afaict, it's complaining that because the writer is special the reader
needs irq protection. Which I don't understand, as the point is that the
writer can run concurrently to the readers (the readers have to
restart). What am I missing?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: ✗ Fi.CI.IGT: failure for drm/i915: Use seqlock in engine stats (rev2)
2018-02-16 10:13 ` Chris Wilson
@ 2018-02-19 9:25 ` Tvrtko Ursulin
2018-02-19 9:29 ` Chris Wilson
0 siblings, 1 reply; 11+ messages in thread
From: Tvrtko Ursulin @ 2018-02-19 9:25 UTC (permalink / raw)
To: Chris Wilson, intel-gfx, Patchwork, Tvrtko Ursulin
On 16/02/2018 10:13, Chris Wilson wrote:
> Quoting Patchwork (2018-02-15 23:50:52)
>> == Series Details ==
>>
>> Series: drm/i915: Use seqlock in engine stats (rev2)
>> URL : https://patchwork.freedesktop.org/series/38347/
>> State : failure
>>
>> == Summary ==
>>
>> Test perf_pmu:
>> Subgroup busy-start-vcs0:
>> pass -> DMESG-WARN (shard-apl)
>> Subgroup busy-idle-check-all-vecs0:
>> pass -> DMESG-WARN (shard-apl)
>> Subgroup render-node-busy-idle-vcs0:
>> pass -> DMESG-WARN (shard-apl)
>> Subgroup most-busy-idle-check-all-bcs0:
>> pass -> DMESG-WARN (shard-apl)
>> Subgroup busy-idle-check-all-bcs0:
>> pass -> DMESG-WARN (shard-apl)
>> Subgroup most-busy-check-all-bcs0:
>> pass -> DMESG-WARN (shard-apl)
>> Subgroup render-node-busy-bcs0:
>> pass -> DMESG-WARN (shard-apl)
>> Subgroup busy-idle-vecs0:
>> pass -> DMESG-WARN (shard-apl)
>
> Afaict, it's complaining that because the writer is special the reader
> needs irq protection. Which I don't understand, as the point is that the
> writer can run concurrently to the readers (the readers have to
> restart). What am I missing?
As discussed on IRC, issue is courtesy of perf read callback running in
hardirq context, that the reader can interrupt the writer and so
incorrectly declare a stable sequence number while reading the mixed up
version of the underlying data.
Writer:
seqno++
modify some fields
<Reader IRQ...
read seqno
read fields
re-read seqno -> OK
>..Reader IRQ
modify other fields
seqno++
So we need to go back to the first, irqsave version. Do you want to
pursue this change right now or leave it for some future work?
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: ✗ Fi.CI.IGT: failure for drm/i915: Use seqlock in engine stats (rev2)
2018-02-19 9:25 ` Tvrtko Ursulin
@ 2018-02-19 9:29 ` Chris Wilson
0 siblings, 0 replies; 11+ messages in thread
From: Chris Wilson @ 2018-02-19 9:29 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx, Patchwork, Tvrtko Ursulin
Quoting Tvrtko Ursulin (2018-02-19 09:25:00)
>
> On 16/02/2018 10:13, Chris Wilson wrote:
> > Afaict, it's complaining that because the writer is special the reader
> > needs irq protection. Which I don't understand, as the point is that the
> > writer can run concurrently to the readers (the readers have to
> > restart). What am I missing?
>
> As discussed on IRC, issue is courtesy of perf read callback running in
> hardirq context, that the reader can interrupt the writer and so
> incorrectly declare a stable sequence number while reading the mixed up
> version of the underlying data.
>
> Writer:
> seqno++
> modify some fields
> <Reader IRQ...
> read seqno
> read fields
> re-read seqno -> OK
> >..Reader IRQ
> modify other fields
> seqno++
>
> So we need to go back to the first, irqsave version. Do you want to
> pursue this change right now or leave it for some future work?
It's a small optimisation (hopefully ;) so pursue at leisure or in the
context of wider work.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-02-19 9:29 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-15 11:13 [PATCH] drm/i915: Use seqlock in engine stats Tvrtko Ursulin
2018-02-15 11:47 ` Chris Wilson
2018-02-15 11:59 ` Tvrtko Ursulin
2018-02-15 12:04 ` Chris Wilson
2018-02-15 13:45 ` [PATCH v2] " Tvrtko Ursulin
2018-02-15 11:47 ` ✓ Fi.CI.BAT: success for " Patchwork
2018-02-15 14:50 ` ✓ Fi.CI.BAT: success for drm/i915: Use seqlock in engine stats (rev2) Patchwork
2018-02-15 23:50 ` ✗ Fi.CI.IGT: failure " Patchwork
2018-02-16 10:13 ` Chris Wilson
2018-02-19 9:25 ` Tvrtko Ursulin
2018-02-19 9:29 ` Chris Wilson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.