[Intel-gfx] [PATCH v10] drm/i915: Extend LRC pinning to cover GPU context writeback

[Tvrtko Ursulin]

On 19/01/16 10:24, Tvrtko Ursulin wrote:
>
>
> On 18/01/16 20:47, Chris Wilson wrote:
>> On Mon, Jan 18, 2016 at 05:14:26PM +0000, Tvrtko Ursulin wrote:
>>>
>>> On 18/01/16 16:53, Chris Wilson wrote:
>>>> On Mon, Jan 18, 2016 at 03:02:25PM +0000, Tvrtko Ursulin wrote:
>>>>> -       while (!list_empty(&ring->request_list)) {
>>>>> -               struct drm_i915_gem_request *request;
>>>>> -
>>>>> -               request = list_first_entry(&ring->request_list,
>>>>> -                                          struct
>>>>> drm_i915_gem_request,
>>>>> -                                          list);
>>>>> -
>>>>> -               if (!i915_gem_request_completed(request, true))
>>>>> +       list_for_each_entry_safe(req, next, &ring->request_list,
>>>>> list) {
>>>>> +               if (!i915_gem_request_completed(req, true))
>>>>>                          break;
>>>>>
>>>>> -               i915_gem_request_retire(request);
>>>>> +               if (!i915.enable_execlists ||
>>>>> !i915.enable_guc_submission) {
>>>>> +                       i915_gem_request_retire(req);
>>>>> +               } else {
>>>>> +                       prev_req = list_prev_entry(req, list);
>>>>> +                       if (prev_req)
>>>>> +                               i915_gem_request_retire(prev_req);
>>>>> +               }
>>>>>          }
>>>>>
>>>>> To explain, this attempts to ensure that in GuC mode requests are only
>>>>> unreferenced if there is a *following* *completed* request.
>>>>>
>>>>> This way, regardless of whether they are using the same or different
>>>>> contexts, we can be sure that the GPU has either completed the
>>>>> context writing, or that the unreference will not cause the final
>>>>> unpin of the context.
>>>>
>>>> This is the first bogus step. contexts have to be unreferenced from
>>>> request retire, not request free. As it stands today, this forces us to
>>>> hold the struct_mutex for the free (causing many foul ups along the
>>>> line).  The only reason why it is like that is because of execlists not
>>>> decoupling its context pinning inside request cancel.
>>>
>>> What is the first bogus step? My idea of how to fix the GuC issue,
>>> or the mention of final unreference in relation to GPU completing
>>> the submission?
>>
>> That we want to want to actually unreference the request. We want to
>> unpin the context at the appropriate juncture. At the moment, it looks
>
> What would be the appropriate juncture? With GuC we don't have the
> equivalent of context complete irq.
>
>> like that you are conflating those two steps: "requests are only
>> unreferenced". Using the retirement mechanism would mean coupling the
>> context unpinning into a subsequent request rather than defer retiring a
>> completed request, for example legacy uses active vma tracking to
>> accomplish the same thing. Aiui, the current claim is that we couldn't
>> do that since the guc may reorder contexts - except that we currently
>> use a global seqno so that would be bad on many levels.
>
> I don't know legacy. :( I can see that request/context lifetime is
> coupled there and associated with request creation to retirement.
>
> Does it have the same problem of seqno signaling completion before the
> GPU is done with writing out the context image and how does it solve that?

Ok I think I am starting to see the legacy code paths.

Interesting areas are i915_switch_context + do_switch which do the 
ring->last_context tracking and make the ring/engine own one extra 
reference on the context.

Then, code paths which want to make sure no user context are active on 
the GPU call i915_gpu_idle and submit a dummy default context request.

The latter even explicitly avoids execlist mode.

So unless I am missing something, we could just unify the behaviour 
between the two. Make ring/engine->last_context do the identical 
tracking as legacy context switching and let i915_gpu_idle idle the GPU 
in execlist mode as well?

Regards,

Tvrtko