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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
