Re: [PATCH v2 4/5] drm/xe/guc: Report major GuC failures using SIGID

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

On 9/1/2026 11:12 PM, Umesh Nerlige Ramappa wrote:
> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> 
> Convert errors during critical GuC flows (init, load, reset, suspend)
> to use the xe_log_err() helper.
> While at it, simplify the error log for the GuC load failure to use the
> cached frequency values instead of fetching them again.
> 
> v2: split guc_load_done changes to their own patch, use available ret
> value for reset errors (Michal)
> 
> 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>
> Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_guc.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
> index 5285d4cecbc8..d0ddaaf35274 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");

note that there will be already "GUC: " prefix in the message,
so maybe just:

	"Initialization error\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");

ditto

>  	return ret;
>  }
>  
> @@ -998,15 +998,16 @@ 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, ret, "GuC reset timed out, GDRST=%#x\n", gdrst);

note that there will be already "GUC: " prefix in the message,
so maybe just:

	"Reset timeout, GDRST=%#x\n"

>  		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);
>  		ret = -EIO;
> +		xe_log_err(gt, GUC, ret,
> +			   "GuC status: %#x, MIA core expected to be in reset\n",
> +			   guc_status);

note that there will be already "GUC: " prefix in the message,
so maybe just:

	"MIA core not in reset, GUC_STATUS=%#x\n"

>  		goto err_out;
>  	}
>  
> @@ -1224,9 +1225,9 @@ static int guc_wait_ucode(struct xe_guc *guc)
>  	cur_freq = xe_guc_pc_get_cur_freq_fw(guc_pc);
>  
>  	if (ret || load_result) {
> -		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",

as "status" is a register dump, maybe use the same naming as before:

	"Load failed, GUC_STATUS=%#x

> +			   status, delta_ms, act_freq, cur_freq);
>  		print_load_status_err(gt, status);

do we want to use xe_log_info() here ^^^ to get a better view?
then maybe above xe_log() could be split and keep only GUC_STATUS
and move rest (freq) to other logs?

>  
>  		return ret ?: load_result;
> @@ -1463,7 +1464,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");

note that there will be already "GUC: " prefix in the message,
so maybe just:

	"Suspend aborted\n"

>  		return ret;
>  	}
>