Re: [PATCH v4 2/4] drm/xe/guc: Report CGP protocol errors using SIGID

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

On 8/28/2026 11:58 AM, Arvind Yadav wrote:
> Route selected GuC CGP protocol error logs through the structured SIGID

what is CGP ?

> logging helper.
> 
> These paths were already reported as errors and return -EPROTO. Use the
> GUC component, which maps to XE_SIGID_RUNTIME_FW, and pass -EPROTO as the
> errno value.
> 
> Keep this conversion separate from the reset-request failure path because
> it reports GuC protocol/message validation errors, not GT_TDR recovery.
> 
> Cc: Mallesh Koujalagi <mallesh.koujalagi@intel.com>
> Cc: Badal Nilawar <badal.nilawar@intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Suggested-by: Mallesh Koujalagi <mallesh.koujalagi@intel.com>
> Signed-off-by: Arvind Yadav <arvind.yadav@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_guc_submit.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index c1d469624215..0bd84360f38b 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> @@ -3508,12 +3508,11 @@ int xe_guc_exec_queue_cgp_context_error_handler(struct xe_guc *guc, u32 *msg,
>  						u32 len)
>  {
>  	struct xe_gt *gt = guc_to_gt(guc);
> -	struct xe_device *xe = guc_to_xe(guc);
>  	struct xe_exec_queue *q;
>  	u32 guc_id = msg[2];
>  
>  	if (unlikely(len != XE_GUC_EXEC_QUEUE_CGP_CONTEXT_ERROR_LEN)) {

can you prepare other patch that will move that

	#define XE_GUC_EXEC_QUEUE_CGP_CONTEXT_ERROR_LEN

to some GuC ABI header in the abi/ folder?
it shouldn't be really defined here
> -		drm_err(&xe->drm, "Invalid length %u", len);
> +		xe_log_err(gt, GUC, -EPROTO, "Invalid CGP_CONTEXT_ERROR length %u\n", len);
>  		return -EPROTO;
>  	}
>  
> @@ -3546,13 +3545,13 @@ int xe_guc_exec_queue_cgp_context_error_handler(struct xe_guc *guc, u32 *msg,
>   */
>  int xe_guc_exec_queue_cgp_sync_done_handler(struct xe_guc *guc, u32 *msg, u32 len)
>  {
> -	struct xe_device *xe = guc_to_xe(guc);
> +	struct xe_gt *gt = guc_to_gt(guc);
>  	struct xe_exec_queue_group *group;
>  	struct xe_exec_queue *q, *upd_q;
>  	u32 guc_id = msg[0];
>  
>  	if (unlikely(len < 1)) {

and add define for this magic 1 

> -		drm_err(&xe->drm, "Invalid CGP_SYNC_DONE length %u", len);
> +		xe_log_err(gt, GUC, -EPROTO, "Invalid CGP_SYNC_DONE length %u\n", len);
>  		return -EPROTO;

and I guess we usually don't print any errors for malformed message
at the G2H handler sides, as there should be a full error message
generated by the CT layer after we return -EPROTO error

see xe_guc_error_capture_handler
see xe_guc_exec_queue_reset_handler
see xe_guc_exec_queue_memory_cat_error_handler
...

>  	}
>  
> @@ -3561,7 +3560,7 @@ int xe_guc_exec_queue_cgp_sync_done_handler(struct xe_guc *guc, u32 *msg, u32 le
>  		return -EPROTO;
>  
>  	if (!xe_exec_queue_is_multi_queue_primary(q)) {
> -		drm_err(&xe->drm, "Unexpected CGP_SYNC_DONE response");
> +		xe_log_err(gt, GUC, -EPROTO, "Unexpected CGP_SYNC_DONE response\n");

I'm wondering it this should be treated as EPROTO ?
what if it is our (xe) fault ?
maybe we should print some more details ?

>  		return -EPROTO;
>  	}
>