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

[Chris Wilson]

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