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

["Mallesh, Koujalagi" <mallesh.koujalagi@intel.com>]

On 06-08-2026 08:11 pm, Michal Wajdeczko wrote:
>
> 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 <mallesh.koujalagi@intel.com>
>> ---
>>   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
Sure, will remove in next revision.
>
>>   			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
Ok.
>
>
>>   			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?
btw, do we've any SIGID based warning helper function? we can see that 
warning message in dmesg right..any suggestion.
>>   
>>   		if (!--count) {
>> -			xe_err(xe, "sysctrl: event flooding\n");
>> +			xe_log_err(xe, SYSCTRL, 0, "sysctrl: event flooding\n");
> again, drop the "sysctrl:" prefix
Sure
> and use some errno, maybe -ETOOMANYREFS ?
We can use  -EOVERFLOW  right.
> and maybe also print response->count to show how many events we left behind?
will add next revision.
> 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)

Hitting the flood limit (16) that means something is abnormal @ firmware 
side which

generate event faster than we can drain them. If we would not use flood 
limit, it would not break the loop.

In such scenario, we would not retry, simply report it.

>
>>   			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?
Agreed,  instead of condition, they would have used xe_assert.
>> -		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

Sure, will update in next revision

Thanks,

-/Mallesh

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