feat(daemon): handle dGPU external displays automatically - #139
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe daemon adds an external-display setting, detects connected DRM displays, separates requested and effective GPU modes, and monitors display changes through shared mode state. ChangesExternal display mode coordination
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DRM
participant DisplayMonitor
participant GPUDiscovery
participant ModeInterface
participant GPU
DRM->>DisplayMonitor: report connector topology change
DisplayMonitor->>GPUDiscovery: inspect external connector status
GPUDiscovery-->>DisplayMonitor: return connected display state
DisplayMonitor->>ModeInterface: apply effective mode
ModeInterface->>GPU: apply GPU mode and eBPF updates
ModeInterface-->>DisplayMonitor: emit effective mode change
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/cardwire-daemon/src/interface/config.rs`:
- Line 19: Add matching #[zbus(property)] getter and setter methods for
ConfigMemory::external_display_auto_switch in the configuration D-Bus interface,
following the existing property methods’ naming, signatures, and Arc<AtomicBool>
load/store pattern so clients can read and update the setting live.
In `@crates/cardwire-daemon/src/interface/mode.rs`:
- Around line 262-266: Add a read-only D-Bus property alongside the mode method
that returns the persisted requested mode, while keeping mode() returning the
effective current_mode_value(). Use the existing requested-mode state/accessor
and expose both values so clients can distinguish configured and active modes.
In `@crates/cardwire-daemon/src/tasks/monitor_display.rs`:
- Around line 99-104: Update the reconcile flow around
external_display_connected so the synchronous sysfs read executes via Tokio’s
blocking-task mechanism rather than directly on the async worker thread, while
preserving its existing error mapping and None => false behavior. Await the
spawned blocking operation before assigning connected, and keep the transition
lock handling unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e7a16204-48b0-4f51-ae74-41602d86d1c2
📒 Files selected for processing (12)
crates/cardwire-daemon/src/core/gpu/discover.rscrates/cardwire-daemon/src/core/gpu/mod.rscrates/cardwire-daemon/src/daemon.rscrates/cardwire-daemon/src/file/config.rscrates/cardwire-daemon/src/interface/config.rscrates/cardwire-daemon/src/interface/debug.rscrates/cardwire-daemon/src/interface/mode.rscrates/cardwire-daemon/src/models.rscrates/cardwire-daemon/src/tasks/mod.rscrates/cardwire-daemon/src/tasks/monitor_display.rsdocs/getting-started/installation.mdnix/nixos-module.nix
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/cardwire-daemon/src/tasks/monitor_display.rs (1)
134-144: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftKeep the failed DRM replay spell pending.
When the write at Line 140 fails, this branch logs the error and still returns success.
effective_modeis alreadyHybrid. A later reconciliation seestarget == previous, so this block is skipped and thechangeevent is not retried. If no new hotplug event arrives, the compositor may not discover the external display.Track a pending replay and retry it during later reconciliation. Clear the pending state only after the write succeeds. Add a failure-and-retry test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cardwire-daemon/src/tasks/monitor_display.rs` around lines 134 - 144, Track failed DRM change-event writes in the display monitor’s reconciliation state, preserving the pending replay when the write in the Hybrid transition branch fails. Retry that pending replay on subsequent reconciliation even when target equals previous, and clear it only after tokio::fs::write succeeds; add a test covering failure followed by a later successful retry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/cardwire-daemon/src/tasks/monitor_display.rs`:
- Around line 134-144: Track failed DRM change-event writes in the display
monitor’s reconciliation state, preserving the pending replay when the write in
the Hybrid transition branch fails. Retry that pending replay on subsequent
reconciliation even when target equals previous, and clear it only after
tokio::fs::write succeeds; add a test covering failure followed by a later
successful retry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b8b1a2ca-6238-4947-a42b-f8146d928043
📒 Files selected for processing (1)
crates/cardwire-daemon/src/tasks/monitor_display.rs
luytan
left a comment
luytan
left a comment
There was a problem hiding this comment.
I will be honest, i think the complexity added in mode.rs is unnecessary and should be avoided. To set a mode, the execution goes throught set_mode() -> self.display_mode.set() -> self.apply_target() -> mode.apply_mode() -> mode.update_mode_bpf_map() -> mode.save_state.
A simpler approach would be to only hold an Arc<RwLock> for the effective mode inside the ModeInterface. ModeInterface can handle applying and persiting modes directly without delegating to an external DisplayMode struct.
The display monitor task can then remain a simple background listener that triggers temporary effective mode updates when displays are connected or disconnedted, eliminating the circular dependency and the extra DisplayMode.
You can add a function named effective_set_mode() inside mode.rs, that would be used by the task to set the mode without saving it to memory
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/cardwire-daemon/src/interface/mode.rs`:
- Around line 183-191: Remove the public(crate) gpu_list() and config()
accessors from ModeInterface, and add a detect_display_target method that
accepts the requested target and internally uses self.gpu_list and self.config
to call detect_external_display_target. Update both monitor_display.rs call
sites to invoke mode.detect_display_target(requested).await? so ModeInterface
retains ownership of its state.
- Around line 204-209: Update set_requested_mode to handle errors from
detect_external_display_target like apply_mode_at_startup: log a warning and
fall back to the requested mode instead of propagating the error. Preserve
successful target detection and ensure the requested mode is still applied and
persisted when external display detection fails.
- Around line 204-217: Update set_requested_mode so that after
save_mode(requested) completes, it emits the zbus-generated
requested_mode_changed signal through the SignalEmitter API, ensuring the
requested_mode property change is broadcast even though save_mode bypasses its
setter.
In `@crates/cardwire-daemon/src/tasks/monitor_display.rs`:
- Around line 74-111: Make reconcile_display_mode’s read-decide-apply sequence
atomic by adding a ModeInterface method that acquires the transition mutex once,
reads the requested mode, detects the display target, and applies the effective
mode while holding the lock. Keep the DISPLAY_RESTORE_WAIT sleep outside the
mutex, then perform the final requested-mode read, target detection, and
effective_set_mode operation through the new atomic method; preserve DRM event
replay and return values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9e1b26c2-e3d0-4ef4-9f66-6502fa6ba494
📒 Files selected for processing (4)
crates/cardwire-daemon/src/interface/mode.rscrates/cardwire-daemon/src/models.rscrates/cardwire-daemon/src/tasks/mod.rscrates/cardwire-daemon/src/tasks/monitor_display.rs
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…tor reconciliation atomic
175fb3e to
3d370ca
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/cardwire-daemon/src/interface/mode.rs (2)
172-183: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winA signal is rung for a bell that never moved.
emit_mode_changefires bothmode_changedandrequested_mode_changedwheneverchangedistrue.changedhere reflects the effective mode changing (fromreconcile_effective_mode/reconcile_display_modein the monitor task), not the persisted requested mode.set_modealready emitsrequested_mode_changedexplicitly on the D-Bus setter path (Line 346), covering the case where the user actually changes the request.When the automatic display-topology reconciliation flips the effective mode,
requested_modeinmode_statehas not changed at all. Emittingrequested_mode_changedhere tells D-Bus clients that the persisted setting changed when it did not, which contradicts the whole point of separatingmode(effective) fromrequested_mode(persisted) documented at Line 356-357.🪄 Proposed fix
pub async fn emit_mode_change( &self, interface: &InterfaceRef<ModeInterface>, changed: bool, ) -> zbus::Result<()> { if changed { self.mode_changed(interface.signal_emitter()).await?; - self.requested_mode_changed(interface.signal_emitter()) - .await?; } Ok(()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cardwire-daemon/src/interface/mode.rs` around lines 172 - 183, Update emit_mode_change to emit only mode_changed when changed is true; remove the requested_mode_changed call because this path reports effective-mode reconciliation, while set_mode remains responsible for requested-mode notifications.
213-222: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused
effective_set_modeincantation. The repository contains no callers, so remove the method and its#[allow(dead_code)]attribute.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cardwire-daemon/src/interface/mode.rs` around lines 213 - 222, Remove the unused effective_set_mode method from the mode interface, including its #[allow(dead_code)] attribute; no replacement or caller changes are needed.crates/cardwire-daemon/src/tasks/monitor_display.rs (1)
75-120: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winThe restoration spell is cast before the waiting begins.
mode.reconcile_effective_mode()at Line 80 both detects the target and applies it atomically, before theinfo!at Line 86-89 and thetokio::time::sleep(DISPLAY_RESTORE_WAIT)at Line 90. So the revert toIntegrated/Smartalready happened by the time the log claims restoration will occur "after {} seconds". The wait and secondreconcile_effective_mode()call only decide whether to flip back toHybridif the display reconnects quickly.For a display that flickers or bounces on disconnect (common with KVM switches and USB-C docks), this causes two real hardware transitions in quick succession: an immediate revert to
Integrated/Smart(blocking the dGPU, restartingnvidia-powerd.servicethrough the sharedapply_modepath), followed by a flip back toHybridmoments later. This defeats the debounce the wait was meant to provide, and it directly undermines the feature's goal of keeping the dGPU available through a physical disconnect blip.Peek the topology (without applying) before deciding to wait, and only call
reconcile_effective_mode()once, after the wait settles the state.🪄 Proposed fix
async fn reconcile_display_mode( mode: &ModeInterface, was_connected: bool, ) -> fdo::Result<(bool, Option<u32>)> { - let (first_changed, first_requested, first_target, first_card) = - mode.reconcile_effective_mode().await?; - - if was_connected - && first_card.is_none() - && matches!(first_requested, Modes::Integrated | Modes::Smart) - { - info!( - "external dGPU display disconnected; restoring the configured mode after {} seconds", - DISPLAY_RESTORE_WAIT.as_secs() - ); - tokio::time::sleep(DISPLAY_RESTORE_WAIT).await; - let (changed, requested, target, card) = mode.reconcile_effective_mode().await?; - if card.is_some() { - info!("external display reconnected; keeping the current mode"); - } - if changed - && target == Modes::Hybrid - && requested != Modes::Hybrid - && let Some(card) = card - { - let path = format!("/sys/class/drm/card{card}/uevent"); - if let Err(err) = tokio::fs::write(&path, "change\n").await { - warn!("failed to replay DRM change event through {path}: {err}"); - } - } - return Ok((changed, card)); - } - - if first_changed - && first_target == Modes::Hybrid - && first_requested != Modes::Hybrid - && let Some(card) = first_card - { - let path = format!("/sys/class/drm/card{card}/uevent"); - if let Err(err) = tokio::fs::write(&path, "change\n").await { - warn!("failed to replay DRM change event through {path}: {err}"); - } - } - - Ok((first_changed, first_card)) + let requested = mode.requested_mode_value().await; + if was_connected && matches!(requested, Modes::Integrated | Modes::Smart) { + let disconnected = matches!(mode.detect_display_target(requested).await, Ok((_, None))); + if disconnected { + info!( + "external dGPU display disconnected; restoring the configured mode after {} seconds", + DISPLAY_RESTORE_WAIT.as_secs() + ); + tokio::time::sleep(DISPLAY_RESTORE_WAIT).await; + } + } + + let (changed, requested, target, card) = mode.reconcile_effective_mode().await?; + if changed && target == Modes::Hybrid && requested != Modes::Hybrid && let Some(card) = card { + let path = format!("/sys/class/drm/card{card}/uevent"); + if let Err(err) = tokio::fs::write(&path, "change\n").await { + warn!("failed to replay DRM change event through {path}: {err}"); + } + } + Ok((changed, card)) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cardwire-daemon/src/tasks/monitor_display.rs` around lines 75 - 120, Update reconcile_display_mode so the initial topology check does not apply a mode; when a prior connection is lost and the requested mode is Integrated or Smart, wait DISPLAY_RESTORE_WAIT before making the single reconcile_effective_mode call. Recheck the settled topology before applying, preserve the reconnect and DRM event handling, and avoid any mode transition before the debounce wait.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/cardwire-daemon/src/interface/mode.rs`:
- Around line 172-183: Update emit_mode_change to emit only mode_changed when
changed is true; remove the requested_mode_changed call because this path
reports effective-mode reconciliation, while set_mode remains responsible for
requested-mode notifications.
- Around line 213-222: Remove the unused effective_set_mode method from the mode
interface, including its #[allow(dead_code)] attribute; no replacement or caller
changes are needed.
In `@crates/cardwire-daemon/src/tasks/monitor_display.rs`:
- Around line 75-120: Update reconcile_display_mode so the initial topology
check does not apply a mode; when a prior connection is lost and the requested
mode is Integrated or Smart, wait DISPLAY_RESTORE_WAIT before making the single
reconcile_effective_mode call. Recheck the settled topology before applying,
preserve the reconnect and DRM event handling, and avoid any mode transition
before the debounce wait.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 09990266-f99e-4c1f-bad7-c466647e3328
📒 Files selected for processing (2)
crates/cardwire-daemon/src/interface/mode.rscrates/cardwire-daemon/src/tasks/monitor_display.rs
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/cardwire-daemon/src/interface/config.rs (1)
57-57: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve the previous D-Bus binding for existing familiars. The rename updates
cardwire-guiandcardwire-cli, but removescom.github.opengamingcollective.cardwireandcom.github.opengamingcollective.cardwire.Config. Existing clients cannot connect. Keep the old endpoint during migration, or document this release as a breaking D-Bus API change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cardwire-daemon/src/interface/config.rs` at line 57, Update the D-Bus interface declaration at the Config interface attribute to retain the previous com.github.opengamingcollective.cardwire.Config binding alongside the renamed org.opengamingcollective.cardwire.Config endpoint, preserving connectivity for existing clients during migration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/cardwire-daemon/src/daemon.rs`:
- Around line 70-79: Update the ModeInterface lookup in spawn_dbus_api to use
the same object path as its registration, /org/opengamingcollective/cardwire,
instead of the mismatched /com/github/opengamingcollective/cardwire path.
Preserve the existing error propagation and background task setup.
In `@crates/cardwire-daemon/src/interface/mode.rs`:
- Around line 172-183: Update emit_mode_change so the changed path emits only
mode_changed and no longer calls requested_mode_changed. Keep
requested_mode_changed emission confined to methods that actually modify the
persisted requested mode, such as set_mode, while preserving the existing
behavior and error propagation for mode_changed.
In `@crates/cardwire-daemon/src/tasks/monitor_display.rs`:
- Around line 34-72: Update detect_external_display_target and its callers to
represent external_display_connected failures as a distinct Unknown topology
result rather than treating them as disconnected or returning (requested, None).
Propagate Unknown through reconcile_display_mode, set_requested_mode, and
apply_mode_at_startup; skip effective-mode changes and connection-state updates
whenever topology is Unknown, preserving the current mode and state.
---
Outside diff comments:
In `@crates/cardwire-daemon/src/interface/config.rs`:
- Line 57: Update the D-Bus interface declaration at the Config interface
attribute to retain the previous com.github.opengamingcollective.cardwire.Config
binding alongside the renamed org.opengamingcollective.cardwire.Config endpoint,
preserving connectivity for existing clients during migration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 745be570-1f13-481b-829a-91a45f6fb116
📒 Files selected for processing (12)
crates/cardwire-daemon/src/core/gpu/discover.rscrates/cardwire-daemon/src/core/gpu/mod.rscrates/cardwire-daemon/src/daemon.rscrates/cardwire-daemon/src/file/config.rscrates/cardwire-daemon/src/interface/config.rscrates/cardwire-daemon/src/interface/debug.rscrates/cardwire-daemon/src/interface/mode.rscrates/cardwire-daemon/src/models.rscrates/cardwire-daemon/src/tasks/mod.rscrates/cardwire-daemon/src/tasks/monitor_display.rsdocs/getting-started/installation.mdnix/nixos-module.nix
2 participants
Description
Add an opt-in
external_display_auto_switchsetting that keeps the dGPU available when a physical external display is connected to a dGPU-only port.When enabled, the daemon monitors DRM topology changes and temporarily overrides Integrated or Smart mode with Hybrid. It preserves the requested mode, exposes and signals the effective mode over D-Bus, and restores the requested mode after the display disconnects. Connector status read failures are treated as unknown topology rather than as a confirmed disconnect.
This prevents Integrated and Smart modes from making a display-driving dGPU unavailable. The setting defaults to false, so existing installations retain their current behavior until it is explicitly enabled.
Related issue: none.
TODO
Checklist: