From: Chris Wilson <chris@chris-wilson.co.uk> To: intel-gfx@lists.freedesktop.org Cc: Chris Wilson <chris@chris-wilson.co.uk>, Tvrtko Ursulin <tvrtko.ursulin@intel.com>, "# v4 . 10+" <stable@vger.kernel.org> Subject: [PATCH] drm/i915: Keep all engine locks across scheduling Date: Sun, 26 Mar 2017 09:44:20 +0100 [thread overview] Message-ID: <20170326084420.3231-1-chris@chris-wilson.co.uk> (raw) 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 dependees. Fixes: 20311bd35060 ("drm/i915/scheduler: Execute requests in order of priorities") Testcase: igt/gem_exec_whisper Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: <stable@vger.kernel.org> # v4.10+ --- drivers/gpu/drm/i915/intel_lrc.c | 54 +++++++++++++++++++++++++++------------- 1 file changed, 37 insertions(+), 17 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index e9822b0b308d..1ca1060c14cc 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -623,30 +623,47 @@ static void execlists_submit_request(struct drm_i915_gem_request *request) spin_unlock_irqrestore(&engine->timeline->lock, flags); } -static struct intel_engine_cs * -pt_lock_engine(struct i915_priotree *pt, struct intel_engine_cs *locked) +static inline struct intel_engine_cs * +pt_lock_engine(struct i915_priotree *pt, unsigned long *locked) { - struct intel_engine_cs *engine; - - engine = container_of(pt, - struct drm_i915_gem_request, - priotree)->engine; - if (engine != locked) { - if (locked) - spin_unlock_irq(&locked->timeline->lock); - spin_lock_irq(&engine->timeline->lock); - } + struct intel_engine_cs *engine = + container_of(pt, struct drm_i915_gem_request, priotree)->engine; + + /* Locking the engines in a random order will rightfully trigger a + * spasm in lockdep. However, we can ignore lockdep (by marking each + * as a seperate nesting) so long as we never nest the + * engine->timeline->lock elsewhere. Also the number of nesting + * subclasses is severely limited (7) which is going to cause an + * issue at some point. + * BUILD_BUG_ON(I915_NUM_ENGINES >= MAX_LOCKDEP_SUBCLASSES); + */ + if (!__test_and_set_bit(engine->id, locked)) + spin_lock_nested(&engine->timeline->lock, + hweight_long(*locked)); return engine; } +static void +unlock_engines(struct drm_i915_private *i915, unsigned long locked) +{ + struct intel_engine_cs *engine; + unsigned long tmp; + + for_each_engine_masked(engine, i915, locked, tmp) + spin_unlock(&engine->timeline->lock); +} + 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; + unsigned long locked = 0; LIST_HEAD(dfs); + BUILD_BUG_ON(I915_NUM_ENGINES > BITS_PER_LONG); + if (!READ_ONCE(request->engine->execlist_first)) prio = max(prio, I915_PRIORITY_STALL); @@ -659,6 +676,9 @@ static void execlists_schedule(struct drm_i915_gem_request *request, int prio) stack.signaler = &request->priotree; list_add(&stack.dfs_link, &dfs); + GEM_BUG_ON(irqs_disabled()); + local_irq_disable(); + /* Recursively bump all dependent priorities to match the new request. * * A naive approach would be to use recursion: @@ -687,7 +707,7 @@ static void execlists_schedule(struct drm_i915_gem_request *request, int prio) if (!RB_EMPTY_NODE(&pt->node)) continue; - engine = pt_lock_engine(pt, engine); + engine = pt_lock_engine(pt, &locked); /* If it is not already in the rbtree, we can update the * priority inplace and skip over it (and its dependencies) @@ -705,7 +725,7 @@ static void execlists_schedule(struct drm_i915_gem_request *request, int prio) INIT_LIST_HEAD(&dep->dfs_link); - engine = pt_lock_engine(pt, engine); + engine = pt_lock_engine(pt, &locked); if (prio <= pt->priority) continue; @@ -718,8 +738,8 @@ static void execlists_schedule(struct drm_i915_gem_request *request, int prio) engine->execlist_first = &pt->node; } - if (engine) - spin_unlock_irq(&engine->timeline->lock); + unlock_engines(request->i915, locked); + local_irq_enable(); /* XXX Do we need to preempt to make room for us and our deps? */ } -- 2.11.0
WARNING: multiple messages have this Message-ID (diff)
From: Chris Wilson <chris@chris-wilson.co.uk> To: intel-gfx@lists.freedesktop.org Cc: "# v4 . 10+" <stable@vger.kernel.org> Subject: [PATCH] drm/i915: Keep all engine locks across scheduling Date: Sun, 26 Mar 2017 09:44:20 +0100 [thread overview] Message-ID: <20170326084420.3231-1-chris@chris-wilson.co.uk> (raw) 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 dependees. Fixes: 20311bd35060 ("drm/i915/scheduler: Execute requests in order of priorities") Testcase: igt/gem_exec_whisper Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: <stable@vger.kernel.org> # v4.10+ --- drivers/gpu/drm/i915/intel_lrc.c | 54 +++++++++++++++++++++++++++------------- 1 file changed, 37 insertions(+), 17 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index e9822b0b308d..1ca1060c14cc 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -623,30 +623,47 @@ static void execlists_submit_request(struct drm_i915_gem_request *request) spin_unlock_irqrestore(&engine->timeline->lock, flags); } -static struct intel_engine_cs * -pt_lock_engine(struct i915_priotree *pt, struct intel_engine_cs *locked) +static inline struct intel_engine_cs * +pt_lock_engine(struct i915_priotree *pt, unsigned long *locked) { - struct intel_engine_cs *engine; - - engine = container_of(pt, - struct drm_i915_gem_request, - priotree)->engine; - if (engine != locked) { - if (locked) - spin_unlock_irq(&locked->timeline->lock); - spin_lock_irq(&engine->timeline->lock); - } + struct intel_engine_cs *engine = + container_of(pt, struct drm_i915_gem_request, priotree)->engine; + + /* Locking the engines in a random order will rightfully trigger a + * spasm in lockdep. However, we can ignore lockdep (by marking each + * as a seperate nesting) so long as we never nest the + * engine->timeline->lock elsewhere. Also the number of nesting + * subclasses is severely limited (7) which is going to cause an + * issue at some point. + * BUILD_BUG_ON(I915_NUM_ENGINES >= MAX_LOCKDEP_SUBCLASSES); + */ + if (!__test_and_set_bit(engine->id, locked)) + spin_lock_nested(&engine->timeline->lock, + hweight_long(*locked)); return engine; } +static void +unlock_engines(struct drm_i915_private *i915, unsigned long locked) +{ + struct intel_engine_cs *engine; + unsigned long tmp; + + for_each_engine_masked(engine, i915, locked, tmp) + spin_unlock(&engine->timeline->lock); +} + 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; + unsigned long locked = 0; LIST_HEAD(dfs); + BUILD_BUG_ON(I915_NUM_ENGINES > BITS_PER_LONG); + if (!READ_ONCE(request->engine->execlist_first)) prio = max(prio, I915_PRIORITY_STALL); @@ -659,6 +676,9 @@ static void execlists_schedule(struct drm_i915_gem_request *request, int prio) stack.signaler = &request->priotree; list_add(&stack.dfs_link, &dfs); + GEM_BUG_ON(irqs_disabled()); + local_irq_disable(); + /* Recursively bump all dependent priorities to match the new request. * * A naive approach would be to use recursion: @@ -687,7 +707,7 @@ static void execlists_schedule(struct drm_i915_gem_request *request, int prio) if (!RB_EMPTY_NODE(&pt->node)) continue; - engine = pt_lock_engine(pt, engine); + engine = pt_lock_engine(pt, &locked); /* If it is not already in the rbtree, we can update the * priority inplace and skip over it (and its dependencies) @@ -705,7 +725,7 @@ static void execlists_schedule(struct drm_i915_gem_request *request, int prio) INIT_LIST_HEAD(&dep->dfs_link); - engine = pt_lock_engine(pt, engine); + engine = pt_lock_engine(pt, &locked); if (prio <= pt->priority) continue; @@ -718,8 +738,8 @@ static void execlists_schedule(struct drm_i915_gem_request *request, int prio) engine->execlist_first = &pt->node; } - if (engine) - spin_unlock_irq(&engine->timeline->lock); + unlock_engines(request->i915, locked); + local_irq_enable(); /* XXX Do we need to preempt to make room for us and our deps? */ } -- 2.11.0 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next reply other threads:[~2017-03-26 8:44 UTC|newest] Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-03-26 8:44 Chris Wilson [this message] 2017-03-26 8:44 ` [PATCH] drm/i915: Keep all engine locks across scheduling Chris Wilson 2017-03-26 8:46 ` Chris Wilson 2017-03-26 8:46 ` Chris Wilson 2017-03-27 10:11 ` [Intel-gfx] " Tvrtko Ursulin 2017-03-27 10:11 ` Tvrtko Ursulin 2017-03-27 10:31 ` [Intel-gfx] " Chris Wilson 2017-03-27 10:31 ` Chris Wilson 2017-03-27 11:39 ` [Intel-gfx] " Tvrtko Ursulin 2017-03-27 11:39 ` Tvrtko Ursulin 2017-03-27 21:06 ` [Intel-gfx] " Chris Wilson 2017-03-27 21:23 ` Chris Wilson 2017-03-26 9:03 ` ✓ Fi.CI.BAT: success for drm/i915: Keep all engine locks across scheduling (rev2) Patchwork 2017-03-27 20:21 ` [PATCH v2] drm/i915: Avoid lock dropping between rescheduling Chris Wilson 2017-03-27 20:21 ` Chris Wilson 2017-03-29 9:33 ` [Intel-gfx] " Tvrtko Ursulin 2017-03-29 9:33 ` Tvrtko Ursulin 2017-03-29 12:15 ` [Intel-gfx] " Chris Wilson 2017-03-29 12:15 ` Chris Wilson 2017-03-27 20:41 ` ✓ Fi.CI.BAT: success for drm/i915: Keep all engine locks across scheduling (rev3) Patchwork
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20170326084420.3231-1-chris@chris-wilson.co.uk \ --to=chris@chris-wilson.co.uk \ --cc=intel-gfx@lists.freedesktop.org \ --cc=stable@vger.kernel.org \ --cc=tvrtko.ursulin@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.