Re: [PATCH 4/4] drm/xe/sysctrl: Add better sysctrl error reporting

On Thu, Aug 06, 2026 at 04:30:44PM +0530, Mallesh Koujalagi wrote:
>Switch sysctrl error messages to xe_log_err() with SYSCTRL tags so
>tools can reliably detect and categorize common sysctrl failures.
>
>Signed-off-by: Mallesh Koujalagi <[email protected]>
>---
> drivers/gpu/drm/xe/xe_sysctrl_event.c   | 11 ++++++----
> drivers/gpu/drm/xe/xe_sysctrl_mailbox.c | 29 ++++++++++++++++---------
> 2 files changed, 26 insertions(+), 14 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_sysctrl_event.c b/drivers/gpu/drm/xe/xe_sysctrl_event.c
>index da395148ee9d..1805a0b4d0d0 100644
>--- a/drivers/gpu/drm/xe/xe_sysctrl_event.c
>+++ b/drivers/gpu/drm/xe/xe_sysctrl_event.c
>@@ -5,6 +5,7 @@
>
> #include "xe_device.h"
> #include "xe_irq.h"
>+#include "xe_log.h"
> #include "xe_printk.h"
> #include "xe_ras.h"
> #include "xe_sysctrl.h"
>@@ -25,13 +26,15 @@ static void get_pending_event(struct xe_sysctrl *sc, struct xe_sysctrl_mailbox_c

This seems to be running in a worker started from probe, but it does not 
fail probe. Do we still need to report this as xe_log_err/SIG_ID?

>
> 		ret = xe_sysctrl_send_command(sc, command, &len);
> 		if (ret) {
>-			xe_err(xe, "sysctrl: failed to get pending event %d\n", ret);
>+			xe_log_err(xe, SYSCTRL, ret,
>+				   "sysctrl: failed to get pending event\n");
> 			return;
> 		}
>
> 		if (len != sizeof(*response)) {
>-			xe_err(xe, "sysctrl: unexpected event response length %zu (expected %zu)\n",
>-			       len, sizeof(*response));
>+			xe_log_err(xe, SYSCTRL, 0,
>+				   "sysctrl: unexpected event response length %zu (expected %zu)\n",
>+				   len, sizeof(*response));
> 			return;
> 		}
>
>@@ -41,7 +44,7 @@ static void get_pending_event(struct xe_sysctrl *sc, struct xe_sysctrl_mailbox_c
> 			xe_warn(xe, "sysctrl: unexpected event %#x\n", response->event);
>
> 		if (!--count) {
>-			xe_err(xe, "sysctrl: event flooding\n");
>+			xe_log_err(xe, SYSCTRL, 0, "sysctrl: event flooding\n");
> 			return;
> 		}
>
>diff --git a/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c b/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c
>index e13eebaac1d0..5c8edf9871c6 100644
>--- a/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c
>+++ b/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c
>@@ -11,6 +11,7 @@
>
> #include "regs/xe_sysctrl_regs.h"
> #include "xe_device.h"
>+#include "xe_log.h"
> #include "xe_mmio.h"
> #include "xe_pm.h"
> #include "xe_printk.h"
>@@ -115,7 +116,8 @@ static int sysctrl_prepare_command(struct xe_device *xe,
> 	xe_assert(xe, command <= SYSCTRL_HDR_COMMAND_MAX);
>
> 	if (data_in_len > XE_SYSCTRL_MB_MAX_MESSAGE_SIZE - sizeof(*hdr)) {
>-		xe_err(xe, "sysctrl: Input data too large: %zu bytes\n", data_in_len);
>+		xe_log_err(xe, SYSCTRL, -EINVAL,
>+			   "sysctrl: Input data too large: %zu bytes\n", data_in_len);
> 		return -EINVAL;
> 	}
>
>@@ -149,7 +151,7 @@ static int sysctrl_send_frames(struct xe_sysctrl *sc,
> 	total_frames = DIV_ROUND_UP(cmd_size, XE_SYSCTRL_MB_FRAME_SIZE);
>
> 	if (!sysctrl_wait_bit_clear(sc, SYSCTRL_MB_CTRL_RUN_BUSY, timeout_ms)) {
>-		xe_err(xe, "sysctrl: Mailbox busy\n");
>+		xe_log_err(xe, SYSCTRL, -EBUSY, "sysctrl: Mailbox busy\n");
> 		return -EBUSY;
> 	}
>
>@@ -160,7 +162,8 @@ static int sysctrl_send_frames(struct xe_sysctrl *sc,
> 		frame_size = min_t(size_t, cmd_size - bytes_sent, XE_SYSCTRL_MB_FRAME_SIZE);
>
> 		if (sysctrl_write_frame(sc, mbox_cmd + bytes_sent, frame_size)) {
>-			xe_err(xe, "sysctrl: Failed to write frame %u\n", frame);
>+			xe_log_err(xe, SYSCTRL, -EIO,
>+				   "sysctrl: Failed to write frame %u\n", frame);
> 			sc->phase_bit = 0;
> 			return -EIO;
> 		}
>@@ -174,7 +177,8 @@ static int sysctrl_send_frames(struct xe_sysctrl *sc,
> 		xe_mmio_write32(sc->mmio, SYSCTRL_MB_CTRL, ctrl_reg);
>
> 		if (!sysctrl_wait_bit_clear(sc, SYSCTRL_MB_CTRL_RUN_BUSY, timeout_ms)) {
>-			xe_err(xe, "sysctrl: Frame %u acknowledgment timeout\n", frame);
>+			xe_log_err(xe, SYSCTRL, -ETIMEDOUT,
>+				   "sysctrl: Frame %u acknowledgment timeout\n", frame);
> 			sc->phase_bit = 0;
> 			return -ETIMEDOUT;
> 		}
>@@ -194,7 +198,8 @@ static int sysctrl_process_frame(struct xe_sysctrl *sc, void *out,
> 	int ret;
>
> 	if (!sysctrl_wait_bit_set(sc, SYSCTRL_MB_CTRL_RUN_BUSY_OUT, timeout_ms)) {
>-		xe_err(xe, "sysctrl: Response frame timeout\n");
>+		xe_log_err(xe, SYSCTRL, -ETIMEDOUT,
>+			   "sysctrl: Response frame timeout\n");
> 		return -ETIMEDOUT;
> 	}
>
>@@ -249,13 +254,15 @@ static int sysctrl_receive_frames(struct xe_sysctrl *sc,
> 	if (!XE_SYSCTRL_HDR_IS_RESPONSE(hdr) ||
> 	    XE_SYSCTRL_HDR_GROUP_ID(hdr) != XE_SYSCTRL_HDR_GROUP_ID(req) ||
> 	    XE_SYSCTRL_HDR_COMMAND(hdr) != XE_SYSCTRL_HDR_COMMAND(req)) {
>-		xe_err(xe, "sysctrl: Response header mismatch\n");
>+		xe_log_err(xe, SYSCTRL, -EPROTO,
>+			   "sysctrl: Response header mismatch\n");
> 		return -EPROTO;
> 	}
>
> 	if (XE_SYSCTRL_HDR_RESULT(hdr) != 0) {
>-		xe_err(xe, "sysctrl: Firmware error: 0x%02lx\n",
>-		       XE_SYSCTRL_HDR_RESULT(hdr));
>+		xe_log_err(xe, SYSCTRL, -EIO,
>+			   "sysctrl: Firmware error: 0x%02lx\n",
>+			   XE_SYSCTRL_HDR_RESULT(hdr));

Other than this ^ one "Firmware error", I am not sure if we want to 
report every sysctrl mailbox error using xe_log_err. Are there callers 
in the probe path that are using these functions and would probe benefit 
from reporting all of these?

Thanks,
Umesh
> 		return -EIO;
> 	}
>
>@@ -381,7 +388,8 @@ int xe_sysctrl_send_command(struct xe_sysctrl *sc,
> 				      cmd->data_in, cmd->data_in_len,
> 				      &mbox_cmd, &cmd_size);
> 	if (ret) {
>-		xe_err(xe, "sysctrl: Failed to prepare command: %pe\n", ERR_PTR(ret));
>+		xe_log_err(xe, SYSCTRL, ret,
>+			   "sysctrl: Failed to prepare command: %pe\n", ERR_PTR(ret));
> 		return ret;
> 	}
>
>@@ -391,7 +399,8 @@ int xe_sysctrl_send_command(struct xe_sysctrl *sc,
> 				   cmd->data_out, cmd->data_out_len, rdata_len,
> 				   XE_SYSCTRL_MB_DEFAULT_TIMEOUT_MS);
> 	if (ret)
>-		xe_err(xe, "sysctrl: Mailbox command failed: %pe\n", ERR_PTR(ret));
>+		xe_log_err(xe, SYSCTRL, ret,
>+			   "sysctrl: Mailbox command failed: %pe\n", ERR_PTR(ret));
>
> 	kfree(mbox_cmd);
>
>-- 
>2.48.1
>