[Intel-gfx] [RFC 4/4] drm/i915: Stop tracking execlists retired requests

[Chris Wilson]

On Mon, Apr 11, 2016 at 10:10:56AM +0100, Tvrtko Ursulin wrote:
> 
> 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.

Yes, both OA, PASID and vGPU depend upon having stable ctx-id for the
lifetime of the context. Dave expressed a concern that the GuC maybe
interpretting it as the LRCA, but as far as I can see, lrc->ring_lcra
and lrc->context_id are distinct and we never use the global context
identifier itself (the closest is the low 32bits of lrc_desc which do
not include our unique id).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre