Re: [PATCH 2/3] drm/i915/execlists: Minimalistic timeslicing

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Quoting Mika Kuoppala (2019-06-20 14:51:24)
>> > +static void
>> > +defer_request(struct i915_request * const rq, struct list_head * const pl)
>> > +{
>> > +     struct i915_dependency *p;
>> > +
>> > +     /*
>> > +      * We want to move the interrupted request to the back of
>> > +      * the round-robin list (i.e. its priority level), but
>> > +      * in doing so, we must then move all requests that were in
>> > +      * flight and were waiting for the interrupted request to
>> > +      * be run after it again.
>> > +      */
>> > +     list_move_tail(&rq->sched.link, pl);
>> > +
>> > +     list_for_each_entry(p, &rq->sched.waiters_list, wait_link) {
>> > +             struct i915_request *w =
>> > +                     container_of(p->waiter, typeof(*w), sched);
>> > +
>> > +             /* Leave semaphores spinning on the other engines */
>> > +             if (w->engine != rq->engine)
>> > +                     continue;
>> > +
>> > +             /* No waiter should start before the active request completed */
>> > +             GEM_BUG_ON(i915_request_started(w));
>> > +
>> > +             GEM_BUG_ON(rq_prio(w) > rq_prio(rq));
>> > +             if (rq_prio(w) < rq_prio(rq))
>> > +                     continue;
>> > +
>> > +             if (list_empty(&w->sched.link))
>> > +                     continue; /* Not yet submitted; unready */
>> > +
>> > +             /*
>> > +              * This should be very shallow as it is limited by the
>> > +              * number of requests that can fit in a ring (<64) and
>> 
>> s/and/or ?
>
> I think "and" works better as each context has their own ring, so it's a
> multiplicative effect.
>

I jumped. But got clarity on irc that this are the contexts in flight.

>> > +              * the number of contexts that can be in flight on this
>> > +              * engine.
>> > +              */
>> > +             defer_request(w, pl);
>> 
>> So the stack frame will be 64*(3*8 + preample/return) at worst case?
>> can be over 2k
>
> Ok, that makes it sound scary -- but we are well within the 8k irq
> limit. (Even interrupts now have 2 pages iirc, but even at 4k we are
> well within bounds.)
>

Should be safe.

>> > @@ -906,6 +982,27 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>> >                        */
>> >                       last->hw_context->lrc_desc |= CTX_DESC_FORCE_RESTORE;
>> >                       last = NULL;
>> > +             } else if (need_timeslice(engine, last) &&
>> > +                        !timer_pending(&engine->execlists.timer)) {
>> > +                     GEM_TRACE("%s: expired last=%llx:%lld, prio=%d, hint=%d\n",
>> > +                               engine->name,
>> > +                               last->fence.context,
>> > +                               last->fence.seqno,
>> > +                               last->sched.attr.priority,
>> > +                               execlists->queue_priority_hint);
>> > +
>> > +                     ring_pause(engine) = 1;
>> > +                     defer_active(engine);
>> > +
>> > +                     /*
>> > +                      * Unlike for preemption, if we rewind and continue
>> > +                      * executing the same context as previously active,
>> > +                      * the order of execution will remain the same and
>> > +                      * the tail will only advance. We do not need to
>> > +                      * force a full context restore, as a lite-restore
>> > +                      * is sufficient to resample the monotonic TAIL.
>> > +                      */
>> 
>> I would have asked about the force preemption without this fine comment.
>> 
>> But this is a similar as the other kind of preemption. So what happens
>> when the contexts are not the same?
>
> It's just a normal preemption event. The old ring regs are saved and we
> don't try and scribble over them. Any future use of the old context will
> have the same RING_TAIL as before or later (new request) so we will
> never try to program a backwards step.

Ok,

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx