[Intel-gfx] [PATCH 5/8] drm/i915: Protect request retirement with timeline->mutex

[Chris Wilson]

Quoting Matthew Auld (2019-08-15 21:33:07)
> On Wed, 14 Aug 2019 at 10:28, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> >
> > Forgo the struct_mutex requirement for request retirement as we have
> > been transitioning over to only using the timeline->mutex for
> > controlling the lifetime of a request on that timeline.
> >
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >  .../gpu/drm/i915/gem/i915_gem_execbuffer.c    | 183 ++++++++++--------
> >  drivers/gpu/drm/i915/gt/intel_context.h       |  18 +-
> >  drivers/gpu/drm/i915/gt/intel_engine_cs.c     |   1 -
> >  drivers/gpu/drm/i915/gt/intel_engine_types.h  |   3 -
> >  drivers/gpu/drm/i915/gt/intel_gt.c            |   2 -
> >  drivers/gpu/drm/i915/gt/intel_gt_types.h      |   2 -
> >  drivers/gpu/drm/i915/gt/intel_lrc.c           |   1 +
> >  drivers/gpu/drm/i915/gt/intel_ringbuffer.c    |  19 +-
> >  drivers/gpu/drm/i915/gt/mock_engine.c         |   1 -
> >  drivers/gpu/drm/i915/gt/selftest_context.c    |   9 +-
> >  drivers/gpu/drm/i915/i915_request.c           | 156 +++++++--------
> >  drivers/gpu/drm/i915/i915_request.h           |   3 -
> >  12 files changed, 209 insertions(+), 189 deletions(-)
> >
> 
> >  bool i915_retire_requests(struct drm_i915_private *i915)
> >  {
> > -       struct intel_ring *ring, *tmp;
> > +       struct intel_gt_timelines *timelines = &i915->gt.timelines;
> > +       struct intel_timeline *tl, *tn;
> > +       LIST_HEAD(free);
> > +
> > +       spin_lock(&timelines->lock);
> > +       list_for_each_entry_safe(tl, tn, &timelines->active_list, link) {
> > +               if (!mutex_trylock(&tl->mutex))
> > +                       continue;
> >
> > -       lockdep_assert_held(&i915->drm.struct_mutex);
> > +               intel_timeline_get(tl);
> > +               GEM_BUG_ON(!tl->active_count);
> > +               tl->active_count++; /* pin the list element */
> > +               spin_unlock(&timelines->lock);
> >
> > -       list_for_each_entry_safe(ring, tmp,
> > -                                &i915->gt.active_rings, active_link) {
> > -               intel_ring_get(ring); /* last rq holds reference! */
> > -               ring_retire_requests(ring);
> > -               intel_ring_put(ring);
> > +               retire_requests(tl);
> > +
> > +               spin_lock(&timelines->lock);
> > +
> > +               /* Restart iteration after dropping lock */
> > +               list_safe_reset_next(tl, tn, link);
> 
> That's a new one.

I was quite chuffed with that loop, all the pinning across the lock
dropping to ensure the list stayed intact and we could resume from where
we left off.

And if all goes to plan, we should be rarely using this loop!
-Chris