Re: [PATCH] drm/xe: Initialize GuC ID mgr before registering guc_submit_fini

[Matthew Brost <matthew.brost@intel.com>]

On Sat, Apr 06, 2024 at 04:46:51PM +0200, Michal Wajdeczko wrote:
> 
> 
> On 05.04.2024 23:27, Matthew Brost wrote:
> > It reasonable to assume that guc_submit_fini may rely on GuC ID mgr
> > being available. With that, register guc_submit_fini after initializing
> > the GuC ID mgr. This will ensure the GuC ID mgr is available in
> > guc_submit_fini due the reverse execution order of
> > drmm_add_action_or_reset.
> 
> that wont fly right now as GuC ID manager expects submission_state.lock
> mutex being still available during it's cleanup, but now it will be
> destroyed in __guc_submit_fini() called prior to __fini_idm()
> 
> please take a look at [1] which should solve all the problems
> 
> [1] https://patchwork.freedesktop.org/series/132118/
> 

Ah, yes good catch. Had CONFIG_PROVE_LOCKING=n during my local testing
and missed this. 

Anyways looked at [1] and gaves RBs there.

Thanks - Matt

> > 
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_guc_submit.c | 19 +++++++++++++------
> >  1 file changed, 13 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> > index 9c30bd9ac8c0..148c5dc2c1ac 100644
> > --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> > +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> > @@ -232,15 +232,20 @@ static struct workqueue_struct *get_submit_wq(struct xe_guc *guc)
> >  }
> >  #endif
> >  
> > -static void guc_submit_fini(struct drm_device *drm, void *arg)
> > +static void __guc_submit_fini(struct xe_guc *guc)
> >  {
> > -	struct xe_guc *guc = arg;
> > -
> >  	xa_destroy(&guc->submission_state.exec_queue_lookup);
> >  	free_submit_wq(guc);
> >  	mutex_destroy(&guc->submission_state.lock);
> >  }
> >  
> > +static void guc_submit_fini(struct drm_device *drm, void *arg)
> > +{
> > +	struct xe_guc *guc = arg;
> > +
> > +	__guc_submit_fini(guc);
> > +}
> > +
> >  static const struct xe_exec_queue_ops guc_exec_queue_ops;
> >  
> >  static void primelockdep(struct xe_guc *guc)
> > @@ -277,11 +282,13 @@ int xe_guc_submit_init(struct xe_guc *guc)
> >  
> >  	primelockdep(guc);
> >  
> > -	err = drmm_add_action_or_reset(&xe->drm, guc_submit_fini, guc);
> > -	if (err)
> > +	err = xe_guc_id_mgr_init(&guc->submission_state.idm, ~0);
> > +	if (err) {
> > +		__guc_submit_fini(guc);
> >  		return err;
> > +	}
> >  
> > -	err = xe_guc_id_mgr_init(&guc->submission_state.idm, ~0);
> > +	err = drmm_add_action_or_reset(&xe->drm, guc_submit_fini, guc);
> >  	if (err)
> >  		return err;
> >