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

[Tvrtko Ursulin]

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"?

> 
>> So this patch changes the meaning of "pending". From pending == "not
>> submitted to ELSP" to pending == "not submitted to ELSP[0]". Which seems
>> to make sense, although it is not the easiest job to figure out the
>> consequences.
> 
> Yes.
>   
>> It even feels like a bugfix since it prevents tasklet scheduling when
>> all ports are filled with higher priority requests than the new one.
> 
> It won't fix any bugs, since we just reduce the number of kicks. Kicking
> and evaluating we have nothing to do is just wasted work. So yes I do
> agree that it is a bug fix, just not enough to merit a Fixes.

Yeah that's fine.

>   
>> 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?

Then there is also "if (port_count(port0)) goto unlock;" suggesting that 
if there were any appends to ELSP[0] we will also fail to act in this 
situation?

> We won't evaluate preemption if we are still awaiting the HWACK from the
> last ELSP write, or if we are still preempting. In both of those cases,
> we expect to receive an interrupt promptly, upon which we then redo our
> evaluations.
> 
>> Also, don't you need to manage execlists->queue_priority after CSB
>> processing now? So that it correctly reflects the priority of request in
>> ELSP[1] after ELSP[0] gets completed? It seems that without it would get
>> stuck at the previous value and then submission could decide to skip
>> scheduling the tasklet if new priority is lower than what was in ELSP[1]
>> before, and so would fail to fill ELSP[1].
> 
> Yes, that is also done here. Since we are always looking one request
> ahead, we either update the priority based on the queue following the
> resubmission on interrupt, or it is left as INT_MIN on completion.
> Indeed, making sure we reset back to INT_MIN is essential so that we
> don't any future submissions from idle.

Okay I see it - because execlists_dequeue is called and runs to the 
queue_priority update bit even when there is nothing in the queue.

> We can add GEM_BUG_ON(queue_priority != INT_MIN) in engines_park to
> check this condition.

Looks like we don't have these hooks set for execlists so it's probably 
more hassle than it would be worth.

Regards,

Tvrtko