unom/punktfunk
6
0
Fork 1
Code Issues 2 Pull Requests Actions Packages Projects Releases 40 Wiki Activity

fix(vdisplay/windows): three teardown gates that each ended with the operator's panels dark

Browse Source 
All in the same ~120 lines, all the same shape: a recovery leg gated on the wrong
flag, so the path that turned something off had no path that turned it back on.

**The DDC wake was nested inside the CCD-restore arm.** Panels are commanded dark over
DDC/CI *before* the isolate, but `panel_on_all()` sat inside
`if let Some(saved) = ccd_saved.take()` — and `isolate_displays_ccd` returns `None`
whenever its `query_active_config` fails. So a failed isolate meant the panels were
never woken, and because the link stays live there is no returning signal to trigger a
DPMS wake either: dark for the rest of the host's life. Hoisted out of that arm, next
to the `pnp_disabled` restore which was already outside it for exactly this reason.

**A later member's isolate could deactivate the physicals with nothing able to restore
them.** If the FIRST member's isolate failed, `ccd_saved` stayed `None` and
`ccd_exclusive` `false`; a second member's isolate then succeeded, blanked the
physicals, and discarded its snapshot as "the group restores the first member's" —
except there wasn't one. Teardown's restore is gated on `ccd_saved`, and the re-assert
watchdog never started. The group now adopts the first SUCCESSFUL isolate's snapshot.

**A non-last-member teardown ran the exclusive isolate on a Primary group.** The gate
was `ccd_saved.is_some()`, but `Topology::Primary` stores a snapshot too (from
`set_virtual_primary_ccd`) — so a shrink cleared `DISPLAYCONFIG_PATH_ACTIVE` on every
non-kept path, blanking precisely the physical displays `Primary` exists to keep lit.
It keys off `ccd_exclusive` now (what the re-assert watchdog already checks), and a
Primary shrink re-promotes a survivor instead, since the departing member may have
been the one holding primary.

Also: `apply_group_layout` had only ever run from `acquire`, so a member LEAVING never
re-arranged the survivors — they kept the origins computed while the departing monitor
was still between them, leaving a dead gap in the desktop until the next acquire. Now
called at the end of `teardown_removed`, which is where the slot map has already shrunk
and after the REMOVE, so it covers all six teardown paths at once.

The shrink gate is extracted as `shrink_action` so it can be pinned without a driver or
a desktop — manager.rs had no tests at all, which is how a gate keyed on the wrong flag
survived. Verified on the Windows runner: 48 passed (was 46), and both new tests FAIL
against the old gate, so they are not vacuous.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
enricobuehler
2026-07-28 19:46:15 +02:00
co-authored by Claude Opus 5
parent 8b076f5cc9
commit 2c015bba30
1 changed files with 131 additions and 27 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/pf-vdisplay/src/vdisplay/windows/manager.rs
+120 -16
View File
@@ -165,6 +165,34 @@ struct GroupState {
ccd_exclusive: bool,
}
/// What a NON-LAST-member teardown owes the group's topology.
///
/// Split out of [`ManagerInner::teardown_removed`] so the gate is testable without a driver, a CCD
/// device or a desktop — the Windows half of this crate has no other way to pin a decision.
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
enum ShrinkAction {
/// Exclusive group: re-issue the isolate over the shrunk managed set.
Reisolate,
/// Primary group: re-promote a survivor — the departing member may have been holding primary.
RepromotePrimary,
/// Extend/Auto, or nothing was ever captured: leave the topology alone.
Nothing,
}
/// `ccd_exclusive` is the discriminator, NOT `ccd_saved.is_some()`: `Topology::Primary` stores a
/// snapshot too (from `set_virtual_primary_ccd`), so keying on the snapshot ran the EXCLUSIVE
/// isolate on a Primary group — clearing `DISPLAYCONFIG_PATH_ACTIVE` on every non-kept path, i.e.
/// blanking the very physical displays `Primary` exists to keep lit.
fn shrink_action(ccd_exclusive: bool, has_saved: bool) -> ShrinkAction {
if ccd_exclusive {
ShrinkAction::Reisolate
} else if has_saved {
ShrinkAction::RepromotePrimary
} else {
ShrinkAction::Nothing
}
}
/// The manager's guarded state: the slot map + the (single) group record. One lock for both — every
/// group mutation happens on a slot transition, so splitting them would only invite lock-order bugs.
#[derive(Default)]
@@ -1164,11 +1192,28 @@ impl VirtualDisplayManager {
} else {
// 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). The returned
// snapshot is DISCARDED — the group restores the FIRST member's.
// re-commit also drives COMMIT_MODES for the new path).
// SAFETY: as above — borrowed slice of Copy ids, owned return, under the
// `state` lock.
let _ = unsafe { isolate_displays_ccd(&keep) };
let snap = unsafe { isolate_displays_ccd(&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
// them back: `teardown_removed`'s restore is gated on `ccd_saved`, and
// the re-assert watchdog never started either. Adopt the first
// SUCCESSFUL isolate as the group's snapshot instead.
if inner.group.ccd_saved.is_none() {
if let Some(snap) = snap {
tracing::warn!(
"display isolate (CCD): the first member captured no restore \
snapshot (its isolate failed) — adopting this member's, so \
teardown can still put the physical displays back"
);
inner.group.ccd_saved = Some(snap);
inner.group.ccd_exclusive = true;
self.ensure_exclusive_watch();
}
}
}
}
Topology::Primary if first_member => {
@@ -1602,11 +1647,15 @@ impl VirtualDisplayManager {
// the one thing its callers could otherwise get wrong. The `take()` above makes
// this the only consumer of that snapshot, so no second restore can race it.
unsafe { restore_displays_ccd(&saved) };
// EXPERIMENTAL `ddc_power_off` wake: the restore re-activated the physical paths, and
// returning signal alone wakes DPMS-off panels on most firmware — the explicit ON is
// belt-and-braces for the rest. The brief settle wait lets the re-activated paths show
// up in EnumDisplayMonitors (no HMONITOR, no DDC channel); teardown is already
// seconds-scale and watched by the 10 s wedge logger above.
}
// EXPERIMENTAL `ddc_power_off` wake. OUTSIDE the `ccd_saved` gate, for the same reason
// `pnp_disabled` is above it: the panels were commanded dark BEFORE the isolate, and
// the isolate can return `None` (its `query_active_config` failed). Nested inside that
// arm, the wake then never ran — and because the link stays live, there is no returning
// signal to trigger a DPMS wake either, so the panels stayed dark for the rest of the
// host's life. The brief settle wait lets re-activated paths show up in
// EnumDisplayMonitors (no HMONITOR, no DDC channel); teardown is already seconds-scale
// and watched by the 10 s wedge logger above.
if inner.group.ddc_panels_off > 0 {
thread::sleep(Duration::from_millis(300));
let woken = crate::ddc::panel_on_all();
@@ -1617,17 +1666,34 @@ impl VirtualDisplayManager {
);
inner.group.ddc_panels_off = 0;
}
}
} else if inner.group.ccd_saved.is_some() {
// Siblings remain and an exclusive isolate is live: re-issue it with the SHRUNK managed
// set (defensive — the departing monitor's path dies with the REMOVE below anyway, but
// a re-commit keeps the surviving set authoritative if the OS re-lit anything). The
// returned snapshot is discarded; the group keeps the first member's.
} else {
match shrink_action(inner.group.ccd_exclusive, inner.group.ccd_saved.is_some()) {
// Re-issue the isolate over the shrunk set (defensive — the departing monitor's
// path dies with the REMOVE below anyway, but a re-commit keeps the surviving set
// authoritative if the OS re-lit anything). The returned snapshot is discarded;
// the group keeps the first member's.
ShrinkAction::Reisolate => {
let keep = inner.target_ids();
// SAFETY: `isolate_displays_ccd` only drives the CCD query/apply FFI over a borrowed
// slice of Copy target ids, under the `state` lock — the sole topology mutator.
// SAFETY: `isolate_displays_ccd` only drives the CCD query/apply FFI over a
// borrowed slice of Copy target ids, under the `state` lock — the sole
// topology mutator.
let _ = unsafe { isolate_displays_ccd(&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
// `reisolate_after_swap`, and for the same reason: `set_virtual_primary_ccd`
// recaptures one and the group must keep the FIRST member's.
ShrinkAction::RepromotePrimary => {
if let Some(&survivor) = inner.target_ids().first() {
let keep_saved = inner.group.ccd_saved.take();
// SAFETY: `Copy` target id by value, owned return, under the `state` lock.
let _ = unsafe { set_virtual_primary_ccd(survivor) };
inner.group.ccd_saved = keep_saved;
}
}
ShrinkAction::Nothing => {}
}
}
// SAFETY: `teardown_removed`'s own `# Safety` contract guarantees `dev` is the live control
// handle, and `remove_monitor` requires exactly that. `&mon.key` borrows the `MonitorKey`
// inside the still-owned `mon`, alive for this synchronous IOCTL, so the pointer the driver
@@ -1648,6 +1714,14 @@ impl VirtualDisplayManager {
"virtual-display monitor removed"
);
}
// Re-arrange whatever is LEFT. `apply_group_layout` had only ever been called from
// `acquire` (fresh create, re-arrival, in-place resize), so a member leaving never
// triggered it: the survivors kept the CCD origins computed when the departing monitor was
// still between them, leaving a dead gap in the desktop that nothing closed until the next
// acquire. Here rather than at the six call sites because this is where the slot map has
// already shrunk, and after the REMOVE so the departing path is genuinely gone. It no-ops
// for a group of fewer than two.
self.apply_group_layout(inner);
done.store(true, Ordering::SeqCst);
}
@@ -2052,3 +2126,33 @@ pub(crate) fn snapshot() -> Vec<ManagedInfo> {
pub(crate) fn force_release(slot: Option<u64>) -> usize {
VDM.get().map(|m| m.force_release(slot)).unwrap_or(0)
}
#[cfg(test)]
mod tests {
use super::{shrink_action, ShrinkAction};
/// The gate a non-last-member teardown keys off. It used to be `ccd_saved.is_some()`, which is
/// true for BOTH topologies — so a `Primary` group shrinking ran the exclusive isolate and
/// blanked the physical displays that `Primary` exists to keep lit.
#[test]
fn a_primary_group_shrink_repromotes_instead_of_isolating() {
// Primary: a snapshot exists (from `set_virtual_primary_ccd`) but exclusivity was never
// asserted. This is the case the old gate got wrong.
assert_eq!(
shrink_action(false, true),
ShrinkAction::RepromotePrimary,
"a Primary group must never run the exclusive isolate on a shrink"
);
// Exclusive: re-isolate over the shrunk managed set, as before.
assert_eq!(shrink_action(true, true), ShrinkAction::Reisolate);
// Extend/Auto captured nothing — leave the operator's topology alone.
assert_eq!(shrink_action(false, false), ShrinkAction::Nothing);
}
/// Exclusivity is the discriminator on its own: a group that asserted it still re-isolates even
/// if the snapshot went missing, because the physicals are deactivated either way.
#[test]
fn exclusivity_decides_without_a_snapshot() {
assert_eq!(shrink_action(true, false), ShrinkAction::Reisolate);
}
}