Quoting Chris Wilson (2018-05-08 21:59:20)
> Quoting Tvrtko Ursulin (2018-05-08 18:45:44)
> >
> > On 07/05/2018 14:57, Chris Wilson wrote:
> > > Prepare to allow the execlists submission to be run from underneath a
> > > hardirq timer context (and not just the current softirq context) as is
> > > required for fast preemption resets and context switches.
> > >
> > > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/intel_lrc.c | 42 ++++++++++++++++++++++----------
> > > 1 file changed, 29 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > > index f9f4064dec0e..15c373ea5b7e 100644
> > > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > > @@ -357,10 +357,13 @@ execlists_unwind_incomplete_requests(struct intel_engine_execlists *execlists)
> > > {
> > > struct intel_engine_cs *engine =
> > > container_of(execlists, typeof(*engine), execlists);
> > > + unsigned long flags;
> > > +
> > > + spin_lock_irqsave(&engine->timeline.lock, flags);
> > >
> > > - spin_lock_irq(&engine->timeline.lock);
> > > __unwind_incomplete_requests(engine);
> > > - spin_unlock_irq(&engine->timeline.lock);
> > > +
> > > + spin_unlock_irqrestore(&engine->timeline.lock, flags);
> > > }
> > >
> > > static inline void
> > > @@ -554,7 +557,7 @@ static void inject_preempt_context(struct intel_engine_cs *engine)
> > > execlists_set_active(&engine->execlists, EXECLISTS_ACTIVE_PREEMPT);
> > > }
> > >
> > > -static void execlists_dequeue(struct intel_engine_cs *engine)
> > > +static bool __execlists_dequeue(struct intel_engine_cs *engine)
> > > {
> > > struct intel_engine_execlists * const execlists = &engine->execlists;
> > > struct execlist_port *port = execlists->port;
> > > @@ -564,6 +567,8 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
> > > struct rb_node *rb;
> > > bool submit = false;
> > >
> > > + lockdep_assert_held(&engine->timeline.lock);
> > > +
> > > /* Hardware submission is through 2 ports. Conceptually each port
> > > * has a (RING_START, RING_HEAD, RING_TAIL) tuple. RING_START is
> > > * static for a context, and unique to each, so we only execute
> > > @@ -585,7 +590,6 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
> > > * and context switches) submission.
> > > */
> > >
> > > - spin_lock_irq(&engine->timeline.lock);
> > > rb = execlists->first;
> > > GEM_BUG_ON(rb_first(&execlists->queue) != rb);
> > >
> > > @@ -600,7 +604,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
> > > EXECLISTS_ACTIVE_USER));
> > > GEM_BUG_ON(!port_count(&port[0]));
> > > if (port_count(&port[0]) > 1)
> > > - goto unlock;
> > > + return false;
> > >
> > > /*
> > > * If we write to ELSP a second time before the HW has had
> > > @@ -610,11 +614,11 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
> > > * the HW to indicate that it has had a chance to respond.
> > > */
> > > if (!execlists_is_active(execlists, EXECLISTS_ACTIVE_HWACK))
> > > - goto unlock;
> > > + return false;
> > >
> > > if (need_preempt(engine, last, execlists->queue_priority)) {
> > > inject_preempt_context(engine);
> > > - goto unlock;
> > > + return false;
> > > }
> > >
> > > /*
> > > @@ -639,7 +643,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
> > > * priorities of the ports haven't been switch.
> > > */
> > > if (port_count(&port[1]))
> > > - goto unlock;
> > > + return false;
> > >
> > > /*
> > > * WaIdleLiteRestore:bdw,skl
> > > @@ -744,13 +748,25 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
> > > /* We must always keep the beast fed if we have work piled up */
> > > GEM_BUG_ON(execlists->first && !port_isset(execlists->port));
> > >
> > > -unlock:
> > > - spin_unlock_irq(&engine->timeline.lock);
> > > -
> > > - if (submit) {
> > > + /* Re-evaluate the executing context setup after each preemptive kick */
> > > + if (last)
> > > execlists_user_begin(execlists, execlists->port);
> > > +
> > > + return submit;
> > > +}
> > > +
> > > +static void execlists_dequeue(struct intel_engine_cs *engine)
> > > +{
> > > + struct intel_engine_execlists * const execlists = &engine->execlists;
> > > + unsigned long flags;
> > > + bool submit;
> > > +
> > > + spin_lock_irqsave(&engine->timeline.lock, flags);
> > > + submit = __execlists_dequeue(engine);
> > > + spin_unlock_irqrestore(&engine->timeline.lock, flags);
> > > +
> > > + if (submit)
> > > execlists_submit_ports(engine);
> > > - }
> >
> > Actually, having read the guc version, why doesn't
> > execlists_submit_ports need to be hardirq safe?
>
> execlists->port[] and the ESLP register are guarded by the tasklet (they
> are only accessed from inside the tasklet). guc caught me off guard
> because it uses a shared wq (and spinlock) for all tasklets. So guc
> requires extending the irq-off section across the shared wq.
I took your r-b and ran with it. The question you asked was exactly the
puzzle I few into with the guc, so I believe we're on the same page :)
Pushed the irqsafe patches, as they are useful for a couple of series.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
