[Intel-gfx] [PATCH v3] drm/i915: Copy across scheduler behaviour flags across submit fences

[Tvrtko Ursulin]

On 10/12/2019 14:29, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-12-10 14:12:31)
>>
>> On 10/12/2019 13:06, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2019-12-10 12:41:36)
>>>>
>>>> On 04/12/2019 21:17, Chris Wilson wrote:
>>>>> +static int
>>>>> +__i915_request_await_execution(struct i915_request *to,
>>>>> +                            struct i915_request *from,
>>>>> +                            void (*hook)(struct i915_request *rq,
>>>>> +                                         struct dma_fence *signal))
>>>>> +{
>>>>> +     int err;
>>>>> +
>>>>> +     /* Submit both requests at the same time */
>>>>> +     err = __await_execution(to, from, hook, I915_FENCE_GFP);
>>>>> +     if (err)
>>>>> +             return err;
>>>>> +
>>>>> +     if (!to->engine->schedule)
>>>>> +             return 0;
>>>>
>>>> Hm is this about scheduler or preemption?
>>>
>>> It's about dependency tracking, and the lack of it.
>>>    
>>>>> +
>>>>> +     /* Squash repeated depenendices to the same timelines */
>>>>
>>>> typo in dependencies
>>>>
>>>>> +     if (intel_timeline_sync_has_start(i915_request_timeline(to),
>>>>> +                                       &from->fence))
>>>>> +             return 0;
>>>>> +
>>>>> +     /* Ensure both start together [after all semaphores in signal] */
>>>>> +     if (intel_engine_has_semaphores(to->engine))
>>>>> +             err =__emit_semaphore_wait(to, from, from->fence.seqno - 1);
>>>>
>>>> So the only thing preventing B' getting onto the same engine as A, as
>>>> soon as B enters a different engine, is the priority order?
>>>
>>> No. Now B' has a dependency on A, so B' is always after A.
>>
>> Yes, true.
>>
>>>> And if I am reading this correctly, change relative to current state is
>>>> to let B' in, but spin on a semaphore, where currently we let it execute
>>>> the actual payload.
>>>>
>>>> It's possible that we do need this if we said we would guarantee bonded
>>>> request would not execute before it's master. Have we made this
>>>> guarantee when discussing the uAPI? We must had..
>>>
>>> We did not make that guarantee as the assumption was that all fences for
>>> B would be passed to B'. However, the since fence slot for IN/SUBMIT
>>
>> I think we must have made a guarantee B' won't start executing before B.
>> That is kind of the central point. Only thing we did not do is
>> guarantee/made effort to start B' together with B. But guarantee got
>> defeated by ELSP and later timeslicing/preemption.
> 
> According to the implementation, we did not ;)
> 
> I agree that the stronger coordination makes more sense for the API, and
> the inheritance of dependencies I think is crucial for exported fences.
>   
>> Previously, miss was if B was in ELSP[1] B' could be put in ELSP[0]
>> (different engines). Which is wrong. And with timeslicing/preemption
>> even more so.
> 
> It wasn't actually an oversight :) My understanding was that B' payload
> would not start before B payload via user semaphores, so I wrote it off
> as a bad bubble.
> 
> The oversight, imo, is that we shouldn't rely on userspace for this and
> with the current implementation it is easy to lose PI.
>   
>> So having await_started or semaphore looks correct in that respect. And
>> scheduler deps cover the A in chain. So I think it's good with this patch.
> 
> Agreed.

For correctness you don't think if !scheduler early return should happen 
after the semaphore and await_start insertion? I know that where we have 
ELSP we have scheduler so maybe it is moot point.

Regards,

Tvrtko