Re: [PATCH v1 2/2] drm/xe/xe_ras: Add RAS support for GPU health indicator

["Purkait, Soham" <soham.purkait@intel.com>]

On 21-04-2026 01:21, Rodrigo Vivi wrote:
> On Mon, Apr 20, 2026 at 05:26:33PM +0200, Andi Shyti wrote:
>> Hi Rodrigo,
>>
>>>>> +static const char * const gpu_health_states[] = { "ok", "warning", "critical" };
>>>>> +static const char * const gpu_health_fmt[] = {
>>>>> +	"[%s] %s %s\n",
>>>>> +	"%s [%s] %s\n",
>>>>> +	"%s %s [%s]\n",
>>>>> +};
>>>> Please, don't use complex sentences in sysfs outputs. Use a
>>>> single string/character/value
>>> I like this one better. So we don't need to have an uAPI entry to define
>>> the meaning of 0, 1, 2.
>>>
>>> Regarding the sysfs rules, as long as it is one entry per sysfs we should
>>> be compliant with the rule. So, we should be good here.
>>>
>>> This style is consistent with the style used in /sys/power/ entries for instance:
>> Well, if it's a sysfs interface, it's a uAPI entry and needs to
>> be documented (something that I don't see in this series, BTW).
> Indeed a good point. Soham, can you please provide a documentation
> in a next revision?

Sure.

Thanks,
Soham

>
>> The sysfs interfaces don't really need to be 1, 2, etc, they can
>> also be strings, as long as userspace applications are able to
>> easily parse the content and know what to expect from them.
> Which is the case here.
> We only have these 3 states that user space can easily parse:
> [ok] warning critical
> ok [warning] critical
> ok warning [critical]
>
>> If we need two strings, then we create two files.
> This part I don't agree. This is just the same as the power cases
> which I already had pointed out.
>
> But right, you do have a point here. Different from power, we don't
> necessarily need to expose all the supported modes + the current one.
> After all, these states won't be that different from state to state.
>
> We just need to document it properly and then just print:
> either
>
> ok
> instead of
> [ok] warning critical
>
> or
> warning
> instead of
> ok [warning] critical
>
> or critical
> instead of
> ok warning [critical]
>
> The full string with all the supported modes do sound bloated for
> this case. Considering that the supported cases themselves shouldn't
> change much and that we keep that documented. My bad, I'm sorry for
> the confusion here.
>
>> If we just need to provide information to system administrators,
>> then we can either use the debugfs or procfs that don't need to
>> be considered as uAPI.
> That is not true. In many data center deployments the 'debugfs'
> is not available and the admin and cluster tools still need this
> information. It needs to be stable API.
>
>> Andi
>>
>>> $ cat /sys/power/mem_sleep
>>> [s2idle] deep
>>>
>>> $ cat /sys/power/state
>>> freeze mem disk