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

More tuned-ppd adjustments #669

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

zacikpa
Copy link
Contributor

@zacikpa zacikpa commented Aug 9, 2024

This PR brings another round of tuned-ppd adjustments and bug fixes. The individual commit messages explain why the changes are being made.

The most prominent change is dropping reverse profile mapping, i.e., from TuneD to PPD profiles. Instead, the daemon now keeps track of the currently active profile and the so-called base profile. This improves tuned-ppd in two ways:

  1. Profile mappings now don't have to be injective.
  2. When tuned-ppd is restarted, the initial profile is not the last active profile as inferred from the restored TuneD profile, but the last base profile, as saved into a file by tuned-ppd. The active and the base profile may differ due to profile holds being active before the restart. These should arguably be released when tuned-ppd is restarted.

@zacikpa
Copy link
Contributor Author

zacikpa commented Aug 12, 2024

CI failures fixed.

@zacikpa
Copy link
Contributor Author

zacikpa commented Aug 20, 2024

Rebased onto master because the issue resolved by the first commit was fixed by c082797.

@zacikpa
Copy link
Contributor Author

zacikpa commented Sep 18, 2024

Added another change - a new Version DBus property, which signifies the level of compatibility with power-profiles-daemon (as suggested in #684 (comment)).

@zacikpa
Copy link
Contributor Author

zacikpa commented Oct 14, 2024

Moved the first commit into a separate PR: #697. That one should ideally land in Fedora 41 before the final freeze. The rest are more complex and will likely require some discussion.

@velsinki
Copy link

velsinki commented Nov 26, 2024

Just chiming in here to say that technically this is api-incompatible still with PPD. The original PPD cannot return an "unknown" active profile, whereas tuned-ppd can.

This can cause a crash in gnome-control-center: see https://bugzilla.redhat.com/show_bug.cgi?id=2324518.

Now, I don't really see a way around this, but it would be good to document this.

Source: I'm the upstream (co)maintainer of gnome-control-center.

@zacikpa
Copy link
Contributor Author

zacikpa commented Nov 27, 2024

Thanks, @velsinki. I'll adjust the code not to perform this check.

@velsinki
Copy link

Thanks for the reply.

I'm not sure that's ideal either, as it would mean tuned-ppd is lying about which profile is active? We'll anyways fix the crash in gnome-control-center, not against keeping "unknown". I don't know what other consumers of PPD are doing though, GNOME Shell should have a similar issue.

@zacikpa
Copy link
Contributor Author

zacikpa commented Nov 27, 2024

I'm not sure that's ideal either, as it would mean tuned-ppd is lying about which profile is active?

Exactly. I agree it's not ideal, but it shifts the responsibility to the users when they mess with the TuneD profiles via multiple methods.

What's your proposed behavior of gnome-control-center if tuned-ppd returns "unknown"? It would certainly be nicer if we could keep the implementation as it is now. I would check with the other users of PPD to see whether they would be able to deal with it too.

@velsinki
Copy link

What's your proposed behavior of gnome-control-center if tuned-ppd returns "unknown"?

We currently show three radio buttons (performance, balanced, power-saver). If tuned-ppd would lie about a profile, we would start the UI with that radio button enabled, as we don't know anything else. That would mean that radio button can't actually be clicked anymore, as it's already active. If however we get "unknown", we can start the UI with no radio buttons selected instead.

Currently we don't handle "unknown", and crash, but that's not hard to support.

@zacikpa
Copy link
Contributor Author

zacikpa commented Nov 27, 2024

If however we get "unknown", we can start the UI with no radio buttons selected instead.

This sounds like very reasonable behavior.

Currently we don't handle "unknown", and crash, but that's not hard to support.

In that case, I'd prefer to keep this as is and check in with the other consumers.

Sorry for the troubles and thanks for looking into this!

@velsinki
Copy link

It would also be nice if the tuned profile is tracked, and if it changes, tuned-ppd anounces an ActiveProfile property change. That would resolve #689.

Instead of querying TuneD for the active profile and
coverting it to a PPD profile using a reverse profile
map, this commit makes tuned-ppd keep track of the
currently active PPD profile.

The daemon now also keeps track of the "base" PPD profile, which is
restored when all profile holds are released or when tuned-ppd is
restarted. For the latter purpose, this profile is also saved in a file.
Direct access via two dictionaries (one for AC, one for DC)
was clumsy, this commit replaces it with a new class - ProfileMap.
This makes tuned-ppd fully API-compatible with
the latest power-profiles-daemon. The property
will signify the level of API compatibility with
power-profiles-daemon.
To be consistent and make sure tuned-ppd works well with
its users no matter the configuration, the balanced profile
must be required in the configuration file, similarly to
the power-saver and performance profiles.

If no default profile is specified in the configuration,
this also sets the default profile to balanced, not raising
any exceptions like before.
Instead of actively polling for changes in the relevant files,
set up inotify handlers which are invoked when the files change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants