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

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 07.07.2025 15:56, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Dong, Zhanjun <zhanjun.dong@intel.com> 
> Sent: Thursday, July 3, 2025 2:39 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>
> Subject: [PATCH v8] 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
>>
>> ---
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Stuart Summers <stuart.summers@intel.com>
>> Cc: Jonathan Cavitt <jonathan.cavitt@intel.com>
>>
>> Change list:
>> 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 | 20 ++++++++++++++++++--
>>  drivers/gpu/drm/xe/xe_guc.h |  1 +
>>  drivers/gpu/drm/xe/xe_uc.c  | 19 ++++++++++++++-----
>>  3 files changed, 33 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
>> index 8573957facae..6643a2cb898b 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_disable_communication(guc);
>> +	return ret;
>>  }
>>  
>>  /**
>> @@ -1337,6 +1341,18 @@ int xe_guc_enable_communication(struct xe_guc *guc)
>>  	return 0;
>>  }
>>  
>> +/**
>> + * xe_guc_disable_communication() - Disable GuC communication
>> + * @guc: The GuC object
>> + *
>> + * This function will disable the GuC communication.
>> + */
>> +void xe_guc_disable_communication(struct xe_guc *guc)
>> +{
>> +	guc->submission_state.enabled = false;
>> +	xe_guc_ct_disable(&guc->ct);
> 
> Didn't this used to just be a wrapper for xe_guc_ct_disable?
> 
> ... Well, I suppose needing to set the submission state enabled to false
> would also be a rather important step, so it's probably good that was added.

hmm, while it might be a required step for shutdown, putting it here
looks like a random choice, it's unbalanced what "enable_communication"
did and also looks like a violation of the layering, as guc_submit code
has its own files and we shouldn't touch internals from the outside

+ @Matt

> 
> Everything else is LGTM, so
> Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> -Jonathan Cavitt
> 
>> +}
>> +
>>  int xe_guc_suspend(struct xe_guc *guc)
>>  {
>>  	struct xe_gt *gt = guc_to_gt(guc);
>> diff --git a/drivers/gpu/drm/xe/xe_guc.h b/drivers/gpu/drm/xe/xe_guc.h
>> index 22cf019a11bf..20823b821f7d 100644
>> --- a/drivers/gpu/drm/xe/xe_guc.h
>> +++ b/drivers/gpu/drm/xe/xe_guc.h
>> @@ -34,6 +34,7 @@ int xe_guc_reset(struct xe_guc *guc);
>>  int xe_guc_upload(struct xe_guc *guc);
>>  int xe_guc_min_load_for_hwconfig(struct xe_guc *guc);
>>  int xe_guc_enable_communication(struct xe_guc *guc);
>> +void xe_guc_disable_communication(struct xe_guc *guc);
>>  int xe_guc_opt_in_features_enable(struct xe_guc *guc);
>>  int xe_guc_suspend(struct xe_guc *guc);
>>  void xe_guc_notify(struct xe_guc *guc);
>> diff --git a/drivers/gpu/drm/xe/xe_uc.c b/drivers/gpu/drm/xe/xe_uc.c
>> index 6431ba3a2c53..1012fe84b379 100644
>> --- a/drivers/gpu/drm/xe/xe_uc.c
>> +++ b/drivers/gpu/drm/xe/xe_uc.c
>> @@ -13,6 +13,7 @@
>>  #include "xe_gt_printk.h"
>>  #include "xe_gt_sriov_vf.h"
>>  #include "xe_guc.h"
>> +#include "xe_guc_ct.h"
>>  #include "xe_guc_pc.h"
>>  #include "xe_guc_engine_activity.h"
>>  #include "xe_huc.h"
>> @@ -158,7 +159,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 +169,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_disable_communication(&uc->guc);
>> +	return err;
>>  }
>>  
>>  /*
>> @@ -202,15 +207,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 +227,10 @@ int xe_uc_load_hw(struct xe_uc *uc)
>>  	xe_gsc_load_start(&uc->gsc);
>>  
>>  	return 0;
>> +
>> +err_out:
>> +	xe_guc_disable_communication(&uc->guc);
>> +	return ret;
>>  }
>>  
>>  int xe_uc_reset_prepare(struct xe_uc *uc)
>> -- 
>> 2.34.1
>>
>>