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/25/2026 7:43 PM, Tauro, Riana wrote:
>
> 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.


I don't see any benefit of having a local counter.  Even if i have a 
resp_counter or
common. I still see a wrapping here.

Retaining the same.

Will change in clear_counter.

Thanks
Riana

>
>>
>> ...
>>
>>> +/**
>>> + * 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;
>>> +}