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

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