----- Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote:
> Quoting Wang Xiayang (2019-08-07 15:54:37)
> > The irq_lock is acquired in multiple functions:
> >
> > 1) i915_request_cancel_breadcrumb
> > <- ... <- panfrost_gpu_irq_handler
> > 2) intel_engine_breadcrumbs_irq
> > <- ... <- cherryview_irq_handler
> > 3) i915_request_enable_breadcrumb
> > 4) virtual_xfer_breadcrumbs
> >
> > The former two functions are reachable from IRQ handlers while
> > the latter two functions are not, and they call spin_lock()
> > which do not disable interrupt. Being preempted by an interrupt
> > acquiring the same lock may lead to deadlock.
> > Other functions acquire irq_lock by spin_lock_irq/irqsave().
> >
> > This patch switches spin_lock() to spin_lock_irq in the two
> > process-context functions.
> >
> > The issue is identified by a static analyzer based on Coccinelle.
> >
> > Signed-off-by: Wang Xiayang <xywang.sjtu@xxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 4 ++--
> > drivers/gpu/drm/i915/gt/intel_lrc.c | 4 ++--
> > 2 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> > index c092bdf5f0bf..e0b46450c2f5 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> > @@ -301,7 +301,7 @@ bool i915_request_enable_breadcrumb(struct i915_request *rq)
> > struct intel_context *ce = rq->hw_context;
> > struct list_head *pos;
> >
> > - spin_lock(&b->irq_lock);
> > + spin_lock_irq(&b->irq_lock);
> > GEM_BUG_ON(test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags));
> >
> > __intel_breadcrumbs_arm_irq(b);
> > @@ -333,7 +333,7 @@ bool i915_request_enable_breadcrumb(struct i915_request *rq)
> > GEM_BUG_ON(!check_signal_order(ce, rq));
> >
> > set_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
> > - spin_unlock(&b->irq_lock);
> > + spin_unlock_irq(&b->irq_lock);
>
> This is very broken, irqs are disabled by the caller and you can't
> unconditionally enable them again here...
>
> > return !__request_completed(rq);
> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > index 82b7ace62d97..42367aeefcce 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > @@ -806,13 +806,13 @@ static void virtual_xfer_breadcrumbs(struct virtual_engine *ve,
> >
> > /* All unattached (rq->engine == old) must already be completed */
> >
> > - spin_lock(&old->breadcrumbs.irq_lock);
> > + spin_lock_irq(&old->breadcrumbs.irq_lock);
> > if (!list_empty(&ve->context.signal_link)) {
> > list_move_tail(&ve->context.signal_link,
> > &engine->breadcrumbs.signalers);
> > intel_engine_queue_breadcrumbs(engine);
> > }
> > - spin_unlock(&old->breadcrumbs.irq_lock);
> > + spin_unlock_irq(&old->breadcrumbs.irq_lock);
>
> Or here.
I see. Deeply sorry for the false alarming.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
