[Intel-gfx] [PATCH] drm/i915: Convert breadcrumbs spinlock to be irqsafe
Tvrtko Ursulin
tursulin at ursulin.net
Fri Oct 28 09:42:22 UTC 2016
On 27/10/16 17:10, Chris Wilson wrote:
> The breadcrumbs are about to be used from within IRQ context sections,
> therefore we need to employ the irqsafe spinlock variants.
>
> (This is split out of the defer global seqno allocation patch due to
> realisation that we need a more complete conversion if we want to defer
> request submission even further.)
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 12 +++++------
> drivers/gpu/drm/i915/intel_breadcrumbs.c | 34 +++++++++++++++++++-------------
> 2 files changed, 26 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 925aaedd7fd9..8d3cb8f5ff41 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -683,14 +683,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(&b->lock);
> + spin_lock_irq(&b->lock);
> for (rb = rb_first(&b->waiters); rb; rb = rb_next(rb)) {
> struct intel_wait *w = container_of(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(&b->lock);
> + spin_unlock_irq(&b->lock);
> }
>
> static int i915_gem_seqno_info(struct seq_file *m, void *data)
> @@ -1355,14 +1355,14 @@ static int i915_hangcheck_info(struct seq_file *m, void *unused)
> yesno(intel_engine_has_waiter(engine)),
> yesno(test_bit(engine->id,
> &dev_priv->gpu_error.missed_irq_rings)));
> - spin_lock(&b->lock);
> + spin_lock_irq(&b->lock);
> for (rb = rb_first(&b->waiters); rb; rb = rb_next(rb)) {
> struct intel_wait *w = container_of(rb, typeof(*w), node);
>
> seq_printf(m, "\t%s [%d] waiting for %x\n",
> w->tsk->comm, w->tsk->pid, w->seqno);
> }
> - spin_unlock(&b->lock);
> + spin_unlock_irq(&b->lock);
>
> seq_printf(m, "\tACTHD = 0x%08llx [current 0x%08llx]\n",
> (long long)engine->hangcheck.acthd,
> @@ -3265,14 +3265,14 @@ static int i915_engine_info(struct seq_file *m, void *unused)
> I915_READ(RING_PP_DIR_DCLV(engine)));
> }
>
> - spin_lock(&b->lock);
> + spin_lock_irq(&b->lock);
> for (rb = rb_first(&b->waiters); rb; rb = rb_next(rb)) {
> struct intel_wait *w = container_of(rb, typeof(*w), node);
>
> seq_printf(m, "\t%s [%d] waiting for %x\n",
> w->tsk->comm, w->tsk->pid, w->seqno);
> }
> - spin_unlock(&b->lock);
> + spin_unlock_irq(&b->lock);
>
> seq_puts(m, "\n");
> }
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index 0d5def0d2dfe..00b25a7cef95 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -83,16 +83,16 @@ static void irq_enable(struct intel_engine_cs *engine)
> */
> engine->breadcrumbs.irq_posted = true;
>
> - spin_lock_irq(&engine->i915->irq_lock);
> + spin_lock(&engine->i915->irq_lock);
> engine->irq_enable(engine);
> - spin_unlock_irq(&engine->i915->irq_lock);
> + spin_unlock(&engine->i915->irq_lock);
> }
>
> static void irq_disable(struct intel_engine_cs *engine)
> {
> - spin_lock_irq(&engine->i915->irq_lock);
> + spin_lock(&engine->i915->irq_lock);
> engine->irq_disable(engine);
> - spin_unlock_irq(&engine->i915->irq_lock);
> + spin_unlock(&engine->i915->irq_lock);
>
> engine->breadcrumbs.irq_posted = false;
> }
> @@ -291,11 +291,12 @@ bool intel_engine_add_wait(struct intel_engine_cs *engine,
> struct intel_wait *wait)
> {
> struct intel_breadcrumbs *b = &engine->breadcrumbs;
> + unsigned long flags;
> bool first;
>
> - spin_lock(&b->lock);
> + spin_lock_irqsave(&b->lock, flags);
It looks to be called from thread context only so _irq would do it.
> first = __intel_engine_add_wait(engine, wait);
> - spin_unlock(&b->lock);
> + spin_unlock_irqrestore(&b->lock, flags);
>
> return first;
> }
> @@ -318,6 +319,7 @@ void intel_engine_remove_wait(struct intel_engine_cs *engine,
> struct intel_wait *wait)
> {
> struct intel_breadcrumbs *b = &engine->breadcrumbs;
> + unsigned long flags;
>
> /* Quick check to see if this waiter was already decoupled from
> * the tree by the bottom-half to avoid contention on the spinlock
> @@ -326,7 +328,7 @@ void intel_engine_remove_wait(struct intel_engine_cs *engine,
> if (RB_EMPTY_NODE(&wait->node))
> return;
>
> - spin_lock(&b->lock);
> + spin_lock_irqsave(&b->lock, flags);
Same.
>
> if (RB_EMPTY_NODE(&wait->node))
> goto out_unlock;
> @@ -400,7 +402,7 @@ void intel_engine_remove_wait(struct intel_engine_cs *engine,
> GEM_BUG_ON(rb_first(&b->waiters) !=
> (b->first_wait ? &b->first_wait->node : NULL));
> GEM_BUG_ON(!rcu_access_pointer(b->irq_seqno_bh) ^ RB_EMPTY_ROOT(&b->waiters));
> - spin_unlock(&b->lock);
> + spin_unlock_irqrestore(&b->lock, flags);
> }
>
> static bool signal_complete(struct drm_i915_gem_request *request)
> @@ -457,6 +459,8 @@ static int intel_breadcrumbs_signaler(void *arg)
> */
> request = READ_ONCE(b->first_signal);
> if (signal_complete(request)) {
> + unsigned long flags;
> +
> /* Wake up all other completed waiters and select the
> * next bottom-half for the next user interrupt.
> */
> @@ -473,14 +477,14 @@ static int intel_breadcrumbs_signaler(void *arg)
> * we just completed - so double check we are still
> * the oldest before picking the next one.
> */
> - spin_lock(&b->lock);
> + spin_lock_irqsave(&b->lock, flags);
This is a thread so _irq.
> if (request == b->first_signal) {
> struct rb_node *rb =
> rb_next(&request->signaling.node);
> b->first_signal = rb ? to_signaler(rb) : NULL;
> }
> rb_erase(&request->signaling.node, &b->signals);
> - spin_unlock(&b->lock);
> + spin_unlock_irqrestore(&b->lock, flags);
>
> i915_gem_request_put(request);
> } else {
> @@ -500,6 +504,7 @@ void 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;
> + unsigned long flags;
> bool first, wakeup;
>
> /* locked by dma_fence_enable_sw_signaling() */
> @@ -511,7 +516,7 @@ void intel_engine_enable_signaling(struct drm_i915_gem_request *request)
> request->signaling.wait.seqno = request->global_seqno;
> i915_gem_request_get(request);
>
> - spin_lock(&b->lock);
> + spin_lock_irqsave(&b->lock, flags);
>
> /* First add ourselves into the list of waiters, but register our
> * bottom-half as the signaller thread. As per usual, only the oldest
> @@ -545,7 +550,7 @@ void intel_engine_enable_signaling(struct drm_i915_gem_request *request)
> if (first)
> smp_store_mb(b->first_signal, request);
>
> - spin_unlock(&b->lock);
> + spin_unlock_irqrestore(&b->lock, flags);
>
> if (wakeup)
> wake_up_process(b->signaler);
> @@ -592,9 +597,10 @@ static void cancel_fake_irq(struct intel_engine_cs *engine)
> void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
> {
> struct intel_breadcrumbs *b = &engine->breadcrumbs;
> + unsigned long flags;
>
> cancel_fake_irq(engine);
> - spin_lock(&b->lock);
> + spin_lock_irqsave(&b->lock, flags);
Called from engine->init only so also the same.
>
> __intel_breadcrumbs_disable_irq(b);
> if (intel_engine_has_waiter(engine)) {
> @@ -607,7 +613,7 @@ void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
> irq_disable(engine);
> }
>
> - spin_unlock(&b->lock);
> + spin_unlock_irqrestore(&b->lock, flags);
> }
>
> void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine)
>
Assuming I got the above right and you agree to change it:
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
Regards,
Tvrtko
More information about the Intel-gfx
mailing list