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

[Chris Wilson]

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