[Intel-gfx] [PATCH] drm/i915: Convert breadcrumbs spinlock to be irqsafe

[Chris Wilson]

On Fri, Oct 28, 2016 at 12:30:55PM +0100, Chris Wilson wrote:
> On Fri, Oct 28, 2016 at 12:26:09PM +0100, Chris Wilson wrote:
> > On Fri, Oct 28, 2016 at 11:49:34AM +0100, Tvrtko Ursulin wrote:
> > > 
> > > 
> > > On 28/10/16 11:42, Chris Wilson wrote:
> > > >On Fri, Oct 28, 2016 at 11:27:43AM +0100, Tvrtko Ursulin wrote:
> > > >>
> > > >>
> > > >>On 28/10/16 11:10, Chris Wilson wrote:
> > > >>>On Fri, Oct 28, 2016 at 10:42:22AM +0100, Tvrtko Ursulin wrote:
> > > >>>>
> > > >>>>
> > > >>>>On 27/10/16 17:10, Chris Wilson wrote:
> > > >>>>>The breadcrumbs are about to be used from within IRQ context sections,
> > > >>>>>therefore we need to employ the irqsafe spinlock variants.
> > > >>>>>
> > > >>>>>(This is split out of the defer global seqno allocation patch due to
> > > >>>>>realisation that we need a more complete conversion if we want to defer
> > > >>>>>request submission even further.)
> > > >>>
> > > >>>[snip]
> > > >>>
> > > >>>>Assuming I got the above right and you agree to change it:
> > > >>>
> > > >>>You made me go and reduce them to _bh as appropriate anyway!!!
> > > >>
> > > >>Hm, but can't enable signalling be called with irqs disabled when
> > > >>fences are exported?
> > > >
> > > >Yes, but that supercedes the spin_lock_bh, so we can just call
> > > >spin_lock() in enabling_signaling as we can assert that we will always
> > > >be called with irqs disabled here (due to spin_lock_irqsafe(fence->lock)
> > > >in the callpath).
> > > 
> > > But as long as the b->lock is taken in the irqs disabled section
> > > somewhere, other callers like signaller thread, debugfs, etc, cannot
> > > just take it with _bh.
> > 
> > Lockdep doesn't complain, if we take spin_lock(b->lock) under irq inside our
> > tasklet (enable_signaling)  so long as we use spin_lock_bh() elsewhere.
> 
> The key part is that we never take the spin_lock in irq context, just I
> am introducing it into softirq context. Hence why, I think, lockdep is
> happy.

However, this doesn't take into account that submit_notify() may be
called from interrupt context from nouveau, and that may then use
enable_signaling, and so we may take b->lock in irqcontext after all.

Ok, this needs a fixes tag.
-Chris
-- 
Chris Wilson, Intel Open Source Technology Centre