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.

Sign up for GitHub

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

Jump to bottom

nixos/nvidia: Add NVIDIA optimus option to allow external GPUs #59856

Merged
merged 1 commit into from May 3, 2019
Merged

nixos/nvidia: Add NVIDIA optimus option to allow external GPUs #59856

merged 1 commit into from May 3, 2019

Conversation

c00w
Copy link
Contributor

@c00w c00w commented Apr 18, 2019 •
edited

Without this option - NVIDIA refuses to use an external GPU.

Motivation for this change

I have a computer with an external NVIDIA GPU connected. I want to run some monitors off the external GPU and some off the internal intel GPU - After a lot of debugging, this was the required change.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@c00w c00w requested a review from infinisil as a code owner April 18, 2019 22:56
@worldofpeace
Copy link
Contributor

It's customary to prefix a commit msg to a nixos module with nixos/$modulename.
In this case it would be nixos/nvidia

https://github.com/NixOS/nixpkgs/blob/master/.github/CONTRIBUTING.md#submitting-changes

@c00w
Copy link
Contributor Author

c00w commented Apr 19, 2019

Thanks for taking a look - should all be sorted now :D

@c00w c00w force-pushed the external_gpu branch 3 times, most recently from 3b19954 to 51bd263 Compare April 21, 2019 19:13
@infinisil
Copy link
Member

Would making this be enabled by default be a problem?

@c00w
Copy link
Contributor Author

c00w commented Apr 23, 2019

Default enabling it shouldn't be a problem + I updated the code to do that.

If it's enabled by default do we still need the option to disable it? It seems like a safe conservative choice to leave it in case someone finds problems or doesn't want to allow it.

@infinisil
Copy link
Member

What do the docs on this option say? I feel like there might be some security implications, but I really have no idea. If there's no problems with enabling it I don't think we need a NixOS option for it even.

@c00w
Copy link
Contributor Author

c00w commented Apr 24, 2019

https://download.nvidia.com/XFree86/Linux-x86_64/396.51/README/xconfigoptions.html#AllowExternalGpus says that if you unplug the GPU you'll have issues. No security issues are mentioned

@infinisil
Copy link
Member

Eh, after reading that description I'm pretty sure we should leave it at false by default and leave it configurable. There's apparently reasons it's false by default.

@c00w
Copy link
Contributor Author

c00w commented Apr 24, 2019

Flipped it back

@c00w
nixos/nvidia: Add NVIDIA optimus option to allow external GPUs
d674949 
Without this option - NVIDIA refuses to use an external GPU.
@flokli
Copy link
Contributor

flokli commented May 3, 2019

@infinisil is this good to merge?

@worldofpeace worldofpeace changed the title Add NVIDIA optimus option to allow external GPUs nixos/nvidia: Add NVIDIA optimus option to allow external GPUs May 3, 2019
worldofpeace
worldofpeace approved these changes May 3, 2019
View reviewed changes
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@worldofpeace worldofpeace merged commit a01943c into NixOS:master May 3, 2019
@worldofpeace
Copy link
Contributor

Thank you @c00w for contributing 🎆

Sorry about all the flipping back and forth, but we did figure out why it's false by default
so we won't break anyone's setups.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@worldofpeace worldofpeace worldofpeace approved these changes

@infinisil infinisil Awaiting requested review from infinisil

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@c00w @worldofpeace @infinisil @flokli @GrahamcOfBorg