Re: [Intel-gfx] [PATCH 3/7] drm/i915: Retire requests along rings

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-04-27 13:50:44)
> 
> On 26/04/2018 18:49, Chris Wilson wrote:
> > +static void __retire_engine_request(struct intel_engine_cs *engine,
> > + struct i915_request *rq)
> > +{
> > + GEM_TRACE("%s(%s) fence %llx:%d, global=%d, current %d\n",
> > +   __func__, engine->name,
> > +   rq->fence.context, rq->fence.seqno,
> > +   rq->global_seqno,
> > +   intel_engine_get_seqno(engine));
> > +
> > + GEM_BUG_ON(!i915_request_completed(rq));
> > +
> > + local_irq_disable();
> > +
> > + spin_lock(>timeline->lock);
> > + GEM_BUG_ON(!list_is_first(>link, >timeline->requests));
> 
> Assert not strictly needed because how the single caller pops the 
> elements off, but maybe in the future something changes.

Indeed, useful if we do add a direct call here; and useful to reiterate
the point that the engine->last_retired_context depends on ordering.

> > +static void __retire_engine_upto(struct intel_engine_cs *engine,
> > +  struct i915_request *rq)
> > +{
> > + struct i915_request *tmp;
> > +
> > + if (list_empty(>link))
> > + return;
> > +
> > + do {
> > + tmp = list_first_entry(>timeline->requests,
> > +typeof(*tmp), link);
> > +
> > + GEM_BUG_ON(tmp->engine != engine);
> 
> Very minor - move this assert to outside the loop as rq->engine != engine?

I felt validating the engine backpointer along the list at various
points will be handy. Especially when request.engine becomes a bit more
volatile.

> >   void i915_request_retire_upto(struct i915_request *rq)
> >   {
> > - struct intel_engine_cs *engine = rq->engine;
> > + struct intel_ring *ring = rq->ring;
> >   struct i915_request *tmp;
> >   
> > + GEM_TRACE("%s fence %llx:%d, global=%d, current %d\n",
> > +   rq->engine->name,
> > +   rq->fence.context, rq->fence.seqno,
> > +   rq->global_seqno,
> > +   intel_engine_get_seqno(rq->engine));
> 
> Maybe we could consolidate these with GEM_TRACE_RQ(rq, "prefix") or 
> something.

Worth trying, if you mean to completely stop me from adding
inconsistency to every GEM_TRACE. Spoilsport.

> > @@ -651,9 +694,9 @@ i915_request_alloc(struct intel_engine_cs *engine, 
> > struct i915_gem_context *ctx)
> >   if (ret)
> >   goto err_unreserve;
> >   
> > - /* Move the oldest request to the slab-cache (if not in use!) */
> > - rq = list_first_entry_or_null(>timeline->requests,
> > -   typeof(*rq), link);
> > + /* Move our oldest request to the slab-cache (if not in use!) */
> > + rq = list_first_entry_or_null(>request_list,
> > +   typeof(*rq), ring_link);
> 
> This one I still think it will reduce the recycling effectiveness. But 
> OK, can leave it for later if it will become noticeable.

Or the inefficiency of retiring more than required. So long as we do
prune regularly, we can hold off the oom daemons.

Yes, it's not as neat as first planned.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/7] drm/i915: Retire requests along rings

[Tvrtko Ursulin]

On 26/04/2018 18:49, 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).

v3: Not just a reference to the context, but we need to keep it pinned
as we manipulate the rings; i.e. we need a pin for both the manipulation
of the engine state during its retirements, and a separate pin for the
manipulation of the ring state.

Signed-off-by: Chris Wilson 
Cc: Tvrtko Ursulin 
---
  drivers/gpu/drm/i915/i915_drv.h   |   3 +-
  drivers/gpu/drm/i915/i915_gem.c   |   1 +
  drivers/gpu/drm/i915/i915_request.c   | 150 +++---
  drivers/gpu/drm/i915/i915_utils.h |   6 +
  drivers/gpu/drm/i915/intel_ringbuffer.c   |   6 +-
  drivers/gpu/drm/i915/intel_ringbuffer.h   |   1 +
  drivers/gpu/drm/i915/selftests/mock_engine.c  |  27 +++-
  .../gpu/drm/i915/selftests/mock_gem_device.c  |   2 +
  8 files changed, 131 insertions(+), 65 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8fd9fb6efba5..1837c01d44d0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2058,8 +2058,9 @@ struct drm_i915_private {
void (*resume)(struct drm_i915_private *);
void (*cleanup_engine)(struct intel_engine_cs *engine);
  
-		struct list_head timelines;

struct i915_gem_timeline global_timeline;
+   struct list_head timelines;
+   struct list_head rings;
u32 active_requests;
u32 request_serial;
  
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c

index 4090bfdda340..f0644d1fbd75 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5600,6 +5600,7 @@ int i915_gem_init_early(struct drm_i915_private *dev_priv)
goto err_dependencies;
  
  	mutex_lock(_priv->drm.struct_mutex);

+   INIT_LIST_HEAD(_priv->gt.rings);
INIT_LIST_HEAD(_priv->gt.timelines);
err = i915_gem_timeline_init__global(dev_priv);
mutex_unlock(_priv->drm.struct_mutex);
diff --git a/drivers/gpu/drm/i915/i915_request.c 
b/drivers/gpu/drm/i915/i915_request.c
index 9358f2cf0c32..e6535255d445 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -286,6 +286,7 @@ static int reserve_gt(struct drm_i915_private *i915)
  
  static void unreserve_gt(struct drm_i915_private *i915)

  {
+   GEM_BUG_ON(!i915->gt.active_requests);
if (!--i915->gt.active_requests)
i915_gem_park(i915);
  }
@@ -298,6 +299,7 @@ void i915_gem_retire_noop(struct i915_gem_active *active,
  
  static void advance_ring(struct i915_request *request)

  {
+   struct intel_ring *ring = request->ring;
unsigned int tail;
  
  	/*

@@ -309,7 +311,8 @@ static void advance_ring(struct i915_request *request)
 * Note this requires that we are always called in request
 * completion order.
 */
-   if (list_is_last(>ring_link, >ring->request_list)) {
+   GEM_BUG_ON(!list_is_first(>ring_link, >request_list));
+   if (list_is_last(>ring_link, >request_list)) {
/*
 * We may race here with execlists resubmitting this request
 * as we retire it. The resubmission will move the ring->tail
@@ -322,9 +325,9 @@ static void advance_ring(struct i915_request *request)
} else {
tail = request->postfix;
}
-   list_del(>ring_link);
+   list_del_init(>ring_link);
  
-	request->ring->head = tail;

+   ring->head = tail;
  }
  
  static void free_capture_list(struct i915_request *request)

@@ -340,30 +343,84 @@ static void free_capture_list(struct