From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com ([134.134.136.24]:54925 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754618AbdC2Jd4 (ORCPT ); Wed, 29 Mar 2017 05:33:56 -0400 Subject: Re: [Intel-gfx] [PATCH v2] drm/i915: Avoid lock dropping between rescheduling To: Chris Wilson , intel-gfx@lists.freedesktop.org References: <20170326084420.3231-1-chris@chris-wilson.co.uk> <20170327202143.7972-1-chris@chris-wilson.co.uk> Cc: "# v4 . 10+" From: Tvrtko Ursulin Message-ID: Date: Wed, 29 Mar 2017 10:33:47 +0100 MIME-Version: 1.0 In-Reply-To: <20170327202143.7972-1-chris@chris-wilson.co.uk> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: stable-owner@vger.kernel.org List-ID: 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 > Cc: Tvrtko Ursulin > Cc: # 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 Regards, Tvrtko