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