Re: [Intel-gfx] [PATCH] gpu/drm/i915: disable interrupt when holding spinlock
- Chris Wilson 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
> > ---
> > 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] gpu/drm/i915: disable interrupt when holding spinlock
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
> ---
> 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.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] gpu/drm/i915: disable interrupt when holding spinlock
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
---
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);
}
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);
}
static void execlists_dequeue(struct intel_engine_cs *engine)
--
2.11.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
