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.
Sultan
