Re: [PATCH v6 1/1] drm/xe/xe_ras: Add RAS GPU health indicator

[Raag Jadav <raag.jadav@intel.com>]

On Mon, Jul 06, 2026 at 10:22:09PM +0530, Soham Purkait wrote:
> Add a sysfs interface that reports the current GPU health state and
> lets admin users and management tools update it but is readable by all
> users. Requests are routed through the sysctrl mailbox. The interface
> is present only on platforms that support the GPU health indicator.
> 
> The interface is a single read/write file at the device level:
> 
>   $ cat /sys/.../device/gpu_health
>   ok
> 
>   $ echo critical > /sys/.../device/gpu_health
> 
>   $ cat /sys/.../device/gpu_health
>   critical

...

> +static const char * const gpu_health_states[] = {
> +	[XE_RAS_HEALTH_STATUS_OK]		= "ok",
> +	[XE_RAS_HEALTH_STATUS_WARNING]		= "warning",
> +	[XE_RAS_HEALTH_STATUS_CRITICAL]		= "critical"

'STATUS' is redundant and doesn't really add much to the meaning.

> +};
> +static_assert(ARRAY_SIZE(gpu_health_states) == XE_RAS_HEALTH_STATUS_MAX);
> +
>  static u8 drm_to_xe_ras_severity(u8 severity)
>  {
>  	switch (severity) {
> @@ -332,6 +339,150 @@ int xe_ras_clear_counter(struct xe_device *xe, u8 severity, u8 component)
>  	return 0;
>  }
>  
> +static ssize_t gpu_health_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct xe_ras_get_health_response response = {0};
> +	struct xe_sysctrl_mailbox_command command = {0};
> +	struct xe_ras_get_health_request request = {0};
> +	struct xe_device *xe = kdev_to_xe_device(dev);
> +	enum xe_ras_health_status health;
> +	size_t rlen = 0;

Does this need initialization?

> +	int ret;
> +
> +	xe_sysctrl_create_command(&command, XE_SYSCTRL_GROUP_GFSP, XE_SYSCTRL_CMD_GET_HEALTH,
> +				  &request, sizeof(request), &response, sizeof(response));
> +	guard(xe_pm_runtime)(xe);
> +	ret = xe_sysctrl_send_command(&xe->sc, &command, &rlen);
> +	if (ret) {
> +		xe_err(xe, "sysctrl: failed to send get health command %d\n", ret);

Make this consistent with existing logs, "failed to get health".

> +		return ret;
> +	}
> +
> +	if (rlen != sizeof(response)) {
> +		xe_err(xe, "sysctrl: unexpected get health response length %zu (expected %zu)\n",
> +		       rlen, sizeof(response));
> +		return -EIO;
> +	}
> +	if (response.current_health >= XE_RAS_HEALTH_STATUS_MAX) {
> +		xe_err(xe, "sysctrl: invalid health state %u\n",
> +		       response.current_health);
> +		return -EIO;
> +	}
> +
> +	health = (enum xe_ras_health_status)response.current_health;

Looking at below code, it's just used as an array index. So does this
need to be an enum?

> +	xe_dbg(xe, "[RAS]: get health:%s\n", gpu_health_states[health]);

Nit: Add a blank space after ':'

> +	return sysfs_emit(buf, "%s\n", gpu_health_states[health]);
> +}
> +
> +static ssize_t gpu_health_store(struct device *dev, struct device_attribute *attr,
> +				const char *buf, size_t count)
> +{
> +	struct xe_ras_set_health_response response = {0};
> +	struct xe_sysctrl_mailbox_command command = {0};
> +	struct xe_ras_set_health_request request = {0};
> +	struct xe_device *xe = kdev_to_xe_device(dev);
> +	enum xe_ras_health_status health;
> +	size_t rlen = 0;

Does this need initialization?

> +	int ras_status;
> +	int state;
> +	int ret;
> +
> +	state = sysfs_match_string(gpu_health_states, buf);
> +	if (state < 0) {
> +		xe_err(xe, "[RAS]: invalid health state '%.*s'\n",
> +		       (int)strcspn(buf, "\n"), buf);

Why do we need this log? Doesn't user already get an error?

> +		return -EINVAL;
> +	}
> +
> +	request.new_health = (u8)state;

Is this cast needed? Doesn't this happen implicitly?

> +	xe_sysctrl_create_command(&command, XE_SYSCTRL_GROUP_GFSP, XE_SYSCTRL_CMD_SET_HEALTH,
> +				  &request, sizeof(request), &response, sizeof(response));
> +	guard(xe_pm_runtime)(xe);
> +	ret = xe_sysctrl_send_command(&xe->sc, &command, &rlen);
> +	if (ret) {
> +		xe_err(xe, "sysctrl: failed to send set health command %d\n", ret);

Make this consistent with existing logs, "failed to set health".

> +		return ret;
> +	}
> +
> +	if (rlen != sizeof(response)) {
> +		xe_err(xe, "sysctrl: unexpected set health response length %zu (expected %zu)\n",
> +		       rlen, sizeof(response));
> +		return -EIO;
> +	}
> +
> +	ras_status = ras_status_to_errno(response.status);

Why not reuse 'ret' here?

> +	if (ras_status) {
> +		xe_err(xe, "sysctrl: set health command failed with status %d\n",

Use '%#x' format.

> +		       response.status);
> +		return ras_status;
> +	}
> +
> +	if (response.current_health >= XE_RAS_HEALTH_STATUS_MAX) {
> +		xe_err(xe, "sysctrl: invalid health state %u\n",
> +		       response.current_health);

Does this need to be an error? Most likely the driver can't do much about
it anyway.

> +		return -EIO;
> +	}
> +
> +	health = (enum xe_ras_health_status)response.current_health;

Same as above.

> +	xe_dbg(xe, "[RAS]: set health:%s\n", gpu_health_states[health]);

Same as above.

> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(gpu_health);
> +
> +static void gpu_health_sysfs_fini(void *arg)
> +{
> +	struct device *dev = arg;
> +
> +	device_remove_file(dev, &dev_attr_gpu_health);
> +}
> +
> +/**
> + * DOC: GPU Health Indicator
> + *
> + * On Intel Xe platforms that support the GPU health indicator interface,
> + * the driver exposes a sysfs attribute at the device level as:
> + *
> + *	``/sys/bus/pci/drivers/xe/<BDF>/gpu_health``
> + *
> + * Reading the attribute returns a single line containing the current GPU
> + * health state. Writing one of the valid values updates the same. Reads
> + * are available to all users; writes are restricted to administrative
> + * users only (attribute permissions ``0644``).
> + *
> + * The valid values for the GPU health state are:
> + *
> + * - ``ok``:
> + *	The GPU is healthy and operating within normal parameters.
> + * - ``warning``:
> + *	The GPU is experiencing minor issues but remains operational.
> + * - ``critical``:
> + *	The GPU is in a critical state and may not be operational.
> + *
> + * See Documentation/ABI/testing/sysfs-driver-intel-xe-ras for the ABI
> + * specification.
> + */
> +static int gpu_health_sysfs_init(struct xe_device *xe)
> +{
> +	struct device *dev = xe->drm.dev;
> +	int err;
> +
> +	err = device_create_file(dev, &dev_attr_gpu_health);
> +	if (err)
> +		return err;
> +
> +	err = devm_add_action_or_reset(dev, gpu_health_sysfs_fini, dev);

Why not use devm_device_add_group() and do away with manual cleanup?

> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
>  /**
>   * xe_ras_init - Initialize Xe RAS
>   * @xe: xe device instance
> @@ -340,6 +491,8 @@ int xe_ras_clear_counter(struct xe_device *xe, u8 severity, u8 component)
>   */
>  void xe_ras_init(struct xe_device *xe)
>  {
> +	int ret;
> +
>  	if (!xe->info.has_drm_ras)
>  		return;
>  
> @@ -350,4 +503,8 @@ void xe_ras_init(struct xe_device *xe)
>  
>  	if (IS_ENABLED(CONFIG_PCIEAER))
>  		ras_usp_aer_init(xe);
> +
> +	ret = gpu_health_sysfs_init(xe);
> +	if (ret)
> +		xe_err(xe, "[RAS]: failed to initialize GPU health sysfs, err=%d\n", ret);

Is '[RAS]' appropriate here? This is not coming from any hardware
component.

>  }
> diff --git a/drivers/gpu/drm/xe/xe_ras_types.h b/drivers/gpu/drm/xe/xe_ras_types.h
> index 6688e11f57a8..dac0e6d2270c 100644
> --- a/drivers/gpu/drm/xe/xe_ras_types.h
> +++ b/drivers/gpu/drm/xe/xe_ras_types.h
> @@ -10,6 +10,21 @@
>  
>  #define XE_RAS_NUM_COUNTERS			16
>  
> +/**
> + * enum xe_ras_health_status - gpu health status values
> + *
> + * @XE_RAS_HEALTH_STATUS_OK: gpu is healthy and operating normally.
> + * @XE_RAS_HEALTH_STATUS_WARNING: gpu has minor issues but is still operational.
> + * @XE_RAS_HEALTH_STATUS_CRITICAL: gpu is in a critical state and may not be operational.
> + * @XE_RAS_HEALTH_STATUS_MAX: Sentinel value for validation
> + */
> +enum xe_ras_health_status {
> +	XE_RAS_HEALTH_STATUS_OK = 0,
> +	XE_RAS_HEALTH_STATUS_WARNING,
> +	XE_RAS_HEALTH_STATUS_CRITICAL,
> +	XE_RAS_HEALTH_STATUS_MAX
> +};
> +
>  /**
>   * struct xe_ras_error_common - Error fields that are common across all products
>   */
> @@ -121,4 +136,46 @@ struct xe_ras_clear_counter_response {
>  	/** @reserved1: Reserved for future use */
>  	u32 reserved1[3];
>  } __packed;
> +
> +/**
> + * struct xe_ras_get_health_request - Request structure for obtaining gpu health
> + */
> +struct xe_ras_get_health_request {
> +	/** @reserved: Reserved for future use. */
> +	u32 reserved[2];
> +} __packed;
> +
> +/**
> + * struct xe_ras_get_health_response - Response structure for obtaining gpu health
> + */
> +struct xe_ras_get_health_response {
> +	/** @current_health: Current gpu health, see &enum xe_ras_health_status */
> +	u8 current_health;

It's already part of get_health structure, so can be just 'health'.

> +	/** @reserved: Reserved for future use */
> +	u8 reserved[3];
> +} __packed;
> +
> +/**
> + * struct xe_ras_set_health_request - Request structure for setting gpu health
> + */
> +struct xe_ras_set_health_request {
> +	/** @new_health: New gpu health to set, see &enum xe_ras_health_status */
> +	u8 new_health;

Ditto.

> +	/** @reserved: Reserved for future use */
> +	u8 reserved[3];
> +} __packed;
> +
> +/**
> + * struct xe_ras_set_health_response - Response structure for setting gpu health
> + */
> +struct xe_ras_set_health_response {
> +	/** @status: Status of set health operation, see &enum xe_ras_response_status */
> +	u32 status;
> +	/** @current_health: Resulting current gpu health, see &enum xe_ras_health_status */
> +	u8 current_health;

Ditto.

Raag

> +	/** @reserved: Reserved for future use */
> +	u8 reserved[3];
> +	/** @reserved1: Reserved for future use */
> +	u32 reserved1[2];
> +} __packed;
>  #endif
> diff --git a/drivers/gpu/drm/xe/xe_sysctrl_mailbox_types.h b/drivers/gpu/drm/xe/xe_sysctrl_mailbox_types.h
> index 6e3753554510..d4da6e9ef774 100644
> --- a/drivers/gpu/drm/xe/xe_sysctrl_mailbox_types.h
> +++ b/drivers/gpu/drm/xe/xe_sysctrl_mailbox_types.h
> @@ -25,11 +25,15 @@ enum xe_sysctrl_group {
>   * @XE_SYSCTRL_CMD_GET_COUNTER: Get error counter value
>   * @XE_SYSCTRL_CMD_CLEAR_COUNTER: Clear error counter value
>   * @XE_SYSCTRL_CMD_GET_PENDING_EVENT: Retrieve pending event
> + * @XE_SYSCTRL_CMD_GET_HEALTH: Retrieve current health status
> + * @XE_SYSCTRL_CMD_SET_HEALTH: Set new health status
>   */
>  enum xe_sysctrl_gfsp_cmd {
>  	XE_SYSCTRL_CMD_GET_COUNTER		= 0x03,
>  	XE_SYSCTRL_CMD_CLEAR_COUNTER		= 0x04,
>  	XE_SYSCTRL_CMD_GET_PENDING_EVENT	= 0x07,
> +	XE_SYSCTRL_CMD_GET_HEALTH		= 0x0B,
> +	XE_SYSCTRL_CMD_SET_HEALTH		= 0x0C,
>  };
>  
>  /**
> -- 
> 2.43.0
>