Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

init: move cpufreq tuning to systemd service #3896

Merged
merged 3 commits into from
Nov 15, 2019

Conversation

antonlacon
Copy link
Contributor

@antonlacon antonlacon commented Oct 12, 2019

This converts this existing cpufreq configuration done in busybox/init and platform_init for rpi into a systemd service script. Tuning choices can be made based on device / proejct / distro / package. This should match what is currently done for different images. The RPi path has been tested.

@kszaq I'm not sure what device you were targeting for with #3538 but this should be a good restarting point for you.

@antonlacon
Copy link
Contributor Author

Added the rpi set-cpufreq script to amlogic as well. This assumes amlogic has only the one cpufreq policy (/sys/devices/systems/cpu/cpufreq/policy0).

@chewitt
Copy link
Member

chewitt commented Oct 16, 2019

I haven't been able to test this PR due to travels but the changes look sane. However I think we need to avoid duplicating scripts among multiple project folders - I guess the same will be needed for Allwinner and Rockchip too? .. also is set-<something>.service a systemd standard naming convention or could we just use cpufreq.service?

@antonlacon
Copy link
Contributor Author

I believe avoiding duplication is probably best done with a symlink between the projects. find_file_path allows finding replacement files by: device -> project -> distro -> package. The alternative would be replacing its logic in the systemd makefile for something like if arch=arm cp ... else cp ...., or taking care of it in the name: cp scripts/cpufreq.$ARCH.sh cpufreq.sh

It's not a standard naming convention, as far as I know. The name comes from a Ubuntu VM (same purpose; different script), and I've seen cpupower.service too. There are no other set-* scripts that I saw on the VM.

@MilhouseVH
Copy link
Contributor

In this case I'd rather accept a small duplication than adding a sym link from one file/project to another, which always "feels" bad to me (and likely to cause confusion/problems). In this case it's a small file, and could easily change in future (to better tune/optimise performance for RPi, which might be different from Amlogic - or vice versa).

packages/sysutils/systemd/package.mk Outdated Show resolved Hide resolved
@antonlacon
Copy link
Contributor Author

Rename complete.

The commits here were essentially to make the current changes done during busybox's init into systemd files for the live system. Thoughts on instead having the RPi version of cpufreq here be the 'default' in the systemd package scripts dir instead? I'd also like to change it such that checks if the cpu governor is ondemand and then making changes. This is so that the default cpu governor in the kernel .config is respected. Such a script could also be extended to cover schedutil or other governors if necessary.

@MilhouseVH
Copy link
Contributor

Should there be a default?

If we have a default it should be nothing, or something that is guaranteed to work safely for all projects and devices (regardless of kernel version, governors etc.). If a "safe for everyone" default is not possible then don't provide a default script, and make enabling of the service conditional on the presence of a $INSTALL/usr/bin/cpufreq script.

I think I'd rather not have a default script - just allow each project to implement their own script if they want to. And if they don't, then cpufreq.service is not enabled.

@antonlacon
Copy link
Contributor Author

The default script for LE is in users' interests, in my opinion. At present, it is only adjusting up_threshold of the ondemand governor. My understanding from the docs (https://www.kernel.org/doc/Documentation/cpu-freq/governors.txt), is that the default value is 95, and LE changes it to 50. This value works by checking if CPU load is over $up_threshold at Polling Point A and if it remains over $up_threshold at Polling Point A+1, then it bumps the frequency up to the next available level.

For LE, my expectation for high CPU load is software decoding, so being more aggressive at increasing CPU frequency should reduce stutter on high bitrate scenes.

I have some cleanup patches for this that I will try to push shortly.

@antonlacon
Copy link
Contributor Author

Cleanup commits pushed:

Only try to enable the cpufreq.service if the script has been installed.
Rely on the kernel config to set the default governor, and then apply the changes when the script's expected governor matches the governor in use. This is done so setting the default cpu governor is done in one location, and if the default is changed, these scripts default to no-op.

@MilhouseVH
Copy link
Contributor

MilhouseVH commented Oct 25, 2019

Thinking about it, there's probably a "more systemd" way to make the service conditional, by adding ConditionPathExists=/usr/bin/cpufreq to the service. Either way should work though. Not enabling the service at all might only become an issue if some other service ever becomes dependent on cpufreq.service (unlikely).

For example: https://github.com/LibreELEC/LibreELEC.tv/blob/master/packages/mediacenter/kodi/system.d/kodi-autostart.service#L5

@antonlacon
Copy link
Contributor Author

Commits squashed. Only change from when last seen is to the service file to use the ConditionPathExist for executing and RemainAfterExit should something ever decide to depend on this.

@chewitt
Copy link
Member

chewitt commented Oct 28, 2019

@antonlacon find_file_path was adjusted to cpufreq but the script is still called set-cpufreq so needs a tweak/rebase. With that correction, LGTM 👍

Signed-off-by: Ian Leonard <antonlacon@gmail.com>
Signed-off-by: Ian Leonard <antonlacon@gmail.com>
Signed-off-by: Ian Leonard <antonlacon@gmail.com>
@antonlacon
Copy link
Contributor Author

Fixed.

@chewitt
Copy link
Member

chewitt commented Nov 7, 2019

@jernejsk @Kwiboo can you validate this change for Allwinner/Rockchip .. can they use the same config as Amlogic or is something different needed?. It would be good to get the PR merged.

@jernejsk
Copy link
Member

jernejsk commented Nov 7, 2019

@chewitt only H3 has frequency scaling table in DT and even that is very simple. A64 and H6 don't have it (yet). I don't plan to add any out-of-tree patches for this at all, because current defaults are choosen carefully to be stable and when using HW decoding, high CPU frequency is not required at all.

Anyway, since those settings are only for "ondemand" governor, it shouldn't change anything for Allwinner images, because default governor is "performance".

And there is that: https://www.phoronix.com/scan.php?page=news_item&px=CPUFreq-Schedutil-Future-Maybe

@MilhouseVH MilhouseVH merged commit ecbd566 into LibreELEC:master Nov 15, 2019
@antonlacon antonlacon deleted the le10-systemd-cpufreq branch November 21, 2019 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants