Background: --------------- PowerTOP is a tool used to provide information related to power consumption, from various sources, into one screen. One of the issues with PowerTOP is that the number of C and P states are hardcoded and thus it doesn't provide complete information on systems which may have more states than the hardcoded ones. Thus, though it works well on intel platforms, it may not on some of the ARM boards.
Changes being suggested: --------------------------------- These are couple of simple patches which try to make PowerTOP generic enough to run on almost any platform, which is plugged into the cpufreq and cpuidle framework. Thus the number of C and P states will no longer be hardcoded (though, the MAX values will be, to allocate enough memory for data structures). Most of the changes required is to make the display be aware of the ACTUAL number of these states and not just depend on some hard coded values.
Where to get the source and patches: ----------------------------------------------- You can get it from the git repo located at git://git.linaro.org/amitarora/powertop.git
The "master" branch has the PowerTOP code in sync with latest upstream powertop git tree. The "linaro" branch has my patches, which need testing. So, once you clone the tree, please do not forget to checkout the "linaro" branch (git checkout -b linaro remotes/origin/linaro)!
Note: If you had cloned it before, please clone a fresh tree, since I have re-written the history for "linaro" branch, to get patches in shape for upstream submission. Something, which I know should be avoided in future.
What to review: -------------------- Once you checkout the "linaro" branch, as suggested above. You can review the two patches with commit id "09049b42e3" and "783a3e6bbe".
What to test: ---------------- Some of the things which could be tested is, o Compile and test on any ARM board that you may have. You can also test on any x86 / x86_64 systems, to check if nothing has been broken. o Compare the output from "master" and "linaro" branches ("-d" dump option may help, if you wish to take a diff) o Check if the number of C states is correct (/sys/devices/system/cpu/cpu0/cpuidle/) o Check if the number of P states is correct (/sys/devices/system/cpu/cpu0/cpufreq/scaling_available_frequencies) o Check if the % values makes sense (say, if total doesn't add to 100! Or, if its a very huge value [1] ). o If number of Wakeups-per-second [1] looks reasonable. o Top causes for wakeups is shown properly o Suggestions are shown properly. Some of the suggestions may show a hot key on the status bar (at bottom).
[1] There is a known issue of %age values and Wakeups-per-sec number being shown very high (in the order of thousands), if some key is hit (say, 'r' to refresh or any other key). This problem can be seen in "master" branch (upstream) too, and hence is not introduced by the new patches. Since the goal of these patches is different, I haven't addressed this issue here.
Note: Thanks a ton to people who tested my initial patches and provided their valuable feedback.
Thanks ! Regards, Amit Arora
Could we get this in a PPA somewhere? Perhaps https://launchpad.net/~linaro-maintainers/+archive/overlay until it's merged in Ubuntu? (Careful: this is a devirtualized PPA)
Thanks!
On Wed, Aug 11, 2010 at 12:04 PM, Loïc Minier loic.minier@linaro.orgwrote:
Could we get this in a PPA somewhere? Perhaps https://launchpad.net/~linaro-maintainers/+archive/overlayhttps://launchpad.net/%7Elinaro-maintainers/+archive/overlay until it's merged in Ubuntu? (Careful: this is a devirtualized PPA)
just a note: the overlay archive is supposed to get packages that will be shipped as part of the official linaro release (either on or off image).
This means that freezes and freeze exception process apply here in a similar way.
If its just to make preview packages available we could create a "sandbox" ppa or something.
On Wed, Aug 11, 2010 at 10:10 AM, Amit Arora amit.arora@linaro.org wrote:
What to review:
Once you checkout the "linaro" branch, as suggested above. You can review the two patches with commit id "09049b42e3" and "783a3e6bbe".
What to test:
Some of the things which could be tested is, o Compile and test on any ARM board that you may have. You can also test on any x86 / x86_64 systems, to check if nothing has been broken. o Compare the output from "master" and "linaro" branches ("-d" dump option may help, if you wish to take a diff) o Check if the number of C states is correct (/sys/devices/system/cpu/cpu0/cpuidle/) o Check if the number of P states is correct (/sys/devices/system/cpu/cpu0/cpufreq/scaling_available_frequencies) o Check if the % values makes sense (say, if total doesn't add to 100! Or, if its a very huge value [1] ). o If number of Wakeups-per-second [1] looks reasonable. o Top causes for wakeups is shown properly o Suggestions are shown properly. Some of the suggestions may show a hot key on the status bar (at bottom).
[1] There is a known issue of %age values and Wakeups-per-sec number being shown very high (in the order of thousands), if some key is hit (say, 'r' to refresh or any other key). This problem can be seen in "master" branch (upstream) too, and hence is not introduced by the new patches. Since the goal of these patches is different, I haven't addressed this issue here.
Some comments:
1. Please run scripts/checkpatch.pl on the patches and fixup the whitespace warnings. 2. In print_cstates(), where you have #ifdef'ed __i386__, the #else should also be restricted to __arm__ IMO. 3. On my quad core desktop, it doesn't show C1 as a supported C-state on startup. (Supported C-states : C2 C3) 4. Works on Beagleboard.
Could you please try and post this to the powertop list this week?
Regards, Amit
On Wed, Aug 11, 2010 at 5:42 PM, Amit Kucheria amit.kucheria@linaro.org wrote:
Some comments:
- Please run scripts/checkpatch.pl on the patches and fixup the
whitespace warnings.
Sure.
- In print_cstates(), where you have #ifdef'ed __i386__, the #else
should also be restricted to __arm__ IMO.
OK.
- On my quad core desktop, it doesn't show C1 as a supported C-state
on startup. (Supported C-states : C2 C3)
That could be because print_cstates() shows only those C states which support MWAIT (have mwait in "desc" file for that particular C state). Should this be different for ARM ? Should we show all the states (whether they have mwait or not in "desc") ?
On a second thought, print_cstates() for intel makes sense, since they want to tell which states are supported by BIOS and which are supported by the CPU (using, cpuid). But, for ARM, we have only one set of supported states and these will be same as what will be shown in the next screen. Thats why I have kept the second patch separate, since we can remove that if its really not required. Thoughts ?
- Works on Beagleboard.
Nice.
Could you please try and post this to the powertop list this week?
Sure. Will wait for one more day for others to comment and then post it.
Thanks! Regards, Amit Arora
Regards, Amit
On Wed, Aug 11, 2010 at 4:31 PM, Amit Arora amit.arora@linaro.org wrote:
On Wed, Aug 11, 2010 at 5:42 PM, Amit Kucheria amit.kucheria@linaro.org wrote:
Some comments:
- Please run scripts/checkpatch.pl on the patches and fixup the
whitespace warnings.
Sure.
- In print_cstates(), where you have #ifdef'ed __i386__, the #else
should also be restricted to __arm__ IMO.
OK.
- On my quad core desktop, it doesn't show C1 as a supported C-state
on startup. (Supported C-states : C2 C3)
That could be because print_cstates() shows only those C states which support MWAIT (have mwait in "desc" file for that particular C state). Should this be different for ARM ? Should we show all the states (whether they have mwait or not in "desc") ?
Atleast on OMAP, /sys/devices/system/cpu/cpu0/cpuidle/stateN/desc is <null>.
On a second thought, print_cstates() for intel makes sense, since they want to tell which states are supported by BIOS and which are supported by the CPU (using, cpuid). But, for ARM, we have only one set of supported states and these will be same as what will be shown in the next screen. Thats why I have kept the second patch separate, since we can remove that if its really not required. Thoughts ?
I think MWAIT is x86-specific. So we don't care about that on ARM.
- Works on Beagleboard.
Nice.
Could you please try and post this to the powertop list this week?
Sure. Will wait for one more day for others to comment and then post it.