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

On 8/6/2026 1:00 PM, 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
>  
>  		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");

using "sysctrl: " prefix here is now redundant, as xe_log_err will add "SYSCTRL: " based on the TAG

>  			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));

ditto

also since this is unexpected protocol error we should use -EPROTO or similar errno


>  			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);

btw, shouldn't we also report that ^^^ warn case?

>  
>  		if (!--count) {
> -			xe_err(xe, "sysctrl: event flooding\n");
> +			xe_log_err(xe, SYSCTRL, 0, "sysctrl: event flooding\n");

again, drop the "sysctrl:" prefix
and use some errno, maybe -ETOOMANYREFS ?

and maybe also print response->count to show how many events we left behind?
hmm, but what will happen to those events that we don't retrieve now?
are they lost (and that's why we need to report an error)
or maybe we will be able to retrieve them in another cycle?
(but then I'm not sure we should report that as an error)

>  			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)) {

hmm, shouldn't this be a simple xe_assert() instead?
after all, it is us who prepares these cmds, right?

> -		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");

drop "sysctrl:" prefix

>  		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);

ditto

>  			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);

ditto

>  			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");

ditto

>  		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");

ditto

>  		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));

ditto

>  		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));

ditto

and drop redundant %pe as xe_log_err() will also print that error 

>  		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));

ditto

>  
>  	kfree(mbox_cmd);
>