refactor(cardwired): rework GPU enumeration, add discrete GPU detection and fix switcheroo - #144
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 PR replaces legacy GPU discovery with Vulkan- and EGL-assisted enumeration. It adds GPU availability and capability state, updates display-mode and watcher handling, filters unavailable devices, and generates vendor-specific Switcheroo environment variables. Build environments now provide the required graphics libraries. ChangesGPU enumeration and routing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DaemonManager
participant GpuEnumerator
participant DisplayMode as display mode
participant Switcheroo
participant Clients as CLI and GUI
DaemonManager->>GpuEnumerator: enumerate PCI graphics devices
GpuEnumerator-->>DaemonManager: return GPU records with availability and capability flags
DaemonManager->>DisplayMode: apply mode using available GPUs
DisplayMode->>DisplayMode: validate discrete GPU and connected-display state
DaemonManager->>Switcheroo: publish available GPU properties
Switcheroo-->>Clients: provide routing and GPU metadata
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
…f doing it manually
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
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/core/gpu/models.rs (1)
139-166: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftBind these four booleans to names before they hex a call site.
GpuDevice::newnow accepts four adjacent unnamedboolvalues. Any transposition ofdiscrete,vfio,available, orvirtual_gpuat a call site still compiles, and the result changes real routing:is_availablegates Switcheroo exposure, mode gating incrates/cardwire-daemon/src/interface/mode.rs, and the block guard incrates/cardwire-daemon/src/interface/gpu.rs. The#[allow(clippy::too_many_arguments)]marker is the lint telling you the same thing.Pass a named flags struct instead. The allow attribute then disappears, and each call site states its intent.
♻️ Proposed shape
+#[derive(Debug, Clone, Copy, Default)] +pub struct GpuFlags { + pub discrete: bool, + pub vfio: bool, + pub available: bool, + pub virtual_gpu: bool, +} + impl GpuDevice { - #[allow(clippy::too_many_arguments)] pub fn new( name: String, pci: PciDevice, render: u32, card: u32, default: Option<bool>, gpu_vendor: GpuVendor, nvidia_minor: Option<u32>, - discrete: bool, - vfio: bool, - available: bool, - virtual_gpu: bool, + flags: GpuFlags, ) -> GpuDevice { GpuDevice { name, pci, render, card, default, gpu_vendor, nvidia_minor, - discrete, - vfio, - available, - virtual_gpu, + discrete: flags.discrete, + vfio: flags.vfio, + available: flags.available, + virtual_gpu: flags.virtual_gpu, } }Call sites in
crates/cardwire-daemon/src/core/gpu/enumerator.rsthen become explicit:GpuFlags { discrete, available, ..Default::default() }🤖 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/core/gpu/models.rs` around lines 139 - 166, Replace the adjacent boolean parameters of GpuDevice::new with a named GpuFlags struct containing discrete, vfio, available, and virtual_gpu, deriving or implementing Default as appropriate. Remove the #[allow(clippy::too_many_arguments)] attribute, update GpuDevice construction to read the struct fields, and revise every enumerator call site to use explicit GpuFlags values such as GpuFlags { discrete, available, ..Default::default() }.crates/cardwire-daemon/src/interface/debug.rs (1)
115-127: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftThis early return abandons the daemon mid-ritual.
By Line 124 the rebuild has already replaced
*pci_listat Line 95, removed every GPU object from the object server, aborted every power task, and clearedgpu_interfaces. IfGpuInterface::buildfails here,refresh_gpureturns at once. The Hybrid fallback described in the comment at Lines 151 to 155 never runs, so the eBPF block map stays exactly as your own comment warns against: stale, with a GPU possibly still blocked. The published GPU API is also left partial, and no watcher exists for the devices that were dropped.The condition then becomes permanent.
*pci_listalready holds the new list, so the nextrefresh_gpufinds no difference and skips the repair entirely. The same reasoning applies to theobject_server.at(...).await?at Line 134.Route both failures through the Hybrid fallback instead of returning directly. Extract the rebuild into a helper that returns a
Result, then apply the existing fallback to its error.🐛 Proposed shape
- for (id, device) in new_gpu_list { - let gpu = GpuInterface::build( - id as u32, - device, - Arc::clone(&self.blocker), - Arc::clone(&self.pci_list), - Arc::clone(&self.gpu_state), - Arc::clone(&self.mode_state), - ) - .map_err(|err| fdo::Error::Failed(err.to_string()))?; - - gpu_interfaces.insert(id, gpu); - } + let mut build_error = None; + for (id, device) in new_gpu_list { + match GpuInterface::build( + id as u32, + device, + Arc::clone(&self.blocker), + Arc::clone(&self.pci_list), + Arc::clone(&self.gpu_state), + Arc::clone(&self.mode_state), + ) { + Ok(gpu) => { + gpu_interfaces.insert(id, gpu); + } + // Record and continue: an early return would leave the eBPF block map stale + // with no hybrid fallback. + Err(err) => { + warn!("failed to build GPU interface {id}: {err}"); + build_error.get_or_insert(fdo::Error::Failed(err.to_string())); + } + } + }Then, after the locks are dropped, if
build_erroris set, applyeffective_set_mode(Modes::Hybrid, true)before returning the error.🤖 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/debug.rs` around lines 115 - 127, Refactor the rebuild logic in refresh_gpu into a helper returning a Result, and route both GpuInterface::build failures and object_server.at failures into that result instead of returning immediately. After releasing the relevant locks, if the rebuild failed, invoke effective_set_mode with Hybrid and force enabled, then return the original error. Ensure the fallback runs before any error return so the daemon restores a consistent Hybrid state.crates/cardwire-daemon/src/models.rs (1)
179-192: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRenew the NVIDIA inode map after GPU hotplug.
DebugInterface::refresh_gpurebuilds GPU IDs, but it does not repopulateCW_EXP_BLK_INO. Sinceblock_exp_inodeonly inserts entries, stale IDs and missing inodes persist. ClearCW_EXP_BLK_INO, then repopulate it from the refreshed GPU list.🤖 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/models.rs` around lines 179 - 192, Update the GPU hotplug refresh flow around DebugInterface::refresh_gpu to clear CW_EXP_BLK_INO before rebuilding NVIDIA inode mappings, then repopulate it from the refreshed GPU list using block_exp_inode. Ensure stale GPU IDs and missing inode entries are removed while preserving the existing NVIDIA/non-default GPU filtering.
🤖 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/core/gpu/egl.rs`:
- Around line 54-64: Trim device_array to the count returned by the second
query_devices call before iterating over it, so query_device_str only receives
populated EGLDeviceEXT handles. Update the enumeration flow around query_devices
and the subsequent for loop while preserving the existing null string-pointer
handling.
- Around line 24-45: Update GpuEnumerator::build to load libEGL, resolve the
required query functions, enumerate EGL devices, and construct the
device-to-render-node map once alongside vlk_physical_devices. Store or pass
that map into build_gpu, and change build_gpu/is_discrete_egl to reuse it
instead of loading EGL, resolving symbols, or enumerating devices for each
graphics PCI device.
In `@crates/cardwire-daemon/src/core/gpu/enumerator.rs`:
- Around line 117-132: Guard the is_discrete_egl probe in the discrete-device
calculation with available, so it is skipped when drm_node_ids fails and render
is u32::MAX. Preserve the existing Vulkan check and EGL error handling for
devices with available set to true.
In `@crates/cardwire-daemon/src/core/gpu/helpers.rs`:
- Around line 107-113: Replace the blocking std::thread::sleep in drm_node_ids
with an async wait, or ensure the helper is only called from blocking workers;
wrap GpuEnumerator::build() and enumerate in
crates/cardwire-daemon/src/models.rs lines 54-55 with
tokio::task::spawn_blocking, and apply the same change in
DebugInterface::refresh_gpu in crates/cardwire-daemon/src/interface/debug.rs
while avoiding holding pci_list or gpu_list write locks across the blocking
task.
In `@crates/cardwire-daemon/src/core/gpu/models.rs`:
- Around line 130-137: The unused _vfio() accessor should be removed from the
model implementation. Delete the _vfio method while retaining the private vfio
field, and leave is_virtual() unchanged for check_default_drm_class().
In `@crates/cardwire-daemon/src/core/gpu/vulkan.rs`:
- Around line 40-59: Update the physical-device insertion logic in the Vulkan
enumeration loop to preserve the first device for each PCI ID instead of
overwriting it. Detect duplicate keys using the existing pci_id and emit a
warning containing the collision context, while leaving the original Arc entry
unchanged.
In `@crates/cardwire-daemon/src/interface/gpu.rs`:
- Around line 304-320: Update the D-Bus handler around
external_display_connected to execute the synchronous display-state check inside
tokio::task::spawn_blocking, matching the existing patterns in the sibling
callers. Await the blocking task and preserve the current Ok(true), Err, and
Ok(false) handling, including refusal to block when the check fails or detects a
connected display.
In `@crates/cardwire-daemon/src/interface/mode.rs`:
- Around line 248-294: In the mode-switch branch, collect the available GPU
entries once and reuse that collection for the exactly-two count, the
has_offload_dgpu topology check, and the offload-card lookup. Update the logic
around has_offload_dgpu and offload_card to eliminate repeated is_available()
filters while preserving the existing validation and disappearance error
behavior.
- Around line 332-340: Update the Modes::Hybrid branch to call unblock_gpu() for
every GPU in gpu_list.values_mut(), removing the is_available() filter. Preserve
the existing error propagation so Hybrid reliably clears stale block entries
even when device availability cannot be resolved.
In `@crates/cardwire-daemon/src/interface/switcheroo.rs`:
- Around line 136-137: Update the Environment insertion in the relevant D-Bus
property getter to handle OwnedValue::try_from(env_val) without unwrap: insert
the converted value only when conversion succeeds, and gracefully skip the
property on failure so the getter cannot panic.
- Around line 57-67: Update the vendor-routing match around GpuVendor::Nvidia to
also use PciDevice::driver() when selecting environment variables. Preserve the
existing proprietary NVIDIA variables for the NVIDIA driver, but emit DRI_PRIME
and a matching VK_LOADER_DRIVERS_SELECT glob such as *nouveau* for Mesa/NVK
devices so Vulkan selects the bound driver.
In `@nix/default.nix`:
- Around line 66-69: Update both GPU-ID substitutions in the Nix build
expression—covering the pci.ids replacement and the amdgpu.ids replacement—to
use --replace-fail instead of --replace-warn, while preserving their existing
paths and replacement values.
---
Outside diff comments:
In `@crates/cardwire-daemon/src/core/gpu/models.rs`:
- Around line 139-166: Replace the adjacent boolean parameters of GpuDevice::new
with a named GpuFlags struct containing discrete, vfio, available, and
virtual_gpu, deriving or implementing Default as appropriate. Remove the
#[allow(clippy::too_many_arguments)] attribute, update GpuDevice construction to
read the struct fields, and revise every enumerator call site to use explicit
GpuFlags values such as GpuFlags { discrete, available, ..Default::default() }.
In `@crates/cardwire-daemon/src/interface/debug.rs`:
- Around line 115-127: Refactor the rebuild logic in refresh_gpu into a helper
returning a Result, and route both GpuInterface::build failures and
object_server.at failures into that result instead of returning immediately.
After releasing the relevant locks, if the rebuild failed, invoke
effective_set_mode with Hybrid and force enabled, then return the original
error. Ensure the fallback runs before any error return so the daemon restores a
consistent Hybrid state.
In `@crates/cardwire-daemon/src/models.rs`:
- Around line 179-192: Update the GPU hotplug refresh flow around
DebugInterface::refresh_gpu to clear CW_EXP_BLK_INO before rebuilding NVIDIA
inode mappings, then repopulate it from the refreshed GPU list using
block_exp_inode. Ensure stale GPU IDs and missing inode entries are removed
while preserving the existing NVIDIA/non-default GPU filtering.
🪄 Autofix
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: 9cdb8930-dedf-454d-a70f-5e78a0a60d69
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
.github/workflows/cicd.ymlCargo.tomlcrates/cardwire-daemon/Cargo.tomlcrates/cardwire-daemon/src/core/gpu/discover.rscrates/cardwire-daemon/src/core/gpu/egl.rscrates/cardwire-daemon/src/core/gpu/enumerator.rscrates/cardwire-daemon/src/core/gpu/helpers.rscrates/cardwire-daemon/src/core/gpu/mod.rscrates/cardwire-daemon/src/core/gpu/models.rscrates/cardwire-daemon/src/core/gpu/vulkan.rscrates/cardwire-daemon/src/daemon.rscrates/cardwire-daemon/src/interface/debug.rscrates/cardwire-daemon/src/interface/gpu.rscrates/cardwire-daemon/src/interface/mode.rscrates/cardwire-daemon/src/interface/switcheroo.rscrates/cardwire-daemon/src/models.rscrates/cardwire-daemon/src/tasks/monitor_display.rsflake.nixnix/default.nix
💤 Files with no reviewable changes (1)
- crates/cardwire-daemon/src/core/gpu/discover.rs
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 (2)
crates/cardwire-daemon/src/interface/debug.rs (2)
175-183: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReturn a D-Bus error when the Hybrid recovery spell fails.
If target reapplication fails and the forced
Modes::Hybridcall also fails, this branch only logs the failures.refresh_gpu()then returnsOk(()). The caller cannot detect that GPU block state may remain stale.Return
fdo::Error::Failedwhen the fallback fails.Proposed fix
if let Err(fb) = self .mode_interface .effective_set_mode(Modes::Hybrid, true) .await { - warn!("failed to fall back to hybrid mode on hotplug: {fb}"); + let message = + format!("failed to re-apply mode on hotplug: {e}; hybrid fallback failed: {fb}"); + error!("{message}"); + return Err(fdo::Error::Failed(message)); }🤖 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/debug.rs` around lines 175 - 183, Update the hotplug recovery branch in refresh_gpu so that when the fallback effective_set_mode(Modes::Hybrid, true) call fails, it returns fdo::Error::Failed instead of only logging and continuing with Ok(()). Preserve both existing warnings and the successful fallback path.
158-175: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftCast hotplug reapplication from the requested mode under one transition.
current_mode_value()returns the temporary effective mode.set_requested_mode()can persistIntegratedwhile a display override sets the effective mode toHybrid. This path then resolvesHybridagain and does not restoreIntegratedwhen the override no longer applies.This path also resolves the target before
effective_set_mode()lockstransition. Ifset_requested_mode()completes during that gap, the forced refresh can overwrite the newer effective mode.Add one
ModeInterfacemethod that lockstransition, readsrequested_mode_value(), resolves the target, and applies it before releasing the lock. Call that method here.🤖 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/debug.rs` around lines 158 - 175, Replace the current hotplug sequence in the relevant debug handler with a new ModeInterface method that holds transition across reading requested_mode_value(), resolving the display target, and applying it via effective_set_mode(). Preserve the existing fallback-to-Hybrid error behavior within that method, then call it from this path so concurrent set_requested_mode() updates cannot be overwritten.
🤖 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/debug.rs`:
- Around line 175-183: Update the hotplug recovery branch in refresh_gpu so that
when the fallback effective_set_mode(Modes::Hybrid, true) call fails, it returns
fdo::Error::Failed instead of only logging and continuing with Ok(()). Preserve
both existing warnings and the successful fallback path.
- Around line 158-175: Replace the current hotplug sequence in the relevant
debug handler with a new ModeInterface method that holds transition across
reading requested_mode_value(), resolving the display target, and applying it
via effective_set_mode(). Preserve the existing fallback-to-Hybrid error
behavior within that method, then call it from this path so concurrent
set_requested_mode() updates cannot be overwritten.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 72961942-10c6-47f8-8d65-1c2084872c5e
📒 Files selected for processing (5)
crates/cardwire-daemon/src/core/gpu/egl.rscrates/cardwire-daemon/src/core/gpu/enumerator.rscrates/cardwire-daemon/src/interface/debug.rscrates/cardwire-daemon/src/interface/mode.rscrates/cardwire-daemon/src/interface/switcheroo.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@nix/default.nix`:
- Line 66: Update the substituteInPlace target in the Nix postPatch
configuration to reference the existing pci_device.rs file under
crates/cardwire-daemon/src/core/pci instead of the absent discover.rs path,
while preserving the pci.ids replacement.
🪄 Autofix
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: 1fb15ffb-09d1-47ef-98f1-a8430172d789
📒 Files selected for processing (1)
nix/default.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/core/gpu/enumerator.rs (1)
97-109: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the
u32::MAXsentinel rune out of user-facing node data.The VFIO path and the
drm_node_idserror path storeu32::MAXincardandrenderwhile settingavailabletofalse. The GUI still renders these fields ascard4294967295 / renderD4294967295for unavailable GPUs. This is an invalid device description.Use optional node IDs, or render
Unavailablewheneveravailableisfalse.Also applies to: 118-123, 138-150
🤖 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/core/gpu/enumerator.rs` around lines 97 - 109, Remove the u32::MAX sentinel from unavailable GPU node data in the GpuDevice::new calls covering the VFIO and drm_node_ids error paths, using optional absent node IDs or the established unavailable representation instead. Ensure unavailable devices cannot render card4294967295 or renderD4294967295, while preserving valid node IDs for available GPUs.
🤖 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/core/gpu/enumerator.rs`:
- Around line 97-109: Remove the u32::MAX sentinel from unavailable GPU node
data in the GpuDevice::new calls covering the VFIO and drm_node_ids error paths,
using optional absent node IDs or the established unavailable representation
instead. Ensure unavailable devices cannot render card4294967295 or
renderD4294967295, while preserving valid node IDs for available GPUs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 77117cc2-8670-4036-aea8-f46509a0a40f
📒 Files selected for processing (3)
crates/cardwire-daemon/src/core/gpu/enumerator.rscrates/cardwire-gui/src/app.rscrates/cardwire-gui/src/ui.rs
💤 Files with no reviewable changes (1)
- crates/cardwire-gui/src/app.rs
1 participant
Description
Rework cardwired's GPU enumeration to be more consistent, more verbose, detect Discrete GPU and fix the switcheroo shim
Changed
GpuEnumerator. It uses udev for the GPU node IDs (renderD/card), use Vulkan + EGL as fallback for discrete/virtual detection.discrete,vfio,virtual_gpuandavailableflags. Unaivalable GPUs are excluded from cardwire logic but kept for listingCARDWIRE_FORCE_GPU=is only implemented in cardwire-ebpf and switcheroo shim but is only honored in smart mode, manual mode per-gpu forcing will come in a next PRTests
Laptop
Hybrid(iGPU + dGPU) without asus-mux:
Integrated using asus dgpu_disable (also showcasing auto gpu_refresh working):
Ultimate (dGPU as default, iGPU as second) with asus-mux
Desktop
Hybrid, dGPU as default and iGPU as secondary
Only dGPU (iGPU disabled in uefi)
Proxmox VM with VFIO
TODO
mode.rsto target offload dGPUs explicitly and guard against blocking primary dGPUs on desktopsmonitor_display.rsto target secondary discrete GPUs rather than!is_defaultChecklist: