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

On 07-08-2026 12:44 am, Umesh Nerlige Ramappa wrote:
> 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?
Yes, we need to report all System controller errors using SIGID API, 
please refer 
https://patchwork.freedesktop.org/patch/743308/?series=171022&rev=3
>
>>
>>         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?

Yes, all System Controller errors should be reported using the SYSCTRL 
SIGID API. In addition, probe path failures should be reported 
independently using the PROBE SIGID API.

This separation ensures that System Controller errors and probe-related 
failures are tracked and categorized appropriately.

For more details about the SIGID reporting framework,

please refer to: https://patchwork.freedesktop.org/series/171022/


Thanks,

-/Mallesh

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