diff --git a/crates/pf-vdisplay/src/vdisplay/windows/manager.rs b/crates/pf-vdisplay/src/vdisplay/windows/manager.rs index 232c9ff9..3a149719 100644 --- a/crates/pf-vdisplay/src/vdisplay/windows/manager.rs +++ b/crates/pf-vdisplay/src/vdisplay/windows/manager.rs @@ -221,6 +221,42 @@ fn poll_gdi_name(target_id: u32) -> Option { None } +/// Test-only fault injection for the CCD isolate. +/// +/// Every Phase-3 recovery leg in this file fires only when [`isolate_displays_ccd`] returns `None`, +/// and on healthy hardware it never does — which is exactly why those legs shipped unexercised on +/// glass. This counter lets a live test fail the next N isolates against the REAL driver and a REAL +/// panel, so the recovery is observed rather than reasoned about. +/// +/// `#[cfg(test)]`-only on purpose: this crate's live hardware tests compile under `cfg(test)`, so +/// the seam is reachable where it is needed WITHOUT shipping a production knob that could leave +/// display isolation silently disabled on an operator's box. +#[cfg(test)] +pub(crate) static FAIL_NEXT_ISOLATES: std::sync::atomic::AtomicU32 = + std::sync::atomic::AtomicU32::new(0); + +/// [`isolate_displays_ccd`] with the test seam in front of it. Every call site in this file goes +/// through here so an injected failure exercises the same gates a real one would. +fn isolate_displays_ccd_seam(keep_target_ids: &[u32]) -> Option { + #[cfg(test)] + { + use std::sync::atomic::Ordering; + if FAIL_NEXT_ISOLATES + .fetch_update(Ordering::Relaxed, Ordering::Relaxed, |n| { + (n > 0).then(|| n - 1) + }) + .is_ok() + { + tracing::warn!( + keep = ?keep_target_ids, + "TEST fault injection: forcing isolate_displays_ccd -> None" + ); + return None; + } + } + isolate_displays_ccd(keep_target_ids) +} + fn shrink_action(ccd_exclusive: bool, has_saved: bool) -> ShrinkAction { if ccd_exclusive { ShrinkAction::Reisolate @@ -975,7 +1011,7 @@ impl VirtualDisplayManager { "re-asserting exclusive topology" ), } - let _ = isolate_displays_ccd(&keep); + let _ = isolate_displays_ccd_seam(&keep); // That same forced re-commit hands the live IDD path a fresh swap-chain, // orphaning the session's capture ring — announce it so the session rebuilds // its capture attachment (same-mode ring recreate + driver re-attach + fresh @@ -1198,7 +1234,7 @@ impl VirtualDisplayManager { if crate::policy::prefs().ddc_power_off() { inner.group.ddc_panels_off = crate::ddc::panel_off_except(n); } - inner.group.ccd_saved = isolate_displays_ccd(&keep); + inner.group.ccd_saved = isolate_displays_ccd_seam(&keep); // EXPERIMENTAL `pnp_disable_monitors` policy axis: AFTER the isolate took, // additionally disable the deactivated monitors' PnP devnodes (persistent // across hot-plug re-arrival) so a standby monitor/TV's periodic wake @@ -1223,7 +1259,7 @@ impl VirtualDisplayManager { // Grown set: re-isolate so the fresh member joins the composited set // (its auto-activate may have lit nothing extra to deactivate, but the // re-commit also drives COMMIT_MODES for the new path). - let snap = isolate_displays_ccd(&keep); + let snap = isolate_displays_ccd_seam(&keep); // Normally DISCARDED — the group restores the FIRST member's snapshot. // But if the first member's isolate FAILED, there is no first snapshot, // and this one just deactivated the physicals with nothing able to put @@ -1643,7 +1679,7 @@ impl VirtualDisplayManager { // snapshot is DISCARDED — the group keeps the first member's (design §6.1). let mut keep = inner.target_ids(); keep.push(new_target); - let _ = isolate_displays_ccd(&keep); + let _ = isolate_displays_ccd_seam(&keep); } Topology::Primary => { // Make the new target primary again (its predecessor held primary), preserving the @@ -1740,7 +1776,7 @@ impl VirtualDisplayManager { // the group keeps the first member's. ShrinkAction::Reisolate => { let keep = inner.target_ids(); - let _ = isolate_displays_ccd(&keep); + let _ = isolate_displays_ccd_seam(&keep); } // Re-promote a survivor rather than leaving the desktop's primary on a target that // is about to be REMOVEd. Same save/restore-the-snapshot dance as diff --git a/crates/pf-vdisplay/src/vdisplay/windows/pf_vdisplay.rs b/crates/pf-vdisplay/src/vdisplay/windows/pf_vdisplay.rs index 5eb8835e..0ab570c8 100644 --- a/crates/pf-vdisplay/src/vdisplay/windows/pf_vdisplay.rs +++ b/crates/pf-vdisplay/src/vdisplay/windows/pf_vdisplay.rs @@ -924,6 +924,161 @@ mod tests { drop(vout); // triggers REMOVE + stops the pinger } + /// Forces `Topology::Exclusive` for the duration of a case and puts the operator's real policy + /// back on drop — including when the case panics. + /// + /// The isolate branch this file's Phase-3 cases exercise runs ONLY under `Exclusive`, and a real + /// install is usually configured otherwise (.173 is `"topology": "extend"`, which is why the + /// first attempt at these cases silently never ran an isolate at all — `topology_action()` + /// returns `effective_topology()` as soon as ANY policy is configured). Note this writes the + /// host's `display-settings.json`; the guard is what makes that safe to do on a real box. + struct ExclusiveTopology(crate::policy::DisplayPolicy); + + impl ExclusiveTopology { + fn force() -> Self { + let original = crate::policy::prefs().get(); + let mut forced = original.clone(); + forced.preset = crate::policy::Preset::Custom; // explicit fields are ignored otherwise + forced.topology = crate::policy::Topology::Exclusive; + crate::policy::prefs() + .set(forced) + .expect("force Topology::Exclusive for this case"); + assert_eq!( + crate::effective_topology(), + crate::policy::Topology::Exclusive, + "the forced policy did not resolve to Exclusive" + ); + Self(original) + } + } + + impl Drop for ExclusiveTopology { + fn drop(&mut self) { + if let Err(e) = crate::policy::prefs().set(self.0.clone()) { + eprintln!("WARNING: could not restore the display policy: {e}"); + } + } + } + + /// §5 3.2 on glass: when the FIRST member's isolate fails, a later member's isolate must be + /// ADOPTED as the group's restore snapshot — otherwise it deactivates the operator's panels + /// with nothing able to put them back. + /// + /// This leg only fires on a FAILED `isolate_displays_ccd`, which real hardware does not + /// produce, so it shipped unexercised. `manager::FAIL_NEXT_ISOLATES` (a `#[cfg(test)]` seam) + /// fails exactly the first isolate, against the real driver and a real panel; the second member + /// then isolates for real and the physical genuinely goes dark mid-test. + /// + /// The assertion is the user-visible one: after both members are torn down, the operator's + /// external panel is ACTIVE again. Without the adoption the group holds no snapshot, + /// `teardown_removed`'s restore is gated on it and never runs, and the panel stays deactivated. + /// + /// ⚠️ Two members means two SLOTS, which is what `slot_id_for(client_fp, …)` keys on — hence the + /// two distinct client fingerprints. Needs `Topology::Exclusive`, which is the default when no + /// policy is configured and `PUNKTFUNK_NO_ISOLATE` is unset; the test asserts an isolate really + /// happened rather than trusting that. + /// + /// ⚠️ If this test leaves the desk dark, recover from the CONSOLE session with + /// `SetDisplayConfig(0,null,0,null, SDC_USE_DATABASE_CURRENT|SDC_APPLY)` — measured rc=0 on + /// .173. `SDC_TOPOLOGY_EXTEND` will NOT do it with a single connected display (rc=31). + #[test] + #[ignore = "needs the pf-vdisplay driver on real hardware; run with --ignored"] + fn live_a_failed_first_isolate_is_recovered_by_adopting_the_next() { + assert!( + std::env::var("PUNKTFUNK_NO_ISOLATE").is_err(), + "PUNKTFUNK_NO_ISOLATE forces Topology::Extend — this case needs Exclusive" + ); + let _topology = ExclusiveTopology::force(); + let physicals_before = active_physicals(); + assert!( + !physicals_before.is_empty(), + "no external physical panel is active, so 'the panel came back' cannot be observed — \ + power the display on first (a TV in standby reads as Code 45 / zero CCD paths)" + ); + println!("physicals before : {physicals_before:?}"); + + // Fail EXACTLY the first member's isolate. + super::super::manager::FAIL_NEXT_ISOLATES.store(1, std::sync::atomic::Ordering::Relaxed); + + let mut vd1 = PfVdisplayDisplay::new().expect("open pf-vdisplay (member 1)"); + vd1.set_client_identity(Some([0xA1; 32])); + let out1 = vd1 + .create(Mode { + width: 1920, + height: 1080, + refresh_hz: 60, + }) + .expect("create member 1"); + thread::sleep(Duration::from_secs(2)); + println!( + "after member 1 (isolate INJECTED to fail): {:?}", + active_targets() + ); + + let mut vd2 = PfVdisplayDisplay::new().expect("open pf-vdisplay (member 2)"); + vd2.set_client_identity(Some([0xB2; 32])); + let out2 = vd2 + .create(Mode { + width: 1280, + height: 720, + refresh_hz: 60, + }) + .expect("create member 2"); + thread::sleep(Duration::from_secs(2)); + let during = active_physicals(); + println!( + "after member 2 (isolate REAL) : {:?}", + active_targets() + ); + println!("physicals during : {during:?}"); + + // The seam must have been consumed — otherwise the injection never took and a pass here + // would prove nothing about the recovery. + assert_eq!( + super::super::manager::FAIL_NEXT_ISOLATES.load(std::sync::atomic::Ordering::Relaxed), + 0, + "the injected isolate failure was never consumed — no isolate ran, so this run proves \ + nothing (is the topology really Exclusive?)" + ); + + drop(out2); + drop(out1); + thread::sleep(Duration::from_secs(6)); // async PnP removal + the restore settling + + let physicals_after = active_physicals(); + println!("physicals after teardown : {physicals_after:?}"); + assert!( + !physicals_after.is_empty(), + "the operator's physical panel was left DEACTIVATED after teardown. The first \ + member's isolate failed, so the group held no restore snapshot; the second member's \ + isolate deactivated the physicals and its snapshot was discarded (sweep §5 3.2). \ + Active targets now: {:?}", + active_targets() + ); + } + + /// The ACTIVE display targets, as `(target_id, friendly)` — not just a count. + /// + /// Counting alone cannot tell "the physical is still lit" from "the physical was deactivated + /// and the virtual took its place", which on a single-panel box are both `1`. Every on-glass + /// claim in this module about panels going dark rests on the identities, so read them. + fn active_targets() -> Vec<(u32, String)> { + pf_win_display::win_display::target_inventory() + .into_iter() + .filter(|t| t.active) + .map(|t| (t.target_id, format!("{} [{}]", t.friendly, t.tech))) + .collect() + } + + /// The active targets that are EXTERNAL PHYSICAL panels — the operator's actual desk. + fn active_physicals() -> Vec<(u32, String)> { + pf_win_display::win_display::target_inventory() + .into_iter() + .filter(|t| t.active && t.external_physical) + .map(|t| (t.target_id, format!("{} [{}]", t.friendly, t.tech))) + .collect() + } + /// `SDC_TOPOLOGY_EXTEND` needs something to extend ACROSS — and that is the state its callers /// are in, which is why this looked like a defect and is not. /// @@ -953,10 +1108,7 @@ mod tests { #[test] #[ignore = "needs the pf-vdisplay driver on real hardware; run with --ignored"] fn live_force_extend_with_a_virtual_display_present() { - fn active_paths() -> Option { - pf_win_display::win_display::count_other_active(&[]) - } - let before = active_paths(); + let before = active_targets(); let mut vd = PfVdisplayDisplay::new().expect("open pf-vdisplay"); let vout = vd .create(Mode { @@ -966,22 +1118,27 @@ mod tests { }) .expect("create virtual display"); thread::sleep(Duration::from_secs(2)); - let with_virtual = active_paths(); + let with_virtual = active_targets(); + let physicals_with_virtual = active_physicals(); pf_win_display::win_display::force_extend_topology(); thread::sleep(Duration::from_secs(2)); - let after_extend = active_paths(); + let after_extend = active_targets(); drop(vout); - thread::sleep(Duration::from_secs(3)); - let after_drop = active_paths(); - println!( - "force-EXTEND on glass: active paths {before:?} -> (virtual up) {with_virtual:?} -> \ - (after force-EXTEND) {after_extend:?} -> (virtual dropped) {after_drop:?}" - ); - assert_ne!( - after_drop, - Some(0), + thread::sleep(Duration::from_secs(6)); // PnP removal is async — a short wait reads a ghost + let after_drop = active_targets(); + println!("force-EXTEND on glass, ACTIVE TARGETS at each step:"); + println!(" before : {before:?}"); + println!(" virtual up : {with_virtual:?} (physicals: {physicals_with_virtual:?})"); + println!(" after force-EXT : {after_extend:?}"); + println!(" virtual dropped : {after_drop:?}"); + assert!( + !after_drop.is_empty(), "the desk was left with NO active display path after the teardown" ); + assert!( + !active_physicals().is_empty(), + "the operator's physical panel was left DEACTIVATED after teardown: {after_drop:?}" + ); } /// Live in-place resize spike — `#[ignore]`d (needs a v4 pf-vdisplay driver installed + the host diff --git a/crates/pf-win-display/src/win_display.rs b/crates/pf-win-display/src/win_display.rs index 721ff45f..dcb50e43 100644 --- a/crates/pf-win-display/src/win_display.rs +++ b/crates/pf-win-display/src/win_display.rs @@ -1000,6 +1000,37 @@ pub struct TargetInventory { pub monitor_device_path: String, } +/// EDID manufacturer id of punktfunk's own IddCx monitors, as it appears in the PnP hardware id and +/// therefore in the CCD monitor device path (`\\?\DISPLAY#PNK….`). The driver stamps `"PNK"` into +/// EDID bytes 8-9 (`packaging/windows/drivers/pf-vdisplay/src/edid.rs`). +const PF_EDID_MANUFACTURER: &str = "PNK"; + +/// Is this monitor device path one of OUR virtual displays? +/// +/// It has to be asked, because the connector class cannot answer it: our IddCx monitor declares +/// `DISPLAYCONFIG_OUTPUT_TECHNOLOGY_HDMI` (driver `monitor.rs`, `IDDCX_MONITOR_INFO::MonitorType`), +/// which [`output_tech_class`]'s allowlist reads as a real external panel — so without this check +/// punktfunk's own display counts as one of the operator's physical monitors. Measured on .173: +/// `target_inventory()` returned `[(4352, "LG TV SSCR2", HDMI), (257, "punktfunk", HDMI)]` with +/// BOTH flagged `external_physical`. +/// +/// That is not cosmetic. [`restore_displays_ccd`]'s last-resort "the desk is not left dark" +/// backstop fires on `connected > 0 && lit == 0` over exactly this set, and the restore runs BEFORE +/// the virtual is REMOVEd — so our own still-active display kept `lit >= 1` and the backstop could +/// never fire, in precisely the situation it was written for. It also made our display a candidate +/// "physical suspect" in the disturbance-attribution inventory, which [`TargetInventory`]'s own doc +/// says can never happen ("only indirect/virtual targets (our own IDD included)"). +/// +/// Matching on the device path rather than the friendly name: the name comes from the EDID's 0xFC +/// descriptor and is what a user sees, while the path carries the manufacturer id the OS itself +/// derived. Allowlist-shaped like [`output_tech_class`] — anything unrecognised stays "not ours", +/// so a third-party virtual display is never silently adopted. +fn is_our_virtual_display(monitor_device_path: &str) -> bool { + monitor_device_path + .to_ascii_uppercase() + .contains(PF_EDID_MANUFACTURER) +} + /// Classify a CCD output technology: `(external physical?, log label)`. Allowlist, not blocklist: /// new/unknown/indirect technologies read as non-external, so a co-installed third-party virtual /// display can never be mistaken for a physical suspect (same precision rule as `monitor_devnode`). @@ -1096,7 +1127,14 @@ pub fn target_inventory() -> Vec { if unsafe { DisplayConfigGetDeviceInfo(&mut req.header) } != 0 { continue; // target with no queryable monitor — nothing to attribute to } - let (external_physical, tech) = output_tech_class(req.outputTechnology); + let monitor_device_path = utf16z_str(&req.monitorDevicePath); + let (mut external_physical, mut tech) = output_tech_class(req.outputTechnology); + // Our own IddCx monitor claims HDMI, so the connector class alone would call it one of the + // operator's panels — see `is_our_virtual_display` for what that broke. + if is_our_virtual_display(&monitor_device_path) { + external_physical = false; + tech = "punktfunk-virtual"; + } out.push(TargetInventory { target_id: t.id, active: active.contains(&key), @@ -1104,7 +1142,7 @@ pub fn target_inventory() -> Vec { internal_panel: tech == "internal-panel", tech, friendly: utf16z_str(&req.monitorFriendlyDeviceName), - monitor_device_path: utf16z_str(&req.monitorDevicePath), + monitor_device_path, }); } out @@ -1685,6 +1723,62 @@ mod live_tests { /// where the query returned 0x57 ERROR_INVALID_PARAMETER (and 0x5 ERROR_ACCESS_DENIED from /// session 0). This case FAILS there without the zero-path short-circuit and passes with it, /// while a box that does have a display lit passes either way. + /// Pure, so it runs everywhere — the identification rule itself needs no hardware. + #[test] + fn our_own_virtual_display_is_never_an_external_physical() { + assert!(super::is_our_virtual_display( + r"\\?\DISPLAY#PNK0000#5&1234abcd&0&UID257#{e6f07b5f-ee97-4a90-b076-33f57bf4eaa7}" + )); + // Case-insensitive: the OS is not consistent about the path's case. + assert!(super::is_our_virtual_display( + r"\\?\display#pnk0000#5&1&0&uid257#{guid}" + )); + // A real panel, and a third-party virtual display, both stay physical suspects. + assert!(!super::is_our_virtual_display( + r"\\?\DISPLAY#GSM83CD#5&367fb4cb&0&UID4352#{e6f07b5f-ee97-4a90-b076-33f57bf4eaa7}" + )); + assert!(!super::is_our_virtual_display( + r"\\?\DISPLAY#SMVD0001#5&1&0&UID999#{guid}" + )); + } + + /// Read-only: prove on real hardware that punktfunk's own display is not counted among the + /// operator's physical panels. Creates and destroys nothing, so it is safe to run against a + /// live host — which matters, because repeated IddCx create/destroy cycles are exactly what + /// wedges the driver's slot pool. + /// + /// Measured on .173 BEFORE the fix: `[(4352, "LG TV SSCR2", HDMI), (257, "punktfunk", HDMI)]` + /// with both flagged `external_physical`, because the driver declares + /// `DISPLAYCONFIG_OUTPUT_TECHNOLOGY_HDMI`. + #[test] + #[ignore = "hardware: reads the live display topology"] + fn our_own_display_is_excluded_from_the_operators_physicals_on_real_hardware() { + let inv = target_inventory(); + for t in &inv { + println!( + "target {:>5} active={:<5} external_physical={:<5} tech={:<18} {:?} {}", + t.target_id, + t.active, + t.external_physical, + t.tech, + t.friendly, + t.monitor_device_path + ); + } + for t in inv + .iter() + .filter(|t| is_our_virtual_display(&t.monitor_device_path)) + { + assert!( + !t.external_physical, + "our own display {} ({:?}) is still counted as one of the operator's physical \ + panels — `restore_displays_ccd`'s dark-desk backstop keys on exactly this set and \ + would never fire", + t.target_id, t.friendly + ); + } + } + #[test] #[ignore = "hardware: reads the live display topology"] fn a_host_with_nothing_lit_reports_zero_actives_rather_than_a_failed_query() {