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); }