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

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

On 04-09-2026 02:36 pm, Tauro, Riana wrote:
>
> On 04-09-2026 13:55, Mallesh, Koujalagi wrote:
>>
>> On 25-08-2026 01:05 pm, Tauro, Riana wrote:
>>>
>>> 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.
>>
>> Ops I miss this.
>>
>> I agree that we should not add SIGID for every minor or noisy 
>> failure. The intent is not to log every non-critical cases,
>>
>> but to capture important intermediate failures that can help explain 
>> the real root cause. If we log only the final critical
>>
>> failure, we may miss the earlier cases that actually caused the 
>> issue. That makes triage and postmortem harder, since
>>
>> we lose visibility into how the failure occurred. The idea is to use 
>> SIGID only for meaningful failure in key execution paths.
>>
>> Even when these failure are not fatal, they can provide valuable 
>> context, make debugging easier and reduce
>>
>> mean-time-to triage (MTTT).
>
>
> Suppose a user uses invalid entries for gpu health or counter 
> management, won't we be polluting the dmesg
> logs with sig id and cper logs unnecessarily since these are not fatal.

No, we should not emit SIGID/CPER for bad user input  or invalid query 
parameters in gpu health/counter management path.

These are usage errors, not real device or firmware failures.


Thanks,

-/Mallesh

>
> Thanks
> Riana
>
>
>>
>>
>> Thanks,
>>
>> -/Mallesh
>>
>>>
>>> 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;