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

Wed Apr 11 10:47:00 UTC 2018

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].

>   
>> 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().

Tunnel vision. :)

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko