[Intel-gfx] [PATCH 2/8] drm/i915/execlists: Suppress preempting self
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Tue Jan 29 10:13:47 UTC 2019
On 29/01/2019 08:58, Chris Wilson wrote:
> In order to avoid preempting ourselves, we currently refuse to schedule
> the tasklet if we reschedule an inflight context. However, this glosses
> over a few issues such as what happens after a CS completion event and
> we then preempt the newly executing context with itself, or if something
> else causes a tasklet_schedule triggering the same evaluation to
> preempt the active context with itself.
>
> However, when we avoid preempting ELSP[0], we still retain the preemption
> value as it may match a second preemption request within the same time period
> that we need to resolve after the next CS event. However, since we only
> store the maximum preemption priority seen, it may not match the
> subsequent event and so we should double check whether or not we
> actually do need to trigger a preempt-to-idle by comparing the top
> priorities from each queue. Later, this gives us a hook for finer
> control over deciding whether the preempt-to-idle is justified.
>
> The sequence of events where we end up preempting for no avail is:
>
> 1. Queue requests/contexts A, B
> 2. Priority boost A; no preemption as it is executing, but keep hint
> 3. After CS switch, B is less than hint, force preempt-to-idle
> 4. Resubmit B after idling
>
> v2: We can simplify a bunch of tests based on the knowledge that PI will
> ensure that earlier requests along the same context will have the highest
> priority.
> v3: Demonstrate the stale preemption hint with a selftest
>
> References: a2bf92e8cc16 ("drm/i915/execlists: Avoid kicking priority on the current context")
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
> drivers/gpu/drm/i915/i915_scheduler.c | 20 ++-
> drivers/gpu/drm/i915/intel_guc_submission.c | 2 +
> drivers/gpu/drm/i915/intel_lrc.c | 96 ++++++++++++--
> drivers/gpu/drm/i915/intel_ringbuffer.h | 1 +
> drivers/gpu/drm/i915/selftests/intel_lrc.c | 138 ++++++++++++++++++++
> 5 files changed, 245 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
> index 0da718ceab43..7db1255665a8 100644
> --- a/drivers/gpu/drm/i915/i915_scheduler.c
> +++ b/drivers/gpu/drm/i915/i915_scheduler.c
> @@ -238,6 +238,18 @@ sched_lock_engine(struct i915_sched_node *node, struct intel_engine_cs *locked)
> return engine;
> }
>
> +static bool inflight(const struct i915_request *rq,
> + const struct intel_engine_cs *engine)
> +{
> + const struct i915_request *active;
> +
> + if (!rq->global_seqno)
> + return false;
> +
> + active = port_request(engine->execlists.port);
> + return active->hw_context == rq->hw_context;
> +}
> +
> static void __i915_schedule(struct i915_request *rq,
> const struct i915_sched_attr *attr)
> {
> @@ -327,6 +339,7 @@ static void __i915_schedule(struct i915_request *rq,
> INIT_LIST_HEAD(&dep->dfs_link);
>
> engine = sched_lock_engine(node, engine);
> + lockdep_assert_held(&engine->timeline.lock);
>
> /* Recheck after acquiring the engine->timeline.lock */
> if (prio <= node->attr.priority || node_signaled(node))
> @@ -355,17 +368,16 @@ static void __i915_schedule(struct i915_request *rq,
> if (prio <= engine->execlists.preempt_priority_hint)
> continue;
>
> + engine->execlists.preempt_priority_hint = prio;
> +
> /*
> * If we are already the currently executing context, don't
> * bother evaluating if we should preempt ourselves.
> */
> - if (node_to_request(node)->global_seqno &&
> - i915_seqno_passed(port_request(engine->execlists.port)->global_seqno,
> - node_to_request(node)->global_seqno))
> + if (inflight(node_to_request(node), engine))
> continue;
>
> /* Defer (tasklet) submission until after all of our updates. */
> - engine->execlists.preempt_priority_hint = prio;
> tasklet_hi_schedule(&engine->execlists.tasklet);
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
> index 6cdd3f097209..55ec122ee9e9 100644
> --- a/drivers/gpu/drm/i915/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/intel_guc_submission.c
> @@ -629,6 +629,8 @@ static void inject_preempt_context(struct work_struct *work)
> EXECLISTS_ACTIVE_PREEMPT);
> tasklet_schedule(&engine->execlists.tasklet);
> }
> +
> + (void)I915_SELFTEST_ONLY(engine->execlists.preempt_hang.count++);
> }
>
> /*
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 9219efa4ee79..b8cddef5aed8 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -188,13 +188,90 @@ static inline int rq_prio(const struct i915_request *rq)
> return rq->sched.attr.priority;
> }
>
> +static int queue_prio(const struct intel_engine_execlists *execlists)
> +{
> + struct i915_priolist *p;
> + struct rb_node *rb;
> +
> + rb = rb_first_cached(&execlists->queue);
> + if (!rb)
> + return INT_MIN;
> +
> + /*
> + * As the priolist[] are inverted, with the highest priority in [0],
> + * we have to flip the index value to become priority.
> + */
> + p = to_priolist(rb);
> + return ((p->priority + 1) << I915_USER_PRIORITY_SHIFT) - ffs(p->used);
Where does the +1 come from?
p->priority stores in the user range. So I expected something like:
(p->priority << SHIFT) + (COUNT - ffs)
Ok it's the same thing.. my algebra is beyond rusty. :)
> +}
> +
> static inline bool need_preempt(const struct intel_engine_cs *engine,
> - const struct i915_request *last,
> - int prio)
> + const struct i915_request *rq)
> +{
> + const int last_prio = rq_prio(rq);
> +
> + if (!intel_engine_has_preemption(engine))
> + return false;
> +
> + if (i915_request_completed(rq))
> + return false;
> +
> + /*
> + * Check if the current priority hint merits a preemption attempt.
> + *
> + * We record the highest value priority we saw during rescheduling
> + * prior to this dequeue, therefore we know that if it is strictly
> + * less than the current tail of ESLP[0], we do not need to force
> + * a preempt-to-idle cycle.
> + *
> + * However, the priority hint is a mere hint that we may need to
> + * preempt. If that hint is stale or we may be trying to preempt
> + * ourselves, ignore the request.
> + */
> + if (!__execlists_need_preempt(engine->execlists.preempt_priority_hint,
> + last_prio))
> + return false;
> +
> + /*
> + * Check against the first request in ELSP[1], it will, thanks to the
> + * power of PI, be the highest priority of that context.
> + */
> + if (!list_is_last(&rq->link, &engine->timeline.requests) &&
> + rq_prio(list_next_entry(rq, link)) > last_prio)
> + return true;
> +
> + /*
> + * If the inflight context did not trigger the preemption, then maybe
> + * it was the set of queued requests? Pick the highest priority in
> + * the queue (the first active priolist) and see if it deserves to be
> + * running instead of ELSP[0].
> + *
> + * The highest priority request in the queue can not be either
> + * ELSP[0] or ELSP[1] as, thanks again to PI, if it was the same
> + * context, it's priority would not exceed ELSP[0] aka last_prio.
> + */
> + return queue_prio(&engine->execlists) > last_prio;
> +}
> +
> +__maybe_unused static inline bool
> +assert_priority_queue(const struct intel_engine_execlists *execlists,
> + const struct i915_request *prev,
> + const struct i915_request *next)
> {
> - return (intel_engine_has_preemption(engine) &&
> - __execlists_need_preempt(prio, rq_prio(last)) &&
> - !i915_request_completed(last));
> + if (!prev)
> + return true;
> +
> + /*
> + * Without preemption, the prev may refer to the still active element
> + * which we refuse to let go.
> + *
> + * Even with preemption, there are times when we think it is better not
> + * to preempt and leave an ostensibly lower priority request in flight.
> + */
> + if (port_request(execlists->port) == prev)
> + return true;
> +
> + return rq_prio(prev) >= rq_prio(next);
> }
>
> /*
> @@ -523,6 +600,8 @@ static void inject_preempt_context(struct intel_engine_cs *engine)
>
> execlists_clear_active(execlists, EXECLISTS_ACTIVE_HWACK);
> execlists_set_active(execlists, EXECLISTS_ACTIVE_PREEMPT);
> +
> + (void)I915_SELFTEST_ONLY(execlists->preempt_hang.count++);
> }
>
> static void complete_preempt_context(struct intel_engine_execlists *execlists)
> @@ -591,7 +670,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
> if (!execlists_is_active(execlists, EXECLISTS_ACTIVE_HWACK))
> return;
>
> - if (need_preempt(engine, last, execlists->preempt_priority_hint)) {
> + if (need_preempt(engine, last)) {
> inject_preempt_context(engine);
> return;
> }
> @@ -637,8 +716,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
> int i;
>
> priolist_for_each_request_consume(rq, rn, p, i) {
> - GEM_BUG_ON(last &&
> - need_preempt(engine, last, rq_prio(rq)));
> + GEM_BUG_ON(!assert_priority_queue(execlists, last, rq));
>
> /*
> * Can we combine this request with the current port?
> @@ -884,6 +962,8 @@ static void process_csb(struct intel_engine_cs *engine)
> const u32 * const buf = execlists->csb_status;
> u8 head, tail;
>
> + lockdep_assert_held(&engine->timeline.lock);
> +
> /*
> * Note that csb_write, csb_status may be either in HWSP or mmio.
> * When reading from the csb_write mmio register, we have to be
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 5e2231d0c2fa..5ce54f23a945 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -203,6 +203,7 @@ struct i915_priolist {
>
> struct st_preempt_hang {
> struct completion completion;
> + unsigned int count;
> bool inject_hang;
> };
>
> diff --git a/drivers/gpu/drm/i915/selftests/intel_lrc.c b/drivers/gpu/drm/i915/selftests/intel_lrc.c
> index 2b2ecd76c2ac..58f534a39118 100644
> --- a/drivers/gpu/drm/i915/selftests/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/selftests/intel_lrc.c
> @@ -268,6 +268,143 @@ static int live_late_preempt(void *arg)
> goto err_ctx_lo;
> }
>
> +struct preempt_client {
> + struct igt_spinner spin;
> + struct i915_gem_context *ctx;
> +};
> +
> +static int preempt_client_init(struct drm_i915_private *i915,
> + struct preempt_client *c)
> +{
> + c->ctx = kernel_context(i915);
> + if (!c->ctx)
> + return -ENOMEM;
> +
> + if (igt_spinner_init(&c->spin, i915))
> + goto err_ctx;
> +
> + return 0;
> +
> +err_ctx:
> + kernel_context_close(c->ctx);
> + return -ENOMEM;
> +}
> +
> +static void preempt_client_fini(struct preempt_client *c)
> +{
> + igt_spinner_fini(&c->spin);
> + kernel_context_close(c->ctx);
> +}
> +
> +static int live_suppress_preempt(void *arg)
> +{
> + struct drm_i915_private *i915 = arg;
> + struct intel_engine_cs *engine;
> + struct i915_sched_attr attr = {
> + .priority = I915_USER_PRIORITY(I915_PRIORITY_MAX)
> + };
> + struct preempt_client a, b;
> + enum intel_engine_id id;
> + intel_wakeref_t wakeref;
> + int err = -ENOMEM;
> +
> + /*
> + * Verify that if a preemption request does not cause a change in
> + * the current execution order, the preempt-to-idle injection is
> + * skipped and that we do not accidentally apply it after the CS
> + * completion event.
> + */
> +
> + if (!HAS_LOGICAL_RING_PREEMPTION(i915))
> + return 0;
> +
> + if (USES_GUC_SUBMISSION(i915))
> + return 0; /* presume black blox */
Pity but future proof I suppose. :/
Even though you bothered to add the counter into the guc path.
Good it's blox and not something worse. :)
> +
> + mutex_lock(&i915->drm.struct_mutex);
> + wakeref = intel_runtime_pm_get(i915);
> +
> + if (preempt_client_init(i915, &a))
> + goto err_unlock;
> + if (preempt_client_init(i915, &b))
> + goto err_client_a;
> +
> + for_each_engine(engine, i915, id) {
> + struct i915_request *rq_a, *rq_b;
> + int depth;
> +
> + engine->execlists.preempt_hang.count = 0;
> +
> + rq_a = igt_spinner_create_request(&a.spin,
> + a.ctx, engine,
> + MI_NOOP);
> + if (IS_ERR(rq_a)) {
> + err = PTR_ERR(rq_a);
> + goto err_client_b;
> + }
> +
> + i915_request_add(rq_a);
> + if (!igt_wait_for_spinner(&a.spin, rq_a)) {
> + pr_err("First client failed to start\n");
> + goto err_wedged;
> + }
> +
> + for (depth = 0; depth < 8; depth++) {
> + rq_b = igt_spinner_create_request(&b.spin,
> + b.ctx, engine,
> + MI_NOOP);
> + if (IS_ERR(rq_b)) {
> + err = PTR_ERR(rq_b);
> + goto err_client_b;
> + }
> + i915_request_add(rq_b);
> +
> + GEM_BUG_ON(i915_request_completed(rq_a));
> + engine->schedule(rq_a, &attr);
> + igt_spinner_end(&a.spin);
> +
> + if (!igt_wait_for_spinner(&b.spin, rq_b)) {
> + pr_err("Second client failed to start\n");
> + goto err_wedged;
> + }
> +
> + swap(a, b);
> + rq_a = rq_b;
Could store rq in the clients as well I think.
> + }
> + igt_spinner_end(&a.spin);
> +
> + if (engine->execlists.preempt_hang.count) {
> + pr_err("Preemption recorded x%d, depth %d; should have been suppressed!\n",
> + engine->execlists.preempt_hang.count,
> + depth);
> + err = -EINVAL;
> + goto err_client_b;
> + }
> +
> + if (igt_flush_test(i915, I915_WAIT_LOCKED))
> + goto err_wedged;
> + }
> +
> + err = 0;
> +err_client_b:
> + preempt_client_fini(&b);
> +err_client_a:
> + preempt_client_fini(&a);
> +err_unlock:
> + if (igt_flush_test(i915, I915_WAIT_LOCKED))
> + err = -EIO;
> + intel_runtime_pm_put(i915, wakeref);
> + mutex_unlock(&i915->drm.struct_mutex);
> + return err;
> +
> +err_wedged:
> + igt_spinner_end(&b.spin);
> + igt_spinner_end(&a.spin);
> + i915_gem_set_wedged(i915);
> + err = -EIO;
> + goto err_client_b;
> +}
> +
> static int live_preempt_hang(void *arg)
> {
> struct drm_i915_private *i915 = arg;
> @@ -647,6 +784,7 @@ int intel_execlists_live_selftests(struct drm_i915_private *i915)
> SUBTEST(live_sanitycheck),
> SUBTEST(live_preempt),
> SUBTEST(live_late_preempt),
> + SUBTEST(live_suppress_preempt),
> SUBTEST(live_preempt_hang),
> SUBTEST(live_preempt_smoke),
> };
>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
Regards,
Tvrtko
More information about the Intel-gfx
mailing list