[Intel-gfx] [PATCH 045/190] drm/i915: Move releasing of the GEM request from free to retire/cancel

[Chris Wilson]

On Tue, Apr 05, 2016 at 03:17:30PM +0100, Tvrtko Ursulin wrote:
> 
> On 05/04/16 15:09, Chris Wilson wrote:
> >On Tue, Apr 05, 2016 at 02:42:16PM +0100, Tvrtko Ursulin wrote:
> >>>>@@ -587,9 +587,6 @@ static int execlists_context_queue(struct
> >>>>drm_i915_gem_request *request)
> >>>>      struct drm_i915_gem_request *cursor;
> >>>>      int num_elements = 0;
> >>>>
> >>>>-    if (request->ctx != ring->default_context)
> >>>>-        intel_lr_context_pin(request);
> >>>>-
> >>>
> >>>Since you remove LRC pin from queue, the lifetime is now either:
> >>>
> >>>1. From request create to cancel.
> >>>2. From request create to execlist retirement.
> >>>
> >>>Would it be more logical to leave the LRC pin in queue, but remove it
> >>>from request creation instead? That would make the LRC pin lifetime only
> >>>a single possibility, from queue to execlist retire.
> >
> >Well what we actually need in request allocation is pinning the
> >ringbuffer. At the moment we do that by pinning the request. We also
> >need to pin the VM in order to manipulate it. We could leave pinning the
> >logical context object til actual submission.
> 
> Hmmm, yes. Have to think how complicated or not would that be.
> 
> >>I felt was so close in getting rid of execlist_retired_req_queue,
> >>using this patch as a starting point, when I realised this patch
> >>does not play nicely with the GuC. Back to the drawing board. :(
> >
> >If you mean what happens if the GuC executes requests out-of-order - it
> >can't in the current model since we only have a single timeline and that
> >would break it badly - then nothing changes. We are only moving the
> >release in time, not decoupling it from any serialisation.
> 
> No, there is no LRC unpin, it is unbalanced in the GuC mode with
> this patch. Unless I am missing something... ?

Under GuC submission mode, we acquire the context with

i915_gem_request_alloc ->
	intel_logical_ring_alloc_request_extras ->
	intel_lr_context_pin

intel_logical_ring_advance_and_submit() keeps track of last_context for
both execlists and GuC

and we release the context from

i915_gem_request_retire or i915_gem_request_cancel ->
	__i915_gem_request_release ->
	intel_lr_context_unpin

(We need to fix that piece of insider knowlege.)

i.e. GuC submission LRC tracking behaves identically to execlists.
There's no feedback from requests to the GuC at the moment to track
active GuC clients though.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre