[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(&engine->timeline->lock);
> > +     GEM_BUG_ON(!list_is_first(&rq->link, &engine->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(&rq->link))
> > +             return;
> > +
> > +     do {
> > +             tmp = list_first_entry(&engine->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(&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 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