Quoting Tvrtko Ursulin (2019-02-04 12:05:34)
>
> On 04/02/2019 08:41, Chris Wilson wrote:
> > On unwinding the active request we give it a small (limited to internal
> > priority levels) boost to prevent it from being gazumped a second time.
> > However, this means that it can be promoted to above the request that
> > triggered the preemption request, causing a preempt-to-idle cycle for no
> > change. We can avoid this if we take the boost into account when
> > checking if the preemption request is valid.
>
> After the previous patch to not preempt on wait this is not true any
> more right?
>
> >
> > v2: After preemption the active request will be after the preemptee if
> > they end up with equal priority.
> >
> > v3: Tvrtko pointed out that this, the existing logic, makes
> > I915_PRIORITY_WAIT non-preemptible. Document this interesting quirk!
> >
> > v4: Prove Tvrtko was right about WAIT being non-preemptible and test it.
> > v5: Except not all priorities were made equal, and the WAIT not preempting
> > is only if we start off as !NEWCLIENT.
> >
> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/intel_lrc.c | 38 ++++++++++++++++++++++++++++----
> > 1 file changed, 34 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > index 773df0bd685b..9b6b3acb9070 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -164,6 +164,8 @@
> > #define WA_TAIL_DWORDS 2
> > #define WA_TAIL_BYTES (sizeof(u32) * WA_TAIL_DWORDS)
> >
> > +#define ACTIVE_PRIORITY (I915_PRIORITY_NEWCLIENT)
> > +
> > static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
> > struct intel_engine_cs *engine,
> > struct intel_context *ce);
> > @@ -190,8 +192,30 @@ static inline int rq_prio(const struct i915_request *rq)
> >
> > static int effective_prio(const struct i915_request *rq)
> > {
> > + int prio = rq_prio(rq);
> > +
> > + /*
> > + * On unwinding the active request, we give it a priority bump
> > + * equivalent to a freshly submitted request. This protects it from
> > + * being gazumped again, but it would be preferable if we didn't
> > + * let it be gazumped in the first place!
> > + *
> > + * See __unwind_incomplete_requests()
> > + */
> > + if (~prio & ACTIVE_PRIORITY && __i915_request_has_started(rq)) {
>
> So effectively a started request cannot be preempted by a new client, on
> the same base priority level.
>
> I am still not sure that we should give special meaning to a started
> request. We don't know how long it would execute or anything. It may be
> the only request, or it may be a last in a coalesced context. And in
> both cases it can be either short or long.
It's a mere reflection of unwind_incomplete_requests. We don't do the
preemption in this case, so why even start.
> > + /*
> > + * After preemption, we insert the active request at the
> > + * end of the new priority level. This means that we will be
> > + * _lower_ priority than the preemptee all things equal (and
> > + * so the preemption is valid), so adjust our comparison
> > + * accordingly.
> > + */
> > + prio |= ACTIVE_PRIORITY;
> > + prio--;
> > + }
> > +
> > /* Restrict mere WAIT boosts from triggering preemption */
> > - return rq_prio(rq) | __NO_PREEMPTION;
> > + return prio | __NO_PREEMPTION;
> > }
> >
> > static int queue_prio(const struct intel_engine_execlists *execlists)
> > @@ -360,7 +384,7 @@ __unwind_incomplete_requests(struct intel_engine_cs *engine)
> > {
> > struct i915_request *rq, *rn, *active = NULL;
> > struct list_head *uninitialized_var(pl);
> > - int prio = I915_PRIORITY_INVALID | I915_PRIORITY_NEWCLIENT;
> > + int prio = I915_PRIORITY_INVALID | ACTIVE_PRIORITY;
> >
> > lockdep_assert_held(&engine->timeline.lock);
> >
> > @@ -391,9 +415,15 @@ __unwind_incomplete_requests(struct intel_engine_cs *engine)
> > * The active request is now effectively the start of a new client
> > * stream, so give it the equivalent small priority bump to prevent
> > * it being gazumped a second time by another peer.
> > + *
> > + * One consequence of this preemption boost is that we may jump
> > + * over lesser priorities (such as I915_PRIORITY_WAIT), effectively
> > + * making those priorities non-preemptible. They will be moved
> forward
>
> Not fully preemptible, just in the same user prio bucket.
It means that the preemption request was ignored; it was unable to
preempt, non-preemptible.
> > + * in the priority queue, but they will not gain immediate access to
> > + * the GPU.
> > */
> > - if (!(prio & I915_PRIORITY_NEWCLIENT)) {
> > - prio |= I915_PRIORITY_NEWCLIENT;
> > + if (~prio & ACTIVE_PRIORITY && __i915_request_has_started(active)) {
> > + prio |= ACTIVE_PRIORITY;
> > active->sched.attr.priority = prio;
> > list_move_tail(&active->sched.link,
> > i915_sched_lookup_priolist(engine, prio));
> >
>
> And here it is a big change as well. We would stop giving boost to
> preempted requests if they haven't started executing yet. And we have no
> accounting to how many times something is preempted, to maybe keep
> bumping their priorities in those cases. Which would probably move
> towards different priority management. Like:
What? There's no change here. The logic is important though, as it can
only apply if the request wasn't waiting.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
