| Message ID | 20170315140107.18720-2-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 15/03/2017 14:01, Chris Wilson wrote: > Check that we have disabled irqs before we take the spin_lock around > reassigned the breadcrumbs.irq_wait. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/intel_breadcrumbs.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c > index 3f222dee4c25..35529b35a276 100644 > --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c > +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c > @@ -301,8 +301,11 @@ static inline void __intel_breadcrumbs_next(struct intel_engine_cs *engine, > { > struct intel_breadcrumbs *b = &engine->breadcrumbs; > > + GEM_BUG_ON(!irqs_disabled()); > + > spin_lock(&b->irq_lock); > GEM_BUG_ON(!b->irq_armed); > + GEM_BUG_ON(!b->irq_wait); > b->irq_wait = to_wait(next); > spin_unlock(&b->irq_lock); > > @@ -395,8 +398,10 @@ 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); > + GEM_BUG_ON(!irqs_disabled()); > + > + spin_lock(&b->irq_lock); > b->irq_wait = wait; > /* After assigning ourselves as the new bottom-half, we must > * perform a cursory check to prevent a missed interrupt. > A single GEM_BUG_ON(!irqs_disabled()) at the top of __intel_engine_add_wait might be more logical? As a weakly related side note, there is a stale comment mentioning b->lock in intel_engine_enable_signalling. Regards, Tvrtko
On Wed, Mar 15, 2017 at 06:20:16PM +0000, Tvrtko Ursulin wrote: > > On 15/03/2017 14:01, Chris Wilson wrote: > >Check that we have disabled irqs before we take the spin_lock around > >reassigned the breadcrumbs.irq_wait. > > > >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >--- > > drivers/gpu/drm/i915/intel_breadcrumbs.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > >diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c > >index 3f222dee4c25..35529b35a276 100644 > >--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c > >+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c > >@@ -301,8 +301,11 @@ static inline void __intel_breadcrumbs_next(struct intel_engine_cs *engine, > > { > > struct intel_breadcrumbs *b = &engine->breadcrumbs; > > > >+ GEM_BUG_ON(!irqs_disabled()); > >+ > > spin_lock(&b->irq_lock); > > GEM_BUG_ON(!b->irq_armed); > >+ GEM_BUG_ON(!b->irq_wait); > > b->irq_wait = to_wait(next); > > spin_unlock(&b->irq_lock); > > > >@@ -395,8 +398,10 @@ 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); > >+ GEM_BUG_ON(!irqs_disabled()); > >+ > >+ spin_lock(&b->irq_lock); > > b->irq_wait = wait; > > /* After assigning ourselves as the new bottom-half, we must > > * perform a cursory check to prevent a missed interrupt. > > > > A single GEM_BUG_ON(!irqs_disabled()) at the top of > __intel_engine_add_wait might be more logical? I wanted to associate it with b->irq_lock, that was my thinking. b->rb_lock also sadly has to be irqsafe. __intel_breadcrumbs_next() also serves remove_wait, did you mean to remove the assert there as well? We can safely ignore this patch, it should be catered by lockdep fairly well, I was just being paranoid and going through the possible causes and documenting my progress. > As a weakly related side note, there is a stale comment mentioning > b->lock in intel_engine_enable_signalling. Ta. -Chris
On 15/03/2017 18:32, Chris Wilson wrote: > On Wed, Mar 15, 2017 at 06:20:16PM +0000, Tvrtko Ursulin wrote: >> >> On 15/03/2017 14:01, Chris Wilson wrote: >>> Check that we have disabled irqs before we take the spin_lock around >>> reassigned the breadcrumbs.irq_wait. >>> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_breadcrumbs.c | 7 ++++++- >>> 1 file changed, 6 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c >>> index 3f222dee4c25..35529b35a276 100644 >>> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c >>> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c >>> @@ -301,8 +301,11 @@ static inline void __intel_breadcrumbs_next(struct intel_engine_cs *engine, >>> { >>> struct intel_breadcrumbs *b = &engine->breadcrumbs; >>> >>> + GEM_BUG_ON(!irqs_disabled()); >>> + >>> spin_lock(&b->irq_lock); >>> GEM_BUG_ON(!b->irq_armed); >>> + GEM_BUG_ON(!b->irq_wait); >>> b->irq_wait = to_wait(next); >>> spin_unlock(&b->irq_lock); >>> >>> @@ -395,8 +398,10 @@ 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); >>> + GEM_BUG_ON(!irqs_disabled()); >>> + >>> + spin_lock(&b->irq_lock); >>> b->irq_wait = wait; >>> /* After assigning ourselves as the new bottom-half, we must >>> * perform a cursory check to prevent a missed interrupt. >>> >> >> A single GEM_BUG_ON(!irqs_disabled()) at the top of >> __intel_engine_add_wait might be more logical? > > I wanted to associate it with b->irq_lock, that was my thinking. > b->rb_lock also sadly has to be irqsafe. That makes sense yes. > __intel_breadcrumbs_next() also serves remove_wait, did you mean to > remove the assert there as well? Yes, I was thinking only one would do it. > We can safely ignore this patch, it should be catered by lockdep fairly > well, I was just being paranoid and going through the possible causes > and documenting my progress. This also makes sense. I think it's the best option. Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c index 3f222dee4c25..35529b35a276 100644 --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c @@ -301,8 +301,11 @@ static inline void __intel_breadcrumbs_next(struct intel_engine_cs *engine, { struct intel_breadcrumbs *b = &engine->breadcrumbs; + GEM_BUG_ON(!irqs_disabled()); + spin_lock(&b->irq_lock); GEM_BUG_ON(!b->irq_armed); + GEM_BUG_ON(!b->irq_wait); b->irq_wait = to_wait(next); spin_unlock(&b->irq_lock); @@ -395,8 +398,10 @@ 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); + GEM_BUG_ON(!irqs_disabled()); + + spin_lock(&b->irq_lock); b->irq_wait = wait; /* After assigning ourselves as the new bottom-half, we must * perform a cursory check to prevent a missed interrupt.
Check that we have disabled irqs before we take the spin_lock around reassigned the breadcrumbs.irq_wait. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- drivers/gpu/drm/i915/intel_breadcrumbs.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)