Re: [PATCH 2/2] drm/i915/gt: Schedule request retirement when timeline idles

[Sultan Alsawaf <sultan@kerneltoast.com>]

On Mon, Mar 30, 2020 at 05:45:25PM +0100, Chris Wilson wrote:
> Quoting Sultan Alsawaf (2020-03-30 17:35:14)
> > On Mon, Mar 30, 2020 at 05:27:28PM +0100, Chris Wilson wrote:
> > > Quoting Sultan Alsawaf (2020-03-30 04:30:57)
> > > > +static void engine_retire(struct work_struct *work)
> > > > +{
> > > > +       struct intel_engine_cs *engine =
> > > > +               container_of(work, typeof(*engine), retire_work);
> > > > +       struct intel_timeline *tl = xchg(&engine->retire, NULL);
> > > > +
> > > > +       do {
> > > > +               struct intel_timeline *next = xchg(&tl->retire, NULL);
> > > > +
> > > > +               /*
> > > > +                * Our goal here is to retire _idle_ timelines as soon as
> > > > +                * possible (as they are idle, we do not expect userspace
> > > > +                * to be cleaning up anytime soon).
> > > > +                *
> > > > +                * If the timeline is currently locked, either it is being
> > > > +                * retired elsewhere or about to be!
> > > > +                */
> > > 
> > > iirc the backport requires the retirement to be wrapped in struct_mutex
> > > 
> > > mutex_lock(&engine->i915->drm.struct_mutex);
> > > 
> > > > +               if (mutex_trylock(&tl->mutex)) {
> > > > +                       retire_requests(tl);
> > > > +                       mutex_unlock(&tl->mutex);
> > > > +               }
> > > 
> > > mutex_unlock(&engine->i915->drm.struct_mutex);
> > > 
> > > Now the question is whether that was for 5.3 or 5.4. I think 5.3 is
> > > where the changes were more severe and where we switch to the 4.19
> > > approach.
> > > 
> > > > +               intel_timeline_put(tl);
> > > > +
> > > > +               GEM_BUG_ON(!next);
> > > > +               tl = ptr_mask_bits(next, 1);
> > > > +       } while (tl);
> > > > +}
> > 
> > In 5.4, the existing retirement instances don't hold
> > `&engine->i915->drm.struct_mutex`, however it is held in 5.3. So it looks like
> > this is fine as-is for 5.4.
> 
> git agrees.
> 
> commit e5dadff4b09376e8ed92ecc0c12f1b9b3b1fbd19
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Thu Aug 15 21:57:09 2019 +0100
> 
>     drm/i915: Protect request retirement with timeline->mutex
> 
> is in v5.4, so we should be safe to retire without the struct_mutex.
> -Chris

No, you were right; `&engine->i915->drm.struct_mutex` needs to be held in 5.4,
otherwise retirement races with vma destruction. I'll send an updated patch.

Sultan