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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

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?

> 
> 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