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

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.

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