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

[Dong, Zhanjun]

Please see my comments below.

Regards,
Zhanjun Dong

On 2025-06-23 10:28 a.m., Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of Zhanjun Dong
> Sent: Wednesday, June 18, 2025 4:33 PM
> To: intel-xe at lists.freedesktop.org
> Cc: Dong, Zhanjun <zhanjun.dong at intel.com>; Wajdeczko, Michal <Michal.Wajdeczko at intel.com>; Summers, Stuart <stuart.summers at intel.com>
> Subject: [PATCH v5] 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 at intel.com>
>> Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/4917
>>
>> ---
>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Cc: Stuart Summers <stuart.summers at intel.com>
>>
>> Change list:
>> 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 |  5 +++++
>>   drivers/gpu/drm/xe/xe_guc.h |  1 +
>>   drivers/gpu/drm/xe/xe_uc.c  | 23 ++++++++++++++++-------
>>   3 files changed, 22 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
>> index 209e5d53c290..9d7175b16cc7 100644
>> --- a/drivers/gpu/drm/xe/xe_guc.c
>> +++ b/drivers/gpu/drm/xe/xe_guc.c
>> @@ -1230,6 +1230,11 @@ int xe_guc_enable_communication(struct xe_guc *guc)
>>   	return 0;
>>   }
>>   
>> +void xe_guc_disable_communication(struct xe_guc *guc)
>> +{
>> +	xe_guc_ct_disable(&guc->ct);
>> +}
>> +
>>   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 58338be44558..285c19929f8c 100644
>> --- a/drivers/gpu/drm/xe/xe_guc.h
>> +++ b/drivers/gpu/drm/xe/xe_guc.h
>> @@ -33,6 +33,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_suspend(struct xe_guc *guc);
>>   void xe_guc_notify(struct xe_guc *guc);
>>   int xe_guc_auth_huc(struct xe_guc *guc, u32 rsa_addr);
>> diff --git a/drivers/gpu/drm/xe/xe_uc.c b/drivers/gpu/drm/xe/xe_uc.c
>> index 3a8751a8b92d..35cf67b2c2f8 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"
>> @@ -157,19 +158,23 @@ static int vf_uc_init_hw(struct xe_uc *uc)
>>   
>>   	err = xe_guc_enable_communication(&uc->guc);
>>   	if (err)
>> -		return err;
>> +		goto err_out;
> 
> I feel like if xe_guc_enable_communication fails, we shouldn't need to
> disable it afterwards, as it should already not be enabled.  Specifically,
> xe_guc_enable_communication fails only when xe_guc_ct_enable fails,
> which would indicate that it wasn't enabled.
> 
> Well... okay, xe_guc_enable_communication can also return an error
> value if xe_memirq_init_guc fails, but this is only the case for systems
> where IS_SRIOV_VF and xe_device_has_memirq return true, which
> isn't a significant subset of systems.  And besides that, if
> xe_memirq_init_guc fails, then we don't even run xe_guc_ct_enable.
> 
> So, either way, the GuC CT shouldn't have been enabled if
> xe_guc_enable_communication failed.  Ergo, we shouldn't need to
> call xe_guc_ct_disable to disable a GuC CT that failed to be enabled.
#1 Yes, either way, if xe_guc_enable_communication returns non zero, ct 
is not enabled yet, so disable is not neccessary here. Will fix it in 
next rev.>
> Though maybe we need to very specifically xe_guc_ct_set_state here
> to declare the CT as disabled to GuC so it doesn't try to access it?  I can
> see that being the case.
If xe_guc_enable_communication returns 0, it means ct is enabled, so 
with the above #1 fix, seems no need to check.>
> If this needs to be fixed, then fix it first, but otherwise:
> Reviewed-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
> -Jonathan Cavitt
Thanks for review. :-)
New rev will be post shortly.

> 
>>   
>>   	err = xe_gt_sriov_vf_connect(uc_to_gt(uc));
>>   	if (err)
>> -		return err;
>> +		goto err_out;
>>   
>>   	uc->guc.submission_state.enabled = true;
>>   
>>   	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;
>>   }
>>   
>>   /*
>> @@ -197,19 +202,19 @@ int xe_uc_init_hw(struct xe_uc *uc)
>>   
>>   	ret = xe_guc_enable_communication(&uc->guc);
>>   	if (ret)
>> -		return ret;
>> +		goto err_out;
>>   
>>   	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);
>>   
>> @@ -221,6 +226,10 @@ int xe_uc_init_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_fini_hw(struct xe_uc *uc)
>> -- 
>> 2.34.1
>>
>>