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

[Tvrtko Ursulin]

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... ?

Regards,

Tvrtko