[Intel-gfx] [PATCH v2] drm/i915: Avoid lock dropping between rescheduling
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Wed Mar 29 09:33:47 UTC 2017
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 at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: <stable at vger.kernel.org> # 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 at intel.com>
Regards,
Tvrtko
More information about the Intel-gfx
mailing list