Re: [PATCH v9] drm/xe/uc: Disable GuC communication on hardware initialization error.

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

On Tue, Jul 08, 2025 at 11:49:47AM -0700, Matthew Brost wrote:
> On Tue, Jul 08, 2025 at 12:36:43PM -0600, Cavitt, Jonathan wrote:
> > -----Original Message-----
> > From: Dong, Zhanjun <zhanjun.dong@intel.com> 
> > Sent: Monday, July 7, 2025 4:11 PM
> > To: intel-xe@lists.freedesktop.org
> > Cc: Dong, Zhanjun <zhanjun.dong@intel.com>; Wajdeczko, Michal <Michal.Wajdeczko@intel.com>; Summers, Stuart <stuart.summers@intel.com>; Cavitt, Jonathan <jonathan.cavitt@intel.com>; Brost, Matthew <matthew.brost@intel.com>
> > Subject: [PATCH v9] drm/xe/uc: Disable GuC communication on hardware initialization error.
> > > 
> > > Disable GuC communication on Xe micro controller hardware initialization
> > > error.
> > > 
> > > Signed-off-by: Zhanjun Dong <zhanjun.dong@intel.com>
> > > Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/4917
> > 
> > I think you need to swap the order of the Closes and Signed-off-by,
> > but that can be done at merge-time if it's necessary, so there's no
> > need to create another revision for this change.
> > 
> > You might also need to remove the period from the commit name,
> > but that too can be a merge-time revision, so again, no need for
> > another revision.
> > 
> > Otherwise:
> > Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> > -Jonathan Cavitt
> > 
> 
> Also the change log is typically above the tags, since it is pretty
> verbose for a simple patch, maybe just drop the change log.
> 

Ignore the above comment, I was just aware of [1].

[1] https://docs.kernel.org/process/submitting-patches.html#commentary

Matt

> Anyways:
> 
> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> 
> > > 
> > > ---
> > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > > Cc: Stuart Summers <stuart.summers@intel.com>
> > > Cc: Jonathan Cavitt <jonathan.cavitt@intel.com>
> > > Cc: Matthew Brost <matthew.brost@intel.com>
> > > 
> > > Change list:
> > > v9: Switched to xe_guc_sanitize
> > > v8: Fix kernel-doc style
> > >     Add error handling in vf_guc_min_load_for_hwconfig
> > > v7: Add kernel-doc for xe_guc_disable_communication
> > >     Unset submission_state.enabled as well
> > > v6: Skip disable ct on xe_guc_enable_communication error
> > > v5: Set wedge is excessive action, revert back to disable ct
> > > v4: Fix typo and add new line
> > > v3: v2 CI re-run
> > > v2: Remove unnecessary jump to err-out
> > >     Drop disable ct, switch to set wedge
> > > ---
> > >  drivers/gpu/drm/xe/xe_guc.c |  8 ++++++--
> > >  drivers/gpu/drm/xe/xe_uc.c  | 18 +++++++++++++-----
> > >  2 files changed, 19 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
> > > index 8573957facae..b1d1d6da3758 100644
> > > --- a/drivers/gpu/drm/xe/xe_guc.c
> > > +++ b/drivers/gpu/drm/xe/xe_guc.c
> > > @@ -1219,13 +1219,17 @@ static int vf_guc_min_load_for_hwconfig(struct xe_guc *guc)
> > >  
> > >  	ret = xe_gt_sriov_vf_connect(gt);
> > >  	if (ret)
> > > -		return ret;
> > > +		goto err_out;
> > >  
> > >  	ret = xe_gt_sriov_vf_query_runtime(gt);
> > >  	if (ret)
> > > -		return ret;
> > > +		goto err_out;
> > >  
> > >  	return 0;
> > > +
> > > +err_out:
> > > +	xe_guc_sanitize(guc);
> > > +	return ret;
> > >  }
> > >  
> > >  /**
> > > diff --git a/drivers/gpu/drm/xe/xe_uc.c b/drivers/gpu/drm/xe/xe_uc.c
> > > index 6431ba3a2c53..3e0c3af235f2 100644
> > > --- a/drivers/gpu/drm/xe/xe_uc.c
> > > +++ b/drivers/gpu/drm/xe/xe_uc.c
> > > @@ -158,7 +158,7 @@ static int vf_uc_load_hw(struct xe_uc *uc)
> > >  
> > >  	err = xe_gt_sriov_vf_connect(uc_to_gt(uc));
> > >  	if (err)
> > > -		return err;
> > > +		goto err_out;
> > >  
> > >  	uc->guc.submission_state.enabled = true;
> > >  
> > > @@ -168,9 +168,13 @@ static int vf_uc_load_hw(struct xe_uc *uc)
> > >  
> > >  	err = xe_gt_record_default_lrcs(uc_to_gt(uc));
> > >  	if (err)
> > > -		return err;
> > > +		goto err_out;
> > >  
> > >  	return 0;
> > > +
> > > +err_out:
> > > +	xe_guc_sanitize(&uc->guc);
> > > +	return err;
> > >  }
> > >  
> > >  /*
> > > @@ -202,15 +206,15 @@ int xe_uc_load_hw(struct xe_uc *uc)
> > >  
> > >  	ret = xe_gt_record_default_lrcs(uc_to_gt(uc));
> > >  	if (ret)
> > > -		return ret;
> > > +		goto err_out;
> > >  
> > >  	ret = xe_guc_post_load_init(&uc->guc);
> > >  	if (ret)
> > > -		return ret;
> > > +		goto err_out;
> > >  
> > >  	ret = xe_guc_pc_start(&uc->guc.pc);
> > >  	if (ret)
> > > -		return ret;
> > > +		goto err_out;
> > >  
> > >  	xe_guc_engine_activity_enable_stats(&uc->guc);
> > >  
> > > @@ -222,6 +226,10 @@ int xe_uc_load_hw(struct xe_uc *uc)
> > >  	xe_gsc_load_start(&uc->gsc);
> > >  
> > >  	return 0;
> > > +
> > > +err_out:
> > > +	xe_guc_sanitize(&uc->guc);
> > > +	return ret;
> > >  }
> > >  
> > >  int xe_uc_reset_prepare(struct xe_uc *uc)
> > > -- 
> > > 2.34.1
> > > 
> > >