Re: [PATCH v4 1/7] drm/xe/sysctrl: Return error codes from sysctrl_wait_bit_clear()

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 8/24/2026 7:44 AM, Tauro, Riana wrote:
> Hi Mallesh/Michal
> 
> On 20-08-2026 15:46, Mallesh Koujalagi wrote:
>> Make sysctrl_wait_bit_clear() return an error code rather than a bool.
>> and update callers to use xe_log_err() with the propagated error code.
> 
> Bit confused here on the usage of SIGID. My assumption based on documentation and discussion was
> that SIG ID is only needed for error logs based on severity and requires a resolution which can be documented.
> Am i missing something?

from "How to pick a SIGID (the uniqueness rule)"

 * A single underlying failure therefore legitimately produces a
 * *chain* of reports from different layers, each with its own SIGID -- e.g. a
 * GuC communication failure is reported as %XE_SIGID_RUNTIME_FW by the firmware
 * path, the failed recovery as %XE_SIGID_GT_TDR by the reset path, and an
 * aborted bind as %XE_SIGID_PROBE by the probe path. That chain lets triage
 * follow a fault from origin to final effect; it is not a duplicate.

so in this case, some sysctrl timeout may eventually lead to a reset and/or
wedge and/or runtime-survivability-mode, and this intermediate FW SIGID may
help in any postmortem triage, and if we don't reach that final state then
we can still provide some generic resolution based on site and errno (COLLECT)

> 
> How do these errors indicate the need for a resolution.
> Do we need it for all kmd logs with components or for errors that need a recovery or can be recoverable?
> 
> Thanks
> Riana
> 
>> Signed-off-by: Mallesh Koujalagi <mallesh.koujalagi@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_sysctrl_mailbox.c | 26 ++++++++++++-------------
>>   1 file changed, 13 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c b/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c
>> index e13eebaac1d0..ef847f0a8f2c 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"
>> @@ -34,15 +35,11 @@ struct xe_sysctrl_mailbox_msg_hdr {
>>   #define XE_SYSCTRL_HDR_RESULT(hdr) \
>>       FIELD_GET(SYSCTRL_HDR_RESULT_MASK, le32_to_cpu((hdr)->data))
>>   -static bool sysctrl_wait_bit_clear(struct xe_sysctrl *sc, u32 bit_mask,
>> -                   unsigned int timeout_ms)
>> +static int sysctrl_wait_bit_clear(struct xe_sysctrl *sc, u32 bit_mask,
>> +                  unsigned int timeout_ms)
>>   {
>> -    int ret;
>> -
>> -    ret = xe_mmio_wait32_not(sc->mmio, SYSCTRL_MB_CTRL, bit_mask, bit_mask,
>> +    return xe_mmio_wait32_not(sc->mmio, SYSCTRL_MB_CTRL, bit_mask, bit_mask,
>>                    timeout_ms * 1000, NULL, false);
>> -
>> -    return ret == 0;
>>   }
>>     static bool sysctrl_wait_bit_set(struct xe_sysctrl *sc, u32 bit_mask,
>> @@ -145,12 +142,14 @@ static int sysctrl_send_frames(struct xe_sysctrl *sc,
>>       struct xe_device *xe = sc_to_xe(sc);
>>       u32 ctrl_reg, total_frames, frame;
>>       size_t bytes_sent, frame_size;
>> +    int ret;
>>         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");
>> -        return -EBUSY;
>> +    ret = sysctrl_wait_bit_clear(sc, SYSCTRL_MB_CTRL_RUN_BUSY, timeout_ms);
>> +    if (ret) {
>> +        xe_log_err(xe, SYSCTRL, ret, "Mailbox busy\n");
>> +        return ret;
>>       }
>>         sc->phase_bit ^= 1;
>> @@ -173,10 +172,11 @@ 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);
>> +        ret = sysctrl_wait_bit_clear(sc, SYSCTRL_MB_CTRL_RUN_BUSY, timeout_ms);
>> +        if (ret) {
>> +            xe_log_err(xe, SYSCTRL, ret, "Frame %u acknowledgment timeout\n", frame);
>>               sc->phase_bit = 0;
>> -            return -ETIMEDOUT;
>> +            return ret;
>>           }
>>             bytes_sent += frame_size;