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

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

On Tue, Sep 15, 2026 at 03:59:22PM -0700, Umesh Nerlige Ramappa wrote:
>On Tue, Sep 15, 2026 at 11:49:43AM +0200, Michal Wajdeczko wrote:
>>
>>
>>On 9/4/2026 1:40 AM, 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.
>>>
>>>v2: (Michal)
>>>- Sync order of errors in g2h_read and __guc_ct_send_locked
>>>- For CT errors use EPIPE and for HXG errors use EPROTO
>>>- Convert the non-fatal EPIPE to EPERM in the helper
>>
>>nit: move change log under --- line
>>
>>>
>>>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 | 51 +++++++++++++++++++++++++++-------
>>> 1 file changed, 41 insertions(+), 10 deletions(-)
>>>
>>>diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
>>>index 5c4733da385c..c5a417fef913 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 = -EPIPE;
>>> 			xe_gt_err(gt, "CT write: non-zero status: %u\n", desc_status);
>>> 			goto corrupted;
>>
>>I'm wondering if maybe we should introduce helper:
>>
>>	int ct_corrupted(ct, const char *msg, ...)
>>	{
>>		va_start()
>>		xe_gt_err(gt, "GUC: CT: %pV", vaf);
>>		va_end()
>>		CT_DEAD()
>>		ct_stop() 	// ?
>>		kick_reset()	// ?
>>		return -EPIPE;
>>	}
>>
>>and just call it instead using goto?
>>
>>this helper can be later reused by g2h_read
>
>kick_reset is not called in the send path for -EPIPE (like you mention 
>below), so not a whole lot of reuse we can do with the helper.

Maybe I can use the helper without the ct_stop and kick_reset...

Umesh
>
>>
>>> 		}
>>>
>>> 		if (tail > h2g->info.size) {
>>> 			desc_write(xe, h2g, status, desc_status | GUC_CTB_STATUS_OVERFLOW);
>>>+			err = -EPIPE;
>>> 			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;
>>> 			xe_gt_err(gt, "CT write: invalid head offset %u >= %u)\n",
>>> 				  desc_head, h2g->info.size);
>>> 			goto corrupted;
>>>@@ -1044,7 +1048,7 @@ static int h2g_write(struct xe_guc_ct *ct, const u32 *action, u32 len,
>>>
>>> corrupted:
>>> 	CT_DEAD(ct, &ct->ctbs.h2g, H2G_WRITE);
>>>-	return -EPIPE;
>>>+	return err;
>>> }
>>>
>>> static int __guc_ct_send_locked(struct xe_guc_ct *ct, const u32 *action,
>>>@@ -1067,11 +1071,6 @@ static int __guc_ct_send_locked(struct xe_guc_ct *ct, const u32 *action,
>>> 		goto out;
>>> 	}
>>>
>>>-	if (unlikely(ct->ctbs.h2g.info.broken)) {
>>>-		ret = -EPIPE;
>>>-		goto out;
>>>-	}
>>>-
>>> 	if (ct->state == XE_GUC_CT_STATE_DISABLED) {
>>> 		ret = -ENODEV;
>>> 		goto out;
>>>@@ -1082,6 +1081,11 @@ static int __guc_ct_send_locked(struct xe_guc_ct *ct, const u32 *action,
>>> 		goto out;
>>> 	}
>>>
>>>+	if (unlikely(ct->ctbs.h2g.info.broken)) {
>>>+		ret = -EPIPE;
>>>+		goto out;
>>
>>to be fixed with -EPERM/-EUCLEAN, or ...
>
>I changed this to EPERM
>
>>
>>... maybe this should be just xe_gt_assert()?
>>
>>IMO we should immediately STOP the CTB once we detect that CTB channel
>>is broken so we should look for STOPPED status rather than info.broken
>
>Maybe, but we don't do that right now. Probably a future improvement.
>
>>
>>>+	}
>>>+
>>> 	xe_gt_assert(gt, xe_guc_ct_enabled(ct));
>>>
>>> 	if (g2h_fence) {
>>>@@ -1809,10 +1813,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;
>>>
>>>@@ -1823,7 +1829,7 @@ static int g2h_read(struct xe_guc_ct *ct, u32 *msg, bool fast_path)
>>> 		return -ECANCELED;
>>>
>>> 	if (g2h->info.broken)
>>>-		return -EPIPE;
>>>+		return -EPERM;
>>>
>>> 	xe_gt_assert(gt, xe_guc_ct_enabled(ct));
>>>
>>>@@ -1840,6 +1846,7 @@ static int g2h_read(struct xe_guc_ct *ct, u32 *msg, bool fast_path)
>>> 		}
>>>
>>> 		if (desc_status) {
>>>+			err = -EPIPE;
>>> 			xe_gt_err(gt, "CT read: non-zero status: %u\n", desc_status);
>>> 			goto corrupted;
>>> 		}
>>>@@ -1871,6 +1878,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 = -EPIPE;
>>> 			xe_gt_err(gt, "CT read: head out of range: %u vs %u\n",
>>> 				  g2h->info.head, g2h->info.size);
>>> 			goto corrupted;
>>>@@ -1878,6 +1886,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 = -EPIPE;
>>> 			xe_gt_err(gt, "CT read: invalid tail offset %u >= %u)\n",
>>> 				  desc_tail, g2h->info.size);
>>> 			goto corrupted;
>>>@@ -1898,6 +1907,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 = -EPIPE;
>>> 		xe_gt_err(gt, "G2H channel broken on read, avail=%d, len=%d, reset required\n",
>>> 			  avail, len);
>>> 		goto corrupted;
>>>@@ -1950,7 +1960,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 +2052,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 -EPERM:		/* CT already declared broken */
>>>+		return false;
>>>+	default:
>>>+		return true;
>>
>>shouldn't this be other way around?
>>IMO original fatal errors are:
>>
>>	-EPIPE
>>	-EPROTO
>>	-EDEADLK
>>
>>as once we hit them, we should trigger a RESET (which will then
>>result in cancelling all pending H2G with -ECANCELED, turning off
>>the CTB), so any new CTB request will get either
>>
>>	-EPERM - already stopped
>>	-ENOTRECOVERABLE - if recovery fails
>>
>>>+	}
>
>Correct, the default case is fatal above and returns true, so it 
>matches what you are saying.
>
>>>+}
>>
>>maybe we should document in a separate DOC section all error codes used by the CTB?
>>
>>	-ENODEV = CTB disabled
>>	-ENOTRECOVERABLE = device wedged
>>	-EDEADLK = CTB deadlocked ==> RESET/STOP
>>	-EPIPE = detected problems with CTB descriptor/channel ==> RESET/STOP
>>	-EPROTO = detected problem with CTB message ==> RESET/STOP
>>	-ECANCELED = pending H2G cancelled due to a RESET
>>	-EPERM = CTB already stopped
>>	-EUCLEAN = CTB already broken
>>		btw, should we ever reach ctb.broken?
>>		shouldn't we move the CTB to STOPPED state before?
>>	...
>
>I will put it in comments in the beginning of this file.
>
>>
>>>+
>>> static void receive_g2h(struct xe_guc_ct *ct)
>>> {
>>> 	bool ongoing;
>>>@@ -2079,8 +2110,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));
>>> 			CT_DEAD(ct, NULL, G2H_RECV);
>>> 			kick_reset(ct);
>>
>>hmm, it looks that during send() we kick_reset() only for EDEADLK
>>error, even if we detect broken channel (EPIPE), right?
>
>Right. That's the current behavior
>
>Thanks,
>Umesh
>>
>>
>>> 		}
>>