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

nixos/jellyfin: better defaults for hardware acceleration #175439

Merged
merged 2 commits into from
Jul 18, 2022

Conversation

Mic92
Copy link
Member

@Mic92 Mic92 commented May 30, 2022

Description of changes

I have not tested any of that. I just changed this based on recent additions to the wiki: https://nixos.wiki/wiki/index.php?oldid=7605&rc_id=7898

cc @makefu for testing

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 22.05 Release Notes (or backporting 21.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot 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/` labels May 30, 2022
DeviceAllow = [
"/dev/dri/*"
];
SupplementaryGroups = [ "video" ];
Copy link
Member Author

Choose a reason for hiding this comment

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

The wiki also suggests render. However I don't think nixos has such a group.

Copy link
Member

Choose a reason for hiding this comment

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

On my system, /dev/dri/render* devices belongs to the render group (and /dev/dri/card* to the video group), so I think it is needed.

Copy link
Member

Choose a reason for hiding this comment

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

Same

$ ls /dev/dri/render* -l
crw-rw-rw- 1 root render 226, 128 May 28 22:38 /dev/dri/renderD128

@@ -64,14 +64,16 @@ in
AmbientCapabilities = "";
CapabilityBoundingSet = "";

# ProtectClock= adds DeviceAllow=char-rtc r
Copy link
Member Author

Choose a reason for hiding this comment

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

Not so sure what to do with this one? Do I still need to pass an empty DeviceAllow before I make additions?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess its not relevant anymore because we are already dropped ProtectClock before that.

Copy link
Member

Choose a reason for hiding this comment

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

I would personally replace this line with ProtectClock = true, now. I think I remember putting that comment here because there was a conflict between ProtectClock = true and DeviceAllow = "" / PrivateDevices = true, but it shouldn't bother us anymore. In any case, I don't see a usecase for jellyfin to modify the hardware / system clock.

Copy link
Member Author

@Mic92 Mic92 Jun 18, 2022

Choose a reason for hiding this comment

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

It cannot do this anyway because the system call filter below blocks such system calls. I don't think an unprivileged user could do this anyway and setuid is not allowed either.

@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 May 30, 2022
@Artturin Artturin requested a review from minijackson May 30, 2022 08:04
@Artturin
Copy link
Member

@peperunas

@makefu
Copy link
Contributor

makefu commented May 30, 2022

The "render" group came from the jellyfin documentation, just added it ( https://jellyfin.org/docs/general/administration/hardware-acceleration.html )

I thought about adding a flag for enabling access to /dev/dri, however i am unsure if it would really provide some security benefits in contrast to the complexity of users trying to find out why hardware transcode does not work.

"/dev/dri/*"
];
SupplementaryGroups = [ "video" ];
PrivateDevices = false;
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to explicitly set it to false here? I'm not aware of an option set here that sets PrivateDevices = true.

Copy link
Contributor

Choose a reason for hiding this comment

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

The main reason was that when keeping it set to true still does not work for me ... i have not investigated further

Copy link
Member

Choose a reason for hiding this comment

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

as a note we should probably keep the comment original

Disabled to allow Jellyfin to access hw accel devices endpoints

Copy link
Member

Choose a reason for hiding this comment

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

I think my wording was a bit ambiguous: I don't mind having PrivateDevices set to false, but since it should be the default behavior, why not remove this line entirely?

@minijackson
Copy link
Member

I haven't tested this yet, but in principle I'm all for it. One thing to note is from the systemd.resource-control(5) man page, under the DeviceAllow= section:

The device group is matched according to filename globbing rules, you may hence use the "*" and "?" wildcards. (Note that such globbing wildcards are not available for device node path specifications!)

From what I understand, this would mean that DeviceAllow = [ "/dev/dri/*" ]; isn't understood by systemd? We might need to put DeviceAllow = [ "char-drm rw" ]; instead.

@makefu
Copy link
Contributor

makefu commented May 31, 2022

@minijackson i will test your proposal

@06kellyjac
Copy link
Member

see also:

#108224 (comment)

@makefu
Copy link
Contributor

makefu commented May 31, 2022

I checked the different parameters, however i have not managed to get jellyfin to transcode without setting privateDevices=false

Upstream also essentially gave up: jellyfin/jellyfin@dd8b9e9#diff-3d806a09406ac27e205da315a5f9ad987eec663f657b3dda296b4712c8a73d73R27

according to #163491 (comment) adding nvidia-specific whitelisting works, however i cannot get it running for vaapi with my intel card.

Error:

[AVHWDeviceContext @ 0x1d119c0] No VA display found for device /dev/dri/renderD128.
Device creation failed: -22.
Failed to set value '/dev/dri/renderD128' for option 'vaapi_device': Invalid argument
Error parsing global options: Invalid argument

adding "render" supplementaryGroup is mandatory for transcoding to work

@Artturin
Copy link
Member

we should use upstream settings unless we need to do something nixos specific
https://github.com/jellyfin/jellyfin/blob/master/debian/jellyfin.service

@Mic92
Copy link
Member Author

Mic92 commented Jun 18, 2022

we should use upstream settings unless we need to do something nixos specific https://github.com/jellyfin/jellyfin/blob/master/debian/jellyfin.service

I synced this up now with upstream.

Comment on lines 58 to 62
StateDirectory = "jellyfin";
StateDirectoryMode = "0700";
CacheDirectory = "jellyfin";
CacheDirectoryMode = "0700";
UMask = "0077";
Copy link
Member Author

Choose a reason for hiding this comment

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

Those are the only settings we add. Everything else should be pretty much the same as upstream.

@@ -49,56 +49,61 @@ in
after = [ "network.target" ];
wantedBy = [ "multi-user.target" ];

# This is mostly follows: https://github.com/jellyfin/jellyfin/blob/master/debian/jellyfin.service
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean https://github.com/jellyfin/jellyfin/blob/master/fedora/jellyfin.service (fedora instead of debian)? I got confused because following the link I saw no hardening

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like they dropped stuff again from debian.

@minijackson
Copy link
Member

@ofborg test jellyfin

Copy link
Member

@minijackson minijackson left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@bobby285271 bobby285271 added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jul 4, 2022
@Mic92 Mic92 merged commit 9a020f3 into NixOS:master Jul 18, 2022
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 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 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants