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

["Yadav, Arvind" <arvind.yadav@intel.com>]

On 28-08-2026 16:07, Michal Wajdeczko wrote:
>
> On 8/28/2026 11:58 AM, Arvind Yadav wrote:
>> Route selected GuC CGP protocol error logs through the structured SIGID
> what is CGP ?


CGP is Context Group Page. It is a KMD-managed page used to pass 
multi-queue group updates to GuC.

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


I will avoid adding SIGID logs in these handlers because malformed G2H 
messages are already reported by the CT layer after the handler returns 
`-EPROTO`.
I will drop the CGP SIGID conversion patch. As per you suggestion we can 
send separate a small cleanup patch.

Thanks,
Arvind

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