[Intel-gfx] [RFC 3/4] drm/i915/execlists: Split insert_request()

[Michał Winiarski]

On Mon, Jul 17, 2017 at 09:42:34AM +0100, Chris Wilson wrote:
> In the next patch we will want to reinsert a request not at the end of
> the priority queue, but at the front. Here we split insert_request()
> into two, the first function retrieves the priority list (for reuse for
> unsubmit later) and a wrapper function to insert at the end of that list
> and to schedule the tasklet if we were first.
> 
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c | 35 +++++++++++++++++++----------------
>  1 file changed, 19 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 8ab0c4b76c98..d227480b3a26 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -292,10 +292,10 @@ uint64_t intel_lr_context_descriptor(struct i915_gem_context *ctx,
>  	return ctx->engine[engine->id].lrc_desc;
>  }
>  
> -static bool
> -insert_request(struct intel_engine_cs *engine,
> -	       struct i915_priotree *pt,
> -	       int prio)
> +static struct i915_priolist *
> +lookup_priolist(struct intel_engine_cs *engine,
> +		struct i915_priotree *pt,
> +		int prio)
>  {
>  	struct i915_priolist *p;
>  	struct rb_node **parent, *rb;
> @@ -317,8 +317,7 @@ insert_request(struct intel_engine_cs *engine,
>  			parent = &rb->rb_right;
>  			first = false;
>  		} else {
> -			list_add_tail(&pt->link, &p->requests);
> -			return false;
> +			return p;
>  		}
>  	}
>  
> @@ -344,16 +343,14 @@ insert_request(struct intel_engine_cs *engine,
>  	}
>  
>  	p->priority = prio;
> +	INIT_LIST_HEAD(&p->requests);
>  	rb_link_node(&p->node, rb, parent);
>  	rb_insert_color(&p->node, &engine->execlist_queue);
>  
> -	INIT_LIST_HEAD(&p->requests);
> -	list_add_tail(&pt->link, &p->requests);
> -
>  	if (first)
>  		engine->execlist_first = &p->node;
>  
> -	return first;
> +	return ptr_pack_bits(p, first, 1);

Matches what I have in my tree, except for "first" hidden in pointer.
Can we #define the bit where we're keeping the "first" status? This way we can
immediatelly see what's going on in the callers.
Or at least add a comment...

With that:

Reviewed-by: Michał Winiarski <michal.winiarski at intel.com>

While this is an RFC, I think 1-3 make the code more clear, and could be pushed
as-is (perhaps this one with slightly amended commit message s/next patch/future)

-Michał

>  }
>  
>  static inline void
> @@ -712,6 +709,17 @@ static void intel_lrc_irq_handler(unsigned long data)
>  	intel_uncore_forcewake_put(dev_priv, engine->fw_domains);
>  }
>  
> +static void insert_request(struct intel_engine_cs *engine,
> +			   struct i915_priotree *pt,
> +			   int prio)
> +{
> +	struct i915_priolist *p = lookup_priolist(engine, pt, prio);
> +
> +	list_add_tail(&pt->link, &ptr_mask_bits(p, 1)->requests);
> +	if (ptr_unmask_bits(p, 1) && execlists_elsp_ready(engine))
> +		tasklet_hi_schedule(&engine->irq_tasklet);
> +}
> +
>  static void execlists_submit_request(struct drm_i915_gem_request *request)
>  {
>  	struct intel_engine_cs *engine = request->engine;
> @@ -720,12 +728,7 @@ 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);
>  
> -	if (insert_request(engine,
> -			   &request->priotree,
> -			   request->priotree.priority)) {
> -		if (execlists_elsp_ready(engine))
> -			tasklet_hi_schedule(&engine->irq_tasklet);
> -	}
> +	insert_request(engine, &request->priotree, request->priotree.priority);
>  
>  	GEM_BUG_ON(!engine->execlist_first);
>  	GEM_BUG_ON(list_empty(&request->priotree.link));
> -- 
> 2.13.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx