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

["Tauro, Riana" <riana.tauro@intel.com>]

On 24-08-2026 16:34, Michal Wajdeczko wrote:
>
> 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)

What if the paths are also used in non-critical scenarios for ex: 
sysctrl commands are used for
general counter telemetry, gpu health querying and firmware status.
Wouldn't this be lead to a lot of sigids in dmesg in case of some 
unrelated failure ..
Why not only add sigid for critical paths when we are sure it will cause 
a chain of failure
instead of these prints on every trivial command failure.

Thanks
Riana

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