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

Read AMD CPU power from MSRs #166

Merged
merged 1 commit into from
Feb 27, 2021
Merged

Conversation

schaerfo
Copy link
Collaborator

@schaerfo schaerfo commented Sep 25, 2020

Because of the unfortunate situation with the amd_energy driver only being available on model 31h, I have implemented reading the power draw directly from MSRs, like in https://github.com/djselbeck/rapl-read-ryzen.

The power reader is available under the following conditions:

  • An AMD family 17h (Ryzen/Epyc) CPU is detected.
  • The msr kernel module is loaded.
  • s-tui is run as root.

I have tested it only on a Ryzen 7 4800HS, and would suggest it is tested on a variety of other CPUs as well. Especially, I would be interested if per-package power reporting works as expected on multi-CPU systems.

Please let me know what you think!

@amanusk
Copy link
Owner

amanusk commented Nov 1, 2020

Hey,
Sorry for the late response to this. I think this is a great addition to have in general.
There were features that relied on reading MSRs directly in s-tui before but were removed to not enforce using root.
This can still be a good idea, until a solution that does not require root is introduced.

If it's not too much trouble, please shoot me an email to amanusk <at> tuta <dot> io to discuss this furter.
And thanks again for the work.

@amanusk
Copy link
Owner

amanusk commented Feb 27, 2021

I still don't have AMD hardware to test this with, unfortunately, but it does not seem to break anything when working with Intel.
I'm going to merge this to master and we can hopefully get feedback on from AMD users.
Now that Intel requires root to get power anyway, seems like this is a good fit

@amanusk amanusk merged commit 5c87727 into amanusk:master Feb 27, 2021
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