Re: [Intel-gfx] [PATCH v2] drm/i915: Avoid lock dropping between rescheduling

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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@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 | 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@intel.com>

Regards,

Tvrtko