[Intel-gfx] [PATCH 07/27] drm/i915: Squash repeated awaits on the same fence

Wed Apr 26 11:18:49 UTC 2017

On Wed, Apr 26, 2017 at 11:54:08AM +0100, Tvrtko Ursulin wrote:
> 
> On 26/04/2017 11:38, Chris Wilson wrote:
> >On Wed, Apr 26, 2017 at 11:20:16AM +0100, Tvrtko Ursulin wrote:
> >>
> >>On 19/04/2017 10:41, Chris Wilson wrote:
> >>>Track the latest fence waited upon on each context, and only add a new
> >>>asynchronous wait if the new fence is more recent than the recorded
> >>>fence for that context. This requires us to filter out unordered
> >>>timelines, which are noted by DMA_FENCE_NO_CONTEXT. However, in the
> >>>absence of a universal identifier, we have to use our own
> >>>i915->mm.unordered_timeline token.
> >>>
> >>>v2: Throw around the debug crutches
> >>>v3: Inline the likely case of the pre-allocation cache being full.
> >>>v4: Drop the pre-allocation support, we can lose the most recent fence
> >>>in case of allocation failure -- it just means we may emit more awaits
> >>>than strictly necessary but will not break.
> >>>v5: Trim allocation size for leaf nodes, they only need an array of u32
> >>>not pointers.
> >>>
> >>>Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> >>>Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>>Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> >>>---
> >>>drivers/gpu/drm/i915/i915_gem_request.c            |  67 +++---
> >>>drivers/gpu/drm/i915/i915_gem_timeline.c           | 260 +++++++++++++++++++++
> >>>drivers/gpu/drm/i915/i915_gem_timeline.h           |  14 ++
> >>>drivers/gpu/drm/i915/selftests/i915_gem_timeline.c | 123 ++++++++++
> >>>.../gpu/drm/i915/selftests/i915_mock_selftests.h   |   1 +
> >>>5 files changed, 438 insertions(+), 27 deletions(-)
> >>>create mode 100644 drivers/gpu/drm/i915/selftests/i915_gem_timeline.c
> >>>
> >>>diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
> >>>index 97c07986b7c1..fb6c31ba3ef9 100644
> >>>--- a/drivers/gpu/drm/i915/i915_gem_request.c
> >>>+++ b/drivers/gpu/drm/i915/i915_gem_request.c
> >>>@@ -730,9 +730,7 @@ int
> >>>i915_gem_request_await_dma_fence(struct drm_i915_gem_request *req,
> >>>				 struct dma_fence *fence)
> >>>{
> >>>-	struct dma_fence_array *array;
> >>>	int ret;
> >>>-	int i;
> >>>
> >>>	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> >>>		return 0;
> >>>@@ -744,39 +742,54 @@ i915_gem_request_await_dma_fence(struct drm_i915_gem_request *req,
> >>>	if (fence->context == req->fence.context)
> >>>		return 0;
> >>>
> >>>-	if (dma_fence_is_i915(fence))
> >>>-		return i915_gem_request_await_request(req, to_request(fence));
> >>>+	/* Squash repeated waits to the same timelines, picking the latest */
> >>>+	if (fence->context != req->i915->mm.unordered_timeline &&
> >>>+	    intel_timeline_sync_get(req->timeline,
> >>>+				    fence->context, fence->seqno))
> >>
> >>Function name is non-intuitive to me. It doesn't seem to get
> >>anything, but is more like query? Since it ends up with
> >>i915_seqno_passed, maybe intel_timeline_sync_is_newer/older ? (give
> >>or take)
> >
> >_get was choosen as the partner for _set, which seemed to make sense.
> >Keep intel_timeline_sync_set() and replace _get with
> >intel_timeline_sync_passed() ?
> >intel_timeline_sync_is_later() ?
> 
> Both are better in my opinion. _get just makes it sounds like it is
> returning something from the object, which it is not. So whichever
> you prefer.
> 
> >>>diff --git a/drivers/gpu/drm/i915/i915_gem_timeline.c b/drivers/gpu/drm/i915/i915_gem_timeline.c
> >>>index b596ca7ee058..f2b734dda895 100644
> >>>--- a/drivers/gpu/drm/i915/i915_gem_timeline.c
> >>>+++ b/drivers/gpu/drm/i915/i915_gem_timeline.c
> >>>@@ -24,6 +24,254 @@
> >>>
> >>>#include "i915_drv.h"
> >>>
> >>>+#define NSYNC 16
> >>>+#define SHIFT ilog2(NSYNC)
> >>>+#define MASK (NSYNC - 1)
> >>>+
> >>>+/* struct intel_timeline_sync is a layer of a radixtree that maps a u64 fence
> >>>+ * context id to the last u32 fence seqno waited upon from that context.
> >>>+ * Unlike lib/radixtree it uses a parent pointer that allows traversal back to
> >>>+ * the root. This allows us to access the whole tree via a single pointer
> >>>+ * to the most recently used layer. We expect fence contexts to be dense
> >>>+ * and most reuse to be on the same i915_gem_context but on neighbouring
> >>>+ * engines (i.e. on adjacent contexts) and reuse the same leaf, a very
> >>>+ * effective lookup cache. If the new lookup is not on the same leaf, we
> >>>+ * expect it to be on the neighbouring branch.
> >>>+ *
> >>>+ * A leaf holds an array of u32 seqno, and has height 0. The bitmap field
> >>>+ * allows us to store whether a particular seqno is valid (i.e. allows us
> >>>+ * to distinguish unset from 0).
> >>>+ *
> >>>+ * A branch holds an array of layer pointers, and has height > 0, and always
> >>>+ * has at least 2 layers (either branches or leaves) below it.
> >>>+ *
> >>>+ */
> >>
> >>@_@ :)
> >>
> >>Ok, so a map of u64 to u32. We can't use IDR or radixtree directly
> >>because of u64 keys. :( How about a hash table? It would be much
> >>simpler to review. :) Seriously, if it would perform close enough it
> >>would be a much much simpler implementation.
> >
> >You want a resizable hashtable. rht is appallingly slow, so you want a
> >custom resizeable ht. They are not as simple as this codewise ;)
> >(Plus a compressed radixtree is part of my plan for scalability
> >improvements for struct reservation_object.)
> 
> Why resizable? I was thinking a normal one. If at any given time we
> have an active set of contexts, or at least lookups are as you say
> below, to neighbouring contexts, that would mean we are talking
> about lookups to different hash buckets.  And for the typical
> working set we would expect many collisions so longer lists in each
> bucket? So maybe NUM_ENGINES * some typical load constant number
> buckets would not be that bad?

Consider a long running display server that will accumulate 10,000s of
thousands of clients in its lifetime, each with their own contents that
get shared by passing around fences/framebuffers. (Or on a shorter scale
any of the context stress tests in igt.) Due to the non-recycling of the
context ids, we can grow to very large tables - but we have no knowledge
of what contexts are no longer required.

To compensate we need to occasionally prune the sync points. For a ht we
could just scrap it, For an idr, we could store last use and delete
stale leaves.

But first we have a question of how many buckets do we give the static
ht? Most processes will be sharing between 2 contexts (render context,
presentation context) except for a display server who may have 10-100 of
clients - and possibly where eliminating repeated syncs is going to be
most valuable. That suggests 256 buckets for every timeline (assuming
just a pair of engines across shared contexts). Overkill for the
majority, and going to be miserable in stress tests.

Finally what do you store in the ht? Fences are the obvious candidate,
but need reaping. Or you just create a ht for the request and only
squash repeated fences inside a single request - that doesn't benefit
from timeline tracking and squashing between requests (but does avoid
keeping fences around forever). Hence why I went with tracking seqno. To
avoid allocations for the ht nodes, we could create an open-addressed ht
with the {context, seqno} embedded in it. It would be efficient, but
needs online resizing and is a fair chunk of new code.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre