On 27/03/2017 21:21, Chris Wilson wrote:
Unlocking is dangerous. In this case we combine an early update to the
out-of-queue request, because we know that it will be inserted into the
correct FIFO priority-ordered slot when it becomes ready in the future.
However, given sufficient enthusiasm, it may become ready as we are
continuing to reschedule, and so may gazump the FIFO if we have since
dropped its spinlock. The result is that it may be executed too early,
before its dependencies.
v2: Move all work into the second phase over the topological sort. This
removes the shortcut on the out-of-rbtree request to ensure that we only
adjust its priority after adjusting all of its dependencies.
Fixes: 20311bd35060 ("drm/i915/scheduler: Execute requests in order of priorities")
Testcase: igt/gem_exec_whisper
Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Cc: <stable@xxxxxxxxxxxxxxx> # v4.10+
---
drivers/gpu/drm/i915/intel_lrc.c | 44 ++++++++++++++++++----------------------
1 file changed, 20 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index b0c3a029b592..91e38e80a095 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -665,8 +665,8 @@ pt_lock_engine(struct i915_priotree *pt, struct intel_engine_cs *locked)
priotree)->engine;
if (engine != locked) {
if (locked)
Could replace "if (locked)" with a GEM_BUG_ON(!locked) now.
- spin_unlock_irq(&locked->timeline->lock);
- spin_lock_irq(&engine->timeline->lock);
+ spin_unlock(&locked->timeline->lock);
+ spin_lock(&engine->timeline->lock);
}
return engine;
@@ -674,7 +674,7 @@ pt_lock_engine(struct i915_priotree *pt, struct intel_engine_cs *locked)
static void execlists_schedule(struct drm_i915_gem_request *request, int prio)
{
- struct intel_engine_cs *engine = NULL;
+ struct intel_engine_cs *engine;
struct i915_dependency *dep, *p;
struct i915_dependency stack;
LIST_HEAD(dfs);
@@ -708,26 +708,23 @@ static void execlists_schedule(struct drm_i915_gem_request *request, int prio)
list_for_each_entry_safe(dep, p, &dfs, dfs_link) {
struct i915_priotree *pt = dep->signaler;
- list_for_each_entry(p, &pt->signalers_list, signal_link)
+ /* Within an engine, there can be no cycle, but we may
+ * refer to the same dependency chain multiple times
+ * (redundant dependencies are not eliminated) and across
+ * engines.
+ */
+ list_for_each_entry(p, &pt->signalers_list, signal_link) {
+ GEM_BUG_ON(p->signaler->priority < pt->priority);
if (prio > READ_ONCE(p->signaler->priority))
list_move_tail(&p->dfs_link, &dfs);
+ }
list_safe_reset_next(dep, p, dfs_link);
- if (!RB_EMPTY_NODE(&pt->node))
- continue;
-
- engine = pt_lock_engine(pt, engine);
-
- /* If it is not already in the rbtree, we can update the
- * priority inplace and skip over it (and its dependencies)
- * if it is referenced *again* as we descend the dfs.
- */
- if (prio > pt->priority && RB_EMPTY_NODE(&pt->node)) {
- pt->priority = prio;
- list_del_init(&dep->dfs_link);
- }
}
+ engine = request->engine;
+ spin_lock_irq(&engine->timeline->lock);
+
/* Fifo and depth-first replacement ensure our deps execute before us */
list_for_each_entry_safe_reverse(dep, p, &dfs, dfs_link) {
struct i915_priotree *pt = dep->signaler;
@@ -739,16 +736,15 @@ static void execlists_schedule(struct drm_i915_gem_request *request, int prio)
if (prio <= pt->priority)
continue;
- GEM_BUG_ON(RB_EMPTY_NODE(&pt->node));
-
pt->priority = prio;
- rb_erase(&pt->node, &engine->execlist_queue);
- if (insert_request(pt, &engine->execlist_queue))
- engine->execlist_first = &pt->node;
+ if (!RB_EMPTY_NODE(&pt->node)) {
+ rb_erase(&pt->node, &engine->execlist_queue);
+ if (insert_request(pt, &engine->execlist_queue))
+ engine->execlist_first = &pt->node;
+ }
}
- if (engine)
- spin_unlock_irq(&engine->timeline->lock);
+ spin_unlock_irq(&engine->timeline->lock);
/* XXX Do we need to preempt to make room for us and our deps? */
}
Looks OK to me. Preferably with the tidy in pt_lock_engine:
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Regards,
Tvrtko