Hi John, On Fri, Mar 29, 2024 at 04:53:05PM -0700, [email protected] wrote: > From: John Harrison <[email protected]> > > The previous fix for the circlular lock splat about the busyness > worker wasn't quite complete. Even though the reset-in-progress flag > is cleared at the start of intel_uc_reset_finish, the entire function > is still inside the reset mutex lock. Not sure why the patch appeared > to fix the issue both locally and in CI. However, it is now back > again. > > There is a further complication the wedge code path within > intel_gt_reset() jumps around so much it results in nested > reset_prepare/_finish calls. That is, the call sequence is: > intel_gt_reset > | reset_prepare > | __intel_gt_set_wedged > | | reset_prepare > | | reset_finish > | reset_finish > > The nested finish means that even if the clear of the in-progress flag > was moved to the end of _finish, it would still be clear for the > entire second call. Surprisingly, this does not seem to be causing any > other problems at present. > > As an aside, a wedge on fini does not call the finish functions at > all. The reset_in_progress flag is left set (twice). > > So instead of trying to cancel the worker anywhere at all in the reset > path, just add a cancel to intel_guc_submission_fini instead. Note > that it is not a problem if the worker is still active during a reset. > Either it will run before the reset path starts locking things and > will simply block the reset code for a tiny amount of time. Or it will > run after the locks have been acquired and will early exit due to the > try-lock. > > Also, do not use the reset-in-progress flag to decide whether a > synchronous cancel is safe (from a lockdep perspective) or not. > Instead, use the actual reset mutex state (both the genuine one and > the custom rolled BACKOFF one). > > Fixes: 0e00a8814eec ("drm/i915/guc: Avoid circular locking issue on busyness > flush") > Signed-off-by: John Harrison <[email protected]> > Cc: Zhanjun Dong <[email protected]> > Cc: John Harrison <[email protected]> > Cc: Andi Shyti <[email protected]> > Cc: Daniel Vetter <[email protected]> > Cc: Daniel Vetter <[email protected]> > Cc: Rodrigo Vivi <[email protected]> > Cc: Nirmoy Das <[email protected]> > Cc: Tvrtko Ursulin <[email protected]> > Cc: Umesh Nerlige Ramappa <[email protected]> > Cc: Andrzej Hajda <[email protected]> > Cc: Matt Roper <[email protected]> > Cc: Jonathan Cavitt <[email protected]> > Cc: Prathap Kumar Valsan <[email protected]> > Cc: Alan Previn <[email protected]> > Cc: Madhumitha Tolakanahalli Pradeep > <[email protected]> > Cc: Daniele Ceraolo Spurio <[email protected]> > Cc: Ashutosh Dixit <[email protected]> > Cc: Dnyaneshwar Bhadane <[email protected]>
Looks good: Reviewed-by: Andi Shyti <[email protected]> Thanks, Andi
