From 7d9eb74853a5d4f5218508b2e3d346f9e6412847 Mon Sep 17 00:00:00 2001 From: Juho Vainio Date: Wed, 21 Oct 2026 14:00:50 +0300 Subject: [PATCH 01/14] feat(dash-tui): collect GPU/memory metrics in-process on Windows ROCMAI-564: the TUI's Windows client arm was a stub (no rocmdashd socket there yet), so Windows users saw no GPU/memory metrics at all. Add WinPerfCollector, reading the OS's own `GPU Engine`/`GPU Adapter Memory` performance counters via `Get-Counter` (the same WDDM/DXGK-backed source Task Manager's GPU tab uses) plus `Win32_VideoController` for adapter inventory/driver version. No AMD tool required. Mirrors amd_smi.rs's subprocess + pure-parse shape. client.rs's Windows connect_and_run now builds Snapshots in-process each tick from HostCollector (already cross-platform) + WinPerfCollector, feeding the same ClientMsg sequence the Unix socket arm produces, so the app loop doesn't need to know which platform it's on. Signed-off-by: Juho Vainio --- Cargo.lock | 1 + crates/rocm-dash-collectors/src/lib.rs | 1 + crates/rocm-dash-collectors/src/winperf.rs | 227 +++++++++++++++++++++ crates/rocm-dash-tui/Cargo.toml | 4 + crates/rocm-dash-tui/src/client.rs | 54 ++++- 5 files changed, 280 insertions(+), 7 deletions(-) create mode 100644 crates/rocm-dash-collectors/src/winperf.rs diff --git a/Cargo.lock b/Cargo.lock index a46a744a0..86069f015 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3777,6 +3777,7 @@ dependencies = [ "ratatui", "reqwest 0.13.4", "rig-core", + "rocm-dash-collectors", "rocm-dash-core", "rocm-deps", "serde", diff --git a/crates/rocm-dash-collectors/src/lib.rs b/crates/rocm-dash-collectors/src/lib.rs index f5b7bcd8f..6ce97985c 100644 --- a/crates/rocm-dash-collectors/src/lib.rs +++ b/crates/rocm-dash-collectors/src/lib.rs @@ -20,3 +20,4 @@ pub mod proc_scan; pub mod sysfs; pub mod vllm_log; pub mod vllm_prom; +pub mod winperf; diff --git a/crates/rocm-dash-collectors/src/winperf.rs b/crates/rocm-dash-collectors/src/winperf.rs new file mode 100644 index 000000000..3d301f8b3 --- /dev/null +++ b/crates/rocm-dash-collectors/src/winperf.rs @@ -0,0 +1,227 @@ +// Copyright © Advanced Micro Devices, Inc., or its affiliates. +// +// SPDX-License-Identifier: MIT + +//! Live Windows GPU metrics via OS performance counters, no AMD tool needed. +//! +//! Reads the `GPU Engine`/`GPU Adapter Memory` categories — the same +//! WDDM/DXGK-backed source Task Manager's GPU tab reads. Mirrors +//! `amd_smi.rs`'s subprocess + pure-parse shape, shelling out to +//! `powershell.exe` instead of `amd-smi`. Not `#[cfg(windows)]`-gated: the +//! shellout fails cleanly (`powershell.exe` not found) on other platforms, +//! same as `amd_smi.rs` when `amd-smi` isn't installed. + +use std::collections::BTreeMap; +use std::io; +use std::time::Duration; + +use rocm_dash_core::metrics::{GpuMetrics, GpuSystemInfo}; +use tokio::process::Command; +use tokio::time::timeout; + +const RUN_TIMEOUT: Duration = Duration::from_secs(5); + +/// `kindkeyvalue[value2]` lines: per-engine utilization, +/// per-adapter dedicated VRAM usage, and a best-effort adapter inventory +/// (name/RAM/driver) for `GpuSystemInfo`. One process spawn for all three. +const WINPERF_SCRIPT: &str = r#" +$ErrorActionPreference = 'SilentlyContinue' +(Get-Counter '\GPU Engine(*)\Utilization Percentage' -ErrorAction SilentlyContinue).CounterSamples | + ForEach-Object { "UTIL`t$($_.InstanceName)`t$($_.CookedValue)" } +(Get-Counter '\GPU Adapter Memory(*)\Dedicated Usage' -ErrorAction SilentlyContinue).CounterSamples | + ForEach-Object { "VRAM`t$($_.InstanceName)`t$($_.CookedValue)" } +Get-CimInstance -ClassName Win32_VideoController -Property Name,AdapterRAM,DriverVersion -ErrorAction SilentlyContinue | + ForEach-Object { "GPU`t$($_.Name)`t$($_.AdapterRAM)`t$($_.DriverVersion)" } +"#; + +#[derive(Debug, Default)] +pub struct WinPerfCollector; + +impl WinPerfCollector { + pub const fn new() -> Self { + Self + } + + pub async fn metrics(&self) -> io::Result> { + let out = Self::run_script().await?; + Ok(parse_metrics(&out)) + } + + /// Best-effort; never fails the caller — an unreachable perf-counter + /// provider just yields an empty/default `GpuSystemInfo`. + pub async fn system_info(&self) -> GpuSystemInfo { + Self::run_script() + .await + .map(|out| parse_system_info(&out)) + .unwrap_or_default() + } + + async fn run_script() -> io::Result { + let out = timeout( + RUN_TIMEOUT, + Command::new("powershell.exe") + .args(["-NoProfile", "-NonInteractive", "-Command", WINPERF_SCRIPT]) + .output(), + ) + .await + .map_err(|_| io::Error::other("powershell Get-Counter query timed out"))??; + if !out.status.success() { + return Err(io::Error::other(format!( + "powershell Get-Counter exited {}: {}", + out.status, + String::from_utf8_lossy(&out.stderr).trim() + ))); + } + Ok(String::from_utf8_lossy(&out.stdout).into_owned()) + } +} + +/// `GPU Engine`/`GPU Adapter Memory` instance names are per-engine +/// (`luid_0x..._0x..._phys_0_eng_0_engtype_3d`) or per-adapter +/// (`luid_0x..._0x..._phys_0`); the LUID+phys prefix identifies the device. +fn device_key(instance: &str) -> &str { + instance.split("_phys_").next().unwrap_or(instance) +} + +/// Builds one `GpuMetrics` per distinct adapter LUID seen in the counter +/// output. Utilization is the max across that adapter's engines (3D/copy/ +/// video/etc.) — the same "overall GPU" reading Task Manager shows. +/// +/// `vram_total_mb` is only filled in when there is exactly one adapter: with +/// more than one, correlating a `GPU Adapter Memory` LUID to a specific +/// `Win32_VideoController` entry isn't reliably possible without vendor- +/// specific APIs, so it's left at 0 (every TUI consumer already guards +/// `vram_total_mb > 0` before dividing). +/// ponytail: single-adapter case (the realistic target: an integrated AMD +/// APU) gets an exact total; multi-GPU hosts degrade to 0 instead of a wrong +/// number. Upgrade path: WMI `MSPower_DeviceEnable`/DXGI LUID correlation if +/// multi-GPU Windows hosts become a real target. +fn parse_metrics(raw: &str) -> Vec { + let mut util: BTreeMap<&str, f32> = BTreeMap::new(); + let mut vram_mb: BTreeMap<&str, u64> = BTreeMap::new(); + let mut order: Vec<&str> = Vec::new(); + + for line in raw.lines() { + let mut parts = line.splitn(3, '\t'); + let (Some(kind), Some(instance), Some(value)) = (parts.next(), parts.next(), parts.next()) + else { + continue; + }; + if kind != "UTIL" && kind != "VRAM" { + continue; // GPU inventory lines are handled separately by parse_adapters. + } + let key = device_key(instance); + if !util.contains_key(key) && !vram_mb.contains_key(key) { + order.push(key); + } + match kind { + "UTIL" => { + let pct: f32 = value.trim().parse().unwrap_or(0.0); + let entry = util.entry(key).or_insert(0.0); + if pct > *entry { + *entry = pct; + } + } + "VRAM" => { + let bytes: f64 = value.trim().parse().unwrap_or(0.0); + *vram_mb.entry(key).or_insert(0) += (bytes / (1024.0 * 1024.0)) as u64; + } + _ => unreachable!(), + } + } + + let adapters = parse_adapters(raw); + let single_adapter_ram_mb = match (order.len(), adapters.len()) { + (1, 1) => adapters[0].ram_bytes / (1024 * 1024), + _ => 0, + }; + + order + .into_iter() + .enumerate() + .map(|(i, key)| GpuMetrics { + device_id: format!("win-gpu-{i}"), + vram_used_mb: vram_mb.get(key).copied().unwrap_or(0), + vram_total_mb: single_adapter_ram_mb, + gpu_utilization_pct: util.get(key).copied().unwrap_or(0.0).min(100.0), + temperature_c: 0.0, + power_w: 0.0, + clock_mhz: None, + }) + .collect() +} + +struct Adapter { + name: String, + ram_bytes: u64, + driver_version: String, +} + +fn parse_adapters(raw: &str) -> Vec { + raw.lines() + .filter_map(|line| { + let mut parts = line.splitn(4, '\t'); + match (parts.next(), parts.next(), parts.next(), parts.next()) { + (Some("GPU"), Some(name), Some(ram), Some(driver)) => Some(Adapter { + name: name.to_string(), + ram_bytes: ram.trim().parse().unwrap_or(0), + driver_version: driver.trim().to_string(), + }), + _ => None, + } + }) + .collect() +} + +fn parse_system_info(raw: &str) -> GpuSystemInfo { + let adapters = parse_adapters(raw); + GpuSystemInfo { + gpu_model: adapters.first().map(|a| a.name.clone()).unwrap_or_default(), + driver_version: adapters.first().map(|a| a.driver_version.clone()), + physical_gpu_count: adapters.len() as u32, + logical_gpu_count: adapters.len() as u32, + ..Default::default() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + const SAMPLE: &str = "UTIL\tluid_0x00000000_0x0000b3a4_phys_0_eng_0_engtype_3d\t12.5\nUTIL\tluid_0x00000000_0x0000b3a4_phys_0_eng_1_engtype_video\t4.0\nVRAM\tluid_0x00000000_0x0000b3a4_phys_0\t8589934592\nGPU\tAMD Radeon(TM) Graphics\t17179869184\t31.0.21021.1000"; + + #[test] + fn parses_single_adapter_metrics_with_total_ram() { + let metrics = parse_metrics(SAMPLE); + assert_eq!(metrics.len(), 1); + let gpu = &metrics[0]; + // Max across the two engine readings, not the sum. + assert!((gpu.gpu_utilization_pct - 12.5).abs() < 0.01); + assert_eq!(gpu.vram_used_mb, 8192); + assert_eq!(gpu.vram_total_mb, 16384); + } + + #[test] + fn parses_system_info_from_adapter_inventory() { + let info = parse_system_info(SAMPLE); + assert_eq!(info.gpu_model, "AMD Radeon(TM) Graphics"); + assert_eq!(info.driver_version.as_deref(), Some("31.0.21021.1000")); + assert_eq!(info.physical_gpu_count, 1); + } + + #[test] + fn multiple_adapters_leave_vram_total_zero() { + let two_adapters = format!( + "{SAMPLE}\nUTIL\tluid_0x00000000_0x0000cafe_phys_0_eng_0_engtype_3d\t1.0\nVRAM\tluid_0x00000000_0x0000cafe_phys_0\t1048576\nGPU\tOther GPU\t4294967296\t1.0" + ); + let metrics = parse_metrics(&two_adapters); + assert_eq!(metrics.len(), 2); + assert!(metrics.iter().all(|g| g.vram_total_mb == 0)); + } + + #[test] + fn empty_output_yields_no_metrics() { + assert!(parse_metrics("").is_empty()); + assert_eq!(parse_system_info("").physical_gpu_count, 0); + } +} diff --git a/crates/rocm-dash-tui/Cargo.toml b/crates/rocm-dash-tui/Cargo.toml index 251b33251..04d580759 100644 --- a/crates/rocm-dash-tui/Cargo.toml +++ b/crates/rocm-dash-tui/Cargo.toml @@ -16,6 +16,10 @@ path = "src/lib.rs" [dependencies] rocm-dash-core = { path = "../rocm-dash-core" } +# Windows-only: the in-process metrics ticker in `client.rs` reuses +# `HostCollector`/`WinPerfCollector` instead of a socket (no rocmdashd on +# Windows yet). Builds on every platform like the rest of the crate. +rocm-dash-collectors = { path = "../rocm-dash-collectors" } rocm-deps = { path = "../rocm-deps" } # Unified dashboard TUI base: ratatui 0.30 + crossterm 0.28. This is the only # ratatui in the workspace — the `rocm` binary no longer carries a (dead) 0.29 diff --git a/crates/rocm-dash-tui/src/client.rs b/crates/rocm-dash-tui/src/client.rs index bd5e50b3f..34b4bbc83 100644 --- a/crates/rocm-dash-tui/src/client.rs +++ b/crates/rocm-dash-tui/src/client.rs @@ -178,12 +178,52 @@ async fn connect_and_run(connect: &str, tx: UnboundedSender) -> anyho Ok(()) } +/// Windows has no rocmdashd socket yet, so this builds `Snapshot`s in-process +/// instead of connecting anywhere: `HostCollector` (already cross-platform) +/// for CPU/memory, `WinPerfCollector` (OS perf counters, no AMD tool needed) +/// for GPU. Same `ClientMsg` sequence the unix arm produces +/// (`Connected` then a stream of `Event::Snapshot`), so the app loop doesn't +/// need to know which platform it's on. #[cfg(windows)] -// Mirrors the signature of the `cfg(unix)` arm, which callers `.await`; -// dropping `async` here would not compile on Windows. -#[allow(clippy::unused_async)] -async fn connect_and_run(connect: &str, _tx: UnboundedSender) -> anyhow::Result<()> { - Err(anyhow!( - "rocm-dash TUI requires Unix domain sockets; not supported on Windows yet (connect={connect})" - )) +async fn connect_and_run(connect: &str, tx: UnboundedSender) -> anyhow::Result<()> { + use rocm_dash_collectors::host::HostCollector; + use rocm_dash_collectors::winperf::WinPerfCollector; + use rocm_dash_core::metrics::Snapshot; + + info!( + connect, + "windows: collecting metrics in-process (no daemon socket)" + ); + let _ = tx.send(ClientMsg::Connected { + host: "localhost".into(), + daemon_version: format!("in-process/{}", env!("CARGO_PKG_VERSION")), + }); + + let mut host = HostCollector::new(); + let gpu = WinPerfCollector::new(); + // Mirrors the daemon's `gpu_tick` cadence (rocm_dash_daemon::runner::RunnerOptions::default()). + let mut ticker = tokio::time::interval(std::time::Duration::from_secs(1)); + + loop { + ticker.tick().await; + let gpus = gpu.metrics().await.unwrap_or_else(|e| { + warn!(error = %e, "windows GPU perf-counter query failed"); + Vec::new() + }); + let snapshot = Snapshot { + timestamp: chrono::Utc::now(), + host: host.tick(), + gpus, + gpu_system_info: Some(gpu.system_info().await), + instances: Vec::new(), + warnings: Vec::new(), + }; + if tx + .send(ClientMsg::Event(Box::new(Event::Snapshot(snapshot)))) + .is_err() + { + break; + } + } + Ok(()) } From 8c070ad2c11a69dd151ff83fdbceb67e28a78e39 Mon Sep 17 00:00:00 2001 From: Juho Vainio Date: Wed, 21 Oct 2026 14:21:45 +0300 Subject: [PATCH 02/14] chore(xtask): allowlist rocm-dash-tui -> rocm-dash-collectors edge Required by ROCMAI-564's in-process Windows metrics ticker in crates/rocm-dash-tui/src/client.rs, which reuses HostCollector and WinPerfCollector directly instead of going through the daemon socket. Signed-off-by: Juho Vainio --- xtask/src/crate_edges.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/xtask/src/crate_edges.rs b/xtask/src/crate_edges.rs index 8c931ad66..fe7b1ec19 100644 --- a/xtask/src/crate_edges.rs +++ b/xtask/src/crate_edges.rs @@ -57,6 +57,7 @@ const ALLOWLIST: &[(&str, &str)] = &[ ("rocm-dash-collectors", "rocm-dash-core"), ("rocm-dash-daemon", "rocm-dash-core"), ("rocm-dash-daemon", "rocm-dash-collectors"), + ("rocm-dash-tui", "rocm-dash-collectors"), ("rocm-dash-tui", "rocm-dash-core"), ("rocm-dash-tui", "rocm-deps"), ("rocm-engine-lemonade", "rocm-core"), From 6a8184bd01a0ad70b225b68d24db043c56ceac59 Mon Sep 17 00:00:00 2001 From: Juho Vainio Date: Wed, 21 Oct 2026 16:01:59 +0300 Subject: [PATCH 03/14] fix(dash-collectors): strip pid prefix when keying GPU Engine counters GPU Engine perf-counter instances are per-process on Windows 10/11 (pid_NNNN_luid_..._phys_0_eng_...), but GPU Adapter Memory instances are per-adapter only (luid_..._phys_0, no pid). device_key() keyed both verbatim, so every process using the GPU produced its own phantom near-zero device instead of aggregating onto the real adapter, which is why real hardware showed "Unknown GPU" with all-zero metrics despite `examine` detecting the GPU correctly. Confirmed against live Get-Counter output from a Strix Point laptop. Signed-off-by: Juho Vainio --- crates/rocm-dash-collectors/src/winperf.rs | 27 ++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/crates/rocm-dash-collectors/src/winperf.rs b/crates/rocm-dash-collectors/src/winperf.rs index 3d301f8b3..10f764cd4 100644 --- a/crates/rocm-dash-collectors/src/winperf.rs +++ b/crates/rocm-dash-collectors/src/winperf.rs @@ -76,10 +76,14 @@ impl WinPerfCollector { } } -/// `GPU Engine`/`GPU Adapter Memory` instance names are per-engine -/// (`luid_0x..._0x..._phys_0_eng_0_engtype_3d`) or per-adapter -/// (`luid_0x..._0x..._phys_0`); the LUID+phys prefix identifies the device. +/// `GPU Engine` instances are per-process-per-engine +/// (`pid_1234_luid_0x..._0x..._phys_0_eng_0_engtype_3d`); `GPU Adapter +/// Memory` instances are per-adapter only (`luid_0x..._0x..._phys_0`, no +/// pid). Strip any `pid_NNNN_` prefix before keying so both counter +/// families collapse onto the same per-adapter key — otherwise every +/// process using the GPU looks like a distinct phantom device. fn device_key(instance: &str) -> &str { + let instance = instance.find("luid_").map_or(instance, |i| &instance[i..]); instance.split("_phys_").next().unwrap_or(instance) } @@ -188,7 +192,9 @@ fn parse_system_info(raw: &str) -> GpuSystemInfo { mod tests { use super::*; - const SAMPLE: &str = "UTIL\tluid_0x00000000_0x0000b3a4_phys_0_eng_0_engtype_3d\t12.5\nUTIL\tluid_0x00000000_0x0000b3a4_phys_0_eng_1_engtype_video\t4.0\nVRAM\tluid_0x00000000_0x0000b3a4_phys_0\t8589934592\nGPU\tAMD Radeon(TM) Graphics\t17179869184\t31.0.21021.1000"; + // Real `GPU Engine` instances are per-process (`pid_NNNN_luid_...`); mix + // in a pid prefix here so the tests actually exercise that shape. + const SAMPLE: &str = "UTIL\tpid_1234_luid_0x00000000_0x0000b3a4_phys_0_eng_0_engtype_3d\t12.5\nUTIL\tpid_1234_luid_0x00000000_0x0000b3a4_phys_0_eng_1_engtype_video\t4.0\nVRAM\tluid_0x00000000_0x0000b3a4_phys_0\t8589934592\nGPU\tAMD Radeon(TM) Graphics\t17179869184\t31.0.21021.1000"; #[test] fn parses_single_adapter_metrics_with_total_ram() { @@ -219,6 +225,19 @@ mod tests { assert!(metrics.iter().all(|g| g.vram_total_mb == 0)); } + #[test] + fn multiple_processes_on_one_adapter_collapse_to_one_device() { + // Two different pids hitting the same adapter's engines, plus a + // single per-adapter VRAM sample (as Windows actually reports it). + let sample = "UTIL\tpid_111_luid_0x00000000_0x0002ff2a_phys_0_eng_0_engtype_3d\t0.682\nUTIL\tpid_222_luid_0x00000000_0x0002ff2a_phys_0_eng_0_engtype_3d\t0.514\nVRAM\tluid_0x00000000_0x0002ff2a_phys_0\t902787072\nGPU\tAMD Radeon 780M Graphics\t1073741824\t32.0.31041.1004"; + let metrics = parse_metrics(sample); + assert_eq!(metrics.len(), 1); + let gpu = &metrics[0]; + assert!((gpu.gpu_utilization_pct - 0.682).abs() < 0.001); + assert_eq!(gpu.vram_used_mb, 902_787_072 / (1024 * 1024)); + assert_eq!(gpu.vram_total_mb, 1_073_741_824 / (1024 * 1024)); + } + #[test] fn empty_output_yields_no_metrics() { assert!(parse_metrics("").is_empty()); From 2adfdbf22a374b805962038030a10e6a4da6965c Mon Sep 17 00:00:00 2001 From: Juho Vainio Date: Wed, 21 Oct 2026 17:05:31 +0300 Subject: [PATCH 04/14] fix(dash-tui): tolerate slow powershell spawn, halve GPU perf-counter calls Real hardware logs (Strix Halo) showed every tick timing out: "powershell Get-Counter query timed out" repeating for 20+ minutes straight, even though the identical script runs near-instantly when invoked manually. The bottleneck is powershell.exe child-process spawn latency, not the Get-Counter query itself, and it consistently exceeds the old 5s budget on real hardware. Bump RUN_TIMEOUT to 15s, and collapse the two independent metrics()/ system_info() spawns (each re-running the identical script) into one snapshot() call so each tick only pays the spawn cost once. Signed-off-by: Juho Vainio --- crates/rocm-dash-collectors/src/winperf.rs | 25 +++++++++++----------- crates/rocm-dash-tui/src/client.rs | 8 +++---- 2 files changed, 17 insertions(+), 16 deletions(-) diff --git a/crates/rocm-dash-collectors/src/winperf.rs b/crates/rocm-dash-collectors/src/winperf.rs index 10f764cd4..f26c986d4 100644 --- a/crates/rocm-dash-collectors/src/winperf.rs +++ b/crates/rocm-dash-collectors/src/winperf.rs @@ -19,7 +19,13 @@ use rocm_dash_core::metrics::{GpuMetrics, GpuSystemInfo}; use tokio::process::Command; use tokio::time::timeout; -const RUN_TIMEOUT: Duration = Duration::from_secs(5); +// 5s was too tight: on real hardware `powershell.exe` child-process spawn +// (not the Get-Counter query itself, which returns near-instantly once +// running) can take well over 5s — observed consistently timing out every +// tick on a Strix Halo laptop while the identical script ran fine +// interactively. 15s gives real-world spawn latency room without masking an +// actually-hung process forever. +const RUN_TIMEOUT: Duration = Duration::from_secs(15); /// `kindkeyvalue[value2]` lines: per-engine utilization, /// per-adapter dedicated VRAM usage, and a best-effort adapter inventory @@ -42,18 +48,13 @@ impl WinPerfCollector { Self } - pub async fn metrics(&self) -> io::Result> { + /// One `powershell.exe` spawn covers both metrics and system info (the + /// script already emits both sections) — calling `metrics()` and + /// `system_info()` separately each tick would shell out twice for the + /// same output, doubling exposure to spawn latency for nothing. + pub async fn snapshot(&self) -> io::Result<(Vec, GpuSystemInfo)> { let out = Self::run_script().await?; - Ok(parse_metrics(&out)) - } - - /// Best-effort; never fails the caller — an unreachable perf-counter - /// provider just yields an empty/default `GpuSystemInfo`. - pub async fn system_info(&self) -> GpuSystemInfo { - Self::run_script() - .await - .map(|out| parse_system_info(&out)) - .unwrap_or_default() + Ok((parse_metrics(&out), parse_system_info(&out))) } async fn run_script() -> io::Result { diff --git a/crates/rocm-dash-tui/src/client.rs b/crates/rocm-dash-tui/src/client.rs index 34b4bbc83..4659b3f62 100644 --- a/crates/rocm-dash-tui/src/client.rs +++ b/crates/rocm-dash-tui/src/client.rs @@ -188,7 +188,7 @@ async fn connect_and_run(connect: &str, tx: UnboundedSender) -> anyho async fn connect_and_run(connect: &str, tx: UnboundedSender) -> anyhow::Result<()> { use rocm_dash_collectors::host::HostCollector; use rocm_dash_collectors::winperf::WinPerfCollector; - use rocm_dash_core::metrics::Snapshot; + use rocm_dash_core::metrics::{GpuSystemInfo, Snapshot}; info!( connect, @@ -206,15 +206,15 @@ async fn connect_and_run(connect: &str, tx: UnboundedSender) -> anyho loop { ticker.tick().await; - let gpus = gpu.metrics().await.unwrap_or_else(|e| { + let (gpus, gpu_system_info) = gpu.snapshot().await.unwrap_or_else(|e| { warn!(error = %e, "windows GPU perf-counter query failed"); - Vec::new() + (Vec::new(), GpuSystemInfo::default()) }); let snapshot = Snapshot { timestamp: chrono::Utc::now(), host: host.tick(), gpus, - gpu_system_info: Some(gpu.system_info().await), + gpu_system_info: Some(gpu_system_info), instances: Vec::new(), warnings: Vec::new(), }; From d2592dd5c3cdc5762e4417b5ff7aff3551934c85 Mon Sep 17 00:00:00 2001 From: Juho Vainio Date: Thu, 22 Oct 2026 09:17:09 +0300 Subject: [PATCH 05/14] fix(dash-collectors,dash-tui): collapse phantom GPU LUIDs, fix VRAM% div-by-zero winperf.rs reported one GpuMetrics per distinct GPU Adapter Memory LUID, but Windows can surface multiple stale/phantom LUIDs for one physical adapter (observed: 3 LUIDs, 2 permanently zero-VRAM, for a single AMD Radeon 780M). When Win32_VideoController confirms there's exactly one real adapter, collapse all LUIDs into one device instead. home.rs's hero VRAM percentage used total.max(1) to dodge division by zero, which silently produced a nonsensical inflated percentage (used_mb * 100) whenever every GPU's vram_total_mb was 0 instead of indicating unknown/N/A. Guard on total > 0 like widgets.rs and dock.rs already do. Signed-off-by: Juho Vainio --- crates/rocm-dash-collectors/src/winperf.rs | 54 ++++++++++++++++++---- crates/rocm-dash-tui/src/ui/tabs/home.rs | 41 ++++++++++++++-- 2 files changed, 81 insertions(+), 14 deletions(-) diff --git a/crates/rocm-dash-collectors/src/winperf.rs b/crates/rocm-dash-collectors/src/winperf.rs index f26c986d4..cdbaa1753 100644 --- a/crates/rocm-dash-collectors/src/winperf.rs +++ b/crates/rocm-dash-collectors/src/winperf.rs @@ -92,11 +92,18 @@ fn device_key(instance: &str) -> &str { /// output. Utilization is the max across that adapter's engines (3D/copy/ /// video/etc.) — the same "overall GPU" reading Task Manager shows. /// -/// `vram_total_mb` is only filled in when there is exactly one adapter: with -/// more than one, correlating a `GPU Adapter Memory` LUID to a specific -/// `Win32_VideoController` entry isn't reliably possible without vendor- -/// specific APIs, so it's left at 0 (every TUI consumer already guards -/// `vram_total_mb > 0` before dividing). +/// Windows can report more than one `GPU Adapter Memory` LUID for a single +/// physical GPU (stale instances left behind by a display-mode switch, or +/// separate WDDM engine nodes) — on an integrated APU this shows up as a +/// "3 GPUs" readout with two phantom zero-VRAM devices. When +/// `Win32_VideoController` says there's exactly one real adapter, collapse +/// every LUID into that one device instead of reporting one per LUID: sum +/// VRAM used (a phantom LUID contributes 0), take the max utilization. +/// +/// With more than one real adapter, correlating a `GPU Adapter Memory` LUID +/// to a specific `Win32_VideoController` entry isn't reliably possible +/// without vendor-specific APIs, so `vram_total_mb` is left at 0 (every TUI +/// consumer already guards `vram_total_mb > 0` before dividing). /// ponytail: single-adapter case (the realistic target: an integrated AMD /// APU) gets an exact total; multi-GPU hosts degrade to 0 instead of a wrong /// number. Upgrade path: WMI `MSPower_DeviceEnable`/DXGI LUID correlation if @@ -136,10 +143,22 @@ fn parse_metrics(raw: &str) -> Vec { } let adapters = parse_adapters(raw); - let single_adapter_ram_mb = match (order.len(), adapters.len()) { - (1, 1) => adapters[0].ram_bytes / (1024 * 1024), - _ => 0, - }; + if let [adapter] = adapters.as_slice() { + if order.is_empty() { + return Vec::new(); + } + let vram_used_mb: u64 = vram_mb.values().sum(); + let gpu_utilization_pct = util.values().copied().fold(0.0f32, f32::max).min(100.0); + return vec![GpuMetrics { + device_id: "win-gpu-0".to_string(), + vram_used_mb, + vram_total_mb: adapter.ram_bytes / (1024 * 1024), + gpu_utilization_pct, + temperature_c: 0.0, + power_w: 0.0, + clock_mhz: None, + }]; + } order .into_iter() @@ -147,7 +166,7 @@ fn parse_metrics(raw: &str) -> Vec { .map(|(i, key)| GpuMetrics { device_id: format!("win-gpu-{i}"), vram_used_mb: vram_mb.get(key).copied().unwrap_or(0), - vram_total_mb: single_adapter_ram_mb, + vram_total_mb: 0, gpu_utilization_pct: util.get(key).copied().unwrap_or(0.0).min(100.0), temperature_c: 0.0, power_w: 0.0, @@ -239,6 +258,21 @@ mod tests { assert_eq!(gpu.vram_total_mb, 1_073_741_824 / (1024 * 1024)); } + #[test] + fn phantom_adapter_memory_luids_collapse_to_one_device_when_only_one_real_adapter() { + // Real-world capture from an AMD Radeon 780M laptop: Windows reports + // 3 distinct `GPU Adapter Memory` LUIDs (2 of them permanently + // zero-VRAM phantoms) for the single physical adapter + // `Win32_VideoController` actually lists. + let sample = "UTIL\tpid_1234_luid_0x00000000_0x0002ff2a_phys_0_eng_0_engtype_3d\t0.5\nVRAM\tluid_0x00000000_0x0002ff2a_phys_0\t897261568\nVRAM\tluid_0x00000000_0x000325a2_phys_0\t0\nVRAM\tluid_0x00000000_0x000325f9_phys_0\t0\nGPU\tAMD Radeon 780M Graphics\t1073741824\t32.0.31041.1004"; + let metrics = parse_metrics(sample); + assert_eq!(metrics.len(), 1); + let gpu = &metrics[0]; + assert!((gpu.gpu_utilization_pct - 0.5).abs() < 0.001); + assert_eq!(gpu.vram_used_mb, 897_261_568 / (1024 * 1024)); + assert_eq!(gpu.vram_total_mb, 1_073_741_824 / (1024 * 1024)); + } + #[test] fn empty_output_yields_no_metrics() { assert!(parse_metrics("").is_empty()); diff --git a/crates/rocm-dash-tui/src/ui/tabs/home.rs b/crates/rocm-dash-tui/src/ui/tabs/home.rs index a3d62bf3a..0c97ab164 100644 --- a/crates/rocm-dash-tui/src/ui/tabs/home.rs +++ b/crates/rocm-dash-tui/src/ui/tabs/home.rs @@ -350,8 +350,12 @@ fn draw_hero_right(f: &mut Frame, area: Rect, state: &AppState, theme: &Theme) { let latest = state.latest.as_ref(); let vram = latest.map_or(0.0, |s| { let used: u64 = s.gpus.iter().map(|g| g.vram_used_mb).sum(); - let total: u64 = s.gpus.iter().map(|g| g.vram_total_mb).sum::().max(1); - used as f64 / total as f64 * 100.0 + let total: u64 = s.gpus.iter().map(|g| g.vram_total_mb).sum(); + if total > 0 { + used as f64 / total as f64 * 100.0 + } else { + 0.0 + } }); let temp = latest.map_or(0.0, |s| { s.gpus @@ -369,8 +373,12 @@ fn draw_hero_right(f: &mut Frame, area: Rect, state: &AppState, theme: &Theme) { &format::pct(vram as f32), &history(state, |s| { let used: u64 = s.gpus.iter().map(|g| g.vram_used_mb).sum(); - let total: u64 = s.gpus.iter().map(|g| g.vram_total_mb).sum::().max(1); - used as f64 / total as f64 * 100.0 + let total: u64 = s.gpus.iter().map(|g| g.vram_total_mb).sum(); + if total > 0 { + used as f64 / total as f64 * 100.0 + } else { + 0.0 + } }), false, theme, @@ -681,6 +689,31 @@ mod tests { assert_eq!(t2, "Node throughput · Unknown GPU"); } + #[test] + fn hero_vram_pct_is_zero_not_garbage_when_total_is_unknown() { + // vram_total_mb == 0 is a legitimate state (e.g. the Windows + // collector can't correlate a multi-adapter host to specific VRAM + // sizes) — must render "0.0%", not `used / 1 * 100`. + let mut s = AppState::new("t".into(), "default-dark".into()); + s.active_tab = ActiveTab::Home; + s.latest = Some(Snapshot { + host: SystemMetrics::default(), + gpus: vec![GpuMetrics { + device_id: "GPU0".into(), + vram_used_mb: 808, + vram_total_mb: 0, + gpu_utilization_pct: 0.5, + temperature_c: 0.0, + power_w: 0.0, + clock_mhz: None, + }], + ..Default::default() + }); + let out = render(&s, 160, 30); + assert!(out.contains("0.0%"), "expected 0.0% VRAM: {out:?}"); + assert!(!out.contains("80800"), "garbage VRAM pct leaked: {out:?}"); + } + #[test] fn home_renders_placeholder_when_empty() { let mut s = AppState::new("t".into(), "default-dark".into()); From 957ac816795590dc33597198e40f4d247bedea1c Mon Sep 17 00:00:00 2001 From: Juho Vainio Date: Thu, 22 Oct 2026 10:03:36 +0300 Subject: [PATCH 06/14] feat(dash-collectors): populate GPU temperature/power/clock via ADLX on Windows winperf.rs previously hardcoded temperature_c/power_w/clock_mhz to 0.0/0.0/None since OS perf counters don't expose them. Add an ADLX (AMD Device Library eXtra) glue path, gated to Windows only, that reads these three metrics when the AMD driver's bundled amdadlx64.dll is present, falling back to the prior defaults on any failure (no driver, non-AMD GPU, unsupported metric) so the snapshot never errors out. Signed-off-by: Juho Vainio --- Cargo.lock | 21 ++++ MANIFEST.md | 2 + THIRD_PARTY_NOTICES.txt | 51 ++++++++++ crates/rocm-dash-collectors/Cargo.toml | 7 ++ crates/rocm-dash-collectors/src/winperf.rs | 106 ++++++++++++++++++--- 5 files changed, 175 insertions(+), 12 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 86069f015..03c9b3967 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -23,6 +23,16 @@ version = "2.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "320119579fcad9c21884f5c4861d16174d0e06250625266f50fe6898340abefa" +[[package]] +name = "adlx" +version = "0.0.0-alpha.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fae45a2d1c9dcfb7fc8bb3069dc821d38218b36d6e1d2ebbd7d80a4751c3aef6" +dependencies = [ + "anyhow", + "libloading", +] + [[package]] name = "aes" version = "0.8.4" @@ -2303,6 +2313,16 @@ version = "0.2.186" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "68ab91017fe16c622486840e4c83c9a37afeff978bd239b5293d61ece587de66" +[[package]] +name = "libloading" +version = "0.8.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d7c4b02199fee7c5d21a5ae7d8cfa79a6ef5bb2fc834d6e9058e89c825efdc55" +dependencies = [ + "cfg-if", + "windows-link", +] + [[package]] name = "libm" version = "0.2.16" @@ -3710,6 +3730,7 @@ dependencies = [ name = "rocm-dash-collectors" version = "0.1.0" dependencies = [ + "adlx", "bollard", "chrono", "csv", diff --git a/MANIFEST.md b/MANIFEST.md index 4fdb8bf83..3b2fc107a 100644 --- a/MANIFEST.md +++ b/MANIFEST.md @@ -36,6 +36,7 @@ repository. | Inflector | 0.11.4 | BSD-2-Clause | | addr2line | 0.25.1 | Apache-2.0 OR MIT | | adler2 | 2.0.1 | 0BSD OR MIT OR Apache-2.0 | +| adlx | 0.0.0-alpha.1 | MIT | | aes | 0.8.4 | MIT OR Apache-2.0 | | aho-corasick | 1.1.4 | Unlicense OR MIT | | allocator-api2 | 0.2.21 | MIT OR Apache-2.0 | @@ -258,6 +259,7 @@ repository. | lab | 0.11.0 | MIT | | lazy_static | 1.5.0 | MIT OR Apache-2.0 | | libc | 0.2.186 | MIT OR Apache-2.0 | +| libloading | 0.8.9 | ISC | | libm | 0.2.16 | MIT | | libredox | 0.1.17 | MIT | | line-clipping | 0.3.7 | MIT OR Apache-2.0 | diff --git a/THIRD_PARTY_NOTICES.txt b/THIRD_PARTY_NOTICES.txt index 5da394269..08cc3b414 100644 --- a/THIRD_PARTY_NOTICES.txt +++ b/THIRD_PARTY_NOTICES.txt @@ -8937,6 +8937,27 @@ OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +================================================================================ +ISC License +================================================================================ + +The following component(s) are licensed under ISC License: + - libloading 0.8.9 + +Copyright © 2015, Simonas Kazlauskas + +Permission to use, copy, modify, and/or distribute this software for any purpose with or without +fee is hereby granted, provided that the above copyright notice and this permission notice appear +in all copies. + +THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES WITH REGARD TO THIS +SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE +AUTHOR BE LIABLE FOR ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES +WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, +NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE OF +THIS SOFTWARE. + + ================================================================================ ISC License ================================================================================ @@ -10684,6 +10705,36 @@ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. +================================================================================ +MIT License +================================================================================ + +The following component(s) are licensed under MIT License: + - adlx 0.0.0-alpha.1 + +MIT License + +Copyright (c) 2024 Traverse Research BV + +Permission is hereby granted, free of charge, to any person obtaining a copy +of this software and associated documentation files (the "Software"), to deal +in the Software without restriction, including without limitation the rights +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +copies of the Software, and to permit persons to whom the Software is +furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice shall be included in all +copies or substantial portions of the Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +SOFTWARE. + + ================================================================================ MIT License ================================================================================ diff --git a/crates/rocm-dash-collectors/Cargo.toml b/crates/rocm-dash-collectors/Cargo.toml index da6c5c631..2e42439c2 100644 --- a/crates/rocm-dash-collectors/Cargo.toml +++ b/crates/rocm-dash-collectors/Cargo.toml @@ -30,3 +30,10 @@ reqwest = { version = "0.12", default-features = false } tokio = { version = "1", features = ["full"] } tempfile = "3" wiremock = "0.6" + +# ADLX binds dynamically to the AMD driver's bundled amdadlx64.dll at +# runtime (not a build-time link), so this only makes sense on Windows; +# winperf.rs's `AdlxHelper::new()` call already degrades to 0.0/None when +# the DLL isn't present (no AMD driver, non-AMD GPU), same as elsewhere. +[target.'cfg(windows)'.dependencies] +adlx = "0.0.0-alpha.1" diff --git a/crates/rocm-dash-collectors/src/winperf.rs b/crates/rocm-dash-collectors/src/winperf.rs index cdbaa1753..1411fb85e 100644 --- a/crates/rocm-dash-collectors/src/winperf.rs +++ b/crates/rocm-dash-collectors/src/winperf.rs @@ -2,7 +2,13 @@ // // SPDX-License-Identifier: MIT -//! Live Windows GPU metrics via OS performance counters, no AMD tool needed. +//! Live Windows GPU metrics via OS performance counters, no AMD tool needed +//! for utilization/VRAM. +//! +//! Temperature, power, and clock speed additionally use ADLX (AMD Device +//! Library eXtra, via the `adlx` crate) when the AMD driver's bundled +//! `amdadlx64.dll` is present; absence (no AMD driver, non-AMD GPU) degrades +//! those three fields to 0.0/None rather than failing the whole snapshot. //! //! Reads the `GPU Engine`/`GPU Adapter Memory` categories — the same //! WDDM/DXGK-backed source Task Manager's GPU tab reads. Mirrors @@ -142,6 +148,7 @@ fn parse_metrics(raw: &str) -> Vec { } } + let adlx = adlx_readings(); let adapters = parse_adapters(raw); if let [adapter] = adapters.as_slice() { if order.is_empty() { @@ -149,32 +156,87 @@ fn parse_metrics(raw: &str) -> Vec { } let vram_used_mb: u64 = vram_mb.values().sum(); let gpu_utilization_pct = util.values().copied().fold(0.0f32, f32::max).min(100.0); + let (temperature_c, power_w, clock_mhz) = apply_adlx(adlx.first()); return vec![GpuMetrics { device_id: "win-gpu-0".to_string(), vram_used_mb, vram_total_mb: adapter.ram_bytes / (1024 * 1024), gpu_utilization_pct, - temperature_c: 0.0, - power_w: 0.0, - clock_mhz: None, + temperature_c, + power_w, + clock_mhz, }]; } order .into_iter() .enumerate() - .map(|(i, key)| GpuMetrics { - device_id: format!("win-gpu-{i}"), - vram_used_mb: vram_mb.get(key).copied().unwrap_or(0), - vram_total_mb: 0, - gpu_utilization_pct: util.get(key).copied().unwrap_or(0.0).min(100.0), - temperature_c: 0.0, - power_w: 0.0, - clock_mhz: None, + .map(|(i, key)| { + let (temperature_c, power_w, clock_mhz) = apply_adlx(adlx.get(i)); + GpuMetrics { + device_id: format!("win-gpu-{i}"), + vram_used_mb: vram_mb.get(key).copied().unwrap_or(0), + vram_total_mb: 0, + gpu_utilization_pct: util.get(key).copied().unwrap_or(0.0).min(100.0), + temperature_c, + power_w, + clock_mhz, + } }) .collect() } +/// Per-GPU temperature/power/clock reading from ADLX. Each field is a plain +/// value (not `Option`): `adlx_readings` only emits an entry for a GPU once +/// `current_gpu_metrics` has already succeeded for it, so unsupported +/// individual metrics on that entry fall back to 0.0/None right there. +struct AdlxReading { + temperature_c: f32, + power_w: f32, + clock_mhz: Option, +} + +/// Matches an ADLX reading to a `parse_metrics` device slot by index (ADLX's +/// GPU list order isn't otherwise correlated to the perf-counter LUIDs here). +/// No reading for that slot, or a 0.0/None `AdlxReading`, degrades to the +/// same defaults `parse_metrics` used before ADLX existed. +fn apply_adlx(reading: Option<&AdlxReading>) -> (f32, f32, Option) { + ( + reading.map_or(0.0, |r| r.temperature_c), + reading.map_or(0.0, |r| r.power_w), + reading.and_then(|r| r.clock_mhz), + ) +} + +/// `AdlxHelper::new()` fails cleanly (`Err`, not a panic) when +/// `amdadlx64.dll` isn't found, so every step here can just bail to an empty +/// Vec via `.ok()?` — same graceful-degrade shape as the rest of this file. +#[cfg(windows)] +fn adlx_readings() -> Vec { + (|| -> Option> { + let helper = adlx::helper::AdlxHelper::new().ok()?; + let system = helper.system(); + let gpus = system.gpus().ok()?; + let services = system.performance_monitoring_services().ok()?; + Some( + gpus.iter() + .filter_map(|gpu| services.current_gpu_metrics(&gpu).ok()) + .map(|m| AdlxReading { + temperature_c: m.temperature().unwrap_or(0.0) as f32, + power_w: m.power().unwrap_or(0.0) as f32, + clock_mhz: m.clock_speed().ok().map(|c| c as f32), + }) + .collect(), + ) + })() + .unwrap_or_default() +} + +#[cfg(not(windows))] +const fn adlx_readings() -> Vec { + Vec::new() +} + struct Adapter { name: String, ram_bytes: u64, @@ -278,4 +340,24 @@ mod tests { assert!(parse_metrics("").is_empty()); assert_eq!(parse_system_info("").physical_gpu_count, 0); } + + #[test] + fn apply_adlx_falls_back_to_defaults_when_missing() { + assert_eq!(apply_adlx(None), (0.0, 0.0, None)); + } + + #[test] + fn apply_adlx_uses_reading_when_present() { + let reading = AdlxReading { + temperature_c: 65.0, + power_w: 12.5, + clock_mhz: Some(1900.0), + }; + assert_eq!(apply_adlx(Some(&reading)), (65.0, 12.5, Some(1900.0))); + } + + // adlx_readings() itself (the real ADLX FFI chain) is Windows-only and + // hardware/driver-dependent — not unit-testable here; CI's + // windows-build-and-test job covers compilation of the #[cfg(windows)] + // path, and apply_adlx above covers the pure mapping logic. } From c300bf3009f83fd1c5446db1bf7d46dbed602725 Mon Sep 17 00:00:00 2001 From: Juho Vainio Date: Thu, 22 Oct 2026 12:09:46 +0300 Subject: [PATCH 07/14] fix(dash-collectors): prefer ADLX VRAM total over truncated WMI AdapterRAM Win32_VideoController.AdapterRAM is a 32-bit WMI field that silently truncates/overflows above 4GiB. ADLX's own TotalVRAM query isn't subject to that, so parse_metrics now prefers it in the single-adapter/single-ADLX-GPU case and falls back to the WMI value otherwise (no driver, non-AMD GPU, multi-GPU host). parse_metrics also now takes adlx readings as a parameter instead of calling adlx_readings() internally, so its matching/fallback logic is unit-testable without the Windows-only ADLX FFI chain. Signed-off-by: Juho Vainio --- crates/rocm-dash-collectors/src/winperf.rs | 103 ++++++++++++++++++--- crates/rocm-dash-tui/src/app/event_loop.rs | 1 + crates/rocm-dash-tui/src/app/mod.rs | 18 ++++ crates/rocm-dash-tui/src/client.rs | 14 ++- 4 files changed, 123 insertions(+), 13 deletions(-) diff --git a/crates/rocm-dash-collectors/src/winperf.rs b/crates/rocm-dash-collectors/src/winperf.rs index 1411fb85e..84559d6ff 100644 --- a/crates/rocm-dash-collectors/src/winperf.rs +++ b/crates/rocm-dash-collectors/src/winperf.rs @@ -60,7 +60,10 @@ impl WinPerfCollector { /// same output, doubling exposure to spawn latency for nothing. pub async fn snapshot(&self) -> io::Result<(Vec, GpuSystemInfo)> { let out = Self::run_script().await?; - Ok((parse_metrics(&out), parse_system_info(&out))) + Ok(( + parse_metrics(&out, &adlx_readings()), + parse_system_info(&out), + )) } async fn run_script() -> io::Result { @@ -114,7 +117,12 @@ fn device_key(instance: &str) -> &str { /// APU) gets an exact total; multi-GPU hosts degrade to 0 instead of a wrong /// number. Upgrade path: WMI `MSPower_DeviceEnable`/DXGI LUID correlation if /// multi-GPU Windows hosts become a real target. -fn parse_metrics(raw: &str) -> Vec { +/// +/// `adlx` is a parameter (rather than this function calling `adlx_readings()` +/// itself) so tests can exercise the ADLX-matching/VRAM-fallback logic below +/// with fixed readings — `adlx_readings()`'s real FFI chain is Windows+driver +/// only and not unit-testable. +fn parse_metrics(raw: &str, adlx: &[AdlxReading]) -> Vec { let mut util: BTreeMap<&str, f32> = BTreeMap::new(); let mut vram_mb: BTreeMap<&str, u64> = BTreeMap::new(); let mut order: Vec<&str> = Vec::new(); @@ -148,7 +156,6 @@ fn parse_metrics(raw: &str) -> Vec { } } - let adlx = adlx_readings(); let adapters = parse_adapters(raw); if let [adapter] = adapters.as_slice() { if order.is_empty() { @@ -157,10 +164,21 @@ fn parse_metrics(raw: &str) -> Vec { let vram_used_mb: u64 = vram_mb.values().sum(); let gpu_utilization_pct = util.values().copied().fold(0.0f32, f32::max).min(100.0); let (temperature_c, power_w, clock_mhz) = apply_adlx(adlx.first()); + // `Win32_VideoController.AdapterRAM` is a 32-bit WMI field: any card + // with >=4GiB VRAM truncates/overflows there. ADLX's own VRAM query + // isn't subject to that, so prefer it when exactly one ADLX GPU + // reading exists (matching this single-real-adapter branch) and + // fall back to the WMI value otherwise (no AMD driver, non-AMD GPU). + let vram_total_mb = match adlx { + [reading] => reading + .total_vram_mb + .map_or(adapter.ram_bytes / (1024 * 1024), u64::from), + _ => adapter.ram_bytes / (1024 * 1024), + }; return vec![GpuMetrics { device_id: "win-gpu-0".to_string(), vram_used_mb, - vram_total_mb: adapter.ram_bytes / (1024 * 1024), + vram_total_mb, gpu_utilization_pct, temperature_c, power_w, @@ -194,6 +212,12 @@ struct AdlxReading { temperature_c: f32, power_w: f32, clock_mhz: Option, + /// ADLX's own VRAM-size query (`IADLXGPU::TotalVRAM`, already in MB). + /// `Win32_VideoController.AdapterRAM` is a 32-bit WMI field that + /// silently truncates/overflows above 4GiB — common on any discrete + /// card, just not on the integrated APU this was tested against — so + /// `parse_metrics` prefers this reading over WMI's when available. + total_vram_mb: Option, } /// Matches an ADLX reading to a `parse_metrics` device slot by index (ADLX's @@ -220,11 +244,12 @@ fn adlx_readings() -> Vec { let services = system.performance_monitoring_services().ok()?; Some( gpus.iter() - .filter_map(|gpu| services.current_gpu_metrics(&gpu).ok()) - .map(|m| AdlxReading { + .filter_map(|gpu| Some((gpu, services.current_gpu_metrics(gpu).ok()?))) + .map(|(gpu, m)| AdlxReading { temperature_c: m.temperature().unwrap_or(0.0) as f32, power_w: m.power().unwrap_or(0.0) as f32, clock_mhz: m.clock_speed().ok().map(|c| c as f32), + total_vram_mb: gpu.total_vram().ok(), }) .collect(), ) @@ -280,7 +305,7 @@ mod tests { #[test] fn parses_single_adapter_metrics_with_total_ram() { - let metrics = parse_metrics(SAMPLE); + let metrics = parse_metrics(SAMPLE, &[]); assert_eq!(metrics.len(), 1); let gpu = &metrics[0]; // Max across the two engine readings, not the sum. @@ -302,7 +327,7 @@ mod tests { let two_adapters = format!( "{SAMPLE}\nUTIL\tluid_0x00000000_0x0000cafe_phys_0_eng_0_engtype_3d\t1.0\nVRAM\tluid_0x00000000_0x0000cafe_phys_0\t1048576\nGPU\tOther GPU\t4294967296\t1.0" ); - let metrics = parse_metrics(&two_adapters); + let metrics = parse_metrics(&two_adapters, &[]); assert_eq!(metrics.len(), 2); assert!(metrics.iter().all(|g| g.vram_total_mb == 0)); } @@ -312,7 +337,7 @@ mod tests { // Two different pids hitting the same adapter's engines, plus a // single per-adapter VRAM sample (as Windows actually reports it). let sample = "UTIL\tpid_111_luid_0x00000000_0x0002ff2a_phys_0_eng_0_engtype_3d\t0.682\nUTIL\tpid_222_luid_0x00000000_0x0002ff2a_phys_0_eng_0_engtype_3d\t0.514\nVRAM\tluid_0x00000000_0x0002ff2a_phys_0\t902787072\nGPU\tAMD Radeon 780M Graphics\t1073741824\t32.0.31041.1004"; - let metrics = parse_metrics(sample); + let metrics = parse_metrics(sample, &[]); assert_eq!(metrics.len(), 1); let gpu = &metrics[0]; assert!((gpu.gpu_utilization_pct - 0.682).abs() < 0.001); @@ -327,7 +352,7 @@ mod tests { // zero-VRAM phantoms) for the single physical adapter // `Win32_VideoController` actually lists. let sample = "UTIL\tpid_1234_luid_0x00000000_0x0002ff2a_phys_0_eng_0_engtype_3d\t0.5\nVRAM\tluid_0x00000000_0x0002ff2a_phys_0\t897261568\nVRAM\tluid_0x00000000_0x000325a2_phys_0\t0\nVRAM\tluid_0x00000000_0x000325f9_phys_0\t0\nGPU\tAMD Radeon 780M Graphics\t1073741824\t32.0.31041.1004"; - let metrics = parse_metrics(sample); + let metrics = parse_metrics(sample, &[]); assert_eq!(metrics.len(), 1); let gpu = &metrics[0]; assert!((gpu.gpu_utilization_pct - 0.5).abs() < 0.001); @@ -335,9 +360,64 @@ mod tests { assert_eq!(gpu.vram_total_mb, 1_073_741_824 / (1024 * 1024)); } + #[test] + fn single_adapter_prefers_adlx_vram_over_wmi_truncated_value() { + // WMI's AdapterRAM here (4294967296 = exactly 4GiB) is the classic + // 32-bit-field overflow case; ADLX's own query reports the real + // 8GiB so parse_metrics should prefer it. + let sample = "UTIL\tluid_0x00000000_0x0000b3a4_phys_0_eng_0_engtype_3d\t1.0\nVRAM\tluid_0x00000000_0x0000b3a4_phys_0\t1048576\nGPU\tDiscrete GPU\t4294967296\t1.0"; + let adlx = [AdlxReading { + temperature_c: 50.0, + power_w: 10.0, + clock_mhz: None, + total_vram_mb: Some(8192), + }]; + let metrics = parse_metrics(sample, &adlx); + assert_eq!(metrics.len(), 1); + assert_eq!(metrics[0].vram_total_mb, 8192); + } + + #[test] + fn multiple_adapters_ignore_adlx_vram_and_leave_total_zero() { + let two_adapters = format!( + "{SAMPLE}\nUTIL\tluid_0x00000000_0x0000cafe_phys_0_eng_0_engtype_3d\t1.0\nVRAM\tluid_0x00000000_0x0000cafe_phys_0\t1048576\nGPU\tOther GPU\t4294967296\t1.0" + ); + let adlx = [AdlxReading { + temperature_c: 50.0, + power_w: 10.0, + clock_mhz: None, + total_vram_mb: Some(8192), + }]; + let metrics = parse_metrics(&two_adapters, &adlx); + assert_eq!(metrics.len(), 2); + assert!(metrics.iter().all(|g| g.vram_total_mb == 0)); + } + + #[test] + fn single_adapter_falls_back_to_wmi_vram_when_adlx_reading_count_mismatches() { + let sample = "UTIL\tluid_0x00000000_0x0000b3a4_phys_0_eng_0_engtype_3d\t1.0\nVRAM\tluid_0x00000000_0x0000b3a4_phys_0\t1048576\nGPU\tDiscrete GPU\t4294967296\t1.0"; + let two_adlx = [ + AdlxReading { + temperature_c: 50.0, + power_w: 10.0, + clock_mhz: None, + total_vram_mb: Some(8192), + }, + AdlxReading { + temperature_c: 40.0, + power_w: 8.0, + clock_mhz: None, + total_vram_mb: Some(4096), + }, + ]; + let metrics = parse_metrics(sample, &two_adlx); + assert_eq!(metrics.len(), 1); + assert_eq!(metrics[0].vram_total_mb, 4_294_967_296 / (1024 * 1024)); + } + #[test] fn empty_output_yields_no_metrics() { - assert!(parse_metrics("").is_empty()); + assert!(parse_metrics("", &[]).is_empty()); assert_eq!(parse_system_info("").physical_gpu_count, 0); } @@ -352,6 +432,7 @@ mod tests { temperature_c: 65.0, power_w: 12.5, clock_mhz: Some(1900.0), + total_vram_mb: Some(16384), }; assert_eq!(apply_adlx(Some(&reading)), (65.0, 12.5, Some(1900.0))); } diff --git a/crates/rocm-dash-tui/src/app/event_loop.rs b/crates/rocm-dash-tui/src/app/event_loop.rs index d7604cdb4..dd6f581e8 100644 --- a/crates/rocm-dash-tui/src/app/event_loop.rs +++ b/crates/rocm-dash-tui/src/app/event_loop.rs @@ -1041,6 +1041,7 @@ async fn event_loop(terminal: &mut Tui, args: &ResolvedArgs) -> color_eyre::Resu state.latest = None; } Some(ClientMsg::Event(ev)) => state.apply_event(*ev), + Some(ClientMsg::HostGpuSnapshot(snap)) => state.push_host_gpu_snapshot(*snap), Some(ClientMsg::ReplaySeek) => state.reset_for_seek(), Some(ClientMsg::ReplayPosition { elapsed_s, total_s }) => { if let Some(r) = state.replay.as_mut() { diff --git a/crates/rocm-dash-tui/src/app/mod.rs b/crates/rocm-dash-tui/src/app/mod.rs index f3c0123bd..5c5a7612e 100644 --- a/crates/rocm-dash-tui/src/app/mod.rs +++ b/crates/rocm-dash-tui/src/app/mod.rs @@ -1490,6 +1490,24 @@ impl AppState { self.clamp_selectors(); } + /// Windows sibling of [`push_snapshot`](Self::push_snapshot) for + /// `ClientMsg::HostGpuSnapshot`: applies host/GPU metrics for + /// charts/history exactly like a real snapshot, but deliberately leaves + /// `instances` and `has_received_snapshot` untouched. The Windows + /// in-process ticker (`client.rs`) always sends an empty `instances` + /// stand-in (no instance-registry daemon exists there yet), so treating + /// it as authoritative the way `push_snapshot` does would permanently + /// blind the issue #145 quit-confirm prompt to models actually being + /// served — see `has_live_instance_with`'s Windows note. + pub(crate) fn push_host_gpu_snapshot(&mut self, snap: Snapshot) { + if self.history.len() == HISTORY_CAP { + self.history.pop_front(); + } + self.history.push_back(snap.clone()); + self.latest = Some(snap); + self.clamp_selectors(); + } + fn upsert_instance(&mut self, inst: Instance) { self.instances.insert(inst.container_id.clone(), inst); self.clamp_selectors(); diff --git a/crates/rocm-dash-tui/src/client.rs b/crates/rocm-dash-tui/src/client.rs index 4659b3f62..fe7844272 100644 --- a/crates/rocm-dash-tui/src/client.rs +++ b/crates/rocm-dash-tui/src/client.rs @@ -13,6 +13,7 @@ use std::path::PathBuf; use anyhow::{Context, anyhow}; +use rocm_dash_core::metrics::Snapshot; use rocm_dash_core::protocol::{Command, Event, PROTOCOL_VERSION}; use tokio::io::{AsyncWriteExt, BufReader}; use tokio::sync::mpsc::UnboundedSender; @@ -33,6 +34,15 @@ pub enum ClientMsg { reason: String, }, Event(Box), + /// Windows in-process synthetic tick (`#[cfg(windows)] connect_and_run`): + /// host/GPU metrics sourced from local collectors, not a real daemon. + /// Deliberately NOT `Event(Snapshot)` — that path treats `instances` as + /// authoritative truth and latches `has_received_snapshot` forever, but + /// this snapshot's `instances` is always an empty stand-in (Windows has + /// no instance registry daemon yet), so routing it through there would + /// permanently blind the issue #145 quit-confirm prompt to models + /// actually being served. See `AppState::push_host_gpu_snapshot`. + HostGpuSnapshot(Box), /// Replay-only: the scrubber jumped backward. The TUI must wipe any /// state derived from past events before the burst-emitted events for /// the new playhead position arrive. @@ -188,7 +198,7 @@ async fn connect_and_run(connect: &str, tx: UnboundedSender) -> anyho async fn connect_and_run(connect: &str, tx: UnboundedSender) -> anyhow::Result<()> { use rocm_dash_collectors::host::HostCollector; use rocm_dash_collectors::winperf::WinPerfCollector; - use rocm_dash_core::metrics::{GpuSystemInfo, Snapshot}; + use rocm_dash_core::metrics::GpuSystemInfo; info!( connect, @@ -219,7 +229,7 @@ async fn connect_and_run(connect: &str, tx: UnboundedSender) -> anyho warnings: Vec::new(), }; if tx - .send(ClientMsg::Event(Box::new(Event::Snapshot(snapshot)))) + .send(ClientMsg::HostGpuSnapshot(Box::new(snapshot))) .is_err() { break; From 041e2f46b0643f424925a7b67e2fbdf09a5e5974 Mon Sep 17 00:00:00 2001 From: Juho Vainio Date: Thu, 22 Oct 2026 12:36:17 +0300 Subject: [PATCH 08/14] fix(dash-collectors): fix Windows cross-compile error in adlx_readings current_gpu_metrics(&gpu) needs gpu borrowed, not moved, and the move has to happen after the call since Gpu isn't Copy. Caught by actually building for x86_64-pc-windows-gnu (this #[cfg(windows)] function was gated out of every check/clippy run on the Linux dev workstation). Signed-off-by: Juho Vainio --- crates/rocm-dash-collectors/src/winperf.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/rocm-dash-collectors/src/winperf.rs b/crates/rocm-dash-collectors/src/winperf.rs index 84559d6ff..685c4d3a7 100644 --- a/crates/rocm-dash-collectors/src/winperf.rs +++ b/crates/rocm-dash-collectors/src/winperf.rs @@ -244,7 +244,10 @@ fn adlx_readings() -> Vec { let services = system.performance_monitoring_services().ok()?; Some( gpus.iter() - .filter_map(|gpu| Some((gpu, services.current_gpu_metrics(gpu).ok()?))) + .filter_map(|gpu| { + let m = services.current_gpu_metrics(&gpu).ok()?; + Some((gpu, m)) + }) .map(|(gpu, m)| AdlxReading { temperature_c: m.temperature().unwrap_or(0.0) as f32, power_w: m.power().unwrap_or(0.0) as f32, From 5a5dd3eff21f06044cb66d40796988f216aec150 Mon Sep 17 00:00:00 2001 From: Juho Vainio Date: Thu, 22 Oct 2026 14:10:11 +0300 Subject: [PATCH 09/14] fix(dash-collectors,dash-tui): resolve PR review findings for Windows GPU metrics - ADLX-to-device matching: only trust temp/power/clock when exactly one real adapter and one ADLX reading exist, matching vram_total_mb's existing guard; multi-adapter hosts degrade every slot to defaults instead of index-based misattribution. - Run adlx_readings() inside spawn_blocking so ADLX FFI/driver calls don't block a tokio worker thread. - kill_on_drop on the powershell.exe Command so a timed-out child is actually killed instead of leaked. - Mitigate amdadlx64.dll DLL search-order hijack via SetDefaultDllDirectories(LOAD_LIBRARY_SEARCH_SYSTEM32), called once process-wide before the first AdlxHelper::new(). - Update stale "Windows has no live dashboard" docs/comments now that the Windows client streams live GPU/CPU/memory (registry view still empty by design, no rocmdashd unix socket there). - Strengthen winperf.rs and home.rs tests flagged as weak/vacuous: direct vram_used_pct unit test, ADLX temp/power/clock assertions, isolated pid-prefix-stripping fixture, order.is_empty() guard test, sum-vs-max VRAM fixture, realistic SAMPLE AdapterRAM value. - Non-blocking cleanups: ticker MissedTickBehavior::Skip matching the daemon's existing choice, fixed stray comment's wrong API name. Signed-off-by: Juho Vainio --- Cargo.lock | 1 + README.md | 6 +- apps/rocm/src/dash.rs | 11 +- crates/rocm-dash-collectors/Cargo.toml | 1 + crates/rocm-dash-collectors/src/winperf.rs | 218 ++++++++++++++++----- crates/rocm-dash-tui/src/app/mod.rs | 14 +- crates/rocm-dash-tui/src/client.rs | 10 +- crates/rocm-dash-tui/src/ui/tabs/home.rs | 49 +++-- docs/manual-testing.md | 16 +- docs/rocm-docs/install/installation.md | 4 +- 10 files changed, 231 insertions(+), 99 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 03c9b3967..697b40780 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3744,6 +3744,7 @@ dependencies = [ "thiserror 2.0.18", "tokio", "tracing", + "windows-sys 0.61.2", "wiremock", ] diff --git a/README.md b/README.md index 5bbba2164..5b841c432 100644 --- a/README.md +++ b/README.md @@ -31,7 +31,7 @@ for Lemonade and vLLM. | Platform | Prebuilt binary | Notes | |---|---|---| | Linux (x86_64) | Yes | Ubuntu 24.04 or newer; full support, including the live dashboard and both inference engines | -| Windows (x86_64) | Yes | CLI and Lemonade serving; no live dashboard or vLLM | +| Windows (x86_64) | Yes | CLI and Lemonade serving; live dashboard shows GPU/CPU/memory but not the serving-instance registry; no vLLM | | WSL2 (x86_64) | Yes (Linux binary) | Ubuntu 24.04 or newer; full support, including the live dashboard | | macOS | No | No official installer, release, CI, or QA coverage | @@ -937,7 +937,9 @@ For `rocm dash`: - `--demo` runs a deterministic synthetic session with no GPU or daemon needed. It works on all platforms. - `--replay ` replays a recorded NDJSON session. -- Live mode requires Unix domain sockets (Linux and WSL2 only). +- Live mode works on Windows too, but shows only GPU/CPU/memory — there's no + `rocmdashd` unix socket to connect to there, so the serving-instance + registry view stays empty (Linux and WSL2 get the full live view). ### Bench diff --git a/apps/rocm/src/dash.rs b/apps/rocm/src/dash.rs index 891255c1d..7af55d7bd 100644 --- a/apps/rocm/src/dash.rs +++ b/apps/rocm/src/dash.rs @@ -339,11 +339,12 @@ pub fn resolved_args( /// Re-reads `services_dir` fresh on every [`snapshot`](ManagedRegistryProbe::snapshot) /// call: a local JSON-file read, no daemon round-trip, no network I/O — cheap /// enough for the quit-confirm gate to call on every render while the prompt -/// is open, and the only source of truth at all on a host whose -/// dashboard-embedded daemon client never connects (Windows — -/// `rocm_dash_tui::client`'s connect path and `maybe_spawn_embedded_daemon` -/// are unix-only, so `AppState::instances` never gets a first snapshot -/// there). +/// is open, and the only source of truth at all on Windows, where +/// `rocm_dash_tui::client`'s `#[cfg(windows)]` arm does connect and stream +/// live GPU/CPU/memory every tick but — by design, there being no +/// `rocmdashd` unix socket to source one from (`maybe_spawn_embedded_daemon` +/// is unix-only) — never sends an instance snapshot, so +/// `AppState::instances` never gets a first one there. /// /// **Known divergence, same shape as [`services_past_attempts`]'s:** this /// reads the record's raw on-disk `status`, not process liveness. A service diff --git a/crates/rocm-dash-collectors/Cargo.toml b/crates/rocm-dash-collectors/Cargo.toml index 2e42439c2..f9f50b69e 100644 --- a/crates/rocm-dash-collectors/Cargo.toml +++ b/crates/rocm-dash-collectors/Cargo.toml @@ -37,3 +37,4 @@ wiremock = "0.6" # the DLL isn't present (no AMD driver, non-AMD GPU), same as elsewhere. [target.'cfg(windows)'.dependencies] adlx = "0.0.0-alpha.1" +windows-sys = { version = "0.61", features = ["Win32_System_LibraryLoader"] } diff --git a/crates/rocm-dash-collectors/src/winperf.rs b/crates/rocm-dash-collectors/src/winperf.rs index 685c4d3a7..83fbed1e0 100644 --- a/crates/rocm-dash-collectors/src/winperf.rs +++ b/crates/rocm-dash-collectors/src/winperf.rs @@ -23,6 +23,7 @@ use std::time::Duration; use rocm_dash_core::metrics::{GpuMetrics, GpuSystemInfo}; use tokio::process::Command; +use tokio::task; use tokio::time::timeout; // 5s was too tight: on real hardware `powershell.exe` child-process spawn @@ -60,10 +61,13 @@ impl WinPerfCollector { /// same output, doubling exposure to spawn latency for nothing. pub async fn snapshot(&self) -> io::Result<(Vec, GpuSystemInfo)> { let out = Self::run_script().await?; - Ok(( - parse_metrics(&out, &adlx_readings()), - parse_system_info(&out), - )) + // adlx_readings() calls into amdadlx64.dll via FFI, which blocks on + // driver I/O — running it directly here would stall a tokio worker + // thread, same hazard run_script() avoids by using an async Command. + let adlx = task::spawn_blocking(adlx_readings) + .await + .unwrap_or_default(); + Ok((parse_metrics(&out, &adlx), parse_system_info(&out))) } async fn run_script() -> io::Result { @@ -71,6 +75,10 @@ impl WinPerfCollector { RUN_TIMEOUT, Command::new("powershell.exe") .args(["-NoProfile", "-NonInteractive", "-Command", WINPERF_SCRIPT]) + // Dropping the child on timeout must actually kill it, or a + // hung powershell.exe (and its driver query) leaks forever + // instead of exiting with the future that gave up on it. + .kill_on_drop(true) .output(), ) .await @@ -115,8 +123,8 @@ fn device_key(instance: &str) -> &str { /// consumer already guards `vram_total_mb > 0` before dividing). /// ponytail: single-adapter case (the realistic target: an integrated AMD /// APU) gets an exact total; multi-GPU hosts degrade to 0 instead of a wrong -/// number. Upgrade path: WMI `MSPower_DeviceEnable`/DXGI LUID correlation if -/// multi-GPU Windows hosts become a real target. +/// number. Upgrade path: DXGI/LUID-based adapter correlation if multi-GPU +/// Windows hosts become a real target. /// /// `adlx` is a parameter (rather than this function calling `adlx_readings()` /// itself) so tests can exercise the ADLX-matching/VRAM-fallback logic below @@ -163,7 +171,14 @@ fn parse_metrics(raw: &str, adlx: &[AdlxReading]) -> Vec { } let vram_used_mb: u64 = vram_mb.values().sum(); let gpu_utilization_pct = util.values().copied().fold(0.0f32, f32::max).min(100.0); - let (temperature_c, power_w, clock_mhz) = apply_adlx(adlx.first()); + // ADLX's GPU list isn't otherwise correlated to the perf-counter + // LUIDs here, so it's only trusted at all when there's exactly one + // ADLX reading to match this one real adapter against — same + // single-reading guard `vram_total_mb` below already uses. + let (temperature_c, power_w, clock_mhz) = match adlx { + [reading] => apply_adlx(Some(reading)), + _ => apply_adlx(None), + }; // `Win32_VideoController.AdapterRAM` is a 32-bit WMI field: any card // with >=4GiB VRAM truncates/overflows there. ADLX's own VRAM query // isn't subject to that, so prefer it when exactly one ADLX GPU @@ -186,28 +201,32 @@ fn parse_metrics(raw: &str, adlx: &[AdlxReading]) -> Vec { }]; } + // More than one real adapter: an ADLX GPU-list index has no reliable + // correlation to a specific perf-counter LUID (same reason + // `vram_total_mb` is left at 0 below), so every slot degrades to + // ADLX-absent defaults rather than risk reporting one GPU's temp/power/ + // clock under a different GPU's device_id. + let (temperature_c, power_w, clock_mhz) = apply_adlx(None); order .into_iter() .enumerate() - .map(|(i, key)| { - let (temperature_c, power_w, clock_mhz) = apply_adlx(adlx.get(i)); - GpuMetrics { - device_id: format!("win-gpu-{i}"), - vram_used_mb: vram_mb.get(key).copied().unwrap_or(0), - vram_total_mb: 0, - gpu_utilization_pct: util.get(key).copied().unwrap_or(0.0).min(100.0), - temperature_c, - power_w, - clock_mhz, - } + .map(|(i, key)| GpuMetrics { + device_id: format!("win-gpu-{i}"), + vram_used_mb: vram_mb.get(key).copied().unwrap_or(0), + vram_total_mb: 0, + gpu_utilization_pct: util.get(key).copied().unwrap_or(0.0).min(100.0), + temperature_c, + power_w, + clock_mhz, }) .collect() } -/// Per-GPU temperature/power/clock reading from ADLX. Each field is a plain -/// value (not `Option`): `adlx_readings` only emits an entry for a GPU once -/// `current_gpu_metrics` has already succeeded for it, so unsupported -/// individual metrics on that entry fall back to 0.0/None right there. +/// Per-GPU temperature/power/clock reading from ADLX. `adlx_readings` emits +/// exactly one entry per GPU in ADLX's list, whether or not +/// `current_gpu_metrics` succeeded for it, so a failure degrades the fields +/// to 0.0/None in place rather than shifting every later GPU's reading out +/// of position. struct AdlxReading { temperature_c: f32, power_w: f32, @@ -220,10 +239,11 @@ struct AdlxReading { total_vram_mb: Option, } -/// Matches an ADLX reading to a `parse_metrics` device slot by index (ADLX's -/// GPU list order isn't otherwise correlated to the perf-counter LUIDs here). -/// No reading for that slot, or a 0.0/None `AdlxReading`, degrades to the -/// same defaults `parse_metrics` used before ADLX existed. +/// `None` (no device-identity correlation to trust, or ADLX unavailable) +/// degrades to the same 0.0/None defaults `parse_metrics` used before ADLX +/// existed. Callers only pass `Some` when there's exactly one real adapter +/// and exactly one ADLX reading — the only case this file can match an ADLX +/// reading to a device without index-based guessing. fn apply_adlx(reading: Option<&AdlxReading>) -> (f32, f32, Option) { ( reading.map_or(0.0, |r| r.temperature_c), @@ -233,10 +253,30 @@ fn apply_adlx(reading: Option<&AdlxReading>) -> (f32, f32, Option) { } /// `AdlxHelper::new()` fails cleanly (`Err`, not a panic) when -/// `amdadlx64.dll` isn't found, so every step here can just bail to an empty -/// Vec via `.ok()?` — same graceful-degrade shape as the rest of this file. +/// `amdadlx64.dll` isn't found, so a total init failure can just bail to an +/// empty Vec via `.ok()?` — same graceful-degrade shape as the rest of this +/// file. A single GPU's metrics query failing, though, must NOT drop that +/// GPU's slot (a `filter_map` here would shift every later GPU's reading one +/// position to the left, silently misattributing it in `parse_metrics`'s +/// index-based correlation) — it degrades to a zeroed `AdlxReading` instead, +/// keeping one slot per GPU. #[cfg(windows)] fn adlx_readings() -> Vec { + // `adlx::helper::AdlxHelper::new()` loads `amdadlx64.dll` by bare name + // (`libloading::Library::new("amdadlx64.dll")`, no search-order + // hardening, no way to pass flags into the vendored crate). Restricting + // the process-wide default DLL search path to System32 closes the + // planted-DLL-in-PATH/cwd hijack vector for that unflagged load. Must + // run before the first `AdlxHelper::new()` call; `Once` makes repeated + // per-tick calls to this function a no-op after the first. + static HARDEN_DLL_SEARCH_PATH: std::sync::Once = std::sync::Once::new(); + #[allow(unsafe_code)] // Win32 FFI + HARDEN_DLL_SEARCH_PATH.call_once(|| unsafe { + windows_sys::Win32::System::LibraryLoader::SetDefaultDllDirectories( + windows_sys::Win32::System::LibraryLoader::LOAD_LIBRARY_SEARCH_SYSTEM32, + ); + }); + (|| -> Option> { let helper = adlx::helper::AdlxHelper::new().ok()?; let system = helper.system(); @@ -244,15 +284,19 @@ fn adlx_readings() -> Vec { let services = system.performance_monitoring_services().ok()?; Some( gpus.iter() - .filter_map(|gpu| { - let m = services.current_gpu_metrics(&gpu).ok()?; - Some((gpu, m)) - }) - .map(|(gpu, m)| AdlxReading { - temperature_c: m.temperature().unwrap_or(0.0) as f32, - power_w: m.power().unwrap_or(0.0) as f32, - clock_mhz: m.clock_speed().ok().map(|c| c as f32), - total_vram_mb: gpu.total_vram().ok(), + .map(|gpu| match services.current_gpu_metrics(&gpu) { + Ok(m) => AdlxReading { + temperature_c: m.temperature().unwrap_or(0.0) as f32, + power_w: m.power().unwrap_or(0.0) as f32, + clock_mhz: m.clock_speed().ok().map(|c| c as f32), + total_vram_mb: gpu.total_vram().ok(), + }, + Err(_) => AdlxReading { + temperature_c: 0.0, + power_w: 0.0, + clock_mhz: None, + total_vram_mb: None, + }, }) .collect(), ) @@ -304,7 +348,10 @@ mod tests { // Real `GPU Engine` instances are per-process (`pid_NNNN_luid_...`); mix // in a pid prefix here so the tests actually exercise that shape. - const SAMPLE: &str = "UTIL\tpid_1234_luid_0x00000000_0x0000b3a4_phys_0_eng_0_engtype_3d\t12.5\nUTIL\tpid_1234_luid_0x00000000_0x0000b3a4_phys_0_eng_1_engtype_video\t4.0\nVRAM\tluid_0x00000000_0x0000b3a4_phys_0\t8589934592\nGPU\tAMD Radeon(TM) Graphics\t17179869184\t31.0.21021.1000"; + // 3221225472 (3GiB) and 1073741824 (1GiB) are both representable in the + // real 32-bit `AdapterRAM` WMI field (unlike 16GiB, which would already + // have overflowed it). + const SAMPLE: &str = "UTIL\tpid_1234_luid_0x00000000_0x0000b3a4_phys_0_eng_0_engtype_3d\t12.5\nUTIL\tpid_1234_luid_0x00000000_0x0000b3a4_phys_0_eng_1_engtype_video\t4.0\nVRAM\tluid_0x00000000_0x0000b3a4_phys_0\t1073741824\nGPU\tAMD Radeon(TM) Graphics\t3221225472\t31.0.21021.1000"; #[test] fn parses_single_adapter_metrics_with_total_ram() { @@ -313,8 +360,8 @@ mod tests { let gpu = &metrics[0]; // Max across the two engine readings, not the sum. assert!((gpu.gpu_utilization_pct - 12.5).abs() < 0.01); - assert_eq!(gpu.vram_used_mb, 8192); - assert_eq!(gpu.vram_total_mb, 16384); + assert_eq!(gpu.vram_used_mb, 1024); + assert_eq!(gpu.vram_total_mb, 3072); } #[test] @@ -338,14 +385,44 @@ mod tests { #[test] fn multiple_processes_on_one_adapter_collapse_to_one_device() { // Two different pids hitting the same adapter's engines, plus a - // single per-adapter VRAM sample (as Windows actually reports it). - let sample = "UTIL\tpid_111_luid_0x00000000_0x0002ff2a_phys_0_eng_0_engtype_3d\t0.682\nUTIL\tpid_222_luid_0x00000000_0x0002ff2a_phys_0_eng_0_engtype_3d\t0.514\nVRAM\tluid_0x00000000_0x0002ff2a_phys_0\t902787072\nGPU\tAMD Radeon 780M Graphics\t1073741824\t32.0.31041.1004"; + // *second*, unrelated real adapter. With only one real adapter (as + // the original fixture had), the single-real-adapter collapse rescue + // below would also force everything to one device, masking whether + // this is actually testing `device_key`'s pid-prefix stripping; with + // two real adapters that rescue never fires (its pattern match + // requires exactly one), isolating the two code paths. + let sample = "UTIL\tpid_111_luid_0x00000000_0x0002ff2a_phys_0_eng_0_engtype_3d\t0.682\nUTIL\tpid_222_luid_0x00000000_0x0002ff2a_phys_0_eng_0_engtype_3d\t0.514\nVRAM\tluid_0x00000000_0x0002ff2a_phys_0\t902787072\nUTIL\tluid_0x00000000_0x0000cafe_phys_0_eng_0_engtype_3d\t0.1\nVRAM\tluid_0x00000000_0x0000cafe_phys_0\t1048576\nGPU\tAMD Radeon 780M Graphics\t1073741824\t32.0.31041.1004\nGPU\tOther GPU\t4294967296\t1.0"; let metrics = parse_metrics(sample, &[]); - assert_eq!(metrics.len(), 1); + assert_eq!(metrics.len(), 2); let gpu = &metrics[0]; assert!((gpu.gpu_utilization_pct - 0.682).abs() < 0.001); assert_eq!(gpu.vram_used_mb, 902_787_072 / (1024 * 1024)); - assert_eq!(gpu.vram_total_mb, 1_073_741_824 / (1024 * 1024)); + // Multi-real-adapter case: vram_total_mb always 0, no LUID correlation. + assert_eq!(gpu.vram_total_mb, 0); + } + + #[test] + fn single_real_adapter_sums_vram_used_across_distinct_luids() { + // Two *distinct* non-zero per-LUID VRAM readings on the one real + // adapter — unlike the phantom-LUID fixture below (0 + 0 + real), + // this is the only fixture where summing vram_used_mb actually + // differs from taking its max, so a sum->max mutation would be + // caught here. + let sample = "UTIL\tluid_0x00000000_0x0002ff2a_phys_0_eng_0_engtype_3d\t1.0\nVRAM\tluid_0x00000000_0x0002ff2a_phys_0\t536870912\nVRAM\tluid_0x00000000_0x000325a2_phys_0\t268435456\nGPU\tAMD Radeon 780M Graphics\t1073741824\t32.0.31041.1004"; + let metrics = parse_metrics(sample, &[]); + assert_eq!(metrics.len(), 1); + assert_eq!(metrics[0].vram_used_mb, 512 + 256); + } + + #[test] + fn gpu_inventory_without_counter_samples_yields_no_metrics() { + // A real adapter shows up in Win32_VideoController but Get-Counter + // returned nothing for it (e.g. a locale where Get-Counter's + // -ErrorAction silently swallows a localized-path mismatch) — must + // not fabricate a zeroed phantom device for it. + let sample = "GPU\tAMD Radeon 780M Graphics\t1073741824\t32.0.31041.1004"; + assert!(parse_metrics(sample, &[]).is_empty()); + assert_eq!(parse_system_info(sample).physical_gpu_count, 1); } #[test] @@ -364,6 +441,7 @@ mod tests { } #[test] + #[allow(clippy::float_cmp)] fn single_adapter_prefers_adlx_vram_over_wmi_truncated_value() { // WMI's AdapterRAM here (4294967296 = exactly 4GiB) is the classic // 32-bit-field overflow case; ADLX's own query reports the real @@ -372,12 +450,15 @@ mod tests { let adlx = [AdlxReading { temperature_c: 50.0, power_w: 10.0, - clock_mhz: None, + clock_mhz: Some(1800.0), total_vram_mb: Some(8192), }]; let metrics = parse_metrics(sample, &adlx); assert_eq!(metrics.len(), 1); assert_eq!(metrics[0].vram_total_mb, 8192); + assert_eq!(metrics[0].temperature_c, 50.0); + assert_eq!(metrics[0].power_w, 10.0); + assert_eq!(metrics[0].clock_mhz, Some(1800.0)); } #[test] @@ -388,34 +469,75 @@ mod tests { let adlx = [AdlxReading { temperature_c: 50.0, power_w: 10.0, - clock_mhz: None, + clock_mhz: Some(1800.0), total_vram_mb: Some(8192), }]; let metrics = parse_metrics(&two_adapters, &adlx); assert_eq!(metrics.len(), 2); assert!(metrics.iter().all(|g| g.vram_total_mb == 0)); + assert!( + metrics + .iter() + .all(|g| g.temperature_c == 0.0 && g.power_w == 0.0 && g.clock_mhz.is_none()) + ); + } + + #[test] + fn multiple_adapters_never_trust_adlx_even_when_reading_count_matches_adapter_count() { + // Two ADLX readings for two real adapters: if `parse_metrics` ever + // started correlating by list position again, this is exactly the + // shape that would make it look like it "worked" by coincidence. + let two_adapters = format!( + "{SAMPLE}\nUTIL\tluid_0x00000000_0x0000cafe_phys_0_eng_0_engtype_3d\t1.0\nVRAM\tluid_0x00000000_0x0000cafe_phys_0\t1048576\nGPU\tOther GPU\t4294967296\t1.0" + ); + let two_adlx = [ + AdlxReading { + temperature_c: 50.0, + power_w: 10.0, + clock_mhz: Some(1800.0), + total_vram_mb: Some(8192), + }, + AdlxReading { + temperature_c: 40.0, + power_w: 8.0, + clock_mhz: Some(1200.0), + total_vram_mb: Some(4096), + }, + ]; + let metrics = parse_metrics(&two_adapters, &two_adlx); + assert_eq!(metrics.len(), 2); + assert!(metrics.iter().all(|g| g.vram_total_mb == 0)); + assert!( + metrics + .iter() + .all(|g| g.temperature_c == 0.0 && g.power_w == 0.0 && g.clock_mhz.is_none()) + ); } #[test] + #[allow(clippy::float_cmp)] fn single_adapter_falls_back_to_wmi_vram_when_adlx_reading_count_mismatches() { let sample = "UTIL\tluid_0x00000000_0x0000b3a4_phys_0_eng_0_engtype_3d\t1.0\nVRAM\tluid_0x00000000_0x0000b3a4_phys_0\t1048576\nGPU\tDiscrete GPU\t4294967296\t1.0"; let two_adlx = [ AdlxReading { temperature_c: 50.0, power_w: 10.0, - clock_mhz: None, + clock_mhz: Some(1800.0), total_vram_mb: Some(8192), }, AdlxReading { temperature_c: 40.0, power_w: 8.0, - clock_mhz: None, + clock_mhz: Some(1200.0), total_vram_mb: Some(4096), }, ]; let metrics = parse_metrics(sample, &two_adlx); assert_eq!(metrics.len(), 1); assert_eq!(metrics[0].vram_total_mb, 4_294_967_296 / (1024 * 1024)); + assert_eq!(metrics[0].temperature_c, 0.0); + assert_eq!(metrics[0].power_w, 0.0); + assert_eq!(metrics[0].clock_mhz, None); } #[test] diff --git a/crates/rocm-dash-tui/src/app/mod.rs b/crates/rocm-dash-tui/src/app/mod.rs index 5c5a7612e..6680c523b 100644 --- a/crates/rocm-dash-tui/src/app/mod.rs +++ b/crates/rocm-dash-tui/src/app/mod.rs @@ -649,12 +649,14 @@ impl AppState { /// populated once that snapshot arrives (see `push_snapshot`), so a `q` /// pressed in that startup window would otherwise see this return `false` /// even though a model actually was being served, simply because the live - /// state hadn't arrived yet. On a host whose dashboard-embedded daemon - /// client never connects at all (Windows — `client.rs`'s connect path is - /// unix-only), `has_received_snapshot` never flips, so the live read - /// stays authoritative for the entire session rather than only a brief - /// startup window — the registry read is cheap and re-run every time this - /// is called, so there is no staleness to outlive. Once a snapshot lands + /// state hadn't arrived yet. On Windows, `client.rs`'s `#[cfg(windows)]` + /// arm does connect and stream live GPU/CPU/memory every tick, but by + /// design never sends an instance snapshot (there's no `rocmdashd` unix + /// socket to source one from), so `has_received_snapshot` never flips + /// there either — the live read stays authoritative for the entire + /// session rather than only a brief startup window — the registry read is + /// cheap and re-run every time this is called, so there is no staleness + /// to outlive. Once a snapshot lands /// on a host where the daemon does connect, `instances` is authoritative /// and the read's *result* is no longer consulted for THIS question (a /// model that legitimately stops later in the session is still reflected diff --git a/crates/rocm-dash-tui/src/client.rs b/crates/rocm-dash-tui/src/client.rs index fe7844272..843d4fdd8 100644 --- a/crates/rocm-dash-tui/src/client.rs +++ b/crates/rocm-dash-tui/src/client.rs @@ -6,8 +6,10 @@ //! forwards everything (including connect/disconnect transitions) to the app loop //! via an mpsc channel. -// On Windows the connection is a #[cfg(unix)] stub, so the unix-socket imports -// below are only used on unix; silence the resulting unused-import noise there. +// On Windows there's no `rocmdashd` unix socket to connect to, so the +// `#[cfg(windows)]` arm builds snapshots in-process instead (see +// `connect_and_run`); the unix-socket imports below are only used on unix, +// so silence the resulting unused-import noise there. #![cfg_attr(windows, allow(unused_imports))] use std::path::PathBuf; @@ -213,6 +215,10 @@ async fn connect_and_run(connect: &str, tx: UnboundedSender) -> anyho let gpu = WinPerfCollector::new(); // Mirrors the daemon's `gpu_tick` cadence (rocm_dash_daemon::runner::RunnerOptions::default()). let mut ticker = tokio::time::interval(std::time::Duration::from_secs(1)); + // A slow powershell/ADLX tick (see `WinPerfCollector::snapshot`) must not + // queue up a burst of catch-up ticks once it returns; matches + // `rocm-dash-daemon/src/runner.rs`'s existing explicit choice. + ticker.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Skip); loop { ticker.tick().await; diff --git a/crates/rocm-dash-tui/src/ui/tabs/home.rs b/crates/rocm-dash-tui/src/ui/tabs/home.rs index 0c97ab164..7d07fb2ab 100644 --- a/crates/rocm-dash-tui/src/ui/tabs/home.rs +++ b/crates/rocm-dash-tui/src/ui/tabs/home.rs @@ -30,6 +30,20 @@ use crate::ui::panel::{self, BoxRole}; use crate::ui::sparkline::BrailleSparkline; use crate::ui::theme::Theme; +/// Summed VRAM used / summed VRAM total across all GPUs, as a percentage. +/// `total == 0` is a legitimate state (e.g. the Windows collector can't +/// correlate a multi-adapter host to specific VRAM sizes) — must read as +/// `0.0`, not `used / 1 * 100`. +fn vram_used_pct(snap: &rocm_dash_core::metrics::Snapshot) -> f64 { + let used: u64 = snap.gpus.iter().map(|g| g.vram_used_mb).sum(); + let total: u64 = snap.gpus.iter().map(|g| g.vram_total_mb).sum(); + if total > 0 { + used as f64 / total as f64 * 100.0 + } else { + 0.0 + } +} + /// History series helper: map each snapshot to a 0..=100 magnitude via `pick`. fn history f64>( state: &AppState, @@ -348,15 +362,7 @@ fn draw_hero_right(f: &mut Frame, area: Rect, state: &AppState, theme: &Theme) { rh[0], ); let latest = state.latest.as_ref(); - let vram = latest.map_or(0.0, |s| { - let used: u64 = s.gpus.iter().map(|g| g.vram_used_mb).sum(); - let total: u64 = s.gpus.iter().map(|g| g.vram_total_mb).sum(); - if total > 0 { - used as f64 / total as f64 * 100.0 - } else { - 0.0 - } - }); + let vram = latest.map_or(0.0, vram_used_pct); let temp = latest.map_or(0.0, |s| { s.gpus .iter() @@ -371,15 +377,7 @@ fn draw_hero_right(f: &mut Frame, area: Rect, state: &AppState, theme: &Theme) { rh[1], "VRAM ", &format::pct(vram as f32), - &history(state, |s| { - let used: u64 = s.gpus.iter().map(|g| g.vram_used_mb).sum(); - let total: u64 = s.gpus.iter().map(|g| g.vram_total_mb).sum(); - if total > 0 { - used as f64 / total as f64 * 100.0 - } else { - 0.0 - } - }), + &history(state, vram_used_pct), false, theme, ); @@ -690,13 +688,12 @@ mod tests { } #[test] - fn hero_vram_pct_is_zero_not_garbage_when_total_is_unknown() { + #[allow(clippy::float_cmp)] + fn vram_used_pct_is_zero_not_garbage_when_total_is_unknown() { // vram_total_mb == 0 is a legitimate state (e.g. the Windows // collector can't correlate a multi-adapter host to specific VRAM - // sizes) — must render "0.0%", not `used / 1 * 100`. - let mut s = AppState::new("t".into(), "default-dark".into()); - s.active_tab = ActiveTab::Home; - s.latest = Some(Snapshot { + // sizes) — must read as 0.0, not `used / 1 * 100` (would be 80800.0). + let snap = Snapshot { host: SystemMetrics::default(), gpus: vec![GpuMetrics { device_id: "GPU0".into(), @@ -708,10 +705,8 @@ mod tests { clock_mhz: None, }], ..Default::default() - }); - let out = render(&s, 160, 30); - assert!(out.contains("0.0%"), "expected 0.0% VRAM: {out:?}"); - assert!(!out.contains("80800"), "garbage VRAM pct leaked: {out:?}"); + }; + assert_eq!(vram_used_pct(&snap), 0.0); } #[test] diff --git a/docs/manual-testing.md b/docs/manual-testing.md index 99810713c..220f1c7e4 100644 --- a/docs/manual-testing.md +++ b/docs/manual-testing.md @@ -531,14 +531,14 @@ Expected result: - A prompt appears naming the serving model and warning that it may still be running in the background after you quit, with the real `rocm services stop --yes` command (the actual id, not a - placeholder) to stop it first. From the dashboard (not the launcher) on a - host where its own daemon client never connects (Windows, for the whole - session), the prompt still shows that same stop command and the service - id, but not the model's name — the dashboard's own live view is where the - name comes from, and it never fills there. The launcher's own copy of the - prompt always names the model, Windows included — it reads the - managed-service registry directly rather than through the dashboard's - daemon connection. + placeholder) to stop it first. From the dashboard (not the launcher) on + Windows, where the dashboard streams live GPU/CPU/memory but never gets a + serving-instance snapshot (no `rocmdashd` unix socket there), the prompt + still shows that same stop command and the service id, but not the + model's name — the dashboard's own live view is where the name comes + from, and it never fills there. The launcher's own copy of the prompt + always names the model, Windows included — it reads the managed-service + registry directly rather than through the dashboard's daemon connection. - From the dashboard (not the launcher), the prompt also offers `s`: pressing it closes the prompt and opens the Services overlay, pre-focused on the serving model, so you can stop it there through the overlay's own diff --git a/docs/rocm-docs/install/installation.md b/docs/rocm-docs/install/installation.md index 4ee7ac5f8..3fb9ed7b5 100644 --- a/docs/rocm-docs/install/installation.md +++ b/docs/rocm-docs/install/installation.md @@ -15,7 +15,9 @@ ROCm CLI ships as a single prebuilt binary. Platform support: For WSL2 setup and diagnosis, see [ROCm CLI on WSL2](wsl.md). -Live dashboard telemetry requires Linux or WSL2 (see +Live dashboard GPU/CPU/memory telemetry works on Windows too; the +serving-instance registry view is Linux/WSL2 only, since there's no +`rocmdashd` unix socket on Windows (see [Interactive interfaces](../getting-started.md#interactive-interfaces)). vLLM serving is Linux or WSL2 only (see [vLLM adapter](../engines/vllm.md)). From 9aab7b1864355047d4acdd1834213816b4c8eaa9 Mon Sep 17 00:00:00 2001 From: Juho Vainio Date: Thu, 22 Oct 2026 14:49:41 +0300 Subject: [PATCH 10/14] fix(dash-tui): show loading placeholders instead of zeroed/Unknown GPU hero MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before the first GPU telemetry snapshot arrives (or a tick returns no GPUs), the Home tab hero rendered "Unknown GPU" and 0%/0°C/0W values that are indistinguishable from real (broken) readings. Gate a new gpu_telemetry_pending() check on "no GPUs arrived yet" and swap in "Detecting GPU…" and "—" placeholders only for that case, leaving the already-tested legitimate-zero-VRAM-total behavior untouched. Signed-off-by: Juho Vainio --- crates/rocm-dash-tui/src/ui/tabs/home.rs | 88 +++++++++++++++++++++--- 1 file changed, 80 insertions(+), 8 deletions(-) diff --git a/crates/rocm-dash-tui/src/ui/tabs/home.rs b/crates/rocm-dash-tui/src/ui/tabs/home.rs index 7d07fb2ab..7db3561db 100644 --- a/crates/rocm-dash-tui/src/ui/tabs/home.rs +++ b/crates/rocm-dash-tui/src/ui/tabs/home.rs @@ -68,6 +68,15 @@ const fn node_load_label(hist_empty: bool, simulated: bool) -> &'static str { } } +/// True until the first GPU telemetry snapshot actually carries any GPUs — +/// distinct from a snapshot that arrived with a legitimately-zero field +/// (e.g. `vram_total_mb == 0`, see `vram_used_pct`). Drives the hero's +/// "still detecting" placeholders so startup/a stalled tick doesn't read as +/// "0% / 0°C / 0W", which looks broken rather than loading. +fn gpu_telemetry_pending(state: &AppState) -> bool { + state.latest.as_ref().is_none_or(|s| s.gpus.is_empty()) +} + /// Peak GPU utilization across the GPUs in a snapshot (the headline metric). fn snap_util(s: &rocm_dash_core::metrics::Snapshot) -> f64 { s.gpus @@ -122,9 +131,14 @@ fn mini_spark( } /// Hero card title in the mock's form: `Node throughput · N × MODEL · ROCm V`. -/// Falls back to `Unknown GPU` when no GPU model is detected, and drops the -/// count/ROCm segments that aren't known rather than inventing them. +/// Falls back to `Detecting GPU…` while no telemetry has arrived at all yet +/// (see `gpu_telemetry_pending`), or to `Unknown GPU` when telemetry arrived +/// but didn't carry a model name — and drops the count/ROCm segments that +/// aren't known rather than inventing them. fn node_throughput_title(state: &AppState) -> String { + if gpu_telemetry_pending(state) { + return "Node throughput · Detecting GPU…".to_string(); + } let info = state .latest .as_ref() @@ -275,7 +289,11 @@ fn draw_hero_left(f: &mut Frame, area: Rect, state: &AppState, theme: &Theme) { lh[0], ); let util = state.latest.as_ref().map_or(0.0, snap_util); - let util_label = format::pct(util as f32); + let util_label = if gpu_telemetry_pending(state) { + "—".to_string() + } else { + format::pct(util as f32) + }; let g = GradientGauge::new(util / 100.0) .stops(theme.ok, theme.warn, theme.err) .track_bg(theme.surface_2) @@ -362,6 +380,7 @@ fn draw_hero_right(f: &mut Frame, area: Rect, state: &AppState, theme: &Theme) { rh[0], ); let latest = state.latest.as_ref(); + let pending = gpu_telemetry_pending(state); let vram = latest.map_or(0.0, vram_used_pct); let temp = latest.map_or(0.0, |s| { s.gpus @@ -372,11 +391,28 @@ fn draw_hero_right(f: &mut Frame, area: Rect, state: &AppState, theme: &Theme) { let power: f64 = latest.map_or(0.0, |s| { s.gpus.iter().map(|g| f64::from(g.power_w)).sum::() }); + // While no GPU telemetry has arrived yet, show "—" rather than 0%/0°C/0W + // — those look like real (broken) readings rather than a loading state. + let vram_label = if pending { + "—".to_string() + } else { + format::pct(vram as f32) + }; + let temp_label = if pending { + "—".to_string() + } else { + format::celsius(temp as f32) + }; + let power_label = if pending { + "—".to_string() + } else { + format::watts(power as f32) + }; mini_spark( f, rh[1], "VRAM ", - &format::pct(vram as f32), + &vram_label, &history(state, vram_used_pct), false, theme, @@ -385,7 +421,7 @@ fn draw_hero_right(f: &mut Frame, area: Rect, state: &AppState, theme: &Theme) { f, rh[2], "TEMP ", - &format::celsius(temp as f32), + &temp_label, &history(state, |s| { s.gpus .iter() @@ -399,7 +435,7 @@ fn draw_hero_right(f: &mut Frame, area: Rect, state: &AppState, theme: &Theme) { f, rh[3], "POWER", - &format::watts(power as f32), + &power_label, // Power can exceed 100; scale to a nominal 1kW ceiling for the trace. &history(state, |s| { s.gpus.iter().map(|g| f64::from(g.power_w)).sum::() / 10.0 @@ -681,10 +717,46 @@ mod tests { assert!(t.starts_with("Node throughput · "), "prefix: {t}"); assert!(t.contains("Instinct MI355X"), "model: {t}"); assert!(t.contains("ROCm 6.2"), "rocm version: {t}"); - // No telemetry → Unknown GPU, no fabricated count/version. + // No telemetry at all yet → "detecting", not a bogus "Unknown GPU". let empty = AppState::new("t".into(), "default-dark".into()); let t2 = node_throughput_title(&empty); - assert_eq!(t2, "Node throughput · Unknown GPU"); + assert_eq!(t2, "Node throughput · Detecting GPU…"); + } + + #[test] + fn node_throughput_title_unknown_gpu_only_when_telemetry_present_but_unnamed() { + // Telemetry arrived (a GPU is present) but carried no model name — + // distinct from "nothing has arrived yet" and must still read + // "Unknown GPU", not the pending/detecting placeholder. + let mut s = AppState::new("t".into(), "default-dark".into()); + s.latest = Some(Snapshot { + host: SystemMetrics::default(), + gpus: vec![GpuMetrics { + device_id: "GPU0".into(), + vram_used_mb: 0, + vram_total_mb: 0, + gpu_utilization_pct: 0.0, + temperature_c: 0.0, + power_w: 0.0, + clock_mhz: None, + }], + gpu_system_info: None, + ..Default::default() + }); + assert_eq!(node_throughput_title(&s), "Node throughput · Unknown GPU"); + } + + #[test] + fn gpu_telemetry_pending_distinguishes_no_data_from_legitimate_zero() { + // Nothing has arrived yet → pending. + let empty = AppState::new("t".into(), "default-dark".into()); + assert!(gpu_telemetry_pending(&empty)); + // A snapshot with no GPUs at all → still pending. + let mut no_gpus = AppState::new("t".into(), "default-dark".into()); + no_gpus.latest = Some(Snapshot::default()); + assert!(gpu_telemetry_pending(&no_gpus)); + // A real GPU present (even with legitimately-zero fields) → not pending. + assert!(!gpu_telemetry_pending(&state_with_gpu())); } #[test] From cbe07c9d826eae9156c04202144e00e4af9b1f36 Mon Sep 17 00:00:00 2001 From: Juho Vainio Date: Thu, 22 Oct 2026 18:34:41 +0300 Subject: [PATCH 11/14] fix(dash-collectors,dash-tui): resolve round 2 review findings for Windows GPU metrics winperf.rs: - sum-then-max UTIL aggregation across engines via device_key/engine_type keying, fixing under-reported utilization on multi-engine devices - WMI_ADAPTER_RAM_CAP_THRESHOLD heuristic and decoupled ADLX total-VRAM read from counter-query success, fixing 0/capped-near-4GiB VRAM - counter_samples_missing_warning surfaces a locale-mismatch warning when WMI finds an adapter but perf counters return nothing, plumbed through snapshot()'s new Vec return - BTreeSet-based stable device ordering, SetDefaultDllDirectories return-value check, corrected test fixtures, new tests ui/tabs/home.rs: - split gpu_telemetry_pending (no snapshot yet) from gpu_data_unavailable (snapshot arrived but empty), so the hero no longer sticks on "Detecting GPU..." once an empty snapshot arrives - extend "-" placeholder logic to zero vram_total/temp/power app/mod.rs: - extract push_history_and_latest shared helper used by push_snapshot and push_host_gpu_snapshot - add coverage test for push_host_gpu_snapshot's instances/has_received behavior client.rs: - fix stale doc comment (ClientMsg::HostGpuSnapshot, not Event::Snapshot) - plumb winperf's new warnings through to the Snapshot on both the Ok and Err arms - sample host.tick() before gpu.snapshot() so the dashboard's own CPU% doesn't include the powershell.exe spawn/wait window registry_probe.rs, Cargo.toml: correct stale/misleading comments dash.feature: add dash-37 no-GPU-telemetry Linux e2e scenario README.md: correct Windows feature-parity claims Addresses review 5615012813 on PR #715. Signed-off-by: Juho Vainio --- README.md | 14 +- crates/rocm-dash-collectors/src/winperf.rs | 306 ++++++++++++++++----- crates/rocm-dash-tui/Cargo.toml | 5 +- crates/rocm-dash-tui/src/app/mod.rs | 60 +++- crates/rocm-dash-tui/src/client.rs | 31 ++- crates/rocm-dash-tui/src/registry_probe.rs | 6 +- crates/rocm-dash-tui/src/ui/mod.rs | 8 +- crates/rocm-dash-tui/src/ui/tabs/home.rs | 138 ++++++++-- tests/e2e-cucumber/features/dash.feature | 19 ++ tests/e2e-cucumber/tests/e2e/dash_steps.rs | 32 +++ 10 files changed, 503 insertions(+), 116 deletions(-) diff --git a/README.md b/README.md index 5b841c432..9e5fcc175 100644 --- a/README.md +++ b/README.md @@ -38,9 +38,10 @@ for Lemonade and vLLM. For WSL2 setup and diagnosis, see [docs/wsl.md](docs/wsl.md). -Live dashboard telemetry requires Linux or WSL2 (see -[Interactive interfaces](#interactive-interfaces)). vLLM serving is Linux or WSL2 -only (see [docs/vllm.md](docs/vllm.md)). +Live dashboard telemetry works on Windows too, but shows only GPU/CPU/memory — +the serving-instance registry view needs the `rocmdashd` unix socket, so it's +Linux/WSL2 only (see [Interactive interfaces](#interactive-interfaces)). vLLM +serving is Linux or WSL2 only (see [docs/vllm.md](docs/vllm.md)). The minimum supported Linux release, native or under WSL2, is Ubuntu 24.04. On other distributions the equivalent requirement is glibc 2.38 with @@ -226,9 +227,10 @@ five tabs (switch with `Tab`/`Shift+Tab` or number keys `1`–`5`): | **Observe** | Live GPU utilization, instances, and benchmark telemetry | | **Chat** | Assistant chat backed by a local server or configured provider | -Live mode reads telemetry over a Unix domain socket, so it requires Linux or -WSL2. Use `rocm dash --demo` for a synthetic session that runs anywhere without a -GPU or daemon. +Live mode works on Windows too, but shows only GPU/CPU/memory — the +serving-instance registry needs the Unix domain socket telemetry, so that view +is Linux/WSL2 only. Use `rocm dash --demo` for a synthetic session that runs +anywhere without a GPU or daemon. ## Configure ROCm and serve a model diff --git a/crates/rocm-dash-collectors/src/winperf.rs b/crates/rocm-dash-collectors/src/winperf.rs index 83fbed1e0..8e2a12152 100644 --- a/crates/rocm-dash-collectors/src/winperf.rs +++ b/crates/rocm-dash-collectors/src/winperf.rs @@ -17,7 +17,7 @@ //! shellout fails cleanly (`powershell.exe` not found) on other platforms, //! same as `amd_smi.rs` when `amd-smi` isn't installed. -use std::collections::BTreeMap; +use std::collections::{BTreeMap, BTreeSet}; use std::io; use std::time::Duration; @@ -59,7 +59,11 @@ impl WinPerfCollector { /// script already emits both sections) — calling `metrics()` and /// `system_info()` separately each tick would shell out twice for the /// same output, doubling exposure to spawn latency for nothing. - pub async fn snapshot(&self) -> io::Result<(Vec, GpuSystemInfo)> { + /// + /// The returned `Vec` surfaces non-fatal problems the caller + /// should show the user (e.g. a locale mismatch, see + /// `counter_samples_missing_warning`) without failing the whole tick. + pub async fn snapshot(&self) -> io::Result<(Vec, GpuSystemInfo, Vec)> { let out = Self::run_script().await?; // adlx_readings() calls into amdadlx64.dll via FFI, which blocks on // driver I/O — running it directly here would stall a tokio worker @@ -67,7 +71,11 @@ impl WinPerfCollector { let adlx = task::spawn_blocking(adlx_readings) .await .unwrap_or_default(); - Ok((parse_metrics(&out, &adlx), parse_system_info(&out))) + let metrics = parse_metrics(&out, &adlx); + let warnings = counter_samples_missing_warning(&out, &metrics) + .into_iter() + .collect(); + Ok((metrics, parse_system_info(&out), warnings)) } async fn run_script() -> io::Result { @@ -94,6 +102,21 @@ impl WinPerfCollector { } } +/// `Win32_VideoController.AdapterRAM` is a 32-bit WMI field: a card with real +/// VRAM at or beyond its ~4GiB ceiling gets silently truncated/capped rather +/// than reporting its true size. A value this close to that ceiling is far +/// more likely a truncated sentinel than an improbably-exact ~4GiB card, so +/// treat it as unknown (0) rather than report a number that's probably wrong. +const WMI_ADAPTER_RAM_CAP_THRESHOLD: u64 = 4_000_000_000; + +const fn wmi_vram_total_mb(ram_bytes: u64) -> u64 { + if ram_bytes >= WMI_ADAPTER_RAM_CAP_THRESHOLD { + 0 + } else { + ram_bytes / (1024 * 1024) + } +} + /// `GPU Engine` instances are per-process-per-engine /// (`pid_1234_luid_0x..._0x..._phys_0_eng_0_engtype_3d`); `GPU Adapter /// Memory` instances are per-adapter only (`luid_0x..._0x..._phys_0`, no @@ -105,6 +128,15 @@ fn device_key(instance: &str) -> &str { instance.split("_phys_").next().unwrap_or(instance) } +/// `GPU Engine` instance names end in `..._eng_N_engtype_TYPE` (e.g. `3d`, +/// `copy`, `videodecode`); this is the engine-type suffix, distinct per +/// engine but shared across every process using that same engine. Pairs +/// with `device_key` in `parse_metrics` to key utilization by (device, +/// engine) rather than by device alone. +fn engine_type(instance: &str) -> &str { + instance.rsplit("_engtype_").next().unwrap_or(instance) +} + /// Builds one `GpuMetrics` per distinct adapter LUID seen in the counter /// output. Utilization is the max across that adapter's engines (3D/copy/ /// video/etc.) — the same "overall GPU" reading Task Manager shows. @@ -120,20 +152,32 @@ fn device_key(instance: &str) -> &str { /// With more than one real adapter, correlating a `GPU Adapter Memory` LUID /// to a specific `Win32_VideoController` entry isn't reliably possible /// without vendor-specific APIs, so `vram_total_mb` is left at 0 (every TUI -/// consumer already guards `vram_total_mb > 0` before dividing). -/// ponytail: single-adapter case (the realistic target: an integrated AMD -/// APU) gets an exact total; multi-GPU hosts degrade to 0 instead of a wrong -/// number. Upgrade path: DXGI/LUID-based adapter correlation if multi-GPU -/// Windows hosts become a real target. +/// consumer already guards `vram_total_mb > 0` before dividing). The +/// single-adapter case — the realistic target, an integrated AMD APU — gets +/// an exact total; multi-GPU hosts degrade to 0 instead of a wrong number. +/// Upgrade path if multi-GPU Windows hosts become a real target: DXGI/LUID- +/// based adapter correlation. /// /// `adlx` is a parameter (rather than this function calling `adlx_readings()` /// itself) so tests can exercise the ADLX-matching/VRAM-fallback logic below /// with fixed readings — `adlx_readings()`'s real FFI chain is Windows+driver /// only and not unit-testable. fn parse_metrics(raw: &str, adlx: &[AdlxReading]) -> Vec { - let mut util: BTreeMap<&str, f32> = BTreeMap::new(); + // Keyed by (device, engine type): `GPU Engine` instances are + // per-process-per-engine, so two processes both drawing on the 3D engine + // each contribute their own share of that engine's capacity and must be + // summed, matching Task Manager. Different engine types (3D vs. copy vs. + // video) run concurrently on separate hardware, so those are maxed + // together below instead of summed. + let mut engine_util: BTreeMap<(&str, &str), f32> = BTreeMap::new(); let mut vram_mb: BTreeMap<&str, u64> = BTreeMap::new(); - let mut order: Vec<&str> = Vec::new(); + // A `BTreeSet` rather than first-seen order: `powershell.exe`'s adapter + // enumeration order isn't guaranteed stable between ticks, which would + // otherwise make `win-gpu-{i}` swap which physical device it names from + // one snapshot to the next (mixing per-device history in the TUI). + // Sorting by the LUID-derived key instead keeps each device's index + // stable for as long as its LUID is (i.e. until reboot/driver reload). + let mut seen: BTreeSet<&str> = BTreeSet::new(); for line in raw.lines() { let mut parts = line.splitn(3, '\t'); @@ -145,16 +189,13 @@ fn parse_metrics(raw: &str, adlx: &[AdlxReading]) -> Vec { continue; // GPU inventory lines are handled separately by parse_adapters. } let key = device_key(instance); - if !util.contains_key(key) && !vram_mb.contains_key(key) { - order.push(key); - } + seen.insert(key); match kind { "UTIL" => { let pct: f32 = value.trim().parse().unwrap_or(0.0); - let entry = util.entry(key).or_insert(0.0); - if pct > *entry { - *entry = pct; - } + *engine_util + .entry((key, engine_type(instance))) + .or_insert(0.0) += pct; } "VRAM" => { let bytes: f64 = value.trim().parse().unwrap_or(0.0); @@ -164,9 +205,17 @@ fn parse_metrics(raw: &str, adlx: &[AdlxReading]) -> Vec { } } + let mut util: BTreeMap<&str, f32> = BTreeMap::new(); + for (&(device, _engine), &pct) in &engine_util { + let entry = util.entry(device).or_insert(0.0); + if pct > *entry { + *entry = pct; + } + } + let adapters = parse_adapters(raw); if let [adapter] = adapters.as_slice() { - if order.is_empty() { + if seen.is_empty() { return Vec::new(); } let vram_used_mb: u64 = vram_mb.values().sum(); @@ -187,8 +236,8 @@ fn parse_metrics(raw: &str, adlx: &[AdlxReading]) -> Vec { let vram_total_mb = match adlx { [reading] => reading .total_vram_mb - .map_or(adapter.ram_bytes / (1024 * 1024), u64::from), - _ => adapter.ram_bytes / (1024 * 1024), + .map_or_else(|| wmi_vram_total_mb(adapter.ram_bytes), u64::from), + _ => wmi_vram_total_mb(adapter.ram_bytes), }; return vec![GpuMetrics { device_id: "win-gpu-0".to_string(), @@ -207,8 +256,7 @@ fn parse_metrics(raw: &str, adlx: &[AdlxReading]) -> Vec { // ADLX-absent defaults rather than risk reporting one GPU's temp/power/ // clock under a different GPU's device_id. let (temperature_c, power_w, clock_mhz) = apply_adlx(None); - order - .into_iter() + seen.into_iter() .enumerate() .map(|(i, key)| GpuMetrics { device_id: format!("win-gpu-{i}"), @@ -256,25 +304,36 @@ fn apply_adlx(reading: Option<&AdlxReading>) -> (f32, f32, Option) { /// `amdadlx64.dll` isn't found, so a total init failure can just bail to an /// empty Vec via `.ok()?` — same graceful-degrade shape as the rest of this /// file. A single GPU's metrics query failing, though, must NOT drop that -/// GPU's slot (a `filter_map` here would shift every later GPU's reading one -/// position to the left, silently misattributing it in `parse_metrics`'s -/// index-based correlation) — it degrades to a zeroed `AdlxReading` instead, -/// keeping one slot per GPU. +/// GPU's slot (a `filter_map` here would silently change the reading count, +/// which `parse_metrics`'s single-reading guard relies on to decide whether +/// an ADLX reading can be trusted at all) — it degrades to a zeroed +/// `AdlxReading` instead, keeping one slot per GPU. #[cfg(windows)] fn adlx_readings() -> Vec { // `adlx::helper::AdlxHelper::new()` loads `amdadlx64.dll` by bare name // (`libloading::Library::new("amdadlx64.dll")`, no search-order - // hardening, no way to pass flags into the vendored crate). Restricting - // the process-wide default DLL search path to System32 closes the - // planted-DLL-in-PATH/cwd hijack vector for that unflagged load. Must - // run before the first `AdlxHelper::new()` call; `Once` makes repeated - // per-tick calls to this function a no-op after the first. + // hardening, no way to pass flags into the `adlx` crate). Restricting + // the process-wide default DLL search path to System32 — this call + // affects every subsequent bare-name `LoadLibrary` in the whole process, + // not just `adlx`'s — closes the planted-DLL-in-PATH/cwd hijack vector + // for that unflagged load. Must run before the first `AdlxHelper::new()` + // call; `Once` makes repeated per-tick calls to this function a no-op + // after the first. static HARDEN_DLL_SEARCH_PATH: std::sync::Once = std::sync::Once::new(); #[allow(unsafe_code)] // Win32 FFI HARDEN_DLL_SEARCH_PATH.call_once(|| unsafe { - windows_sys::Win32::System::LibraryLoader::SetDefaultDllDirectories( + // Failure here (`FALSE`) means the process keeps its default search + // order (which can include the cwd) rather than System32-only; still + // attempt the `AdlxHelper` load afterwards; a stricter search path is + // defense in depth, not a precondition for ADLX to work. + let hardened = windows_sys::Win32::System::LibraryLoader::SetDefaultDllDirectories( windows_sys::Win32::System::LibraryLoader::LOAD_LIBRARY_SEARCH_SYSTEM32, ); + if hardened == 0 { + tracing::warn!( + "SetDefaultDllDirectories failed; amdadlx64.dll load falls back to the default search order" + ); + } }); (|| -> Option> { @@ -284,19 +343,25 @@ fn adlx_readings() -> Vec { let services = system.performance_monitoring_services().ok()?; Some( gpus.iter() - .map(|gpu| match services.current_gpu_metrics(&gpu) { - Ok(m) => AdlxReading { - temperature_c: m.temperature().unwrap_or(0.0) as f32, - power_w: m.power().unwrap_or(0.0) as f32, - clock_mhz: m.clock_speed().ok().map(|c| c as f32), - total_vram_mb: gpu.total_vram().ok(), - }, - Err(_) => AdlxReading { - temperature_c: 0.0, - power_w: 0.0, - clock_mhz: None, - total_vram_mb: None, - }, + .map(|gpu| { + // Independent of current_gpu_metrics: a transient + // metrics-query failure shouldn't also discard an + // otherwise-good VRAM-size reading. + let total_vram_mb = gpu.total_vram().ok(); + match services.current_gpu_metrics(&gpu) { + Ok(m) => AdlxReading { + temperature_c: m.temperature().unwrap_or(0.0) as f32, + power_w: m.power().unwrap_or(0.0) as f32, + clock_mhz: m.clock_speed().ok().map(|c| c as f32), + total_vram_mb, + }, + Err(_) => AdlxReading { + temperature_c: 0.0, + power_w: 0.0, + clock_mhz: None, + total_vram_mb, + }, + } }) .collect(), ) @@ -342,6 +407,27 @@ fn parse_system_info(raw: &str) -> GpuSystemInfo { } } +/// `Win32_VideoController` (WMI) and `Get-Counter` (the perf-counter +/// subsystem) are independent queries in the same script: WMI finding an +/// adapter but every counter query coming back empty isn't "no GPU", it's +/// the counters failing silently. The likeliest cause is `Get-Counter`'s +/// English-only counter paths (`\GPU Engine(*)\...`) not matching on a +/// localized Windows install — the script's `-ErrorAction +/// SilentlyContinue` swallows that mismatch rather than erroring, so this +/// is the only signal left to surface it to the user instead of silently +/// showing an empty GPU panel. +fn counter_samples_missing_warning(raw: &str, metrics: &[GpuMetrics]) -> Option { + if metrics.is_empty() && !parse_adapters(raw).is_empty() { + Some( + "Windows GPU utilization/VRAM counters returned no samples (possible non-English \ + Windows locale — Get-Counter counter paths are localized)" + .to_string(), + ) + } else { + None + } +} + #[cfg(test)] mod tests { use super::*; @@ -372,35 +458,54 @@ mod tests { assert_eq!(info.physical_gpu_count, 1); } - #[test] - fn multiple_adapters_leave_vram_total_zero() { - let two_adapters = format!( - "{SAMPLE}\nUTIL\tluid_0x00000000_0x0000cafe_phys_0_eng_0_engtype_3d\t1.0\nVRAM\tluid_0x00000000_0x0000cafe_phys_0\t1048576\nGPU\tOther GPU\t4294967296\t1.0" - ); - let metrics = parse_metrics(&two_adapters, &[]); - assert_eq!(metrics.len(), 2); - assert!(metrics.iter().all(|g| g.vram_total_mb == 0)); - } - #[test] fn multiple_processes_on_one_adapter_collapse_to_one_device() { - // Two different pids hitting the same adapter's engines, plus a + // Two different pids hitting the same adapter's *same* engine, plus a // *second*, unrelated real adapter. With only one real adapter (as // the original fixture had), the single-real-adapter collapse rescue // below would also force everything to one device, masking whether // this is actually testing `device_key`'s pid-prefix stripping; with // two real adapters that rescue never fires (its pattern match - // requires exactly one), isolating the two code paths. - let sample = "UTIL\tpid_111_luid_0x00000000_0x0002ff2a_phys_0_eng_0_engtype_3d\t0.682\nUTIL\tpid_222_luid_0x00000000_0x0002ff2a_phys_0_eng_0_engtype_3d\t0.514\nVRAM\tluid_0x00000000_0x0002ff2a_phys_0\t902787072\nUTIL\tluid_0x00000000_0x0000cafe_phys_0_eng_0_engtype_3d\t0.1\nVRAM\tluid_0x00000000_0x0000cafe_phys_0\t1048576\nGPU\tAMD Radeon 780M Graphics\t1073741824\t32.0.31041.1004\nGPU\tOther GPU\t4294967296\t1.0"; + // requires exactly one), isolating the two code paths. Both pids + // share the `3d` engine, so their utilization sums (Task Manager + // semantics) rather than taking the max of the two. + let sample = "UTIL\tpid_111_luid_0x00000000_0x0002ff2a_phys_0_eng_0_engtype_3d\t0.682\nUTIL\tpid_222_luid_0x00000000_0x0002ff2a_phys_0_eng_0_engtype_3d\t0.514\nVRAM\tluid_0x00000000_0x0002ff2a_phys_0\t902787072\nUTIL\tluid_0x00000000_0x0000cafe_phys_0_eng_0_engtype_3d\t0.1\nVRAM\tluid_0x00000000_0x0000cafe_phys_0\t1048576\nGPU\tAMD Radeon 780M Graphics\t1073741824\t32.0.31041.1004\nGPU\tOther GPU\t4293918720\t1.0"; let metrics = parse_metrics(sample, &[]); assert_eq!(metrics.len(), 2); - let gpu = &metrics[0]; - assert!((gpu.gpu_utilization_pct - 0.682).abs() < 0.001); - assert_eq!(gpu.vram_used_mb, 902_787_072 / (1024 * 1024)); + // Devices are keyed by sorted LUID string, not first-seen order (see + // `parse_metrics`'s `BTreeSet` comment), so find the device under + // test by its VRAM reading rather than assuming an index. + let gpu = metrics + .iter() + .find(|g| g.vram_used_mb == 902_787_072 / (1024 * 1024)) + .expect("the pid-prefixed adapter's metrics"); + assert!((gpu.gpu_utilization_pct - 1.196).abs() < 0.001); // Multi-real-adapter case: vram_total_mb always 0, no LUID correlation. assert_eq!(gpu.vram_total_mb, 0); } + #[test] + fn utilization_across_different_engine_types_takes_max_not_sum() { + // Same pid, same device, two *different* engine types running + // concurrently on separate hardware — these must be maxed, not + // summed, unlike same-engine readings from different processes. + let sample = "UTIL\tpid_1_luid_0x00000000_0x0002ff2a_phys_0_eng_0_engtype_3d\t90.0\nUTIL\tpid_1_luid_0x00000000_0x0002ff2a_phys_0_eng_1_engtype_copy\t20.0\nVRAM\tluid_0x00000000_0x0002ff2a_phys_0\t1048576\nGPU\tAMD Radeon 780M Graphics\t1073741824\t32.0.31041.1004"; + let metrics = parse_metrics(sample, &[]); + assert_eq!(metrics.len(), 1); + assert!((metrics[0].gpu_utilization_pct - 90.0).abs() < 0.001); + } + + #[test] + fn summed_engine_utilization_above_100_percent_clamps() { + // Several processes hammering the same engine can sum past 100% of + // a single engine's nominal capacity; the reported utilization must + // still clamp at 100, matching Task Manager's own ceiling. + let sample = "UTIL\tpid_1_luid_0x00000000_0x0002ff2a_phys_0_eng_0_engtype_3d\t60.0\nUTIL\tpid_2_luid_0x00000000_0x0002ff2a_phys_0_eng_0_engtype_3d\t70.0\nVRAM\tluid_0x00000000_0x0002ff2a_phys_0\t1048576\nGPU\tAMD Radeon 780M Graphics\t1073741824\t32.0.31041.1004"; + let metrics = parse_metrics(sample, &[]); + assert_eq!(metrics.len(), 1); + assert!((metrics[0].gpu_utilization_pct - 100.0).abs() < 0.001); + } + #[test] fn single_real_adapter_sums_vram_used_across_distinct_luids() { // Two *distinct* non-zero per-LUID VRAM readings on the one real @@ -421,8 +526,27 @@ mod tests { // -ErrorAction silently swallows a localized-path mismatch) — must // not fabricate a zeroed phantom device for it. let sample = "GPU\tAMD Radeon 780M Graphics\t1073741824\t32.0.31041.1004"; - assert!(parse_metrics(sample, &[]).is_empty()); + let metrics = parse_metrics(sample, &[]); + assert!(metrics.is_empty()); assert_eq!(parse_system_info(sample).physical_gpu_count, 1); + // The empty result must not be silently indistinguishable from "no + // GPU" — the caller needs a warning to show instead. + assert!(counter_samples_missing_warning(sample, &metrics).is_some()); + } + + #[test] + fn counter_samples_missing_warning_is_none_when_metrics_present() { + let sample = "UTIL\tluid_0x00000000_0x0000b3a4_phys_0_eng_0_engtype_3d\t1.0\nVRAM\tluid_0x00000000_0x0000b3a4_phys_0\t1048576\nGPU\tDiscrete GPU\t3221225472\t1.0"; + let metrics = parse_metrics(sample, &[]); + assert!(!metrics.is_empty()); + assert!(counter_samples_missing_warning(sample, &metrics).is_none()); + } + + #[test] + fn counter_samples_missing_warning_is_none_when_no_adapters_at_all() { + // No `Win32_VideoController` line either — genuinely no GPU, not a + // counter-query failure, so no warning is warranted. + assert!(counter_samples_missing_warning("", &[]).is_none()); } #[test] @@ -443,10 +567,10 @@ mod tests { #[test] #[allow(clippy::float_cmp)] fn single_adapter_prefers_adlx_vram_over_wmi_truncated_value() { - // WMI's AdapterRAM here (4294967296 = exactly 4GiB) is the classic - // 32-bit-field overflow case; ADLX's own query reports the real - // 8GiB so parse_metrics should prefer it. - let sample = "UTIL\tluid_0x00000000_0x0000b3a4_phys_0_eng_0_engtype_3d\t1.0\nVRAM\tluid_0x00000000_0x0000b3a4_phys_0\t1048576\nGPU\tDiscrete GPU\t4294967296\t1.0"; + // WMI's AdapterRAM here (4293918720, just under the 4GiB cap + // threshold) is the classic 32-bit-field truncation case; ADLX's own + // query reports the real 8GiB so parse_metrics should prefer it. + let sample = "UTIL\tluid_0x00000000_0x0000b3a4_phys_0_eng_0_engtype_3d\t1.0\nVRAM\tluid_0x00000000_0x0000b3a4_phys_0\t1048576\nGPU\tDiscrete GPU\t4293918720\t1.0"; let adlx = [AdlxReading { temperature_c: 50.0, power_w: 10.0, @@ -464,7 +588,7 @@ mod tests { #[test] fn multiple_adapters_ignore_adlx_vram_and_leave_total_zero() { let two_adapters = format!( - "{SAMPLE}\nUTIL\tluid_0x00000000_0x0000cafe_phys_0_eng_0_engtype_3d\t1.0\nVRAM\tluid_0x00000000_0x0000cafe_phys_0\t1048576\nGPU\tOther GPU\t4294967296\t1.0" + "{SAMPLE}\nUTIL\tluid_0x00000000_0x0000cafe_phys_0_eng_0_engtype_3d\t1.0\nVRAM\tluid_0x00000000_0x0000cafe_phys_0\t1048576\nGPU\tOther GPU\t4293918720\t1.0" ); let adlx = [AdlxReading { temperature_c: 50.0, @@ -488,7 +612,7 @@ mod tests { // started correlating by list position again, this is exactly the // shape that would make it look like it "worked" by coincidence. let two_adapters = format!( - "{SAMPLE}\nUTIL\tluid_0x00000000_0x0000cafe_phys_0_eng_0_engtype_3d\t1.0\nVRAM\tluid_0x00000000_0x0000cafe_phys_0\t1048576\nGPU\tOther GPU\t4294967296\t1.0" + "{SAMPLE}\nUTIL\tluid_0x00000000_0x0000cafe_phys_0_eng_0_engtype_3d\t1.0\nVRAM\tluid_0x00000000_0x0000cafe_phys_0\t1048576\nGPU\tOther GPU\t4293918720\t1.0" ); let two_adlx = [ AdlxReading { @@ -517,7 +641,11 @@ mod tests { #[test] #[allow(clippy::float_cmp)] fn single_adapter_falls_back_to_wmi_vram_when_adlx_reading_count_mismatches() { - let sample = "UTIL\tluid_0x00000000_0x0000b3a4_phys_0_eng_0_engtype_3d\t1.0\nVRAM\tluid_0x00000000_0x0000b3a4_phys_0\t1048576\nGPU\tDiscrete GPU\t4294967296\t1.0"; + // WMI's AdapterRAM here is also at the truncation-cap threshold, so + // the fallback value itself degrades to 0 per `wmi_vram_total_mb` — + // see `single_adapter_falls_back_to_wmi_vram_below_cap_threshold` + // for the case where the WMI fallback value is actually trusted. + let sample = "UTIL\tluid_0x00000000_0x0000b3a4_phys_0_eng_0_engtype_3d\t1.0\nVRAM\tluid_0x00000000_0x0000b3a4_phys_0\t1048576\nGPU\tDiscrete GPU\t4293918720\t1.0"; let two_adlx = [ AdlxReading { temperature_c: 50.0, @@ -534,12 +662,50 @@ mod tests { ]; let metrics = parse_metrics(sample, &two_adlx); assert_eq!(metrics.len(), 1); - assert_eq!(metrics[0].vram_total_mb, 4_294_967_296 / (1024 * 1024)); + assert_eq!(metrics[0].vram_total_mb, 0); assert_eq!(metrics[0].temperature_c, 0.0); assert_eq!(metrics[0].power_w, 0.0); assert_eq!(metrics[0].clock_mhz, None); } + #[test] + fn single_adapter_falls_back_to_wmi_vram_below_cap_threshold() { + // No ADLX reading at all, and the WMI value is comfortably below the + // truncation-cap threshold, so it's trusted as-is. + let sample = "UTIL\tluid_0x00000000_0x0000b3a4_phys_0_eng_0_engtype_3d\t1.0\nVRAM\tluid_0x00000000_0x0000b3a4_phys_0\t1048576\nGPU\tDiscrete GPU\t3221225472\t1.0"; + let metrics = parse_metrics(sample, &[]); + assert_eq!(metrics.len(), 1); + assert_eq!(metrics[0].vram_total_mb, 3_221_225_472 / (1024 * 1024)); + } + + #[test] + fn single_adapter_adlx_reading_with_no_total_vram_falls_back_to_wmi() { + // The ADLX reading matches (exactly one, for exactly one real + // adapter) but its own TotalVRAM query failed (`None`) — still + // falls back to the WMI value rather than reporting 0. + let sample = "UTIL\tluid_0x00000000_0x0000b3a4_phys_0_eng_0_engtype_3d\t1.0\nVRAM\tluid_0x00000000_0x0000b3a4_phys_0\t1048576\nGPU\tDiscrete GPU\t3221225472\t1.0"; + let adlx = [AdlxReading { + temperature_c: 50.0, + power_w: 10.0, + clock_mhz: Some(1800.0), + total_vram_mb: None, + }]; + let metrics = parse_metrics(sample, &adlx); + assert_eq!(metrics.len(), 1); + assert_eq!(metrics[0].vram_total_mb, 3_221_225_472 / (1024 * 1024)); + } + + #[test] + fn device_key_without_luid_substring_falls_back_to_whole_instance() { + // Defensive fallback for an unexpected instance-name shape (no + // `luid_` segment at all) — must not panic, and should still key + // consistently rather than silently merging unrelated instances. + assert_eq!( + device_key("some_unusual_instance_name"), + "some_unusual_instance_name" + ); + } + #[test] fn empty_output_yields_no_metrics() { assert!(parse_metrics("", &[]).is_empty()); diff --git a/crates/rocm-dash-tui/Cargo.toml b/crates/rocm-dash-tui/Cargo.toml index 04d580759..1631f865c 100644 --- a/crates/rocm-dash-tui/Cargo.toml +++ b/crates/rocm-dash-tui/Cargo.toml @@ -16,9 +16,10 @@ path = "src/lib.rs" [dependencies] rocm-dash-core = { path = "../rocm-dash-core" } -# Windows-only: the in-process metrics ticker in `client.rs` reuses +# Unconditional dependency, but only exercised by `client.rs`'s +# `#[cfg(windows)]` arm: the in-process metrics ticker there reuses # `HostCollector`/`WinPerfCollector` instead of a socket (no rocmdashd on -# Windows yet). Builds on every platform like the rest of the crate. +# Windows yet). rocm-dash-collectors = { path = "../rocm-dash-collectors" } rocm-deps = { path = "../rocm-deps" } # Unified dashboard TUI base: ratatui 0.30 + crossterm 0.28. This is the only diff --git a/crates/rocm-dash-tui/src/app/mod.rs b/crates/rocm-dash-tui/src/app/mod.rs index 6680c523b..42a6909d4 100644 --- a/crates/rocm-dash-tui/src/app/mod.rs +++ b/crates/rocm-dash-tui/src/app/mod.rs @@ -1484,12 +1484,7 @@ impl AppState { self.instances .insert(inst.container_id.clone(), inst.clone()); } - if self.history.len() == HISTORY_CAP { - self.history.pop_front(); - } - self.history.push_back(snap.clone()); - self.latest = Some(snap); - self.clamp_selectors(); + self.push_history_and_latest(snap); } /// Windows sibling of [`push_snapshot`](Self::push_snapshot) for @@ -1502,6 +1497,14 @@ impl AppState { /// blind the issue #145 quit-confirm prompt to models actually being /// served — see `has_live_instance_with`'s Windows note. pub(crate) fn push_host_gpu_snapshot(&mut self, snap: Snapshot) { + self.push_history_and_latest(snap); + } + + /// Shared tail of [`push_snapshot`](Self::push_snapshot) and + /// [`push_host_gpu_snapshot`](Self::push_host_gpu_snapshot): the + /// history/latest bookkeeping both share, independent of how (or + /// whether) `instances` gets updated. + fn push_history_and_latest(&mut self, snap: Snapshot) { if self.history.len() == HISTORY_CAP { self.history.pop_front(); } @@ -3449,6 +3452,51 @@ mod tests { ); } + #[test] + fn push_host_gpu_snapshot_leaves_instances_and_has_received_snapshot_untouched() { + // Mirrors push_snapshot's issue #145 authority handoff, but for the + // Windows in-process path: a HostGpuSnapshot must still land + // host/GPU metrics for charts/history, while leaving `instances` and + // `has_received_snapshot` exactly as they were — see + // `push_host_gpu_snapshot`'s doc comment for why. + let mut s = st(); + s.instances.insert( + "id0".to_string(), + rocm_dash_core::metrics::Instance { + container_id: "id0".to_string(), + ..Default::default() + }, + ); + assert!(!s.has_received_snapshot); + + let snap = Snapshot { + warnings: vec!["gpu-warning".to_string()], + instances: vec![rocm_dash_core::metrics::Instance { + container_id: "phantom".to_string(), + ..Default::default() + }], + ..Snapshot::default() + }; + s.push_host_gpu_snapshot(snap); + + assert!( + !s.has_received_snapshot, + "a HostGpuSnapshot must never flip has_received_snapshot" + ); + assert_eq!( + s.instances.len(), + 1, + "instances must stay exactly as seeded, ignoring the snapshot's own (always-empty-in-practice) instances" + ); + assert!(s.instances.contains_key("id0")); + assert_eq!( + s.latest.as_ref().map(|s| s.warnings.clone()), + Some(vec!["gpu-warning".to_string()]), + "host/GPU metrics must still land for charts/history" + ); + assert_eq!(s.history.len(), 1); + } + #[test] fn has_live_instance_is_always_false_under_simulated() { // `--demo`/`--replay` synthesize `Instance { status: Running, .. }` diff --git a/crates/rocm-dash-tui/src/client.rs b/crates/rocm-dash-tui/src/client.rs index 843d4fdd8..39540e84a 100644 --- a/crates/rocm-dash-tui/src/client.rs +++ b/crates/rocm-dash-tui/src/client.rs @@ -193,9 +193,10 @@ async fn connect_and_run(connect: &str, tx: UnboundedSender) -> anyho /// Windows has no rocmdashd socket yet, so this builds `Snapshot`s in-process /// instead of connecting anywhere: `HostCollector` (already cross-platform) /// for CPU/memory, `WinPerfCollector` (OS perf counters, no AMD tool needed) -/// for GPU. Same `ClientMsg` sequence the unix arm produces -/// (`Connected` then a stream of `Event::Snapshot`), so the app loop doesn't -/// need to know which platform it's on. +/// for GPU. Sends `Connected` like the unix arm, but then a stream of +/// `ClientMsg::HostGpuSnapshot` rather than `Event::Snapshot` — deliberately +/// not routed through `Event::Snapshot`, to avoid disturbing the "quit while +/// serving" confirmation prompt (see `AppState::push_host_gpu_snapshot`). #[cfg(windows)] async fn connect_and_run(connect: &str, tx: UnboundedSender) -> anyhow::Result<()> { use rocm_dash_collectors::host::HostCollector; @@ -222,17 +223,29 @@ async fn connect_and_run(connect: &str, tx: UnboundedSender) -> anyho loop { ticker.tick().await; - let (gpus, gpu_system_info) = gpu.snapshot().await.unwrap_or_else(|e| { - warn!(error = %e, "windows GPU perf-counter query failed"); - (Vec::new(), GpuSystemInfo::default()) - }); + // Sampled before the GPU query, not after: `host.tick()`'s CPU + // reading would otherwise include the window spent spawning and + // waiting on `powershell.exe` for the GPU counters, inflating the + // dashboard's own CPU% with its own collection cost. + let host_metrics = host.tick(); + let (gpus, gpu_system_info, warnings) = match gpu.snapshot().await { + Ok(v) => v, + Err(e) => { + warn!(error = %e, "windows GPU perf-counter query failed"); + ( + Vec::new(), + GpuSystemInfo::default(), + vec![format!("windows GPU perf-counter query failed: {e}")], + ) + } + }; let snapshot = Snapshot { timestamp: chrono::Utc::now(), - host: host.tick(), + host: host_metrics, gpus, gpu_system_info: Some(gpu_system_info), instances: Vec::new(), - warnings: Vec::new(), + warnings, }; if tx .send(ClientMsg::HostGpuSnapshot(Box::new(snapshot))) diff --git a/crates/rocm-dash-tui/src/registry_probe.rs b/crates/rocm-dash-tui/src/registry_probe.rs index 78d0ef1a5..87614553d 100644 --- a/crates/rocm-dash-tui/src/registry_probe.rs +++ b/crates/rocm-dash-tui/src/registry_probe.rs @@ -15,8 +15,10 @@ //! read would freeze out two real cases: a model started mid-session via the //! Serve wizard (its id would never be in a launch-time snapshot, so it would //! never get a stop command or the `s`-to-open-Services hint), and a host -//! whose dashboard-embedded daemon client never connects at all (Windows — -//! `has_received_snapshot` never flips, so `AppState::instances` stays empty +//! whose daemon client never sends an instance snapshot (Windows — it +//! connects and streams live GPU/CPU/memory every tick, but has no +//! `rocmdashd` socket to source an instance list from, so +//! `has_received_snapshot` never flips and `AppState::instances` stays empty //! for the life of the session). A live probe has neither problem — there is //! no window for it to go stale in. diff --git a/crates/rocm-dash-tui/src/ui/mod.rs b/crates/rocm-dash-tui/src/ui/mod.rs index 35848f94b..441cb57c5 100644 --- a/crates/rocm-dash-tui/src/ui/mod.rs +++ b/crates/rocm-dash-tui/src/ui/mod.rs @@ -1102,9 +1102,11 @@ mod tests { #[test] fn quit_confirm_body_prints_the_stop_command_during_the_startup_fallback_when_the_registry_names_an_id() { - // On a host whose dashboard-embedded daemon client never connects at - // all (Windows — `has_received_snapshot` never flips for the whole - // session), `instances` never fills, so without this the quit prompt + // On a host whose daemon client never sends an instance snapshot + // (Windows — it connects and streams live GPU/CPU/memory every tick, + // but has no `rocmdashd` socket to source an instance list from, so + // `has_received_snapshot` never flips for the whole session), + // `instances` never fills, so without this the quit prompt // could only ever print the generic "a model was serving" sentence — // no model name (expected: `instances` is the only source for that), // but also no stop command and no id, even though the live registry diff --git a/crates/rocm-dash-tui/src/ui/tabs/home.rs b/crates/rocm-dash-tui/src/ui/tabs/home.rs index 7db3561db..ae0a3dbe6 100644 --- a/crates/rocm-dash-tui/src/ui/tabs/home.rs +++ b/crates/rocm-dash-tui/src/ui/tabs/home.rs @@ -68,12 +68,21 @@ const fn node_load_label(hist_empty: bool, simulated: bool) -> &'static str { } } -/// True until the first GPU telemetry snapshot actually carries any GPUs — -/// distinct from a snapshot that arrived with a legitimately-zero field -/// (e.g. `vram_total_mb == 0`, see `vram_used_pct`). Drives the hero's -/// "still detecting" placeholders so startup/a stalled tick doesn't read as -/// "0% / 0°C / 0W", which looks broken rather than loading. -fn gpu_telemetry_pending(state: &AppState) -> bool { +/// True until the first snapshot has arrived at all. Drives only the hero +/// title's "Detecting GPU…" placeholder — a snapshot that arrived but +/// carries no GPUs (or a legitimately-zero field) is a known state, not a +/// still-loading one, and reads "Unknown GPU" instead (see +/// `node_throughput_title`). For the numeric "—" placeholders, see +/// `gpu_data_unavailable`. +const fn gpu_telemetry_pending(state: &AppState) -> bool { + state.latest.is_none() +} + +/// True when there's no usable per-GPU data to show a number for: either no +/// snapshot has arrived yet, or one arrived but carries no GPUs at all. +/// Drives the headline util/vram/temp/power "—" placeholders — distinct +/// from `gpu_telemetry_pending`, which only gates the hero title. +fn gpu_data_unavailable(state: &AppState) -> bool { state.latest.as_ref().is_none_or(|s| s.gpus.is_empty()) } @@ -289,7 +298,7 @@ fn draw_hero_left(f: &mut Frame, area: Rect, state: &AppState, theme: &Theme) { lh[0], ); let util = state.latest.as_ref().map_or(0.0, snap_util); - let util_label = if gpu_telemetry_pending(state) { + let util_label = if gpu_data_unavailable(state) { "—".to_string() } else { format::pct(util as f32) @@ -380,8 +389,9 @@ fn draw_hero_right(f: &mut Frame, area: Rect, state: &AppState, theme: &Theme) { rh[0], ); let latest = state.latest.as_ref(); - let pending = gpu_telemetry_pending(state); + let unavailable = gpu_data_unavailable(state); let vram = latest.map_or(0.0, vram_used_pct); + let vram_total: u64 = latest.map_or(0, |s| s.gpus.iter().map(|g| g.vram_total_mb).sum()); let temp = latest.map_or(0.0, |s| { s.gpus .iter() @@ -391,19 +401,25 @@ fn draw_hero_right(f: &mut Frame, area: Rect, state: &AppState, theme: &Theme) { let power: f64 = latest.map_or(0.0, |s| { s.gpus.iter().map(|g| f64::from(g.power_w)).sum::() }); - // While no GPU telemetry has arrived yet, show "—" rather than 0%/0°C/0W - // — those look like real (broken) readings rather than a loading state. - let vram_label = if pending { + // While there's no GPU telemetry at all, show "—" rather than 0%/0°C/0W + // — those look like real (broken) readings rather than a loading/unknown + // state. The same applies per-metric once data has arrived but a given + // metric reads exactly zero: a multi-adapter Windows host can't + // correlate VRAM totals per-device (`vram_total_mb == 0`, see + // `vram_used_pct`), and temp/power read 0.0 with no ADLX reading — a + // real GPU is never actually at 0°C/0W, so an exact zero there means + // "not measured", not "measured as zero". + let vram_label = if unavailable || vram_total == 0 { "—".to_string() } else { format::pct(vram as f32) }; - let temp_label = if pending { + let temp_label = if unavailable || temp == 0.0 { "—".to_string() } else { format::celsius(temp as f32) }; - let power_label = if pending { + let power_label = if unavailable || power == 0.0 { "—".to_string() } else { format::watts(power as f32) @@ -747,18 +763,104 @@ mod tests { } #[test] - fn gpu_telemetry_pending_distinguishes_no_data_from_legitimate_zero() { - // Nothing has arrived yet → pending. + fn gpu_telemetry_pending_only_tracks_whether_anything_arrived() { + // Nothing has arrived yet → pending (drives the hero title). let empty = AppState::new("t".into(), "default-dark".into()); assert!(gpu_telemetry_pending(&empty)); - // A snapshot with no GPUs at all → still pending. + // A snapshot with no GPUs at all is a known state, not "still + // loading" — the title falls back to "Unknown GPU", not "Detecting". let mut no_gpus = AppState::new("t".into(), "default-dark".into()); no_gpus.latest = Some(Snapshot::default()); - assert!(gpu_telemetry_pending(&no_gpus)); - // A real GPU present (even with legitimately-zero fields) → not pending. + assert!(!gpu_telemetry_pending(&no_gpus)); assert!(!gpu_telemetry_pending(&state_with_gpu())); } + #[test] + fn gpu_data_unavailable_distinguishes_no_data_from_legitimate_zero() { + // Nothing has arrived yet → unavailable. + let empty = AppState::new("t".into(), "default-dark".into()); + assert!(gpu_data_unavailable(&empty)); + // A snapshot with no GPUs at all → still unavailable (no number to show). + let mut no_gpus = AppState::new("t".into(), "default-dark".into()); + no_gpus.latest = Some(Snapshot::default()); + assert!(gpu_data_unavailable(&no_gpus)); + // A real GPU present (even with legitimately-zero fields) → available. + assert!(!gpu_data_unavailable(&state_with_gpu())); + } + + #[test] + fn hero_shows_dash_placeholders_not_fake_zeros_when_metrics_are_unmeasured() { + // A real GPU is present (so the title isn't "Detecting GPU…"), but + // vram_total/temp/power all read exactly zero — e.g. a Windows + // multi-adapter host that can't correlate per-device VRAM totals, + // or no ADLX reading for temp/power. Each must show "—", not a + // fake-looking "0%"/"0°C"/"0W". + let mut s = AppState::new("t".into(), "default-dark".into()); + s.active_tab = ActiveTab::Home; + s.latest = Some(Snapshot { + host: SystemMetrics::default(), + gpus: vec![GpuMetrics { + device_id: "GPU0".into(), + vram_used_mb: 0, + vram_total_mb: 0, + gpu_utilization_pct: 12.0, + temperature_c: 0.0, + power_w: 0.0, + clock_mhz: None, + }], + gpu_system_info: Some(GpuSystemInfo { + gpu_model: "Some GPU".into(), + ..Default::default() + }), + ..Default::default() + }); + let out = render(&s, 160, 30); + // The buffer is row-major with no row separators, so bound each + // check to the segment between consecutive labels — otherwise an + // unrelated "0%"-shaped substring earlier in the row (e.g. the + // "12.0%" utilization gauge) would false-positive the negative + // assertions below. + let vram_segment = out + .split("VRAM") + .nth(1) + .and_then(|s| s.split("TEMP").next()); + let temp_segment = out + .split("TEMP") + .nth(1) + .and_then(|s| s.split("POWER").next()); + let power_segment = out + .split("POWER") + .nth(1) + .and_then(|s| s.split("T/S").next()); + let vram_segment = vram_segment.unwrap_or_else(|| panic!("vram row missing: {out:?}")); + let temp_segment = temp_segment.unwrap_or_else(|| panic!("temp row missing: {out:?}")); + let power_segment = power_segment.unwrap_or_else(|| panic!("power row missing: {out:?}")); + assert!( + vram_segment.contains('—'), + "vram should read —: {vram_segment:?}" + ); + assert!( + !vram_segment.contains('%'), + "vram should not show a fake 0%: {vram_segment:?}" + ); + assert!( + temp_segment.contains('—'), + "temp should read —: {temp_segment:?}" + ); + assert!( + !temp_segment.contains('°'), + "temp should not show a fake 0°C: {temp_segment:?}" + ); + assert!( + power_segment.contains('—'), + "power should read —: {power_segment:?}" + ); + assert!( + !power_segment.contains('W'), + "power should not show a fake 0W: {power_segment:?}" + ); + } + #[test] #[allow(clippy::float_cmp)] fn vram_used_pct_is_zero_not_garbage_when_total_is_unknown() { diff --git a/tests/e2e-cucumber/features/dash.feature b/tests/e2e-cucumber/features/dash.feature index cd82e409b..db9979f20 100644 --- a/tests/e2e-cucumber/features/dash.feature +++ b/tests/e2e-cucumber/features/dash.feature @@ -600,3 +600,22 @@ Feature: Interactive dashboard When the user denies the adopt approval and closes onboarding setup And the user quits the dashboard Then the dashboard exits successfully + + # Exercises the hero's "no GPU telemetry" rendering (see + # `gpu_data_unavailable`/`node_throughput_title` in + # `crates/rocm-dash-tui/src/ui/tabs/home.rs`) with a *real* embedded daemon, + # not `--demo` (whose fixture always has 8 GPUs and can never reach this + # state). This CI runner has no AMD GPU, so the real collector's first + # snapshot — and every one after it — reports zero GPUs, making "unavailable" + # a stable, permanent state here rather than a race. The other half of that + # state machine, the transient "pending" placeholder shown before the first + # snapshot arrives at all, is not covered: it closes in well under a second + # and there's no hook to freeze the daemon mid-handshake, so asserting on it + # would be asserting on a race. + @id:dash-reports-no-gpu-telemetry @requires-os:linux + Scenario: dash-37 - The hero shows placeholders, not fake zeros, when no GPU is detected + When the user opens the dashboard + Then the dashboard home view is displayed + And the hero reports no GPU telemetry available + When the user quits the dashboard + Then the dashboard exits successfully diff --git a/tests/e2e-cucumber/tests/e2e/dash_steps.rs b/tests/e2e-cucumber/tests/e2e/dash_steps.rs index bd979c75a..8d999e548 100644 --- a/tests/e2e-cucumber/tests/e2e/dash_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/dash_steps.rs @@ -1284,6 +1284,38 @@ async fn home_view_displayed(world: &mut E2eWorld) { ); } +#[then("the hero reports no GPU telemetry available")] +async fn hero_reports_no_gpu_telemetry(world: &mut E2eWorld) { + let tui = session(world); + // The VRAM/TEMP/POWER mini-sparks only draw in the hero's wide layout + // (`draw_hero_band`'s `hero.width >= 8 && hero.height >= 6` gate). + tui.use_wide_size() + .unwrap_or_else(|e| panic!("failed to enlarge the dashboard: {e}")); + + // `use_wide_size` only resizes the pty; the TUI redraws asynchronously + // once it processes the resulting SIGWINCH. Wait for that redraw by + // polling for the hero's wide-layout content, not just "Unknown GPU" + // (already visible pre-resize from the title bar alone). + tui.wait_for_screen_where( + "the hero's wide VRAM/TEMP/POWER layout", + |screen| screen.contains("Unknown GPU") && screen.contains("VRAM"), + default_timeout(), + ) + .await + .unwrap_or_else(|e| panic!("the no-GPU hero did not render: {e}")); + let screen = tui.screen_text(); + for label in ["VRAM", "TEMP", "POWER"] { + let row = screen + .lines() + .find(|line| line.contains(label)) + .unwrap_or_else(|| panic!("no {label} row on screen:\n{screen}")); + assert!( + row.contains('—'), + "{label} row should show the \"—\" placeholder, not a fake reading:\n{row}" + ); + } +} + #[then("the dashboard reports live GPU telemetry")] async fn gpu_telemetry_displayed(world: &mut E2eWorld) { let tui = session(world); From d6b894f44fcb7fd6a5426eda985d47f3ee59a2d9 Mon Sep 17 00:00:00 2001 From: Juho Vainio Date: Fri, 23 Oct 2026 13:12:33 +0300 Subject: [PATCH 12/14] fix(dash-collectors): replace em dash with ASCII hyphen in winperf warning The ascii_output_contract test requires all string literals reachable by redirectable output streams to be ASCII, since Windows consoles on non-UTF-8 code pages (e.g. CP437) render em dashes as mojibake. Signed-off-by: Juho Vainio --- crates/rocm-dash-collectors/src/winperf.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/rocm-dash-collectors/src/winperf.rs b/crates/rocm-dash-collectors/src/winperf.rs index 8e2a12152..84665ae24 100644 --- a/crates/rocm-dash-collectors/src/winperf.rs +++ b/crates/rocm-dash-collectors/src/winperf.rs @@ -420,7 +420,7 @@ fn counter_samples_missing_warning(raw: &str, metrics: &[GpuMetrics]) -> Option< if metrics.is_empty() && !parse_adapters(raw).is_empty() { Some( "Windows GPU utilization/VRAM counters returned no samples (possible non-English \ - Windows locale — Get-Counter counter paths are localized)" + Windows locale - Get-Counter counter paths are localized)" .to_string(), ) } else { From 088456985f8f03aac55db7f877189d738fb1909e Mon Sep 17 00:00:00 2001 From: Juho Vainio Date: Fri, 23 Oct 2026 16:57:09 +0300 Subject: [PATCH 13/14] fix(dash-tui,dash-collectors): resolve round 3 review findings for Windows GPU metrics - winperf.rs: fix a stale test fixture that reused an existing LUID instead of introducing a genuinely new one, so the multi-adapter clamp branch was never exercised - winperf.rs: wrap the ADLX query in a timeout; a wedged driver call no longer freezes every other metric this tick, and surfaces as a warning instead of silently degrading - winperf.rs: correct a module doc claim that the powershell.exe shellout always fails cleanly off Windows (it does not under WSL interop, where powershell.exe is on PATH) - client.rs: delete a comment claiming a tick-ordering benefit that HostCollector::tick's CPU delta does not actually provide - dash.feature: correct dash-37's stated premise; the stable zero-GPU state comes from the daemon's /dev/kfd preflight failing for the test harness process, not from the CI runner lacking an AMD GPU - home.rs: mark gpu_telemetry_pending as const fn per clippy PR description also corrected to credit the full hero-placeholder change set and docs edits, and to fix a false claim about why the Windows collector path isn't covered by e2e (the PTY driver is Linux-only for now, not a lack of a GPU Windows runner). Signed-off-by: Juho Vainio --- crates/rocm-dash-collectors/src/winperf.rs | 53 ++++++++++++++++-- crates/rocm-dash-tui/src/app/types.rs | 10 ++++ crates/rocm-dash-tui/src/client.rs | 4 -- crates/rocm-dash-tui/src/ui/tabs/home.rs | 56 +++++++++++++++++-- crates/rocm-dash-tui/src/ui/tabs/instances.rs | 39 +++++++++++-- tests/e2e-cucumber/features/dash.feature | 19 ++++--- tests/e2e-cucumber/tests/e2e/dash_steps.rs | 10 +++- 7 files changed, 165 insertions(+), 26 deletions(-) diff --git a/crates/rocm-dash-collectors/src/winperf.rs b/crates/rocm-dash-collectors/src/winperf.rs index 84665ae24..444c2939c 100644 --- a/crates/rocm-dash-collectors/src/winperf.rs +++ b/crates/rocm-dash-collectors/src/winperf.rs @@ -13,9 +13,14 @@ //! Reads the `GPU Engine`/`GPU Adapter Memory` categories — the same //! WDDM/DXGK-backed source Task Manager's GPU tab reads. Mirrors //! `amd_smi.rs`'s subprocess + pure-parse shape, shelling out to -//! `powershell.exe` instead of `amd-smi`. Not `#[cfg(windows)]`-gated: the -//! shellout fails cleanly (`powershell.exe` not found) on other platforms, -//! same as `amd_smi.rs` when `amd-smi` isn't installed. +//! `powershell.exe` instead of `amd-smi`. Not `#[cfg(windows)]`-gated at the +//! module level, but `snapshot()`'s only caller today is itself +//! `#[cfg(windows)]` (see `client.rs`'s `connect_and_run`). On a plain Linux +//! host `powershell.exe` isn't on `PATH` and this fails cleanly like +//! `amd_smi.rs` does when `amd-smi` is missing — but under WSL interop, +//! `powershell.exe` *is* on `PATH` and this would run against the Windows +//! host's GPUs, so don't add a Linux-reachable caller without reconsidering +//! that. use std::collections::{BTreeMap, BTreeSet}; use std::io; @@ -34,6 +39,12 @@ use tokio::time::timeout; // actually-hung process forever. const RUN_TIMEOUT: Duration = Duration::from_secs(15); +/// ADLX init/read/terminate shouldn't normally take anywhere near this, but a +/// wedged driver call must not freeze CPU/memory updates forever alongside it +/// — same reasoning as `RUN_TIMEOUT`, just a tighter bound since there's no +/// process spawn latency to absorb here. +const ADLX_TIMEOUT: Duration = Duration::from_secs(5); + /// `kindkeyvalue[value2]` lines: per-engine utilization, /// per-adapter dedicated VRAM usage, and a best-effort adapter inventory /// (name/RAM/driver) for `GpuSystemInfo`. One process spawn for all three. @@ -68,12 +79,22 @@ impl WinPerfCollector { // adlx_readings() calls into amdadlx64.dll via FFI, which blocks on // driver I/O — running it directly here would stall a tokio worker // thread, same hazard run_script() avoids by using an async Command. - let adlx = task::spawn_blocking(adlx_readings) - .await - .unwrap_or_default(); + // Timed out the same way: a wedged driver call must not freeze every + // other metric this tick forever, and either failure mode is + // surfaced as a warning rather than silently degrading to defaults. + let (adlx, adlx_warning) = + match timeout(ADLX_TIMEOUT, task::spawn_blocking(adlx_readings)).await { + Ok(Ok(readings)) => (readings, None), + Ok(Err(_)) => (Vec::new(), Some("ADLX reading task panicked".to_string())), + Err(_) => ( + Vec::new(), + Some(format!("ADLX query timed out after {ADLX_TIMEOUT:?}")), + ), + }; let metrics = parse_metrics(&out, &adlx); let warnings = counter_samples_missing_warning(&out, &metrics) .into_iter() + .chain(adlx_warning) .collect(); Ok((metrics, parse_system_info(&out), warnings)) } @@ -638,6 +659,26 @@ mod tests { ); } + #[test] + #[allow(clippy::float_cmp)] + fn multi_adapter_utilization_clamps_to_100_when_summed_processes_exceed_it() { + // Two processes both drawing on the same engine of the same adapter + // (summed per the doc comment on `engine_util`) push the raw total + // past 100 on a *second real adapter* alongside the one in `SAMPLE` — + // the multi-adapter branch's own clamp must still catch it, same as + // the single-adapter branch above it. + let two_adapters = format!( + "{SAMPLE}\nUTIL\tpid_9998_luid_0x00000000_0x0000cafe_phys_0_eng_2_engtype_3d\t95.0\nUTIL\tpid_9999_luid_0x00000000_0x0000cafe_phys_0_eng_3_engtype_3d\t95.0\nGPU\tOther GPU\t4293918720\t1.0" + ); + let metrics = parse_metrics(&two_adapters, &[]); + assert_eq!(metrics.len(), 2); + let gpu = metrics + .iter() + .find(|g| g.vram_used_mb == 0) + .expect("the cafe device's metrics"); + assert_eq!(gpu.gpu_utilization_pct, 100.0); + } + #[test] #[allow(clippy::float_cmp)] fn single_adapter_falls_back_to_wmi_vram_when_adlx_reading_count_mismatches() { diff --git a/crates/rocm-dash-tui/src/app/types.rs b/crates/rocm-dash-tui/src/app/types.rs index 34e79a7ad..2c383757e 100644 --- a/crates/rocm-dash-tui/src/app/types.rs +++ b/crates/rocm-dash-tui/src/app/types.rs @@ -172,6 +172,16 @@ pub enum ConnState { }, } +impl ConnState { + /// True for the Windows in-process collector (see + /// `client::connect_and_run`'s `#[cfg(windows)]` arm), which tags its + /// `daemon_version` this way since there's no real `rocmdashd` behind + /// it — and so no instance registry it could ever populate. + pub fn is_in_process(&self) -> bool { + matches!(self, Self::Connected { version, .. } if version.starts_with("in-process/")) + } +} + #[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] pub enum ActiveTab { // 5-tab IA. Home is the default; ROCm and Serving are the two domain tabs diff --git a/crates/rocm-dash-tui/src/client.rs b/crates/rocm-dash-tui/src/client.rs index 39540e84a..ab2ad8117 100644 --- a/crates/rocm-dash-tui/src/client.rs +++ b/crates/rocm-dash-tui/src/client.rs @@ -223,10 +223,6 @@ async fn connect_and_run(connect: &str, tx: UnboundedSender) -> anyho loop { ticker.tick().await; - // Sampled before the GPU query, not after: `host.tick()`'s CPU - // reading would otherwise include the window spent spawning and - // waiting on `powershell.exe` for the GPU counters, inflating the - // dashboard's own CPU% with its own collection cost. let host_metrics = host.tick(); let (gpus, gpu_system_info, warnings) = match gpu.snapshot().await { Ok(v) => v, diff --git a/crates/rocm-dash-tui/src/ui/tabs/home.rs b/crates/rocm-dash-tui/src/ui/tabs/home.rs index ae0a3dbe6..c9d243399 100644 --- a/crates/rocm-dash-tui/src/ui/tabs/home.rs +++ b/crates/rocm-dash-tui/src/ui/tabs/home.rs @@ -23,7 +23,7 @@ use ratatui::widgets::Paragraph; use rocm_dash_core::state::JobStatus; -use crate::app::{AppState, UpdateStatus}; +use crate::app::{AppState, ConnState, UpdateStatus}; use crate::ui::format; use crate::ui::gradient::GradientGauge; use crate::ui::panel::{self, BoxRole}; @@ -72,10 +72,14 @@ const fn node_load_label(hist_empty: bool, simulated: bool) -> &'static str { /// title's "Detecting GPU…" placeholder — a snapshot that arrived but /// carries no GPUs (or a legitimately-zero field) is a known state, not a /// still-loading one, and reads "Unknown GPU" instead (see -/// `node_throughput_title`). For the numeric "—" placeholders, see -/// `gpu_data_unavailable`. +/// `node_throughput_title`). Gated on the connection still being in its +/// initial handshake: `latest` is also wiped on disconnect +/// (`ClientMsg::Disconnected`, including end-of-`--replay`) and on a replay +/// seek, neither of which means detection restarted — those read "Unknown +/// GPU" too rather than reverting to "still loading". For the numeric "—" +/// placeholders, see `gpu_data_unavailable`. const fn gpu_telemetry_pending(state: &AppState) -> bool { - state.latest.is_none() + state.latest.is_none() && matches!(state.conn, ConnState::Initial | ConnState::Connecting) } /// True when there's no usable per-GPU data to show a number for: either no @@ -861,6 +865,50 @@ mod tests { ); } + #[test] + fn gpu_utilization_gauge_shows_dash_not_a_fake_zero_percent_when_no_gpus_are_reported() { + // A snapshot arrived but carries no GPUs at all (`gpu_data_unavailable` + // true) — the gauge label must read "—", not a fake "0.0%" + // (draw_hero_left's `gpu_data_unavailable` branch). + let mut s = AppState::new("t".into(), "default-dark".into()); + s.active_tab = ActiveTab::Home; + s.latest = Some(Snapshot::default()); + let out = render(&s, 160, 30); + let segment = out + .split("GPU UTILIZATION") + .nth(1) + .and_then(|s| s.split("tokens / watt").next()) + .unwrap_or_else(|| panic!("gpu utilization row missing: {out:?}")); + assert!( + segment.contains('—'), + "gpu utilization should read —, not a fake 0.0%: {segment:?}" + ); + assert!( + !segment.contains("0.0%"), + "gpu utilization should not show a fake 0.0%: {segment:?}" + ); + } + + #[test] + fn gpu_telemetry_pending_does_not_resume_after_a_disconnect_or_replay_end() { + // Once connected, losing the connection (end of `--replay`, a daemon + // drop, ClientMsg::Disconnected) wipes `latest` but must not look + // like detection restarted — the title should stay on "Unknown GPU", + // never flip back to "Detecting GPU…". + let mut s = AppState::new("t".into(), "default-dark".into()); + s.conn = ConnState::Connected { + host: "x".into(), + version: "1".into(), + }; + s.latest = Some(Snapshot::default()); + s.conn = ConnState::Disconnected { + reason: "end of recording".into(), + }; + s.latest = None; + assert!(!gpu_telemetry_pending(&s)); + assert_eq!(node_throughput_title(&s), "Node throughput · Unknown GPU"); + } + #[test] #[allow(clippy::float_cmp)] fn vram_used_pct_is_zero_not_garbage_when_total_is_unknown() { diff --git a/crates/rocm-dash-tui/src/ui/tabs/instances.rs b/crates/rocm-dash-tui/src/ui/tabs/instances.rs index f7b493ef0..02daa89e1 100644 --- a/crates/rocm-dash-tui/src/ui/tabs/instances.rs +++ b/crates/rocm-dash-tui/src/ui/tabs/instances.rs @@ -375,11 +375,15 @@ const fn status_role(status: InstanceStatus) -> BoxRole { } fn draw_empty(f: &mut Frame, area: Rect, state: &AppState, theme: &Theme) { - let body = match &state.conn { - ConnState::Connected { .. } => { - "no instances · start daemon with --enable-docker on a host with vLLM containers" + let body = if state.conn.is_in_process() { + "no instance registry on this platform · vLLM containers aren't available here" + } else { + match &state.conn { + ConnState::Connected { .. } => { + "no instances · start daemon with --enable-docker on a host with vLLM containers" + } + _ => "waiting for daemon…", } - _ => "waiting for daemon…", }; let inner = panel::bento(f, area, Some("Instances"), BoxRole::Neutral, false, theme); let p = Paragraph::new(Line::from(Span::styled( @@ -1354,6 +1358,33 @@ mod tests { out } + #[test] + fn draw_empty_names_the_platform_reason_for_the_in_process_collector() { + use ratatui::Terminal; + use ratatui::backend::TestBackend; + + // The Windows in-process collector (see `ConnState::is_in_process`) + // has no instance registry at all, ever — distinct from "a real + // daemon is connected but nothing is running" or "still connecting". + let mut state = mk_state(HashMap::new(), 0); + state.conn = ConnState::Connected { + host: "localhost".into(), + version: "in-process/0.0.0".into(), + }; + let mut term = Terminal::new(TestBackend::new(80, 20)).unwrap(); + term.draw(|f| draw(f, f.area(), &state, &state.theme)) + .unwrap(); + let grid = buffer_text(&term); + assert!( + grid.contains("no instance registry on this platform"), + "should name the platform limitation, not claim a daemon could be started; got:\n{grid}" + ); + assert!( + !grid.contains("start daemon"), + "must not suggest starting a daemon that doesn't exist on this platform; got:\n{grid}" + ); + } + #[test] fn render_shows_quantization_and_vram_for_populated_instance() { use ratatui::Terminal; diff --git a/tests/e2e-cucumber/features/dash.feature b/tests/e2e-cucumber/features/dash.feature index db9979f20..b5a427e79 100644 --- a/tests/e2e-cucumber/features/dash.feature +++ b/tests/e2e-cucumber/features/dash.feature @@ -605,13 +605,18 @@ Feature: Interactive dashboard # `gpu_data_unavailable`/`node_throughput_title` in # `crates/rocm-dash-tui/src/ui/tabs/home.rs`) with a *real* embedded daemon, # not `--demo` (whose fixture always has 8 GPUs and can never reach this - # state). This CI runner has no AMD GPU, so the real collector's first - # snapshot — and every one after it — reports zero GPUs, making "unavailable" - # a stable, permanent state here rather than a race. The other half of that - # state machine, the transient "pending" placeholder shown before the first - # snapshot arrives at all, is not covered: it closes in well under a second - # and there's no hook to freeze the daemon mid-handshake, so asserting on it - # would be asserting on a race. + # state). The real reason this is a stable, permanent state here rather + # than a race isn't "no AMD GPU" on the runner — it's that the daemon's + # mandatory `/dev/kfd` pre-flight (`amd_smi::device_accessible`, + # `preflight_passes`) fails for this harness process (no read access to the + # device node, or it isn't visible to it at all), the same way it would on + # a real host without render/video group membership. So `detect_with_binary` + # returns `None` and every snapshot reports zero GPUs, regardless of + # whether the GPU lane's runner itself has real hardware. The other half of + # that state machine, the transient "pending" placeholder shown before the + # first snapshot arrives at all, is not covered: it closes in well under a + # second and there's no hook to freeze the daemon mid-handshake, so + # asserting on it would be asserting on a race. @id:dash-reports-no-gpu-telemetry @requires-os:linux Scenario: dash-37 - The hero shows placeholders, not fake zeros, when no GPU is detected When the user opens the dashboard diff --git a/tests/e2e-cucumber/tests/e2e/dash_steps.rs b/tests/e2e-cucumber/tests/e2e/dash_steps.rs index 8d999e548..af3b76636 100644 --- a/tests/e2e-cucumber/tests/e2e/dash_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/dash_steps.rs @@ -1309,8 +1309,16 @@ async fn hero_reports_no_gpu_telemetry(world: &mut E2eWorld) { .lines() .find(|line| line.contains(label)) .unwrap_or_else(|| panic!("no {label} row on screen:\n{screen}")); + // Check the text *after* the label, not the whole row: the + // utilization gauge on the same row draws its own "—" placeholder, + // which would make this pass even if this mini-spark's value were a + // fake reading instead of the placeholder. + let after_label = row + .split_once(label) + .map(|(_, rest)| rest) + .unwrap_or_else(|| panic!("no text after {label} on row:\n{row}")); assert!( - row.contains('—'), + after_label.contains('—'), "{label} row should show the \"—\" placeholder, not a fake reading:\n{row}" ); } From 3f2b2d5d35d5266054ef9832c1b342d6a9b8eb43 Mon Sep 17 00:00:00 2001 From: Juho Vainio Date: Fri, 23 Oct 2026 17:13:26 +0300 Subject: [PATCH 14/14] fix(e2e-cucumber): resolve clippy::map_unwrap_or in hero no-GPU telemetry step Signed-off-by: Juho Vainio --- tests/e2e-cucumber/tests/e2e/dash_steps.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/e2e-cucumber/tests/e2e/dash_steps.rs b/tests/e2e-cucumber/tests/e2e/dash_steps.rs index af3b76636..fa72f8ef7 100644 --- a/tests/e2e-cucumber/tests/e2e/dash_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/dash_steps.rs @@ -1313,10 +1313,10 @@ async fn hero_reports_no_gpu_telemetry(world: &mut E2eWorld) { // utilization gauge on the same row draws its own "—" placeholder, // which would make this pass even if this mini-spark's value were a // fake reading instead of the placeholder. - let after_label = row - .split_once(label) - .map(|(_, rest)| rest) - .unwrap_or_else(|| panic!("no text after {label} on row:\n{row}")); + let after_label = row.split_once(label).map_or_else( + || panic!("no text after {label} on row:\n{row}"), + |(_, rest)| rest, + ); assert!( after_label.contains('—'), "{label} row should show the \"—\" placeholder, not a fake reading:\n{row}"