[Intel-gfx] [PATCH 4/4] drm/i915/gt: Remove timeslice suppression

[Tvrtko Ursulin]

On 07/01/2021 10:27, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2021-01-07 10:16:57)
>>
>> On 06/01/2021 16:08, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2021-01-06 15:57:49)
>>
>> [snip]
>>
>>>>> @@ -1363,16 +1336,16 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>>>>>                         __unwind_incomplete_requests(engine);
>>>>>     
>>>>>                         last = NULL;
>>>>> -             } else if (need_timeslice(engine, last) &&
>>>>> -                        timeslice_expired(execlists, last)) {
>>>>> +             } else if (timeslice_expired(engine, last)) {
>>>>>                         ENGINE_TRACE(engine,
>>>>> -                                  "expired last=%llx:%lld, prio=%d, hint=%d, yield?=%s\n",
>>>>> -                                  last->fence.context,
>>>>> -                                  last->fence.seqno,
>>>>> -                                  last->sched.attr.priority,
>>>>> +                                  "expired:%s last=%llx:%lld, prio=%d, hint=%d, yield?=%s\n",
>>>>> +                                  yesno(timer_expired(&execlists->timer)),
>>>>> +                                  last->fence.context, last->fence.seqno,
>>>>> +                                  rq_prio(last),
>>>>>                                      execlists->queue_priority_hint,
>>>>>                                      yesno(timeslice_yield(execlists, last)));
>>>>>     
>>>>> +                     cancel_timer(&execlists->timer);
>>>>
>>>> What is this cancel for?
>>>
>>> This branch is taken upon yielding the timeslice, but we may not submit
>>> a new pair of contexts, leaving the timer active (and marked as
>>> expired). Since the timer remains expired, we will continuously looped
>>> until a context switch, or some other preemption event.
>>
>> Sorry I was looking at the cancel_timer in process_csb and ended up
>> replying at the wrong spot. The situation there seems to be removing the
>> single timeslice related call (set_timeslice) and adding a cancel_timer
>> which is also not obvious to me what it is about.
> 
> Yes, there the cancel_timer() is equivalent to the old set_timeslice().
> 
> After processing an event, we assume it is a change in context meriting
> a new timeslice. To start a new timeslice rather than continue the old
> one, we remove an existing timer and readd it for the end of the
> timeslice.

I was looking what is the resulting symmetry of start/cancel call sites. 
We end up with a single start_timeslice call from the tasklet, but 
guarded with !pending. That looked confusing at first until I remembered 
you mentioned (or a comment somewhere already says) that the idea is to 
only start the timeslice after the csb ack.

That explains the transition from timer disabled to timer enabled.

Then as long as there are contexts queued code relies on timeslice 
expiry, or re-submission with change, to temporarily suspend the timer.

It looks okay as far as I can see. Will tag the latest.

Regards,

Tvrtko