Re: [PATCH v2 2/3] drm/i915: Split breadcrumbs spinlock into two

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> On Fri, Mar 03, 2017 at 07:26:34PM +0200, Mika Kuoppala wrote:
>> Chris Wilson <chris@chris-wilson.co.uk> 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@intel.com>

> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx