From b977d9b0108defb1cfcad278025c4ded2d1f329b Mon Sep 17 00:00:00 2001 From: Mariano Abad Date: Thu, 30 Jul 2026 10:39:59 -0300 Subject: [PATCH] drm: close the round-7 review findings - the renumbering probe in the DrmDisplaysChanged handler now reads the pushed list at wire_idx, the slot our monitor held in the service's index space, instead of at the index the client chose. the pushed list shares the handshake list's construction, so probing the client index compared two different index spaces whenever a wake or hotplug had renumbered entries - tearing down a healthy stream or missing a real renumbering. - both message-body reads (cpu frame, cursor pixels) now run under a deadline. only the header read re-checked `stop`, so a producer dying between a header and its body pinned the receive thread forever and every rebuild leaked a thread plus its render context. - the drm cursor cache gets a size ceiling (drm ids are derived from the shape's content, so an animated pointer minted a new key per shape and the map grew for the life of the service; x11 ids come from a small serial set, so the ceiling is gated and the stock build is untouched). - has_non_drm_backed_display reads a two-scalar accessor instead of cloning and geometry-augmenting the whole display list on every cursor tick. - the libdrmtap pin validation moved out of import time into build_libdrmtap_so(), so leftover DRMTAP_* environment variables or a malformed sha cannot fail a stock build that never touches libdrmtap. - reworded a workflow comment whose literal expression marker broke actionlint. --- .github/workflows/drm-capture.yml | 4 +- build.py | 64 +++++++++++++++------------ src/server/display_service.rs | 19 ++++---- src/server/drm_capturer.rs | 73 +++++++++++++++++++++++++++---- src/server/input_service.rs | 15 +++++++ 5 files changed, 129 insertions(+), 46 deletions(-) diff --git a/.github/workflows/drm-capture.yml b/.github/workflows/drm-capture.yml index 737e29cd6..9f8b9059f 100644 --- a/.github/workflows/drm-capture.yml +++ b/.github/workflows/drm-capture.yml @@ -293,7 +293,9 @@ jobs: # `[[ ... ]] && cmd` as the last line makes the STEP fail once FLUTTER_VERSION moves off # the pinned value, because the failed test becomes the script's exit status. An explicit # if/else skips instead. Reading the values from the environment rather than interpolating - # ${{ }} into the script also keeps this off zizmor's template-injection list. + # github expressions into the script also keeps this off zizmor's template-injection list. + # (spelled out in prose: a literal expression marker here, even in a comment, is parsed by + # actionlint and breaks workflow linting.) if [[ "$FLUTTER_VERSION" == "3.24.5" ]]; then git apply "$GITHUB_WORKSPACE/.github/patches/flutter_3.24.4_dropdown_menu_enableFilter.diff" else diff --git a/build.py b/build.py index 7bcd28708..27c493b1c 100755 --- a/build.py +++ b/build.py @@ -354,34 +354,41 @@ LIBDRMTAP_SHA = os.environ.get('DRMTAP_SHA', LIBDRMTAP_SHA_PINNED) # DRMTAP_PREBUILT_DIR is in the list because it is the widest of the three: it skips both the fetch # and the sha verification and hands over an object built from nothing this script can see. DRMTAP_UNPINNED_OK = os.environ.get('DRMTAP_ALLOW_UNPINNED') == '1' -_overridden = [ - name - for name, value, pinned in ( - ('DRMTAP_REPO', LIBDRMTAP_REPO, LIBDRMTAP_REPO_PINNED), - ('DRMTAP_SHA', LIBDRMTAP_SHA, LIBDRMTAP_SHA_PINNED), - # `or None` so an empty value reads as unset here exactly as it does in - # build_libdrmtap_so(), which tests it for truthiness. Otherwise `DRMTAP_PREBUILT_DIR=` - # would demand the opt-in for an override that is not going to happen. - ('DRMTAP_PREBUILT_DIR', os.environ.get('DRMTAP_PREBUILT_DIR') or None, None), - ) - if value != pinned -] -if _overridden and not DRMTAP_UNPINNED_OK: - raise Exception( - f'{", ".join(_overridden)} would build libdrmtap from something other than the pinned ' - f'{LIBDRMTAP_REPO_PINNED} at {LIBDRMTAP_SHA_PINNED}. That is supported for local work and ' - 'cross-builds, but it has to be deliberate: set DRMTAP_ALLOW_UNPINNED=1 as well.') -if _overridden: - print(f'WARNING: libdrmtap is NOT the pinned build ({", ".join(_overridden)} set)') -# Both are interpolated into shell commands below, and both are env-overridable, so validate their -# SHAPE before they get there. This is not only about a hostile environment: a truncated or -# abbreviated sha would otherwise reach `git fetch` and fail with something far less obvious than -# saying so here, and an abbreviated one would defeat the point of pinning. -if not re.fullmatch(r'[0-9a-f]{40}', LIBDRMTAP_SHA): - raise Exception( - f'DRMTAP_SHA must be a full 40-character commit sha, got {LIBDRMTAP_SHA!r}') -if not re.fullmatch(r'(https://|git@)[A-Za-z0-9._~:/@-]+', LIBDRMTAP_REPO): - raise Exception(f'DRMTAP_REPO does not look like a git remote url: {LIBDRMTAP_REPO!r}') + + +def _validate_libdrmtap_pin(): + # Called from build_libdrmtap_so(), NOT at import: a stock (non --drm) build must stay + # byte-identical to upstream in behaviour too, and leftover DRMTAP_* variables in the + # environment (or a malformed sha) must not be able to fail a build that never touches + # libdrmtap. + overridden = [ + name + for name, value, pinned in ( + ('DRMTAP_REPO', LIBDRMTAP_REPO, LIBDRMTAP_REPO_PINNED), + ('DRMTAP_SHA', LIBDRMTAP_SHA, LIBDRMTAP_SHA_PINNED), + # `or None` so an empty value reads as unset here exactly as it does in + # build_libdrmtap_so(), which tests it for truthiness. Otherwise `DRMTAP_PREBUILT_DIR=` + # would demand the opt-in for an override that is not going to happen. + ('DRMTAP_PREBUILT_DIR', os.environ.get('DRMTAP_PREBUILT_DIR') or None, None), + ) + if value != pinned + ] + if overridden and not DRMTAP_UNPINNED_OK: + raise Exception( + f'{", ".join(overridden)} would build libdrmtap from something other than the pinned ' + f'{LIBDRMTAP_REPO_PINNED} at {LIBDRMTAP_SHA_PINNED}. That is supported for local work and ' + 'cross-builds, but it has to be deliberate: set DRMTAP_ALLOW_UNPINNED=1 as well.') + if overridden: + print(f'WARNING: libdrmtap is NOT the pinned build ({", ".join(overridden)} set)') + # Both are interpolated into shell commands below, and both are env-overridable, so validate + # their SHAPE before they get there. This is not only about a hostile environment: a truncated + # or abbreviated sha would otherwise reach `git fetch` and fail with something far less obvious + # than saying so here, and an abbreviated one would defeat the point of pinning. + if not re.fullmatch(r'[0-9a-f]{40}', LIBDRMTAP_SHA): + raise Exception( + f'DRMTAP_SHA must be a full 40-character commit sha, got {LIBDRMTAP_SHA!r}') + if not re.fullmatch(r'(https://|git@)[A-Za-z0-9._~:/@-]+', LIBDRMTAP_REPO): + raise Exception(f'DRMTAP_REPO does not look like a git remote url: {LIBDRMTAP_REPO!r}') def _single_real_so(paths, where): @@ -401,6 +408,7 @@ def build_libdrmtap_so(): # CAP_SYS_ADMIN) — no setcap helper, no privileged child. Only the shared # library target is built (the source also carries a helper binary we do not # ship). Returns the path to the built versioned .so (e.g. libdrmtap.so.0.4.x). + _validate_libdrmtap_pin() repo_root = os.path.dirname(os.path.abspath(__file__)) # Allow a caller (e.g. CI) to build the .so ahead of time and hand it in via # DRMTAP_PREBUILT_DIR (must contain the real libdrmtap.so.0.* object). diff --git a/src/server/display_service.rs b/src/server/display_service.rs index c2e7783da..3647d7ee6 100644 --- a/src/server/display_service.rs +++ b/src/server/display_service.rs @@ -462,16 +462,17 @@ pub(super) fn get_display_info(idx: usize) -> Option { // list shorter than the synced list means at least one advertised display is served by PipeWire. #[cfg(all(target_os = "linux", feature = "drm"))] pub fn has_non_drm_backed_display() -> bool { - match super::drm_capturer::get_display_infos() { - // A display served by PipeWire is either ABSENT from the DRM list (a shorter list, e.g. a + match super::drm_capturer::display_count_and_any_demoted() { + // A display served by PipeWire is either ABSENT from the DRM list (a shorter count, e.g. a // pure-portal display) or PRESENT-BUT-DEMOTED (kept in place at the same index and marked - // offline so the index space stays aligned -- see get_display_infos). The length check alone - // misses the demotion case (same length), so a display that is not online-DRM (`!online`) is - // treated as non-DRM-backed too. This is what gates the hidden-cursor sentinel: it stays - // authoritative only in a pure-DRM session. - Some(drm) => { - drm.len() < SYNC_DISPLAYS.lock().unwrap().displays.len() - || drm.iter().any(|d| !d.online) + // offline so the index space stays aligned -- see get_display_infos). The count check alone + // misses the demotion case (same count), so a demoted display is treated as non-DRM-backed + // too. This is what gates the hidden-cursor sentinel: it stays authoritative only in a + // pure-DRM session. The scalar accessor is deliberate: this is polled every cursor tick + // while the sentinel is active, and cloning + geometry-augmenting the whole list per tick + // (what get_display_infos does) answered the same two facts. + Some((count, any_demoted)) => { + count < SYNC_DISPLAYS.lock().unwrap().displays.len() || any_demoted } None => false, } diff --git a/src/server/drm_capturer.rs b/src/server/drm_capturer.rs index 6688881a0..d0355522f 100644 --- a/src/server/drm_capturer.rs +++ b/src/server/drm_capturer.rs @@ -51,6 +51,11 @@ const DISPLAY_LIST_TIMEOUT_MS: u64 = HANDSHAKE_TIMEOUT_MS + 4000; /// the first byte, once for the body). Derived from those parts rather than written as a constant, /// so a change to either one cannot silently invert the relationship again. const HANDSHAKE_WAIT_MS: u64 = DRM_CONNECT_TIMEOUT_MS + DISPLAY_LIST_TIMEOUT_MS * 2 + 500; +/// Deadline for a message BODY once its header has arrived (cpu frame, cursor pixels). Bodies +/// follow their header immediately on a local socket, so this is generous by orders of magnitude; +/// it exists so a producer that dies mid-message cannot pin the receive thread forever (only the +/// header read re-checks `stop`). +const BODY_READ_TIMEOUT: Duration = Duration::from_secs(5); struct FrameSlot { // (width, height, pixel format, packed pixels) of the newest frame not yet consumed by @@ -752,9 +757,16 @@ async fn recv_thread( .saturating_mul(4); // Read the body straight into a recycled frame buffer and publish that same buffer: // the pixels are copied once, by the kernel, on their way out of the socket. + // Deadlined: only the HEADER read re-checks `stop` (the 200ms poll at the loop top), + // so a producer that dies between a header and its body would otherwise pin this + // thread forever -- Drop sets `stop`, nobody observes it, and every rebuild leaks a + // thread plus its render context. The body follows its header immediately on a local + // socket (a 20MB 2880x1800 frame arrives in single-digit ms), so a whole + // BODY_READ_TIMEOUT of silence is a dead producer, not a slow one. let mut buf = shared.slot.lock().unwrap().free.take().unwrap_or_default(); - match conn.next_raw_into(&mut buf).await { - Ok(()) => { + match tokio::time::timeout(BODY_READ_TIMEOUT, conn.next_raw_into(&mut buf)).await { + Err(_) => break "cpu frame body read timed out".to_owned(), + Ok(Ok(())) => { if buf.len() < need { break format!( "cpu frame: body {} bytes < {need} for {width}x{height}", @@ -765,7 +777,7 @@ async fn recv_thread( slot.publish(width as usize, height as usize, Pixfmt::BGRA, buf); shared.cv.notify_one(); } - Err(err) => break format!("frame body: {err}"), + Ok(Err(err)) => break format!("frame body: {err}"), } // Ack this CPU frame too (flow control; see the dma-buf arm above). if let Err(err) = conn.send_frame_ack().await { @@ -788,9 +800,11 @@ async fn recv_thread( .saturating_mul(4); // A cursor is tiny and changes rarely, so this one keeps its own buffer (the frame // recycler is for scanout-sized bodies) and hands it straight to the cursor cache. + // Deadlined for the same reason as the cpu-frame body above. let mut raw = Vec::new(); - match conn.next_raw_into(&mut raw).await { - Ok(()) => { + match tokio::time::timeout(BODY_READ_TIMEOUT, conn.next_raw_into(&mut raw)).await { + Err(_) => break "cursor body read timed out".to_owned(), + Ok(Ok(())) => { if raw.len() < need { break format!( "cursor body {} bytes < {need} for {width}x{height}", @@ -810,7 +824,7 @@ async fn recv_thread( }, ); } - Err(err) => break format!("cursor body: {err}"), + Ok(Err(err)) => break format!("cursor body: {err}"), } } // Live hotplug: the service pushed a fresh display list after a connector-topology change. @@ -818,15 +832,23 @@ async fn recv_thread( // this never trips the wayland::clear() re-probe restart loop). A subsequent // get_display_infos()/get_primary_index() then reports the fresh geometry. Data::DrmDisplaysChanged(list) => { - // Did this stream's index just come to mean a different monitor? Compare against what + // Did this stream's slot just come to mean a different monitor? Compare against what // the service actually bound us to. If it moved, keeping the stream alive would send // monitor A's pixels under monitor B's advertised geometry, and route injected input // by B's rect, until something else happened to fail. End it instead: the video // service rebuilds against the fresh list, which is the same path a resolution change // already takes. Checked BEFORE the list is swapped in, so the comparison is against // the topology this stream was started on. + // + // The probe uses wire_idx, not `display`: this pushed list is in the SERVICE'S index + // space (the same fresh-enumeration construction as the handshake list), and wire_idx + // is where our monitor sat in that space when the stream was bound. `display` is a + // position in the list the CLIENT chose from, which is exactly the index space that + // can disagree with the service's whenever a wake or hotplug renumbered entries -- + // probing it here would pit slot `display` against slot wire_idx and either tear down + // a healthy stream or miss a genuine renumbering. let now_at_our_index = list - .get(display.max(0) as usize) + .get(wire_idx) .map(|d| (d.device.clone(), d.crtc_id)); if bound_to.is_some() && now_at_our_index != bound_to { swap_available_displays(list); @@ -1461,6 +1483,41 @@ pub(super) async fn refresh_displays_for_login() { } } +/// The advertised DRM display count plus whether any display is demoted to PipeWire, as two +/// scalars. `None` until probed/available. This exists for the cursor path, which polls +/// `display_service::has_non_drm_backed_display` on every tick while the hidden-cursor sentinel is +/// active (the steady state whenever the pointer is off a captured CRTC) and only ever needed these +/// two facts -- `get_display_infos` would clone the whole list and run the wayland geometry +/// augmentation per tick just to read `len()` and `online`. +/// +/// Mirrors get_display_infos' demotion semantics exactly: only a MULTI-display host advertises a +/// demoted display (on a single-display host the whole-desktop PipeWire stream IS that display, so +/// it stays online and served). +pub(super) fn display_count_and_any_demoted() -> Option<(usize, bool)> { + // Snapshot the identity keys under DRM_STATE, then consult health with DRM_STATE released -- + // same order as get_display_infos, and the same reason: never hold DRM_STATE while taking one + // of the per-display maps. + let (len, keys): (usize, Vec) = match &*DRM_STATE.lock().unwrap() { + ProbeState::Available(_, list) => ( + list.len(), + if list.len() > 1 { + list.iter().map(connector_key).collect() + } else { + Vec::new() + }, + ), + _ => return None, + }; + let any_demoted = if len > 1 { + let health = DRM_DISPLAY_HEALTH.lock().unwrap(); + keys.iter() + .any(|k| health.get(k).is_some_and(|h| h.demoted())) + } else { + false + }; + Some((len, any_demoted)) +} + /// The cached DRM displays as protobuf `DisplayInfo`, augmented with the compositor's logical layout /// (per-monitor position + scale). `None` until probed/available. pub(super) fn get_display_infos() -> Option> { diff --git a/src/server/input_service.rs b/src/server/input_service.rs index 8b0020ad2..aa6893f39 100644 --- a/src/server/input_service.rs +++ b/src/server/input_service.rs @@ -426,6 +426,21 @@ fn run_cursor(sp: MouseCursorService, state: &mut StateCursor) -> ResultType<()> let mut tmp = Message::new(); tmp.set_cursor_data(data); msg = Arc::new(tmp); + // A DRM cursor id is derived from the shape's pixels plus geometry, so an animated + // pointer mints a new id on every shape change and this map would grow for the life + // of the service, each entry pinning a compressed cursor message. (Upstream's X11 + // ids come from a small set of XFixes serials, so the map is effectively bounded + // there -- which is why the ceiling is gated and the stock build stays untouched.) + // Past the ceiling, drop the map and start over: the next request for any evicted + // shape just recompresses it, and the ceiling comfortably covers every static shape + // plus a generous animation window. + #[cfg(all(target_os = "linux", feature = "drm"))] + { + const CURSOR_CACHE_MAX: usize = 64; + if state.cached_cursor_data.len() >= CURSOR_CACHE_MAX { + state.cached_cursor_data.clear(); + } + } state.cached_cursor_data.insert(cache_key, msg.clone()); super::log::trace!("Cursor data updated, hcursor: {}", cache_key); }