Re: [PATCH v2 2/5] drm/xe/guc: Use different error codes for CT errors

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

On 9/1/2026 11:12 PM, Umesh Nerlige Ramappa wrote:
> Instead of always returning -EPROTO for most errors, use different error
> codes based on the error type. -EPROTO is retained for the cases that
> are genuine protocol violations by the GuC.  The remaining cases now
> report what actually went wrong.
> 
> receive_g2h() used to escalate to CT_DEAD + kick_reset() by matching the
> two error codes that dequeue_one_g2h() could return on a fatal error.
> Invert the check to simplify reset handling.
> 
> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Assisted-by: Claude:claude-opus-5
> ---
>  drivers/gpu/drm/xe/xe_guc_ct.c | 48 +++++++++++++++++++++++++++++-----
>  1 file changed, 42 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
> index 5c4733da385c..dcd457f38b89 100644
> --- a/drivers/gpu/drm/xe/xe_guc_ct.c
> +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
> @@ -947,6 +947,7 @@ static int h2g_write(struct xe_guc_ct *ct, const u32 *action, u32 len,
>  	u32 cmd[H2G_CT_HEADERS];
>  	u32 tail = h2g->info.tail;
>  	u32 full_len;
> +	int err;
>  	struct iosys_map map = IOSYS_MAP_INIT_OFFSET(&h2g->cmds,
>  							 tail * sizeof(u32));
>  
> @@ -961,12 +962,14 @@ static int h2g_write(struct xe_guc_ct *ct, const u32 *action, u32 len,
>  
>  		desc_status = desc_read(xe, h2g, status);
>  		if (desc_status) {
> +			err = -EPROTO;

didn't we agree to use -EPIPE for a bad CT descriptor status?

>  			xe_gt_err(gt, "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);
> +			err = -EPROTO;

hmm, IMO it would be better to use -EPROTO only for the errors related
to the actual HXG messages, and for raw CT transport failures use -EPIPE

unless we want to use -EPIPE only for existing problems in descriptor
and for new error conditions use something else, like -ENFILE?
but maybe that's overkill as it is fatal anyway, and the message already
says what went wrong

>  			xe_gt_err(gt, "CT write: tail out of range: %u vs %u\n",
>  				  tail, h2g->info.size);
>  			goto corrupted;
> @@ -974,6 +977,7 @@ static int h2g_write(struct xe_guc_ct *ct, const u32 *action, u32 len,
>  
>  		if (desc_head >= h2g->info.size) {
>  			desc_write(xe, h2g, status, desc_status | GUC_CTB_STATUS_OVERFLOW);
> +			err = -EPIPE;

we should be consistent 

	-EPIPE = CT already broken
vs
	-EPIPE = all CT channel errors


>  			xe_gt_err(gt, "CT write: invalid head offset %u >= %u)\n",
>  				  desc_head, h2g->info.size);
>  			goto corrupted;
> @@ -1591,14 +1595,19 @@ static int parse_g2h_response(struct xe_guc_ct *ct, u32 *msg, u32 len)
>  	 * failure to trigger a reset.
>  	 */
>  	if (fence & CT_SEQNO_UNTRACKED) {
> -		if (type == GUC_HXG_TYPE_RESPONSE_FAILURE)
> +		int err;
> +
> +		if (type == GUC_HXG_TYPE_RESPONSE_FAILURE) {
> +			err = -EBADE;

I'm little concerned that there was no other proposal here ;)

>  			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]));
> -		else
> +		} else {
> +			err = -EPROTO;
>  			xe_gt_err(gt, "unexpected response %u for FAST_REQ H2G fence 0x%x!\n",
>  				  type, fence);
> +		}
>  
>  		fast_req_report(ct, fence);
>  
> @@ -1608,7 +1617,7 @@ static int parse_g2h_response(struct xe_guc_ct *ct, u32 *msg, u32 len)
>  
>  		CT_DEAD(ct, NULL, PARSE_G2H_RESPONSE);
>  
> -		return -EPROTO;
> +		return err;
>  	}
>  
>  	/* don't erase as we still expect a final response with the same fence */
> @@ -1809,10 +1818,12 @@ static int g2h_read(struct xe_guc_ct *ct, u32 *msg, bool fast_path)
>  	s32 avail;
>  	u32 action;
>  	u32 *hxg;
> +	int err;
>  
>  	xe_gt_assert(gt, xe_guc_ct_initialized(ct));
>  	lockdep_assert_held(&ct->fast_lock);
>  
> +	/* Keep in sync with g2h_err_is_fatal() */
>  	if (xe_device_wedged(xe))
>  		return -ENOTRECOVERABLE;
>  
> @@ -1840,6 +1851,7 @@ static int g2h_read(struct xe_guc_ct *ct, u32 *msg, bool fast_path)
>  		}
>  
>  		if (desc_status) {
> +			err = -EIO;

earlier in this patch in h2g_write you used -EPROTO for the similar issue
but I would go with -EPIPE here
>  			xe_gt_err(gt, "CT read: non-zero status: %u\n", desc_status);
>  			goto corrupted;
>  		}
> @@ -1871,6 +1883,7 @@ 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);
> +			err = -ERANGE;

-EPIPE if we want to treat all CT descriptor problems in the same way

>  			xe_gt_err(gt, "CT read: head out of range: %u vs %u\n",
>  				  g2h->info.head, g2h->info.size);
>  			goto corrupted;
> @@ -1878,6 +1891,7 @@ static int g2h_read(struct xe_guc_ct *ct, u32 *msg, bool fast_path)
>  
>  		if (desc_tail >= g2h->info.size) {
>  			desc_write(xe, g2h, status, desc_status | GUC_CTB_STATUS_OVERFLOW);
> +			err = -ERANGE;

ditto

>  			xe_gt_err(gt, "CT read: invalid tail offset %u >= %u)\n",
>  				  desc_tail, g2h->info.size);
>  			goto corrupted;
> @@ -1898,6 +1912,7 @@ 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) {
> +		err = -EBADMSG;
>  		xe_gt_err(gt, "G2H channel broken on read, avail=%d, len=%d, reset required\n",
>  			  avail, len);
>  		goto corrupted;
> @@ -1950,7 +1965,7 @@ static int g2h_read(struct xe_guc_ct *ct, u32 *msg, bool fast_path)
>  
>  corrupted:
>  	CT_DEAD(ct, &ct->ctbs.g2h, G2H_READ);
> -	return -EPROTO;
> +	return err;
>  }
>  
>  static void g2h_fast_path(struct xe_guc_ct *ct, u32 *msg, u32 len)
> @@ -2042,6 +2057,27 @@ static int dequeue_one_g2h(struct xe_guc_ct *ct)
>  	return 1;
>  }
>  
> +/*
> + * Errors reported by dequeue_one_g2h() come in two flavours: either the channel
> + * is simply not available right now, which is expected and handled gracefully,
> + * or the channel state or the message itself is inconsistent, in which case the
> + * only way forward is to declare the CT dead and reset the GuC. Since the
> + * former is a short and well known list, check against that and treat anything
> + * else as fatal, so that new error codes don't silently escape the escalation.
> + */
> +static bool g2h_err_is_fatal(int err)
> +{
> +	switch (err) {
> +	case -ENOTRECOVERABLE:	/* device wedged */
> +	case -ENODEV:		/* CT disabled */
> +	case -ECANCELED:	/* CT stopped */
> +	case -EPIPE:		/* CT already declared broken */

btw, there is a different order of checks in

	__guc_ct_send_locked()
		if (xe_device_wedged(ct_to_xe(ct))) {
		if (unlikely(ct->ctbs.h2g.info.broken)) {
		if (ct->state == XE_GUC_CT_STATE_DISABLED) {
		if (ct->state == XE_GUC_CT_STATE_STOPPED || xe_gt_recovery_pending(gt)) {

vs

	g2h_read
		if (xe_device_wedged(xe))
		if (ct->state == XE_GUC_CT_STATE_DISABLED)
		if (ct->state == XE_GUC_CT_STATE_STOPPED)
		if (g2h->info.broken)
	
and IMO we should revisit usage of -ECANCELED as it should be used only
for already sent H2G which wait for G2H but were cancelled due to a reset

if someone is still trying to send H2G when CT is already stopped/
then we should use different code, maybe -EPERM ?

-ENOTRECOVERABLE	/* device already wedged */
-ENODEV			/* CT disabled */
-EPERM			/* CT already stopped */
-EPROTO			/* CT message broken */    => reset => stop/start => enabled|wedged
-EPIPE			/* CT channel broken */	   => reset => stop/start => enabled|wedged
-EDEADLK		/* CT deadlock detected */ => reset => stop/start => enabled|wedged
-ECANCELED		/* CT stopped/GT reset */



> +		return false;
> +	default:
> +		return true;

are we sure only all other errors need to be recovered immediately by
the full GT reset? not the other way around?

we should kick reset on the first report of -EPROTO/-EPIPE (when we detect broken
HXG message or CTB descriptor), subsequent calls to send() shall return new
either new errors, like -EPERM, as CTB should be already STOPPED, or -EPIPE
as we might still wait for the reset flow, but CTB is already marked BROKEN.

and I'm not sure that reset would help for -EOPNOTSUPP, as it is unlikely
that driver will suddenly start supporting some new messages ...

> +	}
> +}
> +
>  static void receive_g2h(struct xe_guc_ct *ct)
>  {
>  	bool ongoing;
> @@ -2079,8 +2115,8 @@ static void receive_g2h(struct xe_guc_ct *ct)
>  		ret = dequeue_one_g2h(ct);
>  		mutex_unlock(&ct->lock);
>  
> -		if (unlikely(ret == -EPROTO || ret == -EOPNOTSUPP)) {
> -			xe_gt_err(ct_to_gt(ct), "CT dequeue failed: %d\n", ret);
> +		if (unlikely(ret < 0 && g2h_err_is_fatal(ret))) {
> +			xe_gt_err(ct_to_gt(ct), "CT dequeue failed (%pe)\n", ERR_PTR(ret));

btw, shouldn't we always report an error and just kick reset on fatal?
or maybe even move kick_reset() somewhere else we know it is something
new and fatal (like detected corrupted descriptor) ?

>  			CT_DEAD(ct, NULL, G2H_RECV);
>  			kick_reset(ct);
>  		}