On 24/04/2018 15:57, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2018-04-24 15:46:49)
On 24/04/2018 14:14, Chris Wilson wrote:
In the next patch, rings are the central timeline as requests may jump
between engines. Therefore in the future as we retire in order along the
engine timeline, we may retire out-of-order within a ring (as the ring now
occurs along multiple engines), leading to much hilarity in miscomputing
the position of ring->head.
As an added bonus, retiring along the ring reduces the penalty of having
one execlists client do cleanup for another (old legacy submission
shares a ring between all clients). The downside is that slow and
irregular (off the critical path) process of cleaning up stale requests
after userspace becomes a modicum less efficient.
In the long run, it will become apparent that the ordered
ring->request_list matches the ring->timeline, a fun challenge for the
future will be unifying the two lists to avoid duplication!
v2: We need both engine-order and ring-order processing to maintain our
knowledge of where individual rings have completed upto as well as
knowing what was last executing on any engine. And finally by decoupling
retiring the contexts on the engine and the timelines along the rings,
we do have to keep a reference to the context on each request
(previously it was guaranteed by the context being pinned).
Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursu...@intel.com>
---
@@ -322,9 +325,9 @@ static void advance_ring(struct i915_request *request)
} else {
tail = request->postfix;
}
- list_del(&request->ring_link);
+ list_del_init(&request->ring_link);
Why the _init flavour? There are two list heads for being on two
separate lists, but are either path reachable after being removed from
the respective lists? (I may find the answer as I read on.)
Yes. rq are held elsewhere and may end up being individually retired
(via the retire_upto paths) multiple times. i915_request_retire() should
only be called once (otherwise asserts).
So init + list_empty() is being used to prevent retiring the same
request multiple times.
- if (engine->last_retired_context)
- engine->context_unpin(engine, engine->last_retired_context);
- engine->last_retired_context = request->ctx;
-
- spin_lock_irq(&request->lock);
- if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &request->fence.flags))
- dma_fence_signal_locked(&request->fence);
- if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &request->fence.flags))
- intel_engine_cancel_signaling(request);
- if (request->waitboost) {
- GEM_BUG_ON(!atomic_read(&request->i915->gt_pm.rps.num_waiters));
- atomic_dec(&request->i915->gt_pm.rps.num_waiters);
- }
- spin_unlock_irq(&request->lock);
+ __retire_engine_upto(request->engine, request);
I did not figure out why do you need to do this. Normally retirement
driven from ring timelines would retire requests on the same engine
belonging to a different ring, as it walks over ring timelines.
Right, so retiring along a ring ends up out of order for a particular
engine.
Only for direct callers of i915_request_retire it may make a difference
but I don't understand is it an functional difference or just optimisation.
I was hoping the GEM_BUG_ON(!list_is_first()); explained the rationale.
Thanks, I remembered after pressing send.
This is then from where list_del_init comes from, since this function
can retire wider than the caller would expect. But then it retires on
the engine (upto) and the callers just walks the list and calls retire
only to find already unlinked elements. Could it just as well then
retire it completely?
We're still trying to only do as little work as we can get away with.
I was thinking about something else, but it was a flawed idea since two
walks are crucial for correct ordering for both engine and ring retirement.
- /* Move the oldest request to the slab-cache (if not in use!) */
- rq = list_first_entry_or_null(&engine->timeline->requests,
- typeof(*rq), link);
+ /* Move our oldest request to the slab-cache (if not in use!) */
+ rq = list_first_entry_or_null(&ring->request_list,
+ typeof(*rq), ring_link);
This is less efficient now - I wonder if you should still be looking at
the engine timeline here?
No, either way you have to walk all engine->requests upto this point, or
all ring->requests. To keep the interface manageable, retirement is in
ring order.
On the other hand, we don't retire as often if we can get away with it.
1 step forwards, 1 step backwards.
AFAIR and AFAICS the point of this was to free up the oldest request
just to help with request recycling as new ones are added. By now
changing it to be oldest on the ring timeline it a) my not be very old,
or b) not even completed, while there might be much older and completed
requests on the respective engine timeline.
Hm, or are you saying that it could cause out of order for the ring? I
guess it could yeah.. it would have to use the upto variant.
Related, there could be potential to unify i915_request_retire_upto and
ring_retire_requests. Latter could pass in NULL as the upto request,
just the completed check would need to be different depending on the mode.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
