Re: [CI 08/10] drm/i915/scheduler: Execute requests in order of priorities

[Jani Nikula <jani.nikula@linux.intel.com>]

This patch, or

commit 20311bd35060435badba8a0d46b06d5d184abaf7
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Mon Nov 14 20:41:03 2016 +0000

    drm/i915/scheduler: Execute requests in order of priorities

tricks sparse into warnings. It makes me unhappy to see the sparse
warnings accumulate because that will eventually render sparse useless.

The warnings in-line on the things being warned about.

BR,
Jani.


On Mon, 14 Nov 2016, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> @@ -634,13 +667,112 @@ static void execlists_submit_request(struct drm_i915_gem_request *request)
>  	/* Will be called from irq-context when using foreign fences. */
>  	spin_lock_irqsave(&engine->timeline->lock, flags);
>  
> -	list_add_tail(&request->execlist_link, &engine->execlist_queue);
> +	if (insert_request(&request->priotree, &engine->execlist_queue))
> +		engine->execlist_first = &request->priotree.node;
>  	if (execlists_elsp_idle(engine))
>  		tasklet_hi_schedule(&engine->irq_tasklet);
>  
>  	spin_unlock_irqrestore(&engine->timeline->lock, flags);
>  }
>  
> +static struct intel_engine_cs *
> +pt_lock_engine(struct i915_priotree *pt, struct intel_engine_cs *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);

drivers/gpu/drm/i915/intel_lrc.c:688:40: warning: context imbalance in 'pt_lock_engine' - unexpected unlock

> +		spin_lock_irq(&engine->timeline->lock);
> +	}
> +
> +	return engine;
> +}
> +
> +static void execlists_schedule(struct drm_i915_gem_request *request, int prio)
> +{
> +	struct intel_engine_cs *engine = NULL;
> +	struct i915_dependency *dep, *p;
> +	struct i915_dependency stack;
> +	LIST_HEAD(dfs);
> +
> +	if (prio <= READ_ONCE(request->priotree.priority))
> +		return;
> +
> +	/* Need BKL in order to use the temporary link inside i915_dependency */
> +	lockdep_assert_held(&request->i915->drm.struct_mutex);
> +
> +	stack.signaler = &request->priotree;
> +	list_add(&stack.dfs_link, &dfs);
> +
> +	/* Recursively bump all dependent priorities to match the new request.
> +	 *
> +	 * A naive approach would be to use recursion:
> +	 * static void update_priorities(struct i915_priotree *pt, prio) {
> +	 * 	list_for_each_entry(dep, &pt->signalers_list, signal_link)
> +	 * 		update_priorities(dep->signal, prio)
> +	 * 	insert_request(pt);
> +	 * }
> +	 * but that may have unlimited recursion depth and so runs a very
> +	 * real risk of overunning the kernel stack. Instead, we build
> +	 * a flat list of all dependencies starting with the current request.
> +	 * As we walk the list of dependencies, we add all of its dependencies
> +	 * to the end of the list (this may include an already visited
> +	 * request) and continue to walk onwards onto the new dependencies. The
> +	 * end result is a topological list of requests in reverse order, the
> +	 * last element in the list is the request we must execute first.
> +	 */
> +	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)
> +			if (prio > READ_ONCE(p->signaler->priority))
> +				list_move_tail(&p->dfs_link, &dfs);
> +
> +		p = list_next_entry(dep, 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);
> +		}
> +	}
> +
> +	/* 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;
> +
> +		INIT_LIST_HEAD(&dep->dfs_link);
> +
> +		engine = pt_lock_engine(pt, engine);
> +
> +		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 (engine)
> +		spin_unlock_irq(&engine->timeline->lock);

drivers/gpu/drm/i915/intel_lrc.c:771:32: warning: context imbalance in 'execlists_schedule' - unexpected unlock



-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx