From 919f3b50974d7398cd7c3fd9a3080576ae5bbd5e Mon Sep 17 00:00:00 2001 From: Mariano Abad Date: Fri, 24 Jul 2026 01:00:17 -0300 Subject: [PATCH] drm: convert each display on the GPU that exports it The unprivileged converter opened its render context with drmtap_open_render(NULL), letting libdrmtap auto-select. On a multi-GPU host that can land on a different GPU than the one driving the display, and importing a scanout across vendors can fail permanently on an incompatible tiling modifier. The service already knows the exporting device, so it now names its render node (drmtap_render_node, libdrmtap 0.4.15) in each DrmDisplayInfo, and the consumer opens the converter on that node. The field is serde(default) and empty means auto-select, so a service and a server from mismatched builds still interoperate and a pre-0.4.15 .so degrades to exactly the previous behaviour. The path is realpath-gated to /dev/dri before it is opened, the same gate the capture device gets, since it arrives over IPC. When the named node cannot be opened the converter returns None and the existing need_cpu fallback runs the convert on the exporting GPU service-side, which is the most correct place for it anyway. Added a wire-compat test that a pre-render_node DrmDisplayInfo payload still decodes (empty node) and a current one round-trips the node. --- libs/scrap/src/common/drm_reader.rs | 24 +++++++++++- libs/scrap/src/common/drm_render.rs | 60 ++++++++++++++++++++++------- libs/scrap/src/common/drmtap_dl.rs | 10 +++++ src/ipc.rs | 43 +++++++++++++++++++++ src/server/drm_capturer.rs | 13 ++++++- 5 files changed, 135 insertions(+), 15 deletions(-) diff --git a/libs/scrap/src/common/drm_reader.rs b/libs/scrap/src/common/drm_reader.rs index e4516cdef..b271e3f22 100644 --- a/libs/scrap/src/common/drm_reader.rs +++ b/libs/scrap/src/common/drm_reader.rs @@ -51,7 +51,7 @@ pub struct DisplaySnapshot { /// Returns true only if `path` canonicalizes to a node directly under /dev/dri/. /// This is the realpath gate the libdrmtap helper applied but the in-process /// (direct) path does not, so the service must apply it itself. -fn device_under_dev_dri(path: &str) -> bool { +pub(super) fn device_under_dev_dri(path: &str) -> bool { match std::fs::canonicalize(path) { Ok(p) => p.parent().map_or(false, |d| d == std::path::Path::new("/dev/dri")), Err(_) => false, @@ -223,6 +223,28 @@ impl DrmReader { self.lib.grab_desc.is_some() } + /// Render node (`/dev/dri/renderD*`) of the GPU this reader captures from, to + /// hand to the unprivileged converter so it binds to the device that EXPORTS + /// the scanout. On a multi-GPU host the converter's own auto-selection can + /// land on a different GPU, and importing a scanout across vendors can fail + /// on an incompatible tiling modifier. `None` on a pre-0.4.15 `.so` (the + /// symbol is absent) or on a display-only device with no render node; the + /// converter then auto-selects exactly as before. + pub fn render_node(&mut self) -> Option { + let f = self.lib.render_node?; + // SAFETY: self.ctx is a valid context. The returned pointer is owned by + // the context and stays valid until it is closed, so copying out of it + // here (while &mut self is held) cannot outlive it. + let ptr = unsafe { f(self.ctx) }; + if ptr.is_null() { + return None; + } + unsafe { std::ffi::CStr::from_ptr(ptr) } + .to_str() + .ok() + .map(|s| s.to_owned()) + } + /// Zero-copy EXPORT grab for the split-capture path (root `--service`). Calls /// `drmtap_grab_desc`, which fills a `drmtap_dmabuf_desc` (the scanout dma-buf /// fd + the full plane layout + HDR metadata) WITHOUT mapping, detiling or diff --git a/libs/scrap/src/common/drm_render.rs b/libs/scrap/src/common/drm_render.rs index eb5be020a..605acc3ef 100644 --- a/libs/scrap/src/common/drm_render.rs +++ b/libs/scrap/src/common/drm_render.rs @@ -17,6 +17,7 @@ use super::drmtap_dl::{self, drmtap_ctx, drmtap_dmabuf_desc, drmtap_frame_info, DrmtapLib}; use super::Pixfmt; use hbb_common::log; +use std::ffi::CString; use std::io; use std::os::fd::RawFd; @@ -48,14 +49,16 @@ pub struct RenderConverter { } impl RenderConverter { - /// Open an unprivileged DRM render-node convert context (`drmtap_open_render(NULL)` - /// auto-selects a render node — it opens no KMS card, spawns no helper, and needs - /// no elevated capability). Returns `None` when libdrmtap is unavailable, the split - /// convert symbols are missing (a pre-0.4.9 `.so`), or no render node could be - /// opened (a locked-down seat with no `/dev/dri/renderD*` access) — the caller then - /// degrades to the CPU-mapped / PipeWire path. MUST be called on the thread that - /// will later `convert()` and drop it. - pub fn open_render() -> Option { + /// Open an unprivileged DRM render-node convert context. `node` is the render node + /// of the GPU that exports the scanout (from the service's display list); `None` or + /// an empty/invalid path falls back to libdrmtap auto-selection. It opens no KMS + /// card, spawns no helper, and needs no elevated capability. Returns `None` when + /// libdrmtap is unavailable, the split convert symbols are missing (a pre-0.4.9 + /// `.so`), or no render node could be opened (a locked-down seat with no + /// `/dev/dri/renderD*` access) — the caller then degrades to the service-side CPU + /// convert / PipeWire path. MUST be called on the thread that will later `convert()` + /// and drop it. + pub fn open_render(node: Option<&str>) -> Option { let lib = drmtap_dl::get()?; // The converter needs BOTH split symbols; bail (so the caller degrades) if either // is absent, rather than open a ctx we could never convert with. @@ -67,14 +70,45 @@ impl RenderConverter { ); return None; } - // SAFETY: `open_render` is a resolved C entry point; NULL requests auto-selection - // of a render node. - let ctx = unsafe { open_render(std::ptr::null()) }; + // The service names the render node of the GPU that EXPORTS the scanout, which + // is the only device guaranteed to understand its tiling modifier; auto-select + // (NULL) is the fallback when it cannot. Same /dev/dri gate the capture device + // gets: the path arrives over IPC, and while the peer is root, a converter that + // opens whatever path it is handed is a needless widening. + let node_cstr = match node.filter(|n| !n.is_empty()) { + None => None, + Some(n) => { + if !super::drm_reader::device_under_dev_dri(n) { + log::warn!("drm: render node {n:?} is not under /dev/dri; auto-selecting"); + None + } else { + match CString::new(n) { + Ok(c) => Some(c), + Err(_) => None, // interior NUL + } + } + } + }; + // SAFETY: `open_render` is a resolved C entry point; `node_cstr` outlives the + // call, and NULL requests auto-selection of a render node. + let ctx = unsafe { + open_render(node_cstr.as_ref().map_or(std::ptr::null(), |c| c.as_ptr())) + }; if ctx.is_null() { - log::info!("drmtap_open_render(NULL) failed; no usable DRM render node"); + log::info!( + "drmtap_open_render({}) failed; no usable DRM render node", + node_cstr.as_ref().map_or("NULL".to_owned(), |c| format!("{c:?}")) + ); return None; } - log::info!("drm: opened unprivileged render-node convert context"); + match node_cstr { + Some(c) => log::info!( + "drm: opened unprivileged convert context on the exporting GPU ({c:?})" + ), + None => log::info!( + "drm: opened unprivileged render-node convert context (auto-selected)" + ), + } Some(RenderConverter { lib, ctx }) } diff --git a/libs/scrap/src/common/drmtap_dl.rs b/libs/scrap/src/common/drmtap_dl.rs index c4eb13147..8aa1710bc 100644 --- a/libs/scrap/src/common/drmtap_dl.rs +++ b/libs/scrap/src/common/drmtap_dl.rs @@ -137,6 +137,10 @@ type FnCursorRelease = unsafe extern "C" fn(*mut drmtap_ctx, *mut drmtap_cursor_ type FnGrabDesc = unsafe extern "C" fn(*mut drmtap_ctx, *mut drmtap_dmabuf_desc, *mut drmtap_frame_info) -> c_int; type FnOpenRender = unsafe extern "C" fn(*const c_char) -> *mut drmtap_ctx; +// libdrmtap >= 0.4.15. Names the render node of the device a context is bound to, +// so the exporter can tell the converter which GPU to bind to instead of leaving +// it to auto-selection. Returns a ctx-owned string, or NULL if it has none. +type FnRenderNode = unsafe extern "C" fn(*mut drmtap_ctx) -> *const c_char; type FnConvertDmabuf = unsafe extern "C" fn(*mut drmtap_ctx, *const drmtap_dmabuf_desc, *mut drmtap_frame_info) -> c_int; @@ -159,6 +163,9 @@ pub struct DrmtapLib { pub grab_desc: Option, pub open_render: Option, pub convert_dmabuf: Option, + // libdrmtap >= 0.4.15; `None` on an older .so, where the converter keeps + // relying on `open_render(NULL)` auto-selection exactly as before. + pub render_node: Option, // Parsed (major, minor, patch) from `drmtap_version()`, for feature gating. pub version: (c_int, c_int, c_int), } @@ -225,6 +232,8 @@ impl DrmtapLib { let open_render: Option = lib.get(b"drmtap_open_render").ok().map(|s| *s); let convert_dmabuf: Option = lib.get(b"drmtap_convert_dmabuf").ok().map(|s| *s); + let render_node: Option = + lib.get(b"drmtap_render_node").ok().map(|s| *s); Some(DrmtapLib { _lib: lib, open, @@ -237,6 +246,7 @@ impl DrmtapLib { grab_desc, open_render, convert_dmabuf, + render_node, version: (major, minor, patch), }) } diff --git a/src/ipc.rs b/src/ipc.rs index 85e2e7c2a..a97709285 100644 --- a/src/ipc.rs +++ b/src/ipc.rs @@ -545,6 +545,15 @@ pub struct DrmDisplayInfo { pub width: u32, pub height: u32, pub active: bool, + /// Render node of the GPU that EXPORTS this display's scanout, so the + /// unprivileged converter binds to that device instead of auto-selecting one. + /// On a multi-GPU host auto-selection can land on a different GPU and the + /// cross-vendor import then fails on an incompatible tiling modifier. Empty + /// when the service cannot name it (a pre-0.4.15 libdrmtap, or a display-only + /// device with no render node), which keeps the previous auto-select + /// behaviour; `serde(default)` so an older peer's message still decodes. + #[serde(default)] + pub render_node: String, } /// Serializable metadata descriptor of a scanout dma-buf, shipped over `_drm` as the JSON payload of @@ -1774,6 +1783,9 @@ static DRM_DISPLAY_GENERATION: std::sync::atomic::AtomicU64 = std::sync::atomic: /// device outputs regardless of the reader's target CRTC, so a capture reader can refresh the cache. #[cfg(all(target_os = "linux", feature = "drm"))] fn drm_displays_from_reader(reader: &mut scrap::drm_reader::DrmReader) -> Vec { + // Every display this reader enumerates belongs to the reader's device, so they + // all share its render node. Resolved once here rather than per display. + let render_node = reader.render_node().unwrap_or_default(); reader .displays() .into_iter() @@ -1796,6 +1808,7 @@ fn drm_displays_from_reader(reader: &mut scrap::drm_reader::DrmReader) -> Vec (OwnedFd, OwnedFd) { let mut fds = [0 as libc::c_int; 2]; diff --git a/src/server/drm_capturer.rs b/src/server/drm_capturer.rs index df988bdc9..bdb01fa18 100644 --- a/src/server/drm_capturer.rs +++ b/src/server/drm_capturer.rs @@ -295,11 +295,22 @@ async fn recv_thread( // Skip opening the render-node converter entirely when this display previously failed to convert // (multi-GPU render-node mismatch): request the CPU path so the service does the conversion on the // exporting GPU. Otherwise open it normally and fall back to CPU only if no render node is usable. + // Bind the converter to the GPU that EXPORTS this display's scanout, which the service + // named in the display list. Auto-selection can land on a different GPU on a multi-GPU + // host, and importing a scanout across vendors can fail on an incompatible tiling + // modifier. Empty (an older service or a device with no render node) means auto-select, + // exactly as before. Every display of one device carries the same node, so a display + // index that does not resolve still gets the right answer from the first entry. let force_cpu = drm_prefer_cpu(display); + let render_node = displays + .get(display.max(0) as usize) + .or_else(|| displays.first()) + .map(|d| d.render_node.clone()) + .unwrap_or_default(); let mut converter = if force_cpu { None } else { - RenderConverter::open_render() + RenderConverter::open_render(Some(render_node.as_str())) }; let need_cpu = converter.is_none(); if need_cpu {