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

Wed Apr 11 11:33:39 UTC 2018

Quoting Tvrtko Ursulin (2018-04-11 11:47:00)
> 
> On 11/04/2018 11:36, Chris Wilson wrote:
> > 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].
> 
> I'll assume that means no safe way to just replace ELSP[1] without 
> preempting ELSP[0].

It easily doesn't fall out of the current tracking as we are not expecting
a lite-restore on ELSP[1], and would need to shadow the existing pair of
ELSP as well as the new pair, then figure out just which one we
preempted. It didn't look pretty, but I did try that approach early on
while trying to avoid the preempt-to-idle. (When the dust finally
settles, we should give that another go but it's probably already moot
if gen11 has zero extra preempt latency...
-Chris