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

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

On 5/18/2026 10:56 PM, Raag Jadav wrote:
> On Thu, May 14, 2026 at 10:52:08AM +0530, Riana Tauro wrote:
>> Add request/response structures and helper functions to query system
>> controller to get error counter value.
> ...

I will change the mappings to switch. I was working on events and there is
a need of reverse mapping as well as default check.
So to have consistency, will change drm_ras_to_xe_ras_component and
drm_ras_to_xe_ras_severity to functions with switch.

>
>> +	xe_dbg(xe, "[RAS]: get counter value %u for %s %s\n", response.counter_value,
>> +	       comp_to_str(response.counter.common.component),
>> +	       sev_to_str(response.counter.common.severity));
> A bit heavy handed with nesting :D
> You can use a local counter which will also help remove the wrapping[1].
>
> [1] https://lore.kernel.org/intel-xe/20260512191610.1817578-7-raag.jadav@intel.com/

Sure will have a local pointer to response counter.

>
> ...
>
>> +/**
>> + * struct xe_ras_get_counter_request - Request for get error counter
>> + */
>> +struct xe_ras_get_counter_request {
>> +	/** @counter: Error counter to be queried */
>> +	struct xe_ras_error_class counter;
>> +	/** @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 {
>> +	/** @counter: Error counter that was queried */
>> +	struct xe_ras_error_class counter;
>> +	/** @counter_value: Current counter value */
>> +	u32 counter_value;
> Nit: It's already 'get_counter', so perhaps just 'value'?

sure

>
>> +	/** @timestamp: Timestamp when counter was last updated */
>> +	u64 timestamp;
>> +	/** @threshold_value: Threshold value for the counter */
>> +	u32 threshold_value;
> Ditto, 'threshold'?

sure.

>
>> +	/** @counter_status: Status of the counter */
>> +	u32 counter_status:8;
> 'status'?
>
> We should probably make these consistent across series', so let me know
> which ones you prefer.

Will rename.
The status here is not the same as set operations. This says 
active/threshold reached.
I did not add the enums as it did not indicate pass/failure and is 
unnecessary for
netlink operations.

Let me know if you want me to define those.


>
>> +	/** @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.c b/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c
>> index 3caa9f15875f..b7c4d8c37819 100644
>> --- a/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c
>> +++ b/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c
>> @@ -293,6 +293,34 @@ static int sysctrl_send_command(struct xe_sysctrl *sc,
>>   	return 0;
>>   }
>>   
>> +/**
>> + * xe_sysctrl_prepare_command() - Prepare System controller command structure
>> + * @command: Sysctrl command structure
>> + * @group: Command group ID
>> + * @cmd_id: Command code
>> + * @request: Pointer to request buffer (can be NULL)
>> + * @request_len: Size of request buffer
>> + * @response: Pointer to response buffer
>> + * @response_len: Size of response buffer
>> + *
>> + * Helper function to prepare sysctrl command to be sent via xe_sysctrl_send_command()
>> + */
>> +void xe_sysctrl_prepare_command(struct xe_sysctrl_mailbox_command *command, u8 group, u8 cmd_id,
> There's already a sysctrl_prepare_command() in place, so let's make this
> a bit distinguishable.

Oh..i did not see this. Thank you. will rename

Thanks
Riana

> PS: I've used xe_sysctrl_populate_command() in my series but I'll leave
> it to you.
>
> Raag
>
>> +				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, group) |
>> +		      FIELD_PREP(APP_HDR_COMMAND_MASK, cmd_id);
>> +
>> +	command->header = header;
>> +	command->data_in = request;
>> +	command->data_in_len = request_len;
>> +	command->data_out = response;
>> +	command->data_out_len = response_len;
>> +}