[PATCH 2/4] drm/i915: Retire requests along rings

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-04-20 15:28:35)
> 
> On 20/04/2018 12:35, 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.
> 
> Will we be retiring out of order within a ring? I thought we said not, 
> that is no parallel execution of a single ring on two engines. And if we 
> won't then I don't see how ring is retired out of order.

Within a ring? No. The problem occurred (without this patch) as the ring
popped up on different engines, and so advance_ring() was called out of
sync (but in order on each engine ofc). As a result ring->head jumped
all over the place.

> >   void i915_retire_requests(struct drm_i915_private *i915)
> >   {
> > -     struct intel_engine_cs *engine;
> > -     enum intel_engine_id id;
> > +     struct intel_ring *ring, *next;
> >   
> >       lockdep_assert_held(&i915->drm.struct_mutex);
> >   
> >       if (!i915->gt.active_requests)
> >               return;
> >   
> > -     for_each_engine(engine, i915, id)
> > -             engine_retire_requests(engine);
> > +     list_for_each_entry_safe(ring, next, &i915->gt.rings, link)
> > +             ring_retire_requests(ring);
> 
> This is the less efficient bit - but I actually cannot decide if it 
> matters in practice (to walk potentially so many more lists), or the 
> removal of irq spinlock wins. For real-world use it probably doesn't 
> matter. For pathological IGTs we probably don't care.

Aye. i915_retire_requests() should never be on the critical path; it's
used as the catch up after idling, or by the idle-worker to flush the
requests. It is run under struct_mutex though, and as such not entirely
painless. Solving requests + struct_mutex will certainly be interesting.
-Chris