unparseable, undocumented /sys/class/drm/.../pstate

unparseable, undocumented /sys/class/drm/.../pstate

[Pavel Machek]

Hi!

AFAICT, pstate file will contain something like

07: core 100 MHz memory 123 MHz *
08: core 100-200 MHz memory 123 MHz

...which does not look exactly like one-value-per-file, and I'm pretty
sure userspace will get it wrong if it tries to parse it. Plus, I
don't see required documentation in Documentation/ABI.

Should we disable it for now, so that userspace does not start
depending on it and we'll not have to maintain it forever?

I guess better interface would be something like

pstate/07/core_clock_min
          core_clock_max
	  memory_clock_min
	  memory_clock_max

and then pstate/active containing just the number of active state?

Thanks,
								Pavel

PS: I have no nvidia, got the news at

http://www.phoronix.com/scan.php?page=article&item=nouveau_try_linux316&num=2
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: unparseable, undocumented /sys/class/drm/.../pstate

[Ilia Mirkin]

On Sat, Jun 21, 2014 at 2:02 PM, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
> AFAICT, pstate file will contain something like
>
> 07: core 100 MHz memory 123 MHz *
> 08: core 100-200 MHz memory 123 MHz
>
> ...which does not look exactly like one-value-per-file, and I'm pretty
> sure userspace will get it wrong if it tries to parse it. Plus, I
> don't see required documentation in Documentation/ABI.
>
> Should we disable it for now, so that userspace does not start
> depending on it and we'll not have to maintain it forever?
>
> I guess better interface would be something like
>
> pstate/07/core_clock_min
>           core_clock_max
>           memory_clock_min
>           memory_clock_max
>
> and then pstate/active containing just the number of active state?
>
> Thanks,
>                                                                 Pavel
>
> PS: I have no nvidia, got the news at
>
> http://www.phoronix.com/scan.php?page=article&item=nouveau_try_linux316&num=2

FTR, this file has been in place since 3.13, and there was a different
file before it (performance_levels), with a comparable format since
much earlier (definitely 3.8, probably earlier). I think it's meant a
lot more for people looking at it and echo'ing stuff to it to modify
the levels (where supported), than for programs parsing it. Perhaps
sysfs is the wrong place for this -- what is the right place? debugfs?

  -ilia

Re: unparseable, undocumented /sys/class/drm/.../pstate

[Pavel Machek]

On Sat 2014-06-21 14:22:59, Ilia Mirkin wrote:
> On Sat, Jun 21, 2014 at 2:02 PM, Pavel Machek <pavel@ucw.cz> wrote:
> > Hi!
> >
> > AFAICT, pstate file will contain something like
> >
> > 07: core 100 MHz memory 123 MHz *
> > 08: core 100-200 MHz memory 123 MHz
> >
> > ...which does not look exactly like one-value-per-file, and I'm pretty
> > sure userspace will get it wrong if it tries to parse it. Plus, I
> > don't see required documentation in Documentation/ABI.
...
> 
> FTR, this file has been in place since 3.13, and there was a different
> file before it (performance_levels), with a comparable format since
> much earlier (definitely 3.8, probably earlier). I think it's meant
> a

According to the article, it is only starting to work now. I know
articles can be wrong, but I don't have that hardware... Sorry if it
is the case.

> lot more for people looking at it and echo'ing stuff to it to modify
> the levels (where supported), than for programs parsing it. Perhaps
> sysfs is the wrong place for this -- what is the right place? debugfs?

debugfs would work, yes.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: unparseable, undocumented /sys/class/drm/.../pstate

[Ilia Mirkin]

On Sat, Jun 21, 2014 at 2:50 PM, Pavel Machek <pavel@ucw.cz> wrote:
> On Sat 2014-06-21 14:22:59, Ilia Mirkin wrote:
>> On Sat, Jun 21, 2014 at 2:02 PM, Pavel Machek <pavel@ucw.cz> wrote:
>> > Hi!
>> >
>> > AFAICT, pstate file will contain something like
>> >
>> > 07: core 100 MHz memory 123 MHz *
>> > 08: core 100-200 MHz memory 123 MHz
>> >
>> > ...which does not look exactly like one-value-per-file, and I'm pretty
>> > sure userspace will get it wrong if it tries to parse it. Plus, I
>> > don't see required documentation in Documentation/ABI.
> ...
>>
>> FTR, this file has been in place since 3.13, and there was a different
>> file before it (performance_levels), with a comparable format since
>> much earlier (definitely 3.8, probably earlier). I think it's meant
>> a
>
> According to the article, it is only starting to work now. I know
> articles can be wrong, but I don't have that hardware... Sorry if it
> is the case.

Commit 26fdd78cce3f51a49e1f2d3ad27ee893a28d220e introduced this particular one.
Commit 330c5988ee78045e6a731c3693251aaa5b0d14e3 had introduced the
former version, which was removed in 3.13, replaced with the new one.

>
>> lot more for people looking at it and echo'ing stuff to it to modify
>> the levels (where supported), than for programs parsing it. Perhaps
>> sysfs is the wrong place for this -- what is the right place? debugfs?
>
> debugfs would work, yes.
>                                                                         Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: unparseable, undocumented /sys/class/drm/.../pstate

[Greg KH]

On Sat, Jun 21, 2014 at 02:22:59PM -0400, Ilia Mirkin wrote:
> On Sat, Jun 21, 2014 at 2:02 PM, Pavel Machek <pavel@ucw.cz> wrote:
> > Hi!
> >
> > AFAICT, pstate file will contain something like
> >
> > 07: core 100 MHz memory 123 MHz *
> > 08: core 100-200 MHz memory 123 MHz
> >
> > ...which does not look exactly like one-value-per-file, and I'm pretty
> > sure userspace will get it wrong if it tries to parse it. Plus, I
> > don't see required documentation in Documentation/ABI.
> >
> > Should we disable it for now, so that userspace does not start
> > depending on it and we'll not have to maintain it forever?
> >
> > I guess better interface would be something like
> >
> > pstate/07/core_clock_min
> >           core_clock_max
> >           memory_clock_min
> >           memory_clock_max
> >
> > and then pstate/active containing just the number of active state?
> >
> > Thanks,
> >                                                                 Pavel
> >
> > PS: I have no nvidia, got the news at
> >
> > http://www.phoronix.com/scan.php?page=article&item=nouveau_try_linux316&num=2
> 
> FTR, this file has been in place since 3.13, and there was a different
> file before it (performance_levels), with a comparable format since
> much earlier (definitely 3.8, probably earlier). I think it's meant a
> lot more for people looking at it and echo'ing stuff to it to modify
> the levels (where supported), than for programs parsing it. Perhaps
> sysfs is the wrong place for this -- what is the right place? debugfs?

Yes, please move it to debugfs.

greg k-h

Re: unparseable, undocumented /sys/class/drm/.../pstate

[Ilia Mirkin]

On Sat, Jun 21, 2014 at 3:45 PM, Greg KH <greg@kroah.com> wrote:
> On Sat, Jun 21, 2014 at 02:22:59PM -0400, Ilia Mirkin wrote:
>> On Sat, Jun 21, 2014 at 2:02 PM, Pavel Machek <pavel@ucw.cz> wrote:
>> > Hi!
>> >
>> > AFAICT, pstate file will contain something like
>> >
>> > 07: core 100 MHz memory 123 MHz *
>> > 08: core 100-200 MHz memory 123 MHz
>> >
>> > ...which does not look exactly like one-value-per-file, and I'm pretty
>> > sure userspace will get it wrong if it tries to parse it. Plus, I
>> > don't see required documentation in Documentation/ABI.
>> >
>> > Should we disable it for now, so that userspace does not start
>> > depending on it and we'll not have to maintain it forever?
>> >
>> > I guess better interface would be something like
>> >
>> > pstate/07/core_clock_min
>> >           core_clock_max
>> >           memory_clock_min
>> >           memory_clock_max
>> >
>> > and then pstate/active containing just the number of active state?
>> >
>> > Thanks,
>> >                                                                 Pavel
>> >
>> > PS: I have no nvidia, got the news at
>> >
>> > http://www.phoronix.com/scan.php?page=article&item=nouveau_try_linux316&num=2
>>
>> FTR, this file has been in place since 3.13, and there was a different
>> file before it (performance_levels), with a comparable format since
>> much earlier (definitely 3.8, probably earlier). I think it's meant a
>> lot more for people looking at it and echo'ing stuff to it to modify
>> the levels (where supported), than for programs parsing it. Perhaps
>> sysfs is the wrong place for this -- what is the right place? debugfs?
>
> Yes, please move it to debugfs.

Could we just say that the format of this file is one-per-line of

level: information-for-the-user

And you can echo a level into it to switch to that level? That seems
like a reasonable ABI to have... would be happy to throw it into a
file somewhere... not sure where though.

  -ilia

Re: unparseable, undocumented /sys/class/drm/.../pstate

[Pavel Machek]

On Sun 2014-06-22 22:12:14, Ilia Mirkin wrote:
> On Sat, Jun 21, 2014 at 3:45 PM, Greg KH <greg@kroah.com> wrote:
> > On Sat, Jun 21, 2014 at 02:22:59PM -0400, Ilia Mirkin wrote:
> >> On Sat, Jun 21, 2014 at 2:02 PM, Pavel Machek <pavel@ucw.cz> wrote:
> >> > Hi!
> >> >
> >> > AFAICT, pstate file will contain something like
> >> >
> >> > 07: core 100 MHz memory 123 MHz *
> >> > 08: core 100-200 MHz memory 123 MHz
> >> >
> >> > ...which does not look exactly like one-value-per-file, and I'm pretty
> >> > sure userspace will get it wrong if it tries to parse it. Plus, I
> >> > don't see required documentation in Documentation/ABI.
> >> >
> >> > Should we disable it for now, so that userspace does not start
> >> > depending on it and we'll not have to maintain it forever?
> >> >
> >> > I guess better interface would be something like
> >> >
> >> > pstate/07/core_clock_min
> >> >           core_clock_max
> >> >           memory_clock_min
> >> >           memory_clock_max
> >> >
> >> > and then pstate/active containing just the number of active state?

> Could we just say that the format of this file is one-per-line of
> 
> level: information-for-the-user

But it is not. Management tools will want to parse it, sooner or
later.  What is wrong with solution described above?

> And you can echo a level into it to switch to that level? That seems
> like a reasonable ABI to have... would be happy to throw it into a
> file somewhere... not sure where though.

Documentation/ABI/testing/

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: unparseable, undocumented /sys/class/drm/.../pstate

[Ilia Mirkin]

On Mon, Jun 23, 2014 at 9:02 AM, Pavel Machek <pavel@ucw.cz> wrote:
> On Sun 2014-06-22 22:12:14, Ilia Mirkin wrote:
>> On Sat, Jun 21, 2014 at 3:45 PM, Greg KH <greg@kroah.com> wrote:
>> > On Sat, Jun 21, 2014 at 02:22:59PM -0400, Ilia Mirkin wrote:
>> >> On Sat, Jun 21, 2014 at 2:02 PM, Pavel Machek <pavel@ucw.cz> wrote:
>> >> > Hi!
>> >> >
>> >> > AFAICT, pstate file will contain something like
>> >> >
>> >> > 07: core 100 MHz memory 123 MHz *
>> >> > 08: core 100-200 MHz memory 123 MHz
>> >> >
>> >> > ...which does not look exactly like one-value-per-file, and I'm pretty
>> >> > sure userspace will get it wrong if it tries to parse it. Plus, I
>> >> > don't see required documentation in Documentation/ABI.
>> >> >
>> >> > Should we disable it for now, so that userspace does not start
>> >> > depending on it and we'll not have to maintain it forever?
>> >> >
>> >> > I guess better interface would be something like
>> >> >
>> >> > pstate/07/core_clock_min
>> >> >           core_clock_max
>> >> >           memory_clock_min
>> >> >           memory_clock_max
>> >> >
>> >> > and then pstate/active containing just the number of active state?
>
>> Could we just say that the format of this file is one-per-line of
>>
>> level: information-for-the-user
>
> But it is not.

But it is...

> Management tools will want to parse it, sooner or
> later.  What is wrong with solution described above?

It is complex and annoying to the people that will actually use it.

>
>> And you can echo a level into it to switch to that level? That seems
>> like a reasonable ABI to have... would be happy to throw it into a
>> file somewhere... not sure where though.
>
> Documentation/ABI/testing/

Yes, I got that far. And then I became confused.

  -ilia

Re: unparseable, undocumented /sys/class/drm/.../pstate

[Pavel Machek]

Hi!

> >> >> > I guess better interface would be something like
> >> >> >
> >> >> > pstate/07/core_clock_min
> >> >> >           core_clock_max
> >> >> >           memory_clock_min
> >> >> >           memory_clock_max
> >> >> >
> >> >> > and then pstate/active containing just the number of active state?
> >
> >> Could we just say that the format of this file is one-per-line of
> >>
> >> level: information-for-the-user
> >
> > But it is not.
> 
> But it is...
> 
> > Management tools will want to parse it, sooner or
> > later.  What is wrong with solution described above?
> 
> It is complex and annoying to the people that will actually use it.

grep -r . pstate/ is actually not that bad...

And yes, some kind of utility to select right performance level would
be nice in future... Or maybe not. Perhaps in not so distant future
kernel will use right performance level for given load...?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: unparseable, undocumented /sys/class/drm/.../pstate

[Ilia Mirkin]

On Mon, Jun 23, 2014 at 4:15 PM, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
>> >> >> > I guess better interface would be something like
>> >> >> >
>> >> >> > pstate/07/core_clock_min
>> >> >> >           core_clock_max
>> >> >> >           memory_clock_min
>> >> >> >           memory_clock_max
>> >> >> >
>> >> >> > and then pstate/active containing just the number of active state?
>> >
>> >> Could we just say that the format of this file is one-per-line of
>> >>
>> >> level: information-for-the-user
>> >
>> > But it is not.
>>
>> But it is...
>>
>> > Management tools will want to parse it, sooner or
>> > later.  What is wrong with solution described above?
>>
>> It is complex and annoying to the people that will actually use it.
>
> grep -r . pstate/ is actually not that bad...

While that's a clever trick that anyone who's done a bunch of stuff
with sysfs knows, I doubt the average linux user could come up with
that on their own. I know I didn't.

>
> And yes, some kind of utility to select right performance level would
> be nice in future... Or maybe not. Perhaps in not so distant future
> kernel will use right performance level for given load...?

Eventually yes. Currently switching between levels varies from
unsupported to unreliable depending on the hardware (as in, hangs the
card, or does otherwise-not-great things). Automatic switching
requires regular switching to be reliable :) [And the performance
counters that are presently being worked on to be able to tell the
card load.]

  -ilia

Re: unparseable, undocumented /sys/class/drm/.../pstate

[Greg KH]

On Mon, Jun 23, 2014 at 04:18:39PM -0400, Ilia Mirkin wrote:
> On Mon, Jun 23, 2014 at 4:15 PM, Pavel Machek <pavel@ucw.cz> wrote:
> > Hi!
> >
> >> >> >> > I guess better interface would be something like
> >> >> >> >
> >> >> >> > pstate/07/core_clock_min
> >> >> >> >           core_clock_max
> >> >> >> >           memory_clock_min
> >> >> >> >           memory_clock_max
> >> >> >> >
> >> >> >> > and then pstate/active containing just the number of active state?
> >> >
> >> >> Could we just say that the format of this file is one-per-line of
> >> >>
> >> >> level: information-for-the-user
> >> >
> >> > But it is not.
> >>
> >> But it is...
> >>
> >> > Management tools will want to parse it, sooner or
> >> > later.  What is wrong with solution described above?
> >>
> >> It is complex and annoying to the people that will actually use it.
> >
> > grep -r . pstate/ is actually not that bad...
> 
> While that's a clever trick that anyone who's done a bunch of stuff
> with sysfs knows, I doubt the average linux user could come up with
> that on their own. I know I didn't.

That's fine, why would an "average" Linux user ever need to poke around
in sysfs?  Again, please describe what you are wanting to have exported
to userspace, and what userspace is supposed to do with that
information, before worrying about the actual sysfs file layout.

greg k-h

Re: unparseable, undocumented /sys/class/drm/.../pstate

[Ilia Mirkin]

On Mon, Jun 23, 2014 at 4:26 PM, Greg KH <greg@kroah.com> wrote:
> On Mon, Jun 23, 2014 at 04:18:39PM -0400, Ilia Mirkin wrote:
>> On Mon, Jun 23, 2014 at 4:15 PM, Pavel Machek <pavel@ucw.cz> wrote:
>> > Hi!
>> >
>> >> >> >> > I guess better interface would be something like
>> >> >> >> >
>> >> >> >> > pstate/07/core_clock_min
>> >> >> >> >           core_clock_max
>> >> >> >> >           memory_clock_min
>> >> >> >> >           memory_clock_max
>> >> >> >> >
>> >> >> >> > and then pstate/active containing just the number of active state?
>> >> >
>> >> >> Could we just say that the format of this file is one-per-line of
>> >> >>
>> >> >> level: information-for-the-user
>> >> >
>> >> > But it is not.
>> >>
>> >> But it is...
>> >>
>> >> > Management tools will want to parse it, sooner or
>> >> > later.  What is wrong with solution described above?
>> >>
>> >> It is complex and annoying to the people that will actually use it.
>> >
>> > grep -r . pstate/ is actually not that bad...
>>
>> While that's a clever trick that anyone who's done a bunch of stuff
>> with sysfs knows, I doubt the average linux user could come up with
>> that on their own. I know I didn't.
>
> That's fine, why would an "average" Linux user ever need to poke around
> in sysfs?  Again, please describe what you are wanting to have exported
> to userspace, and what userspace is supposed to do with that
> information, before worrying about the actual sysfs file layout.

It would be nice to allow the end-user to switch between performance
levels on the card.

A particular card exposes some number of levels (well, a particular
card's VBIOS), identified by a value between 0-254 (usually identified
as a 2-char hex string). Each level has various information associated
with it, like timing parameters for various bits of the card, as well
as some more user-friendly concepts like "memory clock speed" etc.

The card's current state may or may not correspond to one of the
predefined levels; often-times the VBIOS initializes the card into
some non-level state. This state may also be of some interest to the
user.

We can't switch to arbitrary speeds, only the defined ones (because of
the various other timing parameters). The level ids don't carry too
much semantic value (higher usually means faster though), so it would
additionally be nice to tell the user some of the more user-friendly
information about each level. Different hardware versions will be able
to expose different types of information (but always in <name> <clock
speed/range> pairs).

  -ilia

Re: unparseable, undocumented /sys/class/drm/.../pstate

[Ben Skeggs]

On Tue, Jun 24, 2014 at 6:26 AM, Greg KH <greg@kroah.com> wrote:
> On Mon, Jun 23, 2014 at 04:18:39PM -0400, Ilia Mirkin wrote:
>> On Mon, Jun 23, 2014 at 4:15 PM, Pavel Machek <pavel@ucw.cz> wrote:
>> > Hi!
>> >
>> >> >> >> > I guess better interface would be something like
>> >> >> >> >
>> >> >> >> > pstate/07/core_clock_min
>> >> >> >> >           core_clock_max
>> >> >> >> >           memory_clock_min
>> >> >> >> >           memory_clock_max
>> >> >> >> >
>> >> >> >> > and then pstate/active containing just the number of active state?
>> >> >
>> >> >> Could we just say that the format of this file is one-per-line of
>> >> >>
>> >> >> level: information-for-the-user
>> >> >
>> >> > But it is not.
>> >>
>> >> But it is...
>> >>
>> >> > Management tools will want to parse it, sooner or
>> >> > later.  What is wrong with solution described above?
>> >>
>> >> It is complex and annoying to the people that will actually use it.
>> >
>> > grep -r . pstate/ is actually not that bad...
>>
>> While that's a clever trick that anyone who's done a bunch of stuff
>> with sysfs knows, I doubt the average linux user could come up with
>> that on their own. I know I didn't.
>
> That's fine, why would an "average" Linux user ever need to poke around
> in sysfs?  Again, please describe what you are wanting to have exported
> to userspace, and what userspace is supposed to do with that
> information, before worrying about the actual sysfs file layout.
Because, at the moment, we can't by default give any kind of automatic
clock management policy due to the fact that in a great number of
cases, we'll likely hang the GPU when changing clock speeds.  The
VBIOS defaults aren't sufficient for more demanding games etc, and
people might want to try/risk selecting the highest level anyway to
see if it'll work for them.  When things actually work, this will all
automagically happen based on load and users should never need to
touch it.

So, we want a file users can write the level identifier into.  Which,
shockingly, is exactly what the file currently does.

I, however, also decided that people might actually want to know what
this "0x0a" they're echoing into the file actually means; So, in the
output (which is a list of valid identifiers), after the identifier
there's a bunch of "<name> <value>" pairs giving an overview of that
this mysterious "0x0a" is.

Sure, we can remove the information and have the informationless list
of identifiers and we'd suddenly be strictly obeying the rules, then
we've also made any potential userspace tool that we're worried about
a lot more useless (what's it going to do, a drop-down list of 0x07,
0x0a, 0x0e, 0x0f?).

Sure, we can split this all up into a directory structure; and make it
a lot more cumbersome for the intended target of the user who just
wants to override an unfortunate but currently necessary default.  I'm
not sure how exactly one-per-line "<id>: <name> <min>-<max> ..." is
hard for userspace to deal with (scanf anyone?), but a directory
structure won't be any easier, the available files will still differ
with each card generation etc and userspace will just have to loop
over a directory list instead of each line of a single file.

But, as I said on IRC yesterday, let's just move this to debugfs to
save this waste of time argument, and move on.

Ben.


>
> greg k-h
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: unparseable, undocumented /sys/class/drm/.../pstate

[Greg KH]

On Tue, Jun 24, 2014 at 10:06:06AM +1000, Ben Skeggs wrote:
> 
> But, as I said on IRC yesterday, let's just move this to debugfs to
> save this waste of time argument, and move on.

I like that idea, great plan :)

greg k-h

Re: unparseable, undocumented /sys/class/drm/.../pstate

[Greg KH]

On Sun, Jun 22, 2014 at 10:12:14PM -0400, Ilia Mirkin wrote:
> On Sat, Jun 21, 2014 at 3:45 PM, Greg KH <greg@kroah.com> wrote:
> > On Sat, Jun 21, 2014 at 02:22:59PM -0400, Ilia Mirkin wrote:
> >> On Sat, Jun 21, 2014 at 2:02 PM, Pavel Machek <pavel@ucw.cz> wrote:
> >> > Hi!
> >> >
> >> > AFAICT, pstate file will contain something like
> >> >
> >> > 07: core 100 MHz memory 123 MHz *
> >> > 08: core 100-200 MHz memory 123 MHz
> >> >
> >> > ...which does not look exactly like one-value-per-file, and I'm pretty
> >> > sure userspace will get it wrong if it tries to parse it. Plus, I
> >> > don't see required documentation in Documentation/ABI.
> >> >
> >> > Should we disable it for now, so that userspace does not start
> >> > depending on it and we'll not have to maintain it forever?
> >> >
> >> > I guess better interface would be something like
> >> >
> >> > pstate/07/core_clock_min
> >> >           core_clock_max
> >> >           memory_clock_min
> >> >           memory_clock_max
> >> >
> >> > and then pstate/active containing just the number of active state?
> >> >
> >> > Thanks,
> >> >                                                                 Pavel
> >> >
> >> > PS: I have no nvidia, got the news at
> >> >
> >> > http://www.phoronix.com/scan.php?page=article&item=nouveau_try_linux316&num=2
> >>
> >> FTR, this file has been in place since 3.13, and there was a different
> >> file before it (performance_levels), with a comparable format since
> >> much earlier (definitely 3.8, probably earlier). I think it's meant a
> >> lot more for people looking at it and echo'ing stuff to it to modify
> >> the levels (where supported), than for programs parsing it. Perhaps
> >> sysfs is the wrong place for this -- what is the right place? debugfs?
> >
> > Yes, please move it to debugfs.
> 
> Could we just say that the format of this file is one-per-line of
> 
> level: information-for-the-user
> 
> And you can echo a level into it to switch to that level? That seems
> like a reasonable ABI to have... would be happy to throw it into a
> file somewhere... not sure where though.

sysfs files are "one value per file", that's it.  Do anything other than
that, and it can not be in sysfs, sorry.

greg k-h

Re: unparseable, undocumented /sys/class/drm/.../pstate

[Ilia Mirkin]

On Mon, Jun 23, 2014 at 12:07 PM, Greg KH <greg@kroah.com> wrote:
> On Sun, Jun 22, 2014 at 10:12:14PM -0400, Ilia Mirkin wrote:
>> On Sat, Jun 21, 2014 at 3:45 PM, Greg KH <greg@kroah.com> wrote:
>> > On Sat, Jun 21, 2014 at 02:22:59PM -0400, Ilia Mirkin wrote:
>> >> On Sat, Jun 21, 2014 at 2:02 PM, Pavel Machek <pavel@ucw.cz> wrote:
>> >> > Hi!
>> >> >
>> >> > AFAICT, pstate file will contain something like
>> >> >
>> >> > 07: core 100 MHz memory 123 MHz *
>> >> > 08: core 100-200 MHz memory 123 MHz
>> >> >
>> >> > ...which does not look exactly like one-value-per-file, and I'm pretty
>> >> > sure userspace will get it wrong if it tries to parse it. Plus, I
>> >> > don't see required documentation in Documentation/ABI.
>> >> >
>> >> > Should we disable it for now, so that userspace does not start
>> >> > depending on it and we'll not have to maintain it forever?
>> >> >
>> >> > I guess better interface would be something like
>> >> >
>> >> > pstate/07/core_clock_min
>> >> >           core_clock_max
>> >> >           memory_clock_min
>> >> >           memory_clock_max
>> >> >
>> >> > and then pstate/active containing just the number of active state?
>> >> >
>> >> > Thanks,
>> >> >                                                                 Pavel
>> >> >
>> >> > PS: I have no nvidia, got the news at
>> >> >
>> >> > http://www.phoronix.com/scan.php?page=article&item=nouveau_try_linux316&num=2
>> >>
>> >> FTR, this file has been in place since 3.13, and there was a different
>> >> file before it (performance_levels), with a comparable format since
>> >> much earlier (definitely 3.8, probably earlier). I think it's meant a
>> >> lot more for people looking at it and echo'ing stuff to it to modify
>> >> the levels (where supported), than for programs parsing it. Perhaps
>> >> sysfs is the wrong place for this -- what is the right place? debugfs?
>> >
>> > Yes, please move it to debugfs.
>>
>> Could we just say that the format of this file is one-per-line of
>>
>> level: information-for-the-user
>>
>> And you can echo a level into it to switch to that level? That seems
>> like a reasonable ABI to have... would be happy to throw it into a
>> file somewhere... not sure where though.
>
> sysfs files are "one value per file", that's it.  Do anything other than
> that, and it can not be in sysfs, sorry.

I think that's a little inconsistent. There are *tons* of files in
sysfs with multiple values. For example "local_cpulist" which contains
the string "0-3" for me, the per-connector "modes" file which has a
list of modes supported, and a "resource" file which has a list of hex
values (which probably have something to do with PCI resources?). [I
purposely picked values coming from different parts of the kernel not
to focus on one subsystem...]

  -ilia

Re: unparseable, undocumented /sys/class/drm/.../pstate

[Greg KH]

On Mon, Jun 23, 2014 at 12:18:51PM -0400, Ilia Mirkin wrote:
> On Mon, Jun 23, 2014 at 12:07 PM, Greg KH <greg@kroah.com> wrote:
> > On Sun, Jun 22, 2014 at 10:12:14PM -0400, Ilia Mirkin wrote:
> >> On Sat, Jun 21, 2014 at 3:45 PM, Greg KH <greg@kroah.com> wrote:
> >> > On Sat, Jun 21, 2014 at 02:22:59PM -0400, Ilia Mirkin wrote:
> >> >> On Sat, Jun 21, 2014 at 2:02 PM, Pavel Machek <pavel@ucw.cz> wrote:
> >> >> > Hi!
> >> >> >
> >> >> > AFAICT, pstate file will contain something like
> >> >> >
> >> >> > 07: core 100 MHz memory 123 MHz *
> >> >> > 08: core 100-200 MHz memory 123 MHz
> >> >> >
> >> >> > ...which does not look exactly like one-value-per-file, and I'm pretty
> >> >> > sure userspace will get it wrong if it tries to parse it. Plus, I
> >> >> > don't see required documentation in Documentation/ABI.
> >> >> >
> >> >> > Should we disable it for now, so that userspace does not start
> >> >> > depending on it and we'll not have to maintain it forever?
> >> >> >
> >> >> > I guess better interface would be something like
> >> >> >
> >> >> > pstate/07/core_clock_min
> >> >> >           core_clock_max
> >> >> >           memory_clock_min
> >> >> >           memory_clock_max
> >> >> >
> >> >> > and then pstate/active containing just the number of active state?
> >> >> >
> >> >> > Thanks,
> >> >> >                                                                 Pavel
> >> >> >
> >> >> > PS: I have no nvidia, got the news at
> >> >> >
> >> >> > http://www.phoronix.com/scan.php?page=article&item=nouveau_try_linux316&num=2
> >> >>
> >> >> FTR, this file has been in place since 3.13, and there was a different
> >> >> file before it (performance_levels), with a comparable format since
> >> >> much earlier (definitely 3.8, probably earlier). I think it's meant a
> >> >> lot more for people looking at it and echo'ing stuff to it to modify
> >> >> the levels (where supported), than for programs parsing it. Perhaps
> >> >> sysfs is the wrong place for this -- what is the right place? debugfs?
> >> >
> >> > Yes, please move it to debugfs.
> >>
> >> Could we just say that the format of this file is one-per-line of
> >>
> >> level: information-for-the-user
> >>
> >> And you can echo a level into it to switch to that level? That seems
> >> like a reasonable ABI to have... would be happy to throw it into a
> >> file somewhere... not sure where though.
> >
> > sysfs files are "one value per file", that's it.  Do anything other than
> > that, and it can not be in sysfs, sorry.
> 
> I think that's a little inconsistent. There are *tons* of files in
> sysfs with multiple values. For example "local_cpulist" which contains
> the string "0-3" for me, the per-connector "modes" file which has a
> list of modes supported, and a "resource" file which has a list of hex
> values (which probably have something to do with PCI resources?). [I
> purposely picked values coming from different parts of the kernel not
> to focus on one subsystem...]

A list of valid "values" that a file can be in is fine if you just then
write one value back to that file.  That's the one exception, but a
minor one given the huge number of sysfs files.  Other than that, if you
know of exceptions to that rule, please point them out and I will be
glad to yell at the developers.

PCI device resources are binary sysfs files, which are just pass-through
files from the firmware/device to userspace, with no parsing done in the
kernel.  So that's just a single 'value' as well.

greg k-h

Re: unparseable, undocumented /sys/class/drm/.../pstate

[Ilia Mirkin]

On Mon, Jun 23, 2014 at 12:36 PM, Greg KH <greg@kroah.com> wrote:
> On Mon, Jun 23, 2014 at 12:18:51PM -0400, Ilia Mirkin wrote:
>> On Mon, Jun 23, 2014 at 12:07 PM, Greg KH <greg@kroah.com> wrote:
>> > On Sun, Jun 22, 2014 at 10:12:14PM -0400, Ilia Mirkin wrote:
>> >> On Sat, Jun 21, 2014 at 3:45 PM, Greg KH <greg@kroah.com> wrote:
>> >> > On Sat, Jun 21, 2014 at 02:22:59PM -0400, Ilia Mirkin wrote:
>> >> >> On Sat, Jun 21, 2014 at 2:02 PM, Pavel Machek <pavel@ucw.cz> wrote:
>> >> >> > Hi!
>> >> >> >
>> >> >> > AFAICT, pstate file will contain something like
>> >> >> >
>> >> >> > 07: core 100 MHz memory 123 MHz *
>> >> >> > 08: core 100-200 MHz memory 123 MHz
>> >> >> >
>> >> >> > ...which does not look exactly like one-value-per-file, and I'm pretty
>> >> >> > sure userspace will get it wrong if it tries to parse it. Plus, I
>> >> >> > don't see required documentation in Documentation/ABI.
>> >> >> >
>> >> >> > Should we disable it for now, so that userspace does not start
>> >> >> > depending on it and we'll not have to maintain it forever?
>> >> >> >
>> >> >> > I guess better interface would be something like
>> >> >> >
>> >> >> > pstate/07/core_clock_min
>> >> >> >           core_clock_max
>> >> >> >           memory_clock_min
>> >> >> >           memory_clock_max
>> >> >> >
>> >> >> > and then pstate/active containing just the number of active state?
>> >> >> >
>> >> >> > Thanks,
>> >> >> >                                                                 Pavel
>> >> >> >
>> >> >> > PS: I have no nvidia, got the news at
>> >> >> >
>> >> >> > http://www.phoronix.com/scan.php?page=article&item=nouveau_try_linux316&num=2
>> >> >>
>> >> >> FTR, this file has been in place since 3.13, and there was a different
>> >> >> file before it (performance_levels), with a comparable format since
>> >> >> much earlier (definitely 3.8, probably earlier). I think it's meant a
>> >> >> lot more for people looking at it and echo'ing stuff to it to modify
>> >> >> the levels (where supported), than for programs parsing it. Perhaps
>> >> >> sysfs is the wrong place for this -- what is the right place? debugfs?
>> >> >
>> >> > Yes, please move it to debugfs.
>> >>
>> >> Could we just say that the format of this file is one-per-line of
>> >>
>> >> level: information-for-the-user
>> >>
>> >> And you can echo a level into it to switch to that level? That seems
>> >> like a reasonable ABI to have... would be happy to throw it into a
>> >> file somewhere... not sure where though.
>> >
>> > sysfs files are "one value per file", that's it.  Do anything other than
>> > that, and it can not be in sysfs, sorry.
>>
>> I think that's a little inconsistent. There are *tons* of files in
>> sysfs with multiple values. For example "local_cpulist" which contains
>> the string "0-3" for me, the per-connector "modes" file which has a
>> list of modes supported, and a "resource" file which has a list of hex
>> values (which probably have something to do with PCI resources?). [I
>> purposely picked values coming from different parts of the kernel not
>> to focus on one subsystem...]
>
> A list of valid "values" that a file can be in is fine if you just then
> write one value back to that file.  That's the one exception, but a
> minor one given the huge number of sysfs files.  Other than that, if you

Which is pretty much what the pstate file is. Would it make things
better if we removed the descriptive info while leaving the pstate
file in place?

> know of exceptions to that rule, please point them out and I will be
> glad to yell at the developers.
>
> PCI device resources are binary sysfs files, which are just pass-through
> files from the firmware/device to userspace, with no parsing done in the
> kernel.  So that's just a single 'value' as well.

$ cat /sys/class/drm/card0/device/resource
0x00000000f0000000 0x00000000f03fffff 0x0000000000140204
0x0000000000000000 0x0000000000000000 0x0000000000000000

Doesn't seem like "binary" in the true sense, but perhaps that's close enough.

  -ilia

Re: unparseable, undocumented /sys/class/drm/.../pstate

[Martin Peres]

Le 23/06/2014 18:40, Ilia Mirkin a écrit :
> On Mon, Jun 23, 2014 at 12:36 PM, Greg KH <greg@kroah.com> wrote:
>> On Mon, Jun 23, 2014 at 12:18:51PM -0400, Ilia Mirkin wrote:
>> A list of valid "values" that a file can be in is fine if you just then
>> write one value back to that file.  That's the one exception, but a
>> minor one given the huge number of sysfs files.  Other than that, if you
>
> Which is pretty much what the pstate file is. Would it make things
> better if we removed the descriptive info while leaving the pstate
> file in place?

This means we should also create a new sysfs file per performance level 
too, right? Is there another way for a driver to expose a list in sysfs?

Since NVIDIA gives different names to performance levels depending on 
the card family, we may need to abstract the name away in order to 
provide some consistency and make listing performance levels easier from 
a program (may it use readdir() or stat()).

Moving the file to debugfs would "fix" the one-value-per-file rule but 
it would also require users to mount debugfs at boot time in order to 
write the default configuration they want for PM instead of just 
changing /etc/sysctl.d/nouveau.conf... On the other hand, I'm not sure 
we can commit on having a stable ABI on the way we display clocks 
(unless people take them as a single value and do not try to parse them) 
as new hardware will alter the semantics of each clock domain, if not 
drop/split some of them!

Whatever we do, it doesn't look like we can find a nice solution that 
fits every use cases unless we write a userspace program to access this 
data, but this seems highly overkill...

Re: unparseable, undocumented /sys/class/drm/.../pstate

[Ilia Mirkin]

On Mon, Jun 23, 2014 at 1:46 PM, Martin Peres <martin.peres@free.fr> wrote:
> Le 23/06/2014 18:40, Ilia Mirkin a écrit :
>>
>> On Mon, Jun 23, 2014 at 12:36 PM, Greg KH <greg@kroah.com> wrote:
>>>
>>> On Mon, Jun 23, 2014 at 12:18:51PM -0400, Ilia Mirkin wrote:
>>> A list of valid "values" that a file can be in is fine if you just then
>>> write one value back to that file.  That's the one exception, but a
>>> minor one given the huge number of sysfs files.  Other than that, if you
>>
>>
>> Which is pretty much what the pstate file is. Would it make things
>> better if we removed the descriptive info while leaving the pstate
>> file in place?
>
>
> This means we should also create a new sysfs file per performance level too,
> right? Is there another way for a driver to expose a list in sysfs?
>
> Since NVIDIA gives different names to performance levels depending on the
> card family, we may need to abstract the name away in order to provide some
> consistency and make listing performance levels easier from a program (may
> it use readdir() or stat()).
>
> Moving the file to debugfs would "fix" the one-value-per-file rule but it
> would also require users to mount debugfs at boot time in order to write the
> default configuration they want for PM instead of just changing
> /etc/sysctl.d/nouveau.conf... On the other hand, I'm not sure we can commit
> on having a stable ABI on the way we display clocks (unless people take them
> as a single value and do not try to parse them) as new hardware will alter
> the semantics of each clock domain, if not drop/split some of them!
>
> Whatever we do, it doesn't look like we can find a nice solution that fits
> every use cases unless we write a userspace program to access this data, but
> this seems highly overkill...

I was thinking just having the list of level ids in the pstate file,
and then stick the current file into debugfs. That way people retain
the ability to see things, as well as use pstate directly for a
configured system.

  -ilia

Re: unparseable, undocumented /sys/class/drm/.../pstate

[Martin Peres]

Le 23/06/2014 19:56, Ilia Mirkin a écrit :
> On Mon, Jun 23, 2014 at 1:46 PM, Martin Peres <martin.peres@free.fr> wrote:
>> Le 23/06/2014 18:40, Ilia Mirkin a écrit :
>>>
>>> On Mon, Jun 23, 2014 at 12:36 PM, Greg KH <greg@kroah.com> wrote:
>>>>
>>>> On Mon, Jun 23, 2014 at 12:18:51PM -0400, Ilia Mirkin wrote:
>>>> A list of valid "values" that a file can be in is fine if you just then
>>>> write one value back to that file.  That's the one exception, but a
>>>> minor one given the huge number of sysfs files.  Other than that, if you
>>>
>>>
>>> Which is pretty much what the pstate file is. Would it make things
>>> better if we removed the descriptive info while leaving the pstate
>>> file in place?
>>
>>
>> This means we should also create a new sysfs file per performance level too,
>> right? Is there another way for a driver to expose a list in sysfs?
>>
>> Since NVIDIA gives different names to performance levels depending on the
>> card family, we may need to abstract the name away in order to provide some
>> consistency and make listing performance levels easier from a program (may
>> it use readdir() or stat()).
>>
>> Moving the file to debugfs would "fix" the one-value-per-file rule but it
>> would also require users to mount debugfs at boot time in order to write the
>> default configuration they want for PM instead of just changing
>> /etc/sysctl.d/nouveau.conf... On the other hand, I'm not sure we can commit
>> on having a stable ABI on the way we display clocks (unless people take them
>> as a single value and do not try to parse them) as new hardware will alter
>> the semantics of each clock domain, if not drop/split some of them!
>>
>> Whatever we do, it doesn't look like we can find a nice solution that fits
>> every use cases unless we write a userspace program to access this data, but
>> this seems highly overkill...
>
> I was thinking just having the list of level ids in the pstate file,
> and then stick the current file into debugfs. That way people retain
> the ability to see things, as well as use pstate directly for a
> configured system.

In this case, would we still use the * to indicate the current perflvl?

Also, are we supposed to output the current perflvl or the current 
configuration in use? Right now, we configure it to either auto (WIP), 
perflvl X at all time or perflvl X when on battery and Y when on sector.
However, when we read pstate, we only get the current perflvl if my 
memory serves me right. Maybe we should create a r-o file that outputs 
the current perflvl and keep pstate for storing the configuration.

Re: unparseable, undocumented /sys/class/drm/.../pstate

[Ilia Mirkin]

On Mon, Jun 23, 2014 at 2:00 PM, Martin Peres <martin.peres@free.fr> wrote:
> Le 23/06/2014 19:56, Ilia Mirkin a écrit :
>
>> On Mon, Jun 23, 2014 at 1:46 PM, Martin Peres <martin.peres@free.fr>
>> wrote:
>>>
>>> Le 23/06/2014 18:40, Ilia Mirkin a écrit :
>>>>
>>>>
>>>> On Mon, Jun 23, 2014 at 12:36 PM, Greg KH <greg@kroah.com> wrote:
>>>>>
>>>>>
>>>>> On Mon, Jun 23, 2014 at 12:18:51PM -0400, Ilia Mirkin wrote:
>>>>> A list of valid "values" that a file can be in is fine if you just then
>>>>> write one value back to that file.  That's the one exception, but a
>>>>> minor one given the huge number of sysfs files.  Other than that, if
>>>>> you
>>>>
>>>>
>>>>
>>>> Which is pretty much what the pstate file is. Would it make things
>>>> better if we removed the descriptive info while leaving the pstate
>>>> file in place?
>>>
>>>
>>>
>>> This means we should also create a new sysfs file per performance level
>>> too,
>>> right? Is there another way for a driver to expose a list in sysfs?
>>>
>>> Since NVIDIA gives different names to performance levels depending on the
>>> card family, we may need to abstract the name away in order to provide
>>> some
>>> consistency and make listing performance levels easier from a program
>>> (may
>>> it use readdir() or stat()).
>>>
>>> Moving the file to debugfs would "fix" the one-value-per-file rule but it
>>> would also require users to mount debugfs at boot time in order to write
>>> the
>>> default configuration they want for PM instead of just changing
>>> /etc/sysctl.d/nouveau.conf... On the other hand, I'm not sure we can
>>> commit
>>> on having a stable ABI on the way we display clocks (unless people take
>>> them
>>> as a single value and do not try to parse them) as new hardware will
>>> alter
>>> the semantics of each clock domain, if not drop/split some of them!
>>>
>>> Whatever we do, it doesn't look like we can find a nice solution that
>>> fits
>>> every use cases unless we write a userspace program to access this data,
>>> but
>>> this seems highly overkill...
>>
>>
>> I was thinking just having the list of level ids in the pstate file,
>> and then stick the current file into debugfs. That way people retain
>> the ability to see things, as well as use pstate directly for a
>> configured system.
>
>
> In this case, would we still use the * to indicate the current perflvl?

That would only be in debugfs. pstate would just list the available
levels and let you set one. (or 'auto', as you point out below)

>
> Also, are we supposed to output the current perflvl or the current
> configuration in use? Right now, we configure it to either auto (WIP),
> perflvl X at all time or perflvl X when on battery and Y when on sector.
> However, when we read pstate, we only get the current perflvl if my memory
> serves me right. Maybe we should create a r-o file that outputs the current
> perflvl and keep pstate for storing the configuration.

Yeah, we could do something like that... ugh, the battery/ac stuff is
a complication. Unclear whether that belongs in the kernel in the
first place... but I guess other drivers do it?

Re: unparseable, undocumented /sys/class/drm/.../pstate

[Greg KH]

On Mon, Jun 23, 2014 at 07:46:03PM +0200, Martin Peres wrote:
> Le 23/06/2014 18:40, Ilia Mirkin a écrit :
> >On Mon, Jun 23, 2014 at 12:36 PM, Greg KH <greg@kroah.com> wrote:
> >>On Mon, Jun 23, 2014 at 12:18:51PM -0400, Ilia Mirkin wrote:
> >>A list of valid "values" that a file can be in is fine if you just then
> >>write one value back to that file.  That's the one exception, but a
> >>minor one given the huge number of sysfs files.  Other than that, if you
> >
> >Which is pretty much what the pstate file is. Would it make things
> >better if we removed the descriptive info while leaving the pstate
> >file in place?
> 
> This means we should also create a new sysfs file per performance level too,
> right? Is there another way for a driver to expose a list in sysfs?

What exactly are you wanting to export to userspace?  What will
userspace do with this information?  Why export anything at all?

Start with defining that, and go from there.

thanks,

greg k-h

Re: unparseable, undocumented /sys/class/drm/.../pstate

[Greg KH]

On Mon, Jun 23, 2014 at 12:40:43PM -0400, Ilia Mirkin wrote:
> On Mon, Jun 23, 2014 at 12:36 PM, Greg KH <greg@kroah.com> wrote:
> > On Mon, Jun 23, 2014 at 12:18:51PM -0400, Ilia Mirkin wrote:
> >> On Mon, Jun 23, 2014 at 12:07 PM, Greg KH <greg@kroah.com> wrote:
> >> > On Sun, Jun 22, 2014 at 10:12:14PM -0400, Ilia Mirkin wrote:
> >> >> On Sat, Jun 21, 2014 at 3:45 PM, Greg KH <greg@kroah.com> wrote:
> >> >> > On Sat, Jun 21, 2014 at 02:22:59PM -0400, Ilia Mirkin wrote:
> >> >> >> On Sat, Jun 21, 2014 at 2:02 PM, Pavel Machek <pavel@ucw.cz> wrote:
> >> >> >> > Hi!
> >> >> >> >
> >> >> >> > AFAICT, pstate file will contain something like
> >> >> >> >
> >> >> >> > 07: core 100 MHz memory 123 MHz *
> >> >> >> > 08: core 100-200 MHz memory 123 MHz
> >> >> >> >
> >> >> >> > ...which does not look exactly like one-value-per-file, and I'm pretty
> >> >> >> > sure userspace will get it wrong if it tries to parse it. Plus, I
> >> >> >> > don't see required documentation in Documentation/ABI.
> >> >> >> >
> >> >> >> > Should we disable it for now, so that userspace does not start
> >> >> >> > depending on it and we'll not have to maintain it forever?
> >> >> >> >
> >> >> >> > I guess better interface would be something like
> >> >> >> >
> >> >> >> > pstate/07/core_clock_min
> >> >> >> >           core_clock_max
> >> >> >> >           memory_clock_min
> >> >> >> >           memory_clock_max
> >> >> >> >
> >> >> >> > and then pstate/active containing just the number of active state?
> >> >> >> >
> >> >> >> > Thanks,
> >> >> >> >                                                                 Pavel
> >> >> >> >
> >> >> >> > PS: I have no nvidia, got the news at
> >> >> >> >
> >> >> >> > http://www.phoronix.com/scan.php?page=article&item=nouveau_try_linux316&num=2
> >> >> >>
> >> >> >> FTR, this file has been in place since 3.13, and there was a different
> >> >> >> file before it (performance_levels), with a comparable format since
> >> >> >> much earlier (definitely 3.8, probably earlier). I think it's meant a
> >> >> >> lot more for people looking at it and echo'ing stuff to it to modify
> >> >> >> the levels (where supported), than for programs parsing it. Perhaps
> >> >> >> sysfs is the wrong place for this -- what is the right place? debugfs?
> >> >> >
> >> >> > Yes, please move it to debugfs.
> >> >>
> >> >> Could we just say that the format of this file is one-per-line of
> >> >>
> >> >> level: information-for-the-user
> >> >>
> >> >> And you can echo a level into it to switch to that level? That seems
> >> >> like a reasonable ABI to have... would be happy to throw it into a
> >> >> file somewhere... not sure where though.
> >> >
> >> > sysfs files are "one value per file", that's it.  Do anything other than
> >> > that, and it can not be in sysfs, sorry.
> >>
> >> I think that's a little inconsistent. There are *tons* of files in
> >> sysfs with multiple values. For example "local_cpulist" which contains
> >> the string "0-3" for me, the per-connector "modes" file which has a
> >> list of modes supported, and a "resource" file which has a list of hex
> >> values (which probably have something to do with PCI resources?). [I
> >> purposely picked values coming from different parts of the kernel not
> >> to focus on one subsystem...]
> >
> > A list of valid "values" that a file can be in is fine if you just then
> > write one value back to that file.  That's the one exception, but a
> > minor one given the huge number of sysfs files.  Other than that, if you
> 
> Which is pretty much what the pstate file is. Would it make things
> better if we removed the descriptive info while leaving the pstate
> file in place?
> 
> > know of exceptions to that rule, please point them out and I will be
> > glad to yell at the developers.
> >
> > PCI device resources are binary sysfs files, which are just pass-through
> > files from the firmware/device to userspace, with no parsing done in the
> > kernel.  So that's just a single 'value' as well.
> 
> $ cat /sys/class/drm/card0/device/resource
> 0x00000000f0000000 0x00000000f03fffff 0x0000000000140204
> 0x0000000000000000 0x0000000000000000 0x0000000000000000
> 
> Doesn't seem like "binary" in the true sense, but perhaps that's close enough.

It's "close enough" :)