Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> writes:
> Quoting Mika Kuoppala (2019-10-21 10:49:14)
>> Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> writes:
>>
>> > If we change the priority of the active context, then it has no impact
>> > on the decision of whether to preempt the active context -- we don't
>> > preempt the context with itself. In this situation, we elide the tasklet
>> > rescheduling and should *not* be marking up the queue_priority_hint as
>> > that may mask a later submission where we decide we don't have to kick
>> > the tasklet as a higher priority submission is pending (spoiler alert,
>> > it was not).
>> >
>> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
>> > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
>> > Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
>> > ---
>> > drivers/gpu/drm/i915/i915_scheduler.c | 37 ++++++++++++++++-----------
>> > 1 file changed, 22 insertions(+), 15 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
>> > index 0ca40f6bf08c..d2edb527dcb8 100644
>> > --- a/drivers/gpu/drm/i915/i915_scheduler.c
>> > +++ b/drivers/gpu/drm/i915/i915_scheduler.c
>> > @@ -189,22 +189,34 @@ static inline bool need_preempt(int prio, int active)
>> > return prio >= max(I915_PRIORITY_NORMAL, active);
>> > }
>> >
>> > -static void kick_submission(struct intel_engine_cs *engine, int prio)
>> > +static void kick_submission(struct intel_engine_cs *engine,
>> > + const struct i915_request *rq,
>> > + int prio)
>> > {
>> > - const struct i915_request *inflight =
>> > - execlists_active(&engine->execlists);
>> > + const struct i915_request *inflight;
>> > +
>> > + /*
>> > + * We only need to kick the tasklet once for the high priority
>> > + * new context we add into the queue.
>> > + */
>> > + if (prio <= engine->execlists.queue_priority_hint)
>> > + return;
>> > +
>> > + /* Nothing currently active? We're overdue for a submission! */
>> > + inflight = execlists_active(&engine->execlists);
>> > + if (!inflight)
>> > + return;
>> >
>> > /*
>> > * If we are already the currently executing context, don't
>> > - * bother evaluating if we should preempt ourselves, or if
>> > - * we expect nothing to change as a result of running the
>> > - * tasklet, i.e. we have not change the priority queue
>> > - * sufficiently to oust the running context.
>> > + * bother evaluating if we should preempt ourselves.
>> > */
>> > - if (!inflight || !need_preempt(prio, rq_prio(inflight)))
>> > + if (inflight->hw_context == rq->hw_context)
>>
>> If there is a tail update at this moment, does the hardware
>> take it into account or do we need to kick?
>
> We are holding the engine->active.lock, so we can't submit at this
> moment. If we are inside process_csb (which is outside of the lock),
> then this stale value if of no consequence as we are inside the tasklet
> already. So if we suppress the kick, we are inside the tasklet and
> didn't need the kick. The other result of giving a kick even though the
> HW as about ready, is just one kick too many. We are just trying to
> reduce the number of unnecessary tasklet executions, ideal is 0 false
> kicks, but any small number is better than kicking on every loop through
> the priority node updates.
Ok, can't submit nor can't change prio. My prime concern was one
kick too little.
Reviewed-by: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
