[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:27:24PM +0100, Chris Wilson wrote:
> 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.

>From IRC, this is what is in my tree not what is at the time of the
patch. Sorry, so what I need to rebalance context-pinning is your
suggestion of tracking the pinned-context on the request:

https://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=tasklet&id=5c57c9d270b5fcbf9e89804c623c96027746ed86

Thanks, I hope that will slot in nicely....
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre