Re: [RFC 4/4] drm/i915: Stop tracking execlists retired requests

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

On 08/04/16 15:57, Chris Wilson wrote:
> On Fri, Apr 08, 2016 at 02:54:58PM +0100, Tvrtko Ursulin wrote:
>> @@ -615,11 +613,6 @@ static void execlists_context_queue(struct drm_i915_gem_request *request)
>>   	struct drm_i915_gem_request *cursor;
>>   	int num_elements = 0;
>>
>> -	if (request->ctx != request->i915->kernel_context)
>> -		intel_lr_context_pin(request->ctx, engine);
>> -
>> -	i915_gem_request_reference(request);
>> -
>>   	spin_lock_bh(&engine->execlist_lock);
>>
>>   	list_for_each_entry(cursor, &engine->execlist_queue, execlist_link)
>> @@ -636,11 +629,12 @@ static void execlists_context_queue(struct drm_i915_gem_request *request)
>>   		if (request->ctx == tail_req->ctx) {
>>   			WARN(tail_req->elsp_submitted != 0,
>>   				"More than 2 already-submitted reqs queued\n");
>> -			list_move_tail(&tail_req->execlist_link,
>> -				       &engine->execlist_retired_req_list);
>> +			list_del(&tail_req->execlist_link);
>> +			i915_gem_request_unreference(tail_req);
>>   		}
>>   	}
>>
>> +	i915_gem_request_reference(request);
>>   	list_add_tail(&request->execlist_link, &engine->execlist_queue);
>
> If you want to get truly radical, we do not need the ref on the request
> until it is submitted to hardware. (As the request cannot be retired
> until it has done so, it can leave the execlist_queue until we commit it
> to hw, or perform the cancel).

Don't know. It is simple and nice that reference is tied to presence on 
execlist_queue.

More importantly, the patch as presented has a flaw that it dereferences 
req->ctx from execlists_check_remove_request where the context pin may 
have disappeared already due context complete interrupts getting behind 
when coallescing.

I will need to cache ctx_id in the request I think. It is fine do to 
that since ctx id must be unique and stable for a request.

Maybe even I should pull in your patch which makes ctx ids stable and 
persistent aligned with context existence and not pin. Think I've seen 
something like that somewhere.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx