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->mutexis 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