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&...
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&...
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
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
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
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&...
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
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&...
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
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
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
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
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
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
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
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
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&...
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
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&...
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
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&...
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
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&...
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
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...
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
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.
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?
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
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&... > > 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" :)
dri-devel@lists.freedesktop.org
-
Ben Skeggs -
Greg KH -
Ilia Mirkin -
Martin Peres -
Pavel Machek