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