Re: [PATCH v7 1/2] drm/xe/sysctrl: Add helper to query application status

On Tue, Aug 18, 2026 at 11:11:13PM +0530, Anoop Vijay wrote:
>   On 15-08-2026 02:51, Umesh Nerlige Ramappa wrote:
>
>     On Fri, Aug 14, 2026 at 02:53:06AM -0700, Anoop, Vijay wrote:
>
>       From: Anoop Vijay <[email protected]>
>
>       Add xe_sysctrl_check_app_status() to query the state of a System
>       Controller application using get_app_status_by_id mailbox command.
>
>       The helper maps xe_sysctrl_app_id values to firmware application
>       IDs and returns the reported application state. Add a convenience
>       wrapper to check diag firmware application readiness.
>
>       Signed-off-by: Anoop Vijay <[email protected]>
>       ---
>       v2: (Badal)
>       - Return SysCtrl firmware application states instead of errno for
>        application lifecycle conditions
>
>       v3: (Riana, Badal)
>       - Replace string-based app identifiers with enum xe_sysctrl_app_id
>       - Use get_app_status_by_id (opcode 0x05)
>       - Move mailbox definitions to xe_sysctrl_mailbox_types.h
>       - Return enum xe_sysctrl_fw_status consistently
>       - Map communication failures to COMM_FAILURE
>       - Add INITIALIZED state and status helpers
>       - Fix Diagnostics typo
>
>       v4: (Anshuman)
>       - Make xe_sysctrl_check_app_status() internal
>       - Remove unused app identifier
>
>       v5: (Anshuman)
>       - Rename OOBMSM status helper to use xe_sysctrl_* prefix
>       - Remove xe_is_diag_fw_ready() helper
>
>       v6: (Riana)
>       - Use xe_sysctrl_create_command() helper instead of manual FIELD_PREP
>        header packing
>       - Rename xe_sysctrl_get_app_status_by_id_{req,resp} to shorter names
>       - Use hex constant for application ID
>       - Move oCode application readiness helper to a separate patch
>       - Add xe_sysctrl_is_diag_fw_ready() helper.
>
>       v7: (Umesh)
>       - Clarify xe_sysctrl_is_diag_fw_ready() comment for sysctrl-less
>       platforms
>       - Move XE_SYSCTRL_APP_RESP_* flags and enum xe_sysctrl_app_id to
>       xe_sysctrl.c
>       ---
>       drivers/gpu/drm/xe/xe_sysctrl.c               | 80 +++++++++++++++++++
>       drivers/gpu/drm/xe/xe_sysctrl.h               |  1 +
>       drivers/gpu/drm/xe/xe_sysctrl_mailbox_types.h | 47 +++++++++++
>       3 files changed, 128 insertions(+)
>
>       diff --git a/drivers/gpu/drm/xe/xe_sysctrl.c
>       b/drivers/gpu/drm/xe/xe_sysctrl.c
>       index 1db20be8158b..c53a26af3189 100644
>       --- a/drivers/gpu/drm/xe/xe_sysctrl.c
>       +++ b/drivers/gpu/drm/xe/xe_sysctrl.c
>       @@ -13,9 +13,11 @@
>       #include "xe_device.h"
>       #include "xe_mmio.h"
>       #include "xe_pm.h"
>       +#include "xe_printk.h"
>       #include "xe_soc_remapper.h"
>       #include "xe_sysctrl.h"
>       #include "xe_sysctrl_mailbox.h"
>       +#include "xe_sysctrl_mailbox_types.h"
>       #include "xe_sysctrl_types.h"
>
>       /**
>       @@ -29,6 +31,20 @@
>        * This module provides initialization and support code for
>       interacting
>        * with System Controller through the mailbox interface.
>        */
>       +
>       +/* Application status flags reported in
>       xe_sysctrl_app_status_resp.flags */
>       +#define XE_SYSCTRL_APP_RESP_VALID    BIT(0)
>       +#define XE_SYSCTRL_APP_RESP_BOOTED    BIT(1)
>       +#define XE_SYSCTRL_APP_RESP_INITIALIZED    BIT(2)
>       +
>       +/*
>       + * Known System Controller application identifiers, keyed by firmware
>       + * application ID.
>       + */
>       +enum xe_sysctrl_app_id {
>       +    XE_SYSCTRL_APP_DIAG    = 0x0D,
>       +};
>       +
>       static void sysctrl_fini(void *arg)
>       {
>           struct xe_device *xe = arg;
>       @@ -130,3 +146,67 @@ void xe_sysctrl_pm_resume(struct xe_device *xe)
>
>           xe_sysctrl_mailbox_init(sc);
>       }
>       +
>       +static enum xe_sysctrl_fw_status
>       +xe_sysctrl_check_app_status(struct xe_device *xe, enum
>       xe_sysctrl_app_id app_id)
>       +{
>       +    struct xe_sysctrl_app_status_req req = {};
>       +    struct xe_sysctrl_app_status_resp resp = {};
>       +    struct xe_sysctrl_mailbox_command cmd = {};
>       +    size_t out_len = 0;
>       +    u32 flags;
>       +    int ret;
>       +
>       +    if (!xe->info.has_sysctrl)
>       +        return XE_SYSCTRL_FIRMWARE_APP_NOTSUPP;
>       +
>       +    req.app_id = (u8)app_id;
>       +
>       +    xe_sysctrl_create_command(&cmd, XE_SYSCTRL_GROUP_CORE,
>       XE_SYSCTRL_CMD_GET_APP_STATUS_BY_ID,
>       +                  &req, sizeof(req), &resp, sizeof(resp));
>       +
>       +    ret = xe_sysctrl_send_command(&xe->sc, &cmd, &out_len);
>       +    if (ret)
>       +        return XE_SYSCTRL_FIRMWARE_COMM_FAILURE;
>       +
>       +    if (out_len != sizeof(resp)) {
>       +        xe_err(xe, "sysctrl: unexpected get app status response
>       length %zu (expected %zu)\n",
>       +               out_len, sizeof(resp));
>       +        return XE_SYSCTRL_FIRMWARE_COMM_FAILURE;
>       +    }
>       +
>       +    flags = resp.flags;
>       +
>       +    if (!(flags & XE_SYSCTRL_APP_RESP_VALID))
>       +        return XE_SYSCTRL_FIRMWARE_APP_INVALID;
>       +
>       +    if (!(flags & XE_SYSCTRL_APP_RESP_BOOTED))
>       +        return XE_SYSCTRL_FIRMWARE_APP_NOT_LOADED;
>       +
>       +    if (!(flags & XE_SYSCTRL_APP_RESP_INITIALIZED))
>       +        return XE_SYSCTRL_FIRMWARE_APP_BOOTED;
>       +
>       +    return XE_SYSCTRL_FIRMWARE_APP_INITIALIZED;
>       +}
>       +
>       +/**
>       + * xe_sysctrl_is_diag_fw_ready() - Check if diag firmware is fully
>       initialized
>       + * @xe: xe device instance
>       + *
>       + * Returns true if diag firmware has reached the initialized state,
>       indicating
>       + * it is ready to handle requests. On platforms without System
>       Controller
>       + * support there is no System Controller mailbox to gate on, so diag
>       firmware
>       + * readiness is not tracked through this path; the function
>       unconditionally
>       + * returns true so callers are not blocked by this check on such
>       platforms.
>
>     I am still not clear how the caller will handle the 2 scenarios - (1) SC
>     is present, but diag app is not initialized vs (2) SC and mailbox are
>     not available and hence Diag App is not supported. The caller will just
>     see true in both cases.
>
>     Instead I would see this as 2 helpers:
>
>     xe_sysctrl_is_diag_fw_supported() that returns
>     status == XE_SYSCTRL_FIRMWARE_APP_NOTSUPP;
>
>     and
>
>     xe_sysctrl_is_diag_fw_ready() that returns
>     status == XE_SYSCTRL_FIRMWARE_APP_INITIALIZED;
>
>     That would be clearer to the caller.
>
>     Thanks,
>     Umesh
>
>   All current/planned callers of xe_sysctrl_is_oobmsm_fw_ready() /
>   xe_sysctrl_is_diag_fw_ready() (xe_vsec.c retry/gate logic,
>   xe_sysctrl_debugfs.c RAS injection gate, xe_sysctrl_fwctl.c capability
>   gate) only need a yes/no answer to "Can this functionality be used now?"
>    and treat NOTSUPP and 'not yet initialized' the same way.

Looking at all these callers in [1] and [2], I don't see a need for 
NOTSUPP. Both fwctl and debugfs callers seem to gate init/registration 
using has_sysctrl already.

I dont' know about xe_vsec, but looking at the code, it has specific 
implementation for different platforms, so it will not call the _ready 
helper on unsupported platforms. @Michael for comments.

We can just drop the XE_SYSCTRL_FIRMWARE_APP_NOTSUPP check in the ready 
helpers.

Thanks,
Umesh

[1] https://patchwork.freedesktop.org/series/171728/
[2] https://patchwork.freedesktop.org/series/161655/
>
>   Folding NOTSUPP into 'ready' is intentional. It lets sysctrl-less
>   platforms skip straight through.
>
>   Splitting into is_supported() + is_ready() wouldn't change any caller's
>   behavior, but would require every caller to do '!supported() || ready()'
>   check. This risks a caller checking only is_ready(), resulting in
>   unnecessary retries or failures on a NOTSUPP platform.
>
>   Given the current usage, I'd prefer to keep the combined helper and add a
>   more granular status/support helper in the future if a real consumer
>   requires that distinction.
>
>   Thanks,
>   Anoop
>
>       + *
>       + * Return: true if diag firmware is initialized, or if System
>       Controller is
>       + * not present on this platform; false otherwise
>       + */
>       +bool xe_sysctrl_is_diag_fw_ready(struct xe_device *xe)
>       +{
>       +    enum xe_sysctrl_fw_status status =
>       +        xe_sysctrl_check_app_status(xe, XE_SYSCTRL_APP_DIAG);
>       +
>       +    return status == XE_SYSCTRL_FIRMWARE_APP_INITIALIZED ||
>       +           status == XE_SYSCTRL_FIRMWARE_APP_NOTSUPP;
>       +}
>       diff --git a/drivers/gpu/drm/xe/xe_sysctrl.h
>       b/drivers/gpu/drm/xe/xe_sysctrl.h
>       index 090dffb6d55f..8dc576796890 100644
>       --- a/drivers/gpu/drm/xe/xe_sysctrl.h
>       +++ b/drivers/gpu/drm/xe/xe_sysctrl.h
>       @@ -20,5 +20,6 @@ void xe_sysctrl_event(struct xe_sysctrl *sc);
>       int xe_sysctrl_init(struct xe_device *xe);
>       void xe_sysctrl_irq_handler(struct xe_device *xe, u32 master_ctl);
>       void xe_sysctrl_pm_resume(struct xe_device *xe);
>       +bool xe_sysctrl_is_diag_fw_ready(struct xe_device *xe);
>
>       #endif
>       diff --git a/drivers/gpu/drm/xe/xe_sysctrl_mailbox_types.h
>       b/drivers/gpu/drm/xe/xe_sysctrl_mailbox_types.h
>       index d0341538ad05..f1e4253fda4b 100644
>       --- a/drivers/gpu/drm/xe/xe_sysctrl_mailbox_types.h
>       +++ b/drivers/gpu/drm/xe/xe_sysctrl_mailbox_types.h
>       @@ -14,9 +14,11 @@
>        * enum xe_sysctrl_group - System Controller command groups
>        *
>        * @XE_SYSCTRL_GROUP_GFSP: GFSP group
>       + * @XE_SYSCTRL_GROUP_CORE: Core group
>        */
>       enum xe_sysctrl_group {
>           XE_SYSCTRL_GROUP_GFSP            = 0x01,
>       +    XE_SYSCTRL_GROUP_CORE            = 0xFF,
>       };
>
>       /**
>       @@ -38,6 +40,51 @@ enum xe_sysctrl_gfsp_cmd {
>           XE_SYSCTRL_CMD_SET_HEALTH        = 0x0C,
>       };
>
>       +/**
>       + * enum xe_sysctrl_core_cmd - Commands supported by Core group
>       + *
>       + * @XE_SYSCTRL_CMD_GET_APP_STATUS_BY_ID: Retrieve application status
>       by ID
>       + */
>       +enum xe_sysctrl_core_cmd {
>       +    XE_SYSCTRL_CMD_GET_APP_STATUS_BY_ID        = 0x05,
>       +};
>       +
>       +/**
>       + * struct xe_sysctrl_app_status_req - Get application status request
>       + *
>       + * @app_id: Application ID for which to retrieve status
>       + */
>       +struct xe_sysctrl_app_status_req {
>       +    u8 app_id;
>       +} __packed;
>       +
>       +/**
>       + * struct xe_sysctrl_app_status_resp - Get application status
>       response
>       + * @flags: Application status flags interpreted by
>       xe_sysctrl_check_app_status()
>       + */
>       +struct xe_sysctrl_app_status_resp {
>       +    u32 flags;
>       +} __packed;
>       +
>       +/**
>       + * enum xe_sysctrl_fw_status - System Controller firmware application
>       lifecycle states
>       + *
>       + * @XE_SYSCTRL_FIRMWARE_APP_INVALID: app_id is not recognized by
>       firmware
>       + * @XE_SYSCTRL_FIRMWARE_APP_NOT_LOADED: application is known but has
>       not yet booted
>       + * @XE_SYSCTRL_FIRMWARE_APP_BOOTED: boot sequence completed,
>       post-boot init pending
>       + * @XE_SYSCTRL_FIRMWARE_APP_INITIALIZED: application fully
>       operational
>       + * @XE_SYSCTRL_FIRMWARE_APP_NOTSUPP: System Controller not available
>       on this device
>       + * @XE_SYSCTRL_FIRMWARE_COMM_FAILURE: communication with System
>       Controller firmware failed
>       + */
>       +enum xe_sysctrl_fw_status {
>       +    XE_SYSCTRL_FIRMWARE_APP_INVALID,
>       +    XE_SYSCTRL_FIRMWARE_APP_NOT_LOADED,
>       +    XE_SYSCTRL_FIRMWARE_APP_BOOTED,
>       +    XE_SYSCTRL_FIRMWARE_APP_INITIALIZED,
>       +    XE_SYSCTRL_FIRMWARE_APP_NOTSUPP,
>       +    XE_SYSCTRL_FIRMWARE_COMM_FAILURE,
>       +};
>       +
>       /**
>        * struct xe_sysctrl_mailbox_command - System Controller mailbox
>       command
>        */
>       -- 
>       2.43.0