Re: [PATCH 3/3] drm/xe/guc: Report errors that cause a CT shutdown using SIGID

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

On 8/27/2026 7:34 AM, Michal Wajdeczko wrote:
>
> On 8/27/2026 2:28 AM, Daniele Ceraolo Spurio wrote:
>> Convert any errors that can cause the CT to be declared as dead to
>> use the xe_log_err() helper. Errors that are escalated to the callers
>> are left for the caller to report with SIGID if needed.
>>
>> 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_ct.c | 91 +++++++++++++++++++---------------
>>   1 file changed, 51 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
>> index 5c4733da385c..4efaf2d24c3e 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_ct.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
>> @@ -30,6 +30,7 @@
>>   #include "xe_guc_relay.h"
>>   #include "xe_guc_submit.h"
>>   #include "xe_guc_tlb_inval.h"
>> +#include "xe_log.h"
>>   #include "xe_map.h"
>>   #include "xe_page_reclaim.h"
>>   #include "xe_pm.h"
>> @@ -679,7 +680,7 @@ static int __xe_guc_ct_start(struct xe_guc_ct *ct, bool needs_register)
>>   	return 0;
>>   
>>   err_out:
>> -	xe_gt_err(gt, "Failed to enable GuC CT (%pe)\n", ERR_PTR(err));
>> +	xe_log_err(gt, GUC, err, "Failed to enable CT\n");
>>   	CT_DEAD(ct, NULL, SETUP);
>>   
>>   	return err;
>> @@ -803,8 +804,9 @@ static bool h2g_has_room(struct xe_guc_ct *ct, u32 cmd_len)
>>   
>>   			desc_write(xe, h2g, status, desc_status | GUC_CTB_STATUS_OVERFLOW);
>>   
>> -			xe_gt_err(ct_to_gt(ct), "CT: invalid head offset %u >= %u)\n",
>> -				  h2g->info.head, h2g->info.size);
>> +			xe_log_err(ct_to_gt(ct), GUC, -EPROTO,
>> +				   "CT: invalid head offset %u >= %u)\n",
>> +				   h2g->info.head, h2g->info.size);
> nit: we usually use -EPROTO to report mismatch in the messages
> while here we have corrupted descriptor, so maybe we can use
> something else, like:
>
> #define	ENFILE		23	/* File table overflow */
> #define	ESPIPE		29	/* Illegal seek */
> #define	EPIPE		32	/* Broken pipe */
> #define	EILSEQ		84	/* Illegal byte sequence */
> #define	EUCLEAN		117	/* Structure needs cleaning */
>
>>   			CT_DEAD(ct, h2g, H2G_HAS_ROOM);
>>   			return false;
>>   		}
>> @@ -873,12 +875,13 @@ static void __g2h_release_space(struct xe_guc_ct *ct, u32 g2h_len)
>>   	bad |= !ct->g2h_outstanding;
>>   
>>   	if (bad) {
>> -		xe_gt_err(ct_to_gt(ct), "Invalid G2H release: %d + %d vs %d - %d -> %d vs %d, outstanding = %d!\n",
>> -			  ct->ctbs.g2h.info.space, g2h_len,
>> -			  ct->ctbs.g2h.info.size, ct->ctbs.g2h.info.resv_space,
>> -			  ct->ctbs.g2h.info.space + g2h_len,
>> -			  ct->ctbs.g2h.info.size - ct->ctbs.g2h.info.resv_space,
>> -			  ct->g2h_outstanding);
>> +		xe_log_err(ct_to_gt(ct), GUC, -EPROTO,
> hmm, here the "bad" flag is more an indication of our (xe) miscalculation,
> not something that FW did wrong, so -EPROTO seems wrong, maybe
>
> #define	ETOOMANYREFS	109	/* Too many references: cannot splice */
>> +			   "Invalid G2H release: %d + %d vs %d - %d -> %d vs %d, outstanding = %d!\n",
>> +			   ct->ctbs.g2h.info.space, g2h_len,
>> +			   ct->ctbs.g2h.info.size, ct->ctbs.g2h.info.resv_space,
>> +			   ct->ctbs.g2h.info.space + g2h_len,
>> +			   ct->ctbs.g2h.info.size - ct->ctbs.g2h.info.resv_space,
>> +			   ct->g2h_outstanding);
>>   		CT_DEAD(ct, &ct->ctbs.g2h, G2H_RELEASE);
>>   		return;
>>   	}
>> @@ -961,21 +964,24 @@ static int h2g_write(struct xe_guc_ct *ct, const u32 *action, u32 len,
>>   
>>   		desc_status = desc_read(xe, h2g, status);
>>   		if (desc_status) {
>> -			xe_gt_err(gt, "CT write: non-zero status: %u\n", desc_status);
>> +			xe_log_err(gt, GUC, -EPROTO,
>> +				   "CT write: non-zero status: %u\n", desc_status);
>>   			goto corrupted;
>>   		}
>>   
>>   		if (tail > h2g->info.size) {
>>   			desc_write(xe, h2g, status, desc_status | GUC_CTB_STATUS_OVERFLOW);
>> -			xe_gt_err(gt, "CT write: tail out of range: %u vs %u\n",
>> -				  tail, h2g->info.size);
>> +			xe_log_err(gt, GUC, -EPROTO,
>> +				   "CT write: tail out of range: %u vs %u\n",
>> +				   tail, h2g->info.size);
>>   			goto corrupted;
>>   		}
>>   
>>   		if (desc_head >= h2g->info.size) {
>>   			desc_write(xe, h2g, status, desc_status | GUC_CTB_STATUS_OVERFLOW);
>> -			xe_gt_err(gt, "CT write: invalid head offset %u >= %u)\n",
>> -				  desc_head, h2g->info.size);
>> +			xe_log_err(gt, GUC, -EPROTO,
> as this indicates that FW found an error in CTB, maybe:
>
> #define	EPIPE		32	/* Broken pipe */
>
>> +				   "CT write: invalid head offset %u >= %u)\n",
>> +				   desc_head, h2g->info.size);
>>   			goto corrupted;
>>   		}
>>   	}
>> @@ -1220,7 +1226,7 @@ static int guc_ct_send_locked(struct xe_guc_ct *ct, const u32 *action, u32 len,
>>   	return ret;
>>   
>>   broken:
>> -	xe_gt_err(gt, "No forward process on H2G, reset required\n");
>> +	xe_log_err(gt, GUC, -EDEADLK, "No forward process on H2G, reset required\n");
>>   	CT_DEAD(ct, &ct->ctbs.h2g, DEADLOCK);
>>   
>>   	return -EDEADLK;
>> @@ -1558,11 +1564,11 @@ static int guc_crash_process_msg(struct xe_guc_ct *ct, u32 action)
>>   	struct xe_gt *gt = ct_to_gt(ct);
>>   
>>   	if (action == XE_GUC_ACTION_NOTIFY_CRASH_DUMP_POSTED)
>> -		xe_gt_err(gt, "GuC Crash dump notification\n");
>> +		xe_log_err(gt, GUC, -EPROTO, "GuC Crash dump notification\n");
>>   	else if (action == XE_GUC_ACTION_NOTIFY_EXCEPTION)
>> -		xe_gt_err(gt, "GuC Exception notification\n");
>> +		xe_log_err(gt, GUC, -EPROTO, "GuC Exception notification\n");
>>   	else
>> -		xe_gt_err(gt, "Unknown GuC crash notification: 0x%04X\n", action);
>> +		xe_log_err(gt, GUC, -EPROTO, "Unknown GuC crash notification: 0x%04X\n", action);
> maybe crashes should be identified as one of:
>
> #define	ENETDOWN	100	/* Network is down */
> #define	ENETUNREACH	101	/* Network is unreachable */
> #define	EHOSTDOWN	112	/* Host is down */
>
>>   
>>   	CT_DEAD(ct, NULL, CRASH);
>>   
>> @@ -1592,13 +1598,15 @@ static int parse_g2h_response(struct xe_guc_ct *ct, u32 *msg, u32 len)
>>   	 */
>>   	if (fence & CT_SEQNO_UNTRACKED) {
>>   		if (type == GUC_HXG_TYPE_RESPONSE_FAILURE)
>> -			xe_gt_err(gt, "FAST_REQ H2G fence 0x%x failed! e=0x%x, h=%u\n",
>> -				  fence,
>> -				  FIELD_GET(GUC_HXG_FAILURE_MSG_0_ERROR, hxg[0]),
>> -				  FIELD_GET(GUC_HXG_FAILURE_MSG_0_HINT, hxg[0]));
>> +			xe_log_err(gt, GUC, -EPROTO,
> FAILURE response is a valid message, maybe:
>
> #define	EBADE		52	/* Invalid exchange */
>
>> +				   "FAST_REQ H2G fence 0x%x failed! e=0x%x, h=%u\n",
>> +				   fence,
>> +				   FIELD_GET(GUC_HXG_FAILURE_MSG_0_ERROR, hxg[0]),
>> +				   FIELD_GET(GUC_HXG_FAILURE_MSG_0_HINT, hxg[0]));
>>   		else
>> -			xe_gt_err(gt, "unexpected response %u for FAST_REQ H2G fence 0x%x!\n",
>> -				  type, fence);
>> +			xe_log_err(gt, GUC, -EPROTO,
>> +				   "unexpected response %u for FAST_REQ H2G fence 0x%x!\n",
>> +				    type, fence);
>>   
>>   		fast_req_report(ct, fence);
>>   
>> @@ -1674,8 +1682,9 @@ static int parse_g2h_msg(struct xe_guc_ct *ct, u32 *msg, u32 len)
>>   
>>   	origin = FIELD_GET(GUC_HXG_MSG_0_ORIGIN, hxg[0]);
>>   	if (unlikely(origin != GUC_HXG_ORIGIN_GUC)) {
>> -		xe_gt_err(gt, "G2H channel broken on read, origin=%u, reset required\n",
>> -			  origin);
>> +		xe_log_err(gt, GUC, -EPROTO,
>> +			   "G2H channel broken on read, origin=%u, reset required\n",
> #define	EBADMSG		74	/* Not a data message */

For this one I can switch to EBADMSG  for the log, but the return value 
needs to stick to EPROTO because the value is returned all the way back 
to receive_g2h, which checks specifically for EPROTO or EOPNOTSUPP. 
Changing this flow to handle different error codes is out of scope of 
this series IMO.

>
>> +			   origin);
>>   		CT_DEAD(ct, &ct->ctbs.g2h, PARSE_G2H_ORIGIN);
>>   
>>   		return -EPROTO;
>> @@ -1693,8 +1702,9 @@ static int parse_g2h_msg(struct xe_guc_ct *ct, u32 *msg, u32 len)
>>   		ret = parse_g2h_response(ct, msg, len);
>>   		break;
>>   	default:
>> -		xe_gt_err(gt, "G2H channel broken on read, type=%u, reset required\n",
>> -			  type);
>> +		xe_log_err(gt, GUC, -EOPNOTSUPP,
>> +			   "G2H channel broken on read, type=%u, reset required\n",
> maybe this should say: "Unexpected message type %u" ?
> and since we rather do not expect new message types in CTBv1 then
> maybe this one should be actually -EPROTO ?

I think it's better to stick with EOPNOTSUPP, but I can reword the message.

>
>> +			   type);
>>   		CT_DEAD(ct, &ct->ctbs.g2h, PARSE_G2H_TYPE);
>>   
>>   		ret = -EOPNOTSUPP;
>> @@ -1792,8 +1802,8 @@ static int process_g2h_msg(struct xe_guc_ct *ct, u32 *msg, u32 len)
>>   	}
>>   
>>   	if (ret) {
>> -		xe_gt_err(gt, "G2H action %#04x failed (%pe) len %u msg %*ph\n",
>> -			  action, ERR_PTR(ret), hxg_len, (int)sizeof(u32) * hxg_len, hxg);
>> +		xe_log_err(gt, GUC, ret, "G2H action %#04x failed (%pe) len %u msg %*ph\n",
>> +			   action, ERR_PTR(ret), hxg_len, (int)sizeof(u32) * hxg_len, hxg);
> drop %pe as it will be already printed
>
>>   		CT_DEAD(ct, NULL, PROCESS_FAILED);
>>   	}
>>   
>> @@ -1840,7 +1850,7 @@ static int g2h_read(struct xe_guc_ct *ct, u32 *msg, bool fast_path)
>>   		}
>>   
>>   		if (desc_status) {
>> -			xe_gt_err(gt, "CT read: non-zero status: %u\n", desc_status);
>> +			xe_log_err(gt, GUC, -EIO, "CT read: non-zero status: %u\n", desc_status);
> #define	EPIPE		32	/* Broken pipe */
>
>>   			goto corrupted;
>>   		}
>>   	}
>> @@ -1871,15 +1881,15 @@ static int g2h_read(struct xe_guc_ct *ct, u32 *msg, bool fast_path)
>>   
>>   		if (g2h->info.head > g2h->info.size) {
>>   			desc_write(xe, g2h, status, desc_status | GUC_CTB_STATUS_OVERFLOW);
>> -			xe_gt_err(gt, "CT read: head out of range: %u vs %u\n",
>> -				  g2h->info.head, g2h->info.size);
>> +			xe_log_err(gt, GUC, -EIO, "CT read: head out of range: %u vs %u\n",
>> +				   g2h->info.head, g2h->info.size);
> as before, one of:
>
> #define	ENFILE		23	/* File table overflow */
> #define	ESPIPE		29	/* Illegal seek */
> #define	EPIPE		32	/* Broken pipe */
> #define	EILSEQ		84	/* Illegal byte sequence */
> #define	EUCLEAN		117	/* Structure needs cleaning */
>
> maybe except EPIPE which we want to use to indicate that CTB error
> was already set earlier (likely by the GuC FW)

IMO grouping all cases where the CTB header is in a bad state (whether 
because the GuC signaled an error or because it wrote and invalid value) 
under EPIPE is cleaner. Having too many different error codes will just 
get confusing.

>
>>   			goto corrupted;
>>   		}
>>   
>>   		if (desc_tail >= g2h->info.size) {
>>   			desc_write(xe, g2h, status, desc_status | GUC_CTB_STATUS_OVERFLOW);
>> -			xe_gt_err(gt, "CT read: invalid tail offset %u >= %u)\n",
>> -				  desc_tail, g2h->info.size);
>> +			xe_log_err(gt, GUC, -EIO, "CT read: invalid tail offset %u >= %u)\n",
>> +				   desc_tail, g2h->info.size);
> ditto
>
>>   			goto corrupted;
>>   		}
>>   	}
>> @@ -1898,8 +1908,9 @@ static int g2h_read(struct xe_guc_ct *ct, u32 *msg, bool fast_path)
>>   			   sizeof(u32));
>>   	len = FIELD_GET(GUC_CTB_MSG_0_NUM_DWORDS, msg[0]) + GUC_CTB_MSG_MIN_LEN;
>>   	if (len > avail) {
>> -		xe_gt_err(gt, "G2H channel broken on read, avail=%d, len=%d, reset required\n",
>> -			  avail, len);
>> +		xe_log_err(gt, GUC, -EIO,
>> +			   "G2H channel broken on read, avail=%d, len=%d, reset required\n",
>> +			   avail, len);
> #define	ENODATA		61	/* No data available */
>
>>   		goto corrupted;
>>   	}
>>   
>> @@ -1981,8 +1992,8 @@ static void g2h_fast_path(struct xe_guc_ct *ct, u32 *msg, u32 len)
>>   	}
>>   
>>   	if (ret) {
>> -		xe_gt_err(gt, "G2H action 0x%04x failed (%pe)\n",
>> -			  action, ERR_PTR(ret));
>> +		xe_log_err(gt, GUC, ret, "G2H action 0x%04x failed (%pe)\n",
>> +			   action, ERR_PTR(ret));
> nit: you may use %#x
> drop %pe
>
>>   		CT_DEAD(ct, NULL, FAST_G2H);
>>   	}
>>   }
>> @@ -2080,7 +2091,7 @@ static void receive_g2h(struct xe_guc_ct *ct)
>>   		mutex_unlock(&ct->lock);
>>   
>>   		if (unlikely(ret == -EPROTO || ret == -EOPNOTSUPP)) {
>> -			xe_gt_err(ct_to_gt(ct), "CT dequeue failed: %d\n", ret);
>> +			xe_log_err(ct_to_gt(ct), GUC, ret, "CT dequeue failed: %d\n", ret);
> drop %d as we will already print ret using %pe
>
> also, maybe worth to mention "..., forcing GT reset" ?
>
>>   			CT_DEAD(ct, NULL, G2H_RECV);
> hmm, I'm pretty sure this is redundant as we already call CT_DEAD
> on every case where we report an error, can you double check?

There is at least one failure case in process_g2h_msg where we don't 
call CT_DEAD. If we want to rework this so that CT_DEAD is not called 
from here I believe it should be done separately.

Apart from the suggestions I have commented on, I am implementing all 
the other ones.

Daniele

>
>>   			kick_reset(ct);
>>   		}