Re: [PATCH v5 2/6] drm/xe/xe_ras: Add support to get error counter in CRI

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

On 5/12/2026 11:17 AM, Raag Jadav wrote:
> On Tue, May 12, 2026 at 10:57:50AM +0530, Tauro, Riana wrote:
>> On 5/11/2026 8:57 PM, Raag Jadav wrote:
>>> On Mon, May 04, 2026 at 12:26:17PM +0530, Riana Tauro wrote:
>>>> Add request/response structures and helper functions to query system
>>>> controller to get error counter value.
>>>>
>>>> Signed-off-by: Riana Tauro <riana.tauro@intel.com>
>>>> ---
>>>> v2: add structures for clear counter
>>>>       move commands to sysctrl file
>>>>       split functions
>>>>       fix commit message (Raag)
>>>>
>>>> v3: fix log message
>>>>       squash patches
>>>>       change error code for sysctrl error (Raag)
>>>>
>>>> v4: rename function
>>>>       remove unecessary macro (Raag)
>>>>       add documentation for enum
>>>>
>>>> v5: rebase
>>>> ---
>>>>    drivers/gpu/drm/xe/xe_ras.c                   | 91 +++++++++++++++++++
>>>>    drivers/gpu/drm/xe/xe_ras.h                   |  4 +
>>>>    drivers/gpu/drm/xe/xe_ras_types.h             | 30 ++++++
>>>>    drivers/gpu/drm/xe/xe_sysctrl_mailbox_types.h |  2 +
>>>>    4 files changed, 127 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/xe/xe_ras.c b/drivers/gpu/drm/xe/xe_ras.c
>>>> index 4cb16b419b0c..47a58ce3b3ca 100644
>>>> --- a/drivers/gpu/drm/xe/xe_ras.c
>>>> +++ b/drivers/gpu/drm/xe/xe_ras.c
>>>> @@ -4,11 +4,14 @@
>>>>     */
>>>>    #include "xe_device.h"
>>>> +#include "xe_pm.h"
>>>>    #include "xe_printk.h"
>>>>    #include "xe_ras.h"
>>>>    #include "xe_ras_types.h"
>>>>    #include "xe_sysctrl.h"
>>>>    #include "xe_sysctrl_event_types.h"
>>>> +#include "xe_sysctrl_mailbox.h"
>>>> +#include "xe_sysctrl_mailbox_types.h"
>>>>    /* Severity of detected errors  */
>>>>    enum xe_ras_severity {
>>>> @@ -50,6 +53,23 @@ static const char *const xe_ras_components[] = {
>>>>    };
>>>>    static_assert(ARRAY_SIZE(xe_ras_components) == XE_RAS_COMP_MAX);
>>>> +/* Mapping from drm_xe_ras_error_component to xe_ras_component */
>>>> +static const int drm_to_xe_ras_component[] = {
>>>> +	[DRM_XE_RAS_ERR_COMP_CORE_COMPUTE]	= XE_RAS_COMP_CORE_COMPUTE,
>>>> +	[DRM_XE_RAS_ERR_COMP_SOC_INTERNAL]	= XE_RAS_COMP_SOC_INTERNAL,
>>>> +	[DRM_XE_RAS_ERR_COMP_DEVICE_MEMORY]	= XE_RAS_COMP_DEVICE_MEMORY,
>>>> +	[DRM_XE_RAS_ERR_COMP_PCIE]		= XE_RAS_COMP_PCIE,
>>>> +	[DRM_XE_RAS_ERR_COMP_FABRIC]		= XE_RAS_COMP_FABRIC
>>>> +};
>>>> +static_assert(ARRAY_SIZE(drm_to_xe_ras_component) == DRM_XE_RAS_ERR_COMP_MAX);
>>>> +
>>>> +/* Mapping from drm_xe_ras_error_severity to xe_ras_severity */
>>>> +static const int drm_to_xe_ras_severity[] = {
>>>> +	[DRM_XE_RAS_ERR_SEV_CORRECTABLE]	= XE_RAS_SEV_CORRECTABLE,
>>>> +	[DRM_XE_RAS_ERR_SEV_UNCORRECTABLE]	= XE_RAS_SEV_UNCORRECTABLE
>>>> +};
>>>> +static_assert(ARRAY_SIZE(drm_to_xe_ras_severity) == DRM_XE_RAS_ERR_SEV_MAX);
>>> So we don't accept new entries unless also added in uapi and vice versa
>>> which is good, but if you feel the need to have bounds checking just
>>> switch() instead.
>> If i move this to switch, there will be inconsistency with the component
>> array.
> With switch() I think you won't need an array ...
>
>> The component array has multiple entries so array is preferable.
>> Also bounds check is already done in upper layers for this so not needed.
> ... but fair.
>
>>>>    static inline const char *sev_to_str(u8 severity)
>>>>    {
>>>>    	if (severity >= XE_RAS_SEV_MAX)
>>>> @@ -66,6 +86,22 @@ static inline const char *comp_to_str(u8 component)
>>>>    	return xe_ras_components[component];
>>>>    }
>>>> +static void prepare_ras_command(struct xe_sysctrl_mailbox_command *command,
>>> This looks like it should be a sysctrl helper (for non-RAS mailbox users).
>> We do not have any non-ras users. We can move if we need it
> I thought we did[1], but I might be missing something.
>
> [1] https://lore.kernel.org/intel-xe/20260320072528.1780651-12-anoop.c.vijay@intel.com/

The header is different here. We can move it when it's actually used.

>
>>>> +				u32 cmd, void *request, size_t request_len,
>>>> +				void *response, size_t response_len)
>>>> +{
>>>> +	struct xe_sysctrl_app_msg_hdr header = {0};
>>>> +
>>>> +	header.data = FIELD_PREP(APP_HDR_GROUP_ID_MASK, XE_SYSCTRL_GROUP_GFSP) |
>>>> +		      FIELD_PREP(APP_HDR_COMMAND_MASK, cmd);
>>>> +
>>>> +	command->header = header;
>>>> +	command->data_in = request;
>>>> +	command->data_in_len = request_len;
>>>> +	command->data_out = response;
>>>> +	command->data_out_len = response_len;
>>>> +}
>>>> +
>>>>    void xe_ras_counter_threshold_crossed(struct xe_device *xe,
>>>>    				      struct xe_sysctrl_event_response *response)
>>>>    {
>>>> @@ -91,3 +127,58 @@ void xe_ras_counter_threshold_crossed(struct xe_device *xe,
>>>>    			comp_to_str(component), sev_to_str(severity));
>>>>    	}
>>>>    }
>>>> +
>>>> +static int get_counter(struct xe_device *xe, struct xe_ras_error_class *error_class,
>>> s/error_class/counter
>>>
>>> Single word parameters usually help with wrapping but ofcourse it's a
>>> personal preference.
>> okay
>>>> +		       u32 *value)
>>>> +{
>>>> +	struct xe_ras_get_counter_response response = {0};
>>>> +	struct xe_ras_get_counter_request request = {0};
>>>> +	struct xe_sysctrl_mailbox_command command = {0};
>>>> +	size_t rlen;
>>>> +	int ret;
>>>> +
>>>> +	request.error_class = *error_class;
>>>> +
>>>> +	prepare_ras_command(&command, XE_SYSCTRL_CMD_GET_COUNTER, &request, sizeof(request),
>>>> +			    &response, sizeof(response));
>>>> +
>>>> +	ret = xe_sysctrl_send_command(&xe->sc, &command, &rlen);
>>>> +	if (ret) {
>>>> +		xe_err(xe, "sysctrl: failed to get counter %d\n", ret);
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	if (rlen != sizeof(response)) {
>>>> +		xe_err(xe, "sysctrl: unexpected get counter response length %zu (expected %zu)\n",
>>>> +		       rlen, sizeof(response));
>>>> +		return -EIO;
>>>> +	}
>>>> +
>>>> +	*value = response.counter_value;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +/**
>>>> + * xe_ras_get_counter() - Get error counter value
>>>> + * @xe: xe device instance
>>>> + * @severity: Error severity level to be queried
>>>> + * @error_id: Error component to be queried
>>>> + * @value: Counter value
>>>> + *
>>>> + * This function retrieves the value of a specific error counter based on
>>>> + * the error severity and component.
>>>> + *
>>>> + * Return: 0 on success, negative error code on failure.
>>>> + */
>>>> +int xe_ras_get_counter(struct xe_device *xe, enum drm_xe_ras_error_severity severity,
>>> const for consistency.
>> We do not need const since it's passed by value
>> consistency ? the upper layer does not have a const
> Then perhaps u32? Anyway, I'll leave it to you.

We have both ras severity and drm uapi. So there might be a confusion.
So i have added the explicit enum. If anyone uses this function, they 
will know
what to use based on function parameter.

Thanks
Riana

>
> Raag
>
>>>> +		       u32 error_id, u32 *value)
>>>> +{
>>>> +	struct xe_ras_error_class error_class = {0};
>>>> +
>>>> +	error_class.common.severity = drm_to_xe_ras_severity[severity];
>>>> +	error_class.common.component = drm_to_xe_ras_component[error_id];
>>>> +
>>>> +	guard(xe_pm_runtime)(xe);
>>>> +	return get_counter(xe, &error_class, value);
>>>> +}
>>>> diff --git a/drivers/gpu/drm/xe/xe_ras.h b/drivers/gpu/drm/xe/xe_ras.h
>>>> index ea90593b62dc..74582c911b02 100644
>>>> --- a/drivers/gpu/drm/xe/xe_ras.h
>>>> +++ b/drivers/gpu/drm/xe/xe_ras.h
>>>> @@ -6,10 +6,14 @@
>>>>    #ifndef _XE_RAS_H_
>>>>    #define _XE_RAS_H_
>>>> +#include <uapi/drm/xe_drm.h>
>>>> +
>>>>    struct xe_device;
>>>>    struct xe_sysctrl_event_response;
>>>>    void xe_ras_counter_threshold_crossed(struct xe_device *xe,
>>>>    				      struct xe_sysctrl_event_response *response);
>>>> +int xe_ras_get_counter(struct xe_device *xe, enum drm_xe_ras_error_severity severity,
>>>> +		       u32 error_id, u32 *value);
>>>>    #endif
>>>> diff --git a/drivers/gpu/drm/xe/xe_ras_types.h b/drivers/gpu/drm/xe/xe_ras_types.h
>>>> index 4e63c67f806a..74d85875cd63 100644
>>>> --- a/drivers/gpu/drm/xe/xe_ras_types.h
>>>> +++ b/drivers/gpu/drm/xe/xe_ras_types.h
>>>> @@ -70,4 +70,34 @@ struct xe_ras_threshold_crossed {
>>>>    	struct xe_ras_error_class counters[XE_RAS_NUM_COUNTERS];
>>>>    } __packed;
>>>> +/**
>>>> + * struct xe_ras_get_counter_request - Request for get error counter
>>>> + */
>>>> +struct xe_ras_get_counter_request {
>>>> +	/** @error_class: Error class counter to be queried */
>>>> +	struct xe_ras_error_class error_class;
>>>> +	/** @reserved: Reserved for future use */
>>>> +	u32 reserved;
>>>> +} __packed;
>>>> +
>>>> +/**
>>>> + * struct xe_ras_get_counter_response - Response for get error counter
>>>> + */
>>>> +struct xe_ras_get_counter_response {
>>>> +	/** @error_class: Error class counter that was queried */
>>>> +	struct xe_ras_error_class error_class;
>>>> +	/** @counter_value: Current counter value */
>>>> +	u32 counter_value;
>>>> +	/** @timestamp: Timestamp when counter was last updated */
>>>> +	u64 timestamp;
>>>> +	/** @threshold_value: Threshold value for the counter */
>>>> +	u32 threshold_value;
>>>> +	/** @counter_status: Status of the counter */
>>>> +	u32 counter_status:8;
>>>> +	/** @reserved: Reserved for future use */
>>>> +	u32 reserved:24;
>>>> +	/** @reserved1: Reserved for future use */
>>>> +	u32 reserved1[56];
>>>> +} __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 84d7c647e743..b315847cbf64 100644
>>>> --- a/drivers/gpu/drm/xe/xe_sysctrl_mailbox_types.h
>>>> +++ b/drivers/gpu/drm/xe/xe_sysctrl_mailbox_types.h
>>>> @@ -22,9 +22,11 @@ enum xe_sysctrl_group {
>>>>    /**
>>>>     * enum xe_sysctrl_gfsp_cmd - Commands supported by GFSP group
>>>>     *
>>>> + * @XE_SYSCTRL_CMD_GET_COUNTER: Get error counter value
>>>>     * @XE_SYSCTRL_CMD_GET_PENDING_EVENT: Retrieve pending event
>>>>     */
>>>>    enum xe_sysctrl_gfsp_cmd {
>>>> +	XE_SYSCTRL_CMD_GET_COUNTER		= 0x03,
>>>>    	XE_SYSCTRL_CMD_GET_PENDING_EVENT	= 0x07,
>>>>    };
>>>> -- 
>>>> 2.47.1
>>>>