Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> writes: > On Fri, Mar 03, 2017 at 07:26:34PM +0200, Mika Kuoppala wrote: >> Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> writes: >> > @@ -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); >> >> In here I was thinking that you want both locks help, but >> then can't find a reason why. Perhaps just to ensure that >> the wait tree stays still. > > Right, here we are just rearming the irq, so I didn't think taking the > rb_lock helped with clarity. (Just the opposite, this showed nicely the > divison in labour between the locks :) > >> > 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); >> >> Wrong lock taken and relased in this function? > > Here we can simply take the outer rb_lock, as that guarantees the rbtree > won't change and therefore also the first_wait/irq_wait cannot be > lost. Applied and indeed the first_wait is always modified by under rb_lock. Reviewed-by: Mika Kuoppala <mika.kuoppala@xxxxxxxxx> > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
