Chris Wilson <ch...@chris-wilson.co.uk> writes: > Quoting Mika Kuoppala (2018-03-02 15:50:53) >> Chris Wilson <ch...@chris-wilson.co.uk> writes: >> >> > During reset/wedging, we have to clean up the requests on the timeline >> > and flush the pending interrupt state. Currently, we are abusing the irq >> > disabling of the timeline spinlock to protect the irq state in >> > conjunction to the engine's timeline requests, but this is accidental >> > and conflates the spinlock with the irq state. A baffling state of >> > affairs for the reader. >> > >> > Instead, explicitly disable irqs over the critical section, and separate >> > modifying the irq state from the timeline's requests. >> > >> > Suggested-by: Mika Kuoppala <mika.kuopp...@linux.intel.com> >> > Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk> >> > Cc: Mika Kuoppala <mika.kuopp...@linux.intel.com> >> > Cc: Michel Thierry <michel.thie...@intel.com> >> > --- >> > drivers/gpu/drm/i915/intel_lrc.c | 21 +++++++++++++++------ >> > 1 file changed, 15 insertions(+), 6 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c >> > b/drivers/gpu/drm/i915/intel_lrc.c >> > index 0482e54c94f0..7d1109aceabb 100644 >> > --- a/drivers/gpu/drm/i915/intel_lrc.c >> > +++ b/drivers/gpu/drm/i915/intel_lrc.c >> > @@ -689,11 +689,13 @@ static void execlists_cancel_requests(struct >> > intel_engine_cs *engine) >> > >> > GEM_TRACE("%s\n", engine->name); >> > >> > - spin_lock_irqsave(&engine->timeline->lock, flags); >> > + local_irq_save(flags); >> >> Chris explained in irc that this is for lockdep only. It was a bit >> confusing as we already are guaranteed exclusive access to >> state by tasklet being killed and dead at this point. >> >> I think this warrants a comment that this is to soothe lockdep. > > /* > * Before we call engine->cancel_requests(), we should have exclusive > * access to the submission state. This is arranged for us by the caller > * disabling the interrupt generation, the tasklet and other threads > * that may then access the same state, giving us a free hand to > * reset state. However, we still need to let lockdep be aware that > * we know this state may be accessed in hardirq context, so we > * disable the irq around this manipulation and we want to keep > * the spinlock focused on its duties and not accidentally conflate > * coverage to the submission's irq state. (Similarly, although we > * shouldn't need to disable irq around the manipulation of the > * submission's irq state, we also wish to remind ourselves that > * it is irq state.) > */ > >> > >> > /* Cancel the requests on the HW and clear the ELSP tracker. */ >> > execlists_cancel_port_requests(execlists); >> > >> > + spin_lock(&engine->timeline->lock); >> > @@ -1618,10 +1622,11 @@ static void reset_common_ring(struct >> > intel_engine_cs *engine, >> > GEM_TRACE("%s seqno=%x\n", >> > engine->name, request ? request->global_seqno : 0); >> > >> > - spin_lock_irqsave(&engine->timeline->lock, flags); > > /* See execlists_cancel_requests() for the irq/spinlock split. */ >> > + local_irq_save(flags); > > Good?
Much more than I bargained for. Excellent! -Mika _______________________________________________ Intel-gfx mailing list Intelfirstname.lastname@example.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx