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

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

On 8/27/2026 11:33 PM, Daniele Ceraolo Spurio wrote:
> 
> 
> 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.

I assume you're referring to this code:

		if (unlikely(ret == -EPROTO || ret == -EOPNOTSUPP)) {
			xe_gt_err(ct_to_gt(ct), "CT dequeue failed: %d\n", ret);
			CT_DEAD(ct, NULL, G2H_RECV);
			kick_reset(ct);
		}

where those two errors were used to start a reset but maybe
we should just move kick_reset() to the CT_DEAD() and look
at the dead_reason instead?

@Matt

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

sure, as a prerequisite series ;)

my point is, that before rapidly switching to use xe_log,
we should make sure reported errors are meaningful, and
if needed we should do some cleanup first

> 
> Apart from the suggestions I have commented on, I am implementing all the other ones.
> 
> Daniele
> 
>>
>>>               kick_reset(ct);
>>>           }
>