[Intel-gfx] [PATCH 02/13] drm/i915: Only queue requests during execlists submission

[Mika Kuoppala]

Chris Wilson <chris at chris-wilson.co.uk> writes:

> On Fri, Aug 26, 2016 at 12:41:16PM +0300, Mika Kuoppala wrote:
>> Chris Wilson <chris at chris-wilson.co.uk> writes:
>> 
>> > Leave the more complicated request dequeueing to the tasklet and instead
>> > just kick start the tasklet if we detect we are adding the first
>> > request.
>> >
>> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>> > ---
>> >  drivers/gpu/drm/i915/intel_lrc.c | 28 ++++------------------------
>> >  1 file changed, 4 insertions(+), 24 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> > index 6b49df4316f4..ca52b8e63305 100644
>> > --- a/drivers/gpu/drm/i915/intel_lrc.c
>> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> > @@ -609,35 +609,15 @@ static void intel_lrc_irq_handler(unsigned long data)
>> >  static void execlists_submit_request(struct drm_i915_gem_request *request)
>> >  {
>> >  	struct intel_engine_cs *engine = request->engine;
>> > -	struct drm_i915_gem_request *cursor;
>> > -	int num_elements = 0;
>> >  
>> >  	spin_lock_bh(&engine->execlist_lock);
>> >  
>> > -	list_for_each_entry(cursor, &engine->execlist_queue, execlist_link)
>> > -		if (++num_elements > 2)
>> > -			break;
>> > -
>> > -	if (num_elements > 2) {
>> > -		struct drm_i915_gem_request *tail_req;
>> > -
>> > -		tail_req = list_last_entry(&engine->execlist_queue,
>> > -					   struct drm_i915_gem_request,
>> > -					   execlist_link);
>> > -
>> > -		if (request->ctx == tail_req->ctx) {
>> > -			WARN(tail_req->elsp_submitted != 0,
>> > -				"More than 2 already-submitted reqs queued\n");
>> > -			list_del(&tail_req->execlist_link);
>> > -			i915_gem_request_put(tail_req);
>> > -		}
>> > -	}
>> > -
>> >  	i915_gem_request_get(request);
>> > -	list_add_tail(&request->execlist_link, &engine->execlist_queue);
>> >  	request->ctx_hw_id = request->ctx->hw_id;
>> > -	if (num_elements == 0)
>> > -		execlists_unqueue(engine);
>> > +
>> > +	list_add_tail(&request->execlist_link, &engine->execlist_queue);
>> > +	if (request->execlist_link.prev == &engine->execlist_queue)
>> 
>> Why not list_first_entry()?
>
> Ah, because it's not as magic as I thought :)
>
> if (list_first_entry(&engine->execlist_queue,
> 		     struct drm_i915_gem_request,
> 		     execlist_link) == request)
>
> I think is reason enough.

Agreed. Looked at

if (list_is_singular(&engine->execlist_queue))

but that it will add needless empty check.

If that is abomination, just add comment that we need to resubmit
if the list was empty.

Without digging the list internals, the next reader also
could be more happy with

head->next == head->prev check.

-Mika

> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre