[Intel-gfx] [PATCH] drm/i915: Don't disable interrupts independently of the lock

[Chris Wilson]

Quoting Sebastian Andrzej Siewior (2019-10-10 19:26:10)
> On 2019-10-10 19:11:27 [+0100], Chris Wilson wrote:
> > > --- a/drivers/gpu/drm/i915/i915_request.c
> > > +++ b/drivers/gpu/drm/i915/i915_request.c
> > > @@ -251,15 +251,13 @@ static bool i915_request_retire(struct i
> > >                 active->retire(active, rq);
> > >         }
> > >  
> > > -       local_irq_disable();
> > > -
> > >         /*
> > >          * We only loosely track inflight requests across preemption,
> > >          * and so we may find ourselves attempting to retire a _completed_
> > >          * request that we have removed from the HW and put back on a run
> > >          * queue.
> > >          */
> > > -       spin_lock(&rq->engine->active.lock);
> > > +       spin_lock_irq(&rq->engine->active.lock);
> > >         list_del(&rq->sched.link);
> > >         spin_unlock(&rq->engine->active.lock);
> > >  
> > > @@ -278,9 +276,7 @@ static bool i915_request_retire(struct i
> > >                 __notify_execute_cb(rq);
> > >         }
> > >         GEM_BUG_ON(!list_empty(&rq->execute_cb));
> > > -       spin_unlock(&rq->lock);
> > > -
> > > -       local_irq_enable();
> > > +       spin_unlock_irq(&rq->lock);
> > 
> > Nothing screams about the imbalance? irq off from one lock to the other?
> 
> There is no imbalance, is there? Interrupts are disabled as part of
> acquiring the first lock and enabled again as part of releasing the
> second lock.
> It may not look beautiful. 

Sure, it's at the same scope, I just expect at some point lockdep to
complain :)
 
> I'm just not sure if this
> 
> |         spin_lock_irq(&rq->engine->active.lock);
> |         list_del(&rq->sched.link);
> |         spin_unlock_irq(&rq->engine->active.lock);
> | 
> |         spin_lock_irq(&rq->lock);
> |         i915_request_mark_complete(rq);
> …
> |         spin_unlock_irq(&rq->lock);
> 
> has been avoided because an interrupt here could change something or if
> this is just an optimisation.

Just avoiding the back-to-back enable/disable.
-Chris