Re: [Intel-gfx] [PATCH v7 2/2] drm/i915/guc: Close deregister-context race against CT-loss

["Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>]

On Thu, 2023-11-30 at 16:18 -0500, Vivi, Rodrigo wrote:
> On Wed, Nov 29, 2023 at 04:20:13PM -0800, Alan Previn wrote:
alan:snip
> > +
> >  	if (unlikely(disabled)) {
> >  		release_guc_id(guc, ce);
> >  		__guc_context_destroy(ce);
> > -		return;
> > +		return 0;
> 
> is success the right return case here?
alan: yes: we may discover "disabled == true" if submission_disabled
found that gt-is-wedged. I dont believe such a case will happen as
part of flushing destroyed_worker_func during suspend but may occur
as part of regular runtime context closing that just happens to
get queued in the middle of a gt-reset/wedging process. In such a case,
the reset-prepare code will sanitize everytihng including cleaning up
the pending-destructoin-contexts-link-list. So its either we pick it
from here and dump it ... or reset picks it first and dumps it there
(where both dumpings only occur if guc got disabled first).


Supplemental: How regular context cleanup leads to the same path -->

i915_sw_fence_notify -> engines_notify -> free_engines_rcu ->
intel_context_put -> kref_put(&ce->ref..) -> ce->ops->destroy ->

(where ce->ops = engine->cops and engine->cops = guc_context_ops)
*and, guc_context_ops->destroy == guc_context_destroy so ->

ce->ops->destroy -> guc_context_destroy ->
queue_work(..&guc->submission_state.destroyed_worker);
-> [eventually] -> the same guc_lrc_unpin above

However with additional "if (!intel_guc_is_ready(guc))" in destroyed_worker_func
as part of this patch, hitting this "disabled==true" case will be even less likely.
As far as i can tell, its only if we started resetting / wedging right after this
queued worker got started. (i ran out of time to check if reset can ever occur
from within the same system_unbound_wq but then i recall we also have CI using
debugfs to force wedging for select (/all?) igt tests) so i suspect it can still
happen in parallel. NOTE: the original checking of the "is disabled" is not new
code - its the original code.

...alan

P.S.- oh man, that took a lot of code tracing as i can't remember these paths by heart.