[Intel-gfx] [PATCH 01/18] drm/i915/execlists: Set queue priority from secondary port

Wed Apr 11 10:36:07 UTC 2018

Quoting Tvrtko Ursulin (2018-04-11 11:23:01)
> 
> On 10/04/2018 15:56, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2018-04-10 15:42:07)
> >>
> >> On 10/04/2018 15:24, Chris Wilson wrote:
> >>> Quoting Tvrtko Ursulin (2018-04-10 15:05:33)
> >>>>
> >>>> On 09/04/2018 12:13, Chris Wilson wrote:
> >>>>> We can refine our current execlists->queue_priority if we inspect
> >>>>> ELSP[1] rather than the head of the unsubmitted queue. Currently, we use
> >>>>> the unsubmitted queue and say that if a subsequent request is more than
> >>>>> important than the current queue, we will rerun the submission tasklet
> >>>>
> >>>> s/more than important/more important/
> >>>>
> >>>>> to evaluate the need for preemption. However, we only want to preempt if
> >>>>> we need to jump ahead of a currently executing request in ELSP. The
> >>>>> second reason for running the submission tasklet is amalgamate requests
> >>>>> into the active context on ELSP[0] to avoid a stall when ELSP[0] drains.
> >>>>> (Though repeatedly amalgamating requests into the active context and
> >>>>> triggering many lite-restore is off question gain, the goal really is to
> >>>>> put a context into ELSP[1] to cover the interrupt.) So if instead of
> >>>>> looking at the head of the queue, we look at the context in ELSP[1] we
> >>>>> can answer both of the questions more accurately -- we don't need to
> >>>>> rerun the submission tasklet unless our new request is important enough
> >>>>> to feed into, at least, ELSP[1].
> >>>>>
> >>>>> References: f6322eddaff7 ("drm/i915/preemption: Allow preemption between submission ports")
> >>>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> >>>>> Cc: Michał Winiarski <michal.winiarski at intel.com>
> >>>>> Cc: Michel Thierry <michel.thierry at intel.com>
> >>>>> Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> >>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>>>> ---
> >>>>>     drivers/gpu/drm/i915/intel_lrc.c | 3 ++-
> >>>>>     1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> >>>>> index 3592288e4696..b47d53d284ca 100644
> >>>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
> >>>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> >>>>> @@ -713,7 +713,8 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
> >>>>>                         kmem_cache_free(engine->i915->priorities, p);
> >>>>>         }
> >>>>>     done:
> >>>>> -     execlists->queue_priority = rb ? to_priolist(rb)->priority : INT_MIN;
> >>>>> +     execlists->queue_priority =
> >>>>> +             port != execlists->port ? rq_prio(last) : INT_MIN;
> >>>>
> >>>> Please bear with my questions since I am not 100% up to date with
> >>>> preemption.
> >>>>
> >>>> Should this be rq_prio("port[1]") for future proofing? Or you really
> >>>> mean last port? In which case it wouldn't be the highest pending
> >>>> priority as kerneldoc for execlists->queue_priority says.
> >>>
> >>> I mean "secondary" port, so yes using last executing port under the
> >>> assumption that we grow into a ring of many useless submission ports.
> >>> The kerneldoc is no more or no less accurate. :)
> >>
> >> "That we _don't_ grow"?
> > 
> > Hmm, no, when we get "last_port" it slots right into here. I just don't
> > have the future facing code to prevent Mika from having to think a
> > little. The intent is that when there is a ELSP slot available,
> > queue_priority is INT_MIN, when there are none, then rq_prio(last).
> > 
> > My bad for remembering what I want the code to be without remembering
> > what the code says.
> >   
> >>>> Although I failed to understand what do we do in both cases if a new
> >>>> request arrives of higher prio than the one in ELSP[1]. Looks like
> >>>> nothing? Wait until GPU moves it to ELSP[0] and preempt then? Is this so
> >>>> because we can't safely or I misread something?
> >>>
> >>> This is covered by igt/gem_exec_schedule/preempt-queue*. If we receive a
> >>> request higher than ELSP[1], we start a preemption as
> >>>
> >>>        if (need_preempt(engine, last, execlists->queue_priority)) {
> >>>
> >>> will evaluate to true. It's either the lowest executing priority (new
> >>> code), or lowest pending priority (old code). In either case, if the new
> >>> request is more important than the queue_priority, we preempt.
> >>
> >> How when "last" is request from ELSP[0]? And also
> >> execlists->queue_priority has not yet been updated to reflect the new
> >> priority?
> > 
> > When we start executing last on ELSP[0] there will have been another
> > execlists_dequeue() where we see an empty slot (or fill it) and update
> > queue_priority. If we are down to the last request, it will be set to
> > INT_MIN. Upon its completion, it will remain INT_MIN.
> 
> I don't see it yet, let me walk through it:
> 
> 0. Initial situation, GPU busy with two requests, no outstanding ones:
> 
> ELSP[0] = prio 2
> ELSP[1] = prio 0
> 
> 1. queue_priority = 0
> 2. New execbuf comes along with prio=1.
> 3. We choose to schedule the tasklet - good.
> 4. execlists_dequeue runs
> 
> last = prio 2
> 
> if (need_preempt(engine, last, execlists->queue_priority))
> 
> queue_priority = 0, so will not preempt last which is prio 2 - so no 
> preemption - good.
> 
> queue_priority remains at zero since we "goto unlock" with both ports 
> busy and no preemption is triggered.
> 
> 5. ELSP[0] completes, new ELSP[0] with prio 0.
> 
> (Before we missed the opportunity to replace ELSP[1] with higher prio 1 
> waiting request before ELSP[0] completed - perhaps we have no choice? 
> But ok.. carrying on..)

We don't want to interrupt the higher priority task in ELSP[0] to sort
out ELSP[1].

> 6. execlist_dequeue
> 
> lasts = prio 0
> 
> if (need_preempt(engine, last, execlists->queue_priority))
> 
> queue_priority = 0, so again preemption not triggered.

queue_priority is 1 from queue_request().
-Chris