Re: [PATCH 2/3] drm/xe/guc: Report major GuC failures using SIGID

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

On 8/27/2026 2:28 AM, Daniele Ceraolo Spurio wrote:
> Convert errors during critical GuC flows (init, load, reset, suspend)
> to use the xe_log_err() helper.
> For the GuC load failure scenario, the possible failure modes have been
> bucketed into 3 different possible error codes to distinguish a bad
> binary from a driver error or unexpected HW behavior.
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Aravind Iddamsetty <aravind.iddamsetty@intel.com>
> Cc: Mallesh Koujalagi <mallesh.koujalagi@intel.com>
> Cc: Alan Previn Teres Alexis <alan.previn.teres.alexis@intel.com>
> Cc: Julia Filipchuk <julia.filipchuk@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_guc.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
> index c7f8bbd4cb92..484cb47fbc81 100644
> --- a/drivers/gpu/drm/xe/xe_guc.c
> +++ b/drivers/gpu/drm/xe/xe_guc.c
> @@ -777,7 +777,7 @@ int xe_guc_init_noalloc(struct xe_guc *guc)
>  	return 0;
>  
>  out:
> -	xe_gt_err(gt, "GuC init failed with %pe\n", ERR_PTR(ret));
> +	xe_log_err(gt, GUC, ret, "GuC init failed\n");
>  	return ret;
>  }
>  
> @@ -845,7 +845,7 @@ int xe_guc_init(struct xe_guc *guc)
>  	return 0;
>  
>  out:
> -	xe_gt_err(gt, "GuC init failed with %pe\n", ERR_PTR(ret));
> +	xe_log_err(gt, GUC, ret, "GuC init failed\n");
>  	return ret;
>  }
>  
> @@ -998,14 +998,15 @@ int xe_guc_reset(struct xe_guc *guc)
>  
>  	ret = xe_mmio_wait32(mmio, GDRST, GRDOM_GUC, 0, 5000, &gdrst, false);
>  	if (ret) {
> -		xe_gt_err(gt, "GuC reset timed out, GDRST=%#x\n", gdrst);
> +		xe_log_err(gt, GUC, -EIO, "GuC reset timed out, GDRST=%#x\n", gdrst);

IIUC we want to pass error codes as-is (if available), not to
replace them with some ad-hoc different errno

>  		goto err_out;
>  	}
>  
>  	guc_status = xe_mmio_read32(mmio, GUC_STATUS);
>  	if (!(guc_status & GS_MIA_IN_RESET)) {
> -		xe_gt_err(gt, "GuC status: %#x, MIA core expected to be in reset\n",
> -			  guc_status);
> +		xe_log_err(gt, GUC, -EIO,
> +			   "GuC status: %#x, MIA core expected to be in reset\n",
> +			   guc_status);

nit: we could try to pass 'ret'
>  		ret = -EIO;
>  		goto err_out;
>  	}
> @@ -1163,14 +1164,16 @@ static int guc_load_done(struct xe_gt *gt, u32 *status, u32 *tries)

better to move changes to load_done() into a separate patch
also update load_done() function comment which still mention only -1/1

and maybe to follow general api convention we should use 0
to indicate success and -EBUSY for in-progress ?

and then

	ret = poll_timeout_us(load_result = guc_load_done(gt, &status, &tries),
				load_result != -EBUSY, ...
>  	case XE_GUC_LOAD_STATUS_ERROR_DEVID_INVALID_GUCTYPE:
>  	case XE_GUC_LOAD_STATUS_HWCONFIG_ERROR:
>  	case XE_GUC_LOAD_STATUS_BOOTROM_VERSION_MISMATCH:
> +		return -ENOEXEC;
>  	case XE_GUC_LOAD_STATUS_DPC_ERROR:
>  	case XE_GUC_LOAD_STATUS_EXCEPTION:
> +		return -EIO;
>  	case XE_GUC_LOAD_STATUS_INIT_DATA_INVALID:
>  	case XE_GUC_LOAD_STATUS_MPU_DATA_INVALID:
>  	case XE_GUC_LOAD_STATUS_INIT_MMIO_SAVE_RESTORE_INVALID:
>  	case XE_GUC_LOAD_STATUS_KLV_WORKAROUND_INIT_ERROR:
>  	case XE_GUC_LOAD_STATUS_INVALID_FTR_FLAG:
> -		return -1;
> +		return -EINVAL;
>  	}
>  
>  	switch (bootrom) {
> @@ -1184,7 +1187,7 @@ static int guc_load_done(struct xe_gt *gt, u32 *status, u32 *tries)
>  	case XE_BOOTROM_STATUS_MPUMAP_INCORRECT:
>  	case XE_BOOTROM_STATUS_EXCEPTION:
>  	case XE_BOOTROM_STATUS_PROD_KEY_CHECK_FAILURE:
> -		return -1;
> +		return -ENOEXEC;
>  	}
>  
>  	if (++*tries >= 100) {
> @@ -1222,9 +1225,10 @@ static int guc_wait_ucode(struct xe_guc *guc)
>  	cur_freq = xe_guc_pc_get_cur_freq_fw(guc_pc);
>  
>  	if (ret || load_result <= 0) {
> -		xe_gt_err(gt, "load failed: status = 0x%08X, time = %lldms, freq = %dMHz (req %dMHz)\n",
> -			  status, delta_ms, xe_guc_pc_get_act_freq(guc_pc),
> -			  xe_guc_pc_get_cur_freq_fw(guc_pc));
> +		xe_log_err(gt, GUC, ret ?: load_result,
> +			   "load failed: status = 0x%08X, time = %lldms, freq = %dMHz (req %dMHz)\n",
> +			   status, delta_ms, xe_guc_pc_get_act_freq(guc_pc),
> +			   xe_guc_pc_get_cur_freq_fw(guc_pc));

we can use 'act_freq' and 'cur_freq'

>  		print_load_status_err(gt, status);
>  
>  		return -EPROTO;

hmm, -EPROTO was likely already incorrect, but now we should

		return ret ?: load_result;

> @@ -1461,7 +1465,7 @@ int xe_guc_suspend(struct xe_guc *guc)
>  
>  	ret = xe_guc_softreset(guc);
>  	if (ret) {
> -		xe_gt_err(gt, "GuC suspend failed: %pe\n", ERR_PTR(ret));
> +		xe_log_err(gt, GUC, ret, "GuC suspend failed\n");
>  		return ret;
>  	}
>