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

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

On 9/4/2026 1:40 AM, 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)
> v3: Remove 'GuC' string from logs and cleanup logs (Michal)

nit: move change log under ---

> 
> 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>

your s-o-b is needed

> ---
>  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..9684f6528641 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, "Initialization error\n");

nit: early initialization ? otherwise msg will be exactly the same as
in the next error, making it harder to investigate

>  	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, "Initialization error\n");
>  	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, "Reset timeout, GDRST=%#x\n", gdrst);
>  		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,
> +			   "MIA core not in reset, GUC_STATUS=%#x\n",

nit: this should still fit in the line above

> +			   guc_status);
>  		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, GUC_STATUS=%#x, time = %lldms, freq = %dMHz (req %dMHz)\n",
> +			   status, delta_ms, act_freq, cur_freq);

nit: freq are u32, shouldn't we use %u ?

>  		print_load_status_err(gt, status);
>  
>  		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, "Suspend aborted\n");
>  		return ret;
>  	}
>  

with nits and sob fixed,

	Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>