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

On 8/18/2026 3:10 PM, Mallesh Koujalagi wrote:
> Switch sysctrl error messages to xe_log_err() with SYSCTRL tags so

s/Switch sysctr error/Switch the System Controller (sysctrl) error

> tools can reliably detect and categorize common sysctrl failures.
> 
> Signed-off-by: Mallesh Koujalagi <[email protected]>
> ---
> v2:
> - Use -EOVERFLOW.
> - Drop sysctrl in xe_log_err. (Michal)
> - Drop redundant %pe.
> - Print response->count.
> 
> v3:
> - Rebase.
> ---
>  drivers/gpu/drm/xe/xe_sysctrl_event.c   | 12 ++++++----
>  drivers/gpu/drm/xe/xe_sysctrl_mailbox.c | 29 ++++++++++++++++---------
>  2 files changed, 27 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..4341ea99cdc3 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,
> +				   "failed to get pending event\n");

nit1: no need to break line

nit2: we should be consistent (at least per file) whether
the error message should start with lowercase or uppercase

>  			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, -EMSGSIZE,
> +				   "unexpected event response length %zu (expected %zu)\n",
> +				   len, sizeof(*response));
>  			return;
>  		}
>  
> @@ -41,7 +44,8 @@ static void get_pending_event(struct xe_sysctrl *sc, struct xe_sysctrl_mailbox_c
>  			xe_warn(xe, "sysctrl: unexpected event %#x\n", response->event);

we do have xe_log_err_info() now - maybe we can use it here?

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

nit: from the dmesg line it will be hard to guess what this %u really mean

>  			return;
>  		}
>  
> diff --git a/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c b/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c
> index e13eebaac1d0..a96ef2864526 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,
> +			   "Input data too large: %zu bytes\n", data_in_len);

didn't we agree to change that to xe_assert() ?

(this could be done as a preparation patch)

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

nit: shouldn't sysctrl_wait_bit_clear() and friends return -errno instead of bool?
then we wouldn't need to figure out the errno code at the callers side

(this could be done as a preparation patch)

> -		xe_err(xe, "sysctrl: Mailbox busy\n");
> +		xe_log_err(xe, SYSCTRL, -EBUSY, "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)) {

hmm, actually sysctrl_write_frame() always return 0
either change that function to void
or use the returned error code

(also as separate patch)

> -			xe_err(xe, "sysctrl: Failed to write frame %u\n", frame);
> +			xe_log_err(xe, SYSCTRL, -EIO,
> +				   "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,
> +				   "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,
> +			   "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,
> +			   "Response header mismatch\n");

maybe we should print the unexpected data from the header?

>  		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,
> +			   "Firmware error: 0x%02lx\n",
> +			   XE_SYSCTRL_HDR_RESULT(hdr));
>  		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,
> +			   "Failed to prepare command\n");

nit: do we need to split the line here?

and maybe it is good to know which command actually failed?


>  		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,
> +			   "Mailbox command failed\n");

ditto

> 
>  	kfree(mbox_cmd);
>