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

xorg: init xf86-video-vboxvideo at 1.0.0 #50280

Merged
merged 1 commit into from
Nov 14, 2018
Merged

xorg: init xf86-video-vboxvideo at 1.0.0 #50280

merged 1 commit into from
Nov 14, 2018

Conversation

vcunat
Copy link
Member

@vcunat vcunat commented Nov 12, 2018 •
edited
Loading

... and switch to it by default in virtualbox guests

Motivation for this change

#31889

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 nox --run "nox-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)
  • Fits CONTRIBUTING.md.

@vcunat
xorg: init xf86-video-vboxvideo at 1.0.0
9108b24 
... and switch to it by default in virtualbox guests
@vcunat vcunat mentioned this pull request Nov 12, 2018
Closed
@matthewbauer
Copy link
Member

Thanks for doing this!!! The only thing i would want to make sure is that it’s kept reasonably up to date from version to version.

@GrahamcOfBorg GrahamcOfBorg added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Nov 12, 2018
@vcunat
Copy link
Member Author

vcunat commented Nov 12, 2018 •
edited
Loading

So far they've only released a single version. It gets compiled against our default xorg-server, just like all our standard X drivers.

matthewbauer
matthewbauer reviewed Nov 13, 2018
View reviewed changes
nixos/modules/services/x11/xserver.nix
@@ -13,7 +13,8 @@ let

# Map video driver names to driver packages. FIXME: move into card-specific modules.
knownVideoDrivers = {
virtualbox = { modules = [ kernelPackages.virtualboxGuestAdditions ]; driverName = "vboxvideo"; };
Copy link
Member

Choose a reason for hiding this comment

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

@vcunat I think we should leave in virtualbox guest additions for other things. I believe it provides more than just the driver. If it's listed last, I think it will get lower precedence in the case of conflicts.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, didn't realize this only effects xserver, so disregard the previous comment.

@jhillyerd
Copy link
Contributor

Tried it on Windows 10 amd64 virtualbox host, log: https://gist.github.com/jhillyerd/6ab6d20ea440c4da2865bbb34db24cc8

The driver does load now, but still falls back to modeset for me. Might be my vbox setup though.

@matthewbauer
Copy link
Member

That’s what it should do i think.

@vcunat
Copy link
Member Author

vcunat commented Nov 14, 2018

It's X driver. I can't see any relation to text-mode modesetting, if that's what you mean.

@vcunat vcunat merged commit 9108b24 into NixOS:master Nov 14, 2018
vcunat added a commit that referenced this pull request Nov 14, 2018
@vcunat
Merge #50280: xorg: init xf86-video-vboxvideo ...
e229065 
Based on reports X wouldn't start out of the box and seems OK now.
In case there are still some problems, we can improve later.
I checked that nixos.tests.virtualbox.* still succeed.
@vcunat vcunat deleted the p/vboxvideo branch November 14, 2018 19:42
@vcunat
Copy link
Member Author

vcunat commented Nov 14, 2018 •
edited
Loading

I wonder – this might even be safe enough for 18.09, given the (supposedly) broken state of X before the change. Still, I know really nothing about VMs, so I won't do that myself, at least until getting more feedback.

@jhillyerd
Copy link
Contributor

If I understand correctly, modesetting is also a video driver of sorts that can draw on basic framebuffer. But I haven't spent time configuring Xorg in years, so I'm very out of the loop.

[    13.108] (II) VBoxVideo: guest driver for VirtualBox: vbox
[    13.108] (II) modesetting: Driver for Modesetting Kernel Drivers: kms
[    13.111] (II) vboxvideo: kernel driver found, not loading.
[    13.112] (II) modeset(0): using drv /dev/dri/card0

I don't understand the "kernel driver found" part.

This PR didn't make anything worse for me, X worked before and after it.

@vcunat
Copy link
Member Author

vcunat commented Nov 14, 2018

Oh, I've been blind. You end up using the modesetting driver, both before and after this PR :-/

@vcunat vcunat mentioned this pull request Jan 13, 2019
Closed
10 tasks
@milibopp
Copy link
Contributor

One should be able to use this fix by switching to 19.03, right?

@vcunat
Copy link
Member Author

vcunat commented Feb 28, 2019

Yes, unstable or 19.03. But I'm not really confident this change fixed anything.

@macalinao
Copy link
Contributor

I don't think this fixed things for me.

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

@matthewbauer matthewbauer matthewbauer left review comments

@infinisil infinisil Awaiting requested review from infinisil

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 9.needs: community feedback 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@vcunat @matthewbauer @jhillyerd @milibopp @macalinao @GrahamcOfBorg