From f72ad9d5c738843c9e071f91733da88ee7d3340d Mon Sep 17 00:00:00 2001 From: Mariano Abad Date: Wed, 29 Jul 2026 22:10:43 -0300 Subject: [PATCH] drm: stop routing gates from paying for the availability probe A Major finding I skipped twice, and the file already argued against itself: wayland.rs's own NOTE says re-probing _drm from the async enumeration path blocks the executor long enough to trip 'deadline has elapsed' and spiral into a restart loop -- and then six routing gates called is_available(), which runs query_displays() inline whenever the state is Unknown (cold start, or a NEGATIVE_TTL expiry mid-session). ensure_inited, is_inited, get_displays_and_primary and clear() are exactly the paths the NOTE names. is_available_cached() is a single mutex read: KNOWN-available or not. The six gates use it, which is safe because they are routing decisions, not capability ones -- a cold cache answers 'not DRM' and the caller takes the PipeWire path it would have taken anyway. Switching all seven, which is what the finding literally suggested, would have introduced a worse bug: warm_availability calls query_displays() directly, so is_available() would have had ZERO callers and nothing would ever probe lazily again. A --server that started before the root service would then never see DRM for the rest of its life. get_capturer_for_display keeps the probing form -- it is sync, on the plain video thread, it is the capture-build path where a definitive answer is the point, and it is what makes a cold cache recoverable. --- src/server/display_service.rs | 2 +- src/server/drm_capturer.rs | 23 +++++++++++++++++++++++ src/server/wayland.rs | 15 ++++++++++----- 3 files changed, 34 insertions(+), 6 deletions(-) diff --git a/src/server/display_service.rs b/src/server/display_service.rs index e8c08f3f7..c2e7783da 100644 --- a/src/server/display_service.rs +++ b/src/server/display_service.rs @@ -340,7 +340,7 @@ fn check_get_displays_changed_msg() -> Option { // from the DRM display list here. Without this the display service broadcasts an empty // list that overwrites the login peer-info displays and the client shows "No displays". #[cfg(feature = "drm")] - if super::drm_capturer::is_available() { + if super::drm_capturer::is_available_cached() { if let Some(displays) = super::drm_capturer::get_display_infos() { SYNC_DISPLAYS.lock().unwrap().check_changed(&displays); } diff --git a/src/server/drm_capturer.rs b/src/server/drm_capturer.rs index 9801bdfcc..37c3dc3b7 100644 --- a/src/server/drm_capturer.rs +++ b/src/server/drm_capturer.rs @@ -1075,10 +1075,33 @@ impl Drop for UinputRefreshGuard { } } +/// Cache-only view of the same verdict: is DRM capture KNOWN to be available right now. Never probes, +/// never blocks, never expires a stale negative -- one mutex read. +/// +/// This is the form the routing gates must use. `is_available()` below can run the blocking +/// `query_displays()` inline whenever the state is `Unknown` (a cold start, or a `NEGATIVE_TTL` +/// expiry mid-session), which is seconds of IPC. Paying that inside `wayland::clear()`, +/// `is_inited()`, the display enumeration or `get_displays_and_primary()` is exactly the stall +/// `wayland.rs`'s own NOTE says must never happen there: it blocks the executor long enough to trip +/// "deadline has elapsed" and spiral into a video-service restart loop. The comment was right and the +/// code used the probing accessor anyway. +/// +/// The distinction is safe because these are ROUTING decisions, not capability decisions: with the +/// cache cold they answer "not DRM" and the caller takes the PipeWire path it would have taken +/// anyway, and `warm_availability` seeds the cache at `--server` start so a genuine DRM host answers +/// true from the first connection. Only the capture-build path (`get_capturer_info`) still uses the +/// probing form, where paying for a definitive answer is the entire point. +pub(super) fn is_available_cached() -> bool { + matches!(&*DRM_STATE.lock().unwrap(), ProbeState::Available(..)) +} + /// Whether the root service offers DRM/KMS capture. The positive result and a definitive negative /// (connected, but no displays) are cached; a transient probe error stays `Unknown` for a few /// retries. Normally the cache is warmed at `--server` startup (`warm_availability`), so the first /// client connection hits the fast `Available` path. +/// +/// MAY BLOCK for seconds (see `is_available_cached`). Use it only where a definitive verdict is worth +/// that, never as a routing gate. pub(super) fn is_available() -> bool { // Fast path under the lock: read the cached verdict, expiring a stale negative so a host that had // no displays at probe time can still enable DRM once displays appear (without a --server diff --git a/src/server/wayland.rs b/src/server/wayland.rs index bbc9f80a7..047496ab4 100644 --- a/src/server/wayland.rs +++ b/src/server/wayland.rs @@ -178,7 +178,7 @@ pub(super) async fn ensure_inited() -> ResultType<()> { // IPC, so there is no PipeWire recorder to initialize here. But we still must set the uinput // desktop rect (check_init does this on the PipeWire path, and the DRM path skips check_init). #[cfg(feature = "drm")] - if super::drm_capturer::is_available() { + if super::drm_capturer::is_available_cached() { update_uinput_resolution().await; return Ok(()); } @@ -190,7 +190,7 @@ pub(super) fn is_inited() -> Option { None } else { #[cfg(feature = "drm")] - if super::drm_capturer::is_available() { + if super::drm_capturer::is_available_cached() { return None; } if CAP_DISPLAY_INFO.read().unwrap().is_empty() { @@ -320,7 +320,7 @@ pub(super) async fn check_init() -> ResultType<()> { pub(super) async fn get_displays_and_primary() -> ResultType<(Vec, usize)> { #[cfg(feature = "drm")] - if super::drm_capturer::is_available() { + if super::drm_capturer::is_available_cached() { if let Some(displays) = super::drm_capturer::get_display_infos() { // DRM connector order is not the compositor's primary; resolve the real primary from // the compositor layout (matched by normalized connector name), not a hardcoded index 0. @@ -351,7 +351,7 @@ pub fn clear() { // against STALE geometry after a monitor hotplug/rotation/scale change. Invalidate it on teardown // so the next session re-reads fresh geometry (lazily, on the next enumeration) and self-heals. #[cfg(feature = "drm")] - if super::drm_capturer::is_available() { + if super::drm_capturer::is_available_cached() { scrap::wayland::display::clear_wayland_displays_cache(); } // NOTE: intentionally do NOT reset the DRM probe cache here. `clear()` runs on every capturer @@ -397,6 +397,11 @@ pub(super) fn get_capturer_for_display( // render-node-absent seat or a convert failure on the unprivileged side) must NOT propagate out // and restart-loop this per-display video service. Instead fall THROUGH to PipeWire for just this // display; the other DRM outputs keep streaming over DRM. + // The ONE gate that keeps the probing form on purpose: this runs on the plain video thread, + // not an async executor, and it is the capture-build path, so a definitive verdict is worth + // seconds here. It is also what makes a cold cache recoverable at all -- warm_availability + // gives up after its attempts, so if EVERY gate were cache-only a --server that started + // before the root service would never see DRM again for the rest of its life. #[cfg(feature = "drm")] if super::drm_capturer::is_available() { match super::drm_capturer::get_capturer_info(display_idx) { @@ -436,7 +441,7 @@ pub(super) fn get_capturer_for_display( // that display) and is served normally. On a pure-PipeWire host is_available() is false and // this guard is skipped, preserving upstream behavior exactly. #[cfg(feature = "drm")] - if super::drm_capturer::is_available() { + if super::drm_capturer::is_available_cached() { if let Some(advertised) = super::drm_capturer::get_display_infos() .and_then(|l| l.get(display_idx).cloned()) {