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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
