From a1ff0dde0cfd3e15b9c1b6fda225e4856cfd4bad Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Tue, 11 Aug 2026 08:48:49 +0200 Subject: [PATCH] fix(pf-vdisplay): the host promised HDR and cursor forwarding for gamescope sessions it did not start MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `gamescope_ours_and` answered "did WE spawn this gamescope?" by reading `PUNKTFUNK_GAMESCOPE_NODE`. Phase 2.3 deleted the code that published that key — routing.rs's own doc says "Nothing is written back to the two knobs" — but this consumer was never migrated, so the read now returns "not attaching" for every attach. Both consumers then answer for a session this host has no flags on. On a plain box with a foreign gamescope already running, `pick_gamescope_mode` resolves Attach at its fifth rung while the env key stays unset, and the probe half only inspects the resolved BINARY, which is our patched build: * `gamescope_composites_cursor()` returns true, so the host attaches no XFixes reader and blends nothing — while the stock gamescope actually running was never given `--pipewire-composite-cursor`, so the stream carries no pointer at all. * `gamescope_hdr_available()` returns true, so the Welcome fixes `bit_depth` at 10 and the session negotiates BT.2020/PQ over an 8-bit SDR composite. The Welcome cannot take that back. The same two failures hit the `capture_monitor` mirror route on any Bazzite or SteamOS box, where the running Game Mode gamescope is by definition not one this host spawned. The question is now asked of the resolved route rather than the environment, via a pure `session_is_a_foreign_gamescope` that runs — and is tested — on every platform. The residual gap is named in the doc rather than papered over: `create_managed_session`'s create-time degrade to a foreign attach is still invisible to a ladder re-run. Also in this commit: * Two unguarded session-env reads now take `ENV_LOCK` (`detect()`'s `XDG_CURRENT_DESKTOP` fallback and `effective_topology()`'s legacy pins). `apply_session_env` `set_var`s those same keys from another thread, which is the glibc setenv/getenv race this crate's own lib.rs documents as UB. * `mirror.rs`'s `names_ours_conclusively` was a `matches!` whose omitted default was the UNSAFE direction — a new backend would silently get its own virtual displays mirrored. Now exhaustive, so adding a `Compositor` is a compile error at the one site where the answer is a safety decision. * `MirrorDisplay` overrides `poolable_now() -> false`; its `create` always reports `External`, so the trait's `true` default was a pre-create claim contradicting the post-create fact. The trait doc now says plainly that the default is a default and not a fact. * The crate front-door doc listed 3 of 7 backends and quoted line counts half the size of the current crate; `routing.rs`'s summary was attached to the wrong item and described a published env channel that no longer exists; `available()` is no longer documented as cheap when it forks `gamescope --version` and does an unbudgeted Wayland roundtrip per call. --- crates/pf-vdisplay/src/lib.rs | 182 ++++++++++++++++++--- crates/pf-vdisplay/src/vdisplay/backend.rs | 14 +- crates/pf-vdisplay/src/vdisplay/mirror.rs | 66 +++++++- crates/pf-vdisplay/src/vdisplay/routing.rs | 46 +++--- 4 files changed, 257 insertions(+), 51 deletions(-) diff --git a/crates/pf-vdisplay/src/lib.rs b/crates/pf-vdisplay/src/lib.rs index a4b1fbab..3107bbdc 100644 --- a/crates/pf-vdisplay/src/lib.rs +++ b/crates/pf-vdisplay/src/lib.rs @@ -8,17 +8,36 @@ //! * **KWin** — privileged `zkde_screencast_unstable_v1::stream_virtual_output` ([`kwin`]). //! * **wlroots/Sway** — `swaymsg create_output` + `output mode --custom` ([`wlroots`]). //! * **Mutter/GNOME** — D-Bus `RemoteDesktop` + `ScreenCast.RecordVirtual` ([`mutter`]). +//! * **Hyprland** — `hyprctl output create headless` + the xdg-desktop-portal-hyprland ScreenCast +//! portal. Its own backend, not a wlroots dialect (`design/hyprland-support.md` D1). +//! * **gamescope** — three sub-modes behind one backend ([`GamescopeRoute`]): bare +//! **spawn** of a nested headless session, host-**managed** `gamescope-session-plus`/SteamOS +//! takeover, and **attach** to a session somebody else started. By far the largest backend here, +//! because it owns session lifecycle rather than just minting an output. +//! * **monitor mirror** — no virtual display at all: stream a PHYSICAL head the compositor already +//! has (the `PUNKTFUNK_CAPTURE_MONITOR` pin), reporting [`DisplayOwnership::External`] so none of +//! the lifecycle policy is applied to someone else's screen. +//! * **Windows pf-vdisplay** — the all-Rust IddCx driver + its `manager`, the sole Windows backend. +//! +//! No list of file sizes here: it rots. The rule instead — the Linux backends plus the Windows +//! manager are the bulk of this crate, and the platform-neutral half (`policy`, `registry`, +//! `lifecycle`, `layout`, `identity`, `admission`, `monitors`, `session`, `routing`, `proc`, +//! `portal_config`) is the minority that every platform's CI actually compiles and tests. //! //! [`VirtualDisplay::create`] returns a [`VirtualOutput`]: the PipeWire node to capture plus an //! owned keepalive whose `Drop` releases the output (RAII — no explicit `destroy`). Capture //! consumes the node via the host `capture::capture_virtual_output`. -// `dead_code` is ENFORCED on Linux, where ~10k of this crate's ~17k lines live. Off elsewhere for -// one structural reason: `proc`, `session`, `routing`, `monitors` and `lifecycle` are declared -// unconditionally but exist to serve the Linux backends, so on Windows/macOS most of their surface -// is legitimately unreferenced. Scoping it this way rather than crate-wide keeps the platform that -// owns the code honest. (Was a bare crate-wide allow whose "scaffold, defined ahead of the target -// that uses them" rationale had stopped being true.) +// `dead_code` is ENFORCED on Linux, where the clear majority of this crate lives — every compositor +// backend under `vdisplay/linux/` plus everything only they consume, which is roughly half the crate +// on its own and the half that carries the session-lifecycle risk. Off elsewhere for one structural +// reason: `proc`, `session`, `routing`, `monitors` and `lifecycle` are declared unconditionally but +// exist to serve the Linux backends, so on Windows/macOS most of their surface is legitimately +// unreferenced. Note what that waives: the Windows backend (`vdisplay/windows/`, itself thousands of +// lines) gets NO dead-code enforcement, so an orphaned Windows path has to be found by review. +// Scoping it this way rather than crate-wide still keeps the platform that owns most of the code +// honest. (Was a bare crate-wide allow whose "scaffold, defined ahead of the target that uses them" +// rationale had stopped being true.) #![cfg_attr(not(target_os = "linux"), allow(dead_code))] // Every `unsafe` block in this file carries a `// SAFETY:` proof; enforce it (unsafe-proof program). #![deny(clippy::undocumented_unsafe_blocks)] @@ -200,9 +219,16 @@ impl Compositor { /// The compositor backends usable on this host *right now*: gamescope wherever its binary is /// installed (it spawns a nested session — independent of the running desktop), plus the live /// session's own compositor (KWin / Mutter / wlroots / Hyprland) when the host runs inside it. -/// Cheap, side-effect-free probes — safe to call per management request. A concrete client -/// preference is validated against this set before it's honored (see the punktfunk/1 handshake's -/// resolution). +/// Side-effect-free, but **not cheap, and not memoized**: every call re-walks `/proc` +/// ([`detect_active_session`]), and each backend probe that the live/pinned short-circuit below does +/// not exempt does real work — `gamescope::is_available` FORKS `gamescope --version`, +/// `kwin::is_available` does a Wayland registry roundtrip, `wlroots`/`hyprland` read a socket path +/// and `mutter` a D-Bus name. So a console polling `/host/compositors` on a KDE box still forks a +/// gamescope per poll, on a thread the caller must therefore not assume is cheap to block (mgmt +/// calls it inline on the async runtime). Callers wanting a hot path should cache the answer; +/// treating this as free is what the "cheap, safe per management request" claim this doc used to +/// make invited. A concrete client preference is validated against this set before it's honored +/// (see the punktfunk/1 handshake's resolution). /// /// The **live session is the primary signal**, ahead of each backend's own probe. Those probes read /// the process env (`XDG_CURRENT_DESKTOP` for Mutter, `WAYLAND_DISPLAY` for KWin's registry @@ -311,7 +337,12 @@ pub fn detect() -> Result { if let Some(c) = compositor_for_kind(detect_active_session().kind) { return Ok(c); } - let desktop = std::env::var("XDG_CURRENT_DESKTOP") + // Under [`ENV_LOCK`]: `apply_session_env` `set_var`s — and, for a dead session, + // `remove_var`s — this very key from another session's `spawn_blocking`, and a glibc + // `getenv` concurrent with a `setenv` is the `environ` realloc data race ENV_LOCK exists + // for (it is UB regardless of which key each side touches, so "different variable" is no + // defence). Read-then-drop: only the read needs serializing. + let desktop = with_env_lock(|| std::env::var("XDG_CURRENT_DESKTOP")) .unwrap_or_default() .to_ascii_uppercase(); if desktop.contains("KDE") { @@ -559,13 +590,18 @@ pub fn effective_topology() -> policy::Topology { return resolve_topology(e.topology); } // Unconfigured: honor a legacy operator env if present (a host runs one desktop backend, so at - // most one of these is set), else the Auto default. - let legacy = [ - "PUNKTFUNK_KWIN_VIRTUAL_PRIMARY", - "PUNKTFUNK_MUTTER_VIRTUAL_PRIMARY", - ] - .iter() - .find_map(|k| std::env::var(k).ok()); + // most one of these is set), else the Auto default. Read under [`ENV_LOCK`] like every other + // env read on the session-setup path: this runs inside `create`, concurrent with another + // session's `apply_session_env` `set_var`s, and glibc's `environ` realloc makes a racing + // `getenv` UB no matter that these particular keys are ones nobody writes. + let legacy = with_env_lock(|| { + [ + "PUNKTFUNK_KWIN_VIRTUAL_PRIMARY", + "PUNKTFUNK_MUTTER_VIRTUAL_PRIMARY", + ] + .iter() + .find_map(|k| std::env::var(k).ok()) + }); match legacy.as_deref().map(str::trim) { Some("1" | "true" | "yes" | "on") => policy::Topology::Exclusive, Some("0" | "false" | "no" | "off") => policy::Topology::Extend, @@ -637,19 +673,79 @@ pub fn gamescope_composites_cursor() -> bool { /// /// A host-managed `gamescope-session-plus` / SteamOS session counts as a spawn: we own its /// `GAMESCOPE_BIN` wrapper (or PATH shim), so the flags are ours. +/// +/// **Ask the resolved ROUTE, never the env.** This used to test the spawn-vs-attach term by reading +/// `PUNKTFUNK_GAMESCOPE_NODE`, which worked only while `apply_input_env` PUBLISHED its decision into +/// that key. Phase 2.3 deleted the publication (routing.rs: "Nothing is written back to the two +/// knobs") and left the key as an operator override — rung 2 of a 6-rung ladder — so the session +/// that reaches [`GamescopeRoute::Attach`] at the ladder's rung 5 instead (a foreign gamescope on an +/// infra-less box), and the monitor-pin mirror that never consults the ladder at all, both answered +/// "ours". The two consequences were silent and unrecoverable: the punktfunk/1 Welcome fixed the +/// session at 10-bit BT.2020/PQ against a foreign 8-bit SDR composite, and the host skipped the +/// XFixes cursor reconstruction for a session whose gamescope was never given +/// `--pipewire-composite-cursor` — a stream with no pointer in it at all. +/// +/// **Two residual gaps**, both of which need a route this crate cannot see from here: +/// +/// * the ladder is re-run with `dedicated_launch = false`, since a capability query carries no +/// session context — so it cannot see the one input that would move a session from +/// Managed/Attach to Spawn; +/// * `create_managed_session` can degrade a resolved `Managed` to an ATTACH at create time (a +/// mask-fragile DM it may not stop — it then mirrors the box's own game-mode session). That +/// happens after this answer is due, and the ladder re-run here still says `Managed`, so such a +/// session is still credited with flags it does not own. +/// +/// The first fails closed; the second does not. Both close the same way: give these two functions +/// the session's own [`GamescopeRoute`] (which `SessionContext` already carries) and have the +/// backend report the degrade — a change to two public signatures and every host call site. fn gamescope_ours_and(#[cfg(target_os = "linux")] probe: fn() -> bool) -> bool { #[cfg(target_os = "linux")] { - let attaching = with_env_lock(|| std::env::var_os("PUNKTFUNK_GAMESCOPE_NODE").is_some()); - !attaching && probe() + // `probe` first: it is memoized (the `--version` banner is parsed once per process), while + // the route resolution walks `/proc` for a foreign gamescope. On a box with a stock + // gamescope the answer is already `false` and the walk never happens. + probe() + && !session_is_a_foreign_gamescope( + capture_monitor().is_some(), + resolve_gamescope_route(Compositor::Gamescope, false).as_ref(), + ) } #[cfg(not(target_os = "linux"))] false } -// Platform-neutral per-client stable display-id map (Stage 3): Windows seeds the monitor EDID + -// ConnectorIndex from the id; KWin names its output from it. `allow(dead_code)` because only Windows -// consumes it in non-test code today — the KWin wiring is the next Stage-3 step. +/// Pure predicate behind [`gamescope_ours_and`]: is the gamescope this session will use one +/// SOMEBODY ELSE started, whose spawn flags we therefore cannot vouch for? +/// +/// Two ways to land on a foreign session, and both must count: +/// +/// * `mirror_pinned` — a `PUNKTFUNK_CAPTURE_MONITOR` pin routes [`open`] to the mirror backend, +/// whose gamescope arm attaches to the node the RUNNING session already publishes without +/// consulting the sub-mode ladder at all. On a Bazzite/SteamOS box that session is Game Mode's, +/// i.e. by definition not ours. +/// * a [`GamescopeRoute::Attach`] verdict — however the ladder reached it (operator override, +/// or the foreign-gamescope rung). +/// +/// [`GamescopeRoute::Managed`] is NOT foreign: the managed takeover starts the session through our +/// own `GAMESCOPE_BIN` wrapper / PATH shim, so its flags are the ones we chose. +/// +/// `mirror_pinned` is judged from the pin alone, not from whether the mirror actually took: [`open`] +/// degrades a pin to the virtual-display path when the session reports no physical heads, and a +/// pinned box that lands there is called foreign here although it will bare-spawn. That is the +/// fail-closed direction — a capability withheld from a session that could have had it — and the +/// alternative (enumerating heads from a capability query) would put a compositor roundtrip on a +/// path that must answer before anything exists to ask. +fn session_is_a_foreign_gamescope(mirror_pinned: bool, route: Option<&GamescopeRoute>) -> bool { + mirror_pinned || matches!(route, Some(GamescopeRoute::Attach { .. })) +} + +// Platform-neutral per-client stable display-id map: Windows seeds the monitor EDID serial + +// IddCx ConnectorIndex from the id; KWin names its output `Virtual-punktfunk-` (kwin.rs's +// `resolve_slot` call); Mutter cannot carry the id into its virtual monitor at all, so it keys the +// host-persisted `ScaleMap` on the same identity key. All three are production call sites, so the +// `allow(dead_code)` below no longer stands for "unwired yet" (it did when only Windows consumed the +// map); it now covers whatever helpers no CURRENT backend reaches. Worth re-testing without it — +// that has to happen on a Linux build, since this is the platform where dead_code is enforced. #[allow(dead_code)] #[path = "vdisplay/identity.rs"] pub(crate) mod identity; @@ -735,6 +831,48 @@ mod tests { assert_eq!(compositor_for_kind(ActiveKind::None), None); } + /// The spawn-vs-attach term behind [`gamescope_hdr_available`] / + /// [`gamescope_composites_cursor`]. Both answers are IRREVOCABLE once the punktfunk/1 Welcome + /// has gone out (bit depth is fixed there; the session plan's cursor decision feeds the encoder + /// open), so an over-promise here is not recoverable at runtime — which is why the regression + /// this pins mattered: the term used to be read off `PUNKTFUNK_GAMESCOPE_NODE`, a key nothing + /// writes any more, so every foreign session answered "ours". + #[test] + fn only_a_session_we_start_can_promise_gamescope_capabilities() { + // Attach — however the ladder got there — is somebody else's session: unknown spawn flags. + assert!(session_is_a_foreign_gamescope( + false, + Some(&GamescopeRoute::Attach { + node: "auto".into() + }) + )); + assert!(session_is_a_foreign_gamescope( + false, + Some(&GamescopeRoute::Attach { node: "42".into() }) + )); + // A bare spawn is ours by definition; so is the managed takeover (it starts gamescope + // through our own GAMESCOPE_BIN wrapper / PATH shim, so the flags are the ones we chose). + assert!(!session_is_a_foreign_gamescope( + false, + Some(&GamescopeRoute::Spawn) + )); + assert!(!session_is_a_foreign_gamescope( + false, + Some(&GamescopeRoute::Managed { + client: "steam".into() + }) + )); + // No route at all = not a gamescope session; the binary probe alone then decides. + assert!(!session_is_a_foreign_gamescope(false, None)); + // A monitor pin bypasses the ladder entirely (mirror backend → attach to the node the + // RUNNING session publishes), so it is foreign whatever the ladder would have said. + assert!(session_is_a_foreign_gamescope(true, None)); + assert!(session_is_a_foreign_gamescope( + true, + Some(&GamescopeRoute::Spawn) + )); + } + #[test] fn detect_active_session_is_side_effect_free_and_terminates() { // A pure probe of /proc + the runtime dir: it must not panic and must return promptly on diff --git a/crates/pf-vdisplay/src/vdisplay/backend.rs b/crates/pf-vdisplay/src/vdisplay/backend.rs index 6b2e663d..33dc6444 100644 --- a/crates/pf-vdisplay/src/vdisplay/backend.rs +++ b/crates/pf-vdisplay/src/vdisplay/backend.rs @@ -225,9 +225,17 @@ pub trait VirtualDisplay: Send { /// ([`DisplayOwnership::Owned`], keep-alive-able) display? The registry consults this **before** /// its keep-alive reuse lookup, so it never hands a kept display of one flavor to a request of /// another — specifically a gamescope managed/attach acquire must not reuse a kept **bare-spawn** - /// (they share the backend name `"gamescope"`). Default `true`; only gamescope overrides it, - /// returning `false` when the env selects attach/managed (consistent with the `ownership` its - /// `create` will report). See `design/gamemode-and-dedicated-sessions.md` A1. + /// (they share the backend name `"gamescope"`). Overridden by gamescope (`false` unless the + /// resolved [`GamescopeRoute`](crate::GamescopeRoute) carried on the instance is `Spawn` — it + /// reads `self.route`, NOT env; the sub-mode stopped travelling through + /// `PUNKTFUNK_GAMESCOPE_NODE`/`_SESSION` in Phase 2.3) and by the mirror backend (`false` + /// always). See `design/gamemode-and-dedicated-sessions.md` A1. + /// + /// The default `true` is a DEFAULT, not a fact: it happens to be right for every backend that + /// creates a display it owns, and it is wrong for any backend whose `create` reports something + /// other than [`DisplayOwnership::Owned`] — this answer and that one must agree, and nothing + /// enforces it. A required method would; making it one costs an impl in each of the five + /// per-compositor backends plus Windows. fn poolable_now(&self) -> bool { true } diff --git a/crates/pf-vdisplay/src/vdisplay/mirror.rs b/crates/pf-vdisplay/src/vdisplay/mirror.rs index b054439f..0e524f5f 100644 --- a/crates/pf-vdisplay/src/vdisplay/mirror.rs +++ b/crates/pf-vdisplay/src/vdisplay/mirror.rs @@ -65,6 +65,16 @@ impl VirtualDisplay for MirrorDisplay { self.hw_cursor } + fn poolable_now(&self) -> bool { + // Never. `create` below always reports `DisplayOwnership::External` — we did not make this + // head and must not keep it — so the registry never pools a mirror, and the trait's `true` + // default was a claim this backend cannot honour on any request. It costs nothing today + // (the reuse lookup can only miss: no `"mirror"` entry ever enters the pool), but it is the + // answer the registry consults BEFORE `create` gets to declare ownership, so leaving it + // optimistic means the one pre-create statement of intent contradicts the post-create fact. + false + } + fn create(&mut self, _mode: Mode) -> Result { // Resolve the pin against the live head list FIRST: it yields the geometry the input anchor // needs, and it turns "that monitor is gone" into one clear error before any compositor @@ -101,7 +111,14 @@ impl VirtualDisplay for MirrorDisplay { Compositor::Gamescope => { crate::gamescope::stream_existing_output(&target.connector, self.hw_cursor)? } - #[allow(unreachable_patterns)] + // Gated to non-Linux (`monitors::list`'s shape), NOT the bare `#[allow(unreachable_ + // patterns)] other =>` this replaced: with it, a newly added `Compositor` variant fell + // through to a runtime bail on the very platform that would define it, silently, in the + // one place that decides which backends can mirror a head. Cfg'd out on Linux, the match + // is exhaustive and the new variant is a compile error here instead. The arm exists at + // all only because every arm above is itself `cfg(target_os = "linux")` — this module is + // Linux-only today, so it is a placeholder that keeps the shape honest if that changes. + #[cfg(not(target_os = "linux"))] other => bail!( "mirroring an existing monitor is not supported on the {} backend", other.id() @@ -172,11 +189,28 @@ fn check_mirrorable(target: &monitors::PhysicalMonitor, compositor: Compositor) Ok(()) } -/// Does this compositor's `managed` flag mean "ours, for certain"? KWin outputs carry the -/// `Virtual-punktfunk` prefix we chose, and Hyprland's are `PF-N` — both ours by construction. -/// Sway's `HEADLESS-N` is sway's own generic naming, so it is a hint, not proof. +/// Does this compositor's `managed` flag mean "ours, for certain"? +/// +/// EXHAUSTIVE on purpose, unlike the `matches!` it used to be. This is the one table in the crate +/// whose un-listed default is the UNSAFE direction: a `false` sends [`check_mirrorable`] down the +/// warn-and-proceed branch, which for a backend that DOES name its managed outputs by construction +/// (the KWin/Hyprland shape — i.e. both backends that have the property today) means streaming +/// punktfunk's own virtual display back to the client, the capture loop +/// `one_of_our_own_virtual_displays_is_refused` exists to forbid. Adding a `Compositor` variant must +/// therefore be a compile error here rather than a silent opt-out. (Contrast +/// [`Compositor::needs_live_session`], also a `matches!` — its omitted default is the safe one.) fn names_ours_conclusively(compositor: Compositor) -> bool { - matches!(compositor, Compositor::Kwin | Compositor::Hyprland) + match compositor { + // Ours by construction: KWin outputs carry the `Virtual-punktfunk-` name the identity + // module hands the backend, Hyprland's are `PF-N`. Nothing else mints those names. + Compositor::Kwin | Compositor::Hyprland => true, + // Sway names EVERY headless output `HEADLESS-N`, its own included; Mutter's virtual monitors + // carry no distinguishing name at all (it won't take one from us); and gamescope's + // `list_monitors` only ever reports the real DRM head a Game Mode session drives, so + // `managed` is never even set there. A hint at most — refusing would break the legitimate + // headless-sway setup this feature serves. + Compositor::Wlroots | Compositor::Mutter | Compositor::Gamescope => false, + } } /// mHz → whole Hz for [`VirtualOutput::preferred_mode`], never 0 (the negotiation treats 0 as @@ -252,6 +286,28 @@ mod tests { assert!(check_mirrorable(&m, Compositor::Hyprland).is_err()); } + /// Pin the conclusive-naming table per variant. The answer is a safety decision whose wrong + /// direction is the SILENT one: a backend that mints punktfunk-named outputs but is missing + /// from the `true` arm takes the warn-and-proceed branch and streams our own virtual display + /// back to the client. Exhaustive `match` + this test = the new variant has to be considered. + #[test] + fn the_conclusive_naming_table_is_pinned_per_backend() { + assert!(names_ours_conclusively(Compositor::Kwin)); + assert!(names_ours_conclusively(Compositor::Hyprland)); + assert!(!names_ours_conclusively(Compositor::Wlroots)); + assert!(!names_ours_conclusively(Compositor::Mutter)); + assert!(!names_ours_conclusively(Compositor::Gamescope)); + } + + /// The registry asks `poolable_now` BEFORE `create` gets to report ownership, so the two must + /// agree: a mirror's `create` always reports `External` (we did not make this head), therefore + /// no mirror request is ever poolable. + #[test] + fn a_mirrored_head_is_never_registry_poolable() { + let vd = MirrorDisplay::new(Compositor::Kwin, "DP-2".into()).unwrap(); + assert!(!vd.poolable_now()); + } + /// A head listed but not driving a mode (enabled yet modeless) would negotiate a 0x0 stream. #[test] fn a_head_with_no_current_mode_is_refused() { diff --git a/crates/pf-vdisplay/src/vdisplay/routing.rs b/crates/pf-vdisplay/src/vdisplay/routing.rs index 5d3494b1..f31ed4e6 100644 --- a/crates/pf-vdisplay/src/vdisplay/routing.rs +++ b/crates/pf-vdisplay/src/vdisplay/routing.rs @@ -77,29 +77,22 @@ fn pick_gamescope_mode( } } -/// Route input to match the chosen video backend (they must not diverge), via the highest-priority -/// `PUNKTFUNK_INPUT_BACKEND` knob the injector honors. For gamescope the sub-mode ladder -/// ([`pick_gamescope_mode`]) selects **managed** (a host-managed session at the client's mode — -/// tears the TV's autologin down on connect, restored on a debounced idle; only where -/// session-plus/SteamOS actually exists), **attach** (mirror a running gamescope at its own mode; -/// explicit via `PUNKTFUNK_GAMESCOPE_ATTACH`/`PUNKTFUNK_GAMESCOPE_NODE`, or the fallback for a -/// foreign gamescope on an infra-less box), or **bare spawn** (a per-session headless gamescope -/// nesting the session's launch command — the plain-distro default). `PUNKTFUNK_GAMESCOPE_MANAGED` -/// forces managed over all of it. -/// The operator's gamescope overrides, sampled ONCE — before this module has written anything. +/// The operator's gamescope overrides, sampled ONCE — at first use, and never written back. /// -/// [`apply_input_env`] both WRITES `PUNKTFUNK_GAMESCOPE_NODE`/`_SESSION` (to publish the sub-mode it -/// chose) and READS them as operator overrides. Reading them live therefore fed the ladder its own -/// previous output: the Attach arm sets `_NODE=auto`, and `node_env` sits at rung 2 of +/// `apply_input_env` used to both WRITE `PUNKTFUNK_GAMESCOPE_NODE`/`_SESSION` (to publish the +/// sub-mode it chose) and READ them as operator overrides. Reading them live therefore fed the +/// ladder its own previous output: the Attach arm set `_NODE=auto`, and `node_env` sits at rung 2 of /// [`pick_gamescope_mode`] — ABOVE `dedicated_launch` at rung 3 — so one Attach decision latched /// Attach for the rest of the host's life and silently overrode `game_session=dedicated`. Only rung /// 1 (`_MANAGED`) could escape, because the Spawn arm that would clear the keys sits below the rung /// that by then always fired. /// /// Sampling at first use keeps the override's actual meaning — "the operator set this before we -/// ran" — and makes it immune to our own writes. The live reads that remain -/// ([`launch_is_nested`], gamescope's `poolable_now`) are deliberate: those consume the PUBLISHED -/// decision, which is what the keys carry after this function has run. +/// ran". Nothing publishes these keys any more (see [`resolve_gamescope_route`]): the resolved +/// decision travels as a [`GamescopeRoute`] VALUE carried on the backend instance, and every +/// consumer takes it that way — [`launch_is_nested`] by parameter, gamescope's `poolable_now` off +/// `self.route`, `crate::gamescope_hdr_available` by re-resolving the ladder. A change that +/// "restores" the write to serve some reader would restore the latch with it. #[cfg(target_os = "linux")] static OPERATOR_GAMESCOPE: std::sync::OnceLock = std::sync::OnceLock::new(); @@ -138,6 +131,16 @@ fn operator_gamescope() -> &'static OperatorGamescope { }) } +/// Route input to match the chosen video backend (they must not diverge), via the highest-priority +/// `PUNKTFUNK_INPUT_BACKEND` knob the injector honors. +/// +/// For gamescope the sub-mode ladder ([`pick_gamescope_mode`]) selects **managed** (a host-managed +/// session at the client's mode — tears the TV's autologin down on connect, restored on a debounced +/// idle; only where session-plus/SteamOS actually exists), **attach** (mirror a running gamescope at +/// its own mode; explicit via `PUNKTFUNK_GAMESCOPE_ATTACH`/`PUNKTFUNK_GAMESCOPE_NODE`, or the +/// fallback for a foreign gamescope on an infra-less box), or **bare spawn** (a per-session headless +/// gamescope nesting the session's launch command — the plain-distro default). +/// `PUNKTFUNK_GAMESCOPE_MANAGED` forces managed over all of it. /// /// Returns the resolved [`GamescopeRoute`] when `chosen` is gamescope — the caller must carry it to /// the backend instance via `VirtualDisplay::set_gamescope_route`. It is a RETURN VALUE and no @@ -449,11 +452,12 @@ mod tests { assert_eq!(pick(true, false, false, true, false, false, false), Attach); } - /// The ladder must not be able to read back its own output. `apply_input_env`'s Attach arm - /// writes `PUNKTFUNK_GAMESCOPE_NODE=auto`, and `node_env` outranks `dedicated_launch` — so when - /// the override was read live, one Attach latched Attach for the host's lifetime and silently - /// overrode `game_session=dedicated`. Sampling once is what breaks the loop; this pins that the - /// sample does not move when the key is written afterwards. + /// The ladder must not be able to read back its own output. `apply_input_env`'s Attach arm used + /// to write `PUNKTFUNK_GAMESCOPE_NODE=auto`, and `node_env` outranks `dedicated_launch` — so + /// while the override was read live, one Attach latched Attach for the host's lifetime and + /// silently overrode `game_session=dedicated`. Sampling once is what breaks the loop, and it is + /// what makes restoring the write a non-event rather than a relapse; this pins that the sample + /// does not move when the key is written afterwards. #[test] #[cfg(target_os = "linux")] fn operator_overrides_do_not_see_our_own_writes() {