[PATCH 2/3] drm/i915: Split breadcrumbs spinlock into two
Chris Wilson
chris at chris-wilson.co.uk
Fri Mar 3 11:09:34 UTC 2017
As we now take the breadcrumbs spinlock within the interrupt handler, we
wish to minimise its hold time. During the interrupt we do not care
about the state of the full rbtree, only that of the first element, so
we can guard that with a separate lock.
Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_debugfs.c | 12 +++----
drivers/gpu/drm/i915/i915_drv.h | 4 +--
drivers/gpu/drm/i915/i915_gpu_error.c | 8 ++---
drivers/gpu/drm/i915/i915_irq.c | 4 +--
drivers/gpu/drm/i915/intel_breadcrumbs.c | 58 ++++++++++++++++++--------------
drivers/gpu/drm/i915/intel_ringbuffer.h | 6 ++--
6 files changed, 51 insertions(+), 41 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 95046822e8e0..aa2d726b4349 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -700,14 +700,14 @@ static void i915_ring_seqno_info(struct seq_file *m,
seq_printf(m, "Current sequence (%s): %x\n",
engine->name, intel_engine_get_seqno(engine));
- spin_lock_irq(&b->lock);
+ spin_lock_irq(&b->rb_lock);
for (rb = rb_first(&b->waiters); rb; rb = rb_next(rb)) {
struct intel_wait *w = rb_entry(rb, typeof(*w), node);
seq_printf(m, "Waiting (%s): %s [%d] on %x\n",
engine->name, w->tsk->comm, w->tsk->pid, w->seqno);
}
- spin_unlock_irq(&b->lock);
+ spin_unlock_irq(&b->rb_lock);
}
static int i915_gem_seqno_info(struct seq_file *m, void *data)
@@ -1354,14 +1354,14 @@ static int i915_hangcheck_info(struct seq_file *m, void *unused)
&dev_priv->gpu_error.missed_irq_rings)),
yesno(engine->hangcheck.stalled));
- spin_lock_irq(&b->lock);
+ spin_lock_irq(&b->rb_lock);
for (rb = rb_first(&b->waiters); rb; rb = rb_next(rb)) {
struct intel_wait *w = rb_entry(rb, typeof(*w), node);
seq_printf(m, "\t%s [%d] waiting for %x\n",
w->tsk->comm, w->tsk->pid, w->seqno);
}
- spin_unlock_irq(&b->lock);
+ spin_unlock_irq(&b->rb_lock);
seq_printf(m, "\tACTHD = 0x%08llx [current 0x%08llx]\n",
(long long)engine->hangcheck.acthd,
@@ -3359,14 +3359,14 @@ static int i915_engine_info(struct seq_file *m, void *unused)
I915_READ(RING_PP_DIR_DCLV(engine)));
}
- spin_lock_irq(&b->lock);
+ spin_lock_irq(&b->rb_lock);
for (rb = rb_first(&b->waiters); rb; rb = rb_next(rb)) {
struct intel_wait *w = rb_entry(rb, typeof(*w), node);
seq_printf(m, "\t%s [%d] waiting for %x\n",
w->tsk->comm, w->tsk->pid, w->seqno);
}
- spin_unlock_irq(&b->lock);
+ spin_unlock_irq(&b->rb_lock);
seq_puts(m, "\n");
}
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a639aae33ce6..564419cff447 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -4099,7 +4099,7 @@ __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).
*/
- spin_lock_irqsave(&b->lock, flags);
+ spin_lock_irqsave(&b->irq_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
@@ -4108,7 +4108,7 @@ __i915_request_irq_complete(const struct drm_i915_gem_request *req)
* ourself.
*/
wake_up_process(b->first_wait->tsk);
- spin_unlock_irqrestore(&b->lock, flags);
+ spin_unlock_irqrestore(&b->irq_lock, flags);
if (__i915_gem_request_completed(req, seqno))
return true;
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 061af8040498..8effc59f5cb5 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1111,7 +1111,7 @@ static void error_record_engine_waiters(struct intel_engine_cs *engine,
if (RB_EMPTY_ROOT(&b->waiters))
return;
- if (!spin_trylock_irq(&b->lock)) {
+ if (!spin_trylock_irq(&b->rb_lock)) {
ee->waiters = ERR_PTR(-EDEADLK);
return;
}
@@ -1119,7 +1119,7 @@ static void error_record_engine_waiters(struct intel_engine_cs *engine,
count = 0;
for (rb = rb_first(&b->waiters); rb != NULL; rb = rb_next(rb))
count++;
- spin_unlock_irq(&b->lock);
+ spin_unlock_irq(&b->rb_lock);
waiter = NULL;
if (count)
@@ -1129,7 +1129,7 @@ static void error_record_engine_waiters(struct intel_engine_cs *engine,
if (!waiter)
return;
- if (!spin_trylock_irq(&b->lock)) {
+ if (!spin_trylock_irq(&b->rb_lock)) {
kfree(waiter);
ee->waiters = ERR_PTR(-EDEADLK);
return;
@@ -1147,7 +1147,7 @@ static void error_record_engine_waiters(struct intel_engine_cs *engine,
if (++ee->num_waiters == count)
break;
}
- spin_unlock_irq(&b->lock);
+ spin_unlock_irq(&b->rb_lock);
}
static void error_record_engine_registers(struct i915_gpu_state *error,
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 5fa2c4c56b09..3f39e36fa566 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1041,7 +1041,7 @@ static void notify_ring(struct intel_engine_cs *engine)
rcu_read_lock();
- spin_lock(&engine->breadcrumbs.lock);
+ spin_lock(&engine->breadcrumbs.irq_lock);
wait = engine->breadcrumbs.first_wait;
if (wait) {
/* We use a callback from the dma-fence to submit
@@ -1063,7 +1063,7 @@ static void notify_ring(struct intel_engine_cs *engine)
} else {
__intel_engine_disarm_breadcrumbs(engine);
}
- spin_unlock(&engine->breadcrumbs.lock);
+ spin_unlock(&engine->breadcrumbs.irq_lock);
if (rq)
dma_fence_signal(&rq->fence);
diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index 1cc50304f824..34200f14bade 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -31,6 +31,8 @@ static unsigned int __intel_breadcrumbs_wakeup(struct intel_breadcrumbs *b)
struct intel_wait *wait;
unsigned int result = 0;
+ lockdep_assert_held(&b->irq_lock);
+
wait = b->first_wait;
if (wait) {
result = ENGINE_WAKEUP_WAITER;
@@ -47,9 +49,9 @@ unsigned int intel_engine_wakeup(struct intel_engine_cs *engine)
unsigned long flags;
unsigned int result;
- spin_lock_irqsave(&b->lock, flags);
+ spin_lock_irqsave(&b->irq_lock, flags);
result = __intel_breadcrumbs_wakeup(b);
- spin_unlock_irqrestore(&b->lock, flags);
+ spin_unlock_irqrestore(&b->irq_lock, flags);
return result;
}
@@ -117,10 +119,10 @@ static void intel_breadcrumbs_fake_irq(unsigned long data)
* coherent seqno check.
*/
- spin_lock_irqsave(&b->lock, flags);
+ spin_lock_irqsave(&b->irq_lock, flags);
if (!__intel_breadcrumbs_wakeup(b))
__intel_engine_disarm_breadcrumbs(engine);
- spin_unlock_irqrestore(&b->lock, flags);
+ spin_unlock_irqrestore(&b->irq_lock, flags);
if (!b->irq_armed)
return;
@@ -164,7 +166,7 @@ void __intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
{
struct intel_breadcrumbs *b = &engine->breadcrumbs;
- lockdep_assert_held(&b->lock);
+ lockdep_assert_held(&b->irq_lock);
if (b->irq_enabled) {
irq_disable(engine);
@@ -182,7 +184,7 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
if (!b->irq_armed)
return;
- spin_lock_irqsave(&b->lock, flags);
+ spin_lock_irqsave(&b->irq_lock, flags);
/* We only disarm the irq when we are idle (all requests completed),
* so if there remains a sleeping waiter, it missed the request
@@ -193,7 +195,7 @@ void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
__intel_engine_disarm_breadcrumbs(engine);
- spin_unlock_irqrestore(&b->lock, flags);
+ spin_unlock_irqrestore(&b->irq_lock, flags);
}
static bool use_fake_irq(const struct intel_breadcrumbs *b)
@@ -228,7 +230,7 @@ static void __intel_breadcrumbs_enable_irq(struct intel_breadcrumbs *b)
container_of(b, struct intel_engine_cs, breadcrumbs);
struct drm_i915_private *i915 = engine->i915;
- lockdep_assert_held(&b->lock);
+ lockdep_assert_held(&b->irq_lock);
if (b->irq_armed)
return;
@@ -276,7 +278,7 @@ static inline struct intel_wait *to_wait(struct rb_node *node)
static inline void __intel_breadcrumbs_finish(struct intel_breadcrumbs *b,
struct intel_wait *wait)
{
- lockdep_assert_held(&b->lock);
+ lockdep_assert_held(&b->rb_lock);
/* This request is completed, so remove it from the tree, mark it as
* complete, and *then* wake up the associated task.
@@ -292,8 +294,10 @@ static inline void __intel_breadcrumbs_next(struct intel_engine_cs *engine,
{
struct intel_breadcrumbs *b = &engine->breadcrumbs;
+ spin_lock(&b->irq_lock);
GEM_BUG_ON(!b->irq_armed);
b->first_wait = to_wait(next);
+ spin_unlock(&b->irq_lock);
/* We always wake up the next waiter that takes over as the bottom-half
* as we may delegate not only the irq-seqno barrier to the next waiter
@@ -383,6 +387,7 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
}
if (first) {
+ spin_lock(&b->irq_lock);
GEM_BUG_ON(rb_first(&b->waiters) != &wait->node);
b->first_wait = wait;
/* After assigning ourselves as the new bottom-half, we must
@@ -394,6 +399,7 @@ static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
* and so we miss the wake up.
*/
__intel_breadcrumbs_enable_irq(b);
+ spin_unlock(&b->irq_lock);
}
GEM_BUG_ON(!b->first_wait);
GEM_BUG_ON(rb_first(&b->waiters) != &b->first_wait->node);
@@ -407,9 +413,9 @@ bool intel_engine_add_wait(struct intel_engine_cs *engine,
struct intel_breadcrumbs *b = &engine->breadcrumbs;
bool first;
- spin_lock_irq(&b->lock);
+ spin_lock_irq(&b->rb_lock);
first = __intel_engine_add_wait(engine, wait);
- spin_unlock_irq(&b->lock);
+ spin_unlock_irq(&b->rb_lock);
return first;
}
@@ -433,7 +439,7 @@ static void __intel_engine_remove_wait(struct intel_engine_cs *engine,
{
struct intel_breadcrumbs *b = &engine->breadcrumbs;
- lockdep_assert_held(&b->lock);
+ lockdep_assert_held(&b->rb_lock);
if (RB_EMPTY_NODE(&wait->node))
goto out;
@@ -503,9 +509,9 @@ void intel_engine_remove_wait(struct intel_engine_cs *engine,
if (RB_EMPTY_NODE(&wait->node))
return;
- spin_lock_irq(&b->lock);
+ spin_lock_irq(&b->rb_lock);
__intel_engine_remove_wait(engine, wait);
- spin_unlock_irq(&b->lock);
+ spin_unlock_irq(&b->rb_lock);
}
static bool signal_valid(const struct drm_i915_gem_request *request)
@@ -575,7 +581,7 @@ static int intel_breadcrumbs_signaler(void *arg)
dma_fence_signal(&request->fence);
local_bh_enable(); /* kick start the tasklets */
- spin_lock_irq(&b->lock);
+ spin_lock_irq(&b->rb_lock);
/* Wake up all other completed waiters and select the
* next bottom-half for the next user interrupt.
@@ -598,7 +604,7 @@ static int intel_breadcrumbs_signaler(void *arg)
rb_erase(&request->signaling.node, &b->signals);
RB_CLEAR_NODE(&request->signaling.node);
- spin_unlock_irq(&b->lock);
+ spin_unlock_irq(&b->rb_lock);
i915_gem_request_put(request);
} else {
@@ -655,7 +661,7 @@ void intel_engine_enable_signaling(struct drm_i915_gem_request *request)
request->signaling.wait.seqno = seqno;
i915_gem_request_get(request);
- spin_lock(&b->lock);
+ spin_lock(&b->rb_lock);
/* First add ourselves into the list of waiters, but register our
* bottom-half as the signaller thread. As per usual, only the oldest
@@ -689,7 +695,7 @@ void intel_engine_enable_signaling(struct drm_i915_gem_request *request)
if (first)
rcu_assign_pointer(b->first_signal, request);
- spin_unlock(&b->lock);
+ spin_unlock(&b->rb_lock);
if (wakeup)
wake_up_process(b->signaler);
@@ -704,7 +710,7 @@ void intel_engine_cancel_signaling(struct drm_i915_gem_request *request)
lockdep_assert_held(&request->lock);
GEM_BUG_ON(!request->signaling.wait.seqno);
- spin_lock(&b->lock);
+ spin_lock(&b->rb_lock);
if (!RB_EMPTY_NODE(&request->signaling.node)) {
if (request == rcu_access_pointer(b->first_signal)) {
@@ -720,7 +726,7 @@ void intel_engine_cancel_signaling(struct drm_i915_gem_request *request)
__intel_engine_remove_wait(engine, &request->signaling.wait);
- spin_unlock(&b->lock);
+ spin_unlock(&b->rb_lock);
request->signaling.wait.seqno = 0;
}
@@ -730,7 +736,9 @@ 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);
+ spin_lock_init(&b->rb_lock);
+ spin_lock_init(&b->irq_lock);
+
setup_timer(&b->fake_irq,
intel_breadcrumbs_fake_irq,
(unsigned long)engine);
@@ -768,7 +776,7 @@ void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
struct intel_breadcrumbs *b = &engine->breadcrumbs;
cancel_fake_irq(engine);
- spin_lock_irq(&b->lock);
+ spin_lock_irq(&b->irq_lock);
if (b->irq_enabled)
irq_enable(engine);
@@ -787,7 +795,7 @@ void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
if (b->irq_armed)
enable_fake_irq(b);
- spin_unlock_irq(&b->lock);
+ spin_unlock_irq(&b->irq_lock);
}
void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine)
@@ -811,7 +819,7 @@ bool intel_breadcrumbs_busy(struct intel_engine_cs *engine)
struct intel_breadcrumbs *b = &engine->breadcrumbs;
bool busy = false;
- spin_lock_irq(&b->lock);
+ spin_lock_irq(&b->rb_lock);
if (b->first_wait) {
wake_up_process(b->first_wait->tsk);
@@ -823,7 +831,7 @@ bool intel_breadcrumbs_busy(struct intel_engine_cs *engine)
busy |= intel_engine_flag(engine);
}
- spin_unlock_irq(&b->lock);
+ spin_unlock_irq(&b->rb_lock);
return busy;
}
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 3dd6eee4a08b..2c42b3ce37b5 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -235,10 +235,12 @@ struct intel_engine_cs {
* the overhead of waking that client is much preferred.
*/
struct intel_breadcrumbs {
- spinlock_t lock; /* protects the lists of requests; irqsafe */
+ spinlock_t irq_lock; /* protects first_wait & irq_*; irqsafe */
+ struct intel_wait *first_wait; /* oldest waiter by retirement */
+
+ spinlock_t rb_lock; /* protects the rbtrees; irqsafe */
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 *signaler; /* used for fence signalling */
struct drm_i915_gem_request __rcu *first_signal;
struct timer_list fake_irq; /* used after a missed interrupt */
--
2.11.0
More information about the Intel-gfx-trybot
mailing list