* [PATCH 1/8] drm/i915: Rename execlists->queue_priority to preempt_priority_hint
@ 2019-01-29 8:58 Chris Wilson
2019-01-29 8:58 ` [PATCH 2/8] drm/i915/execlists: Suppress preempting self Chris Wilson
` (12 more replies)
0 siblings, 13 replies; 23+ messages in thread
From: Chris Wilson @ 2019-01-29 8:58 UTC (permalink / raw)
To: intel-gfx
After noticing that we trigger preemption events for currently executing
requests, as well as requests that complete before the preemption and
attempting to suppress those preemption events, it is wise to not
consider the queue_priority to be authoritative. As we only track the
maximum priority seen between dequeue passes, if the maximum priority
request is no longer available for dequeuing (it completed or is even
executing on another engine), we have no knowledge of the previous
queue_priority as it would require us to keep a full history of enqueued
requests -- but we already have that history in the priolists!
Rename the queue_priority to preempt_priority_hint so that we do not
confuse it as being the maximum priority in the queue, but merely an
indication that we have seen a new maximum priority value and as such we
should check whether it should preempt the currently running request.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/i915_scheduler.c | 11 +++++------
drivers/gpu/drm/i915/intel_engine_cs.c | 4 ++--
drivers/gpu/drm/i915/intel_guc_submission.c | 5 +++--
drivers/gpu/drm/i915/intel_lrc.c | 20 +++++++++++---------
drivers/gpu/drm/i915/intel_ringbuffer.h | 8 ++++++--
5 files changed, 27 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
index 340faea6c08a..0da718ceab43 100644
--- a/drivers/gpu/drm/i915/i915_scheduler.c
+++ b/drivers/gpu/drm/i915/i915_scheduler.c
@@ -127,8 +127,7 @@ static inline struct i915_priolist *to_priolist(struct rb_node *rb)
return rb_entry(rb, struct i915_priolist, node);
}
-static void assert_priolists(struct intel_engine_execlists * const execlists,
- long queue_priority)
+static void assert_priolists(struct intel_engine_execlists * const execlists)
{
struct rb_node *rb;
long last_prio, i;
@@ -139,7 +138,7 @@ static void assert_priolists(struct intel_engine_execlists * const execlists,
GEM_BUG_ON(rb_first_cached(&execlists->queue) !=
rb_first(&execlists->queue.rb_root));
- last_prio = (queue_priority >> I915_USER_PRIORITY_SHIFT) + 1;
+ last_prio = (INT_MAX >> I915_USER_PRIORITY_SHIFT) + 1;
for (rb = rb_first_cached(&execlists->queue); rb; rb = rb_next(rb)) {
const struct i915_priolist *p = to_priolist(rb);
@@ -166,7 +165,7 @@ i915_sched_lookup_priolist(struct intel_engine_cs *engine, int prio)
int idx, i;
lockdep_assert_held(&engine->timeline.lock);
- assert_priolists(execlists, INT_MAX);
+ assert_priolists(execlists);
/* buckets sorted from highest [in slot 0] to lowest priority */
idx = I915_PRIORITY_COUNT - (prio & I915_PRIORITY_MASK) - 1;
@@ -353,7 +352,7 @@ static void __i915_schedule(struct i915_request *rq,
continue;
}
- if (prio <= engine->execlists.queue_priority)
+ if (prio <= engine->execlists.preempt_priority_hint)
continue;
/*
@@ -366,7 +365,7 @@ static void __i915_schedule(struct i915_request *rq,
continue;
/* Defer (tasklet) submission until after all of our updates. */
- engine->execlists.queue_priority = prio;
+ engine->execlists.preempt_priority_hint = prio;
tasklet_hi_schedule(&engine->execlists.tasklet);
}
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index ead9c4371fe1..5a80db990351 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -480,7 +480,7 @@ static void intel_engine_init_execlist(struct intel_engine_cs *engine)
GEM_BUG_ON(!is_power_of_2(execlists_num_ports(execlists)));
GEM_BUG_ON(execlists_num_ports(execlists) > EXECLIST_MAX_PORTS);
- execlists->queue_priority = INT_MIN;
+ execlists->preempt_priority_hint = INT_MIN;
execlists->queue = RB_ROOT_CACHED;
}
@@ -1178,7 +1178,7 @@ void intel_engines_park(struct drm_i915_private *i915)
}
/* Must be reset upon idling, or we may miss the busy wakeup. */
- GEM_BUG_ON(engine->execlists.queue_priority != INT_MIN);
+ GEM_BUG_ON(engine->execlists.preempt_priority_hint != INT_MIN);
if (engine->park)
engine->park(engine);
diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
index 4295ade0d613..6cdd3f097209 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -737,7 +737,7 @@ static bool __guc_dequeue(struct intel_engine_cs *engine)
if (intel_engine_has_preemption(engine)) {
struct guc_preempt_work *preempt_work =
&engine->i915->guc.preempt_work[engine->id];
- int prio = execlists->queue_priority;
+ int prio = execlists->preempt_priority_hint;
if (__execlists_need_preempt(prio, port_prio(port))) {
execlists_set_active(execlists,
@@ -783,7 +783,8 @@ static bool __guc_dequeue(struct intel_engine_cs *engine)
kmem_cache_free(engine->i915->priorities, p);
}
done:
- execlists->queue_priority = rb ? to_priolist(rb)->priority : INT_MIN;
+ execlists->preempt_priority_hint =
+ rb ? to_priolist(rb)->priority : INT_MIN;
if (submit)
port_assign(port, last);
if (last)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index fdbb3fe8eac9..9219efa4ee79 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -591,7 +591,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->queue_priority)) {
+ if (need_preempt(engine, last, execlists->preempt_priority_hint)) {
inject_preempt_context(engine);
return;
}
@@ -699,20 +699,20 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
/*
* Here be a bit of magic! Or sleight-of-hand, whichever you prefer.
*
- * We choose queue_priority such that if we add a request of greater
+ * We choose the priority hint such that if we add a request of greater
* priority than this, we kick the submission tasklet to decide on
* the right order of submitting the requests to hardware. We must
* also be prepared to reorder requests as they are in-flight on the
- * HW. We derive the queue_priority then as the first "hole" in
+ * HW. We derive the priority hint then as the first "hole" in
* the HW submission ports and if there are no available slots,
* the priority of the lowest executing request, i.e. last.
*
* When we do receive a higher priority request ready to run from the
- * user, see queue_request(), the queue_priority is bumped to that
+ * user, see queue_request(), the priority hint is bumped to that
* request triggering preemption on the next dequeue (or subsequent
* interrupt for secondary ports).
*/
- execlists->queue_priority =
+ execlists->preempt_priority_hint =
port != execlists->port ? rq_prio(last) : INT_MIN;
if (submit) {
@@ -861,7 +861,7 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
/* Remaining _unready_ requests will be nop'ed when submitted */
- execlists->queue_priority = INT_MIN;
+ execlists->preempt_priority_hint = INT_MIN;
execlists->queue = RB_ROOT_CACHED;
GEM_BUG_ON(port_isset(execlists->port));
@@ -1092,8 +1092,8 @@ static void __submit_queue_imm(struct intel_engine_cs *engine)
static void submit_queue(struct intel_engine_cs *engine, int prio)
{
- if (prio > engine->execlists.queue_priority) {
- engine->execlists.queue_priority = prio;
+ if (prio > engine->execlists.preempt_priority_hint) {
+ engine->execlists.preempt_priority_hint = prio;
__submit_queue_imm(engine);
}
}
@@ -2748,7 +2748,9 @@ void intel_execlists_show_requests(struct intel_engine_cs *engine,
last = NULL;
count = 0;
- drm_printf(m, "\t\tQueue priority: %d\n", execlists->queue_priority);
+ if (execlists->preempt_priority_hint != INT_MIN)
+ drm_printf(m, "\t\tPreempt priority hint: %d\n",
+ execlists->preempt_priority_hint);
for (rb = rb_first_cached(&execlists->queue); rb; rb = rb_next(rb)) {
struct i915_priolist *p = rb_entry(rb, typeof(*p), node);
int i;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 2927b712b973..5e2231d0c2fa 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -293,14 +293,18 @@ struct intel_engine_execlists {
unsigned int port_mask;
/**
- * @queue_priority: Highest pending priority.
+ * @preempt_priority_hint: Highest pending priority.
*
* When we add requests into the queue, or adjust the priority of
* executing requests, we compute the maximum priority of those
* pending requests. We can then use this value to determine if
* we need to preempt the executing requests to service the queue.
+ * However, since the we may have recorded the priority of an inflight
+ * request we wanted to preempt but since completed, at the time of
+ * dequeuing the priority hint may no longer may match the highest
+ * available request priority.
*/
- int queue_priority;
+ int preempt_priority_hint;
/**
* @queue: queue of requests, in priority lists
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 2/8] drm/i915/execlists: Suppress preempting self
2019-01-29 8:58 [PATCH 1/8] drm/i915: Rename execlists->queue_priority to preempt_priority_hint Chris Wilson
@ 2019-01-29 8:58 ` Chris Wilson
2019-01-29 10:13 ` Tvrtko Ursulin
2019-01-29 8:58 ` [PATCH 3/8] drm/i915/execlists: Suppress redundant preemption Chris Wilson
` (11 subsequent siblings)
12 siblings, 1 reply; 23+ messages in thread
From: Chris Wilson @ 2019-01-29 8:58 UTC (permalink / raw)
To: intel-gfx
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@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@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);
+}
+
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 */
+
+ 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;
+ }
+ 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),
};
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 3/8] drm/i915/execlists: Suppress redundant preemption
2019-01-29 8:58 [PATCH 1/8] drm/i915: Rename execlists->queue_priority to preempt_priority_hint Chris Wilson
2019-01-29 8:58 ` [PATCH 2/8] drm/i915/execlists: Suppress preempting self Chris Wilson
@ 2019-01-29 8:58 ` Chris Wilson
2019-01-29 11:15 ` Tvrtko Ursulin
2019-01-29 15:27 ` [PATCH] " Chris Wilson
2019-01-29 8:58 ` [PATCH 4/8] drm/i915/selftests: Exercise some AB...BA preemption chains Chris Wilson
` (10 subsequent siblings)
12 siblings, 2 replies; 23+ messages in thread
From: Chris Wilson @ 2019-01-29 8:58 UTC (permalink / raw)
To: intel-gfx
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.
v2: After preemption the active request will be after the preemptee if
they end up with equal priority.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/intel_lrc.c | 39 ++++++++++++++++++++++++++++----
1 file changed, 35 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index b8cddef5aed8..f4b0be81c6db 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);
@@ -188,6 +190,34 @@ static inline int rq_prio(const struct i915_request *rq)
return rq->sched.attr.priority;
}
+static inline int active_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) != ACTIVE_PRIORITY &&
+ i915_request_started(rq)) {
+ /*
+ * 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--;
+ }
+
+ return prio;
+}
+
static int queue_prio(const struct intel_engine_execlists *execlists)
{
struct i915_priolist *p;
@@ -208,7 +238,7 @@ static int queue_prio(const struct intel_engine_execlists *execlists)
static inline bool need_preempt(const struct intel_engine_cs *engine,
const struct i915_request *rq)
{
- const int last_prio = rq_prio(rq);
+ int last_prio;
if (!intel_engine_has_preemption(engine))
return false;
@@ -228,6 +258,7 @@ static inline bool need_preempt(const struct intel_engine_cs *engine,
* preempt. If that hint is stale or we may be trying to preempt
* ourselves, ignore the request.
*/
+ last_prio = active_prio(rq);
if (!__execlists_need_preempt(engine->execlists.preempt_priority_hint,
last_prio))
return false;
@@ -353,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);
@@ -385,8 +416,8 @@ __unwind_incomplete_requests(struct intel_engine_cs *engine)
* stream, so give it the equivalent small priority bump to prevent
* it being gazumped a second time by another peer.
*/
- if (!(prio & I915_PRIORITY_NEWCLIENT)) {
- prio |= I915_PRIORITY_NEWCLIENT;
+ if ((prio & ACTIVE_PRIORITY) != ACTIVE_PRIORITY) {
+ prio |= ACTIVE_PRIORITY;
active->sched.attr.priority = prio;
list_move_tail(&active->sched.link,
i915_sched_lookup_priolist(engine, prio));
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 4/8] drm/i915/selftests: Exercise some AB...BA preemption chains
2019-01-29 8:58 [PATCH 1/8] drm/i915: Rename execlists->queue_priority to preempt_priority_hint Chris Wilson
2019-01-29 8:58 ` [PATCH 2/8] drm/i915/execlists: Suppress preempting self Chris Wilson
2019-01-29 8:58 ` [PATCH 3/8] drm/i915/execlists: Suppress redundant preemption Chris Wilson
@ 2019-01-29 8:58 ` Chris Wilson
2019-01-29 8:58 ` [PATCH 5/8] drm/i915: Identify active requests Chris Wilson
` (9 subsequent siblings)
12 siblings, 0 replies; 23+ messages in thread
From: Chris Wilson @ 2019-01-29 8:58 UTC (permalink / raw)
To: intel-gfx
Build a chain using 2 contexts (A, B) then request a preemption such
that a later A request runs before the spinner in B.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/selftests/intel_lrc.c | 103 +++++++++++++++++++++
1 file changed, 103 insertions(+)
diff --git a/drivers/gpu/drm/i915/selftests/intel_lrc.c b/drivers/gpu/drm/i915/selftests/intel_lrc.c
index 58f534a39118..fe58ad711e2f 100644
--- a/drivers/gpu/drm/i915/selftests/intel_lrc.c
+++ b/drivers/gpu/drm/i915/selftests/intel_lrc.c
@@ -4,6 +4,8 @@
* Copyright © 2018 Intel Corporation
*/
+#include <linux/prime_numbers.h>
+
#include "../i915_reset.h"
#include "../i915_selftest.h"
@@ -405,6 +407,106 @@ static int live_suppress_preempt(void *arg)
goto err_client_b;
}
+static int live_chain_preempt(void *arg)
+{
+ struct drm_i915_private *i915 = arg;
+ struct intel_engine_cs *engine;
+ struct preempt_client hi, lo;
+ enum intel_engine_id id;
+ intel_wakeref_t wakeref;
+ int err = -ENOMEM;
+
+ /*
+ * Build a chain AB...BA between two contexts (A, B) and request
+ * preemption of the last request. It should then complete before
+ * the previously submitted spinner in B.
+ */
+
+ if (!HAS_LOGICAL_RING_PREEMPTION(i915))
+ return 0;
+
+ mutex_lock(&i915->drm.struct_mutex);
+ wakeref = intel_runtime_pm_get(i915);
+
+ if (preempt_client_init(i915, &hi))
+ goto err_unlock;
+
+ if (preempt_client_init(i915, &lo))
+ goto err_client_hi;
+
+ for_each_engine(engine, i915, id) {
+ struct i915_sched_attr attr = {
+ .priority = I915_USER_PRIORITY(I915_PRIORITY_MAX),
+ };
+ int count, i;
+
+ for_each_prime_number_from(count, 1, 32) { /* must fit ring! */
+ struct i915_request *rq;
+
+ rq = igt_spinner_create_request(&hi.spin,
+ hi.ctx, engine,
+ MI_ARB_CHECK);
+ if (IS_ERR(rq))
+ goto err_wedged;
+ i915_request_add(rq);
+ if (!igt_wait_for_spinner(&hi.spin, rq))
+ goto err_wedged;
+
+ rq = igt_spinner_create_request(&lo.spin,
+ lo.ctx, engine,
+ MI_ARB_CHECK);
+ if (IS_ERR(rq))
+ goto err_wedged;
+ i915_request_add(rq);
+
+ for (i = 0; i < count; i++) {
+ rq = i915_request_alloc(engine, lo.ctx);
+ if (IS_ERR(rq))
+ goto err_wedged;
+ i915_request_add(rq);
+ }
+
+ rq = i915_request_alloc(engine, hi.ctx);
+ if (IS_ERR(rq))
+ goto err_wedged;
+ i915_request_add(rq);
+ engine->schedule(rq, &attr);
+
+ igt_spinner_end(&hi.spin);
+ if (i915_request_wait(rq, I915_WAIT_LOCKED, HZ / 5) < 0) {
+ struct drm_printer p =
+ drm_info_printer(i915->drm.dev);
+
+ pr_err("Failed to preempt over chain of %d\n",
+ count);
+ intel_engine_dump(engine, &p,
+ "%s\n", engine->name);
+ goto err_wedged;
+ }
+ igt_spinner_end(&lo.spin);
+ }
+ }
+
+ err = 0;
+err_client_lo:
+ preempt_client_fini(&lo);
+err_client_hi:
+ preempt_client_fini(&hi);
+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(&hi.spin);
+ igt_spinner_end(&lo.spin);
+ i915_gem_set_wedged(i915);
+ err = -EIO;
+ goto err_client_lo;
+}
+
static int live_preempt_hang(void *arg)
{
struct drm_i915_private *i915 = arg;
@@ -785,6 +887,7 @@ int intel_execlists_live_selftests(struct drm_i915_private *i915)
SUBTEST(live_preempt),
SUBTEST(live_late_preempt),
SUBTEST(live_suppress_preempt),
+ SUBTEST(live_chain_preempt),
SUBTEST(live_preempt_hang),
SUBTEST(live_preempt_smoke),
};
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 5/8] drm/i915: Identify active requests
2019-01-29 8:58 [PATCH 1/8] drm/i915: Rename execlists->queue_priority to preempt_priority_hint Chris Wilson
` (2 preceding siblings ...)
2019-01-29 8:58 ` [PATCH 4/8] drm/i915/selftests: Exercise some AB...BA preemption chains Chris Wilson
@ 2019-01-29 8:58 ` Chris Wilson
2019-01-29 8:58 ` [PATCH 6/8] drm/i915: Remove the intel_engine_notify tracepoint Chris Wilson
` (8 subsequent siblings)
12 siblings, 0 replies; 23+ messages in thread
From: Chris Wilson @ 2019-01-29 8:58 UTC (permalink / raw)
To: intel-gfx
To allow requests to forgo a common execution timeline, one question we
need to be able to answer is "is this request running?". To track
whether a request has started on HW, we can emit a breadcrumb at the
beginning of the request and check its timeline's HWSP to see if the
breadcrumb has advanced past the start of this request. (This is in
contrast to the global timeline where we need only ask if we are on the
global timeline and if the timeline has advanced past the end of the
previous request.)
There is still confusion from a preempted request, which has already
started but relinquished the HW to a high priority request. For the
common case, this discrepancy should be negligible. However, for
identification of hung requests, knowing which one was running at the
time of the hang will be much more important.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/i915_gem.c | 15 ++++++++
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 12 ++++++
drivers/gpu/drm/i915/i915_request.c | 10 ++---
drivers/gpu/drm/i915/i915_request.h | 1 +
drivers/gpu/drm/i915/i915_timeline.c | 1 +
drivers/gpu/drm/i915/i915_timeline.h | 2 +
drivers/gpu/drm/i915/intel_engine_cs.c | 8 ++--
drivers/gpu/drm/i915/intel_lrc.c | 39 +++++++++++++++++---
drivers/gpu/drm/i915/intel_ringbuffer.c | 25 ++++++++-----
drivers/gpu/drm/i915/intel_ringbuffer.h | 6 ++-
drivers/gpu/drm/i915/selftests/mock_engine.c | 2 +-
11 files changed, 96 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 05627000b77d..e802af64d628 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2873,6 +2873,14 @@ i915_gem_object_pwrite_gtt(struct drm_i915_gem_object *obj,
return 0;
}
+static bool match_ring(struct i915_request *rq)
+{
+ struct drm_i915_private *dev_priv = rq->i915;
+ u32 ring = I915_READ(RING_START(rq->engine->mmio_base));
+
+ return ring == i915_ggtt_offset(rq->ring->vma);
+}
+
struct i915_request *
i915_gem_find_active_request(struct intel_engine_cs *engine)
{
@@ -2895,6 +2903,13 @@ i915_gem_find_active_request(struct intel_engine_cs *engine)
if (i915_request_completed(request))
continue;
+ if (!i915_request_started(request))
+ break;
+
+ /* More than one preemptible request may match! */
+ if (!match_ring(request))
+ break;
+
active = request;
break;
}
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index f250109e1f66..8eedf7cac493 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1976,6 +1976,18 @@ static int eb_submit(struct i915_execbuffer *eb)
return err;
}
+ /*
+ * After we completed waiting for other engines (using HW semaphores)
+ * then we can signal that this request/batch is ready to run. This
+ * allows us to determine if the batch is still waiting on the GPU
+ * or actually running by checking the breadcrumb.
+ */
+ if (eb->engine->emit_init_breadcrumb) {
+ err = eb->engine->emit_init_breadcrumb(eb->request);
+ if (err)
+ return err;
+ }
+
err = eb->engine->emit_bb_start(eb->request,
eb->batch->node.start +
eb->batch_start_offset,
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 4d58770e6a8c..7db15b7b3de8 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -333,7 +333,7 @@ void i915_request_retire_upto(struct i915_request *rq)
static u32 timeline_get_seqno(struct i915_timeline *tl)
{
- return ++tl->seqno;
+ return tl->seqno += 1 + tl->has_initial_breadcrumb;
}
static void move_to_timeline(struct i915_request *request,
@@ -382,8 +382,8 @@ void __i915_request_submit(struct i915_request *request)
intel_engine_enable_signaling(request, false);
spin_unlock(&request->lock);
- engine->emit_breadcrumb(request,
- request->ring->vaddr + request->postfix);
+ engine->emit_fini_breadcrumb(request,
+ request->ring->vaddr + request->postfix);
/* Transfer from per-context onto the global per-engine timeline */
move_to_timeline(request, &engine->timeline);
@@ -657,7 +657,7 @@ i915_request_alloc(struct intel_engine_cs *engine, struct i915_gem_context *ctx)
* around inside i915_request_add() there is sufficient space at
* the beginning of the ring as well.
*/
- rq->reserved_space = 2 * engine->emit_breadcrumb_dw * sizeof(u32);
+ rq->reserved_space = 2 * engine->emit_fini_breadcrumb_dw * sizeof(u32);
/*
* Record the position of the start of the request so that
@@ -908,7 +908,7 @@ void i915_request_add(struct i915_request *request)
* GPU processing the request, we never over-estimate the
* position of the ring's HEAD.
*/
- cs = intel_ring_begin(request, engine->emit_breadcrumb_dw);
+ cs = intel_ring_begin(request, engine->emit_fini_breadcrumb_dw);
GEM_BUG_ON(IS_ERR(cs));
request->postfix = intel_ring_offset(request, cs);
diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h
index 96c586d6ff4d..340d6216791c 100644
--- a/drivers/gpu/drm/i915/i915_request.h
+++ b/drivers/gpu/drm/i915/i915_request.h
@@ -344,6 +344,7 @@ static inline bool i915_request_started(const struct i915_request *rq)
if (i915_request_signaled(rq))
return true;
+ /* Remember: started but may have since been preempted! */
return i915_seqno_passed(hwsp_seqno(rq), rq->fence.seqno - 1);
}
diff --git a/drivers/gpu/drm/i915/i915_timeline.c b/drivers/gpu/drm/i915/i915_timeline.c
index 79838d89bdb9..5ea3af393ffe 100644
--- a/drivers/gpu/drm/i915/i915_timeline.c
+++ b/drivers/gpu/drm/i915/i915_timeline.c
@@ -135,6 +135,7 @@ int i915_timeline_init(struct drm_i915_private *i915,
timeline->i915 = i915;
timeline->name = name;
timeline->pin_count = 0;
+ timeline->has_initial_breadcrumb = !hwsp;
timeline->hwsp_offset = I915_GEM_HWS_SEQNO_ADDR;
if (!hwsp) {
diff --git a/drivers/gpu/drm/i915/i915_timeline.h b/drivers/gpu/drm/i915/i915_timeline.h
index ab736e2e5707..8caeb66d1cd5 100644
--- a/drivers/gpu/drm/i915/i915_timeline.h
+++ b/drivers/gpu/drm/i915/i915_timeline.h
@@ -48,6 +48,8 @@ struct i915_timeline {
struct i915_vma *hwsp_ggtt;
u32 hwsp_offset;
+ bool has_initial_breadcrumb;
+
/**
* List of breadcrumbs associated with GPU requests currently
* outstanding.
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 5a80db990351..d25e510b7b08 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -664,7 +664,7 @@ static int measure_breadcrumb_dw(struct intel_engine_cs *engine)
if (dw < 0)
goto out_timeline;
- dw = engine->emit_breadcrumb(&frame->rq, frame->cs) - frame->cs;
+ dw = engine->emit_fini_breadcrumb(&frame->rq, frame->cs) - frame->cs;
i915_timeline_unpin(&frame->timeline);
@@ -725,7 +725,7 @@ int intel_engine_init_common(struct intel_engine_cs *engine)
if (ret < 0)
goto err_breadcrumbs;
- engine->emit_breadcrumb_dw = ret;
+ engine->emit_fini_breadcrumb_dw = ret;
return 0;
@@ -1297,7 +1297,9 @@ static void print_request(struct drm_printer *m,
drm_printf(m, "%s%x%s [%llx:%llx]%s @ %dms: %s\n",
prefix,
rq->global_seqno,
- i915_request_completed(rq) ? "!" : "",
+ i915_request_completed(rq) ? "!" :
+ i915_request_started(rq) ? "*" :
+ "",
rq->fence.context, rq->fence.seqno,
buf,
jiffies_to_msecs(jiffies - rq->emitted_jiffies),
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index f4b0be81c6db..c767877533ef 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -734,7 +734,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
* WaIdleLiteRestore:bdw,skl
* Apply the wa NOOPs to prevent
* ring:HEAD == rq:TAIL as we resubmit the
- * request. See gen8_emit_breadcrumb() for
+ * request. See gen8_emit_fini_breadcrumb() for
* where we prepare the padding after the
* end of the request.
*/
@@ -1394,6 +1394,34 @@ execlists_context_pin(struct intel_engine_cs *engine,
return __execlists_context_pin(engine, ctx, ce);
}
+static int gen8_emit_init_breadcrumb(struct i915_request *rq)
+{
+ u32 *cs;
+
+ GEM_BUG_ON(!rq->timeline->has_initial_breadcrumb);
+
+ cs = intel_ring_begin(rq, 6);
+ if (IS_ERR(cs))
+ return PTR_ERR(cs);
+
+ /*
+ * Check if we have been preempted before we even get started.
+ *
+ * After this point i915_request_started() reports true, even if
+ * we get preempted and so are no longer running.
+ */
+ *cs++ = MI_ARB_CHECK;
+ *cs++ = MI_NOOP;
+
+ *cs++ = MI_STORE_DWORD_IMM_GEN4 | MI_USE_GGTT;
+ *cs++ = rq->timeline->hwsp_offset;
+ *cs++ = 0;
+ *cs++ = rq->fence.seqno - 1;
+
+ intel_ring_advance(rq, cs);
+ return 0;
+}
+
static int emit_pdps(struct i915_request *rq)
{
const struct intel_engine_cs * const engine = rq->engine;
@@ -2150,7 +2178,7 @@ static u32 *gen8_emit_wa_tail(struct i915_request *request, u32 *cs)
return cs;
}
-static u32 *gen8_emit_breadcrumb(struct i915_request *request, u32 *cs)
+static u32 *gen8_emit_fini_breadcrumb(struct i915_request *request, u32 *cs)
{
/* w/a: bit 5 needs to be zero for MI_FLUSH_DW address. */
BUILD_BUG_ON(I915_GEM_HWS_INDEX_ADDR & (1 << 5));
@@ -2172,7 +2200,7 @@ static u32 *gen8_emit_breadcrumb(struct i915_request *request, u32 *cs)
return gen8_emit_wa_tail(request, cs);
}
-static u32 *gen8_emit_breadcrumb_rcs(struct i915_request *request, u32 *cs)
+static u32 *gen8_emit_fini_breadcrumb_rcs(struct i915_request *request, u32 *cs)
{
cs = gen8_emit_ggtt_write_rcs(cs,
request->fence.seqno,
@@ -2287,7 +2315,8 @@ logical_ring_default_vfuncs(struct intel_engine_cs *engine)
engine->request_alloc = execlists_request_alloc;
engine->emit_flush = gen8_emit_flush;
- engine->emit_breadcrumb = gen8_emit_breadcrumb;
+ engine->emit_init_breadcrumb = gen8_emit_init_breadcrumb;
+ engine->emit_fini_breadcrumb = gen8_emit_fini_breadcrumb;
engine->set_default_submission = intel_execlists_set_default_submission;
@@ -2400,7 +2429,7 @@ int logical_render_ring_init(struct intel_engine_cs *engine)
/* Override some for render ring. */
engine->init_context = gen8_init_rcs_context;
engine->emit_flush = gen8_emit_flush_render;
- engine->emit_breadcrumb = gen8_emit_breadcrumb_rcs;
+ engine->emit_fini_breadcrumb = gen8_emit_fini_breadcrumb_rcs;
ret = logical_ring_init(engine);
if (ret)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index ee3719324e2d..668ed67336a2 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1607,6 +1607,7 @@ static int intel_init_ring_buffer(struct intel_engine_cs *engine)
err = PTR_ERR(timeline);
goto err;
}
+ GEM_BUG_ON(timeline->has_initial_breadcrumb);
ring = intel_engine_create_ring(engine, timeline, 32 * PAGE_SIZE);
i915_timeline_put(timeline);
@@ -1960,6 +1961,7 @@ static int ring_request_alloc(struct i915_request *request)
int ret;
GEM_BUG_ON(!request->hw_context->pin_count);
+ GEM_BUG_ON(request->timeline->has_initial_breadcrumb);
/*
* Flush enough space to reduce the likelihood of waiting after
@@ -2296,9 +2298,14 @@ static void intel_ring_default_vfuncs(struct drm_i915_private *dev_priv,
engine->context_pin = intel_ring_context_pin;
engine->request_alloc = ring_request_alloc;
- engine->emit_breadcrumb = i9xx_emit_breadcrumb;
+ /*
+ * Using a global execution timeline; the previous final breadcrumb is
+ * equivalent to our next initial bread so we can elide
+ * engine->emit_init_breadcrumb().
+ */
+ engine->emit_fini_breadcrumb = i9xx_emit_breadcrumb;
if (IS_GEN(dev_priv, 5))
- engine->emit_breadcrumb = gen5_emit_breadcrumb;
+ engine->emit_fini_breadcrumb = gen5_emit_breadcrumb;
engine->set_default_submission = i9xx_set_default_submission;
@@ -2327,11 +2334,11 @@ int intel_init_render_ring_buffer(struct intel_engine_cs *engine)
if (INTEL_GEN(dev_priv) >= 7) {
engine->init_context = intel_rcs_ctx_init;
engine->emit_flush = gen7_render_ring_flush;
- engine->emit_breadcrumb = gen7_rcs_emit_breadcrumb;
+ engine->emit_fini_breadcrumb = gen7_rcs_emit_breadcrumb;
} else if (IS_GEN(dev_priv, 6)) {
engine->init_context = intel_rcs_ctx_init;
engine->emit_flush = gen6_render_ring_flush;
- engine->emit_breadcrumb = gen6_rcs_emit_breadcrumb;
+ engine->emit_fini_breadcrumb = gen6_rcs_emit_breadcrumb;
} else if (IS_GEN(dev_priv, 5)) {
engine->emit_flush = gen4_render_ring_flush;
} else {
@@ -2368,9 +2375,9 @@ int intel_init_bsd_ring_buffer(struct intel_engine_cs *engine)
engine->irq_enable_mask = GT_BSD_USER_INTERRUPT;
if (IS_GEN(dev_priv, 6))
- engine->emit_breadcrumb = gen6_xcs_emit_breadcrumb;
+ engine->emit_fini_breadcrumb = gen6_xcs_emit_breadcrumb;
else
- engine->emit_breadcrumb = gen7_xcs_emit_breadcrumb;
+ engine->emit_fini_breadcrumb = gen7_xcs_emit_breadcrumb;
} else {
engine->emit_flush = bsd_ring_flush;
if (IS_GEN(dev_priv, 5))
@@ -2394,9 +2401,9 @@ int intel_init_blt_ring_buffer(struct intel_engine_cs *engine)
engine->irq_enable_mask = GT_BLT_USER_INTERRUPT;
if (IS_GEN(dev_priv, 6))
- engine->emit_breadcrumb = gen6_xcs_emit_breadcrumb;
+ engine->emit_fini_breadcrumb = gen6_xcs_emit_breadcrumb;
else
- engine->emit_breadcrumb = gen7_xcs_emit_breadcrumb;
+ engine->emit_fini_breadcrumb = gen7_xcs_emit_breadcrumb;
return intel_init_ring_buffer(engine);
}
@@ -2414,7 +2421,7 @@ int intel_init_vebox_ring_buffer(struct intel_engine_cs *engine)
engine->irq_enable = hsw_vebox_irq_enable;
engine->irq_disable = hsw_vebox_irq_disable;
- engine->emit_breadcrumb = gen7_xcs_emit_breadcrumb;
+ engine->emit_fini_breadcrumb = gen7_xcs_emit_breadcrumb;
return intel_init_ring_buffer(engine);
}
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 5ce54f23a945..ae753c45cbe7 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -468,8 +468,10 @@ struct intel_engine_cs {
unsigned int dispatch_flags);
#define I915_DISPATCH_SECURE BIT(0)
#define I915_DISPATCH_PINNED BIT(1)
- u32 *(*emit_breadcrumb)(struct i915_request *rq, u32 *cs);
- int emit_breadcrumb_dw;
+ int (*emit_init_breadcrumb)(struct i915_request *rq);
+ u32 *(*emit_fini_breadcrumb)(struct i915_request *rq,
+ u32 *cs);
+ unsigned int emit_fini_breadcrumb_dw;
/* Pass the request to the hardware queue (e.g. directly into
* the legacy ringbuffer or to the end of an execlist).
diff --git a/drivers/gpu/drm/i915/selftests/mock_engine.c b/drivers/gpu/drm/i915/selftests/mock_engine.c
index 95e890d7f58b..3b226ebc6bc4 100644
--- a/drivers/gpu/drm/i915/selftests/mock_engine.c
+++ b/drivers/gpu/drm/i915/selftests/mock_engine.c
@@ -227,7 +227,7 @@ struct intel_engine_cs *mock_engine(struct drm_i915_private *i915,
engine->base.context_pin = mock_context_pin;
engine->base.request_alloc = mock_request_alloc;
engine->base.emit_flush = mock_emit_flush;
- engine->base.emit_breadcrumb = mock_emit_breadcrumb;
+ engine->base.emit_fini_breadcrumb = mock_emit_breadcrumb;
engine->base.submit_request = mock_submit_request;
if (i915_timeline_init(i915,
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 6/8] drm/i915: Remove the intel_engine_notify tracepoint
2019-01-29 8:58 [PATCH 1/8] drm/i915: Rename execlists->queue_priority to preempt_priority_hint Chris Wilson
` (3 preceding siblings ...)
2019-01-29 8:58 ` [PATCH 5/8] drm/i915: Identify active requests Chris Wilson
@ 2019-01-29 8:58 ` Chris Wilson
2019-01-29 8:58 ` [PATCH 7/8] drm/i915: Replace global breadcrumbs with per-context interrupt tracking Chris Wilson
` (7 subsequent siblings)
12 siblings, 0 replies; 23+ messages in thread
From: Chris Wilson @ 2019-01-29 8:58 UTC (permalink / raw)
To: intel-gfx
The global seqno is defunct and so we have no meaningful indicator of
forward progress for an engine. You need to listen to the request
signaling tracepoints instead.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/i915_irq.c | 2 --
drivers/gpu/drm/i915/i915_trace.h | 25 -------------------------
2 files changed, 27 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index caade521c174..e1d2312ec937 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1229,8 +1229,6 @@ static void notify_ring(struct intel_engine_cs *engine)
wake_up_process(tsk);
rcu_read_unlock();
-
- trace_intel_engine_notify(engine, wait);
}
static void vlv_c0_read(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
index 43da14f08dc0..eab313c3163c 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -752,31 +752,6 @@ trace_i915_request_out(struct i915_request *rq)
#endif
#endif
-TRACE_EVENT(intel_engine_notify,
- TP_PROTO(struct intel_engine_cs *engine, bool waiters),
- TP_ARGS(engine, waiters),
-
- TP_STRUCT__entry(
- __field(u32, dev)
- __field(u16, class)
- __field(u16, instance)
- __field(u32, seqno)
- __field(bool, waiters)
- ),
-
- TP_fast_assign(
- __entry->dev = engine->i915->drm.primary->index;
- __entry->class = engine->uabi_class;
- __entry->instance = engine->instance;
- __entry->seqno = intel_engine_get_seqno(engine);
- __entry->waiters = waiters;
- ),
-
- TP_printk("dev=%u, engine=%u:%u, seqno=%u, waiters=%u",
- __entry->dev, __entry->class, __entry->instance,
- __entry->seqno, __entry->waiters)
-);
-
DEFINE_EVENT(i915_request, i915_request_retire,
TP_PROTO(struct i915_request *rq),
TP_ARGS(rq)
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 7/8] drm/i915: Replace global breadcrumbs with per-context interrupt tracking
2019-01-29 8:58 [PATCH 1/8] drm/i915: Rename execlists->queue_priority to preempt_priority_hint Chris Wilson
` (4 preceding siblings ...)
2019-01-29 8:58 ` [PATCH 6/8] drm/i915: Remove the intel_engine_notify tracepoint Chris Wilson
@ 2019-01-29 8:58 ` Chris Wilson
2019-01-29 8:58 ` [PATCH 8/8] drm/i915: Drop fake breadcrumb irq Chris Wilson
` (6 subsequent siblings)
12 siblings, 0 replies; 23+ messages in thread
From: Chris Wilson @ 2019-01-29 8:58 UTC (permalink / raw)
To: intel-gfx
A few years ago, see commit 688e6c725816 ("drm/i915: Slaughter the
thundering i915_wait_request herd"), the issue of handling multiple
clients waiting in parallel was brought to our attention. The
requirement was that every client should be woken immediately upon its
request being signaled, without incurring any cpu overhead.
To handle certain fragility of our hw meant that we could not do a
simple check inside the irq handler (some generations required almost
unbounded delays before we could be sure of seqno coherency) and so
request completion checking required delegation.
Before commit 688e6c725816, the solution was simple. Every client
waiting on a request would be woken on every interrupt and each would do
a heavyweight check to see if their request was complete. Commit
688e6c725816 introduced an rbtree so that only the earliest waiter on
the global timeline would woken, and would wake the next and so on.
(Along with various complications to handle requests being reordered
along the global timeline, and also a requirement for kthread to provide
a delegate for fence signaling that had no process context.)
The global rbtree depends on knowing the execution timeline (and global
seqno). Without knowing that order, we must instead check all contexts
queued to the HW to see which may have advanced. We trim that list by
only checking queued contexts that are being waited on, but still we
keep a list of all active contexts and their active signalers that we
inspect from inside the irq handler. By moving the waiters onto the fence
signal list, we can combine the client wakeup with the dma_fence
signaling (a dramatic reduction in complexity, but does require the HW
being coherent, the seqno must be visible from the cpu before the
interrupt is raised - we keep a timer backup just in case).
Having previously fixed all the issues with irq-seqno serialisation (by
inserting delays onto the GPU after each request instead of random delays
on the CPU after each interrupt), we can rely on the seqno state to
perfom direct wakeups from the interrupt handler. This allows us to
preserve our single context switch behaviour of the current routine,
with the only downside that we lose the RT priority sorting of wakeups.
In general, direct wakeup latency of multiple clients is about the same
(about 10% better in most cases) with a reduction in total CPU time spent
in the waiter (about 20-50% depending on gen). Average herd behaviour is
improved, but at the cost of not delegating wakeups on task_prio.
v2: Capture fence signaling state for error state and add comments to
warm even the most cold of hearts.
v3: Check if the request is still active before busywaiting
v4: Reduce the amount of pointer misdirection with list_for_each_safe
and using a local i915_request variable inside the loops
v5: Add a missing pluralisation to a purely informative selftest message.
References: 688e6c725816 ("drm/i915: Slaughter the thundering i915_wait_request herd")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/i915_debugfs.c | 28 +-
drivers/gpu/drm/i915/i915_gem_context.c | 3 +
drivers/gpu/drm/i915/i915_gem_context.h | 2 +
drivers/gpu/drm/i915/i915_gpu_error.c | 83 +-
drivers/gpu/drm/i915/i915_gpu_error.h | 9 +-
drivers/gpu/drm/i915/i915_irq.c | 88 +-
drivers/gpu/drm/i915/i915_request.c | 142 ++-
drivers/gpu/drm/i915/i915_request.h | 72 +-
drivers/gpu/drm/i915/i915_reset.c | 16 +-
drivers/gpu/drm/i915/i915_scheduler.c | 2 +-
drivers/gpu/drm/i915/intel_breadcrumbs.c | 818 +++++-------------
drivers/gpu/drm/i915/intel_engine_cs.c | 35 +-
drivers/gpu/drm/i915/intel_ringbuffer.c | 2 +-
drivers/gpu/drm/i915/intel_ringbuffer.h | 94 +-
.../drm/i915/selftests/i915_mock_selftests.h | 1 -
drivers/gpu/drm/i915/selftests/i915_request.c | 425 +++++++++
drivers/gpu/drm/i915/selftests/igt_spinner.c | 5 -
.../drm/i915/selftests/intel_breadcrumbs.c | 470 ----------
.../gpu/drm/i915/selftests/intel_hangcheck.c | 2 +-
drivers/gpu/drm/i915/selftests/lib_sw_fence.c | 54 ++
drivers/gpu/drm/i915/selftests/lib_sw_fence.h | 3 +
drivers/gpu/drm/i915/selftests/mock_engine.c | 17 +-
drivers/gpu/drm/i915/selftests/mock_engine.h | 6 -
23 files changed, 894 insertions(+), 1483 deletions(-)
delete mode 100644 drivers/gpu/drm/i915/selftests/intel_breadcrumbs.c
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index f5ac03f06e26..b1ac0f78cb42 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1316,29 +1316,16 @@ static int i915_hangcheck_info(struct seq_file *m, void *unused)
seq_printf(m, "GT active? %s\n", yesno(dev_priv->gt.awake));
for_each_engine(engine, dev_priv, id) {
- struct intel_breadcrumbs *b = &engine->breadcrumbs;
- struct rb_node *rb;
-
seq_printf(m, "%s:\n", engine->name);
seq_printf(m, "\tseqno = %x [current %x, last %x], %dms ago\n",
engine->hangcheck.seqno, seqno[id],
intel_engine_last_submit(engine),
jiffies_to_msecs(jiffies -
engine->hangcheck.action_timestamp));
- seq_printf(m, "\twaiters? %s, fake irq active? %s\n",
- yesno(intel_engine_has_waiter(engine)),
+ seq_printf(m, "\tfake irq active? %s\n",
yesno(test_bit(engine->id,
&dev_priv->gpu_error.missed_irq_rings)));
- spin_lock_irq(&b->rb_lock);
- for (rb = rb_first(&b->waiters); rb; rb = rb_next(rb)) {
- struct intel_wait *w = rb_entry(rb, typeof(*w), node);
-
- seq_printf(m, "\t%s [%d] waiting for %x\n",
- w->tsk->comm, w->tsk->pid, w->seqno);
- }
- spin_unlock_irq(&b->rb_lock);
-
seq_printf(m, "\tACTHD = 0x%08llx [current 0x%08llx]\n",
(long long)engine->hangcheck.acthd,
(long long)acthd[id]);
@@ -2022,18 +2009,6 @@ static int i915_swizzle_info(struct seq_file *m, void *data)
return 0;
}
-static int count_irq_waiters(struct drm_i915_private *i915)
-{
- struct intel_engine_cs *engine;
- enum intel_engine_id id;
- int count = 0;
-
- for_each_engine(engine, i915, id)
- count += intel_engine_has_waiter(engine);
-
- return count;
-}
-
static const char *rps_power_to_str(unsigned int power)
{
static const char * const strings[] = {
@@ -2073,7 +2048,6 @@ static int i915_rps_boost_info(struct seq_file *m, void *data)
seq_printf(m, "RPS enabled? %d\n", rps->enabled);
seq_printf(m, "GPU busy? %s [%d requests]\n",
yesno(dev_priv->gt.awake), dev_priv->gt.active_requests);
- seq_printf(m, "CPU waiting? %d\n", count_irq_waiters(dev_priv));
seq_printf(m, "Boosts outstanding? %d\n",
atomic_read(&rps->num_waiters));
seq_printf(m, "Interactive? %d\n", READ_ONCE(rps->power.interactive));
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 93e84751370f..6faf1f6faab5 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -327,6 +327,9 @@ intel_context_init(struct intel_context *ce,
struct intel_engine_cs *engine)
{
ce->gem_context = ctx;
+
+ INIT_LIST_HEAD(&ce->signal_link);
+ INIT_LIST_HEAD(&ce->signals);
}
static struct i915_gem_context *
diff --git a/drivers/gpu/drm/i915/i915_gem_context.h b/drivers/gpu/drm/i915/i915_gem_context.h
index 3769438228f6..6ba40ff6b91f 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.h
+++ b/drivers/gpu/drm/i915/i915_gem_context.h
@@ -164,6 +164,8 @@ struct i915_gem_context {
struct intel_context {
struct i915_gem_context *gem_context;
struct intel_engine_cs *active;
+ struct list_head signal_link;
+ struct list_head signals;
struct i915_vma *state;
struct intel_ring *ring;
u32 *lrc_reg_state;
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 898e06014295..304a7ef7f7fb 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -447,9 +447,14 @@ static void error_print_request(struct drm_i915_error_state_buf *m,
if (!erq->seqno)
return;
- err_printf(m, "%s pid %d, ban score %d, seqno %8x:%08x, prio %d, emitted %dms, start %08x, head %08x, tail %08x\n",
+ err_printf(m, "%s pid %d, ban score %d, seqno %8x:%08x%s%s, prio %d, emitted %dms, start %08x, head %08x, tail %08x\n",
prefix, erq->pid, erq->ban_score,
- erq->context, erq->seqno, erq->sched_attr.priority,
+ erq->context, erq->seqno,
+ test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
+ &erq->flags) ? "!" : "",
+ test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
+ &erq->flags) ? "+" : "",
+ erq->sched_attr.priority,
jiffies_to_msecs(erq->jiffies - epoch),
erq->start, erq->head, erq->tail);
}
@@ -530,7 +535,6 @@ static void error_print_engine(struct drm_i915_error_state_buf *m,
}
err_printf(m, " seqno: 0x%08x\n", ee->seqno);
err_printf(m, " last_seqno: 0x%08x\n", ee->last_seqno);
- err_printf(m, " waiting: %s\n", yesno(ee->waiting));
err_printf(m, " ring->head: 0x%08x\n", ee->cpu_ring_head);
err_printf(m, " ring->tail: 0x%08x\n", ee->cpu_ring_tail);
err_printf(m, " hangcheck timestamp: %dms (%lu%s)\n",
@@ -804,21 +808,6 @@ static void __err_print_to_sgl(struct drm_i915_error_state_buf *m,
error->epoch);
}
- if (IS_ERR(ee->waiters)) {
- err_printf(m, "%s --- ? waiters [unable to acquire spinlock]\n",
- m->i915->engine[i]->name);
- } else if (ee->num_waiters) {
- err_printf(m, "%s --- %d waiters\n",
- m->i915->engine[i]->name,
- ee->num_waiters);
- for (j = 0; j < ee->num_waiters; j++) {
- err_printf(m, " seqno 0x%08x for %s [%d]\n",
- ee->waiters[j].seqno,
- ee->waiters[j].comm,
- ee->waiters[j].pid);
- }
- }
-
print_error_obj(m, m->i915->engine[i],
"ringbuffer", ee->ringbuffer);
@@ -1000,8 +989,6 @@ void __i915_gpu_state_free(struct kref *error_ref)
i915_error_object_free(ee->wa_ctx);
kfree(ee->requests);
- if (!IS_ERR_OR_NULL(ee->waiters))
- kfree(ee->waiters);
}
for (i = 0; i < ARRAY_SIZE(error->active_bo); i++)
@@ -1205,59 +1192,6 @@ static void gen6_record_semaphore_state(struct intel_engine_cs *engine,
I915_READ(RING_SYNC_2(engine->mmio_base));
}
-static void error_record_engine_waiters(struct intel_engine_cs *engine,
- struct drm_i915_error_engine *ee)
-{
- struct intel_breadcrumbs *b = &engine->breadcrumbs;
- struct drm_i915_error_waiter *waiter;
- struct rb_node *rb;
- int count;
-
- ee->num_waiters = 0;
- ee->waiters = NULL;
-
- if (RB_EMPTY_ROOT(&b->waiters))
- return;
-
- if (!spin_trylock_irq(&b->rb_lock)) {
- ee->waiters = ERR_PTR(-EDEADLK);
- return;
- }
-
- count = 0;
- for (rb = rb_first(&b->waiters); rb != NULL; rb = rb_next(rb))
- count++;
- spin_unlock_irq(&b->rb_lock);
-
- waiter = NULL;
- if (count)
- waiter = kmalloc_array(count,
- sizeof(struct drm_i915_error_waiter),
- GFP_ATOMIC);
- if (!waiter)
- return;
-
- if (!spin_trylock_irq(&b->rb_lock)) {
- kfree(waiter);
- ee->waiters = ERR_PTR(-EDEADLK);
- return;
- }
-
- ee->waiters = waiter;
- for (rb = rb_first(&b->waiters); rb; rb = rb_next(rb)) {
- struct intel_wait *w = rb_entry(rb, typeof(*w), node);
-
- strcpy(waiter->comm, w->tsk->comm);
- waiter->pid = w->tsk->pid;
- waiter->seqno = w->seqno;
- waiter++;
-
- if (++ee->num_waiters == count)
- break;
- }
- spin_unlock_irq(&b->rb_lock);
-}
-
static void error_record_engine_registers(struct i915_gpu_state *error,
struct intel_engine_cs *engine,
struct drm_i915_error_engine *ee)
@@ -1293,7 +1227,6 @@ static void error_record_engine_registers(struct i915_gpu_state *error,
intel_engine_get_instdone(engine, &ee->instdone);
- ee->waiting = intel_engine_has_waiter(engine);
ee->instpm = I915_READ(RING_INSTPM(engine->mmio_base));
ee->acthd = intel_engine_get_active_head(engine);
ee->seqno = intel_engine_get_seqno(engine);
@@ -1367,6 +1300,7 @@ static void record_request(struct i915_request *request,
{
struct i915_gem_context *ctx = request->gem_context;
+ erq->flags = request->fence.flags;
erq->context = ctx->hw_id;
erq->sched_attr = request->sched.attr;
erq->ban_score = atomic_read(&ctx->ban_score);
@@ -1542,7 +1476,6 @@ static void gem_record_rings(struct i915_gpu_state *error)
ee->engine_id = i;
error_record_engine_registers(error, engine, ee);
- error_record_engine_waiters(engine, ee);
error_record_engine_execlists(engine, ee);
request = i915_gem_find_active_request(engine);
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.h b/drivers/gpu/drm/i915/i915_gpu_error.h
index 231173786eae..74757c424aab 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.h
+++ b/drivers/gpu/drm/i915/i915_gpu_error.h
@@ -82,8 +82,6 @@ struct i915_gpu_state {
int engine_id;
/* Software tracked state */
bool idle;
- bool waiting;
- int num_waiters;
unsigned long hangcheck_timestamp;
struct i915_address_space *vm;
int num_requests;
@@ -147,6 +145,7 @@ struct i915_gpu_state {
struct drm_i915_error_object *default_state;
struct drm_i915_error_request {
+ unsigned long flags;
long jiffies;
pid_t pid;
u32 context;
@@ -159,12 +158,6 @@ struct i915_gpu_state {
} *requests, execlist[EXECLIST_MAX_PORTS];
unsigned int num_ports;
- struct drm_i915_error_waiter {
- char comm[TASK_COMM_LEN];
- pid_t pid;
- u32 seqno;
- } *waiters;
-
struct {
u32 gfx_mode;
union {
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index e1d2312ec937..4b23b2fd1fad 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -28,12 +28,14 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-#include <linux/sysrq.h>
-#include <linux/slab.h>
#include <linux/circ_buf.h>
+#include <linux/slab.h>
+#include <linux/sysrq.h>
+
#include <drm/drm_irq.h>
#include <drm/drm_drv.h>
#include <drm/i915_drm.h>
+
#include "i915_drv.h"
#include "i915_trace.h"
#include "intel_drv.h"
@@ -1171,66 +1173,6 @@ static void ironlake_rps_change_irq_handler(struct drm_i915_private *dev_priv)
return;
}
-static void notify_ring(struct intel_engine_cs *engine)
-{
- const u32 seqno = intel_engine_get_seqno(engine);
- struct i915_request *rq = NULL;
- struct task_struct *tsk = NULL;
- struct intel_wait *wait;
-
- if (unlikely(!engine->breadcrumbs.irq_armed))
- return;
-
- rcu_read_lock();
-
- spin_lock(&engine->breadcrumbs.irq_lock);
- wait = engine->breadcrumbs.irq_wait;
- if (wait) {
- /*
- * We use a callback from the dma-fence to submit
- * requests after waiting on our own requests. To
- * ensure minimum delay in queuing the next request to
- * hardware, signal the fence now rather than wait for
- * the signaler to be woken up. We still wake up the
- * waiter in order to handle the irq-seqno coherency
- * issues (we may receive the interrupt before the
- * seqno is written, see __i915_request_irq_complete())
- * and to handle coalescing of multiple seqno updates
- * and many waiters.
- */
- if (i915_seqno_passed(seqno, wait->seqno)) {
- struct i915_request *waiter = wait->request;
-
- if (waiter &&
- !i915_request_signaled(waiter) &&
- intel_wait_check_request(wait, waiter))
- rq = i915_request_get(waiter);
-
- tsk = wait->tsk;
- }
-
- engine->breadcrumbs.irq_count++;
- } else {
- if (engine->breadcrumbs.irq_armed)
- __intel_engine_disarm_breadcrumbs(engine);
- }
- spin_unlock(&engine->breadcrumbs.irq_lock);
-
- if (rq) {
- spin_lock(&rq->lock);
- dma_fence_signal_locked(&rq->fence);
- GEM_BUG_ON(!i915_request_completed(rq));
- spin_unlock(&rq->lock);
-
- i915_request_put(rq);
- }
-
- if (tsk && tsk->state & TASK_NORMAL)
- wake_up_process(tsk);
-
- rcu_read_unlock();
-}
-
static void vlv_c0_read(struct drm_i915_private *dev_priv,
struct intel_rps_ei *ei)
{
@@ -1475,20 +1417,20 @@ static void ilk_gt_irq_handler(struct drm_i915_private *dev_priv,
u32 gt_iir)
{
if (gt_iir & GT_RENDER_USER_INTERRUPT)
- notify_ring(dev_priv->engine[RCS]);
+ intel_engine_breadcrumbs_irq(dev_priv->engine[RCS]);
if (gt_iir & ILK_BSD_USER_INTERRUPT)
- notify_ring(dev_priv->engine[VCS]);
+ intel_engine_breadcrumbs_irq(dev_priv->engine[VCS]);
}
static void snb_gt_irq_handler(struct drm_i915_private *dev_priv,
u32 gt_iir)
{
if (gt_iir & GT_RENDER_USER_INTERRUPT)
- notify_ring(dev_priv->engine[RCS]);
+ intel_engine_breadcrumbs_irq(dev_priv->engine[RCS]);
if (gt_iir & GT_BSD_USER_INTERRUPT)
- notify_ring(dev_priv->engine[VCS]);
+ intel_engine_breadcrumbs_irq(dev_priv->engine[VCS]);
if (gt_iir & GT_BLT_USER_INTERRUPT)
- notify_ring(dev_priv->engine[BCS]);
+ intel_engine_breadcrumbs_irq(dev_priv->engine[BCS]);
if (gt_iir & (GT_BLT_CS_ERROR_INTERRUPT |
GT_BSD_CS_ERROR_INTERRUPT |
@@ -1508,7 +1450,7 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 iir)
tasklet = true;
if (iir & GT_RENDER_USER_INTERRUPT) {
- notify_ring(engine);
+ intel_engine_breadcrumbs_irq(engine);
tasklet |= USES_GUC_SUBMISSION(engine->i915);
}
@@ -1854,7 +1796,7 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
if (HAS_VEBOX(dev_priv)) {
if (pm_iir & PM_VEBOX_USER_INTERRUPT)
- notify_ring(dev_priv->engine[VECS]);
+ intel_engine_breadcrumbs_irq(dev_priv->engine[VECS]);
if (pm_iir & PM_VEBOX_CS_ERROR_INTERRUPT)
DRM_DEBUG("Command parser error, pm_iir 0x%08x\n", pm_iir);
@@ -4278,7 +4220,7 @@ static irqreturn_t i8xx_irq_handler(int irq, void *arg)
I915_WRITE16(IIR, iir);
if (iir & I915_USER_INTERRUPT)
- notify_ring(dev_priv->engine[RCS]);
+ intel_engine_breadcrumbs_irq(dev_priv->engine[RCS]);
if (iir & I915_MASTER_ERROR_INTERRUPT)
i8xx_error_irq_handler(dev_priv, eir, eir_stuck);
@@ -4386,7 +4328,7 @@ static irqreturn_t i915_irq_handler(int irq, void *arg)
I915_WRITE(IIR, iir);
if (iir & I915_USER_INTERRUPT)
- notify_ring(dev_priv->engine[RCS]);
+ intel_engine_breadcrumbs_irq(dev_priv->engine[RCS]);
if (iir & I915_MASTER_ERROR_INTERRUPT)
i9xx_error_irq_handler(dev_priv, eir, eir_stuck);
@@ -4531,10 +4473,10 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)
I915_WRITE(IIR, iir);
if (iir & I915_USER_INTERRUPT)
- notify_ring(dev_priv->engine[RCS]);
+ intel_engine_breadcrumbs_irq(dev_priv->engine[RCS]);
if (iir & I915_BSD_USER_INTERRUPT)
- notify_ring(dev_priv->engine[VCS]);
+ intel_engine_breadcrumbs_irq(dev_priv->engine[VCS]);
if (iir & I915_MASTER_ERROR_INTERRUPT)
i9xx_error_irq_handler(dev_priv, eir, eir_stuck);
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 7db15b7b3de8..9ed5baf157a3 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -60,7 +60,7 @@ static bool i915_fence_signaled(struct dma_fence *fence)
static bool i915_fence_enable_signaling(struct dma_fence *fence)
{
- return intel_engine_enable_signaling(to_request(fence), true);
+ return i915_request_enable_breadcrumb(to_request(fence));
}
static signed long i915_fence_wait(struct dma_fence *fence,
@@ -203,7 +203,7 @@ static void __retire_engine_request(struct intel_engine_cs *engine,
if (!i915_request_signaled(rq))
dma_fence_signal_locked(&rq->fence);
if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &rq->fence.flags))
- intel_engine_cancel_signaling(rq);
+ i915_request_cancel_breadcrumb(rq);
if (rq->waitboost) {
GEM_BUG_ON(!atomic_read(&rq->i915->gt_pm.rps.num_waiters));
atomic_dec(&rq->i915->gt_pm.rps.num_waiters);
@@ -377,9 +377,12 @@ void __i915_request_submit(struct i915_request *request)
/* We may be recursing from the signal callback of another i915 fence */
spin_lock_nested(&request->lock, SINGLE_DEPTH_NESTING);
+ GEM_BUG_ON(test_bit(I915_FENCE_FLAG_ACTIVE, &request->fence.flags));
+ set_bit(I915_FENCE_FLAG_ACTIVE, &request->fence.flags);
request->global_seqno = seqno;
- if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &request->fence.flags))
- intel_engine_enable_signaling(request, false);
+ if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &request->fence.flags) &&
+ !i915_request_enable_breadcrumb(request))
+ intel_engine_queue_breadcrumbs(engine);
spin_unlock(&request->lock);
engine->emit_fini_breadcrumb(request,
@@ -389,8 +392,6 @@ void __i915_request_submit(struct i915_request *request)
move_to_timeline(request, &engine->timeline);
trace_i915_request_execute(request);
-
- wake_up_all(&request->execute);
}
void i915_request_submit(struct i915_request *request)
@@ -433,7 +434,9 @@ void __i915_request_unsubmit(struct i915_request *request)
spin_lock_nested(&request->lock, SINGLE_DEPTH_NESTING);
request->global_seqno = 0;
if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &request->fence.flags))
- intel_engine_cancel_signaling(request);
+ i915_request_cancel_breadcrumb(request);
+ GEM_BUG_ON(!test_bit(I915_FENCE_FLAG_ACTIVE, &request->fence.flags));
+ clear_bit(I915_FENCE_FLAG_ACTIVE, &request->fence.flags);
spin_unlock(&request->lock);
/* Transfer back from the global per-engine timeline to per-context */
@@ -633,13 +636,11 @@ i915_request_alloc(struct intel_engine_cs *engine, struct i915_gem_context *ctx)
/* We bump the ref for the fence chain */
i915_sw_fence_init(&i915_request_get(rq)->submit, submit_notify);
- init_waitqueue_head(&rq->execute);
i915_sched_node_init(&rq->sched);
/* No zalloc, must clear what we need by hand */
rq->global_seqno = 0;
- rq->signaling.wait.seqno = 0;
rq->file_priv = NULL;
rq->batch = NULL;
rq->capture_list = NULL;
@@ -1030,13 +1031,10 @@ static bool busywait_stop(unsigned long timeout, unsigned int cpu)
return this_cpu != cpu;
}
-static bool __i915_spin_request(const struct i915_request *rq,
- u32 seqno, int state, unsigned long timeout_us)
+static bool __i915_spin_request(const struct i915_request * const rq,
+ int state, unsigned long timeout_us)
{
- struct intel_engine_cs *engine = rq->engine;
- unsigned int irq, cpu;
-
- GEM_BUG_ON(!seqno);
+ unsigned int cpu;
/*
* Only wait for the request if we know it is likely to complete.
@@ -1044,12 +1042,12 @@ static bool __i915_spin_request(const struct i915_request *rq,
* We don't track the timestamps around requests, nor the average
* request length, so we do not have a good indicator that this
* request will complete within the timeout. What we do know is the
- * order in which requests are executed by the engine and so we can
- * tell if the request has started. If the request hasn't started yet,
- * it is a fair assumption that it will not complete within our
- * relatively short timeout.
+ * order in which requests are executed by the context and so we can
+ * tell if the request has been started. If the request is not even
+ * running yet, it is a fair assumption that it will not complete
+ * within our relatively short timeout.
*/
- if (!intel_engine_has_started(engine, seqno))
+ if (!i915_request_is_running(rq))
return false;
/*
@@ -1063,20 +1061,10 @@ static bool __i915_spin_request(const struct i915_request *rq,
* takes to sleep on a request, on the order of a microsecond.
*/
- irq = READ_ONCE(engine->breadcrumbs.irq_count);
timeout_us += local_clock_us(&cpu);
do {
- if (intel_engine_has_completed(engine, seqno))
- return seqno == i915_request_global_seqno(rq);
-
- /*
- * Seqno are meant to be ordered *before* the interrupt. If
- * we see an interrupt without a corresponding seqno advance,
- * assume we won't see one in the near future but require
- * the engine->seqno_barrier() to fixup coherency.
- */
- if (READ_ONCE(engine->breadcrumbs.irq_count) != irq)
- break;
+ if (i915_request_completed(rq))
+ return true;
if (signal_pending_state(state, current))
break;
@@ -1090,6 +1078,18 @@ static bool __i915_spin_request(const struct i915_request *rq,
return false;
}
+struct request_wait {
+ struct dma_fence_cb cb;
+ struct task_struct *tsk;
+};
+
+static void request_wait_wake(struct dma_fence *fence, struct dma_fence_cb *cb)
+{
+ struct request_wait *wait = container_of(cb, typeof(*wait), cb);
+
+ wake_up_process(wait->tsk);
+}
+
/**
* i915_request_wait - wait until execution of request has finished
* @rq: the request to wait upon
@@ -1115,8 +1115,7 @@ long i915_request_wait(struct i915_request *rq,
{
const int state = flags & I915_WAIT_INTERRUPTIBLE ?
TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE;
- DEFINE_WAIT_FUNC(exec, default_wake_function);
- struct intel_wait wait;
+ struct request_wait wait;
might_sleep();
GEM_BUG_ON(timeout < 0);
@@ -1128,47 +1127,24 @@ long i915_request_wait(struct i915_request *rq,
return -ETIME;
trace_i915_request_wait_begin(rq, flags);
- add_wait_queue(&rq->execute, &exec);
- intel_wait_init(&wait);
- if (flags & I915_WAIT_PRIORITY)
- i915_schedule_bump_priority(rq, I915_PRIORITY_WAIT);
-
-restart:
- do {
- set_current_state(state);
- if (intel_wait_update_request(&wait, rq))
- break;
-
- if (signal_pending_state(state, current)) {
- timeout = -ERESTARTSYS;
- goto complete;
- }
- if (!timeout) {
- timeout = -ETIME;
- goto complete;
- }
+ /* Optimistic short spin before touching IRQs */
+ if (__i915_spin_request(rq, state, 5))
+ goto out;
- timeout = io_schedule_timeout(timeout);
- } while (1);
+ if (flags & I915_WAIT_PRIORITY)
+ i915_schedule_bump_priority(rq, I915_PRIORITY_WAIT);
- GEM_BUG_ON(!intel_wait_has_seqno(&wait));
- GEM_BUG_ON(!i915_sw_fence_signaled(&rq->submit));
+ wait.tsk = current;
+ if (dma_fence_add_callback(&rq->fence, &wait.cb, request_wait_wake))
+ goto out;
- /* Optimistic short spin before touching IRQs */
- if (__i915_spin_request(rq, wait.seqno, state, 5))
- goto complete;
+ for (;;) {
+ set_current_state(state);
- set_current_state(state);
- if (intel_engine_add_wait(rq->engine, &wait))
- /*
- * In order to check that we haven't missed the interrupt
- * as we enabled it, we need to kick ourselves to do a
- * coherent check on the seqno before we sleep.
- */
- goto wakeup;
+ if (i915_request_completed(rq))
+ break;
- for (;;) {
if (signal_pending_state(state, current)) {
timeout = -ERESTARTSYS;
break;
@@ -1180,33 +1156,13 @@ long i915_request_wait(struct i915_request *rq,
}
timeout = io_schedule_timeout(timeout);
-
- if (intel_wait_complete(&wait) &&
- intel_wait_check_request(&wait, rq))
- break;
-
- set_current_state(state);
-
-wakeup:
- if (i915_request_completed(rq))
- break;
-
- /* Only spin if we know the GPU is processing this request */
- if (__i915_spin_request(rq, wait.seqno, state, 2))
- break;
-
- if (!intel_wait_check_request(&wait, rq)) {
- intel_engine_remove_wait(rq->engine, &wait);
- goto restart;
- }
}
-
- intel_engine_remove_wait(rq->engine, &wait);
-complete:
__set_current_state(TASK_RUNNING);
- remove_wait_queue(&rq->execute, &exec);
- trace_i915_request_wait_end(rq);
+ dma_fence_remove_callback(&rq->fence, &wait.cb);
+
+out:
+ trace_i915_request_wait_end(rq);
return timeout;
}
diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h
index 340d6216791c..3cffb96203b9 100644
--- a/drivers/gpu/drm/i915/i915_request.h
+++ b/drivers/gpu/drm/i915/i915_request.h
@@ -38,23 +38,34 @@ struct drm_i915_gem_object;
struct i915_request;
struct i915_timeline;
-struct intel_wait {
- struct rb_node node;
- struct task_struct *tsk;
- struct i915_request *request;
- u32 seqno;
-};
-
-struct intel_signal_node {
- struct intel_wait wait;
- struct list_head link;
-};
-
struct i915_capture_list {
struct i915_capture_list *next;
struct i915_vma *vma;
};
+enum {
+ /*
+ * I915_FENCE_FLAG_ACTIVE - this request is currently submitted to HW.
+ *
+ * Set by __i915_request_submit() on handing over to HW, and cleared
+ * by __i915_request_unsubmit() if we preempt this request.
+ *
+ * Finally cleared for consistency on retiring the request, when
+ * we know the HW is no longer running this request.
+ *
+ * See i915_request_is_active()
+ */
+ I915_FENCE_FLAG_ACTIVE = DMA_FENCE_FLAG_USER_BITS,
+
+ /*
+ * I915_FENCE_FLAG_SIGNAL - this request is currently on signal_list
+ *
+ * Internal bookkeeping used by the breadcrumb code to track when
+ * a request is on the various signal_list.
+ */
+ I915_FENCE_FLAG_SIGNAL,
+};
+
/**
* Request queue structure.
*
@@ -97,7 +108,7 @@ struct i915_request {
struct intel_context *hw_context;
struct intel_ring *ring;
struct i915_timeline *timeline;
- struct intel_signal_node signaling;
+ struct list_head signal_link;
/*
* The rcu epoch of when this request was allocated. Used to judiciously
@@ -116,7 +127,6 @@ struct i915_request {
*/
struct i915_sw_fence submit;
wait_queue_entry_t submitq;
- wait_queue_head_t execute;
/*
* A list of everyone we wait upon, and everyone who waits upon us.
@@ -255,7 +265,7 @@ i915_request_put(struct i915_request *rq)
* that it has passed the global seqno and the global seqno is unchanged
* after the read, it is indeed complete).
*/
-static u32
+static inline u32
i915_request_global_seqno(const struct i915_request *request)
{
return READ_ONCE(request->global_seqno);
@@ -277,6 +287,10 @@ void i915_request_skip(struct i915_request *request, int error);
void __i915_request_unsubmit(struct i915_request *request);
void i915_request_unsubmit(struct i915_request *request);
+/* Note: part of the intel_breadcrumbs family */
+bool i915_request_enable_breadcrumb(struct i915_request *request);
+void i915_request_cancel_breadcrumb(struct i915_request *request);
+
long i915_request_wait(struct i915_request *rq,
unsigned int flags,
long timeout)
@@ -293,6 +307,11 @@ static inline bool i915_request_signaled(const struct i915_request *rq)
return test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &rq->fence.flags);
}
+static inline bool i915_request_is_active(const struct i915_request *rq)
+{
+ return test_bit(I915_FENCE_FLAG_ACTIVE, &rq->fence.flags);
+}
+
/**
* Returns true if seq1 is later than seq2.
*/
@@ -330,6 +349,11 @@ static inline u32 hwsp_seqno(const struct i915_request *rq)
return seqno;
}
+static inline bool __i915_request_has_started(const struct i915_request *rq)
+{
+ return i915_seqno_passed(hwsp_seqno(rq), rq->fence.seqno - 1);
+}
+
/**
* i915_request_started - check if the request has begun being executed
* @rq: the request
@@ -345,7 +369,23 @@ static inline bool i915_request_started(const struct i915_request *rq)
return true;
/* Remember: started but may have since been preempted! */
- return i915_seqno_passed(hwsp_seqno(rq), rq->fence.seqno - 1);
+ return __i915_request_has_started(rq);
+}
+
+/**
+ * i915_request_is_running - check if the request may actually be executing
+ * @rq: the request
+ *
+ * Returns true if the request is currently submitted to hardware, has passed
+ * its start point (i.e. the context is setup and not busywaiting). Note that
+ * it may no longer be running by the time the function returns!
+ */
+static inline bool i915_request_is_running(const struct i915_request *rq)
+{
+ if (!i915_request_is_active(rq))
+ return false;
+
+ return __i915_request_has_started(rq);
}
static inline bool i915_request_completed(const struct i915_request *rq)
diff --git a/drivers/gpu/drm/i915/i915_reset.c b/drivers/gpu/drm/i915/i915_reset.c
index acf3c777e49d..4462007a681c 100644
--- a/drivers/gpu/drm/i915/i915_reset.c
+++ b/drivers/gpu/drm/i915/i915_reset.c
@@ -29,7 +29,7 @@ static void engine_skip_context(struct i915_request *rq)
spin_lock(&timeline->lock);
- if (rq->global_seqno) {
+ if (i915_request_is_active(rq)) {
list_for_each_entry_continue(rq,
&engine->timeline.requests, link)
if (rq->gem_context == hung_ctx)
@@ -751,18 +751,20 @@ static void reset_restart(struct drm_i915_private *i915)
static void nop_submit_request(struct i915_request *request)
{
+ struct intel_engine_cs *engine = request->engine;
unsigned long flags;
GEM_TRACE("%s fence %llx:%lld -> -EIO\n",
- request->engine->name,
- request->fence.context, request->fence.seqno);
+ engine->name, request->fence.context, request->fence.seqno);
dma_fence_set_error(&request->fence, -EIO);
- spin_lock_irqsave(&request->engine->timeline.lock, flags);
+ spin_lock_irqsave(&engine->timeline.lock, flags);
__i915_request_submit(request);
i915_request_mark_complete(request);
- intel_engine_write_global_seqno(request->engine, request->global_seqno);
- spin_unlock_irqrestore(&request->engine->timeline.lock, flags);
+ intel_engine_write_global_seqno(engine, request->global_seqno);
+ spin_unlock_irqrestore(&engine->timeline.lock, flags);
+
+ intel_engine_queue_breadcrumbs(engine);
}
void i915_gem_set_wedged(struct drm_i915_private *i915)
@@ -817,7 +819,7 @@ void i915_gem_set_wedged(struct drm_i915_private *i915)
for_each_engine(engine, i915, id) {
reset_finish_engine(engine);
- intel_engine_wakeup(engine);
+ intel_engine_signal_breadcrumbs(engine);
}
smp_mb__before_atomic();
diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
index 7db1255665a8..8ae68d2fc134 100644
--- a/drivers/gpu/drm/i915/i915_scheduler.c
+++ b/drivers/gpu/drm/i915/i915_scheduler.c
@@ -243,7 +243,7 @@ static bool inflight(const struct i915_request *rq,
{
const struct i915_request *active;
- if (!rq->global_seqno)
+ if (!i915_request_is_active(rq))
return false;
active = port_request(engine->execlists.port);
diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index b58915b8708b..b0795b0ad227 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -29,48 +29,149 @@
#define task_asleep(tsk) ((tsk)->state & TASK_NORMAL && !(tsk)->on_rq)
-static unsigned int __intel_breadcrumbs_wakeup(struct intel_breadcrumbs *b)
+static void irq_enable(struct intel_engine_cs *engine)
+{
+ if (!engine->irq_enable)
+ return;
+
+ /* Caller disables interrupts */
+ spin_lock(&engine->i915->irq_lock);
+ engine->irq_enable(engine);
+ spin_unlock(&engine->i915->irq_lock);
+}
+
+static void irq_disable(struct intel_engine_cs *engine)
{
- struct intel_wait *wait;
- unsigned int result = 0;
+ if (!engine->irq_disable)
+ return;
+
+ /* Caller disables interrupts */
+ spin_lock(&engine->i915->irq_lock);
+ engine->irq_disable(engine);
+ spin_unlock(&engine->i915->irq_lock);
+}
+static void __intel_breadcrumbs_disarm_irq(struct intel_breadcrumbs *b)
+{
lockdep_assert_held(&b->irq_lock);
- wait = b->irq_wait;
- if (wait) {
+ GEM_BUG_ON(!b->irq_enabled);
+ if (!--b->irq_enabled)
+ irq_disable(container_of(b,
+ struct intel_engine_cs,
+ breadcrumbs));
+
+ b->irq_armed = false;
+}
+
+void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
+{
+ struct intel_breadcrumbs *b = &engine->breadcrumbs;
+
+ if (!b->irq_armed)
+ return;
+
+ spin_lock_irq(&b->irq_lock);
+ if (b->irq_armed)
+ __intel_breadcrumbs_disarm_irq(b);
+ spin_unlock_irq(&b->irq_lock);
+}
+
+static inline bool __request_completed(const struct i915_request *rq)
+{
+ return i915_seqno_passed(__hwsp_seqno(rq), rq->fence.seqno);
+}
+
+bool intel_engine_breadcrumbs_irq(struct intel_engine_cs *engine)
+{
+ struct intel_breadcrumbs *b = &engine->breadcrumbs;
+ struct intel_context *ce, *cn;
+ struct list_head *pos, *next;
+ LIST_HEAD(signal);
+
+ spin_lock(&b->irq_lock);
+
+ b->irq_fired = true;
+ if (b->irq_armed && list_empty(&b->signalers))
+ __intel_breadcrumbs_disarm_irq(b);
+
+ list_for_each_entry_safe(ce, cn, &b->signalers, signal_link) {
+ GEM_BUG_ON(list_empty(&ce->signals));
+
+ list_for_each_safe(pos, next, &ce->signals) {
+ struct i915_request *rq =
+ list_entry(pos, typeof(*rq), signal_link);
+
+ if (!__request_completed(rq))
+ break;
+
+ GEM_BUG_ON(!test_bit(I915_FENCE_FLAG_SIGNAL,
+ &rq->fence.flags));
+ clear_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
+
+ /*
+ * We may race with direct invocation of
+ * dma_fence_signal(), e.g. i915_request_retire(),
+ * in which case we can skip processing it ourselves.
+ */
+ if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
+ &rq->fence.flags))
+ continue;
+
+ /*
+ * Queue for execution after dropping the signaling
+ * spinlock as the callback chain may end up adding
+ * more signalers to the same context or engine.
+ */
+ i915_request_get(rq);
+ list_add_tail(&rq->signal_link, &signal);
+ }
+
/*
- * N.B. Since task_asleep() and ttwu are not atomic, the
- * waiter may actually go to sleep after the check, causing
- * us to suppress a valid wakeup. We prefer to reduce the
- * number of false positive missed_breadcrumb() warnings
- * at the expense of a few false negatives, as it it easy
- * to trigger a false positive under heavy load. Enough
- * signal should remain from genuine missed_breadcrumb()
- * for us to detect in CI.
+ * We process the list deletion in bulk, only using a list_add
+ * (not list_move) above but keeping the status of
+ * rq->signal_link known with the I915_FENCE_FLAG_SIGNAL bit.
*/
- bool was_asleep = task_asleep(wait->tsk);
+ if (!list_is_first(pos, &ce->signals)) {
+ /* Advance the list to the first incomplete request */
+ __list_del_many(&ce->signals, pos);
+ if (&ce->signals == pos) /* now empty */
+ list_del_init(&ce->signal_link);
+ }
+ }
+
+ spin_unlock(&b->irq_lock);
+
+ list_for_each_safe(pos, next, &signal) {
+ struct i915_request *rq =
+ list_entry(pos, typeof(*rq), signal_link);
- result = ENGINE_WAKEUP_WAITER;
- if (wake_up_process(wait->tsk) && was_asleep)
- result |= ENGINE_WAKEUP_ASLEEP;
+ dma_fence_signal(&rq->fence);
+ i915_request_put(rq);
}
- return result;
+ return !list_empty(&signal);
}
-unsigned int intel_engine_wakeup(struct intel_engine_cs *engine)
+bool intel_engine_signal_breadcrumbs(struct intel_engine_cs *engine)
{
- struct intel_breadcrumbs *b = &engine->breadcrumbs;
- unsigned long flags;
- unsigned int result;
+ bool result;
- spin_lock_irqsave(&b->irq_lock, flags);
- result = __intel_breadcrumbs_wakeup(b);
- spin_unlock_irqrestore(&b->irq_lock, flags);
+ local_irq_disable();
+ result = intel_engine_breadcrumbs_irq(engine);
+ local_irq_enable();
return result;
}
+static void signal_irq_work(struct irq_work *work)
+{
+ struct intel_engine_cs *engine =
+ container_of(work, typeof(*engine), breadcrumbs.irq_work);
+
+ intel_engine_breadcrumbs_irq(engine);
+}
+
static unsigned long wait_timeout(void)
{
return round_jiffies_up(jiffies + DRM_I915_HANGCHECK_JIFFIES);
@@ -94,19 +195,15 @@ static void intel_breadcrumbs_hangcheck(struct timer_list *t)
struct intel_engine_cs *engine =
from_timer(engine, t, breadcrumbs.hangcheck);
struct intel_breadcrumbs *b = &engine->breadcrumbs;
- unsigned int irq_count;
if (!b->irq_armed)
return;
- irq_count = READ_ONCE(b->irq_count);
- if (b->hangcheck_interrupts != irq_count) {
- b->hangcheck_interrupts = irq_count;
- mod_timer(&b->hangcheck, wait_timeout());
- return;
- }
+ if (b->irq_fired)
+ goto rearm;
- /* We keep the hangcheck timer alive until we disarm the irq, even
+ /*
+ * We keep the hangcheck timer alive until we disarm the irq, even
* if there are no waiters at present.
*
* If the waiter was currently running, assume it hasn't had a chance
@@ -118,10 +215,13 @@ static void intel_breadcrumbs_hangcheck(struct timer_list *t)
* but we still have a waiter. Assuming all batches complete within
* DRM_I915_HANGCHECK_JIFFIES [1.5s]!
*/
- if (intel_engine_wakeup(engine) & ENGINE_WAKEUP_ASLEEP) {
+ synchronize_hardirq(engine->i915->drm.irq);
+ if (intel_engine_signal_breadcrumbs(engine)) {
missed_breadcrumb(engine);
mod_timer(&b->fake_irq, jiffies + 1);
} else {
+rearm:
+ b->irq_fired = false;
mod_timer(&b->hangcheck, wait_timeout());
}
}
@@ -140,11 +240,7 @@ static void intel_breadcrumbs_fake_irq(struct timer_list *t)
* oldest waiter to do the coherent seqno check.
*/
- spin_lock_irq(&b->irq_lock);
- if (b->irq_armed && !__intel_breadcrumbs_wakeup(b))
- __intel_engine_disarm_breadcrumbs(engine);
- spin_unlock_irq(&b->irq_lock);
- if (!b->irq_armed)
+ if (!intel_engine_signal_breadcrumbs(engine) && !b->irq_armed)
return;
/* If the user has disabled the fake-irq, restore the hangchecking */
@@ -156,43 +252,6 @@ static void intel_breadcrumbs_fake_irq(struct timer_list *t)
mod_timer(&b->fake_irq, jiffies + 1);
}
-static void irq_enable(struct intel_engine_cs *engine)
-{
- if (!engine->irq_enable)
- return;
-
- /* Caller disables interrupts */
- spin_lock(&engine->i915->irq_lock);
- engine->irq_enable(engine);
- spin_unlock(&engine->i915->irq_lock);
-}
-
-static void irq_disable(struct intel_engine_cs *engine)
-{
- if (!engine->irq_disable)
- return;
-
- /* Caller disables interrupts */
- spin_lock(&engine->i915->irq_lock);
- engine->irq_disable(engine);
- spin_unlock(&engine->i915->irq_lock);
-}
-
-void __intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
-{
- struct intel_breadcrumbs *b = &engine->breadcrumbs;
-
- lockdep_assert_held(&b->irq_lock);
- GEM_BUG_ON(b->irq_wait);
- GEM_BUG_ON(!b->irq_armed);
-
- GEM_BUG_ON(!b->irq_enabled);
- if (!--b->irq_enabled)
- irq_disable(engine);
-
- b->irq_armed = false;
-}
-
void intel_engine_pin_breadcrumbs_irq(struct intel_engine_cs *engine)
{
struct intel_breadcrumbs *b = &engine->breadcrumbs;
@@ -215,40 +274,6 @@ void intel_engine_unpin_breadcrumbs_irq(struct intel_engine_cs *engine)
spin_unlock_irq(&b->irq_lock);
}
-void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
-{
- struct intel_breadcrumbs *b = &engine->breadcrumbs;
- struct intel_wait *wait, *n;
-
- if (!b->irq_armed)
- return;
-
- /*
- * We only disarm the irq when we are idle (all requests completed),
- * so if the bottom-half remains asleep, it missed the request
- * completion.
- */
- if (intel_engine_wakeup(engine) & ENGINE_WAKEUP_ASLEEP)
- missed_breadcrumb(engine);
-
- spin_lock_irq(&b->rb_lock);
-
- spin_lock(&b->irq_lock);
- b->irq_wait = NULL;
- if (b->irq_armed)
- __intel_engine_disarm_breadcrumbs(engine);
- spin_unlock(&b->irq_lock);
-
- rbtree_postorder_for_each_entry_safe(wait, n, &b->waiters, node) {
- GEM_BUG_ON(!intel_engine_signaled(engine, wait->seqno));
- RB_CLEAR_NODE(&wait->node);
- wake_up_process(wait->tsk);
- }
- b->waiters = RB_ROOT;
-
- spin_unlock_irq(&b->rb_lock);
-}
-
static bool use_fake_irq(const struct intel_breadcrumbs *b)
{
const struct intel_engine_cs *engine =
@@ -264,7 +289,7 @@ static bool use_fake_irq(const struct intel_breadcrumbs *b)
* engine->seqno_barrier(), a timing error that should be transient
* and unlikely to reoccur.
*/
- return READ_ONCE(b->irq_count) == b->hangcheck_interrupts;
+ return !b->irq_fired;
}
static void enable_fake_irq(struct intel_breadcrumbs *b)
@@ -276,7 +301,7 @@ static void enable_fake_irq(struct intel_breadcrumbs *b)
mod_timer(&b->hangcheck, wait_timeout());
}
-static bool __intel_breadcrumbs_enable_irq(struct intel_breadcrumbs *b)
+static bool __intel_breadcrumbs_arm_irq(struct intel_breadcrumbs *b)
{
struct intel_engine_cs *engine =
container_of(b, struct intel_engine_cs, breadcrumbs);
@@ -315,536 +340,149 @@ static bool __intel_breadcrumbs_enable_irq(struct intel_breadcrumbs *b)
return enabled;
}
-static inline struct intel_wait *to_wait(struct rb_node *node)
+void intel_engine_init_breadcrumbs(struct intel_engine_cs *engine)
{
- return rb_entry(node, struct intel_wait, node);
-}
+ struct intel_breadcrumbs *b = &engine->breadcrumbs;
-static inline void __intel_breadcrumbs_finish(struct intel_breadcrumbs *b,
- struct intel_wait *wait)
-{
- lockdep_assert_held(&b->rb_lock);
- GEM_BUG_ON(b->irq_wait == wait);
+ spin_lock_init(&b->irq_lock);
+ INIT_LIST_HEAD(&b->signalers);
- /*
- * This request is completed, so remove it from the tree, mark it as
- * complete, and *then* wake up the associated task. N.B. when the
- * task wakes up, it will find the empty rb_node, discern that it
- * has already been removed from the tree and skip the serialisation
- * of the b->rb_lock and b->irq_lock. This means that the destruction
- * of the intel_wait is not serialised with the interrupt handler
- * by the waiter - it must instead be serialised by the caller.
- */
- rb_erase(&wait->node, &b->waiters);
- RB_CLEAR_NODE(&wait->node);
+ init_irq_work(&b->irq_work, signal_irq_work);
- if (wait->tsk->state != TASK_RUNNING)
- wake_up_process(wait->tsk); /* implicit smp_wmb() */
+ timer_setup(&b->fake_irq, intel_breadcrumbs_fake_irq, 0);
+ timer_setup(&b->hangcheck, intel_breadcrumbs_hangcheck, 0);
}
-static inline void __intel_breadcrumbs_next(struct intel_engine_cs *engine,
- struct rb_node *next)
+static void cancel_fake_irq(struct intel_engine_cs *engine)
{
struct intel_breadcrumbs *b = &engine->breadcrumbs;
- spin_lock(&b->irq_lock);
- GEM_BUG_ON(!b->irq_armed);
- GEM_BUG_ON(!b->irq_wait);
- b->irq_wait = to_wait(next);
- spin_unlock(&b->irq_lock);
-
- /* We always wake up the next waiter that takes over as the bottom-half
- * as we may delegate not only the irq-seqno barrier to the next waiter
- * but also the task of waking up concurrent waiters.
- */
- if (next)
- wake_up_process(to_wait(next)->tsk);
+ del_timer_sync(&b->fake_irq); /* may queue b->hangcheck */
+ del_timer_sync(&b->hangcheck);
+ clear_bit(engine->id, &engine->i915->gpu_error.missed_irq_rings);
}
-static bool __intel_engine_add_wait(struct intel_engine_cs *engine,
- struct intel_wait *wait)
+void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
{
struct intel_breadcrumbs *b = &engine->breadcrumbs;
- struct rb_node **p, *parent, *completed;
- bool first, armed;
- u32 seqno;
+ unsigned long flags;
- GEM_BUG_ON(!wait->seqno);
+ spin_lock_irqsave(&b->irq_lock, flags);
- /* Insert the request into the retirement ordered list
- * of waiters by walking the rbtree. If we are the oldest
- * seqno in the tree (the first to be retired), then
- * set ourselves as the bottom-half.
- *
- * As we descend the tree, prune completed branches since we hold the
- * spinlock we know that the first_waiter must be delayed and can
- * reduce some of the sequential wake up latency if we take action
- * ourselves and wake up the completed tasks in parallel. Also, by
- * removing stale elements in the tree, we may be able to reduce the
- * ping-pong between the old bottom-half and ourselves as first-waiter.
+ /*
+ * Leave the fake_irq timer enabled (if it is running), but clear the
+ * bit so that it turns itself off on its next wake up and goes back
+ * to the long hangcheck interval if still required.
*/
- armed = false;
- first = true;
- parent = NULL;
- completed = NULL;
- seqno = intel_engine_get_seqno(engine);
-
- /* If the request completed before we managed to grab the spinlock,
- * return now before adding ourselves to the rbtree. We let the
- * current bottom-half handle any pending wakeups and instead
- * try and get out of the way quickly.
- */
- if (i915_seqno_passed(seqno, wait->seqno)) {
- RB_CLEAR_NODE(&wait->node);
- return first;
- }
-
- p = &b->waiters.rb_node;
- while (*p) {
- parent = *p;
- if (wait->seqno == to_wait(parent)->seqno) {
- /* We have multiple waiters on the same seqno, select
- * the highest priority task (that with the smallest
- * task->prio) to serve as the bottom-half for this
- * group.
- */
- if (wait->tsk->prio > to_wait(parent)->tsk->prio) {
- p = &parent->rb_right;
- first = false;
- } else {
- p = &parent->rb_left;
- }
- } else if (i915_seqno_passed(wait->seqno,
- to_wait(parent)->seqno)) {
- p = &parent->rb_right;
- if (i915_seqno_passed(seqno, to_wait(parent)->seqno))
- completed = parent;
- else
- first = false;
- } else {
- p = &parent->rb_left;
- }
- }
- rb_link_node(&wait->node, parent, p);
- rb_insert_color(&wait->node, &b->waiters);
-
- if (first) {
- spin_lock(&b->irq_lock);
- b->irq_wait = wait;
- /* After assigning ourselves as the new bottom-half, we must
- * perform a cursory check to prevent a missed interrupt.
- * Either we miss the interrupt whilst programming the hardware,
- * or if there was a previous waiter (for a later seqno) they
- * may be woken instead of us (due to the inherent race
- * in the unlocked read of b->irq_seqno_bh in the irq handler)
- * and so we miss the wake up.
- */
- armed = __intel_breadcrumbs_enable_irq(b);
- spin_unlock(&b->irq_lock);
- }
-
- if (completed) {
- /* Advance the bottom-half (b->irq_wait) before we wake up
- * the waiters who may scribble over their intel_wait
- * just as the interrupt handler is dereferencing it via
- * b->irq_wait.
- */
- if (!first) {
- struct rb_node *next = rb_next(completed);
- GEM_BUG_ON(next == &wait->node);
- __intel_breadcrumbs_next(engine, next);
- }
-
- do {
- struct intel_wait *crumb = to_wait(completed);
- completed = rb_prev(completed);
- __intel_breadcrumbs_finish(b, crumb);
- } while (completed);
- }
-
- GEM_BUG_ON(!b->irq_wait);
- GEM_BUG_ON(!b->irq_armed);
- GEM_BUG_ON(rb_first(&b->waiters) != &b->irq_wait->node);
-
- return armed;
-}
-
-bool intel_engine_add_wait(struct intel_engine_cs *engine,
- struct intel_wait *wait)
-{
- struct intel_breadcrumbs *b = &engine->breadcrumbs;
- bool armed;
-
- spin_lock_irq(&b->rb_lock);
- armed = __intel_engine_add_wait(engine, wait);
- spin_unlock_irq(&b->rb_lock);
- if (armed)
- return armed;
-
- /* Make the caller recheck if its request has already started. */
- return intel_engine_has_started(engine, wait->seqno);
-}
-
-static inline bool chain_wakeup(struct rb_node *rb, int priority)
-{
- return rb && to_wait(rb)->tsk->prio <= priority;
-}
+ clear_bit(engine->id, &engine->i915->gpu_error.missed_irq_rings);
-static inline int wakeup_priority(struct intel_breadcrumbs *b,
- struct task_struct *tsk)
-{
- if (tsk == b->signaler)
- return INT_MIN;
+ if (b->irq_enabled)
+ irq_enable(engine);
else
- return tsk->prio;
-}
-
-static void __intel_engine_remove_wait(struct intel_engine_cs *engine,
- struct intel_wait *wait)
-{
- struct intel_breadcrumbs *b = &engine->breadcrumbs;
-
- lockdep_assert_held(&b->rb_lock);
-
- if (RB_EMPTY_NODE(&wait->node))
- goto out;
-
- if (b->irq_wait == wait) {
- const int priority = wakeup_priority(b, wait->tsk);
- struct rb_node *next;
-
- /* We are the current bottom-half. Find the next candidate,
- * the first waiter in the queue on the remaining oldest
- * request. As multiple seqnos may complete in the time it
- * takes us to wake up and find the next waiter, we have to
- * wake up that waiter for it to perform its own coherent
- * completion check.
- */
- next = rb_next(&wait->node);
- if (chain_wakeup(next, priority)) {
- /* If the next waiter is already complete,
- * wake it up and continue onto the next waiter. So
- * if have a small herd, they will wake up in parallel
- * rather than sequentially, which should reduce
- * the overall latency in waking all the completed
- * clients.
- *
- * However, waking up a chain adds extra latency to
- * the first_waiter. This is undesirable if that
- * waiter is a high priority task.
- */
- u32 seqno = intel_engine_get_seqno(engine);
-
- while (i915_seqno_passed(seqno, to_wait(next)->seqno)) {
- struct rb_node *n = rb_next(next);
-
- __intel_breadcrumbs_finish(b, to_wait(next));
- next = n;
- if (!chain_wakeup(next, priority))
- break;
- }
- }
-
- __intel_breadcrumbs_next(engine, next);
- } else {
- GEM_BUG_ON(rb_first(&b->waiters) == &wait->node);
- }
-
- GEM_BUG_ON(RB_EMPTY_NODE(&wait->node));
- rb_erase(&wait->node, &b->waiters);
- RB_CLEAR_NODE(&wait->node);
+ irq_disable(engine);
-out:
- GEM_BUG_ON(b->irq_wait == wait);
- GEM_BUG_ON(rb_first(&b->waiters) !=
- (b->irq_wait ? &b->irq_wait->node : NULL));
+ spin_unlock_irqrestore(&b->irq_lock, flags);
}
-void intel_engine_remove_wait(struct intel_engine_cs *engine,
- struct intel_wait *wait)
+void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine)
{
- struct intel_breadcrumbs *b = &engine->breadcrumbs;
-
- /* Quick check to see if this waiter was already decoupled from
- * the tree by the bottom-half to avoid contention on the spinlock
- * by the herd.
- */
- if (RB_EMPTY_NODE(&wait->node)) {
- GEM_BUG_ON(READ_ONCE(b->irq_wait) == wait);
- return;
- }
-
- spin_lock_irq(&b->rb_lock);
- __intel_engine_remove_wait(engine, wait);
- spin_unlock_irq(&b->rb_lock);
+ cancel_fake_irq(engine);
}
-static void signaler_set_rtpriority(void)
+bool i915_request_enable_breadcrumb(struct i915_request *rq)
{
- struct sched_param param = { .sched_priority = 1 };
-
- sched_setscheduler_nocheck(current, SCHED_FIFO, ¶m);
-}
+ struct intel_breadcrumbs *b = &rq->engine->breadcrumbs;
-static int intel_breadcrumbs_signaler(void *arg)
-{
- struct intel_engine_cs *engine = arg;
- struct intel_breadcrumbs *b = &engine->breadcrumbs;
- struct i915_request *rq, *n;
+ GEM_BUG_ON(test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags));
- /* Install ourselves with high priority to reduce signalling latency */
- signaler_set_rtpriority();
+ if (!test_bit(I915_FENCE_FLAG_ACTIVE, &rq->fence.flags))
+ return true;
- do {
- bool do_schedule = true;
- LIST_HEAD(list);
- u32 seqno;
+ spin_lock(&b->irq_lock);
+ if (test_bit(I915_FENCE_FLAG_ACTIVE, &rq->fence.flags) &&
+ !__request_completed(rq)) {
+ struct intel_context *ce = rq->hw_context;
+ struct list_head *pos;
- set_current_state(TASK_INTERRUPTIBLE);
- if (list_empty(&b->signals))
- goto sleep;
+ __intel_breadcrumbs_arm_irq(b);
/*
- * We are either woken up by the interrupt bottom-half,
- * or by a client adding a new signaller. In both cases,
- * the GPU seqno may have advanced beyond our oldest signal.
- * If it has, propagate the signal, remove the waiter and
- * check again with the next oldest signal. Otherwise we
- * need to wait for a new interrupt from the GPU or for
- * a new client.
+ * We keep the seqno in retirement order, so we can break
+ * inside intel_engine_breadcrumbs_irq as soon as we've passed
+ * the last completed request (or seen a request that hasn't
+ * event started). We could iterate the timeline->requests list,
+ * but keeping a separate signalers_list has the advantage of
+ * hopefully being much smaller than the full list and so
+ * provides faster iteration and detection when there are no
+ * more interrupts required for this context.
+ *
+ * We typically expect to add new signalers in order, so we
+ * start looking for our insertion point from the tail of
+ * the list.
*/
- seqno = intel_engine_get_seqno(engine);
-
- spin_lock_irq(&b->rb_lock);
- list_for_each_entry_safe(rq, n, &b->signals, signaling.link) {
- u32 this = rq->signaling.wait.seqno;
+ list_for_each_prev(pos, &ce->signals) {
+ struct i915_request *it =
+ list_entry(pos, typeof(*it), signal_link);
- GEM_BUG_ON(!rq->signaling.wait.seqno);
-
- if (!i915_seqno_passed(seqno, this))
+ if (i915_seqno_passed(rq->fence.seqno, it->fence.seqno))
break;
-
- if (likely(this == i915_request_global_seqno(rq))) {
- __intel_engine_remove_wait(engine,
- &rq->signaling.wait);
-
- rq->signaling.wait.seqno = 0;
- __list_del_entry(&rq->signaling.link);
-
- if (!i915_request_signaled(rq)) {
- list_add_tail(&rq->signaling.link,
- &list);
- i915_request_get(rq);
- }
- }
}
- spin_unlock_irq(&b->rb_lock);
-
- if (!list_empty(&list)) {
- local_bh_disable();
- list_for_each_entry_safe(rq, n, &list, signaling.link) {
- dma_fence_signal(&rq->fence);
- GEM_BUG_ON(!i915_request_completed(rq));
- i915_request_put(rq);
- }
- local_bh_enable(); /* kick start the tasklets */
-
- /*
- * If the engine is saturated we may be continually
- * processing completed requests. This angers the
- * NMI watchdog if we never let anything else
- * have access to the CPU. Let's pretend to be nice
- * and relinquish the CPU if we burn through the
- * entire RT timeslice!
- */
- do_schedule = need_resched();
- }
-
- if (unlikely(do_schedule)) {
-sleep:
- if (kthread_should_park())
- kthread_parkme();
-
- if (unlikely(kthread_should_stop()))
- break;
-
- schedule();
- }
- } while (1);
- __set_current_state(TASK_RUNNING);
-
- return 0;
-}
+ list_add(&rq->signal_link, pos);
+ if (pos == &ce->signals) /* catch transitions from empty list */
+ list_move_tail(&ce->signal_link, &b->signalers);
-static void insert_signal(struct intel_breadcrumbs *b,
- struct i915_request *request,
- const u32 seqno)
-{
- struct i915_request *iter;
-
- lockdep_assert_held(&b->rb_lock);
-
- /*
- * A reasonable assumption is that we are called to add signals
- * in sequence, as the requests are submitted for execution and
- * assigned a global_seqno. This will be the case for the majority
- * of internally generated signals (inter-engine signaling).
- *
- * Out of order waiters triggering random signaling enabling will
- * be more problematic, but hopefully rare enough and the list
- * small enough that the O(N) insertion sort is not an issue.
- */
-
- list_for_each_entry_reverse(iter, &b->signals, signaling.link)
- if (i915_seqno_passed(seqno, iter->signaling.wait.seqno))
- break;
-
- list_add(&request->signaling.link, &iter->signaling.link);
-}
-
-bool intel_engine_enable_signaling(struct i915_request *request, bool wakeup)
-{
- struct intel_engine_cs *engine = request->engine;
- struct intel_breadcrumbs *b = &engine->breadcrumbs;
- struct intel_wait *wait = &request->signaling.wait;
- u32 seqno;
-
- /*
- * Note that we may be called from an interrupt handler on another
- * device (e.g. nouveau signaling a fence completion causing us
- * to submit a request, and so enable signaling). As such,
- * we need to make sure that all other users of b->rb_lock protect
- * against interrupts, i.e. use spin_lock_irqsave.
- */
-
- /* locked by dma_fence_enable_sw_signaling() (irqsafe fence->lock) */
- GEM_BUG_ON(!irqs_disabled());
- lockdep_assert_held(&request->lock);
-
- seqno = i915_request_global_seqno(request);
- if (!seqno) /* will be enabled later upon execution */
- return true;
-
- GEM_BUG_ON(wait->seqno);
- wait->tsk = b->signaler;
- wait->request = request;
- wait->seqno = seqno;
-
- /*
- * Add ourselves into the list of waiters, but registering our
- * bottom-half as the signaller thread. As per usual, only the oldest
- * waiter (not just signaller) is tasked as the bottom-half waking
- * up all completed waiters after the user interrupt.
- *
- * If we are the oldest waiter, enable the irq (after which we
- * must double check that the seqno did not complete).
- */
- spin_lock(&b->rb_lock);
- insert_signal(b, request, seqno);
- wakeup &= __intel_engine_add_wait(engine, wait);
- spin_unlock(&b->rb_lock);
-
- if (wakeup) {
- wake_up_process(b->signaler);
- return !intel_wait_complete(wait);
+ set_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
}
+ spin_unlock(&b->irq_lock);
- return true;
+ return !__request_completed(rq);
}
-void intel_engine_cancel_signaling(struct i915_request *request)
+void i915_request_cancel_breadcrumb(struct i915_request *rq)
{
- struct intel_engine_cs *engine = request->engine;
- struct intel_breadcrumbs *b = &engine->breadcrumbs;
+ struct intel_breadcrumbs *b = &rq->engine->breadcrumbs;
- GEM_BUG_ON(!irqs_disabled());
- lockdep_assert_held(&request->lock);
-
- if (!READ_ONCE(request->signaling.wait.seqno))
+ if (!test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags))
return;
- spin_lock(&b->rb_lock);
- __intel_engine_remove_wait(engine, &request->signaling.wait);
- if (fetch_and_zero(&request->signaling.wait.seqno))
- __list_del_entry(&request->signaling.link);
- spin_unlock(&b->rb_lock);
-}
-
-int intel_engine_init_breadcrumbs(struct intel_engine_cs *engine)
-{
- struct intel_breadcrumbs *b = &engine->breadcrumbs;
- struct task_struct *tsk;
-
- spin_lock_init(&b->rb_lock);
- spin_lock_init(&b->irq_lock);
-
- timer_setup(&b->fake_irq, intel_breadcrumbs_fake_irq, 0);
- timer_setup(&b->hangcheck, intel_breadcrumbs_hangcheck, 0);
-
- INIT_LIST_HEAD(&b->signals);
-
- /* Spawn a thread to provide a common bottom-half for all signals.
- * As this is an asynchronous interface we cannot steal the current
- * task for handling the bottom-half to the user interrupt, therefore
- * we create a thread to do the coherent seqno dance after the
- * interrupt and then signal the waitqueue (via the dma-buf/fence).
- */
- tsk = kthread_run(intel_breadcrumbs_signaler, engine,
- "i915/signal:%d", engine->id);
- if (IS_ERR(tsk))
- return PTR_ERR(tsk);
-
- b->signaler = tsk;
-
- return 0;
-}
+ spin_lock(&b->irq_lock);
+ if (test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags)) {
+ struct intel_context *ce = rq->hw_context;
-static void cancel_fake_irq(struct intel_engine_cs *engine)
-{
- struct intel_breadcrumbs *b = &engine->breadcrumbs;
+ list_del(&rq->signal_link);
+ if (list_empty(&ce->signals))
+ list_del_init(&ce->signal_link);
- del_timer_sync(&b->fake_irq); /* may queue b->hangcheck */
- del_timer_sync(&b->hangcheck);
- clear_bit(engine->id, &engine->i915->gpu_error.missed_irq_rings);
+ clear_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
+ }
+ spin_unlock(&b->irq_lock);
}
-void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
+void intel_engine_print_breadcrumbs(struct intel_engine_cs *engine,
+ struct drm_printer *p)
{
struct intel_breadcrumbs *b = &engine->breadcrumbs;
- unsigned long flags;
+ struct intel_context *ce;
+ struct i915_request *rq;
- spin_lock_irqsave(&b->irq_lock, flags);
-
- /*
- * Leave the fake_irq timer enabled (if it is running), but clear the
- * bit so that it turns itself off on its next wake up and goes back
- * to the long hangcheck interval if still required.
- */
- clear_bit(engine->id, &engine->i915->gpu_error.missed_irq_rings);
-
- if (b->irq_enabled)
- irq_enable(engine);
- else
- irq_disable(engine);
-
- spin_unlock_irqrestore(&b->irq_lock, flags);
-}
-
-void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine)
-{
- struct intel_breadcrumbs *b = &engine->breadcrumbs;
+ if (list_empty(&b->signalers))
+ return;
- /* The engines should be idle and all requests accounted for! */
- WARN_ON(READ_ONCE(b->irq_wait));
- WARN_ON(!RB_EMPTY_ROOT(&b->waiters));
- WARN_ON(!list_empty(&b->signals));
+ drm_printf(p, "Signals:\n");
- if (!IS_ERR_OR_NULL(b->signaler))
- kthread_stop(b->signaler);
+ spin_lock_irq(&b->irq_lock);
+ list_for_each_entry(ce, &b->signalers, signal_link) {
+ list_for_each_entry(rq, &ce->signals, signal_link) {
+ drm_printf(p, "\t[%llx:%llx%s] @ %dms\n",
+ rq->fence.context, rq->fence.seqno,
+ i915_request_completed(rq) ? "!" :
+ i915_request_started(rq) ? "*" :
+ "",
+ jiffies_to_msecs(jiffies - rq->emitted_jiffies));
+ }
+ }
+ spin_unlock_irq(&b->irq_lock);
- cancel_fake_irq(engine);
+ if (test_bit(engine->id, &engine->i915->gpu_error.missed_irq_rings))
+ drm_printf(p, "Fake irq active\n");
}
-
-#if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
-#include "selftests/intel_breadcrumbs.c"
-#endif
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index d25e510b7b08..ee43c98227bb 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -458,12 +458,6 @@ int intel_engines_init(struct drm_i915_private *dev_priv)
void intel_engine_write_global_seqno(struct intel_engine_cs *engine, u32 seqno)
{
intel_write_status_page(engine, I915_GEM_HWS_INDEX, seqno);
-
- /* After manually advancing the seqno, fake the interrupt in case
- * there are any waiters for that seqno.
- */
- intel_engine_wakeup(engine);
-
GEM_BUG_ON(intel_engine_get_seqno(engine) != seqno);
}
@@ -607,6 +601,7 @@ int intel_engine_setup_common(struct intel_engine_cs *engine)
i915_timeline_set_subclass(&engine->timeline, TIMELINE_ENGINE);
+ intel_engine_init_breadcrumbs(engine);
intel_engine_init_execlist(engine);
intel_engine_init_hangcheck(engine);
intel_engine_init_batch_pool(engine);
@@ -717,20 +712,14 @@ int intel_engine_init_common(struct intel_engine_cs *engine)
}
}
- ret = intel_engine_init_breadcrumbs(engine);
- if (ret)
- goto err_unpin_preempt;
-
ret = measure_breadcrumb_dw(engine);
if (ret < 0)
- goto err_breadcrumbs;
+ goto err_unpin_preempt;
engine->emit_fini_breadcrumb_dw = ret;
return 0;
-err_breadcrumbs:
- intel_engine_fini_breadcrumbs(engine);
err_unpin_preempt:
if (i915->preempt_context)
__intel_context_unpin(i915->preempt_context, engine);
@@ -1294,12 +1283,14 @@ static void print_request(struct drm_printer *m,
x = print_sched_attr(rq->i915, &rq->sched.attr, buf, x, sizeof(buf));
- drm_printf(m, "%s%x%s [%llx:%llx]%s @ %dms: %s\n",
+ drm_printf(m, "%s%x%s%s [%llx:%llx]%s @ %dms: %s\n",
prefix,
rq->global_seqno,
i915_request_completed(rq) ? "!" :
i915_request_started(rq) ? "*" :
"",
+ test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
+ &rq->fence.flags) ? "+" : "",
rq->fence.context, rq->fence.seqno,
buf,
jiffies_to_msecs(jiffies - rq->emitted_jiffies),
@@ -1492,12 +1483,9 @@ void intel_engine_dump(struct intel_engine_cs *engine,
struct drm_printer *m,
const char *header, ...)
{
- struct intel_breadcrumbs * const b = &engine->breadcrumbs;
struct i915_gpu_error * const error = &engine->i915->gpu_error;
struct i915_request *rq;
intel_wakeref_t wakeref;
- unsigned long flags;
- struct rb_node *rb;
if (header) {
va_list ap;
@@ -1565,21 +1553,12 @@ void intel_engine_dump(struct intel_engine_cs *engine,
intel_execlists_show_requests(engine, m, print_request, 8);
- spin_lock_irqsave(&b->rb_lock, flags);
- for (rb = rb_first(&b->waiters); rb; rb = rb_next(rb)) {
- struct intel_wait *w = rb_entry(rb, typeof(*w), node);
-
- drm_printf(m, "\t%s [%d:%c] waiting for %x\n",
- w->tsk->comm, w->tsk->pid,
- task_state_to_char(w->tsk),
- w->seqno);
- }
- spin_unlock_irqrestore(&b->rb_lock, flags);
-
drm_printf(m, "HWSP:\n");
hexdump(m, engine->status_page.addr, PAGE_SIZE);
drm_printf(m, "Idle? %s\n", yesno(intel_engine_is_idle(engine)));
+
+ intel_engine_print_breadcrumbs(engine, m);
}
static u8 user_class_map[] = {
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 668ed67336a2..b889b27f8aeb 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -743,7 +743,7 @@ static int init_ring_common(struct intel_engine_cs *engine)
}
/* Papering over lost _interrupts_ immediately following the restart */
- intel_engine_wakeup(engine);
+ intel_engine_queue_breadcrumbs(engine);
out:
intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index ae753c45cbe7..efef2aa1abce 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -5,6 +5,7 @@
#include <drm/drm_util.h>
#include <linux/hashtable.h>
+#include <linux/irq_work.h>
#include <linux/seqlock.h>
#include "i915_gem_batch_pool.h"
@@ -381,22 +382,19 @@ struct intel_engine_cs {
* the overhead of waking that client is much preferred.
*/
struct intel_breadcrumbs {
- spinlock_t irq_lock; /* protects irq_*; irqsafe */
- struct intel_wait *irq_wait; /* oldest waiter by retirement */
+ spinlock_t irq_lock;
+ struct list_head signalers;
- spinlock_t rb_lock; /* protects the rb and wraps irq_lock */
- struct rb_root waiters; /* sorted by retirement, priority */
- struct list_head signals; /* sorted by retirement */
- struct task_struct *signaler; /* used for fence signalling */
+ struct irq_work irq_work; /* for use from inside irq_lock */
struct timer_list fake_irq; /* used after a missed interrupt */
struct timer_list hangcheck; /* detect missed interrupts */
unsigned int hangcheck_interrupts;
unsigned int irq_enabled;
- unsigned int irq_count;
- bool irq_armed : 1;
+ bool irq_armed;
+ bool irq_fired;
} breadcrumbs;
struct {
@@ -885,83 +883,29 @@ static inline bool intel_engine_has_started(struct intel_engine_cs *engine,
void intel_engine_get_instdone(struct intel_engine_cs *engine,
struct intel_instdone *instdone);
-/* intel_breadcrumbs.c -- user interrupt bottom-half for waiters */
-int intel_engine_init_breadcrumbs(struct intel_engine_cs *engine);
-
-static inline void intel_wait_init(struct intel_wait *wait)
-{
- wait->tsk = current;
- wait->request = NULL;
-}
-
-static inline void intel_wait_init_for_seqno(struct intel_wait *wait, u32 seqno)
-{
- wait->tsk = current;
- wait->seqno = seqno;
-}
-
-static inline bool intel_wait_has_seqno(const struct intel_wait *wait)
-{
- return wait->seqno;
-}
-
-static inline bool
-intel_wait_update_seqno(struct intel_wait *wait, u32 seqno)
-{
- wait->seqno = seqno;
- return intel_wait_has_seqno(wait);
-}
-
-static inline bool
-intel_wait_update_request(struct intel_wait *wait,
- const struct i915_request *rq)
-{
- return intel_wait_update_seqno(wait, i915_request_global_seqno(rq));
-}
-
-static inline bool
-intel_wait_check_seqno(const struct intel_wait *wait, u32 seqno)
-{
- return wait->seqno == seqno;
-}
-
-static inline bool
-intel_wait_check_request(const struct intel_wait *wait,
- const struct i915_request *rq)
-{
- return intel_wait_check_seqno(wait, i915_request_global_seqno(rq));
-}
+void intel_engine_init_breadcrumbs(struct intel_engine_cs *engine);
+void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine);
-static inline bool intel_wait_complete(const struct intel_wait *wait)
-{
- return RB_EMPTY_NODE(&wait->node);
-}
+void intel_engine_pin_breadcrumbs_irq(struct intel_engine_cs *engine);
+void intel_engine_unpin_breadcrumbs_irq(struct intel_engine_cs *engine);
-bool intel_engine_add_wait(struct intel_engine_cs *engine,
- struct intel_wait *wait);
-void intel_engine_remove_wait(struct intel_engine_cs *engine,
- struct intel_wait *wait);
-bool intel_engine_enable_signaling(struct i915_request *request, bool wakeup);
-void intel_engine_cancel_signaling(struct i915_request *request);
+bool intel_engine_signal_breadcrumbs(struct intel_engine_cs *engine);
+void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine);
-static inline bool intel_engine_has_waiter(const struct intel_engine_cs *engine)
+static inline void
+intel_engine_queue_breadcrumbs(struct intel_engine_cs *engine)
{
- return READ_ONCE(engine->breadcrumbs.irq_wait);
+ irq_work_queue(&engine->breadcrumbs.irq_work);
}
-unsigned int intel_engine_wakeup(struct intel_engine_cs *engine);
-#define ENGINE_WAKEUP_WAITER BIT(0)
-#define ENGINE_WAKEUP_ASLEEP BIT(1)
-
-void intel_engine_pin_breadcrumbs_irq(struct intel_engine_cs *engine);
-void intel_engine_unpin_breadcrumbs_irq(struct intel_engine_cs *engine);
-
-void __intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine);
-void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine);
+bool intel_engine_breadcrumbs_irq(struct intel_engine_cs *engine);
void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine);
void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine);
+void intel_engine_print_breadcrumbs(struct intel_engine_cs *engine,
+ struct drm_printer *p);
+
static inline u32 *gen8_emit_pipe_control(u32 *batch, u32 flags, u32 offset)
{
memset(batch, 0, 6 * sizeof(u32));
diff --git a/drivers/gpu/drm/i915/selftests/i915_mock_selftests.h b/drivers/gpu/drm/i915/selftests/i915_mock_selftests.h
index 4a83a1c6c406..88e5ab586337 100644
--- a/drivers/gpu/drm/i915/selftests/i915_mock_selftests.h
+++ b/drivers/gpu/drm/i915/selftests/i915_mock_selftests.h
@@ -15,7 +15,6 @@ selftest(scatterlist, scatterlist_mock_selftests)
selftest(syncmap, i915_syncmap_mock_selftests)
selftest(uncore, intel_uncore_mock_selftests)
selftest(engine, intel_engine_cs_mock_selftests)
-selftest(breadcrumbs, intel_breadcrumbs_mock_selftests)
selftest(timelines, i915_timeline_mock_selftests)
selftest(requests, i915_request_mock_selftests)
selftest(objects, i915_gem_object_mock_selftests)
diff --git a/drivers/gpu/drm/i915/selftests/i915_request.c b/drivers/gpu/drm/i915/selftests/i915_request.c
index 4d4b86b5fa11..6733dc5b6b4c 100644
--- a/drivers/gpu/drm/i915/selftests/i915_request.c
+++ b/drivers/gpu/drm/i915/selftests/i915_request.c
@@ -25,9 +25,12 @@
#include <linux/prime_numbers.h>
#include "../i915_selftest.h"
+#include "i915_random.h"
#include "igt_live_test.h"
+#include "lib_sw_fence.h"
#include "mock_context.h"
+#include "mock_drm.h"
#include "mock_gem_device.h"
static int igt_add_request(void *arg)
@@ -247,6 +250,254 @@ static int igt_request_rewind(void *arg)
return err;
}
+struct smoketest {
+ struct intel_engine_cs *engine;
+ struct i915_gem_context **contexts;
+ atomic_long_t num_waits, num_fences;
+ int ncontexts, max_batch;
+ struct i915_request *(*request_alloc)(struct i915_gem_context *,
+ struct intel_engine_cs *);
+};
+
+static struct i915_request *
+__mock_request_alloc(struct i915_gem_context *ctx,
+ struct intel_engine_cs *engine)
+{
+ return mock_request(engine, ctx, 0);
+}
+
+static struct i915_request *
+__live_request_alloc(struct i915_gem_context *ctx,
+ struct intel_engine_cs *engine)
+{
+ return i915_request_alloc(engine, ctx);
+}
+
+static int __igt_breadcrumbs_smoketest(void *arg)
+{
+ struct smoketest *t = arg;
+ struct mutex * const BKL = &t->engine->i915->drm.struct_mutex;
+ const unsigned int max_batch = min(t->ncontexts, t->max_batch) - 1;
+ const unsigned int total = 4 * t->ncontexts + 1;
+ unsigned int num_waits = 0, num_fences = 0;
+ struct i915_request **requests;
+ I915_RND_STATE(prng);
+ unsigned int *order;
+ int err = 0;
+
+ /*
+ * A very simple test to catch the most egregious of list handling bugs.
+ *
+ * At its heart, we simply create oodles of requests running across
+ * multiple kthreads and enable signaling on them, for the sole purpose
+ * of stressing our breadcrumb handling. The only inspection we do is
+ * that the fences were marked as signaled.
+ */
+
+ requests = kmalloc_array(total, sizeof(*requests), GFP_KERNEL);
+ if (!requests)
+ return -ENOMEM;
+
+ order = i915_random_order(total, &prng);
+ if (!order) {
+ err = -ENOMEM;
+ goto out_requests;
+ }
+
+ while (!kthread_should_stop()) {
+ struct i915_sw_fence *submit, *wait;
+ unsigned int n, count;
+
+ submit = heap_fence_create(GFP_KERNEL);
+ if (!submit) {
+ err = -ENOMEM;
+ break;
+ }
+
+ wait = heap_fence_create(GFP_KERNEL);
+ if (!wait) {
+ i915_sw_fence_commit(submit);
+ heap_fence_put(submit);
+ err = ENOMEM;
+ break;
+ }
+
+ i915_random_reorder(order, total, &prng);
+ count = 1 + i915_prandom_u32_max_state(max_batch, &prng);
+
+ for (n = 0; n < count; n++) {
+ struct i915_gem_context *ctx =
+ t->contexts[order[n] % t->ncontexts];
+ struct i915_request *rq;
+
+ mutex_lock(BKL);
+
+ rq = t->request_alloc(ctx, t->engine);
+ if (IS_ERR(rq)) {
+ mutex_unlock(BKL);
+ err = PTR_ERR(rq);
+ count = n;
+ break;
+ }
+
+ err = i915_sw_fence_await_sw_fence_gfp(&rq->submit,
+ submit,
+ GFP_KERNEL);
+
+ requests[n] = i915_request_get(rq);
+ i915_request_add(rq);
+
+ mutex_unlock(BKL);
+
+ if (err >= 0)
+ err = i915_sw_fence_await_dma_fence(wait,
+ &rq->fence,
+ 0,
+ GFP_KERNEL);
+
+ if (err < 0) {
+ i915_request_put(rq);
+ count = n;
+ break;
+ }
+ }
+
+ i915_sw_fence_commit(submit);
+ i915_sw_fence_commit(wait);
+
+ if (!wait_event_timeout(wait->wait,
+ i915_sw_fence_done(wait),
+ HZ / 2)) {
+ struct i915_request *rq = requests[count - 1];
+
+ pr_err("waiting for %d fences (last %llx:%lld) on %s timed out!\n",
+ count,
+ rq->fence.context, rq->fence.seqno,
+ t->engine->name);
+ i915_gem_set_wedged(t->engine->i915);
+ GEM_BUG_ON(!i915_request_completed(rq));
+ i915_sw_fence_wait(wait);
+ err = -EIO;
+ }
+
+ for (n = 0; n < count; n++) {
+ struct i915_request *rq = requests[n];
+
+ if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
+ &rq->fence.flags)) {
+ pr_err("%llu:%llu was not signaled!\n",
+ rq->fence.context, rq->fence.seqno);
+ err = -EINVAL;
+ }
+
+ i915_request_put(rq);
+ }
+
+ heap_fence_put(wait);
+ heap_fence_put(submit);
+
+ if (err < 0)
+ break;
+
+ num_fences += count;
+ num_waits++;
+
+ cond_resched();
+ }
+
+ atomic_long_add(num_fences, &t->num_fences);
+ atomic_long_add(num_waits, &t->num_waits);
+
+ kfree(order);
+out_requests:
+ kfree(requests);
+ return err;
+}
+
+static int mock_breadcrumbs_smoketest(void *arg)
+{
+ struct drm_i915_private *i915 = arg;
+ struct smoketest t = {
+ .engine = i915->engine[RCS],
+ .ncontexts = 1024,
+ .max_batch = 1024,
+ .request_alloc = __mock_request_alloc
+ };
+ unsigned int ncpus = num_online_cpus();
+ struct task_struct **threads;
+ unsigned int n;
+ int ret = 0;
+
+ /*
+ * Smoketest our breadcrumb/signal handling for requests across multiple
+ * threads. A very simple test to only catch the most egregious of bugs.
+ * See __igt_breadcrumbs_smoketest();
+ */
+
+ threads = kmalloc_array(ncpus, sizeof(*threads), GFP_KERNEL);
+ if (!threads)
+ return -ENOMEM;
+
+ t.contexts =
+ kmalloc_array(t.ncontexts, sizeof(*t.contexts), GFP_KERNEL);
+ if (!t.contexts) {
+ ret = -ENOMEM;
+ goto out_threads;
+ }
+
+ mutex_lock(&t.engine->i915->drm.struct_mutex);
+ for (n = 0; n < t.ncontexts; n++) {
+ t.contexts[n] = mock_context(t.engine->i915, "mock");
+ if (!t.contexts[n]) {
+ ret = -ENOMEM;
+ goto out_contexts;
+ }
+ }
+ mutex_unlock(&t.engine->i915->drm.struct_mutex);
+
+ for (n = 0; n < ncpus; n++) {
+ threads[n] = kthread_run(__igt_breadcrumbs_smoketest,
+ &t, "igt/%d", n);
+ if (IS_ERR(threads[n])) {
+ ret = PTR_ERR(threads[n]);
+ ncpus = n;
+ break;
+ }
+
+ get_task_struct(threads[n]);
+ }
+
+ msleep(jiffies_to_msecs(i915_selftest.timeout_jiffies));
+
+ for (n = 0; n < ncpus; n++) {
+ int err;
+
+ err = kthread_stop(threads[n]);
+ if (err < 0 && !ret)
+ ret = err;
+
+ put_task_struct(threads[n]);
+ }
+ pr_info("Completed %lu waits for %lu fence across %d cpus\n",
+ atomic_long_read(&t.num_waits),
+ atomic_long_read(&t.num_fences),
+ ncpus);
+
+ mutex_lock(&t.engine->i915->drm.struct_mutex);
+out_contexts:
+ for (n = 0; n < t.ncontexts; n++) {
+ if (!t.contexts[n])
+ break;
+ mock_context_close(t.contexts[n]);
+ }
+ mutex_unlock(&t.engine->i915->drm.struct_mutex);
+ kfree(t.contexts);
+out_threads:
+ kfree(threads);
+
+ return ret;
+}
+
int i915_request_mock_selftests(void)
{
static const struct i915_subtest tests[] = {
@@ -254,6 +505,7 @@ int i915_request_mock_selftests(void)
SUBTEST(igt_wait_request),
SUBTEST(igt_fence_wait),
SUBTEST(igt_request_rewind),
+ SUBTEST(mock_breadcrumbs_smoketest),
};
struct drm_i915_private *i915;
intel_wakeref_t wakeref;
@@ -812,6 +1064,178 @@ static int live_sequential_engines(void *arg)
return err;
}
+static int
+max_batches(struct i915_gem_context *ctx, struct intel_engine_cs *engine)
+{
+ struct i915_request *rq;
+ int ret;
+
+ /*
+ * Before execlists, all contexts share the same ringbuffer. With
+ * execlists, each context/engine has a separate ringbuffer and
+ * for the purposes of this test, inexhaustible.
+ *
+ * For the global ringbuffer though, we have to be very careful
+ * that we do not wrap while preventing the execution of requests
+ * with a unsignaled fence.
+ */
+ if (HAS_EXECLISTS(ctx->i915))
+ return INT_MAX;
+
+ rq = i915_request_alloc(engine, ctx);
+ if (IS_ERR(rq)) {
+ ret = PTR_ERR(rq);
+ } else {
+ int sz;
+
+ ret = rq->ring->size - rq->reserved_space;
+ i915_request_add(rq);
+
+ sz = rq->ring->emit - rq->head;
+ if (sz < 0)
+ sz += rq->ring->size;
+ ret /= sz;
+ ret /= 2; /* leave half spare, in case of emergency! */
+ }
+
+ return ret;
+}
+
+static int live_breadcrumbs_smoketest(void *arg)
+{
+ struct drm_i915_private *i915 = arg;
+ struct smoketest t[I915_NUM_ENGINES];
+ unsigned int ncpus = num_online_cpus();
+ unsigned long num_waits, num_fences;
+ struct intel_engine_cs *engine;
+ struct task_struct **threads;
+ struct igt_live_test live;
+ enum intel_engine_id id;
+ intel_wakeref_t wakeref;
+ struct drm_file *file;
+ unsigned int n;
+ int ret = 0;
+
+ /*
+ * Smoketest our breadcrumb/signal handling for requests across multiple
+ * threads. A very simple test to only catch the most egregious of bugs.
+ * See __igt_breadcrumbs_smoketest();
+ *
+ * On real hardware this time.
+ */
+
+ wakeref = intel_runtime_pm_get(i915);
+
+ file = mock_file(i915);
+ if (IS_ERR(file)) {
+ ret = PTR_ERR(file);
+ goto out_rpm;
+ }
+
+ threads = kcalloc(ncpus * I915_NUM_ENGINES,
+ sizeof(*threads),
+ GFP_KERNEL);
+ if (!threads) {
+ ret = -ENOMEM;
+ goto out_file;
+ }
+
+ memset(&t[0], 0, sizeof(t[0]));
+ t[0].request_alloc = __live_request_alloc;
+ t[0].ncontexts = 64;
+ t[0].contexts = kmalloc_array(t[0].ncontexts,
+ sizeof(*t[0].contexts),
+ GFP_KERNEL);
+ if (!t[0].contexts) {
+ ret = -ENOMEM;
+ goto out_threads;
+ }
+
+ mutex_lock(&i915->drm.struct_mutex);
+ for (n = 0; n < t[0].ncontexts; n++) {
+ t[0].contexts[n] = live_context(i915, file);
+ if (!t[0].contexts[n]) {
+ ret = -ENOMEM;
+ goto out_contexts;
+ }
+ }
+
+ ret = igt_live_test_begin(&live, i915, __func__, "");
+ if (ret)
+ goto out_contexts;
+
+ for_each_engine(engine, i915, id) {
+ t[id] = t[0];
+ t[id].engine = engine;
+ t[id].max_batch = max_batches(t[0].contexts[0], engine);
+ if (t[id].max_batch < 0) {
+ ret = t[id].max_batch;
+ mutex_unlock(&i915->drm.struct_mutex);
+ goto out_flush;
+ }
+ /* One ring interleaved between requests from all cpus */
+ t[id].max_batch /= num_online_cpus() + 1;
+ pr_debug("Limiting batches to %d requests on %s\n",
+ t[id].max_batch, engine->name);
+
+ for (n = 0; n < ncpus; n++) {
+ struct task_struct *tsk;
+
+ tsk = kthread_run(__igt_breadcrumbs_smoketest,
+ &t[id], "igt/%d.%d", id, n);
+ if (IS_ERR(tsk)) {
+ ret = PTR_ERR(tsk);
+ mutex_unlock(&i915->drm.struct_mutex);
+ goto out_flush;
+ }
+
+ get_task_struct(tsk);
+ threads[id * ncpus + n] = tsk;
+ }
+ }
+ mutex_unlock(&i915->drm.struct_mutex);
+
+ msleep(jiffies_to_msecs(i915_selftest.timeout_jiffies));
+
+out_flush:
+ num_waits = 0;
+ num_fences = 0;
+ for_each_engine(engine, i915, id) {
+ for (n = 0; n < ncpus; n++) {
+ struct task_struct *tsk = threads[id * ncpus + n];
+ int err;
+
+ if (!tsk)
+ continue;
+
+ err = kthread_stop(tsk);
+ if (err < 0 && !ret)
+ ret = err;
+
+ put_task_struct(tsk);
+ }
+
+ num_waits += atomic_long_read(&t[id].num_waits);
+ num_fences += atomic_long_read(&t[id].num_fences);
+ }
+ pr_info("Completed %lu waits for %lu fences across %d engines and %d cpus\n",
+ num_waits, num_fences, RUNTIME_INFO(i915)->num_rings, ncpus);
+
+ mutex_lock(&i915->drm.struct_mutex);
+ ret = igt_live_test_end(&live) ?: ret;
+out_contexts:
+ mutex_unlock(&i915->drm.struct_mutex);
+ kfree(t[0].contexts);
+out_threads:
+ kfree(threads);
+out_file:
+ mock_file_free(i915, file);
+out_rpm:
+ intel_runtime_pm_put(i915, wakeref);
+
+ return ret;
+}
+
int i915_request_live_selftests(struct drm_i915_private *i915)
{
static const struct i915_subtest tests[] = {
@@ -819,6 +1243,7 @@ int i915_request_live_selftests(struct drm_i915_private *i915)
SUBTEST(live_all_engines),
SUBTEST(live_sequential_engines),
SUBTEST(live_empty_request),
+ SUBTEST(live_breadcrumbs_smoketest),
};
if (i915_terminally_wedged(&i915->gpu_error))
diff --git a/drivers/gpu/drm/i915/selftests/igt_spinner.c b/drivers/gpu/drm/i915/selftests/igt_spinner.c
index 0e70df0230b8..9ebd9225684e 100644
--- a/drivers/gpu/drm/i915/selftests/igt_spinner.c
+++ b/drivers/gpu/drm/i915/selftests/igt_spinner.c
@@ -185,11 +185,6 @@ void igt_spinner_fini(struct igt_spinner *spin)
bool igt_wait_for_spinner(struct igt_spinner *spin, struct i915_request *rq)
{
- if (!wait_event_timeout(rq->execute,
- READ_ONCE(rq->global_seqno),
- msecs_to_jiffies(10)))
- return false;
-
return !(wait_for_us(i915_seqno_passed(hws_seqno(spin, rq),
rq->fence.seqno),
10) &&
diff --git a/drivers/gpu/drm/i915/selftests/intel_breadcrumbs.c b/drivers/gpu/drm/i915/selftests/intel_breadcrumbs.c
deleted file mode 100644
index f03b407fdbe2..000000000000
--- a/drivers/gpu/drm/i915/selftests/intel_breadcrumbs.c
+++ /dev/null
@@ -1,470 +0,0 @@
-/*
- * Copyright © 2016 Intel Corporation
- *
- * Permission is hereby granted, free of charge, to any person obtaining a
- * copy of this software and associated documentation files (the "Software"),
- * to deal in the Software without restriction, including without limitation
- * the rights to use, copy, modify, merge, publish, distribute, sublicense,
- * and/or sell copies of the Software, and to permit persons to whom the
- * Software is furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice (including the next
- * paragraph) shall be included in all copies or substantial portions of the
- * Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
- * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
- * IN THE SOFTWARE.
- *
- */
-
-#include "../i915_selftest.h"
-#include "i915_random.h"
-
-#include "mock_gem_device.h"
-#include "mock_engine.h"
-
-static int check_rbtree(struct intel_engine_cs *engine,
- const unsigned long *bitmap,
- const struct intel_wait *waiters,
- const int count)
-{
- struct intel_breadcrumbs *b = &engine->breadcrumbs;
- struct rb_node *rb;
- int n;
-
- if (&b->irq_wait->node != rb_first(&b->waiters)) {
- pr_err("First waiter does not match first element of wait-tree\n");
- return -EINVAL;
- }
-
- n = find_first_bit(bitmap, count);
- for (rb = rb_first(&b->waiters); rb; rb = rb_next(rb)) {
- struct intel_wait *w = container_of(rb, typeof(*w), node);
- int idx = w - waiters;
-
- if (!test_bit(idx, bitmap)) {
- pr_err("waiter[%d, seqno=%d] removed but still in wait-tree\n",
- idx, w->seqno);
- return -EINVAL;
- }
-
- if (n != idx) {
- pr_err("waiter[%d, seqno=%d] does not match expected next element in tree [%d]\n",
- idx, w->seqno, n);
- return -EINVAL;
- }
-
- n = find_next_bit(bitmap, count, n + 1);
- }
-
- return 0;
-}
-
-static int check_completion(struct intel_engine_cs *engine,
- const unsigned long *bitmap,
- const struct intel_wait *waiters,
- const int count)
-{
- int n;
-
- for (n = 0; n < count; n++) {
- if (intel_wait_complete(&waiters[n]) != !!test_bit(n, bitmap))
- continue;
-
- pr_err("waiter[%d, seqno=%d] is %s, but expected %s\n",
- n, waiters[n].seqno,
- intel_wait_complete(&waiters[n]) ? "complete" : "active",
- test_bit(n, bitmap) ? "active" : "complete");
- return -EINVAL;
- }
-
- return 0;
-}
-
-static int check_rbtree_empty(struct intel_engine_cs *engine)
-{
- struct intel_breadcrumbs *b = &engine->breadcrumbs;
-
- if (b->irq_wait) {
- pr_err("Empty breadcrumbs still has a waiter\n");
- return -EINVAL;
- }
-
- if (!RB_EMPTY_ROOT(&b->waiters)) {
- pr_err("Empty breadcrumbs, but wait-tree not empty\n");
- return -EINVAL;
- }
-
- return 0;
-}
-
-static int igt_random_insert_remove(void *arg)
-{
- const u32 seqno_bias = 0x1000;
- I915_RND_STATE(prng);
- struct intel_engine_cs *engine = arg;
- struct intel_wait *waiters;
- const int count = 4096;
- unsigned int *order;
- unsigned long *bitmap;
- int err = -ENOMEM;
- int n;
-
- mock_engine_reset(engine);
-
- waiters = kvmalloc_array(count, sizeof(*waiters), GFP_KERNEL);
- if (!waiters)
- goto out_engines;
-
- bitmap = kcalloc(DIV_ROUND_UP(count, BITS_PER_LONG), sizeof(*bitmap),
- GFP_KERNEL);
- if (!bitmap)
- goto out_waiters;
-
- order = i915_random_order(count, &prng);
- if (!order)
- goto out_bitmap;
-
- for (n = 0; n < count; n++)
- intel_wait_init_for_seqno(&waiters[n], seqno_bias + n);
-
- err = check_rbtree(engine, bitmap, waiters, count);
- if (err)
- goto out_order;
-
- /* Add and remove waiters into the rbtree in random order. At each
- * step, we verify that the rbtree is correctly ordered.
- */
- for (n = 0; n < count; n++) {
- int i = order[n];
-
- intel_engine_add_wait(engine, &waiters[i]);
- __set_bit(i, bitmap);
-
- err = check_rbtree(engine, bitmap, waiters, count);
- if (err)
- goto out_order;
- }
-
- i915_random_reorder(order, count, &prng);
- for (n = 0; n < count; n++) {
- int i = order[n];
-
- intel_engine_remove_wait(engine, &waiters[i]);
- __clear_bit(i, bitmap);
-
- err = check_rbtree(engine, bitmap, waiters, count);
- if (err)
- goto out_order;
- }
-
- err = check_rbtree_empty(engine);
-out_order:
- kfree(order);
-out_bitmap:
- kfree(bitmap);
-out_waiters:
- kvfree(waiters);
-out_engines:
- mock_engine_flush(engine);
- return err;
-}
-
-static int igt_insert_complete(void *arg)
-{
- const u32 seqno_bias = 0x1000;
- struct intel_engine_cs *engine = arg;
- struct intel_wait *waiters;
- const int count = 4096;
- unsigned long *bitmap;
- int err = -ENOMEM;
- int n, m;
-
- mock_engine_reset(engine);
-
- waiters = kvmalloc_array(count, sizeof(*waiters), GFP_KERNEL);
- if (!waiters)
- goto out_engines;
-
- bitmap = kcalloc(DIV_ROUND_UP(count, BITS_PER_LONG), sizeof(*bitmap),
- GFP_KERNEL);
- if (!bitmap)
- goto out_waiters;
-
- for (n = 0; n < count; n++) {
- intel_wait_init_for_seqno(&waiters[n], n + seqno_bias);
- intel_engine_add_wait(engine, &waiters[n]);
- __set_bit(n, bitmap);
- }
- err = check_rbtree(engine, bitmap, waiters, count);
- if (err)
- goto out_bitmap;
-
- /* On each step, we advance the seqno so that several waiters are then
- * complete (we increase the seqno by increasingly larger values to
- * retire more and more waiters at once). All retired waiters should
- * be woken and removed from the rbtree, and so that we check.
- */
- for (n = 0; n < count; n = m) {
- int seqno = 2 * n;
-
- GEM_BUG_ON(find_first_bit(bitmap, count) != n);
-
- if (intel_wait_complete(&waiters[n])) {
- pr_err("waiter[%d, seqno=%d] completed too early\n",
- n, waiters[n].seqno);
- err = -EINVAL;
- goto out_bitmap;
- }
-
- /* complete the following waiters */
- mock_seqno_advance(engine, seqno + seqno_bias);
- for (m = n; m <= seqno; m++) {
- if (m == count)
- break;
-
- GEM_BUG_ON(!test_bit(m, bitmap));
- __clear_bit(m, bitmap);
- }
-
- intel_engine_remove_wait(engine, &waiters[n]);
- RB_CLEAR_NODE(&waiters[n].node);
-
- err = check_rbtree(engine, bitmap, waiters, count);
- if (err) {
- pr_err("rbtree corrupt after seqno advance to %d\n",
- seqno + seqno_bias);
- goto out_bitmap;
- }
-
- err = check_completion(engine, bitmap, waiters, count);
- if (err) {
- pr_err("completions after seqno advance to %d failed\n",
- seqno + seqno_bias);
- goto out_bitmap;
- }
- }
-
- err = check_rbtree_empty(engine);
-out_bitmap:
- kfree(bitmap);
-out_waiters:
- kvfree(waiters);
-out_engines:
- mock_engine_flush(engine);
- return err;
-}
-
-struct igt_wakeup {
- struct task_struct *tsk;
- atomic_t *ready, *set, *done;
- struct intel_engine_cs *engine;
- unsigned long flags;
-#define STOP 0
-#define IDLE 1
- wait_queue_head_t *wq;
- u32 seqno;
-};
-
-static bool wait_for_ready(struct igt_wakeup *w)
-{
- DEFINE_WAIT(ready);
-
- set_bit(IDLE, &w->flags);
- if (atomic_dec_and_test(w->done))
- wake_up_var(w->done);
-
- if (test_bit(STOP, &w->flags))
- goto out;
-
- for (;;) {
- prepare_to_wait(w->wq, &ready, TASK_INTERRUPTIBLE);
- if (atomic_read(w->ready) == 0)
- break;
-
- schedule();
- }
- finish_wait(w->wq, &ready);
-
-out:
- clear_bit(IDLE, &w->flags);
- if (atomic_dec_and_test(w->set))
- wake_up_var(w->set);
-
- return !test_bit(STOP, &w->flags);
-}
-
-static int igt_wakeup_thread(void *arg)
-{
- struct igt_wakeup *w = arg;
- struct intel_wait wait;
-
- while (wait_for_ready(w)) {
- GEM_BUG_ON(kthread_should_stop());
-
- intel_wait_init_for_seqno(&wait, w->seqno);
- intel_engine_add_wait(w->engine, &wait);
- for (;;) {
- set_current_state(TASK_UNINTERRUPTIBLE);
- if (i915_seqno_passed(intel_engine_get_seqno(w->engine),
- w->seqno))
- break;
-
- if (test_bit(STOP, &w->flags)) /* emergency escape */
- break;
-
- schedule();
- }
- intel_engine_remove_wait(w->engine, &wait);
- __set_current_state(TASK_RUNNING);
- }
-
- return 0;
-}
-
-static void igt_wake_all_sync(atomic_t *ready,
- atomic_t *set,
- atomic_t *done,
- wait_queue_head_t *wq,
- int count)
-{
- atomic_set(set, count);
- atomic_set(ready, 0);
- wake_up_all(wq);
-
- wait_var_event(set, !atomic_read(set));
- atomic_set(ready, count);
- atomic_set(done, count);
-}
-
-static int igt_wakeup(void *arg)
-{
- I915_RND_STATE(prng);
- struct intel_engine_cs *engine = arg;
- struct igt_wakeup *waiters;
- DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wq);
- const int count = 4096;
- const u32 max_seqno = count / 4;
- atomic_t ready, set, done;
- int err = -ENOMEM;
- int n, step;
-
- mock_engine_reset(engine);
-
- waiters = kvmalloc_array(count, sizeof(*waiters), GFP_KERNEL);
- if (!waiters)
- goto out_engines;
-
- /* Create a large number of threads, each waiting on a random seqno.
- * Multiple waiters will be waiting for the same seqno.
- */
- atomic_set(&ready, count);
- for (n = 0; n < count; n++) {
- waiters[n].wq = &wq;
- waiters[n].ready = &ready;
- waiters[n].set = &set;
- waiters[n].done = &done;
- waiters[n].engine = engine;
- waiters[n].flags = BIT(IDLE);
-
- waiters[n].tsk = kthread_run(igt_wakeup_thread, &waiters[n],
- "i915/igt:%d", n);
- if (IS_ERR(waiters[n].tsk))
- goto out_waiters;
-
- get_task_struct(waiters[n].tsk);
- }
-
- for (step = 1; step <= max_seqno; step <<= 1) {
- u32 seqno;
-
- /* The waiter threads start paused as we assign them a random
- * seqno and reset the engine. Once the engine is reset,
- * we signal that the threads may begin their wait upon their
- * seqno.
- */
- for (n = 0; n < count; n++) {
- GEM_BUG_ON(!test_bit(IDLE, &waiters[n].flags));
- waiters[n].seqno =
- 1 + prandom_u32_state(&prng) % max_seqno;
- }
- mock_seqno_advance(engine, 0);
- igt_wake_all_sync(&ready, &set, &done, &wq, count);
-
- /* Simulate the GPU doing chunks of work, with one or more
- * seqno appearing to finish at the same time. A random number
- * of threads will be waiting upon the update and hopefully be
- * woken.
- */
- for (seqno = 1; seqno <= max_seqno + step; seqno += step) {
- usleep_range(50, 500);
- mock_seqno_advance(engine, seqno);
- }
- GEM_BUG_ON(intel_engine_get_seqno(engine) < 1 + max_seqno);
-
- /* With the seqno now beyond any of the waiting threads, they
- * should all be woken, see that they are complete and signal
- * that they are ready for the next test. We wait until all
- * threads are complete and waiting for us (i.e. not a seqno).
- */
- if (!wait_var_event_timeout(&done,
- !atomic_read(&done), 10 * HZ)) {
- pr_err("Timed out waiting for %d remaining waiters\n",
- atomic_read(&done));
- err = -ETIMEDOUT;
- break;
- }
-
- err = check_rbtree_empty(engine);
- if (err)
- break;
- }
-
-out_waiters:
- for (n = 0; n < count; n++) {
- if (IS_ERR(waiters[n].tsk))
- break;
-
- set_bit(STOP, &waiters[n].flags);
- }
- mock_seqno_advance(engine, INT_MAX); /* wakeup any broken waiters */
- igt_wake_all_sync(&ready, &set, &done, &wq, n);
-
- for (n = 0; n < count; n++) {
- if (IS_ERR(waiters[n].tsk))
- break;
-
- kthread_stop(waiters[n].tsk);
- put_task_struct(waiters[n].tsk);
- }
-
- kvfree(waiters);
-out_engines:
- mock_engine_flush(engine);
- return err;
-}
-
-int intel_breadcrumbs_mock_selftests(void)
-{
- static const struct i915_subtest tests[] = {
- SUBTEST(igt_random_insert_remove),
- SUBTEST(igt_insert_complete),
- SUBTEST(igt_wakeup),
- };
- struct drm_i915_private *i915;
- int err;
-
- i915 = mock_gem_device();
- if (!i915)
- return -ENOMEM;
-
- err = i915_subtests(tests, i915->engine[RCS]);
- drm_dev_put(&i915->drm);
-
- return err;
-}
diff --git a/drivers/gpu/drm/i915/selftests/intel_hangcheck.c b/drivers/gpu/drm/i915/selftests/intel_hangcheck.c
index 2c38ea5892d9..7b6f3bea9ef8 100644
--- a/drivers/gpu/drm/i915/selftests/intel_hangcheck.c
+++ b/drivers/gpu/drm/i915/selftests/intel_hangcheck.c
@@ -1127,7 +1127,7 @@ static int __igt_reset_evict_vma(struct drm_i915_private *i915,
wait_for_completion(&arg.completion);
- if (wait_for(waitqueue_active(&rq->execute), 10)) {
+ if (wait_for(!list_empty(&rq->fence.cb_list), 10)) {
struct drm_printer p = drm_info_printer(i915->drm.dev);
pr_err("igt/evict_vma kthread did not wait\n");
diff --git a/drivers/gpu/drm/i915/selftests/lib_sw_fence.c b/drivers/gpu/drm/i915/selftests/lib_sw_fence.c
index b26f07b55d86..2bfa72c1654b 100644
--- a/drivers/gpu/drm/i915/selftests/lib_sw_fence.c
+++ b/drivers/gpu/drm/i915/selftests/lib_sw_fence.c
@@ -76,3 +76,57 @@ void timed_fence_fini(struct timed_fence *tf)
destroy_timer_on_stack(&tf->timer);
i915_sw_fence_fini(&tf->fence);
}
+
+struct heap_fence {
+ struct i915_sw_fence fence;
+ union {
+ struct kref ref;
+ struct rcu_head rcu;
+ };
+};
+
+static int __i915_sw_fence_call
+heap_fence_notify(struct i915_sw_fence *fence, enum i915_sw_fence_notify state)
+{
+ struct heap_fence *h = container_of(fence, typeof(*h), fence);
+
+ switch (state) {
+ case FENCE_COMPLETE:
+ break;
+
+ case FENCE_FREE:
+ heap_fence_put(&h->fence);
+ }
+
+ return NOTIFY_DONE;
+}
+
+struct i915_sw_fence *heap_fence_create(gfp_t gfp)
+{
+ struct heap_fence *h;
+
+ h = kmalloc(sizeof(*h), gfp);
+ if (!h)
+ return NULL;
+
+ i915_sw_fence_init(&h->fence, heap_fence_notify);
+ refcount_set(&h->ref.refcount, 2);
+
+ return &h->fence;
+}
+
+static void heap_fence_release(struct kref *ref)
+{
+ struct heap_fence *h = container_of(ref, typeof(*h), ref);
+
+ i915_sw_fence_fini(&h->fence);
+
+ kfree_rcu(h, rcu);
+}
+
+void heap_fence_put(struct i915_sw_fence *fence)
+{
+ struct heap_fence *h = container_of(fence, typeof(*h), fence);
+
+ kref_put(&h->ref, heap_fence_release);
+}
diff --git a/drivers/gpu/drm/i915/selftests/lib_sw_fence.h b/drivers/gpu/drm/i915/selftests/lib_sw_fence.h
index 474aafb92ae1..1f9927e10f3a 100644
--- a/drivers/gpu/drm/i915/selftests/lib_sw_fence.h
+++ b/drivers/gpu/drm/i915/selftests/lib_sw_fence.h
@@ -39,4 +39,7 @@ struct timed_fence {
void timed_fence_init(struct timed_fence *tf, unsigned long expires);
void timed_fence_fini(struct timed_fence *tf);
+struct i915_sw_fence *heap_fence_create(gfp_t gfp);
+void heap_fence_put(struct i915_sw_fence *fence);
+
#endif /* _LIB_SW_FENCE_H_ */
diff --git a/drivers/gpu/drm/i915/selftests/mock_engine.c b/drivers/gpu/drm/i915/selftests/mock_engine.c
index 3b226ebc6bc4..08f0cab02e0f 100644
--- a/drivers/gpu/drm/i915/selftests/mock_engine.c
+++ b/drivers/gpu/drm/i915/selftests/mock_engine.c
@@ -86,17 +86,21 @@ static struct mock_request *first_request(struct mock_engine *engine)
static void advance(struct mock_request *request)
{
list_del_init(&request->link);
- mock_seqno_advance(request->base.engine, request->base.global_seqno);
+ intel_engine_write_global_seqno(request->base.engine,
+ request->base.global_seqno);
i915_request_mark_complete(&request->base);
GEM_BUG_ON(!i915_request_completed(&request->base));
+
+ intel_engine_queue_breadcrumbs(request->base.engine);
}
static void hw_delay_complete(struct timer_list *t)
{
struct mock_engine *engine = from_timer(engine, t, hw_delay);
struct mock_request *request;
+ unsigned long flags;
- spin_lock(&engine->hw_lock);
+ spin_lock_irqsave(&engine->hw_lock, flags);
/* Timer fired, first request is complete */
request = first_request(engine);
@@ -116,7 +120,7 @@ static void hw_delay_complete(struct timer_list *t)
advance(request);
}
- spin_unlock(&engine->hw_lock);
+ spin_unlock_irqrestore(&engine->hw_lock, flags);
}
static void mock_context_unpin(struct intel_context *ce)
@@ -191,11 +195,12 @@ static void mock_submit_request(struct i915_request *request)
struct mock_request *mock = container_of(request, typeof(*mock), base);
struct mock_engine *engine =
container_of(request->engine, typeof(*engine), base);
+ unsigned long flags;
i915_request_submit(request);
GEM_BUG_ON(!request->global_seqno);
- spin_lock_irq(&engine->hw_lock);
+ spin_lock_irqsave(&engine->hw_lock, flags);
list_add_tail(&mock->link, &engine->hw_queue);
if (mock->link.prev == &engine->hw_queue) {
if (mock->delay)
@@ -203,7 +208,7 @@ static void mock_submit_request(struct i915_request *request)
else
advance(mock);
}
- spin_unlock_irq(&engine->hw_lock);
+ spin_unlock_irqrestore(&engine->hw_lock, flags);
}
struct intel_engine_cs *mock_engine(struct drm_i915_private *i915,
@@ -273,7 +278,7 @@ void mock_engine_flush(struct intel_engine_cs *engine)
void mock_engine_reset(struct intel_engine_cs *engine)
{
- intel_write_status_page(engine, I915_GEM_HWS_INDEX, 0);
+ intel_engine_write_global_seqno(engine, 0);
}
void mock_engine_free(struct intel_engine_cs *engine)
diff --git a/drivers/gpu/drm/i915/selftests/mock_engine.h b/drivers/gpu/drm/i915/selftests/mock_engine.h
index 133d0c21790d..b9cc3a245f16 100644
--- a/drivers/gpu/drm/i915/selftests/mock_engine.h
+++ b/drivers/gpu/drm/i915/selftests/mock_engine.h
@@ -46,10 +46,4 @@ void mock_engine_flush(struct intel_engine_cs *engine);
void mock_engine_reset(struct intel_engine_cs *engine);
void mock_engine_free(struct intel_engine_cs *engine);
-static inline void mock_seqno_advance(struct intel_engine_cs *engine, u32 seqno)
-{
- intel_write_status_page(engine, I915_GEM_HWS_INDEX, seqno);
- intel_engine_wakeup(engine);
-}
-
#endif /* !__MOCK_ENGINE_H__ */
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 8/8] drm/i915: Drop fake breadcrumb irq
2019-01-29 8:58 [PATCH 1/8] drm/i915: Rename execlists->queue_priority to preempt_priority_hint Chris Wilson
` (5 preceding siblings ...)
2019-01-29 8:58 ` [PATCH 7/8] drm/i915: Replace global breadcrumbs with per-context interrupt tracking Chris Wilson
@ 2019-01-29 8:58 ` Chris Wilson
2019-01-29 9:40 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/8] drm/i915: Rename execlists->queue_priority to preempt_priority_hint Patchwork
` (5 subsequent siblings)
12 siblings, 0 replies; 23+ messages in thread
From: Chris Wilson @ 2019-01-29 8:58 UTC (permalink / raw)
To: intel-gfx
Missed breadcrumb detection is defunct due to the tight coupling with
dma_fence signaling and the myriad ways we may signal fences from
everywhere but from an interrupt, i.e. we frequently signal a fence
before we even see its interrupt. This means that even if we miss an
interrupt for a fence, it still is signaled before our breadcrumb
hangcheck fires, so simplify the breadcrumb hangchecking by moving it
into the GPU hangcheck and forgo fake interrupts.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/i915_debugfs.c | 93 -----------
drivers/gpu/drm/i915/i915_gpu_error.c | 2 -
drivers/gpu/drm/i915/i915_gpu_error.h | 5 -
drivers/gpu/drm/i915/intel_breadcrumbs.c | 147 +-----------------
drivers/gpu/drm/i915/intel_hangcheck.c | 2 +
drivers/gpu/drm/i915/intel_ringbuffer.h | 5 -
.../gpu/drm/i915/selftests/igt_live_test.c | 7 -
7 files changed, 5 insertions(+), 256 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index b1ac0f78cb42..fa2c226fc779 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1322,9 +1322,6 @@ static int i915_hangcheck_info(struct seq_file *m, void *unused)
intel_engine_last_submit(engine),
jiffies_to_msecs(jiffies -
engine->hangcheck.action_timestamp));
- seq_printf(m, "\tfake irq active? %s\n",
- yesno(test_bit(engine->id,
- &dev_priv->gpu_error.missed_irq_rings)));
seq_printf(m, "\tACTHD = 0x%08llx [current 0x%08llx]\n",
(long long)engine->hangcheck.acthd,
@@ -3900,94 +3897,6 @@ DEFINE_SIMPLE_ATTRIBUTE(i915_wedged_fops,
i915_wedged_get, i915_wedged_set,
"%llu\n");
-static int
-fault_irq_set(struct drm_i915_private *i915,
- unsigned long *irq,
- unsigned long val)
-{
- int err;
-
- err = mutex_lock_interruptible(&i915->drm.struct_mutex);
- if (err)
- return err;
-
- err = i915_gem_wait_for_idle(i915,
- I915_WAIT_LOCKED |
- I915_WAIT_INTERRUPTIBLE,
- MAX_SCHEDULE_TIMEOUT);
- if (err)
- goto err_unlock;
-
- *irq = val;
- mutex_unlock(&i915->drm.struct_mutex);
-
- /* Flush idle worker to disarm irq */
- drain_delayed_work(&i915->gt.idle_work);
-
- return 0;
-
-err_unlock:
- mutex_unlock(&i915->drm.struct_mutex);
- return err;
-}
-
-static int
-i915_ring_missed_irq_get(void *data, u64 *val)
-{
- struct drm_i915_private *dev_priv = data;
-
- *val = dev_priv->gpu_error.missed_irq_rings;
- return 0;
-}
-
-static int
-i915_ring_missed_irq_set(void *data, u64 val)
-{
- struct drm_i915_private *i915 = data;
-
- return fault_irq_set(i915, &i915->gpu_error.missed_irq_rings, val);
-}
-
-DEFINE_SIMPLE_ATTRIBUTE(i915_ring_missed_irq_fops,
- i915_ring_missed_irq_get, i915_ring_missed_irq_set,
- "0x%08llx\n");
-
-static int
-i915_ring_test_irq_get(void *data, u64 *val)
-{
- struct drm_i915_private *dev_priv = data;
-
- *val = dev_priv->gpu_error.test_irq_rings;
-
- return 0;
-}
-
-static int
-i915_ring_test_irq_set(void *data, u64 val)
-{
- struct drm_i915_private *i915 = data;
-
- /* GuC keeps the user interrupt permanently enabled for submission */
- if (USES_GUC_SUBMISSION(i915))
- return -ENODEV;
-
- /*
- * From icl, we can no longer individually mask interrupt generation
- * from each engine.
- */
- if (INTEL_GEN(i915) >= 11)
- return -ENODEV;
-
- val &= INTEL_INFO(i915)->ring_mask;
- DRM_DEBUG_DRIVER("Masking interrupts on rings 0x%08llx\n", val);
-
- return fault_irq_set(i915, &i915->gpu_error.test_irq_rings, val);
-}
-
-DEFINE_SIMPLE_ATTRIBUTE(i915_ring_test_irq_fops,
- i915_ring_test_irq_get, i915_ring_test_irq_set,
- "0x%08llx\n");
-
#define DROP_UNBOUND BIT(0)
#define DROP_BOUND BIT(1)
#define DROP_RETIRE BIT(2)
@@ -4751,8 +4660,6 @@ static const struct i915_debugfs_files {
} i915_debugfs_files[] = {
{"i915_wedged", &i915_wedged_fops},
{"i915_cache_sharing", &i915_cache_sharing_fops},
- {"i915_ring_missed_irq", &i915_ring_missed_irq_fops},
- {"i915_ring_test_irq", &i915_ring_test_irq_fops},
{"i915_gem_drop_caches", &i915_drop_caches_fops},
#if IS_ENABLED(CONFIG_DRM_I915_CAPTURE_ERROR)
{"i915_error_state", &i915_error_state_fops},
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 304a7ef7f7fb..6e2e5ed2bd0a 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -723,8 +723,6 @@ static void __err_print_to_sgl(struct drm_i915_error_state_buf *m,
err_printf(m, "FORCEWAKE: 0x%08x\n", error->forcewake);
err_printf(m, "DERRMR: 0x%08x\n", error->derrmr);
err_printf(m, "CCID: 0x%08x\n", error->ccid);
- err_printf(m, "Missed interrupts: 0x%08lx\n",
- m->i915->gpu_error.missed_irq_rings);
for (i = 0; i < error->nfence; i++)
err_printf(m, " fence[%d] = %08llx\n", i, error->fence[i]);
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.h b/drivers/gpu/drm/i915/i915_gpu_error.h
index 74757c424aab..53b1f22dd365 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.h
+++ b/drivers/gpu/drm/i915/i915_gpu_error.h
@@ -204,8 +204,6 @@ struct i915_gpu_error {
atomic_t pending_fb_pin;
- unsigned long missed_irq_rings;
-
/**
* State variable controlling the reset flow and count
*
@@ -274,9 +272,6 @@ struct i915_gpu_error {
*/
wait_queue_head_t reset_queue;
- /* For missed irq/seqno simulation. */
- unsigned long test_irq_rings;
-
struct i915_gpu_restart *restart;
};
diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index b0795b0ad227..cacaa1d04d17 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -91,7 +91,6 @@ bool intel_engine_breadcrumbs_irq(struct intel_engine_cs *engine)
spin_lock(&b->irq_lock);
- b->irq_fired = true;
if (b->irq_armed && list_empty(&b->signalers))
__intel_breadcrumbs_disarm_irq(b);
@@ -172,86 +171,6 @@ static void signal_irq_work(struct irq_work *work)
intel_engine_breadcrumbs_irq(engine);
}
-static unsigned long wait_timeout(void)
-{
- return round_jiffies_up(jiffies + DRM_I915_HANGCHECK_JIFFIES);
-}
-
-static noinline void missed_breadcrumb(struct intel_engine_cs *engine)
-{
- if (GEM_SHOW_DEBUG()) {
- struct drm_printer p = drm_debug_printer(__func__);
-
- intel_engine_dump(engine, &p,
- "%s missed breadcrumb at %pS\n",
- engine->name, __builtin_return_address(0));
- }
-
- set_bit(engine->id, &engine->i915->gpu_error.missed_irq_rings);
-}
-
-static void intel_breadcrumbs_hangcheck(struct timer_list *t)
-{
- struct intel_engine_cs *engine =
- from_timer(engine, t, breadcrumbs.hangcheck);
- struct intel_breadcrumbs *b = &engine->breadcrumbs;
-
- if (!b->irq_armed)
- return;
-
- if (b->irq_fired)
- goto rearm;
-
- /*
- * We keep the hangcheck timer alive until we disarm the irq, even
- * if there are no waiters at present.
- *
- * If the waiter was currently running, assume it hasn't had a chance
- * to process the pending interrupt (e.g, low priority task on a loaded
- * system) and wait until it sleeps before declaring a missed interrupt.
- *
- * If the waiter was asleep (and not even pending a wakeup), then we
- * must have missed an interrupt as the GPU has stopped advancing
- * but we still have a waiter. Assuming all batches complete within
- * DRM_I915_HANGCHECK_JIFFIES [1.5s]!
- */
- synchronize_hardirq(engine->i915->drm.irq);
- if (intel_engine_signal_breadcrumbs(engine)) {
- missed_breadcrumb(engine);
- mod_timer(&b->fake_irq, jiffies + 1);
- } else {
-rearm:
- b->irq_fired = false;
- mod_timer(&b->hangcheck, wait_timeout());
- }
-}
-
-static void intel_breadcrumbs_fake_irq(struct timer_list *t)
-{
- struct intel_engine_cs *engine =
- from_timer(engine, t, breadcrumbs.fake_irq);
- struct intel_breadcrumbs *b = &engine->breadcrumbs;
-
- /*
- * The timer persists in case we cannot enable interrupts,
- * or if we have previously seen seqno/interrupt incoherency
- * ("missed interrupt" syndrome, better known as a "missed breadcrumb").
- * Here the worker will wake up every jiffie in order to kick the
- * oldest waiter to do the coherent seqno check.
- */
-
- if (!intel_engine_signal_breadcrumbs(engine) && !b->irq_armed)
- return;
-
- /* If the user has disabled the fake-irq, restore the hangchecking */
- if (!test_bit(engine->id, &engine->i915->gpu_error.missed_irq_rings)) {
- mod_timer(&b->hangcheck, wait_timeout());
- return;
- }
-
- mod_timer(&b->fake_irq, jiffies + 1);
-}
-
void intel_engine_pin_breadcrumbs_irq(struct intel_engine_cs *engine)
{
struct intel_breadcrumbs *b = &engine->breadcrumbs;
@@ -274,43 +193,14 @@ void intel_engine_unpin_breadcrumbs_irq(struct intel_engine_cs *engine)
spin_unlock_irq(&b->irq_lock);
}
-static bool use_fake_irq(const struct intel_breadcrumbs *b)
-{
- const struct intel_engine_cs *engine =
- container_of(b, struct intel_engine_cs, breadcrumbs);
-
- if (!test_bit(engine->id, &engine->i915->gpu_error.missed_irq_rings))
- return false;
-
- /*
- * Only start with the heavy weight fake irq timer if we have not
- * seen any interrupts since enabling it the first time. If the
- * interrupts are still arriving, it means we made a mistake in our
- * engine->seqno_barrier(), a timing error that should be transient
- * and unlikely to reoccur.
- */
- return !b->irq_fired;
-}
-
-static void enable_fake_irq(struct intel_breadcrumbs *b)
-{
- /* Ensure we never sleep indefinitely */
- if (!b->irq_enabled || use_fake_irq(b))
- mod_timer(&b->fake_irq, jiffies + 1);
- else
- mod_timer(&b->hangcheck, wait_timeout());
-}
-
-static bool __intel_breadcrumbs_arm_irq(struct intel_breadcrumbs *b)
+static void __intel_breadcrumbs_arm_irq(struct intel_breadcrumbs *b)
{
struct intel_engine_cs *engine =
container_of(b, struct intel_engine_cs, breadcrumbs);
- struct drm_i915_private *i915 = engine->i915;
- bool enabled;
lockdep_assert_held(&b->irq_lock);
if (b->irq_armed)
- return false;
+ return;
/*
* The breadcrumb irq will be disarmed on the interrupt after the
@@ -328,16 +218,8 @@ static bool __intel_breadcrumbs_arm_irq(struct intel_breadcrumbs *b)
* the driver is idle) we disarm the breadcrumbs.
*/
- /* No interrupts? Kick the waiter every jiffie! */
- enabled = false;
- if (!b->irq_enabled++ &&
- !test_bit(engine->id, &i915->gpu_error.test_irq_rings)) {
+ if (!b->irq_enabled++)
irq_enable(engine);
- enabled = true;
- }
-
- enable_fake_irq(b);
- return enabled;
}
void intel_engine_init_breadcrumbs(struct intel_engine_cs *engine)
@@ -348,18 +230,6 @@ void intel_engine_init_breadcrumbs(struct intel_engine_cs *engine)
INIT_LIST_HEAD(&b->signalers);
init_irq_work(&b->irq_work, signal_irq_work);
-
- timer_setup(&b->fake_irq, intel_breadcrumbs_fake_irq, 0);
- timer_setup(&b->hangcheck, intel_breadcrumbs_hangcheck, 0);
-}
-
-static void cancel_fake_irq(struct intel_engine_cs *engine)
-{
- struct intel_breadcrumbs *b = &engine->breadcrumbs;
-
- del_timer_sync(&b->fake_irq); /* may queue b->hangcheck */
- del_timer_sync(&b->hangcheck);
- clear_bit(engine->id, &engine->i915->gpu_error.missed_irq_rings);
}
void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
@@ -369,13 +239,6 @@ void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
spin_lock_irqsave(&b->irq_lock, flags);
- /*
- * Leave the fake_irq timer enabled (if it is running), but clear the
- * bit so that it turns itself off on its next wake up and goes back
- * to the long hangcheck interval if still required.
- */
- clear_bit(engine->id, &engine->i915->gpu_error.missed_irq_rings);
-
if (b->irq_enabled)
irq_enable(engine);
else
@@ -386,7 +249,6 @@ void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine)
{
- cancel_fake_irq(engine);
}
bool i915_request_enable_breadcrumb(struct i915_request *rq)
@@ -482,7 +344,4 @@ void intel_engine_print_breadcrumbs(struct intel_engine_cs *engine,
}
}
spin_unlock_irq(&b->irq_lock);
-
- if (test_bit(engine->id, &engine->i915->gpu_error.missed_irq_rings))
- drm_printf(p, "Fake irq active\n");
}
diff --git a/drivers/gpu/drm/i915/intel_hangcheck.c b/drivers/gpu/drm/i915/intel_hangcheck.c
index 5662d6fed523..a219c796e56d 100644
--- a/drivers/gpu/drm/i915/intel_hangcheck.c
+++ b/drivers/gpu/drm/i915/intel_hangcheck.c
@@ -275,6 +275,8 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
for_each_engine(engine, dev_priv, id) {
struct hangcheck hc;
+ intel_engine_signal_breadcrumbs(engine);
+
hangcheck_load_sample(engine, &hc);
hangcheck_accumulate_sample(engine, &hc);
hangcheck_store_sample(engine, &hc);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index efef2aa1abce..8bbdf9fba196 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -387,14 +387,9 @@ struct intel_engine_cs {
struct irq_work irq_work; /* for use from inside irq_lock */
- struct timer_list fake_irq; /* used after a missed interrupt */
- struct timer_list hangcheck; /* detect missed interrupts */
-
- unsigned int hangcheck_interrupts;
unsigned int irq_enabled;
bool irq_armed;
- bool irq_fired;
} breadcrumbs;
struct {
diff --git a/drivers/gpu/drm/i915/selftests/igt_live_test.c b/drivers/gpu/drm/i915/selftests/igt_live_test.c
index 5deb485fb942..3e902761cd16 100644
--- a/drivers/gpu/drm/i915/selftests/igt_live_test.c
+++ b/drivers/gpu/drm/i915/selftests/igt_live_test.c
@@ -35,7 +35,6 @@ int igt_live_test_begin(struct igt_live_test *t,
return err;
}
- i915->gpu_error.missed_irq_rings = 0;
t->reset_global = i915_reset_count(&i915->gpu_error);
for_each_engine(engine, i915, id)
@@ -75,11 +74,5 @@ int igt_live_test_end(struct igt_live_test *t)
return -EIO;
}
- if (i915->gpu_error.missed_irq_rings) {
- pr_err("%s(%s): Missed interrupts on engines %lx\n",
- t->func, t->name, i915->gpu_error.missed_irq_rings);
- return -EIO;
- }
-
return 0;
}
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 23+ messages in thread
* ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/8] drm/i915: Rename execlists->queue_priority to preempt_priority_hint
2019-01-29 8:58 [PATCH 1/8] drm/i915: Rename execlists->queue_priority to preempt_priority_hint Chris Wilson
` (6 preceding siblings ...)
2019-01-29 8:58 ` [PATCH 8/8] drm/i915: Drop fake breadcrumb irq Chris Wilson
@ 2019-01-29 9:40 ` Patchwork
2019-01-29 9:44 ` ✗ Fi.CI.SPARSE: " Patchwork
` (4 subsequent siblings)
12 siblings, 0 replies; 23+ messages in thread
From: Patchwork @ 2019-01-29 9:40 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/8] drm/i915: Rename execlists->queue_priority to preempt_priority_hint
URL : https://patchwork.freedesktop.org/series/55886/
State : warning
== Summary ==
$ dim checkpatch origin/drm-tip
a7e07cfaf594 drm/i915: Rename execlists->queue_priority to preempt_priority_hint
00ae93e298ab drm/i915/execlists: Suppress preempting self
-:14: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#14:
value as it may match a second preemption request within the same time period
-:34: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit a2bf92e8cc16 ("drm/i915/execlists: Avoid kicking priority on the current context")'
#34:
References: a2bf92e8cc16 ("drm/i915/execlists: Avoid kicking priority on the current context")
total: 1 errors, 1 warnings, 0 checks, 338 lines checked
0843d5822f7f drm/i915/execlists: Suppress redundant preemption
6c938314510d drm/i915/selftests: Exercise some AB...BA preemption chains
76ddda75ac6b drm/i915: Identify active requests
c77b5722e555 drm/i915: Remove the intel_engine_notify tracepoint
515345460aad drm/i915: Replace global breadcrumbs with per-context interrupt tracking
-:18: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 688e6c725816 ("drm/i915: Slaughter the thundering i915_wait_request herd")'
#18:
Before commit 688e6c725816, the solution was simple. Every client
-:21: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 688e6c725816 ("drm/i915: Slaughter the thundering i915_wait_request herd")'
#21:
688e6c725816 introduced an rbtree so that only the earliest waiter on
-:56: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#56:
References: 688e6c725816 ("drm/i915: Slaughter the thundering i915_wait_request herd")
-:56: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 688e6c725816 ("drm/i915: Slaughter the thundering i915_wait_request herd")'
#56:
References: 688e6c725816 ("drm/i915: Slaughter the thundering i915_wait_request herd")
-:2195: WARNING:FUNCTION_ARGUMENTS: function definition argument 'struct i915_gem_context *' should also have an identifier name
#2195: FILE: drivers/gpu/drm/i915/selftests/i915_request.c:258:
+ struct i915_request *(*request_alloc)(struct i915_gem_context *,
-:2195: WARNING:FUNCTION_ARGUMENTS: function definition argument 'struct intel_engine_cs *' should also have an identifier name
#2195: FILE: drivers/gpu/drm/i915/selftests/i915_request.c:258:
+ struct i915_request *(*request_alloc)(struct i915_gem_context *,
-:2221: WARNING:LINE_SPACING: Missing a blank line after declarations
#2221: FILE: drivers/gpu/drm/i915/selftests/i915_request.c:284:
+ struct i915_request **requests;
+ I915_RND_STATE(prng);
-:2653: WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), does MAINTAINERS need updating?
#2653:
deleted file mode 100644
total: 3 errors, 5 warnings, 0 checks, 2586 lines checked
d46f6a5d0ea3 drm/i915: Drop fake breadcrumb irq
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* ✗ Fi.CI.SPARSE: warning for series starting with [1/8] drm/i915: Rename execlists->queue_priority to preempt_priority_hint
2019-01-29 8:58 [PATCH 1/8] drm/i915: Rename execlists->queue_priority to preempt_priority_hint Chris Wilson
` (7 preceding siblings ...)
2019-01-29 9:40 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/8] drm/i915: Rename execlists->queue_priority to preempt_priority_hint Patchwork
@ 2019-01-29 9:44 ` Patchwork
2019-01-29 9:58 ` ✓ Fi.CI.BAT: success " Patchwork
` (3 subsequent siblings)
12 siblings, 0 replies; 23+ messages in thread
From: Patchwork @ 2019-01-29 9:44 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/8] drm/i915: Rename execlists->queue_priority to preempt_priority_hint
URL : https://patchwork.freedesktop.org/series/55886/
State : warning
== Summary ==
$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915: Rename execlists->queue_priority to preempt_priority_hint
Okay!
Commit: drm/i915/execlists: Suppress preempting self
-drivers/gpu/drm/i915/intel_ringbuffer.h:600:23: warning: expression using sizeof(void)
Commit: drm/i915/execlists: Suppress redundant preemption
Okay!
Commit: drm/i915/selftests: Exercise some AB...BA preemption chains
Okay!
Commit: drm/i915: Identify active requests
Okay!
Commit: drm/i915: Remove the intel_engine_notify tracepoint
Okay!
Commit: drm/i915: Replace global breadcrumbs with per-context interrupt tracking
+drivers/gpu/drm/i915/selftests/i915_request.c:280:40: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/i915_request.c:280:40: warning: expression using sizeof(void)
-./include/linux/mm.h:619:13: error: not a function <noident>
-./include/linux/mm.h:619:13: error: not a function <noident>
-./include/linux/mm.h:619:13: error: undefined identifier '__builtin_mul_overflow'
-./include/linux/mm.h:619:13: warning: call with no type!
+./include/linux/slab.h:664:13: error: not a function <noident>
+./include/linux/slab.h:664:13: error: not a function <noident>
Commit: drm/i915: Drop fake breadcrumb irq
Okay!
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* ✓ Fi.CI.BAT: success for series starting with [1/8] drm/i915: Rename execlists->queue_priority to preempt_priority_hint
2019-01-29 8:58 [PATCH 1/8] drm/i915: Rename execlists->queue_priority to preempt_priority_hint Chris Wilson
` (8 preceding siblings ...)
2019-01-29 9:44 ` ✗ Fi.CI.SPARSE: " Patchwork
@ 2019-01-29 9:58 ` Patchwork
2019-01-29 10:14 ` [PATCH 1/8] " Tvrtko Ursulin
` (2 subsequent siblings)
12 siblings, 0 replies; 23+ messages in thread
From: Patchwork @ 2019-01-29 9:58 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/8] drm/i915: Rename execlists->queue_priority to preempt_priority_hint
URL : https://patchwork.freedesktop.org/series/55886/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_5499 -> Patchwork_12065
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/55886/revisions/1/mbox/
Known issues
------------
Here are the changes found in Patchwork_12065 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@amdgpu/amd_basic@cs-compute:
- fi-kbl-8809g: NOTRUN -> FAIL [fdo#108094]
* igt@amdgpu/amd_prime@amd-to-i915:
- fi-kbl-8809g: NOTRUN -> FAIL [fdo#107341]
* igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
- fi-byt-clapper: PASS -> FAIL [fdo#103191] / [fdo#107362] +1
#### Possible fixes ####
* igt@amdgpu/amd_basic@userptr:
- fi-kbl-8809g: DMESG-WARN [fdo#108965] -> PASS
* igt@i915_selftest@live_execlists:
- fi-apl-guc: INCOMPLETE [fdo#103927] -> PASS
* igt@prime_vgem@basic-fence-flip:
- fi-kbl-7500u: {SKIP} [fdo#109271] -> PASS +33
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
[fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
[fdo#107341]: https://bugs.freedesktop.org/show_bug.cgi?id=107341
[fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
[fdo#108094]: https://bugs.freedesktop.org/show_bug.cgi?id=108094
[fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
[fdo#108965]: https://bugs.freedesktop.org/show_bug.cgi?id=108965
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
Participating hosts (45 -> 40)
------------------------------
Missing (5): fi-kbl-soraka fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-icl-y
Build changes
-------------
* Linux: CI_DRM_5499 -> Patchwork_12065
CI_DRM_5499: 3c874f04ad225c2cdf9552c5e70182006696f258 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4796: d1bd9c6ad6f3482bbccf4aa6417dd449e9efbe39 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_12065: d46f6a5d0ea38fc4cd547e83590ff9338368dceb @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
d46f6a5d0ea3 drm/i915: Drop fake breadcrumb irq
515345460aad drm/i915: Replace global breadcrumbs with per-context interrupt tracking
c77b5722e555 drm/i915: Remove the intel_engine_notify tracepoint
76ddda75ac6b drm/i915: Identify active requests
6c938314510d drm/i915/selftests: Exercise some AB...BA preemption chains
0843d5822f7f drm/i915/execlists: Suppress redundant preemption
00ae93e298ab drm/i915/execlists: Suppress preempting self
a7e07cfaf594 drm/i915: Rename execlists->queue_priority to preempt_priority_hint
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12065/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/8] drm/i915/execlists: Suppress preempting self
2019-01-29 8:58 ` [PATCH 2/8] drm/i915/execlists: Suppress preempting self Chris Wilson
@ 2019-01-29 10:13 ` Tvrtko Ursulin
2019-01-29 10:22 ` Chris Wilson
0 siblings, 1 reply; 23+ messages in thread
From: Tvrtko Ursulin @ 2019-01-29 10:13 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
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@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/8] drm/i915: Rename execlists->queue_priority to preempt_priority_hint
2019-01-29 8:58 [PATCH 1/8] drm/i915: Rename execlists->queue_priority to preempt_priority_hint Chris Wilson
` (9 preceding siblings ...)
2019-01-29 9:58 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2019-01-29 10:14 ` Tvrtko Ursulin
2019-01-29 10:24 ` Chris Wilson
2019-01-29 11:33 ` ✓ Fi.CI.IGT: success for series starting with [1/8] " Patchwork
2019-01-29 16:29 ` ✗ Fi.CI.BAT: failure for series starting with [1/8] drm/i915: Rename execlists->queue_priority to preempt_priority_hint (rev2) Patchwork
12 siblings, 1 reply; 23+ messages in thread
From: Tvrtko Ursulin @ 2019-01-29 10:14 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 29/01/2019 08:58, Chris Wilson wrote:
> After noticing that we trigger preemption events for currently executing
> requests, as well as requests that complete before the preemption and
> attempting to suppress those preemption events, it is wise to not
> consider the queue_priority to be authoritative. As we only track the
> maximum priority seen between dequeue passes, if the maximum priority
> request is no longer available for dequeuing (it completed or is even
> executing on another engine), we have no knowledge of the previous
> queue_priority as it would require us to keep a full history of enqueued
> requests -- but we already have that history in the priolists!
>
> Rename the queue_priority to preempt_priority_hint so that we do not
> confuse it as being the maximum priority in the queue, but merely an
> indication that we have seen a new maximum priority value and as such we
> should check whether it should preempt the currently running request.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
> drivers/gpu/drm/i915/i915_scheduler.c | 11 +++++------
> drivers/gpu/drm/i915/intel_engine_cs.c | 4 ++--
> drivers/gpu/drm/i915/intel_guc_submission.c | 5 +++--
> drivers/gpu/drm/i915/intel_lrc.c | 20 +++++++++++---------
> drivers/gpu/drm/i915/intel_ringbuffer.h | 8 ++++++--
> 5 files changed, 27 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
> index 340faea6c08a..0da718ceab43 100644
> --- a/drivers/gpu/drm/i915/i915_scheduler.c
> +++ b/drivers/gpu/drm/i915/i915_scheduler.c
> @@ -127,8 +127,7 @@ static inline struct i915_priolist *to_priolist(struct rb_node *rb)
> return rb_entry(rb, struct i915_priolist, node);
> }
>
> -static void assert_priolists(struct intel_engine_execlists * const execlists,
> - long queue_priority)
> +static void assert_priolists(struct intel_engine_execlists * const execlists)
> {
> struct rb_node *rb;
> long last_prio, i;
> @@ -139,7 +138,7 @@ static void assert_priolists(struct intel_engine_execlists * const execlists,
> GEM_BUG_ON(rb_first_cached(&execlists->queue) !=
> rb_first(&execlists->queue.rb_root));
>
> - last_prio = (queue_priority >> I915_USER_PRIORITY_SHIFT) + 1;
> + last_prio = (INT_MAX >> I915_USER_PRIORITY_SHIFT) + 1;
> for (rb = rb_first_cached(&execlists->queue); rb; rb = rb_next(rb)) {
> const struct i915_priolist *p = to_priolist(rb);
>
> @@ -166,7 +165,7 @@ i915_sched_lookup_priolist(struct intel_engine_cs *engine, int prio)
> int idx, i;
>
> lockdep_assert_held(&engine->timeline.lock);
> - assert_priolists(execlists, INT_MAX);
> + assert_priolists(execlists);
>
> /* buckets sorted from highest [in slot 0] to lowest priority */
> idx = I915_PRIORITY_COUNT - (prio & I915_PRIORITY_MASK) - 1;
> @@ -353,7 +352,7 @@ static void __i915_schedule(struct i915_request *rq,
> continue;
> }
>
> - if (prio <= engine->execlists.queue_priority)
> + if (prio <= engine->execlists.preempt_priority_hint)
> continue;
>
> /*
> @@ -366,7 +365,7 @@ static void __i915_schedule(struct i915_request *rq,
> continue;
>
> /* Defer (tasklet) submission until after all of our updates. */
> - engine->execlists.queue_priority = prio;
> + engine->execlists.preempt_priority_hint = prio;
> tasklet_hi_schedule(&engine->execlists.tasklet);
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index ead9c4371fe1..5a80db990351 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -480,7 +480,7 @@ static void intel_engine_init_execlist(struct intel_engine_cs *engine)
> GEM_BUG_ON(!is_power_of_2(execlists_num_ports(execlists)));
> GEM_BUG_ON(execlists_num_ports(execlists) > EXECLIST_MAX_PORTS);
>
> - execlists->queue_priority = INT_MIN;
> + execlists->preempt_priority_hint = INT_MIN;
> execlists->queue = RB_ROOT_CACHED;
> }
>
> @@ -1178,7 +1178,7 @@ void intel_engines_park(struct drm_i915_private *i915)
> }
>
> /* Must be reset upon idling, or we may miss the busy wakeup. */
> - GEM_BUG_ON(engine->execlists.queue_priority != INT_MIN);
> + GEM_BUG_ON(engine->execlists.preempt_priority_hint != INT_MIN);
>
> if (engine->park)
> engine->park(engine);
> diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
> index 4295ade0d613..6cdd3f097209 100644
> --- a/drivers/gpu/drm/i915/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/intel_guc_submission.c
> @@ -737,7 +737,7 @@ static bool __guc_dequeue(struct intel_engine_cs *engine)
> if (intel_engine_has_preemption(engine)) {
> struct guc_preempt_work *preempt_work =
> &engine->i915->guc.preempt_work[engine->id];
> - int prio = execlists->queue_priority;
> + int prio = execlists->preempt_priority_hint;
>
> if (__execlists_need_preempt(prio, port_prio(port))) {
> execlists_set_active(execlists,
> @@ -783,7 +783,8 @@ static bool __guc_dequeue(struct intel_engine_cs *engine)
> kmem_cache_free(engine->i915->priorities, p);
> }
> done:
> - execlists->queue_priority = rb ? to_priolist(rb)->priority : INT_MIN;
> + execlists->preempt_priority_hint =
> + rb ? to_priolist(rb)->priority : INT_MIN;
> if (submit)
> port_assign(port, last);
> if (last)
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index fdbb3fe8eac9..9219efa4ee79 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -591,7 +591,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->queue_priority)) {
> + if (need_preempt(engine, last, execlists->preempt_priority_hint)) {
> inject_preempt_context(engine);
> return;
> }
> @@ -699,20 +699,20 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
> /*
> * Here be a bit of magic! Or sleight-of-hand, whichever you prefer.
> *
> - * We choose queue_priority such that if we add a request of greater
> + * We choose the priority hint such that if we add a request of greater
> * priority than this, we kick the submission tasklet to decide on
> * the right order of submitting the requests to hardware. We must
> * also be prepared to reorder requests as they are in-flight on the
> - * HW. We derive the queue_priority then as the first "hole" in
> + * HW. We derive the priority hint then as the first "hole" in
> * the HW submission ports and if there are no available slots,
> * the priority of the lowest executing request, i.e. last.
> *
> * When we do receive a higher priority request ready to run from the
> - * user, see queue_request(), the queue_priority is bumped to that
> + * user, see queue_request(), the priority hint is bumped to that
> * request triggering preemption on the next dequeue (or subsequent
> * interrupt for secondary ports).
> */
> - execlists->queue_priority =
> + execlists->preempt_priority_hint =
> port != execlists->port ? rq_prio(last) : INT_MIN;
>
> if (submit) {
> @@ -861,7 +861,7 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
>
> /* Remaining _unready_ requests will be nop'ed when submitted */
>
> - execlists->queue_priority = INT_MIN;
> + execlists->preempt_priority_hint = INT_MIN;
> execlists->queue = RB_ROOT_CACHED;
> GEM_BUG_ON(port_isset(execlists->port));
>
> @@ -1092,8 +1092,8 @@ static void __submit_queue_imm(struct intel_engine_cs *engine)
>
> static void submit_queue(struct intel_engine_cs *engine, int prio)
> {
> - if (prio > engine->execlists.queue_priority) {
> - engine->execlists.queue_priority = prio;
> + if (prio > engine->execlists.preempt_priority_hint) {
> + engine->execlists.preempt_priority_hint = prio;
> __submit_queue_imm(engine);
> }
> }
> @@ -2748,7 +2748,9 @@ void intel_execlists_show_requests(struct intel_engine_cs *engine,
>
> last = NULL;
> count = 0;
> - drm_printf(m, "\t\tQueue priority: %d\n", execlists->queue_priority);
> + if (execlists->preempt_priority_hint != INT_MIN)
> + drm_printf(m, "\t\tPreempt priority hint: %d\n",
> + execlists->preempt_priority_hint);
> for (rb = rb_first_cached(&execlists->queue); rb; rb = rb_next(rb)) {
> struct i915_priolist *p = rb_entry(rb, typeof(*p), node);
> int i;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 2927b712b973..5e2231d0c2fa 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -293,14 +293,18 @@ struct intel_engine_execlists {
> unsigned int port_mask;
>
> /**
> - * @queue_priority: Highest pending priority.
> + * @preempt_priority_hint: Highest pending priority.
Would queue_priority_hint be better since it is used not just for
preemption but to start an idle GPU?
Regards,
Tvrtko
> *
> * When we add requests into the queue, or adjust the priority of
> * executing requests, we compute the maximum priority of those
> * pending requests. We can then use this value to determine if
> * we need to preempt the executing requests to service the queue.
> + * However, since the we may have recorded the priority of an inflight
> + * request we wanted to preempt but since completed, at the time of
> + * dequeuing the priority hint may no longer may match the highest
> + * available request priority.
> */
> - int queue_priority;
> + int preempt_priority_hint;
>
> /**
> * @queue: queue of requests, in priority lists
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/8] drm/i915/execlists: Suppress preempting self
2019-01-29 10:13 ` Tvrtko Ursulin
@ 2019-01-29 10:22 ` Chris Wilson
0 siblings, 0 replies; 23+ messages in thread
From: Chris Wilson @ 2019-01-29 10:22 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx
Quoting Tvrtko Ursulin (2019-01-29 10:13:47)
>
> On 29/01/2019 08:58, Chris Wilson wrote:
> > 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. :/
Yeah, this definitely feels like an implementation detail (and that we
instrument the code to easily detect) that we will not be privy to in
future.
> Even though you bothered to add the counter into the guc path.
;)
> Good it's blox and not something worse. :)
I'm keeping that typo now!
> > + 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.
Possibly, but rq_a, rq_b I've only used in this test so far, and if we
were to store them in the client struct, we should be more careful with
refs :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/8] drm/i915: Rename execlists->queue_priority to preempt_priority_hint
2019-01-29 10:14 ` [PATCH 1/8] " Tvrtko Ursulin
@ 2019-01-29 10:24 ` Chris Wilson
2019-01-29 10:41 ` Tvrtko Ursulin
0 siblings, 1 reply; 23+ messages in thread
From: Chris Wilson @ 2019-01-29 10:24 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx
Quoting Tvrtko Ursulin (2019-01-29 10:14:45)
>
> On 29/01/2019 08:58, Chris Wilson wrote:
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> > index 2927b712b973..5e2231d0c2fa 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> > @@ -293,14 +293,18 @@ struct intel_engine_execlists {
> > unsigned int port_mask;
> >
> > /**
> > - * @queue_priority: Highest pending priority.
> > + * @preempt_priority_hint: Highest pending priority.
>
> Would queue_priority_hint be better since it is used not just for
> preemption but to start an idle GPU?
~o~
It's 2 letters shorter so that's a plus.
And it doesn't always relate to preemption per se. If you are happy with
adding _hint, then queue_priority_hint it shall be.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/8] drm/i915: Rename execlists->queue_priority to preempt_priority_hint
2019-01-29 10:24 ` Chris Wilson
@ 2019-01-29 10:41 ` Tvrtko Ursulin
0 siblings, 0 replies; 23+ messages in thread
From: Tvrtko Ursulin @ 2019-01-29 10:41 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 29/01/2019 10:24, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-01-29 10:14:45)
>>
>> On 29/01/2019 08:58, Chris Wilson wrote:
>>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
>>> index 2927b712b973..5e2231d0c2fa 100644
>>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
>>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
>>> @@ -293,14 +293,18 @@ struct intel_engine_execlists {
>>> unsigned int port_mask;
>>>
>>> /**
>>> - * @queue_priority: Highest pending priority.
>>> + * @preempt_priority_hint: Highest pending priority.
>>
>> Would queue_priority_hint be better since it is used not just for
>> preemption but to start an idle GPU?
>
> ~o~
>
> It's 2 letters shorter so that's a plus.
>
> And it doesn't always relate to preemption per se. If you are happy with
> adding _hint, then queue_priority_hint it shall be.
Yes I am, I know it is a rebasing hassle, but hint seems a worthy suffix
and preemption would be less accurate.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/8] drm/i915/execlists: Suppress redundant preemption
2019-01-29 8:58 ` [PATCH 3/8] drm/i915/execlists: Suppress redundant preemption Chris Wilson
@ 2019-01-29 11:15 ` Tvrtko Ursulin
2019-01-29 11:21 ` Chris Wilson
2019-01-29 15:27 ` [PATCH] " Chris Wilson
1 sibling, 1 reply; 23+ messages in thread
From: Tvrtko Ursulin @ 2019-01-29 11:15 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 29/01/2019 08:58, 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
How can it go higher? Only because I915_PRIORITY_NEWCLIENT is higher
than I915_PRIORITY_WAIT? In that case would re-ordering those help? I
don't remember if there was a reason for the current order.
> change. We can avoid this if we take the boost into account when
> checking if the preemption request is valid.
>
> v2: After preemption the active request will be after the preemptee if
> they end up with equal priority.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/intel_lrc.c | 39 ++++++++++++++++++++++++++++----
> 1 file changed, 35 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index b8cddef5aed8..f4b0be81c6db 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);
> @@ -188,6 +190,34 @@ static inline int rq_prio(const struct i915_request *rq)
> return rq->sched.attr.priority;
> }
>
> +static inline int active_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) != ACTIVE_PRIORITY &&
if (!(prio & ACTIVE_PRIORITY) && ? It is a single bit.
> + i915_request_started(rq)) {
> + /*
> + * After preemption, we insert the active request at the
> + * end of the new priority level. This means that we will be
Side question - why do we not move it to the head of the new priority
level? It was preempted, so it sounds fair that it would go to the head.
> + * _lower_ priority than the preemptee all things equal (and
> + * so the preemption is valid), so adjust our comparison
> + * accordingly.
> + */
> + prio |= ACTIVE_PRIORITY;
> + prio--;
So any running request not marked with NEWCLIENT/ACTIVE_PRIORITY will
get it here, minus one, which effectively means applying
I915_PRIORITY_WAIT to it. Or just keeping NEWCLIENT if it had WAIT already.
So kind of like a micro priority bump, only if the request is running.
Do you view it as end of the context stream so make preemption less
likely on the very end of the stream? But what if it is a long batch,
why we wouldn't want to preempt it?
Regards,
Tvrtko
> + }
> +
> + return prio;
> +}
> +
> static int queue_prio(const struct intel_engine_execlists *execlists)
> {
> struct i915_priolist *p;
> @@ -208,7 +238,7 @@ static int queue_prio(const struct intel_engine_execlists *execlists)
> static inline bool need_preempt(const struct intel_engine_cs *engine,
> const struct i915_request *rq)
> {
> - const int last_prio = rq_prio(rq);
> + int last_prio;
>
> if (!intel_engine_has_preemption(engine))
> return false;
> @@ -228,6 +258,7 @@ static inline bool need_preempt(const struct intel_engine_cs *engine,
> * preempt. If that hint is stale or we may be trying to preempt
> * ourselves, ignore the request.
> */
> + last_prio = active_prio(rq);
> if (!__execlists_need_preempt(engine->execlists.preempt_priority_hint,
> last_prio))
> return false;
> @@ -353,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);
>
> @@ -385,8 +416,8 @@ __unwind_incomplete_requests(struct intel_engine_cs *engine)
> * stream, so give it the equivalent small priority bump to prevent
> * it being gazumped a second time by another peer.
> */
> - if (!(prio & I915_PRIORITY_NEWCLIENT)) {
> - prio |= I915_PRIORITY_NEWCLIENT;
> + if ((prio & ACTIVE_PRIORITY) != ACTIVE_PRIORITY) {
> + prio |= ACTIVE_PRIORITY;
> active->sched.attr.priority = prio;
> list_move_tail(&active->sched.link,
> i915_sched_lookup_priolist(engine, prio));
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/8] drm/i915/execlists: Suppress redundant preemption
2019-01-29 11:15 ` Tvrtko Ursulin
@ 2019-01-29 11:21 ` Chris Wilson
2019-01-29 12:26 ` Tvrtko Ursulin
0 siblings, 1 reply; 23+ messages in thread
From: Chris Wilson @ 2019-01-29 11:21 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx
Quoting Tvrtko Ursulin (2019-01-29 11:15:35)
>
> On 29/01/2019 08:58, 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
>
> How can it go higher? Only because I915_PRIORITY_NEWCLIENT is higher
> than I915_PRIORITY_WAIT? In that case would re-ordering those help? I
> don't remember if there was a reason for the current order.
Yes. There's more reasons later (media-bench is quite insistent about
this ;).
You were quite adamant in insisting that WAIT be the most minor of
boosts :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* ✓ Fi.CI.IGT: success for series starting with [1/8] drm/i915: Rename execlists->queue_priority to preempt_priority_hint
2019-01-29 8:58 [PATCH 1/8] drm/i915: Rename execlists->queue_priority to preempt_priority_hint Chris Wilson
` (10 preceding siblings ...)
2019-01-29 10:14 ` [PATCH 1/8] " Tvrtko Ursulin
@ 2019-01-29 11:33 ` Patchwork
2019-01-29 16:29 ` ✗ Fi.CI.BAT: failure for series starting with [1/8] drm/i915: Rename execlists->queue_priority to preempt_priority_hint (rev2) Patchwork
12 siblings, 0 replies; 23+ messages in thread
From: Patchwork @ 2019-01-29 11:33 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/8] drm/i915: Rename execlists->queue_priority to preempt_priority_hint
URL : https://patchwork.freedesktop.org/series/55886/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_5499_full -> Patchwork_12065_full
====================================================
Summary
-------
**SUCCESS**
No regressions found.
Known issues
------------
Here are the changes found in Patchwork_12065_full that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@kms_busy@extended-modeset-hang-newfb-render-b:
- shard-kbl: PASS -> DMESG-WARN [fdo#107956]
* igt@kms_cursor_crc@cursor-128x42-random:
- shard-glk: PASS -> FAIL [fdo#103232]
* igt@kms_cursor_legacy@cursor-vs-flip-varying-size:
- shard-apl: PASS -> INCOMPLETE [fdo#103927]
* igt@kms_flip@flip-vs-expired-vblank-interruptible:
- shard-glk: PASS -> FAIL [fdo#102887] / [fdo#105363]
* igt@kms_plane_alpha_blend@pipe-c-alpha-opaque-fb:
- shard-glk: PASS -> FAIL [fdo#108145]
* igt@kms_plane_multiple@atomic-pipe-a-tiling-x:
- shard-apl: PASS -> FAIL [fdo#103166] +1
* igt@kms_setmode@basic:
- shard-kbl: PASS -> FAIL [fdo#99912]
* igt@kms_vblank@pipe-a-ts-continuation-suspend:
- shard-hsw: PASS -> FAIL [fdo#104894]
#### Possible fixes ####
* igt@gem_eio@reset-stress:
- shard-hsw: INCOMPLETE [fdo#103540] / [fdo#109482] -> PASS
* igt@kms_busy@extended-modeset-hang-newfb-render-c:
- shard-kbl: DMESG-WARN [fdo#107956] -> PASS
* igt@kms_ccs@pipe-a-crc-sprite-planes-basic:
- shard-glk: FAIL [fdo#108145] -> PASS
* igt@kms_cursor_crc@cursor-128x128-suspend:
- shard-apl: FAIL [fdo#103191] / [fdo#103232] -> PASS
* igt@kms_cursor_crc@cursor-256x256-sliding:
- shard-glk: FAIL [fdo#103232] -> PASS +1
* igt@kms_cursor_crc@cursor-64x64-onscreen:
- shard-apl: FAIL [fdo#103232] -> PASS +2
* igt@kms_draw_crc@fill-fb:
- shard-glk: FAIL [fdo#103184] -> PASS
* igt@kms_flip@dpms-vs-vblank-race:
- shard-kbl: FAIL [fdo#103060] -> PASS
* igt@kms_plane_multiple@atomic-pipe-b-tiling-yf:
- shard-apl: FAIL [fdo#103166] -> PASS +1
#### Warnings ####
* igt@kms_cursor_legacy@cursorb-vs-flipa-varying-size:
- shard-snb: {SKIP} [fdo#109271] -> INCOMPLETE [fdo#105411] / [fdo#107469]
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#102887]: https://bugs.freedesktop.org/show_bug.cgi?id=102887
[fdo#103060]: https://bugs.freedesktop.org/show_bug.cgi?id=103060
[fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
[fdo#103184]: https://bugs.freedesktop.org/show_bug.cgi?id=103184
[fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
[fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
[fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
[fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
[fdo#104894]: https://bugs.freedesktop.org/show_bug.cgi?id=104894
[fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
[fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411
[fdo#107469]: https://bugs.freedesktop.org/show_bug.cgi?id=107469
[fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956
[fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
[fdo#109482]: https://bugs.freedesktop.org/show_bug.cgi?id=109482
[fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912
Participating hosts (6 -> 5)
------------------------------
Missing (1): shard-skl
Build changes
-------------
* Linux: CI_DRM_5499 -> Patchwork_12065
CI_DRM_5499: 3c874f04ad225c2cdf9552c5e70182006696f258 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4796: d1bd9c6ad6f3482bbccf4aa6417dd449e9efbe39 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_12065: d46f6a5d0ea38fc4cd547e83590ff9338368dceb @ git://anongit.freedesktop.org/gfx-ci/linux
piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12065/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/8] drm/i915/execlists: Suppress redundant preemption
2019-01-29 11:21 ` Chris Wilson
@ 2019-01-29 12:26 ` Tvrtko Ursulin
2019-01-29 12:34 ` Chris Wilson
0 siblings, 1 reply; 23+ messages in thread
From: Tvrtko Ursulin @ 2019-01-29 12:26 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 29/01/2019 11:21, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-01-29 11:15:35)
>>
>> On 29/01/2019 08:58, 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
>>
>> How can it go higher? Only because I915_PRIORITY_NEWCLIENT is higher
>> than I915_PRIORITY_WAIT? In that case would re-ordering those help? I
>> don't remember if there was a reason for the current order.
>
> Yes. There's more reasons later (media-bench is quite insistent about
> this ;).
>
> You were quite adamant in insisting that WAIT be the most minor of
> boosts :)
I was afraid that might have been the case. :) So we are firmly into
tweaking the scheduler for particulars clients territory.. and that
before we even have time-slicing!
In my mind it probably still makes sense that WAIT is lowest bump.
How about a we add a new smallest internal dedicated level for preempted
contexts? Because I don't like how this patch would intermingle with
either WAIT or NEWCLIENT.
#define I915_PRIORITY_PREEMPTED ((u8)BIT(0)) /* Must be lowest */
#define I915_PRIORITY_WAIT ((u8)BIT(1))
#define I915_PRIORITY_NEWCLIENT ((u8)BIT(2))
?
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/8] drm/i915/execlists: Suppress redundant preemption
2019-01-29 12:26 ` Tvrtko Ursulin
@ 2019-01-29 12:34 ` Chris Wilson
0 siblings, 0 replies; 23+ messages in thread
From: Chris Wilson @ 2019-01-29 12:34 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx
Quoting Tvrtko Ursulin (2019-01-29 12:26:46)
>
> On 29/01/2019 11:21, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-01-29 11:15:35)
> >>
> >> On 29/01/2019 08:58, 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
> >>
> >> How can it go higher? Only because I915_PRIORITY_NEWCLIENT is higher
> >> than I915_PRIORITY_WAIT? In that case would re-ordering those help? I
> >> don't remember if there was a reason for the current order.
> >
> > Yes. There's more reasons later (media-bench is quite insistent about
> > this ;).
> >
> > You were quite adamant in insisting that WAIT be the most minor of
> > boosts :)
>
> I was afraid that might have been the case. :) So we are firmly into
> tweaking the scheduler for particulars clients territory.. and that
> before we even have time-slicing!
What's coming up is a significantly different kettle of fish, I'm sure
you will agree when you start thinking about submitting dependent jobs
in parallel.
> In my mind it probably still makes sense that WAIT is lowest bump.
I agree. I like having WAIT, but I concur that it shouldn't be trivially
abusable.
> How about a we add a new smallest internal dedicated level for preempted
> contexts? Because I don't like how this patch would intermingle with
> either WAIT or NEWCLIENT.
>
> #define I915_PRIORITY_PREEMPTED ((u8)BIT(0)) /* Must be lowest */
> #define I915_PRIORITY_WAIT ((u8)BIT(1))
> #define I915_PRIORITY_NEWCLIENT ((u8)BIT(2))
>
> ?
No, the preempted client already goes to the front of their respective
bucket which is the same as the end of the one above.
The trick that comes later is what happens if we want to add a level
above NEWCLIENT that we wish to promote on preemption? Which is the
consideration for adding semaphores.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] drm/i915/execlists: Suppress redundant preemption
2019-01-29 8:58 ` [PATCH 3/8] drm/i915/execlists: Suppress redundant preemption Chris Wilson
2019-01-29 11:15 ` Tvrtko Ursulin
@ 2019-01-29 15:27 ` Chris Wilson
1 sibling, 0 replies; 23+ messages in thread
From: Chris Wilson @ 2019-01-29 15:27 UTC (permalink / raw)
To: intel-gfx
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.
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!
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/intel_lrc.c | 45 +++++++++++++++++++++++++++++---
1 file changed, 41 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index dec4e1b04ae1..62d98b7edc7a 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);
@@ -188,6 +190,34 @@ static inline int rq_prio(const struct i915_request *rq)
return rq->sched.attr.priority;
}
+static inline int active_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) != ACTIVE_PRIORITY &&
+ i915_request_started(rq)) {
+ /*
+ * 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--;
+ }
+
+ return prio;
+}
+
static int queue_prio(const struct intel_engine_execlists *execlists)
{
struct i915_priolist *p;
@@ -208,7 +238,7 @@ static int queue_prio(const struct intel_engine_execlists *execlists)
static inline bool need_preempt(const struct intel_engine_cs *engine,
const struct i915_request *rq)
{
- const int last_prio = rq_prio(rq);
+ int last_prio;
if (!intel_engine_has_preemption(engine))
return false;
@@ -228,6 +258,7 @@ static inline bool need_preempt(const struct intel_engine_cs *engine,
* preempt. If that hint is stale or we may be trying to preempt
* ourselves, ignore the request.
*/
+ last_prio = active_prio(rq);
if (!__execlists_need_preempt(engine->execlists.queue_priority_hint,
last_prio))
return false;
@@ -353,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);
@@ -384,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
+ * 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) != ACTIVE_PRIORITY) {
+ prio |= ACTIVE_PRIORITY;
active->sched.attr.priority = prio;
list_move_tail(&active->sched.link,
i915_sched_lookup_priolist(engine, prio));
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 23+ messages in thread
* ✗ Fi.CI.BAT: failure for series starting with [1/8] drm/i915: Rename execlists->queue_priority to preempt_priority_hint (rev2)
2019-01-29 8:58 [PATCH 1/8] drm/i915: Rename execlists->queue_priority to preempt_priority_hint Chris Wilson
` (11 preceding siblings ...)
2019-01-29 11:33 ` ✓ Fi.CI.IGT: success for series starting with [1/8] " Patchwork
@ 2019-01-29 16:29 ` Patchwork
12 siblings, 0 replies; 23+ messages in thread
From: Patchwork @ 2019-01-29 16:29 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/8] drm/i915: Rename execlists->queue_priority to preempt_priority_hint (rev2)
URL : https://patchwork.freedesktop.org/series/55886/
State : failure
== Summary ==
Applying: drm/i915: Rename execlists->queue_priority to preempt_priority_hint
Applying: drm/i915/execlists: Suppress preempting self
Applying: drm/i915/execlists: Suppress redundant preemption
Using index info to reconstruct a base tree...
M drivers/gpu/drm/i915/intel_lrc.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/intel_lrc.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/intel_lrc.c
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch' to see the failed patch
Patch failed at 0003 drm/i915/execlists: Suppress redundant preemption
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2019-01-29 16:29 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-29 8:58 [PATCH 1/8] drm/i915: Rename execlists->queue_priority to preempt_priority_hint Chris Wilson
2019-01-29 8:58 ` [PATCH 2/8] drm/i915/execlists: Suppress preempting self Chris Wilson
2019-01-29 10:13 ` Tvrtko Ursulin
2019-01-29 10:22 ` Chris Wilson
2019-01-29 8:58 ` [PATCH 3/8] drm/i915/execlists: Suppress redundant preemption Chris Wilson
2019-01-29 11:15 ` Tvrtko Ursulin
2019-01-29 11:21 ` Chris Wilson
2019-01-29 12:26 ` Tvrtko Ursulin
2019-01-29 12:34 ` Chris Wilson
2019-01-29 15:27 ` [PATCH] " Chris Wilson
2019-01-29 8:58 ` [PATCH 4/8] drm/i915/selftests: Exercise some AB...BA preemption chains Chris Wilson
2019-01-29 8:58 ` [PATCH 5/8] drm/i915: Identify active requests Chris Wilson
2019-01-29 8:58 ` [PATCH 6/8] drm/i915: Remove the intel_engine_notify tracepoint Chris Wilson
2019-01-29 8:58 ` [PATCH 7/8] drm/i915: Replace global breadcrumbs with per-context interrupt tracking Chris Wilson
2019-01-29 8:58 ` [PATCH 8/8] drm/i915: Drop fake breadcrumb irq Chris Wilson
2019-01-29 9:40 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/8] drm/i915: Rename execlists->queue_priority to preempt_priority_hint Patchwork
2019-01-29 9:44 ` ✗ Fi.CI.SPARSE: " Patchwork
2019-01-29 9:58 ` ✓ Fi.CI.BAT: success " Patchwork
2019-01-29 10:14 ` [PATCH 1/8] " Tvrtko Ursulin
2019-01-29 10:24 ` Chris Wilson
2019-01-29 10:41 ` Tvrtko Ursulin
2019-01-29 11:33 ` ✓ Fi.CI.IGT: success for series starting with [1/8] " Patchwork
2019-01-29 16:29 ` ✗ Fi.CI.BAT: failure for series starting with [1/8] drm/i915: Rename execlists->queue_priority to preempt_priority_hint (rev2) Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.