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

treewide: big opengl cleanups #320228

Merged
merged 3 commits into from
Jun 20, 2024
Merged

treewide: big opengl cleanups #320228

merged 3 commits into from
Jun 20, 2024

Conversation

K900
Copy link
Contributor

@K900 K900 commented Jun 16, 2024

Description of changes

This is long overdue.

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.11 Release Notes (or backporting 23.11 and 24.05 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.

Add a 👍 reaction to pull requests you find important.

It's dead, Jim.
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` labels Jun 16, 2024
@K900 K900 force-pushed the opengl-cleanups branch 2 times, most recently from b68060a to 034628c Compare June 16, 2024 11:05
@mweinelt
Copy link
Member

Manual build probably requires updating nixos/doc/manual/configuration/gpu-accel.chapter.md.

@github-actions github-actions bot added the 6.topic: steam Steam game store/launcher (store.steampowered.com) label Jun 16, 2024
@K900
Copy link
Contributor Author

K900 commented Jun 16, 2024

Yep, forgot to commit all the renames. Should be there now.

K900 added 2 commits June 16, 2024 14:11
- rename hardware.opengl to hardware.graphics
- remove hardware.opengl.driSupport, which does nothing
- remove hardware.opengl.setLdLibraryPath, which should never be done
- rename hardware.opengl.driSupport32Bit to hardware.graphics.enable32Bit
- lost of small docs / formatting cleanups
@K900 K900 force-pushed the opengl-cleanups branch from 034628c to 1e3c610 Compare June 16, 2024 11:11
imports = [
(lib.mkRenamedOptionModule [ "services" "xserver" "vaapiDrivers" ] [ "hardware" "opengl" "extraPackages" ])
(lib.mkRemovedOptionModule [ "hardware" "opengl" "s3tcSupport" ] "S3TC support is now always enabled in Mesa.")
(lib.mkRemovedOptionModule [ "hardware" "opengl" "driSupport"] "The setting can be removed.")
Copy link
Member

Choose a reason for hiding this comment

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

We should probably mention, that this was a no-op in the first place

@Tungsten842
Copy link
Member

Tungsten842 commented Jun 16, 2024

I am not sure that "graphics" is a good name in this case.
Consider that hardware.opengl.extraPackages is currently also used by: opencl and level-zero.
I believe that putting compute only APIs in a "graphics" namespace causes unnecessary confusion, the previous naming, hardware.drivers seemed more appropriate in this case: #158079

@ofborg ofborg bot requested review from jagajaga and jonringer June 16, 2024 12:03
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jun 16, 2024
@K900
Copy link
Contributor Author

K900 commented Jun 16, 2024

hardware.drivers is even worse, because it gives you absolutely no idea what "drivers" there are or why you should care. graphics is suboptimal, but accelerators is way too non-obvious.

@Atemu
Copy link
Member

Atemu commented Jun 16, 2024

The idea would be to have hardware.drivers.vulkan, hardware.drivers.opengl etc. which would make it quite obvious which drivers there are. Even non-nerdy people use the term "drivers" to refer to this software component. If they typed "vulkan drivers nvidia" into search.nixos.org, I believe they'd find the correct option.

I also take issue with graphics though, especially as many signs point to more task-specific accelerator hardware in the future that has nothing to do with graphical processing.
I'd find enabling drivers for a compression accelerator via hardware.graphics equally confusing as configuring Vulkan via hardware.opengl. Let's not repeat that mistake.

I agree that accelerator is much to technical of a term but I simply know of no better term that is neutral with respect to the task being carried out.

Additionally, uninformed users should never need to be touching these. A typical desktop user for instance should only have to enable a desktop session and/or Steam and those would in turn enable the accelerator APIs typically required for those use-cases.
Anyone who does need to touch these manually ought to learn some terminology. I think that's a reasonable ask.

I won't block on this as this is already an improvement over the status quo and I've clearly not found the priority to produce code for my preferred options API but please consider my design choices and why I made them.

@K900
Copy link
Contributor Author

K900 commented Jun 16, 2024

Really the problem with separating the options like what you're proposing is that Mesa is all of those at the same time, and Mesa is what most people will be running. Technically, we could separate OpenGL, OpenCL, Vulkan, VAAPI etc lookup paths, since they're symlinks anyway, and just link the right manifests into place, but that feels like a lot of work for questionable gain?

@Atemu
Copy link
Member

Atemu commented Jun 16, 2024

Really the problem with separating the options like what you're proposing is that Mesa is all of those at the same time

Not quite and that's an issue. It's a geat implementation of most of the most critical and useful acceleration APIs but by far not all of them. I love mesa but I don't think it's a good idea to equate it with hardware acceleration as a whole. There is a world outside of Mesa and it does not support every accel API under the sky and that's an issue if you center your NixOS options around everything being mesa. Think CUDA, ROCm, oneAPI etc. or even just alternative implementations of the APIs mesa supports for which there are valid use-cases.
This is also again quite graphics-centric thinking as there are far more hardware accelerators than just GPUs but require userspace drivers in much the same way.

Mesa is what most people will be running

That is true but enforcing that through the design of the NixOS options is the wrong way of achieving that IMO. Ensuring the user has the most sensible drivers installed should be handled through use-case-driven sane defaults.

Technically, we could separate OpenGL, OpenCL, Vulkan, VAAPI etc lookup paths, since they're symlinks anyway, and just link the right manifests into place, but that feels like a lot of work for questionable gain?

You don't have to do but designing it like that leaves you the option to should that become relevant. In my initial implementation I still threw them into /run/opengl-driver but you could totally separate it in the implementation later you wanted to.

Speaking of which, I had a draft with some progress somewhere with some more notes and discussion but I'm having a really hard time finding it. I'll link it here when I find it.

@Atemu
Copy link
Member

Atemu commented Jun 16, 2024

Found it: #141803 (comment)

@K900
Copy link
Contributor Author

K900 commented Jun 17, 2024

I do really like your draft. I'll see if I can muster the spoons to actually try implementing it, because it does not seem too difficult now...

@jonringer
Copy link
Contributor

I'll defer to @Atemu on this matter. I removed most of the references of opengl from packaging concerns, module re-design I like their direction, and looks like the related information was already brought up.

@K900
Copy link
Contributor Author

K900 commented Jun 20, 2024

OK, let's just send this, and I'll try a more ambitious cleanup over the weekend.

@K900 K900 merged commit 20b7b4f into NixOS:master Jun 20, 2024
24 checks passed
Copy link
Contributor

Backport failed for release-24.05, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release-24.05
git worktree add -d .worktree/backport-320228-to-release-24.05 origin/release-24.05
cd .worktree/backport-320228-to-release-24.05
git switch --create backport-320228-to-release-24.05
git cherry-pick -x 951601ccab7c3e8f9afa3ed78f2046863e6fa81d 98cef4c27326d0f9e521654441929c1c7c64f8e9 1e3c610b8442bb22be3ca39542894f16d3c94d5c

@K900
Copy link
Contributor Author

K900 commented Jun 21, 2024

Definitely not.

@K900 K900 deleted the opengl-cleanups branch June 21, 2024 16:42
@themaxhero
Copy link
Contributor

themaxhero commented Jun 21, 2024

hardware.drivers is even worse, because it gives you absolutely no idea what "drivers" there are or why you should care. graphics is suboptimal, but accelerators is way too non-obvious.

What about hardware.gpu?

@K900
Copy link
Contributor Author

K900 commented Jun 21, 2024

It's not really just GPUs, but anyway, I'll have a PR with a more granular approach to this eventually.

@JohnRTitor
Copy link
Contributor

Definitely not.

Is there a reason not to? Due to nixos-hardware not being versioned, about time we get bug reports from stable users.

@K900
Copy link
Contributor Author

K900 commented Jun 21, 2024

It's an API break. We don't do those on stable releases. nixos-hardware should just use the old options for now, and remove the removed ones.

@SomeoneSerge
Copy link
Contributor

Oh, not sure how I missed this. This is great and indeed long overdue, thank you @K900!

Regarding the naming, I think hardware.graphics isn't much different from hardware.opengl and about as misleading. As you say it's not "just gpus", and it's not "just graphics". I still like hardware.drivers (personal preference: hardware.impure-drivers) as the most precise so far. I am not concerned with how much or little information this name conveys to an uninformed reader on the first encounter. NixOS is in the process of establishing essentially a new concept here: separating pure dependencies/services from those that must be kept impure. As long as the new name (whatever it be) makes sense, is reasonably documented and pointed at with mkRenamedOptionModule, it's perfectly reasonable to introduce a new term once in a while.

@jhvst
Copy link
Contributor

jhvst commented Jun 28, 2024

Why was setLdLibraryPath removed?

@Atemu
Copy link
Member

Atemu commented Jun 28, 2024

AFAICT because the last driver relying on this terrible hack was removed (AMDGPU-PRO).

It should receive a mkRenamedOptionModule though.

What's your use-case?

@jhvst

This comment was marked as off-topic.

@K900

This comment was marked as off-topic.

@jhvst

This comment was marked as off-topic.

octvs added a commit to octvs/deepl that referenced this pull request Aug 25, 2024
Atry added a commit to Atry/nixos-wsl-vscode that referenced this pull request Sep 10, 2024
Atry added a commit to Atry/nix-ml-ops that referenced this pull request Sep 12, 2024
sigprof added a commit to sigprof/nur-packages that referenced this pull request Jan 1, 2025
NixOS/nixpkgs#320228 removed the `hardware.opengl.setLdLibraryPath`
option, which was required for the `legacy_340` NVIDIA driver to work
properly (NixOS/nixpkgs#120602).  The machine which needed the
`legacy_340` driver is no longer in use for quite some time, so there is
no way to test any possible fixes; just drop the obsolete option.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: steam Steam game store/launcher (store.steampowered.com) 8.has: clean-up 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants