fix(linux): drop implicit DRM master for card fds#5286
Conversation
|
|
I would like to emphasize again that this issue is both important and long-standing. Due to this problem, compositors and display managers may fail to start successfully if Sunshine is running beforehand. The root cause is related to DRM master ownership: these components require DRM master privileges to operate correctly. However, when no process currently holds DRM master, the first process that opens the DRM device will implicitly acquire it — in this case, Sunshine. The issue is that Sunshine does not release the DRM master after acquiring it. As a result, users are forced to restart Sunshine whenever they switch between a display manager and a compositor, which significantly disrupts the workflow. This pull request has been open for three weeks. I would appreciate if it could receive some attention, as it addresses a real usability issue affecting daily usage. |
|
yes, i will fix conflict be made this days, and test it. |
Kishi85
left a comment
Kishi85
left a comment
There was a problem hiding this comment.
I've had this on my local (boilerplate) install for a bit before it broke and couldn't find any obvious issues.
Tested it because it might fix a system freeze issue I'm having with relogin of the same user due to a wayland crash (but no luck so far on that).
| * @return A file descriptor with `O_CLOEXEC` set that is guaranteed not to be DRM master, | ||
| * or `-1` on failure. Any partially-opened fd is closed before returning `-1`. | ||
| */ | ||
| int open_drm_card_fd_non_master(const char *path) { |
There was a problem hiding this comment.
IMHO the parameter here should be a std::filesystem::path as we should use the proper and safe C++ types as long as possible.
I'd also suggest rolling the
#ifdef SUNSHINE_BUILD_DRM
return platf::open_drm_card_fd_non_master(device_path.c_str());
#else
return open(device_path.c_str(), O_RDWR);
#endifpattern (from other files) into this function and using it throughout the rest of the code instead to keep things simpler (also changing the function name to reflect this) and just call that function:
int open_drm_card_fd(std::filesystem::path path, int flags=O_RDWR) {
#ifdef SUNSHINE_BUILD_DRM
//<Insert-your-open_drm_card_fd_non_master-code-here>//
#else
return open(path.c_str(), flags);
#endif
}EDIT: Added suggestion to keep other codeparts simpler
|
|
||
| auto master = is_master(); | ||
| if (master < 0) { | ||
| return close_and_fail(); |
There was a problem hiding this comment.
Not sure if we should fail if we cannot drop (or check for) DRM master (here and in R216, R223, R228) as it's not ideal but functionally still works albeit with issues (as it does right now).
Logging a warning should be enough to hint at DRM master problems could be related to Sunshine.
We do the same for process capabilities in case we cannot drop them.
| } | ||
|
|
||
| std::string path = std::string("/dev/dri/") + entry->d_name; | ||
| int fd = open(path.c_str(), O_RDWR); |
There was a problem hiding this comment.
Also not sure if it's necessary here but this might also need to be changed? Just close the comment if I'm wrong.
Subsequent open() using the result of find_render_node_with_display() elsewhere in the codebase might also need updating to catch all cases.
EDIT: I'd also highly recommend using the pattern suggested for open_drm_card_fd for this and the subsequent changes to keep things simple.
EDIT2: Not catching all the cases might explain my still ongoing problems (with wayland sessions) when using kwingrab/portalgrab.
There was a problem hiding this comment.
Missing open() calls I've found:
Sunshine/src/platform/linux/wayland.cpp
Line 250 in 3891e6a
Sunshine/src/platform/linux/vaapi.cpp
Line 752 in 3891e6a
There was a problem hiding this comment.
For those two missing open() calls, they don't need to resolve the DRM master problem. They only need to consider the DRM master issue when opening .../cardX, not .../renderDX.
|
See Kishi85@0f45c3e as an example for my earlier suggestions. Feel free to just cherry pick that change or include it yourself (a |
panxi39
force-pushed
the
master
branch
2 times, most recently
from
473c04c to
f344356
Compare
|
There is another problem now. Whenever we open a DRM primary node ( It happens regardless of why we opened the node. In particular it happens on CUDA-only builds too:
To close this gap I think we should link libdrm whenever anything in the build opens a card primary node, i.e. also when Concretely, the smallest change would be: if(${SUNSHINE_ENABLE_DRM} OR ${SUNSHINE_ENABLE_WAYLAND} OR ${SUNSHINE_ENABLE_VULKAN}
OR ${SUNSHINE_ENABLE_KWIN} OR ${SUNSHINE_ENABLE_PORTAL} OR CUDA_FOUND)
find_package(LIBDRM REQUIRED)What do you think about this problem? |
I think if they set SUNSHINE_ENABLE_DRM to OFF in the configure/build, we should not use anything that requires DRM. |
|
I re-checked the build paths and adjusted the change accordingly. For the normal Linux builds, Based on that, I wrapped the libdrm include and |
do you mean that if drm not enabled, cuda shouldn't be enable? add those judge logic in make scripts? |
|
i think the only except condition is only enable x11 and cuda? |
I agree with @ReenigneArcher here if libdrm is not desired we should not call anything that needs it. I'd propose this fix which add an additional log message for the CUDA without libdrm edge case on Linux (intentionally just for Linux and as an info message as it's not due to an error): diff --git a/src/platform/linux/misc.cpp b/src/platform/linux/misc.cpp
index 21f69c26..af984a90 100644
--- a/src/platform/linux/misc.cpp
+++ b/src/platform/linux/misc.cpp
@@ -156,7 +156,6 @@ namespace platf {
*/
using ifaddr_t = util::safe_ptr<ifaddrs, freeifaddrs>;
-#ifdef SUNSHINE_BUILD_DRM
/**
* @brief Open a DRM card node, dropping implicit DRM master when possible.
*
@@ -164,6 +163,7 @@ namespace platf {
* as warnings but do not fail the call.
*/
int open_drm_card_fd(const std::filesystem::path &path, int flags) {
+#if SUNSHINE_BUILD_DRM
int fd = open(path.c_str(), flags | O_CLOEXEC);
if (fd < 0) {
BOOST_LOG(error) << "Couldn't open: "sv << path.string() << ": "sv << strerror(errno);
@@ -222,8 +222,13 @@ namespace platf {
}
return fd;
- }
+#else
+ #ifndef __FreeBSD__
+ BOOST_LOG(info) << "Sunshine compiled without DRM support. Cannot control Linux DRM master state for "sv << path.string();
+ #endif
+ return open(path.c_str(), flags);
#endif
+ } |
When the first process opens /dev/dri/cardN, the kernel implicitly grants it DRM master. Sunshine never released this, which prevented compositors and display managers from re-acquiring master on VT switches and forced users to restart Sunshine after every session change. Introduce platf::open_drm_card_fd() that opens the node with O_CLOEXEC, probes it with DRM_IOCTL_AUTH_MAGIC, and calls drmDropMaster if the kernel implicitly handed us master. Co-Authored-By: Kishi85 <41839133+Kishi85@users.noreply.github.com>
|
@Kishi85 Could you please help me test this patch in your environment? If compositor start normally when sunshine start first and can freely switch betwen dm and compostior? |
|
@panxi39 Unfortunately can't test that right now as I have another issue (still unsure about the cause but it's happening without sunshine being started as well) preventing me from doing just that on my system, causing my KDE wayland session to fail and freeze the whole DRM in the process upon switching. From a logical standpoint this PR now retains all previous functionality with added log messages and tries to resolve the implicit master issue if possible. Just applying it does not cause any immediate issues which is a good thing. |
I think this pr should not be merge now, i will test it later to make sure this change definately resloved the problem. |
|
I have thoroughly tested this patch in my environment and it works perfectly. I have verified its compatibility with SDDM, GDM, and Getty on Niri, and all display managers/sessions are normal. Please consider to merge this patch now. |
|
LGTM as well @ReenigneArcher this is good to merge |
3 participants
Description
Sunshine does not require DRM master privileges, but it may unintentionally acquire DRM master when opening a DRM device before other applications.
This can prevent applications that actually need DRM master, such as compositors, from running correctly.
Automatically drop DRM master after opening the DRM device to avoid holding unnecessary DRM master ownership and reduce conflicts with other DRM users.
Screenshot
Issues Fixed or Closed
Roadmap Issues
Type of Change
Checklist
AI Usage