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

ryzen_smu: init at 0.1.5, ryzen_monitor_ng: init at 2.0.5 #271342

Merged
merged 6 commits into from Apr 11, 2024

Conversation

PhDyellow
Copy link

@PhDyellow PhDyellow commented Dec 1, 2023

Description of changes

ryzen_monitor_ng is a tool for monitoring power information on desktop AMD processors.
It can also set parameters for the CPU, so can offer some of the features of Ryzen Master on Linux.

It depends on the linux kernel driver ryzen_smu.

ryzen_smu includes a userspace program monitor_cpu.

https://gitlab.com/mann1x/ryzen_smu

https://github.com/mann1x/ryzen_monitor_ng

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Priorities

Add a 👍 reaction to pull requests you find important.

@PhDyellow
Copy link
Author

Tested on a 5900X CPU in a clevo laptop.

Copy link
Member

@Cryolitia Cryolitia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to introduce these two patches to support Phonix and Rembrandt.

Cryolitia@9aecdf4#diff-2cc356432ce45563cde8df0d10e36f41c3524284e9ceb526c281f3a61219ffdaR44

Would you like to co-maintain the module ryzen-smu, together. I take the liberty of making this request. I would be honored if you would accept it.

pkgs/os-specific/linux/ryzen-smu/monitor-cpu.nix Outdated Show resolved Hide resolved
version = "0.1.5";

src = fetchFromGitLab {
owner = "mann1x";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please tell us why you choose a fork instead of the origin repo.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ryzen_smu programs have gone through a lot of forks themselves.

ryzen_smu is the original ryzen_smu. It included monitor_cpu. ryzen_monitor extended monitor_cpu and ryzen_monitor_ng extended ryzen_monitor.

To support ryzen_monitor_ng, mann1x forked ryzen_smu but hasn't merged the changes. See here and here.

However, when I built ryzen_monitor_ng from mann1x's repo it wouldn't run because of a version mismatch in libsmu. kvic-z's fork of ryzen_monitor_ng corrects the version mismatch and adds Matisse.

The patches you suggest come from yet another fork.

I do like your approach of patching the original repos. I hope it doesn't lead to conflicts between mann1x's fork and moson-mo's. Worth a try, even if it leads to a lot of patches.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's so complex that I have just realized. I have noticed that the origin repo hasn't updated since half year ago. Do you think if it's a good idea that we maintain a fork in person, picking PRs and testing, then releasing.

Anyway, please at least leave a comment in the Nix packaging source code about it as long as you choose a repo which is not the origin one. Otherwise, other people who don't see this discussion would get confused.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is looking like we will need to maintain a repo with all the patches. PRs are not getting merged upstream quickly. Are you willing to set up the repo? I will change the src if you do.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To support ryzen_monitor_ng, mann1x forked ryzen_smu but hasn't merged the changes. See here and here.

I have noticed the same commits with different sha256 in mann1x's repo and leogx9r's repo, such as https://gitlab.com/leogx9r/ryzen_smu/-/commit/e61177d0ddaebfaeca52094b20a2289287a0838b and https://gitlab.com/mann1x/ryzen_smu/-/commit/adaf53c77dc68b99643ab7af729eb9c592cf5e01. Has it already rebased or I miss something other place.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is looking like we will need to maintain a repo with all the patches. PRs are not getting merged upstream quickly. Are you willing to set up the repo? I will change the src if you do.

https://github.com/Cryolitia/ryzen_smu

nixos/modules/hardware/ryzen-smu.nix Outdated Show resolved Hide resolved
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/ryzenadj-and-ryzen-smu/31326/4

@PhDyellow
Copy link
Author

I would be happy to co-maintain it with you. Given the number of forks and patches that ryzen_smu uses, I'll need the help.

Your patches contain some better nix code than I have written, but it looks like you wrote it independently to this PR. May I pull in the relevant parts of your patches and merge/squash/rebase the whole PR?

@Cryolitia
Copy link
Member

I would be happy to co-maintain it with you. Given the number of forks and patches that ryzen_smu uses, I'll need the help.

Your patches contain some better nix code than I have written, but it looks like you wrote it independently to this PR. May I pull in the relevant parts of your patches and merge/squash/rebase the whole PR?

Feel free to use my source code ^_^

@Cryolitia
Copy link
Member

Tested on multi AMD processors, results here: https://gitlab.com/leogx9r/ryzen_smu/-/merge_requests/12#note_1704774029

TLDR: Each processor has different issues, and no one processor has all the features working properly.

@PhDyellow
Copy link
Author

Thanks @Cryolitia, I have updated this PR with your recommendations. The patches apply cleanly to the mann1x/ryzen_smu fork, so I think we can get away without starting yet another repo.

If it looks good to you, I will squash the commits down for a clean merge.

@PhDyellow
Copy link
Author

Please test ryzen_smu with ryzen_monitor_ng, it produces more accurate readings than monitor_cpu

@ofborg ofborg bot requested a review from Cryolitia December 27, 2023 08:50
@Cryolitia
Copy link
Member

Cryolitia commented Dec 27, 2023

It's so complex that I have just realized. I have noticed that the origin repo hasn't updated since half year ago. Do you think if it's a good idea that we maintain a fork in person, picking PRs and testing, then releasing.

Just because that I have found other interesting PRs , such as https://gitlab.com/leogx9r/ryzen_smu/-/merge_requests/13

It's unacceptable to add more and more patches for a dead repo. And It will be a disaster one day these patches conflict.

@Cryolitia
Copy link
Member

please change this PR's title to ryzen_smu: init at 0.1.5, ryzen_monitor_ng: init at 2.0.5

@Cryolitia
Copy link
Member

Cryolitia commented Dec 27, 2023

tested PhDyellow@9221b58
image

@PhDyellow PhDyellow changed the title ryzen_smu and ryzen_monitor_ng ryzen_smu: init at 0.1.5, ryzen_monitor_ng: init at 2.0.5 Dec 28, 2023
@PhDyellow
Copy link
Author

Thanks for testing. It looks like the patches aren't working yet.

Can you share the output of:

sudo cat /sys/kernel/ryzen_smu_drv/codename

@PhDyellow
Copy link
Author

Ah, got it! monitor_cpu wasn't patched, so it wouldn't accept Phoenix CPUs.

@ofborg ofborg bot requested a review from Cryolitia December 28, 2023 01:22
nixos/modules/hardware/cpu/amd-ryzen-smu.nix Outdated Show resolved Hide resolved
nixos/modules/hardware/cpu/amd-ryzen-smu.nix Outdated Show resolved Hide resolved
nixos/modules/programs/ryzen-monitor-ng.nix Outdated Show resolved Hide resolved
nixos/modules/programs/ryzen-monitor-ng.nix Show resolved Hide resolved
Comment on lines 20 to 22
make clean
rm src/ryzen_monitor
make
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we use makeTarget and preBuild?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pointer, code should be clearer now.

pkgs/os-specific/linux/ryzen-smu/default.nix Outdated Show resolved Hide resolved
pkgs/os-specific/linux/ryzen-smu/default.nix Outdated Show resolved Hide resolved
pkgs/os-specific/linux/ryzen-smu/default.nix Outdated Show resolved Hide resolved
pkgs/os-specific/linux/ryzen-smu/default.nix Outdated Show resolved Hide resolved
@ofborg ofborg bot requested a review from Cryolitia April 10, 2024 02:09
Copy link
Member

@Aleksanaa Aleksanaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm. Please squash your commits.

@Aleksanaa
Copy link
Member

Why this suggested change was reverted? #271342 (comment)

@Aleksanaa
Copy link
Member

Sorry, the nixos module should be added in another commit (not package), and follow commit conventions in https://github.com/NixOS/nixpkgs/tree/master/nixos#commit-conventions

@PhDyellow
Copy link
Author

Why this suggested change was reverted? #271342 (comment)

Thanks for catching that, I didn't mean to revert that.

Sorry, the nixos module should be added in another commit (not package), and follow commit conventions in https://github.com/NixOS/nixpkgs/tree/master/nixos#commit-conventions

Makes sense, I will split up the commits as soon as I can.

Phil Dyer added 5 commits April 11, 2024 11:39
A Linux kernel driver that exposes access to the
SMU (System Management Unit) for certain AMD Ryzen Processors.

Contains monitor_cpu, a userspace tool for viewing info.

Using fork of original to match ryzen_monitor_ng, a more advanced
userspace tool for accessing the SMU via this kernel module,
planned for a later commit.
Provide a module for installing ryzen_smu, a Linux kernel driver
that exposes access to the SMU (System Management Unit) for
certain AMD Ryzen Processors.

Installs monitor_cpu, a userspace tool for viewing info.

Using fork of original to match ryzen_monitor_ng, a more advanced
userspace tool for accessing the SMU via this kernel module,
planned for a later commit.
A userspace tool for setting and getting AMD CPU power
and performance parameters.

Relies on the ryzen_smu kernel module.
A userspace tool for setting and getting AMD CPU power
and performance parameters.

The module adds `ryzen_monitor_ng` to `environment.systemPackages` and
enables the `ryzen-smu` module, as `ryzen_monitor_ng` requires the
`ryzen_smu` kernel module to function.
@ofborg ofborg bot requested a review from Cryolitia April 11, 2024 02:15
@Aleksanaa Aleksanaa merged commit 029c95c into NixOS:master Apr 11, 2024
26 checks passed
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/looking-for-documentation-on-maketargets-within-stdenv-mkderivation/45182/1

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