From 9d7b7a8c9bce32a5919a2a4ad60df4fc63782cf3 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Fri, 2 Oct 2026 12:31:03 +0000 Subject: [PATCH 1/5] test(examine): a property harness for AMD GPU classification MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The GPU classifiers in `examine` are pure functions over strings a host hands them, and the strings are messy in ways no example test enumerates: `lspci` spells a codename with a die number, Windows decorates names with "(TM)", `rocminfo` pads columns, and `pci.ids` sometimes has no name at all. This builds generators from real fixture shapes — actual `lspci -nn -D` lines, `rocminfo` agent blocks, `Win32_VideoController` rows, AMD marketing names — and perturbs them, because a uniform-random string generator never reaches the branches that matter. Four invariants to start, all of which the classifiers already satisfy: totality and determinism, exact round-tripping of an `lspci` line's device text, independence from how a name is spelled, and agreement between the two Windows probes that read the same adapter row. The properties that assert *correct* classification come with the fixes that make them pass, so no commit here is red. `generator_reach_is_measured_and_sufficient` asserts floors on how far the generators actually reach. A generator that never samples the interesting region passes every property vacuously, so the reach is measured rather than assumed. Adds `proptest` as a dev-dependency of `rocm-core`, with the eight transitive crates that brings recorded in MANIFEST.md. `THIRD_PARTY_NOTICES.txt` is unaffected: `about.toml` sets `ignore-dev-dependencies`, since dev-dependencies are not shipped in the distributed binaries. Failing-case seed files are gitignored — the classification corpora are swept exhaustively by the deterministic tests that follow, so no coverage rides on seed luck. Signed-off-by: Roman Inflianskas --- .gitignore | 4 + Cargo.lock | 81 +++- MANIFEST.md | 8 + crates/rocm-core/Cargo.toml | 3 + crates/rocm-core/src/examine.rs | 9 + crates/rocm-core/src/examine_proptests.rs | 486 ++++++++++++++++++++++ 6 files changed, 589 insertions(+), 2 deletions(-) create mode 100644 crates/rocm-core/src/examine_proptests.rs diff --git a/.gitignore b/.gitignore index e8ccf9a30..f1b363479 100644 --- a/.gitignore +++ b/.gitignore @@ -20,6 +20,10 @@ __pycache__/ /plans/ /workspace/ /e2e-consolidated-report.md +# proptest writes failing-case seeds here. The classification corpora are swept +# exhaustively by deterministic tests alongside the properties, so a regression +# cannot hide behind seed luck and these files carry no coverage of their own. +**/proptest-regressions/ # Sphinx docs: _toc.yml is generated from _toc.yml.in at build time docs/rocm-docs/sphinx/_toc.yml diff --git a/Cargo.lock b/Cargo.lock index 549cde179..9b6faf65d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -446,7 +446,16 @@ version = "0.5.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0700ddab506f33b20a03b13996eccd309a48e5ff77d0d95926aa0210fb4e95f1" dependencies = [ - "bit-vec", + "bit-vec 0.6.3", +] + +[[package]] +name = "bit-set" +version = "0.8.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "08807e080ed7f9d5433fa9b275196cfc35414f66a0c79d864dc51a0d825231a3" +dependencies = [ + "bit-vec 0.8.0", ] [[package]] @@ -455,6 +464,12 @@ version = "0.6.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "349f9b6a179ed607305526ca489b34ad0a41aed5f7980fa90eb03160b69598fb" +[[package]] +name = "bit-vec" +version = "0.8.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5e764a1d40d510daf35e07be9eb06e75770908c27d411ee6c92109c9840eaaf7" + [[package]] name = "bitflags" version = "1.3.2" @@ -1415,7 +1430,7 @@ version = "0.11.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b95f7c0680e4142284cf8b22c14a476e87d61b004a3a0861872b32ef7ead40a2" dependencies = [ - "bit-set", + "bit-set 0.5.3", "regex", ] @@ -3179,6 +3194,25 @@ dependencies = [ "version_check", ] +[[package]] +name = "proptest" +version = "1.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4b45fcc2344c680f5025fe57779faef368840d0bd1f42f216291f0dc4ace4744" +dependencies = [ + "bit-set 0.8.0", + "bit-vec 0.8.0", + "bitflags 2.13.0", + "num-traits", + "rand 0.9.4", + "rand_chacha 0.9.0", + "rand_xorshift", + "regex-syntax", + "rusty-fork", + "tempfile", + "unarray", +] + [[package]] name = "pulldown-cmark" version = "0.12.2" @@ -3190,6 +3224,12 @@ dependencies = [ "unicase", ] +[[package]] +name = "quick-error" +version = "1.2.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a1d01941d82fa2ab50be1e79e6714289dd7cde78eba4c074bc5a4374f650dfe0" + [[package]] name = "quick-xml" version = "0.39.4" @@ -3335,6 +3375,15 @@ dependencies = [ "getrandom 0.3.4", ] +[[package]] +name = "rand_xorshift" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "513962919efc330f829edb2535844d1b912b0fbe2ca165d613e4e8788bb05a5a" +dependencies = [ + "rand_core 0.9.5", +] + [[package]] name = "ratatui" version = "0.30.2" @@ -3673,6 +3722,7 @@ dependencies = [ "cc", "directories", "libc", + "proptest", "rand 0.9.4", "regex", "rsa", @@ -4026,6 +4076,18 @@ version = "1.0.22" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b39cdef0fa800fc44525c84ccb54a029961a8215f9619753635a9c0d2538d46d" +[[package]] +name = "rusty-fork" +version = "0.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cc6bf79ff24e648f6da1f8d1f011e9cac26491b619e6b9280f2b47f1774e6ee2" +dependencies = [ + "fnv", + "quick-error", + "tempfile", + "wait-timeout", +] + [[package]] name = "ryu" version = "1.0.23" @@ -5201,6 +5263,12 @@ dependencies = [ "windows-sys 0.61.2", ] +[[package]] +name = "unarray" +version = "0.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "eaea85b334db583fe3274d12b4cd1880032beab409c0d774be044d4480ab9a94" + [[package]] name = "unicase" version = "2.9.0" @@ -5358,6 +5426,15 @@ dependencies = [ "utf8parse", ] +[[package]] +name = "wait-timeout" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "09ac3b126d3914f9849036f826e054cbabdc8519970b8998ddaf3b5bd3c65f11" +dependencies = [ + "libc", +] + [[package]] name = "walkdir" version = "2.5.0" diff --git a/MANIFEST.md b/MANIFEST.md index 9fc4a680d..c08be830b 100644 --- a/MANIFEST.md +++ b/MANIFEST.md @@ -75,7 +75,9 @@ repository. | base64 | 0.22.1 | MIT OR Apache-2.0 | | base64ct | 1.8.3 | Apache-2.0 OR MIT | | bit-set | 0.5.3 | MIT/Apache-2.0 | +| bit-set | 0.8.0 | Apache-2.0 OR MIT | | bit-vec | 0.6.3 | MIT/Apache-2.0 | +| bit-vec | 0.8.0 | Apache-2.0 OR MIT | | bitflags | 1.3.2 | MIT/Apache-2.0 | | bitflags | 2.13.0 | MIT OR Apache-2.0 | | block-buffer | 0.10.4 | MIT OR Apache-2.0 | @@ -353,7 +355,9 @@ repository. | proc-macro-crate | 3.5.0 | MIT OR Apache-2.0 | | proc-macro2 | 1.0.106 | MIT OR Apache-2.0 | | proc-macro2-diagnostics | 0.10.1 | MIT/Apache-2.0 | +| proptest | 1.11.0 | MIT OR Apache-2.0 | | pulldown-cmark | 0.12.2 | MIT | +| quick-error | 1.2.3 | MIT/Apache-2.0 | | quick-xml | 0.39.4 | MIT | | quinn | 0.11.11 | MIT OR Apache-2.0 | | quinn-proto | 0.11.15 | MIT OR Apache-2.0 | @@ -367,6 +371,7 @@ repository. | rand_chacha | 0.9.0 | MIT OR Apache-2.0 | | rand_core | 0.6.4 | MIT OR Apache-2.0 | | rand_core | 0.9.5 | MIT OR Apache-2.0 | +| rand_xorshift | 0.4.0 | MIT OR Apache-2.0 | | ratatui | 0.30.2 | MIT | | ratatui-core | 0.1.2 | MIT | | ratatui-crossterm | 0.1.2 | MIT | @@ -402,6 +407,7 @@ repository. | rustls-platform-verifier-android | 0.1.1 | MIT OR Apache-2.0 | | rustls-webpki | 0.103.13 | ISC | | rustversion | 1.0.22 | MIT OR Apache-2.0 | +| rusty-fork | 0.3.1 | MIT/Apache-2.0 | | ryu | 1.0.23 | Apache-2.0 OR BSL-1.0 | | same-file | 1.0.6 | Unlicense/MIT | | schannel | 0.1.29 | MIT | @@ -513,6 +519,7 @@ repository. | typenum | 1.20.1 | MIT OR Apache-2.0 | | ucd-trie | 0.1.7 | MIT OR Apache-2.0 | | uds_windows | 1.2.1 | MIT | +| unarray | 0.1.4 | MIT OR Apache-2.0 | | unicase | 2.9.0 | MIT OR Apache-2.0 | | unicode-ident | 1.0.24 | (MIT OR Apache-2.0) AND Unicode-3.0 | | unicode-linebreak | 0.1.5 | Apache-2.0 | @@ -532,6 +539,7 @@ repository. | vt100 | 0.16.2 | MIT | | vte | 0.15.0 | Apache-2.0 OR MIT | | vtparse | 0.6.2 | MIT | +| wait-timeout | 0.2.1 | MIT/Apache-2.0 | | walkdir | 2.5.0 | Unlicense/MIT | | want | 0.3.1 | MIT | | wasi | 0.11.1+wasi-snapshot-preview1 | Apache-2.0 WITH LLVM-exception OR Apache-2.0 OR MIT | diff --git a/crates/rocm-core/Cargo.toml b/crates/rocm-core/Cargo.toml index 4c19b7821..798674a4e 100644 --- a/crates/rocm-core/Cargo.toml +++ b/crates/rocm-core/Cargo.toml @@ -27,5 +27,8 @@ sysinfo.workspace = true toml = "1.1" ureq = { version = "2.12", features = ["native-certs"] } +[dev-dependencies] +proptest = "1" + [target.'cfg(target_os = "windows")'.dependencies] windows-sys = { version = "0.61", features = ["Win32_Foundation", "Win32_Security", "Win32_System_Registry", "Win32_System_SystemInformation", "Win32_System_Threading"] } diff --git a/crates/rocm-core/src/examine.rs b/crates/rocm-core/src/examine.rs index 2a3244f12..19f4219a1 100644 --- a/crates/rocm-core/src/examine.rs +++ b/crates/rocm-core/src/examine.rs @@ -2391,6 +2391,15 @@ fn probe_msvc_redist_windows(e: &mut Examination) { e.msvc_redist_present = Some(present); } +/// Property-based coverage of the pure GPU classifiers above. +/// +/// A child module of `examine` rather than a sibling, so it can reach the +/// private classifiers here *and* the crate-root install-family tables it has +/// to cross-check them against. +#[cfg(test)] +#[path = "examine_proptests.rs"] +mod proptests; + #[cfg(test)] mod tests { use super::*; diff --git a/crates/rocm-core/src/examine_proptests.rs b/crates/rocm-core/src/examine_proptests.rs new file mode 100644 index 000000000..65bef9785 --- /dev/null +++ b/crates/rocm-core/src/examine_proptests.rs @@ -0,0 +1,486 @@ +// Copyright © Advanced Micro Devices, Inc., or its affiliates. +// +// SPDX-License-Identifier: MIT + +//! Property-based tests for AMD GPU detection and classification. +//! +//! The generators here are built from *real* fixture shapes — actual `lspci +//! -nn -D` lines, `rocminfo` agent blocks, `Win32_VideoController` rows and AMD +//! marketing names — and then mutated (case, whitespace, decorations, dropped +//! fields, truncation, duplication). A uniform-random string generator never +//! reaches the deep parser branches, so every strategy below starts from a +//! corpus and perturbs it. +//! +//! Each property states an invariant the *report* must hold, not an +//! implementation detail, so a failure names a user-visible defect. +//! +//! What is here is the harness and the invariants the classifiers already +//! satisfy: totality, determinism, round-tripping an `lspci` line, and +//! independence from how a name is spelled. The properties that assert +//! *correct classification* arrive with the fixes that make them pass, so +//! that no commit in this history is red. + +use super::{ + classify_amd_marketing_name, extract_lspci_name, gfx_is_apu_family, is_lspci_gpu_line, +}; +use proptest::prelude::*; +use proptest::strategy::ValueTree; +use proptest::test_runner::{Config, TestRunner}; + +// --------------------------------------------------------------------------- +// Corpora: real strings, taken from pci.ids, from this repo's own fixtures and +// from the hardware named in ROCm/rocm-cli#448 / #449. +// --------------------------------------------------------------------------- + +/// `(device_name_as_lspci_prints_it, pci_device_id, true_gfx_target, +/// is_really_an_apu)`. +/// +/// The names are the `pci.ids` device strings an `lspci -nn` line carries after +/// the `[AMD/ATI]` vendor tag. The last two fields are ground truth about the +/// silicon, not about what the code says, so a pairing drawn from one row is +/// always a *coherent* host: the gfx target really is the one that device +/// reports. +const AMD_LSPCI_DEVICES: &[(&str, &str, &str, bool)] = &[ + // Discrete RDNA2 / RDNA3 / RDNA4. + ( + "Navi 31 [Radeon RX 7900 XT/7900 XTX/7900 GRE/7900M]", + "744c", + "gfx1100", + false, + ), + ( + "Navi 32 [Radeon RX 7700 XT / 7800 XT]", + "747e", + "gfx1101", + false, + ), + ( + "Navi 33 [Radeon RX 7600/7600 XT/7600M XT/7600S/7700S / PRO W7600]", + "7480", + "gfx1102", + false, + ), + ( + "Navi 21 [Radeon RX 6800/6800 XT / 6900 XT]", + "73bf", + "gfx1030", + false, + ), + ( + "Navi 22 [Radeon RX 6700 XT / 6800M / 6950 XT]", + "73df", + "gfx1031", + false, + ), + ( + "Navi 23 [Radeon RX 6600/6600 XT/6600M]", + "73ff", + "gfx1032", + false, + ), + ( + "Navi 24 [Radeon RX 6400/6500 XT/6500M]", + "743f", + "gfx1034", + false, + ), + ("Navi 48 [Radeon RX 9070/9070 XT]", "7550", "gfx1201", false), + // Datacenter. + ("Aqua Vanjaram [Instinct MI300X]", "74a1", "gfx942", false), + // APUs, oldest first. Every one of these is an integrated GPU sharing + // system memory; `has_apu` must be true on a host that has one. + ("Renoir", "1636", "gfx90c", true), + ("Cezanne", "1638", "gfx90c", true), + ("Lucienne", "164c", "gfx90c", true), + ("Barcelo", "15e7", "gfx90c", true), + ("VanGogh [AMD Custom GPU 0405]", "163f", "gfx1033", true), + ("Rembrandt [Radeon 680M]", "1681", "gfx1035", true), + ("Raphael", "164e", "gfx1036", true), + ("Phoenix1", "15bf", "gfx1103", true), + ("Phoenix3", "1900", "gfx1103", true), + ("Strix [Radeon 880M / 890M]", "150e", "gfx1150", true), + ("Krackan Point [Radeon 860M]", "1114", "gfx1152", true), + // Strix Halo, as reported in ROCm/rocm-cli#449: pci.ids has no entry, so + // `lspci` prints the bare word "Device". + ("Device", "1586", "gfx1151", true), +]; + +/// PCI classes an `lspci -nn` line can carry, with the ones this probe must +/// enumerate first and the bridge (which it must not) last. +const LSPCI_CLASSES: &[(&str, bool)] = &[ + ("VGA compatible controller [0300]", true), + ("Display controller [0380]", true), + ("3D controller [0302]", true), + ("Processing accelerators [1200]", true), + ("PCI bridge [0604]", false), +]; + +/// AMD marketing names as `rocminfo`'s `Marketing Name:` and Windows' +/// `Win32_VideoController.Name` report them, with ground truth about the part. +/// +/// `(name, true_gfx_target, is_really_an_apu)`. +const AMD_MARKETING_NAMES: &[(&str, &str, bool)] = &[ + ("AMD Radeon RX 7900 XTX", "gfx1100", false), + ("AMD Radeon RX 7800 XT", "gfx1101", false), + ("AMD Radeon RX 7600", "gfx1102", false), + ("AMD Radeon RX 9070 XT", "gfx1201", false), + ("AMD Radeon PRO W7900", "gfx1100", false), + ("AMD Instinct MI300X", "gfx942", false), + ("AMD Radeon RX 6900 XT", "gfx1030", false), + // APUs. + ("AMD Radeon(TM) 610M Graphics", "gfx1036", true), + ("AMD Radeon(TM) 660M Graphics", "gfx1035", true), + ("AMD Radeon(TM) 680M Graphics", "gfx1035", true), + ("AMD Radeon(TM) 740M Graphics", "gfx1103", true), + ("AMD Radeon(TM) 760M Graphics", "gfx1103", true), + ("AMD Radeon(TM) 780M Graphics", "gfx1103", true), + ("AMD Radeon(TM) 820M Graphics", "gfx1153", true), + ("AMD Radeon(TM) 840M Graphics", "gfx1152", true), + ("AMD Radeon(TM) 860M Graphics", "gfx1152", true), + ("AMD Radeon(TM) 880M Graphics", "gfx1150", true), + ("AMD Radeon(TM) 890M Graphics", "gfx1150", true), + ("AMD Radeon 8040S Graphics", "gfx1151", true), + ("AMD Radeon 8050S Graphics", "gfx1151", true), + ("AMD Radeon 8060S Graphics", "gfx1151", true), + ("AMD Custom GPU 0405", "gfx1033", true), +]; + +// --------------------------------------------------------------------------- +// Mutators: the perturbations real-world messiness applies to these strings. +// --------------------------------------------------------------------------- + +/// Textual perturbations applied to a fixture-derived string. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum Mutation { + None, + Upper, + Lower, + PadInnerWhitespace, + PadOuterWhitespace, + TrademarkDecoration, + RegisteredDecoration, + TruncateTail, + TruncateHead, + DropBrackets, + TabsForSpaces, +} + +fn mutation() -> impl Strategy { + prop_oneof![ + 6 => Just(Mutation::None), + 2 => Just(Mutation::Upper), + 2 => Just(Mutation::Lower), + 2 => Just(Mutation::PadInnerWhitespace), + 2 => Just(Mutation::PadOuterWhitespace), + 2 => Just(Mutation::TrademarkDecoration), + 1 => Just(Mutation::RegisteredDecoration), + 1 => Just(Mutation::TruncateTail), + 1 => Just(Mutation::TruncateHead), + 1 => Just(Mutation::DropBrackets), + 1 => Just(Mutation::TabsForSpaces), + ] +} + +/// The mutations that preserve the *identity* of the device being named. +/// +/// Case, whitespace and vendor decorations do; truncation and bracket removal +/// can destroy the token a lookup keys on, so properties that assert a +/// device-specific verdict draw from this narrower set rather than filtering +/// the wider one — a filter spends the generator's reject budget on draws it +/// was always going to discard, and proptest aborts the test when that budget +/// runs out. +fn identity_preserving_mutation() -> impl Strategy { + prop_oneof![ + 6 => Just(Mutation::None), + 2 => Just(Mutation::Upper), + 2 => Just(Mutation::Lower), + 2 => Just(Mutation::PadInnerWhitespace), + 2 => Just(Mutation::PadOuterWhitespace), + 2 => Just(Mutation::TrademarkDecoration), + 1 => Just(Mutation::RegisteredDecoration), + 1 => Just(Mutation::TabsForSpaces), + ] +} + +fn apply_mutation(text: &str, m: Mutation) -> String { + match m { + Mutation::None => text.to_owned(), + Mutation::Upper => text.to_uppercase(), + Mutation::Lower => text.to_lowercase(), + Mutation::PadInnerWhitespace => text.replace(' ', " "), + Mutation::PadOuterWhitespace => format!(" {text}\t "), + Mutation::TrademarkDecoration => text.replacen("Radeon", "Radeon(TM)", 1), + Mutation::RegisteredDecoration => text.replacen("AMD", "AMD(R)", 1), + Mutation::TruncateTail => { + let keep = text.len().saturating_mul(2) / 3; + text.chars().take(keep).collect() + } + Mutation::TruncateHead => { + let drop = text.len() / 4; + text.chars().skip(drop).collect() + } + Mutation::DropBrackets => text.replace(['[', ']'], ""), + Mutation::TabsForSpaces => text.replace(' ', "\t"), + } +} + +// --------------------------------------------------------------------------- +// Strategies +// --------------------------------------------------------------------------- + +/// A plausible PCI address, including the host-bridge-adjacent high bus numbers +/// a Strix Halo iGPU enumerates on (`0000:66:00.0` in #449). +fn pci_address() -> impl Strategy { + (0u32..2, 0u32..0x100, 0u32..0x20, 0u32..8) + .prop_map(|(dom, bus, dev, func)| format!("{dom:04x}:{bus:02x}:{dev:02x}.{func}")) +} + +/// A full `lspci -nn -D` line drawn from the real corpus, with the real vendor +/// tag, an optional revision suffix, and a mutation applied. +fn lspci_line() -> impl Strategy +{ + ( + pci_address(), + proptest::sample::select(LSPCI_CLASSES), + proptest::sample::select(AMD_LSPCI_DEVICES), + proptest::option::of(0u32..0x100), + mutation(), + ) + .prop_map(|(addr, class, device, rev, m)| { + let rev = rev.map_or_else(String::new, |r| format!(" (rev {r:02x})")); + let line = format!( + "{addr} {}: Advanced Micro Devices, Inc. [AMD/ATI] {} [1002:{}]{rev}", + class.0, device.0, device.1 + ); + (apply_mutation(&line, m), device) + }) +} + +/// A marketing name drawn from the real corpus with a mutation applied, paired +/// with the ground truth for the part it names. +fn marketing_name() -> impl Strategy +{ + (proptest::sample::select(AMD_MARKETING_NAMES), mutation()) + .prop_map(|(entry, m)| (apply_mutation(entry.0, m), entry, m)) +} + +/// The same, restricted to spellings that still name the same device. +fn marketing_name_spelled_differently() +-> impl Strategy { + ( + proptest::sample::select(AMD_MARKETING_NAMES), + identity_preserving_mutation(), + ) + .prop_map(|(entry, m)| (apply_mutation(entry.0, m), entry)) +} + +// --------------------------------------------------------------------------- +// Properties +// --------------------------------------------------------------------------- + +proptest! { + #![proptest_config(ProptestConfig::with_cases(2048))] + + /// Totality and determinism: no fixture-derived or mutated line may panic + /// any of the pure classifiers, and repeated calls must agree. + #[test] + fn classification_is_total_and_deterministic((line, _) in lspci_line()) { + prop_assert_eq!(is_lspci_gpu_line(&line), is_lspci_gpu_line(&line)); + let name = extract_lspci_name(&line); + prop_assert_eq!(&name, &extract_lspci_name(&line)); + let verdict = classify_amd_marketing_name(&name); + prop_assert_eq!(&verdict, &classify_amd_marketing_name(&name)); + prop_assert_eq!(gfx_is_apu_family(&verdict.0), gfx_is_apu_family(&verdict.0)); + } + + /// `extract_lspci_name` must recover exactly the vendor-plus-device text of + /// an unmutated `lspci -nn` line: everything between the class `]:` and the + /// trailing `[vendor:device]`. + #[test] + fn lspci_name_extraction_round_trips( + addr in pci_address(), + class in proptest::sample::select(LSPCI_CLASSES), + device in proptest::sample::select(AMD_LSPCI_DEVICES), + rev in proptest::option::of(0u32..0x100), + ) { + let rev = rev.map_or_else(String::new, |r| format!(" (rev {r:02x})")); + let line = format!( + "{addr} {}: Advanced Micro Devices, Inc. [AMD/ATI] {} [1002:{}]{rev}", + class.0, device.0, device.1 + ); + let expected = format!("Advanced Micro Devices, Inc. [AMD/ATI] {}", device.0); + prop_assert_eq!(extract_lspci_name(&line), expected); + } + + /// Classification must not depend on spelling: case, inner/outer + /// whitespace and vendor decorations name the same device. + #[test] + fn classification_is_spelling_invariant((name, truth) in marketing_name_spelled_differently()) { + let canonical = classify_amd_marketing_name(truth.0); + let mutated = classify_amd_marketing_name(&name); + prop_assert_eq!( + &canonical, + &mutated, + "{:?} and {:?} are the same device but classify differently", + truth.0, + name, + ); + } + + /// The Windows display probe and `examine`'s own Windows GPU enumeration + /// read the same row and must not disagree about the gfx target. + #[test] + fn windows_probes_agree_on_gfx_target( + entry in proptest::sample::select(AMD_MARKETING_NAMES), + subsys in 0u32..0x1_0000, + ) { + // Only coherent rows: the PNP id must name the same part the marketing + // name does, or the two probes are being asked about different GPUs. + let Some(device_id) = entry_device_id(entry.0) else { + return Ok(()); + }; + // A Win32_VideoController row as `probe_gpus_windows` reads it: name, + // driver version, PNP device id. + let pnp = format!("PCI\\VEN_1002&DEV_{device_id}&SUBSYS_{subsys:04x}1002&REV_C1"); + let row = format!("{}\t32.0.1\t{pnp}", entry.0); + let install_target = crate::parse_windows_display_gfx_target(&row); + let examine_target = classify_amd_marketing_name(entry.0).0; + if let Some(install_target) = install_target + && !examine_target.is_empty() + { + prop_assert_eq!( + &examine_target, + &install_target, + "the Windows display probe and examine disagree about {}", + entry.0, + ); + } + } + +} + +/// The PCI device id for a marketing name, when the corpus pins one. +fn entry_device_id(name: &str) -> Option<&'static str> { + match name { + "AMD Radeon(TM) 610M Graphics" => Some("164e"), + "AMD Radeon(TM) 660M Graphics" | "AMD Radeon(TM) 680M Graphics" => Some("1681"), + "AMD Radeon(TM) 740M Graphics" + | "AMD Radeon(TM) 760M Graphics" + | "AMD Radeon(TM) 780M Graphics" => Some("15bf"), + "AMD Radeon(TM) 860M Graphics" | "AMD Radeon(TM) 840M Graphics" => Some("1114"), + "AMD Custom GPU 0405" => Some("163f"), + "AMD Radeon RX 9070 XT" => Some("7550"), + _ => None, + } +} + +/// Measure how far the generators actually reach, by drawing from them +/// directly and tallying which branches each draw lands in. +/// +/// A generator that never samples the interesting region passes every property +/// vacuously, so the reach is asserted, not assumed: a regression that stops +/// the corpus from parsing (say, a changed `lspci` line shape) must fail here +/// rather than silently turn the whole file green. +#[test] +fn generator_reach_is_measured_and_sufficient() { + const DRAWS: u32 = 4096; + let mut runner = TestRunner::new(Config::with_cases(DRAWS)); + + let mut lspci_parsed = 0u64; + let mut lspci_gpu_class = 0u64; + let mut lspci_apu_device = 0u64; + let mut lspci_classified_apu = 0u64; + let mut lspci_gfx_resolved = 0u64; + for _ in 0..DRAWS { + let (line, device) = lspci_line() + .new_tree(&mut runner) + .expect("lspci strategy") + .current(); + if is_lspci_gpu_line(&line) { + lspci_gpu_class += 1; + } + let name = extract_lspci_name(&line); + if !name.is_empty() { + lspci_parsed += 1; + } + if device.3 { + lspci_apu_device += 1; + } + let (gfx, is_apu) = classify_amd_marketing_name(&name); + if is_apu { + lspci_classified_apu += 1; + } + if !gfx.is_empty() { + lspci_gfx_resolved += 1; + } + } + + let mut marketing_examine = 0u64; + let mut marketing_install = 0u64; + let mut marketing_both = 0u64; + let mut marketing_apu_truth = 0u64; + for _ in 0..DRAWS { + let (name, truth, _m) = marketing_name() + .new_tree(&mut runner) + .expect("marketing strategy") + .current(); + let examine = classify_amd_marketing_name(&name).0; + let install = crate::gfx_target_from_amd_marketing_name(&name); + if !examine.is_empty() { + marketing_examine += 1; + } + if install.is_some() { + marketing_install += 1; + } + if !examine.is_empty() && install.is_some() { + marketing_both += 1; + } + if truth.2 { + marketing_apu_truth += 1; + } + } + + let pct = |n: u64| (n as f64) * 100.0 / f64::from(DRAWS); + println!( + "generator reach over {DRAWS} draws each:\n\ + lspci lines : name parsed {:.1}%, GPU-class {:.1}%, APU silicon {:.1}%, \ + classified APU {:.1}%, gfx resolved {:.1}%\n\ + marketing names : examine resolved {:.1}%, install resolved {:.1}%, both {:.1}%, \ + APU silicon {:.1}%", + pct(lspci_parsed), + pct(lspci_gpu_class), + pct(lspci_apu_device), + pct(lspci_classified_apu), + pct(lspci_gfx_resolved), + pct(marketing_examine), + pct(marketing_install), + pct(marketing_both), + pct(marketing_apu_truth), + ); + // Floors, not exact counts: the draws are random, but a generator that + // reaches none of these is worthless. + assert!( + lspci_parsed > u64::from(DRAWS) / 2, + "fewer than half the lspci draws produced a parseable device name" + ); + // A third, not a half: one class in five is the PCI bridge the probe must + // *not* enumerate, and the case/tab mutations deliberately break the + // case-sensitive class match so the negative branch is sampled too. + assert!( + lspci_gpu_class > u64::from(DRAWS) / 3, + "fewer than a third of the lspci draws landed on an enumerated GPU class" + ); + assert!( + lspci_apu_device > u64::from(DRAWS) / 4, + "the lspci corpus barely samples APU silicon" + ); + assert!( + lspci_gfx_resolved > 0, + "no lspci draw ever resolved a gfx target" + ); + assert!( + marketing_both > u64::from(DRAWS) / 8, + "the two marketing tables almost never both answer, so the agreement \ + property is near-vacuous" + ); +} From ac1696e65e07537aff9537f4fc32c3a753f49904 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Fri, 2 Oct 2026 12:34:37 +0000 Subject: [PATCH 2/5] fix(examine): classify integrated AMD GPUs by target, not by prefix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `gfx_is_apu_family` matched a gfx110x / gfx115x prefix, so every pre-RDNA3 APU read as a discrete card: gfx90c (Renoir, Cezanne, Lucienne, Barcelo), gfx1033 (Van Gogh), gfx1035 (Radeon 680M/660M) and gfx1036, which is the iGPU on every Ryzen 7000-and-later desktop part. Two consequences a user sees. `serve` compares reported VRAM against a low-memory floor unless the GPU is an APU, so on those machines it measured the BIOS carve-out and warned about low VRAM while the model was being served out of system RAM — the exact false warning that gate exists to withhold. And `diagnose` gates the iGPU+dGPU collision check on `has_apu && has_discrete_amd`, which never both held, so the check never fired on the host shape it was written for. Packaging is a property of the silicon, so it is now stated once, keyed by target, rather than inferred from a numeric prefix that two different families share. The table also carries gfx902, gfx909 and gfx1037, which no lookup table produces — `gfx_target_from_gc_version` synthesises them from the GC version in DRM ip-discovery — and gfx1250, whose family the CLI already installs. It is not every target AMD has shipped and cannot be; an unlisted target answers "unknown", not "discrete". `examine` also kept its own marketing-name table. It resolved a strict subset of what the crate's other table resolves — 28% of a generated corpus against 92% — and the two disagreed about Krackan Point, which it called gfx1150 where the PCI device-id table and the 860M/840M SKUs it ships as both say gfx1152. Those are separate TheRock build families, so the disagreement selected the wrong runtime build. Beside it sat a third list, of APU codename fragments, that answered "is this an APU?" without ever producing a target, so a Renoir iGPU came back with an empty gfx target. Both are gone: one table maps a name to a target, and the packaging follows from the target. Folding those codenames in makes gfx90c reachable from a name for the first time, and `normalize_therock_family` would have filed it under `gfx90X-dcgpu` — a datacenter family, which `preferred_serve_engine_for_ therock_family` also reads as "prefer vLLM". No published family covers a Renoir-class iGPU, so it now answers none and the user is asked to choose. Both GPU probes prefer the PCI device id to the marketing string, since the id names the part where the string only hints at it: on Windows out of the PNP id, on Linux out of the `[1002:xxxx]` tag `lspci -nn` already prints. An adapter named "AMD Radeon(TM) 840M Graphics" resolves gfx1152 rather than nothing. No Gherkin scenario accompanies this (AGENTS.md §3). All three observable behaviours need a host with a pre-RDNA3 AMD APU, and no lane has one — the Strix Halo lanes report gfx1151, which was already classified as integrated before this change, so a scenario there would pass either way. `examine` reads the real host with no injection seam, so the behaviour cannot be planted either. Coverage sits one level down instead: the collision check runs through the real diagnosis catalog on a synthesised Raphael + RX 7900 XTX host, and the Rembrandt laptop case drives the real post-PCI probe sequence against a planted KFD topology. Signed-off-by: Roman Inflianskas --- apps/rocm/src/main.rs | 24 + crates/rocm-core/src/examine.rs | 325 +++++++++--- crates/rocm-core/src/examine_proptests.rs | 593 +++++++++++++++++++++- crates/rocm-core/src/lib.rs | 108 +++- 4 files changed, 964 insertions(+), 86 deletions(-) diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index 1f0655acc..94dd417cc 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -31458,6 +31458,30 @@ install therock"; assert!(vram_capacity_is_meaningful(None, 1)); } + /// Every APU, not just the RDNA3+ ones. + /// + /// `gfx_is_apu_family` used to recognise only gfx1103 and gfx115x, so the + /// pre-RDNA3 APUs below were read as discrete parts with real private VRAM. + /// They have none: the BIOS carve-out this compares against is a few hundred + /// MB on a Ryzen desktop iGPU, so `rocm serve` warned about "low VRAM" on a + /// 64 GB machine that is serving the model out of system RAM — the exact + /// false warning this gate exists to withhold. (Van Gogh is the Steam Deck.) + #[test] + fn vram_capacity_is_withheld_on_every_apu_not_just_rdna3() { + for (target, part) in [ + ("gfx90c", "Renoir / Cezanne / Lucienne / Barcelo"), + ("gfx1033", "Van Gogh"), + ("gfx1035", "Rembrandt, Radeon 680M"), + ("gfx1036", "Raphael, Radeon 610M"), + ] { + assert!( + !vram_capacity_is_meaningful(Some(target), 1), + "{part} ({target}) is an APU with no private VRAM, but its \ + carve-out is being treated as a real capacity" + ); + } + } + /// Every distro whose plan actually emits privileged commands, so the /// escalation tests below sweep all of them rather than whichever one was /// remembered. Adding a distro to the planner without adding it here would diff --git a/crates/rocm-core/src/examine.rs b/crates/rocm-core/src/examine.rs index 19f4219a1..01b60b126 100644 --- a/crates/rocm-core/src/examine.rs +++ b/crates/rocm-core/src/examine.rs @@ -57,22 +57,6 @@ const AMDGPU_INSTALL_MARKERS: &[&str] = &[ "/etc/yum.repos.d/rocm.repo", ]; -/// Marketing-name fragments that identify an AMD APU when `rocminfo` is absent. -const APU_KEYWORDS: &[&str] = &[ - "strix halo", - "ryzen ai max", - "phoenix", - "hawk point", - "strix point", - "krackan", - "rembrandt", - "raphael", - "barcelo", - "lucienne", - "renoir", - "cezanne", -]; - /// A single GPU as enumerated by `lspci`/`rocminfo` (Linux) or the display /// inventory (Windows). #[derive(Debug, Clone, Default, Serialize, Deserialize, PartialEq, Eq)] @@ -993,45 +977,130 @@ fn probe_cpu_windows(e: &mut Examination) { // --------------------------------------------------------------------------- /// Best-effort `(gfx_target, is_apu)` for an AMD marketing name. +/// +/// Both halves come from the crate's own marketing-name table: the target by +/// lookup, the packaging by classifying that target. `examine` used to carry a +/// second, smaller copy of the same name→target knowledge, which resolved a +/// strict subset of what [`crate::gfx_target_from_amd_marketing_name`] resolves +/// and disagreed with it on Krackan Point. It also carried a third list of +/// codename fragments that only answered "APU?" and never produced a target, so +/// a Renoir iGPU came back with an empty `gfx_target`. One table answers both +/// questions now, and a name whose target is unknown is reported as unknown +/// rather than guessed at. fn classify_amd_marketing_name(name: &str) -> (String, bool) { - let mut n = name.to_lowercase(); - for deco in ["(tm)", "(r)", "(c)", "(\u{2122})"] { - n = n.replace(deco, " "); - } - let n = n.split_whitespace().collect::>().join(" "); - let contains = |needle: &str| n.contains(needle); - if contains("ryzen ai max") || contains("strix halo") { - return ("gfx1151".to_owned(), true); - } - if contains("radeon 8050s") || contains("radeon 8060s") || contains("radeon 8045s") { - return ("gfx1151".to_owned(), true); - } - if contains("radeon 880m") - || contains("radeon 890m") - || contains("strix point") - || contains("krackan") - { - return ("gfx1150".to_owned(), true); - } - if contains("radeon 780m") - || contains("radeon 760m") - || contains("radeon 740m") - || contains("phoenix") - || contains("hawk point") - { - return ("gfx1103".to_owned(), true); - } - (String::new(), APU_KEYWORDS.iter().any(|kw| n.contains(kw))) + let gfx = crate::gfx_target_from_amd_marketing_name(name).unwrap_or_default(); + (gfx.to_owned(), gfx_is_apu_family(gfx)) } -/// Whether a gfx target belongs to an AMD APU family. +/// How an AMD gfx target is packaged: on the CPU package, sharing system +/// memory, or on a card of its own with private VRAM. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum GfxPackaging { + /// An APU / integrated GPU. No private VRAM: what the driver reports as + /// "VRAM" is a carve-out of system RAM. + Integrated, + /// A discrete GPU or datacenter accelerator, with its own memory. + Discrete, +} + +/// How each AMD gfx target this crate can encounter is packaged. +/// +/// Target → packaging is a property of the *silicon*, so it is keyed by target +/// and stated once here rather than being spread across the name and device-id +/// tables, which are keyed by marketing string and PCI id respectively and +/// would each have to repeat it. It is not derived from those tables either: a +/// target's packaging must not depend on whether someone happened to list a SKU +/// name for it. `every_target_the_lookup_tables_produce_has_a_packaging` ties +/// the three together so the set cannot silently fall behind them. /// -/// APUs: gfx1103 (Phoenix / Hawk Point) and the gfx115x parts (Strix Point / -/// Strix Halo). Their neighbors gfx1100 / gfx1101 / gfx1102 (Navi 31 / 32 / 33) -/// share the gfx110x prefix but are *discrete* RDNA3 GPUs, so they must not -/// match — otherwise they inflate `has_apu` and suppress `has_discrete_amd`, -/// which gates the iGPU+dGPU collision fix. (Target -> product per LLVM -/// AMDGPUUsage.) +/// Two limits, both deliberate. +/// +/// This is not every target AMD has ever shipped, and it cannot be: a part +/// nobody here has seen is `None`, not "discrete" — see +/// [`gfx_target_packaging`]. The drift guard only covers targets the lookup +/// tables produce, so targets that reach the CLI another way (notably +/// [`crate::gfx_target_from_gc_version`], which synthesises one from the GC +/// version in DRM ip-discovery and so can name a part no table lists) have to +/// be added here by hand. +/// +/// And the "packaging is a property of the target" premise has one known +/// exception: gfx942 covers both the MI300X accelerator and the MI300A, which +/// is an APU with unified HBM. It is listed `Discrete` because MI300X is what +/// the ROCm hosts here run and because that is the answer the CLI has always +/// given; an MI300A is therefore classified wrongly, and telling them apart +/// needs a signal this table does not have. +const GFX_TARGET_PACKAGING: &[(&str, GfxPackaging)] = &[ + // Integrated Vega: Raven Ridge (Ryzen 2000), Picasso (Ryzen 3000), and the + // Ryzen 4000/5000 iGPUs (Renoir, Cezanne, Lucienne, Barcelo). + ("gfx902", GfxPackaging::Integrated), + ("gfx909", GfxPackaging::Integrated), + ("gfx90c", GfxPackaging::Integrated), + // Integrated RDNA2: Van Gogh (Steam Deck), Rembrandt (Radeon 680M/660M), + // Raphael (Radeon 610M, every Ryzen 7000+ desktop iGPU), Mendocino. + ("gfx1033", GfxPackaging::Integrated), + ("gfx1035", GfxPackaging::Integrated), + ("gfx1036", GfxPackaging::Integrated), + ("gfx1037", GfxPackaging::Integrated), + // Integrated RDNA3 / RDNA3.5: Phoenix, Hawk Point, Strix Point, Strix Halo, + // Krackan Point. + ("gfx1103", GfxPackaging::Integrated), + ("gfx1150", GfxPackaging::Integrated), + ("gfx1151", GfxPackaging::Integrated), + ("gfx1152", GfxPackaging::Integrated), + ("gfx1153", GfxPackaging::Integrated), + // Discrete GCN/CDNA. See the gfx942 caveat above. + ("gfx900", GfxPackaging::Discrete), + ("gfx906", GfxPackaging::Discrete), + ("gfx908", GfxPackaging::Discrete), + ("gfx90a", GfxPackaging::Discrete), + ("gfx942", GfxPackaging::Discrete), + ("gfx950", GfxPackaging::Discrete), + // Discrete RDNA1 (Navi 10 / 12 / 14). + ("gfx1010", GfxPackaging::Discrete), + ("gfx1011", GfxPackaging::Discrete), + ("gfx1012", GfxPackaging::Discrete), + // Discrete RDNA2 (Navi 21 / 22 / 23 / 24). + ("gfx1030", GfxPackaging::Discrete), + ("gfx1031", GfxPackaging::Discrete), + ("gfx1032", GfxPackaging::Discrete), + ("gfx1034", GfxPackaging::Discrete), + // Discrete RDNA3 (Navi 31 / 32 / 33). These share the gfx110x prefix with + // the Phoenix APU above and must stay distinct from it: they drive + // `has_discrete_amd`, which gates the iGPU+dGPU collision fix. + ("gfx1100", GfxPackaging::Discrete), + ("gfx1101", GfxPackaging::Discrete), + ("gfx1102", GfxPackaging::Discrete), + // Discrete RDNA4 (Navi 44 / 48). + ("gfx1200", GfxPackaging::Discrete), + ("gfx1201", GfxPackaging::Discrete), + // Discrete datacenter: the gfx125X-dcgpu family the CLI already installs. + ("gfx1250", GfxPackaging::Discrete), +]; + +/// How a gfx target is packaged, or `None` when this crate has no entry for it. +/// +/// The three-way answer is the point. `false` from [`gfx_is_apu_family`] cannot +/// distinguish "this is a discrete card" from "never heard of this target", and +/// callers that fold a target into an existing verdict need to: an unrecognised +/// target is not evidence of a discrete GPU, so it must not overwrite what +/// another probe already established. See [`apply_rocminfo_gpu_agents`]. +/// +/// Leading `gfx` plus the alphanumeric run is what is matched, so a feature +/// suffix (`gfx1036:sramecc+:xnack-`) or a generic-target suffix +/// (`gfx1036-generic`) names the same part, and case does not matter. +fn gfx_target_packaging(gfx: &str) -> Option { + let gfx = gfx.trim().to_ascii_lowercase(); + let base: String = gfx + .chars() + .take_while(char::is_ascii_alphanumeric) + .collect(); + GFX_TARGET_PACKAGING + .iter() + .find(|(target, _)| *target == base) + .map(|(_, packaging)| *packaging) +} + +/// Whether a gfx target belongs to an AMD APU family. /// /// Two consumers rely on this, for different reasons: /// @@ -1043,25 +1112,13 @@ fn classify_amd_marketing_name(name: &str) -> (String, bool) { /// This answers a question about the *part*, not about the host: a machine can /// pair an APU with a discrete card, so a true verdict here does not mean every /// GPU on the host is integrated. +/// +/// A target this crate does not recognise answers `false`, which is the right +/// default for both consumers — neither may treat an unknown part as unified +/// memory. Callers that must tell "no" apart from "don't know" use +/// [`gfx_target_packaging`] instead. pub fn gfx_is_apu_family(gfx: &str) -> bool { - let g = gfx.to_lowercase(); - // gfx115x: every Strix part is an APU. - if gfx_model_digit(&g, "gfx115").is_some() { - return true; - } - // gfx110x: only gfx1103 and above are APUs; gfx1100/1101/1102 are discrete. - if let Some(digit) = gfx_model_digit(&g, "gfx110") { - return digit >= 3; - } - false -} - -/// The model digit that follows `prefix` in a gfx target, e.g. `3` from -/// `gfx1103` given prefix `gfx110`. Returns `None` when `gfx` does not start -/// with `prefix` or has no digit there. Any trailing feature suffix (such as -/// `:sramecc+:xnack-`) is ignored, matching how gcnArchName can be reported. -fn gfx_model_digit(gfx: &str, prefix: &str) -> Option { - gfx.strip_prefix(prefix)?.chars().next()?.to_digit(10) + gfx_target_packaging(gfx) == Some(GfxPackaging::Integrated) } /// Whether an `lspci -nn` line describes a GPU this probe should enumerate. @@ -1121,17 +1178,43 @@ fn probe_gpus_lspci(e: &mut Examination) { if !is_amd { continue; } - let (gfx_guess, is_apu_guess) = classify_amd_marketing_name(&name); + // `-nn` puts the PCI device id on the line, and this crate decodes ids + // already, so prefer it to the name for the same reason the Windows + // probe prefers the PNP id: the id names the part, the marketing string + // only hints at it. It matters most where there is no hint at all — + // when `pci.ids` has no entry, `lspci` prints the bare word "Device" + // and the name carries nothing. + let gfx_guess = extract_lspci_device_id(line) + .and_then(|device_id| crate::gfx_target_from_amd_pci_device_id(&device_id)) + .map_or_else(|| classify_amd_marketing_name(&name).0, str::to_owned); e.gpus.push(Gpu { name, + is_apu: Some(gfx_is_apu_family(&gfx_guess)), gfx_target: gfx_guess, pci_id, - is_apu: Some(is_apu_guess), is_amd: true, }); } } +/// The AMD PCI device id an `lspci -nn` line carries, as the four hex digits +/// after `1002:` in the trailing `[vendor:device]` tag. +/// +/// Only AMD's vendor id is accepted: the tag is what distinguishes a device id +/// from the several other bracketed numbers on the line (the PCI class, and any +/// subsystem name `pci.ids` supplies), and a four-digit number lifted from one +/// of those would decode to an unrelated part. +fn extract_lspci_device_id(line: &str) -> Option { + let lower = line.to_ascii_lowercase(); + let start = lower.rfind("[1002:")? + "[1002:".len(); + let device_id: String = lower[start..] + .chars() + .take_while(char::is_ascii_hexdigit) + .take(4) + .collect(); + (device_id.len() == 4).then_some(device_id) +} + /// Pull the marketing name out of an `lspci -nn` line: the text between the /// controller-kind `]:` and the trailing `[vendor:device]`. fn extract_lspci_name(line: &str) -> String { @@ -1246,7 +1329,7 @@ fn apply_rocminfo_gpu_agents(e: &mut Examination, out: &str) { } gpu.is_apu = Some(gfx_is_apu_family(&gfx)); } else { - let is_apu = gfx_is_apu_family(&gfx); + let is_apu = Some(gfx_is_apu_family(&gfx)); e.gpus.push(Gpu { name: if marketing.is_empty() { "AMD GPU".to_owned() @@ -1255,7 +1338,7 @@ fn apply_rocminfo_gpu_agents(e: &mut Examination, out: &str) { }, gfx_target: gfx, is_amd: true, - is_apu: Some(is_apu), + is_apu, ..Gpu::default() }); } @@ -2293,12 +2376,18 @@ fn probe_gpus_windows(e: &mut Examination) { if !is_amd { continue; } - let (gfx_guess, is_apu_guess) = classify_amd_marketing_name(&name); + // The PNP id on this very row carries the PCI device id, which names + // the part exactly where the marketing string only hints at it — a + // "Radeon(TM) 840M Graphics" row resolves nothing by name alone on the + // tables that predate this, but `DEV_1114` is unambiguous. Same + // precedence, and the same decoder, as the install-side display probe. + let gfx_guess = crate::parse_windows_display_gfx_target(line) + .unwrap_or_else(|| classify_amd_marketing_name(&name).0); e.gpus.push(Gpu { name, + is_apu: Some(gfx_is_apu_family(&gfx_guess)), gfx_target: gfx_guess, pci_id: pnp, - is_apu: Some(is_apu_guess), is_amd: true, }); } @@ -2391,7 +2480,7 @@ fn probe_msvc_redist_windows(e: &mut Examination) { e.msvc_redist_present = Some(present); } -/// Property-based coverage of the pure GPU classifiers above. +/// Property-based coverage of the pure GPU/driver classifiers above. /// /// A child module of `examine` rather than a sibling, so it can reach the /// private classifiers here *and* the crate-root install-family tables it has @@ -3861,17 +3950,97 @@ mod tests { assert!(gfx_is_apu_family("gfx1151")); assert!(gfx_is_apu_family("gfx1152")); assert!(gfx_is_apu_family("gfx1153")); + // The pre-RDNA3 APUs, which a prefix rule over gfx110x / gfx115x could + // not reach: a Ryzen 7000+ desktop ships gfx1036 on every single SKU, + // and reading it as a discrete card is what produced a low-VRAM + // warning against a BIOS carve-out on hosts serving from system RAM. + assert!(gfx_is_apu_family("gfx90c")); + assert!(gfx_is_apu_family("gfx1033")); + assert!(gfx_is_apu_family("gfx1035")); + assert!(gfx_is_apu_family("gfx1036")); + // gfx90a is the MI200 accelerator, one character away from the Renoir + // iGPU above and emphatically not an APU. + assert!(!gfx_is_apu_family("gfx90a")); + // Unrelated families are never APUs. + assert!(!gfx_is_apu_family("gfx1200")); + assert!(!gfx_is_apu_family("gfx942")); + // The targets no lookup table produces, so the cross-table drift guard + // cannot reach them: `gfx_target_from_gc_version` builds these straight + // out of the GC version in DRM ip-discovery. gfx1037 is Mendocino, + // which sells as a Radeon 610M exactly like gfx1036 does. + assert!(gfx_is_apu_family("gfx902")); + assert!(gfx_is_apu_family("gfx909")); + assert!(gfx_is_apu_family("gfx1037")); + // gfx90a is the MI200 accelerator, one character away from the Renoir + // iGPU above and emphatically not an APU. + assert!(!gfx_is_apu_family("gfx90a")); // Unrelated families are never APUs. assert!(!gfx_is_apu_family("gfx1200")); assert!(!gfx_is_apu_family("gfx942")); - // A trailing gcnArchName feature suffix must not change the verdict. - assert!(gfx_is_apu_family("gfx1103:sramecc+:xnack-")); + assert!(!gfx_is_apu_family("gfx1250")); + // A trailing gcnArchName feature suffix must not change the verdict, + // and neither does a generic-target suffix or upper case. Spelled on + // targets outside gfx110x/gfx115x on purpose: a prefix rule over those + // two answers `gfx1151-generic` correctly by accident, so an assertion + // written there would pass with the suffix handling removed. + assert!(gfx_is_apu_family("gfx1036:sramecc+:xnack-")); assert!(!gfx_is_apu_family("gfx1100:xnack-")); + assert!(gfx_is_apu_family("GFX90C")); + assert!(gfx_is_apu_family("gfx1036-generic")); // Degenerate inputs never match. assert!(!gfx_is_apu_family("gfx110")); assert!(!gfx_is_apu_family("")); } + /// The `lspci -nn` device id is preferred to the marketing string, and only + /// AMD's vendor tag is read as one. + #[test] + fn the_lspci_device_id_is_read_from_the_amd_vendor_tag() { + // Krackan Point: the id says gfx1152, and so does the name here — but + // the id is what is consulted. + assert_eq!( + extract_lspci_device_id( + "0000:c5:00.0 VGA compatible controller [0300]: Advanced Micro Devices, Inc. \ + [AMD/ATI] Krackan Point [Radeon 860M] [1002:1114] (rev c1)" + ) + .as_deref(), + Some("1114") + ); + // The PCI class `[0300]` and any subsystem tag are four-ish numbers on + // the same line; neither is a device id. + assert_eq!( + extract_lspci_device_id( + "0000:01:00.0 VGA compatible controller [0300]: NVIDIA Corporation \ + GA102 [GeForce RTX 3090] [10de:2204] (rev a1)" + ), + None + ); + assert_eq!( + extract_lspci_device_id("no bracketed vendor tag here"), + None + ); + } + + /// An unrecognised target is "cannot say", not "discrete". + /// + /// [`gfx_is_apu_family`] has to collapse that to `false`, which is why the + /// distinction lives in [`gfx_target_packaging`]: callers that fold a + /// target into a verdict another probe already reached need to tell a + /// negative answer apart from no answer. + #[test] + fn an_unknown_target_has_no_packaging_verdict() { + assert_eq!(gfx_target_packaging("gfx9999"), None); + assert_eq!(gfx_target_packaging(""), None); + assert_eq!( + gfx_target_packaging("gfx1036"), + Some(GfxPackaging::Integrated) + ); + assert_eq!( + gfx_target_packaging("gfx1100"), + Some(GfxPackaging::Discrete) + ); + } + #[test] fn apu_plus_discrete_neighbor_sets_both_category_flags() { // A machine with a real APU (gfx1103 Phoenix) and its discrete RDNA3 diff --git a/crates/rocm-core/src/examine_proptests.rs b/crates/rocm-core/src/examine_proptests.rs index 65bef9785..4d62c4b37 100644 --- a/crates/rocm-core/src/examine_proptests.rs +++ b/crates/rocm-core/src/examine_proptests.rs @@ -13,15 +13,10 @@ //! //! Each property states an invariant the *report* must hold, not an //! implementation detail, so a failure names a user-visible defect. -//! -//! What is here is the harness and the invariants the classifiers already -//! satisfy: totality, determinism, round-tripping an `lspci` line, and -//! independence from how a name is spelled. The properties that assert -//! *correct classification* arrive with the fixes that make them pass, so -//! that no commit in this history is red. use super::{ - classify_amd_marketing_name, extract_lspci_name, gfx_is_apu_family, is_lspci_gpu_line, + Examination, GFX_TARGET_PACKAGING, Gpu, apply_rocminfo_gpu_agents, classify_amd_marketing_name, + extract_lspci_name, gfx_is_apu_family, is_lspci_gpu_line, summarise_gpu_categories, }; use proptest::prelude::*; use proptest::strategy::ValueTree; @@ -145,6 +140,46 @@ const AMD_MARKETING_NAMES: &[(&str, &str, bool)] = &[ ("AMD Custom GPU 0405", "gfx1033", true), ]; +/// gfx targets a real host reports, with ground truth about packaging. +/// +/// Not every one of these is produced by a lookup table — gfx902, gfx909 and +/// gfx1037 reach the CLI only through +/// [`crate::gfx_target_from_gc_version`], which synthesises a target from the +/// GC version DRM ip-discovery reports. That is exactly why they are here: the +/// cross-table drift guard cannot see them, so ground truth has to. +const GFX_TARGETS: &[(&str, bool)] = &[ + ("gfx900", false), + ("gfx906", false), + ("gfx908", false), + ("gfx90a", false), + ("gfx942", false), + ("gfx950", false), + ("gfx1010", false), + ("gfx1030", false), + ("gfx1031", false), + ("gfx1032", false), + ("gfx1034", false), + ("gfx1100", false), + ("gfx1101", false), + ("gfx1102", false), + ("gfx1200", false), + ("gfx1201", false), + ("gfx1250", false), + // APU targets. + ("gfx902", true), + ("gfx909", true), + ("gfx90c", true), + ("gfx1033", true), + ("gfx1035", true), + ("gfx1036", true), + ("gfx1037", true), + ("gfx1103", true), + ("gfx1150", true), + ("gfx1151", true), + ("gfx1152", true), + ("gfx1153", true), +]; + // --------------------------------------------------------------------------- // Mutators: the perturbations real-world messiness applies to these strings. // --------------------------------------------------------------------------- @@ -274,6 +309,83 @@ fn marketing_name_spelled_differently() .prop_map(|(entry, m)| (apply_mutation(entry.0, m), entry)) } +/// Marketing names of parts that really are APUs, differently spelled. +fn apu_marketing_name() -> impl Strategy { + let apus: Vec<_> = AMD_MARKETING_NAMES + .iter() + .copied() + .filter(|entry| entry.2) + .collect(); + assert!(!apus.is_empty(), "the marketing corpus has no APU"); + ( + proptest::sample::select(apus), + identity_preserving_mutation(), + ) + .prop_map(|(entry, m)| (apply_mutation(entry.0, m), entry)) +} + +/// `lspci` entries for parts that really are APUs *and* carry a model name. +/// +/// See [`LSPCI_DEVICES_WITHOUT_A_PCI_IDS_NAME`] for why the nameless row is not +/// a candidate for any name-keyed property. +fn apu_lspci_devices() -> Vec<(&'static str, &'static str, &'static str, bool)> { + let apus: Vec<_> = AMD_LSPCI_DEVICES + .iter() + .copied() + .filter(|device| device.3 && !LSPCI_DEVICES_WITHOUT_A_PCI_IDS_NAME.contains(&device.0)) + .collect(); + assert!(!apus.is_empty(), "the lspci corpus has no named APU"); + apus +} + +/// A `rocminfo` agent listing for the given targets, in the agent block shape +/// the current parser can digest (no ISA sub-entries). +fn rocminfo_output(agents: &[(&str, &str)]) -> String { + rocminfo_output_shaped(agents, false) +} + +/// `with_isa_entries` reproduces what every `rocminfo` since ROCm 5 actually +/// prints: each GPU agent is followed by indented ISA sub-entries that *also* +/// begin with `Name:`. The parser keeps a running `cur_name` and lets those +/// lines overwrite the agent's gfx name, so the agent is dropped and the whole +/// fold becomes a no-op — ROCm/rocm-cli#393. +/// +/// Both shapes are generated on purpose. The clean one is the only shape that +/// currently reaches the `is_apu` overwrite, so it is where the APU downgrade +/// shows; the real one shows that the overwrite is unreachable on a real host +/// today, and stops being unreachable the moment #393 is fixed. +fn rocminfo_output_shaped(agents: &[(&str, &str)], with_isa_entries: bool) -> String { + use std::fmt::Write as _; + let mut out = String::from( + "=====================\nHSA System Attributes\n=====================\n\ + Runtime Version: 1.1\n\n", + ); + out.push_str( + "==========\nHSA Agents\n==========\n*******\nAgent 1\n*******\n \ + Name: AMD Ryzen 9 7950X\n \ + Marketing Name: AMD Ryzen 9 7950X\n \ + Device Type: CPU\n", + ); + for (index, (gfx, marketing)) in agents.iter().enumerate() { + let _ = write!( + out, + "*******\nAgent {}\n*******\n Name: {gfx}\n \ + Marketing Name: {marketing}\n Device Type: GPU\n", + index + 2 + ); + if with_isa_entries { + let family = gfx.get(..5).unwrap_or(gfx); + let _ = write!( + out, + " ISA Info:\n ISA 1\n Name: \ + amdgcn-amd-amdhsa--{gfx}\n ISA 2\n Name: \ + amdgcn-amd-amdhsa--{family}-generic\n" + ); + } + } + out +} + // --------------------------------------------------------------------------- // Properties // --------------------------------------------------------------------------- @@ -312,6 +424,63 @@ proptest! { prop_assert_eq!(extract_lspci_name(&line), expected); } + /// A name the install-family detector resolves to an APU target must not be + /// reported by `examine` as "not an APU". A lookup miss is not evidence of + /// a discrete GPU. + #[test] + fn examine_never_calls_a_known_apu_discrete((name, truth) in apu_marketing_name()) { + let (_target, is_apu) = classify_amd_marketing_name(&name); + prop_assert!( + is_apu, + "{name} is an APU ({}) but classify_amd_marketing_name says is_apu=false", + truth.1, + ); + } + + /// Folding a `rocminfo` reading into the report must never *downgrade* an + /// APU verdict the PCI scan already reached. More evidence must not produce + /// a worse answer. + #[test] + fn rocminfo_never_downgrades_an_apu_verdict( + device in proptest::sample::select(apu_lspci_devices()), + addr in pci_address(), + marketing in proptest::sample::select(&["", "AMD Radeon Graphics"][..]), + ) { + // A coherent host: the gfx target is the one this very device reports. + let name = format!("Advanced Micro Devices, Inc. [AMD/ATI] {}", device.0); + let (gfx_guess, is_apu_guess) = classify_amd_marketing_name(&name); + prop_assert!( + is_apu_guess, + "the PCI scan must already know {} is an APU before this property \ + can say anything about preserving that verdict", + device.0, + ); + + let mut e = Examination { + gpus: vec![Gpu { + name, + gfx_target: gfx_guess, + pci_id: addr, + is_apu: Some(true), + is_amd: true, + }], + ..Examination::default() + }; + apply_rocminfo_gpu_agents(&mut e, &rocminfo_output(&[(device.2, marketing)])); + summarise_gpu_categories(&mut e); + prop_assert!( + e.has_apu, + "lspci classified {} as an APU, then rocminfo reporting {} flipped has_apu to false", + device.0, + device.2, + ); + prop_assert!( + !e.has_discrete_amd, + "an APU-only host must not report has_discrete_amd (gfx {})", + device.2, + ); + } + /// Classification must not depend on spelling: case, inner/outer /// whitespace and vendor decorations name the same device. #[test] @@ -327,6 +496,27 @@ proptest! { ); } + /// `gfx_is_apu_family` must agree with ground truth for every target a real + /// host reports, and must be suffix-insensitive. + #[test] + fn gfx_apu_family_matches_ground_truth( + gfx in proptest::sample::select(GFX_TARGETS), + suffix in proptest::sample::select(&["", ":xnack-", ":sramecc+:xnack-", ":sramecc-"][..]), + upper in any::(), + ) { + let spelled = if upper { + format!("{}{suffix}", gfx.0.to_uppercase()) + } else { + format!("{}{suffix}", gfx.0) + }; + prop_assert_eq!( + gfx_is_apu_family(&spelled), + gfx.1, + "gfx_is_apu_family({}) disagrees with ground truth", + spelled, + ); + } + /// The Windows display probe and `examine`'s own Windows GPU enumeration /// read the same row and must not disagree about the gfx target. #[test] @@ -357,8 +547,48 @@ proptest! { } } + /// `probe_gpus_windows` has the PNP device id in hand on every row, and the + /// crate already decodes it. When that decode names an APU target, the + /// report must not come back `is_apu=false` just because the marketing + /// name was not in `examine`'s own smaller table. + #[test] + fn the_windows_row_is_not_called_discrete_when_its_pnp_id_names_an_apu( + entry in proptest::sample::select(AMD_MARKETING_NAMES), + subsys in 0u32..0x1_0000, + ) { + let Some(device_id) = entry_device_id(entry.0) else { + return Ok(()); + }; + let pnp = format!("PCI\\VEN_1002&DEV_{device_id}&SUBSYS_{subsys:04x}1002&REV_C1"); + let row = format!("{}\t32.0.1\t{pnp}", entry.0); + let Some(install_target) = crate::parse_windows_display_gfx_target(&row) else { + return Ok(()); + }; + prop_assume!(gfx_is_apu_family(&install_target)); + let (_target, is_apu) = classify_amd_marketing_name(entry.0); + prop_assert!( + is_apu, + "{} has PNP id {} which this crate decodes to the APU target {}, \ + yet examine reports is_apu=false", + entry.0, + pnp, + install_target, + ); + } + } +/// Corpus entries whose `lspci` text names no model at all. +/// +/// `lspci` prints the `pci.ids` device string, and when that database has no +/// entry for an id it prints the bare word `Device`. A name-keyed lookup can +/// never classify such a row: the identifying information is in the +/// `[1002:xxxx]` id on the same line, which the PCI scan currently discards. +/// They are excluded from the name-classification sweeps below rather than +/// dropped from the corpus, because the parsing and totality properties still +/// have to survive them. +const LSPCI_DEVICES_WITHOUT_A_PCI_IDS_NAME: &[&str] = &["Device"]; + /// The PCI device id for a marketing name, when the corpus pins one. fn entry_device_id(name: &str) -> Option<&'static str> { match name { @@ -374,6 +604,355 @@ fn entry_device_id(name: &str) -> Option<&'static str> { } } +/// Enumerate every corpus entry the shrinker would otherwise collapse to one +/// minimal case, so the full extent of a divergence is visible in the failure +/// rather than just its first example. +#[test] +fn every_known_apu_is_classified_as_an_apu() { + let mut misses: Vec = Vec::new(); + for (name, target, is_apu) in AMD_MARKETING_NAMES { + if !is_apu { + continue; + } + let (examine_target, examine_is_apu) = classify_amd_marketing_name(name); + if !examine_is_apu { + misses.push(format!( + " marketing name {name:?} ({target}): examine says is_apu=false, \ + gfx_target={examine_target:?}" + )); + } + } + for (gfx, is_apu) in GFX_TARGETS { + if *is_apu && !gfx_is_apu_family(gfx) { + misses.push(format!(" gfx target {gfx}: gfx_is_apu_family says false")); + } + } + for (device, id, _gfx, is_apu) in AMD_LSPCI_DEVICES { + if !is_apu || LSPCI_DEVICES_WITHOUT_A_PCI_IDS_NAME.contains(device) { + continue; + } + let name = format!("Advanced Micro Devices, Inc. [AMD/ATI] {device}"); + if !classify_amd_marketing_name(&name).1 { + misses.push(format!( + " lspci device {device:?} [1002:{id}]: examine says is_apu=false" + )); + } + } + assert!( + misses.is_empty(), + "parts that are APUs but are not classified as APUs:\n{}", + misses.join("\n") + ); +} + +/// A reported `gfx_target` must never *contradict* the part: an unresolved +/// lookup is recoverable, a wrong answer is not. +#[test] +fn no_part_is_labelled_with_the_wrong_gfx_target() { + let mut wrong: Vec = Vec::new(); + for (device, id, gfx, _is_apu) in AMD_LSPCI_DEVICES { + let name = format!("Advanced Micro Devices, Inc. [AMD/ATI] {device}"); + let guess = classify_amd_marketing_name(&name).0; + if !guess.is_empty() && guess != *gfx { + wrong.push(format!( + " lspci {device:?} [1002:{id}] is {gfx}, but examine reports {guess}" + )); + } + } + for (name, gfx, _is_apu) in AMD_MARKETING_NAMES { + let guess = classify_amd_marketing_name(name).0; + if !guess.is_empty() && guess != *gfx { + wrong.push(format!( + " marketing name {name:?} is {gfx}, but examine reports {guess}" + )); + } + } + assert!( + wrong.is_empty(), + "parts labelled with a gfx target that is not theirs:\n{}", + wrong.join("\n") + ); +} + +/// The APU verdict must survive a `rocminfo` fold whether or not +/// ROCm/rocm-cli#393 has been fixed. +/// +/// #393 is that `rocminfo` prints indented ISA `Name:` sub-entries after each +/// agent, the parser's running `cur_name` ends up holding +/// `amdgcn-amd-amdhsa--gfx11-generic`, the agent is dropped and the whole fold +/// returns early. That made the `is_apu` overwrite unreachable on a real host, +/// which is the only reason this misclassification was latent rather than +/// live — and fixing #393 is exactly what would have made it live. +/// +/// Both shapes are driven here so the verdict cannot depend on which side of +/// #393 the parser is on. The fix for #393 belongs to #393; this only has to +/// hold under either parser. +#[test] +fn the_apu_verdict_holds_on_both_sides_of_the_rocminfo_isa_name_defect() { + let build = |with_isa: bool| { + let name = "Advanced Micro Devices, Inc. [AMD/ATI] Raphael".to_owned(); + let (gfx_target, is_apu) = classify_amd_marketing_name(&name); + assert_eq!(gfx_target, "gfx1036", "the PCI scan alone names the part"); + assert!(is_apu, "the PCI scan alone knows Raphael is an APU"); + let mut e = Examination { + gpus: vec![Gpu { + name, + gfx_target, + pci_id: "0000:14:00.0".to_owned(), + is_apu: Some(is_apu), + is_amd: true, + }], + ..Examination::default() + }; + apply_rocminfo_gpu_agents( + &mut e, + &rocminfo_output_shaped(&[("gfx1036", "AMD Radeon Graphics")], with_isa), + ); + summarise_gpu_categories(&mut e); + e + }; + + for (with_isa, parser) in [ + (true, "with the agent-dropping parser of #393"), + (false, "with a parser that reads the agent"), + ] { + let e = build(with_isa); + assert_eq!(e.gpus[0].gfx_target, "gfx1036", "{parser}: {:?}", e.gpus); + assert!( + e.has_apu, + "{parser}: the Raphael iGPU must still be an APU: {:?}", + e.gpus + ); + assert!( + !e.has_discrete_amd, + "{parser}: an iGPU-only host must not report a discrete AMD GPU: {:?}", + e.gpus + ); + } +} + +/// Every gfx target the crate's two lookup tables can hand back must have a +/// packaging entry. +/// +/// `GFX_TARGET_PACKAGING` is keyed by target while the marketing-name and PCI +/// device-id tables are keyed by name and id, so nothing in the type system +/// ties them together. An entry added to either of those for a part this one +/// has never heard of would answer `is_apu = false` — "discrete" — for an APU, +/// which is the original defect in a new place. +#[test] +fn every_target_the_lookup_tables_produce_has_a_packaging() { + let classified = |target: &str| { + GFX_TARGET_PACKAGING + .iter() + .any(|(known, _)| *known == target) + }; + let mut unclassified: Vec = Vec::new(); + for entry in crate::AMD_MARKETING_GFX_TARGETS { + if !classified(entry.gfx_target) { + unclassified.push(format!( + " marketing pattern {:?} -> {} has no packaging entry", + entry.pattern, entry.gfx_target + )); + } + } + // The device-id lookup is a `match`, not an iterable table, so sweep its + // whole input domain: every 4-hex-digit PCI device id. + for id in 0..=0xffff_u32 { + let id = format!("{id:04x}"); + if let Some(target) = crate::gfx_target_from_amd_pci_device_id(&id) + && !classified(target) + { + unclassified.push(format!( + " PCI device id {id} -> {target} has no packaging entry" + )); + } + } + unclassified.sort(); + unclassified.dedup(); + assert!( + unclassified.is_empty(), + "targets a lookup table produces but GFX_TARGET_PACKAGING does not \ + classify:\n{}", + unclassified.join("\n") + ); +} + +/// What the downgrade costs a user, run through the real diagnosis catalog. +/// +/// A Ryzen 7000 desktop pairing the Raphael iGPU with an RX 7900 XTX is the +/// textbook iGPU+dGPU collision host: `check_9_igpu_dgpu_collision` exists for +/// it and fires only when `has_apu && has_discrete_amd`. Once `rocminfo` marks +/// the iGPU not-an-APU, `has_apu` is false and the check scores zero, so a user +/// whose workload segfaults on the wrong device is told nothing. +#[test] +fn the_igpu_dgpu_collision_check_still_fires_on_a_raphael_plus_rx7900_host() { + let igpu = "0000:14:00.0 VGA compatible controller [0300]: Advanced Micro Devices, Inc. \ + [AMD/ATI] Raphael [1002:164e] (rev c1)"; + let dgpu = "0000:03:00.0 VGA compatible controller [0300]: Advanced Micro Devices, Inc. \ + [AMD/ATI] Navi 31 [Radeon RX 7900 XT/7900 XTX/7900 GRE/7900M] [1002:744c]"; + let mut gpus = Vec::new(); + for (line, pci) in [(dgpu, "0000:03:00.0"), (igpu, "0000:14:00.0")] { + let name = extract_lspci_name(line); + let (gfx_target, is_apu) = classify_amd_marketing_name(&name); + gpus.push(Gpu { + name, + gfx_target, + pci_id: pci.to_owned(), + is_apu: Some(is_apu), + is_amd: true, + }); + } + let mut e = Examination { + os_family: "linux".to_owned(), + gpus, + ..Examination::default() + }; + // rocminfo in KFD node order, matching the PCI order the scan produced. + apply_rocminfo_gpu_agents( + &mut e, + &rocminfo_output(&[ + ("gfx1100", "AMD Radeon RX 7900 XTX"), + ("gfx1036", "AMD Radeon Graphics"), + ]), + ); + summarise_gpu_categories(&mut e); + + let report = crate::diagnose::diagnose(&e, "my training run segfaults"); + let collision = report.matched.iter().find(|d| d.id == "fix-9-igpu-dgpu"); + let Some(collision) = collision.filter(|d| d.score > 0) else { + panic!( + "an iGPU+dGPU host must raise fix-9-igpu-dgpu; has_apu={}, \ + has_discrete_amd={}, gpus={:?}", + e.has_apu, e.has_discrete_amd, e.gpus, + ); + }; + // And it must name the right card as the one to pin. The whole reason this + // check exists is that the user cannot tell which ordinal is the dGPU, so a + // diagnosis that fires but confuses the two is no better than silence. + let notes = collision + .fix + .as_ref() + .map(|fix| fix.notes.join(" ")) + .unwrap_or_default(); + assert!( + notes.contains("[\"gfx1100\"]") && notes.contains("[\"gfx1036\"]"), + "the collision note must name gfx1100 as the discrete GPU and gfx1036 \ + as the APU, got: {notes}" + ); +} + +/// The complete list of parts whose APU verdict a `rocminfo` reading downgrades. +#[test] +fn no_known_apu_has_its_verdict_downgraded_by_rocminfo() { + let mut downgraded: Vec = Vec::new(); + for (device, id, gfx, is_apu) in AMD_LSPCI_DEVICES { + if !is_apu { + continue; + } + let name = format!("Advanced Micro Devices, Inc. [AMD/ATI] {device}"); + let (gfx_guess, is_apu_guess) = classify_amd_marketing_name(&name); + if !is_apu_guess { + continue; + } + let mut e = Examination { + gpus: vec![Gpu { + name: name.clone(), + gfx_target: gfx_guess, + pci_id: "0000:04:00.0".to_owned(), + is_apu: Some(true), + is_amd: true, + }], + ..Examination::default() + }; + apply_rocminfo_gpu_agents(&mut e, &rocminfo_output(&[(gfx, "AMD Radeon Graphics")])); + summarise_gpu_categories(&mut e); + if !e.has_apu { + downgraded.push(format!( + " lspci {device:?} [1002:{id}] + rocminfo {gfx} -> has_apu=false, \ + has_discrete_amd={}", + e.has_discrete_amd + )); + } + } + assert!( + downgraded.is_empty(), + "rocminfo downgraded an APU verdict the PCI scan already reached:\n{}", + downgraded.join("\n") + ); +} + +/// The downgrade driven through the *whole* post-PCI sequence, against a +/// planted KFD topology, rather than through `apply_rocminfo_gpu_agents` alone. +/// +/// A Ryzen 6800H laptop: `lspci` names the iGPU "Rembrandt [Radeon 680M]", the +/// kernel exposes one KFD node for it at `0000:04:00.0` reporting +/// `gfx_target_version 100305` (gfx1035), and `rocminfo` agrees. Every source +/// describes an APU; the report must not call it a discrete GPU. +#[test] +fn a_rembrandt_laptop_is_not_reported_as_a_discrete_gpu() { + let root = std::env::temp_dir().join(format!( + "rocm-core-proptest-kfd-{}-{}", + std::process::id(), + crate::unix_time_millis() + )); + let nodes = root.join("nodes"); + std::fs::create_dir_all(nodes.join("0")).expect("plant the CPU node"); + std::fs::write( + nodes.join("0").join("properties"), + "cpu_cores_count 16\ngfx_target_version 0\nlocation_id 0\ndomain 0\n", + ) + .expect("plant the CPU node properties"); + std::fs::create_dir_all(nodes.join("1")).expect("plant the GPU node"); + std::fs::write( + nodes.join("1").join("properties"), + "simd_count 12\ngfx_target_version 100305\nlocation_id 1024\ndomain 0\n", + ) + .expect("plant the GPU node properties"); + + let line = "0000:04:00.0 VGA compatible controller [0300]: Advanced Micro Devices, Inc. \ + [AMD/ATI] Rembrandt [Radeon 680M] [1002:1681] (rev c8)"; + let name = extract_lspci_name(line); + let (gfx_guess, is_apu_guess) = classify_amd_marketing_name(&name); + assert!( + is_apu_guess, + "the PCI scan alone already knows Rembrandt is an APU" + ); + + let mut e = Examination { + gpus: vec![Gpu { + name, + gfx_target: gfx_guess, + pci_id: "0000:04:00.0".to_owned(), + is_apu: Some(is_apu_guess), + is_amd: true, + }], + ..Examination::default() + }; + super::probe_gpus_after_lspci( + &mut e, + super::GpuProbeSources { + kfd_nodes: &nodes, + rocminfo: Some(&rocminfo_output(&[("gfx1035", "AMD Radeon Graphics")])), + sysfs_gfx_target: || None, + }, + ); + summarise_gpu_categories(&mut e); + std::fs::remove_dir_all(&root).ok(); + + assert_eq!(e.gpus.len(), 1, "one GPU: {:?}", e.gpus); + assert_eq!(e.gpus[0].gfx_target, "gfx1035"); + assert!( + e.has_apu, + "a Radeon 680M iGPU must report has_apu, got {:?}", + e.gpus + ); + assert!( + !e.has_discrete_amd, + "an iGPU-only laptop must not report has_discrete_amd, got {:?}", + e.gpus + ); +} + /// Measure how far the generators actually reach, by drawing from them /// directly and tallying which branches each draw lands in. /// diff --git a/crates/rocm-core/src/lib.rs b/crates/rocm-core/src/lib.rs index c9d096f50..5aa5adba6 100644 --- a/crates/rocm-core/src/lib.rs +++ b/crates/rocm-core/src/lib.rs @@ -4381,6 +4381,13 @@ pub fn normalize_therock_family(value: &str) -> Option { value if value.starts_with("gfx1153") => Some("gfx1153".to_owned()), "gfx1200" | "gfx1201" => Some("gfx120X-all".to_owned()), value if value.starts_with("gfx125") => Some("gfx125X-dcgpu".to_owned()), + // Renoir / Cezanne / Lucienne / Barcelo. Vega-architecture *integrated* + // graphics, so the `gfx90` catch-all below would file it under + // `gfx90X-dcgpu` — a datacenter family, which also makes vLLM the + // preferred serving engine. No published family covers gfx90c, and + // "none" sends the user to pick one instead of silently installing a + // runtime built for other silicon. + value if value.starts_with("gfx90c") => None, value if value.starts_with("gfx900") => Some("gfx900".to_owned()), value if value.starts_with("gfx906") => Some("gfx906".to_owned()), value if value.starts_with("gfx908") => Some("gfx908".to_owned()), @@ -5066,6 +5073,14 @@ const AMD_MARKETING_GFX_TARGETS: &[AmdMarketingGfxTarget] = &[ pattern: "8040s", gfx_target: "gfx1151", }, + AmdMarketingGfxTarget { + pattern: "strix halo", + gfx_target: "gfx1151", + }, + AmdMarketingGfxTarget { + pattern: "ryzen ai max", + gfx_target: "gfx1151", + }, // RDNA3.5 APUs. AmdMarketingGfxTarget { pattern: "890m", @@ -5075,6 +5090,10 @@ const AMD_MARKETING_GFX_TARGETS: &[AmdMarketingGfxTarget] = &[ pattern: "880m", gfx_target: "gfx1150", }, + AmdMarketingGfxTarget { + pattern: "strix point", + gfx_target: "gfx1150", + }, AmdMarketingGfxTarget { pattern: "860m", gfx_target: "gfx1152", @@ -5083,11 +5102,20 @@ const AMD_MARKETING_GFX_TARGETS: &[AmdMarketingGfxTarget] = &[ pattern: "840m", gfx_target: "gfx1152", }, + // Krackan Point. `gfx_target_from_amd_pci_device_id("1114")` and the + // 860M/840M SKUs it sells as both say gfx1152, and gfx1150 and gfx1152 are + // separate TheRock build families, so naming it gfx1150 picked the wrong + // runtime build. + AmdMarketingGfxTarget { + pattern: "krackan", + gfx_target: "gfx1152", + }, AmdMarketingGfxTarget { pattern: "820m", gfx_target: "gfx1153", }, - // RDNA3 APUs. + // RDNA3 APUs. `lspci` prints the codename with a trailing die number + // (`Phoenix1`, `Phoenix3`), which is a different token from `phoenix`. AmdMarketingGfxTarget { pattern: "780m", gfx_target: "gfx1103", @@ -5100,6 +5128,26 @@ const AMD_MARKETING_GFX_TARGETS: &[AmdMarketingGfxTarget] = &[ pattern: "740m", gfx_target: "gfx1103", }, + AmdMarketingGfxTarget { + pattern: "phoenix", + gfx_target: "gfx1103", + }, + AmdMarketingGfxTarget { + pattern: "phoenix1", + gfx_target: "gfx1103", + }, + AmdMarketingGfxTarget { + pattern: "phoenix2", + gfx_target: "gfx1103", + }, + AmdMarketingGfxTarget { + pattern: "phoenix3", + gfx_target: "gfx1103", + }, + AmdMarketingGfxTarget { + pattern: "hawk point", + gfx_target: "gfx1103", + }, // RDNA2 APUs. AmdMarketingGfxTarget { pattern: "680m", @@ -5109,10 +5157,18 @@ const AMD_MARKETING_GFX_TARGETS: &[AmdMarketingGfxTarget] = &[ pattern: "660m", gfx_target: "gfx1035", }, + AmdMarketingGfxTarget { + pattern: "rembrandt", + gfx_target: "gfx1035", + }, AmdMarketingGfxTarget { pattern: "610m", gfx_target: "gfx1036", }, + AmdMarketingGfxTarget { + pattern: "raphael", + gfx_target: "gfx1036", + }, AmdMarketingGfxTarget { pattern: "steam deck", gfx_target: "gfx1033", @@ -5121,6 +5177,34 @@ const AMD_MARKETING_GFX_TARGETS: &[AmdMarketingGfxTarget] = &[ pattern: "van gogh", gfx_target: "gfx1033", }, + // `lspci` spells it as one word, and the Steam Deck's own display name is + // the part number rather than any Radeon model. + AmdMarketingGfxTarget { + pattern: "vangogh", + gfx_target: "gfx1033", + }, + AmdMarketingGfxTarget { + pattern: "custom gpu 0405", + gfx_target: "gfx1033", + }, + // Vega-based Ryzen 4000/5000 iGPUs. These sell as a bare "AMD Radeon + // Graphics", so the codename `lspci` reports is the only usable signal. + AmdMarketingGfxTarget { + pattern: "renoir", + gfx_target: "gfx90c", + }, + AmdMarketingGfxTarget { + pattern: "cezanne", + gfx_target: "gfx90c", + }, + AmdMarketingGfxTarget { + pattern: "lucienne", + gfx_target: "gfx90c", + }, + AmdMarketingGfxTarget { + pattern: "barcelo", + gfx_target: "gfx90c", + }, ]; fn normalize_marketing_name_for_match(value: &str) -> String { @@ -9965,6 +10049,28 @@ mod tests { ); } + /// A Renoir-class iGPU has no published family, and must not be filed under + /// a datacenter one. + /// + /// gfx90c is Vega-architecture *integrated* graphics, so the `gfx90` + /// catch-all would answer `gfx90X-dcgpu` — which is not just a wrong label: + /// `preferred_serve_engine_for_therock_family` reads `-dcgpu` and picks + /// vLLM. Its neighbours are genuinely discrete and must keep their + /// families. + #[test] + fn normalize_therock_family_refuses_to_file_an_igpu_under_a_datacenter_family() { + assert_eq!(normalize_therock_family("gfx90c"), None); + assert_eq!(normalize_therock_family("gfx90c:xnack-"), None); + assert_eq!( + normalize_therock_family("gfx90a"), + Some("gfx90a".to_owned()) + ); + assert_eq!( + normalize_therock_family("gfx900"), + Some("gfx900".to_owned()) + ); + } + #[test] fn normalize_therock_family_maps_gfx1201_to_gfx120x_all() { assert_eq!( From 086fa68f8b64194b0b285feaae654ec39727d42c Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Fri, 2 Oct 2026 12:35:45 +0000 Subject: [PATCH 3/5] fix(examine): stop a rocminfo reading downgrading an APU verdict MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Folding a `rocminfo` agent into the report overwrote `is_apu` with the target-derived answer unconditionally, and that answer is `false` both for a discrete GPU and for a target this crate has never heard of. So a GPU the PCI scan had already identified as an APU became a discrete one the moment `rocminfo` named a target outside the packaging table: more evidence produced a worse answer, clearing `has_apu` and with it the iGPU+dGPU collision diagnosis. A target whose packaging is known still wins. The target names the silicon, where the PCI marketing string is a heuristic over vendor-chosen text, so when the two genuinely disagree the target is the better witness. An unknown target is not a disagreement, though — it is an absence of evidence, and it now leaves the earlier verdict standing. The branch that creates a GPU for an agent with no PCI row to pair with — the ordinary ROCm container, which ships `rocminfo` but not `pciutils` — followed the same rule only halfway: it had no earlier verdict to preserve, but it also had the agent's own marketing name, and it discarded it to write `Some(false)`. `summarise_gpu_categories` reads that as "this is a discrete GPU", so an iGPU-only laptop reported a discrete AMD GPU. It now takes the target when the packaging is known, the marketing name when that resolves a part, and leaves `is_apu` unset when neither answers — the same conclusion `probe_gpus_sysfs_fallback` reaches, for the same stated reason. The `hipInfo` fold on Windows had the same shape and follows the same rule. `folding_in_rocminfo_only_ever_adds_knowledge` and `an_agent_with_no_pci_row_is_classified_from_what_evidence_there_is` are the two tests that discriminate this change; the other rocminfo tests in that module pass with it reverted, because they exercise targets the packaging table knows. This was latent rather than live: `rocminfo` prints indented ISA `Name:` sub-entries that the agent parser lets overwrite the agent's own name, so the fold returns early on a real host (ROCm/rocm-cli#393) and never reached the overwrite. Fixing that parser is what would have made this live, so the test drives both parser shapes and holds the verdict under either. Signed-off-by: Roman Inflianskas --- crates/rocm-core/src/examine.rs | 39 ++++++- crates/rocm-core/src/examine_proptests.rs | 123 ++++++++++++++++++++++ 2 files changed, 159 insertions(+), 3 deletions(-) diff --git a/crates/rocm-core/src/examine.rs b/crates/rocm-core/src/examine.rs index 01b60b126..ad037b285 100644 --- a/crates/rocm-core/src/examine.rs +++ b/crates/rocm-core/src/examine.rs @@ -1327,9 +1327,37 @@ fn apply_rocminfo_gpu_agents(e: &mut Examination, out: &str) { if !marketing.is_empty() && gpu_name_is_unknown(&gpu.name) { gpu.name = marketing; } - gpu.is_apu = Some(gfx_is_apu_family(&gfx)); + // The target names the silicon, so where this crate knows how that + // target is packaged it outranks the PCI marketing string, which is + // a guess over vendor-chosen text. A target it does *not* know is + // not evidence of a discrete GPU, so it leaves the PCI scan's + // verdict standing rather than replacing it with a default `false`: + // reading `rocminfo` must not make the answer worse than not + // reading it. Before this, folding in `rocminfo` turned every + // APU whose target was unlisted — which was every pre-RDNA3 APU — + // into a discrete GPU, clearing `has_apu` and silently suppressing + // the iGPU+dGPU collision diagnosis on the one host shape it is + // written for. + if let Some(packaging) = gfx_target_packaging(&gfx) { + gpu.is_apu = Some(packaging == GfxPackaging::Integrated); + } } else { - let is_apu = Some(gfx_is_apu_family(&gfx)); + // An agent with no PCI row to pair with: the ordinary ROCm + // container, where `rocminfo` is present and `pciutils` is not. + // Same precedence as above, applied to the evidence that exists + // here — the target when its packaging is known, else the agent's + // own marketing name when that resolves a part. When neither + // answers, `is_apu` stays unset rather than defaulting to `false`: + // `summarise_gpu_categories` reads `Some(false)` as "this is a + // discrete GPU", so guessing it would report a discrete card on an + // iGPU-only laptop. That is the same reasoning, and the same + // answer, as [`probe_gpus_sysfs_fallback`]. + let is_apu = gfx_target_packaging(&gfx) + .map(|packaging| packaging == GfxPackaging::Integrated) + .or_else(|| { + let (named_target, named_is_apu) = classify_amd_marketing_name(&marketing); + (!named_target.is_empty()).then_some(named_is_apu) + }); e.gpus.push(Gpu { name: if marketing.is_empty() { "AMD GPU".to_owned() @@ -2438,7 +2466,12 @@ fn probe_hip_sdk_windows(e: &mut Examination) { .find(|g| g.is_amd && g.gfx_target.is_empty()) { gpu.gfx_target = gfx; - gpu.is_apu = Some(gfx_is_apu_family(&gpu.gfx_target)); + // Same rule as `apply_rocminfo_gpu_agents`: a target whose + // packaging this crate knows outranks the display name, an + // unknown one leaves that verdict alone. + if let Some(packaging) = gfx_target_packaging(&gpu.gfx_target) { + gpu.is_apu = Some(packaging == GfxPackaging::Integrated); + } } } } else { diff --git a/crates/rocm-core/src/examine_proptests.rs b/crates/rocm-core/src/examine_proptests.rs index 4d62c4b37..195754719 100644 --- a/crates/rocm-core/src/examine_proptests.rs +++ b/crates/rocm-core/src/examine_proptests.rs @@ -731,6 +731,129 @@ fn the_apu_verdict_holds_on_both_sides_of_the_rocminfo_isa_name_defect() { } } +/// Reading `rocminfo` may only ever improve the report, never worsen it. +/// +/// The two parser shapes of +/// [`the_apu_verdict_holds_on_both_sides_of_the_rocminfo_isa_name_defect`] agree +/// about Raphael because the PCI name already resolves it, so that test alone +/// cannot show the fold doing anything. These two hosts separate the cases: +/// +/// - a Strix Halo whose `lspci` entry has no `pci.ids` name, so the PCI scan +/// contributes nothing and only the agent can supply the target: the fold +/// must *add* the APU verdict once the agent is readable; +/// - an iGPU whose agent reports a target this crate has never heard of: the +/// fold knows the target but not its packaging, which is not evidence of a +/// discrete GPU, so the PCI scan's verdict has to survive untouched. +#[test] +fn folding_in_rocminfo_only_ever_adds_knowledge() { + let host = |lspci_name: &str, agent_target: &str, with_isa: bool| { + let name = format!("Advanced Micro Devices, Inc. [AMD/ATI] {lspci_name}"); + let (gfx_target, is_apu) = classify_amd_marketing_name(&name); + let mut e = Examination { + gpus: vec![Gpu { + name, + gfx_target, + pci_id: "0000:66:00.0".to_owned(), + is_apu: Some(is_apu), + is_amd: true, + }], + ..Examination::default() + }; + apply_rocminfo_gpu_agents( + &mut e, + &rocminfo_output_shaped(&[(agent_target, "AMD Radeon Graphics")], with_isa), + ); + summarise_gpu_categories(&mut e); + e + }; + + // A nameless Strix Halo: nothing is known before the fold. + let before = host("Device", "gfx1151", true); + assert_eq!(before.gpus[0].gfx_target, "", "{:?}", before.gpus); + assert!(!before.has_apu, "{:?}", before.gpus); + // Once the agent is readable the fold supplies both the target and the + // packaging that follows from it. + let after = host("Device", "gfx1151", false); + assert_eq!(after.gpus[0].gfx_target, "gfx1151", "{:?}", after.gpus); + assert!( + after.has_apu, + "a readable gfx1151 agent must establish the APU verdict the PCI scan \ + could not reach: {:?}", + after.gpus + ); + + // A Rembrandt laptop whose agent names a target this crate has no packaging + // entry for. The target is still worth recording; the verdict is not the + // fold's to revise. + let unknown = host("Rembrandt [Radeon 680M]", "gfx1154", false); + assert_eq!(unknown.gpus[0].gfx_target, "gfx1154", "{:?}", unknown.gpus); + assert!( + unknown.has_apu, + "an unrecognised target is not evidence of a discrete GPU, so the PCI \ + scan's APU verdict must survive: {:?}", + unknown.gpus + ); + assert!( + !unknown.has_discrete_amd, + "and it must certainly not be promoted to a discrete GPU: {:?}", + unknown.gpus + ); +} + +/// The same rule where there is no PCI row at all: an ordinary ROCm container, +/// which ships `rocminfo` but not `pciutils`. +/// +/// Every agent then lands in the branch that *creates* a GPU entry, so there is +/// no earlier verdict to preserve — but there is still evidence, and still a +/// difference between "not an APU" and "cannot say". Defaulting the latter to +/// `Some(false)` reports a discrete AMD GPU on a laptop that has none, which is +/// what gates the iGPU+dGPU diagnosis on, and it throws away the agent's own +/// marketing name while doing it. [`super::probe_gpus_sysfs_fallback`] reaches +/// the same conclusion for the same reason and leaves `is_apu` unset. +#[test] +fn an_agent_with_no_pci_row_is_classified_from_what_evidence_there_is() { + let container = |agent_target: &str, marketing: &str| { + let mut e = Examination::default(); + apply_rocminfo_gpu_agents(&mut e, &rocminfo_output(&[(agent_target, marketing)])); + summarise_gpu_categories(&mut e); + e + }; + + // The target is enough on its own. + let known = container("gfx1036", "AMD Radeon Graphics"); + assert_eq!(known.gpus[0].is_apu, Some(true), "{:?}", known.gpus); + assert!(!known.has_discrete_amd, "{:?}", known.gpus); + + // An unlisted target, but the agent names the part: the name answers. + let named = container("gfx1154", "AMD Radeon(TM) 780M Graphics"); + assert_eq!(named.gpus[0].is_apu, Some(true), "{:?}", named.gpus); + assert!( + !named.has_discrete_amd, + "an iGPU-only laptop must not report a discrete AMD GPU just because \ + its target is new: {:?}", + named.gpus + ); + + // Neither answers. Saying "discrete" here would be inventing a fact. + let silent = container("gfx1154", ""); + assert_eq!(silent.gpus[0].is_apu, None, "{:?}", silent.gpus); + assert!( + !silent.has_apu && !silent.has_discrete_amd, + "{:?}", + silent.gpus + ); + assert!( + silent.has_amd_gpu, + "the GPU is still present and still AMD: {:?}", + silent.gpus + ); + + // A discrete card still reports as one. + let discrete = container("gfx1100", "AMD Radeon RX 7900 XTX"); + assert_eq!(discrete.gpus[0].is_apu, Some(false), "{:?}", discrete.gpus); + assert!(discrete.has_discrete_amd, "{:?}", discrete.gpus); +} + /// Every gfx target the crate's two lookup tables can hand back must have a /// packaging entry. /// From a019fc8d9772a07a2f69732dc9c1983f07641a02 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Fri, 2 Oct 2026 14:35:25 +0000 Subject: [PATCH 4/5] fix(therock): give gfx90c a family of its own, not none MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit series stopped `normalize_therock_family` filing a Renoir-class iGPU under `gfx90X-dcgpu`, a datacenter family that also makes vLLM the preferred serving engine, by answering `None`. That overcorrected into a dead end. With no family, `resolve_family` tells the user to re-run with an explicit `--family`; but `AggregateDeviceTarget::resolve` then requires the detected target to normalize to the family named, and `None` matches nothing, so every value the user can pass is refused as "belongs to no recognized package family". The CLI's own remediation advice could not be followed. gfx90c now normalizes to `gfx90c`, the single-target shape gfx900, gfx906, gfx908 and gfx90a already use, and is listed in `known_therock_families`. It is not `-dcgpu`, so no serving engine is preferred for it. Assumed rather than verified: that TheRock's index names this family `gfx90c`. Nothing in this repository records a gfx90c family under any name, and the index could not be queried from here. The assumption fails safely: `known_therock_families` is documented as recognition, not availability, and where a channel publishes no `rocm-sdk-device-gfx90c` payload the install refuses by naming the payload it looked for and points at the other channel — the same outcome as for any other recognised family with no published payload. Signed-off-by: Roman Inflianskas --- apps/rocm/src/therock.rs | 29 ++++++++++++++++++++++++++++ crates/rocm-core/src/lib.rs | 38 +++++++++++++++++++++++++------------ 2 files changed, 55 insertions(+), 12 deletions(-) diff --git a/apps/rocm/src/therock.rs b/apps/rocm/src/therock.rs index d8c236eae..aa9865f9b 100644 --- a/apps/rocm/src/therock.rs +++ b/apps/rocm/src/therock.rs @@ -7632,6 +7632,35 @@ mod tests { ); } + /// A Renoir-class iGPU can be installed for when its payload is published. + /// + /// The family comes from `normalize_therock_family` rather than being + /// written out, because that is what `resolve_family` hands this check: + /// a family that never matches the detected target turns every `--family` + /// the user tries into "belongs to no recognized package family". + #[test] + fn a_published_renoir_class_target_resolves_to_its_own_payload() { + let family = normalize_therock_family("gfx90c") + .expect("gfx90c must belong to some family, or no --family can install for it"); + let mut published = published_device_targets(); + published.push("gfx90c".to_owned()); + + assert_eq!( + AggregateDeviceTarget::resolve(Some("gfx90c"), &family, &published), + AggregateDeviceTarget::Exact("gfx90c".to_owned()) + ); + // And where the channel publishes none, the refusal names the payload + // it looked for instead of blaming the family. + let unpublished = + AggregateDeviceTarget::resolve(Some("gfx90c"), &family, &published_device_targets()); + assert!( + unpublished + .reason() + .is_some_and(|reason| reason.contains("no `device-gfx90c` payload")), + "{unpublished:?}" + ); + } + #[test] fn a_detected_target_from_another_family_is_undetermined() { let target = AggregateDeviceTarget::resolve( diff --git a/crates/rocm-core/src/lib.rs b/crates/rocm-core/src/lib.rs index 5aa5adba6..bcbefb94e 100644 --- a/crates/rocm-core/src/lib.rs +++ b/crates/rocm-core/src/lib.rs @@ -4381,13 +4381,14 @@ pub fn normalize_therock_family(value: &str) -> Option { value if value.starts_with("gfx1153") => Some("gfx1153".to_owned()), "gfx1200" | "gfx1201" => Some("gfx120X-all".to_owned()), value if value.starts_with("gfx125") => Some("gfx125X-dcgpu".to_owned()), - // Renoir / Cezanne / Lucienne / Barcelo. Vega-architecture *integrated* + // Renoir / Cezanne / Lucienne / Barcelo: Vega-architecture *integrated* // graphics, so the `gfx90` catch-all below would file it under // `gfx90X-dcgpu` — a datacenter family, which also makes vLLM the - // preferred serving engine. No published family covers gfx90c, and - // "none" sends the user to pick one instead of silently installing a - // runtime built for other silicon. - value if value.starts_with("gfx90c") => None, + // preferred serving engine. It gets a single-target family of its own, + // like gfx900/gfx906/gfx908/gfx90a beside it. Whether a channel + // publishes a gfx90c payload is the index's answer, not this table's: + // when it does not, the install says so by name. + value if value.starts_with("gfx90c") => Some("gfx90c".to_owned()), value if value.starts_with("gfx900") => Some("gfx900".to_owned()), value if value.starts_with("gfx906") => Some("gfx906".to_owned()), value if value.starts_with("gfx908") => Some("gfx908".to_owned()), @@ -4424,6 +4425,7 @@ pub const fn known_therock_families() -> &'static [&'static str] { "gfx906", "gfx908", "gfx90a", + "gfx90c", "gfx94X-dcgpu", "gfx950-dcgpu", "gfx101X-dgpu", @@ -10049,18 +10051,30 @@ mod tests { ); } - /// A Renoir-class iGPU has no published family, and must not be filed under - /// a datacenter one. + /// A Renoir-class iGPU is its own family, not a datacenter one. /// /// gfx90c is Vega-architecture *integrated* graphics, so the `gfx90` /// catch-all would answer `gfx90X-dcgpu` — which is not just a wrong label: /// `preferred_serve_engine_for_therock_family` reads `-dcgpu` and picks - /// vLLM. Its neighbours are genuinely discrete and must keep their - /// families. + /// vLLM. Answering `None` instead is no better: every `--family` value is + /// then rejected for this host, so the "re-run with an explicit + /// `--family`" advice the install prints cannot be followed. Its neighbours + /// are genuinely discrete and keep their families. #[test] - fn normalize_therock_family_refuses_to_file_an_igpu_under_a_datacenter_family() { - assert_eq!(normalize_therock_family("gfx90c"), None); - assert_eq!(normalize_therock_family("gfx90c:xnack-"), None); + fn a_renoir_class_igpu_is_its_own_family_and_not_a_datacenter_one() { + assert_eq!( + normalize_therock_family("gfx90c"), + Some("gfx90c".to_owned()) + ); + assert_eq!( + normalize_therock_family("gfx90c:xnack-"), + Some("gfx90c".to_owned()) + ); + assert!(known_therock_families().contains(&"gfx90c")); + assert_eq!( + preferred_serve_engine_for_therock_family(Some("gfx90c")), + None + ); assert_eq!( normalize_therock_family("gfx90a"), Some("gfx90a".to_owned()) From 3a5be499ff491a715b77525d321e6f35eea72b68 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Fri, 2 Oct 2026 14:35:49 +0000 Subject: [PATCH 5/5] test(examine): pin the GPU probes' own row logic, not a copy of it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commits made both PCI probes prefer the device id to the marketing name, and made the hipInfo fold leave an unrecognised target's verdict alone. None of the three was exercised by any test that could fail on it: no test reached `probe_gpus_lspci` or `probe_gpus_windows`, which shell out, and the tests that built PCI rows assembled them from the classifiers by hand, re-implementing the name-only logic the probes had just stopped using. Making the Linux scan read the name only, or restoring the unconditional overwrite in the hipInfo fold, left every test green. Each probe now keeps only its process launch; what it does with the output moves into a pure fold over caller-supplied text — `apply_lspci_gpus`, `apply_windows_display_rows`, `apply_hipinfo_gcn_arch_names` — the split `apply_rocminfo_gpu_agents` already has. Every test that needs a PCI-sourced GPU now runs a real row through the real fold. New tests sweep every id the device-id table maps through a row whose name carries nothing (`Device` on Linux; on Windows "AMD Radeon(TM) Graphics", which is what Renoir, Rembrandt and Raphael iGPUs actually report), so each probe's id preference fails for all of them at once if it regresses. The hipInfo rule cannot be shown to matter end to end today: the display probe only reaches an APU verdict by resolving a target, and the fold only touches rows without one, so an unknown target meets `Some(false)` either way. Its test says so, and pins the fold's contract on a constructed state instead, so the first new source of a target-less verdict does not inherit the downgrade silently. An earlier message here said an unlisted target answers "unknown", not "discrete". That is true of `gfx_target_packaging` and of the rocminfo fold, and not of the PCI probes: a row neither table identifies is reported discrete, on purpose. The only consumer that acts on the verdict is the iGPU+dGPU collision check, which needs `has_apu && has_discrete_amd`. Reporting an unidentifiable row as unknown would clear `has_discrete_amd` beside every discrete card the tables do not name — RX 5000, Vega, Radeon VII — on exactly the hybrid host that check exists for. Reporting it discrete costs a wrong `has_discrete_amd` on an iGPU-only host the tables do not name, Mendocino for one, where the check still cannot fire. That trade is now one function, `pci_row_is_apu`, documented there and pinned by a test that drives both hosts through the diagnosis catalog. Also: the cross-check of the Windows probes is restated as what it now is — they share a decoder, so it guards the row shape each one feeds it, not agreement between two tables; a test that no discrete corpus part is ever classified as an APU, which also puts the MI300X row to work; corrected provenance for gfx902/gfx909, which come from KFD's packed `gfx_target_version` rather than DRM ip-discovery; and a duplicated block of assertions removed. Signed-off-by: Roman Inflianskas --- crates/rocm-core/src/examine.rs | 122 ++++-- crates/rocm-core/src/examine_proptests.rs | 446 +++++++++++++++------- 2 files changed, 395 insertions(+), 173 deletions(-) diff --git a/crates/rocm-core/src/examine.rs b/crates/rocm-core/src/examine.rs index ad037b285..5c1332a1f 100644 --- a/crates/rocm-core/src/examine.rs +++ b/crates/rocm-core/src/examine.rs @@ -1018,10 +1018,11 @@ enum GfxPackaging { /// This is not every target AMD has ever shipped, and it cannot be: a part /// nobody here has seen is `None`, not "discrete" — see /// [`gfx_target_packaging`]. The drift guard only covers targets the lookup -/// tables produce, so targets that reach the CLI another way (notably -/// [`crate::gfx_target_from_gc_version`], which synthesises one from the GC -/// version in DRM ip-discovery and so can name a part no table lists) have to -/// be added here by hand. +/// tables produce, so targets that reach the CLI another way have to be added +/// here by hand. The notable one is [`crate::gfx_target_from_gc_version`], +/// which synthesises a target from a GC version — KFD's packed +/// `gfx_target_version` or DRM ip-discovery — and so can name a part no table +/// lists: gfx902, gfx909 and gfx1037 arrive that way. /// /// And the "packaging is a property of the target" premise has one known /// exception: gfx942 covers both the MI300X accelerator and the MI300A, which @@ -1150,6 +1151,17 @@ fn probe_gpus_lspci(e: &mut Examination) { .push("lspci returned non-zero; PCI enumeration incomplete".to_owned()); return; } + apply_lspci_gpus(e, &out); +} + +/// Fold an `lspci -nn -D` listing into the GPU list, against caller-supplied +/// output. +/// +/// Split from the process launch for the same reason +/// [`apply_rocminfo_gpu_agents`] is: what matters here is how a row is +/// classified, and a test that had to run the real `lspci` could only assert it +/// on the hardware it happened to be run on. +fn apply_lspci_gpus(e: &mut Examination, out: &str) { for line in out.lines() { if !is_lspci_gpu_line(line) { continue; @@ -1189,7 +1201,7 @@ fn probe_gpus_lspci(e: &mut Examination) { .map_or_else(|| classify_amd_marketing_name(&name).0, str::to_owned); e.gpus.push(Gpu { name, - is_apu: Some(gfx_is_apu_family(&gfx_guess)), + is_apu: pci_row_is_apu(&gfx_guess), gfx_target: gfx_guess, pci_id, is_amd: true, @@ -1197,6 +1209,39 @@ fn probe_gpus_lspci(e: &mut Examination) { } } +/// The APU verdict a PCI-enumerated AMD row starts with, from whatever target +/// its id or name resolved — `""` when neither did. +/// +/// An unresolved row is reported as `Some(false)`, "not an APU", and that is a +/// deliberate default rather than a fact; it is the one place in this module +/// where "cannot say" is not left as `None`. The two errors cost different +/// amounts, and the only consumer that acts on the verdict decides which: +/// `check_9_igpu_dgpu_collision` fires on `has_apu && has_discrete_amd`. +/// +/// - Calling an unrecognised *discrete* card "cannot say" clears +/// `has_discrete_amd` on exactly the hybrid host that check is written for. +/// That is not a corner case: the discrete catalogue is large and the SKU +/// tables cover a fraction of it — every RX 5000, Vega 56/64 and Radeon VII, +/// for a start. +/// - Calling an unrecognised *integrated* GPU discrete sets `has_discrete_amd` +/// on an iGPU-only host. The collision check still needs `has_apu`, which +/// such a host does not have, so nothing fires; the cost is a wrong field in +/// `rocm examine --json`. And the integrated set is small and enumerated by +/// codename in the marketing table, so an unresolved row is far more often +/// a discrete card than an integrated one. +/// +/// `rocminfo` and `hipInfo` revise this verdict whenever they report a target +/// whose packaging is known. Where they do not, it stands. +/// +/// [`apply_rocminfo_gpu_agents`] does *not* use this default for an agent it +/// cannot pair with a PCI row, and the difference is the base rate, not +/// inconsistency: an agent carries the silicon's own target, and the packaging +/// table covers the targets this CLI supports, so an agent whose target is not +/// in it is genuinely unfamiliar silicon, about which there is nothing to say. +fn pci_row_is_apu(gfx_guess: &str) -> Option { + Some(gfx_is_apu_family(gfx_guess)) +} + /// The AMD PCI device id an `lspci -nn` line carries, as the four hex digits /// after `1002:` in the trailing `[vendor:device]` tag. /// @@ -2371,6 +2416,14 @@ fn probe_gpus_windows(e: &mut Examination) { .push("Win32_VideoController query failed; cannot enumerate GPUs.".to_owned()); return; } + apply_windows_display_rows(e, &out); +} + +/// Fold `Win32_VideoController` rows — `namedriverPNP id`, as +/// [`WIN_GPU_SCRIPT`] prints them — into the GPU list, against caller-supplied +/// output. Split from the PowerShell launch so the classification can be +/// asserted on a Linux test host, where this probe never otherwise runs. +fn apply_windows_display_rows(e: &mut Examination, out: &str) { for line in out.lines() { let line = line.trim(); if line.is_empty() { @@ -2413,7 +2466,7 @@ fn probe_gpus_windows(e: &mut Examination) { .unwrap_or_else(|| classify_amd_marketing_name(&name).0); e.gpus.push(Gpu { name, - is_apu: Some(gfx_is_apu_family(&gfx_guess)), + is_apu: pci_row_is_apu(&gfx_guess), gfx_target: gfx_guess, pci_id: pnp, is_amd: true, @@ -2457,23 +2510,7 @@ fn probe_hip_sdk_windows(e: &mut Examination) { let (rc, out, _) = run(&hipinfo.to_string_lossy(), &[], Duration::from_secs(15)); if rc == 0 { e.hipinfo_status = "ok".to_owned(); - for line in out.lines() { - if let Some(rest) = line.trim().strip_prefix("gcnArchName:") - && let Some(gfx) = crate::extract_first_gfx_token(rest) - && let Some(gpu) = e - .gpus - .iter_mut() - .find(|g| g.is_amd && g.gfx_target.is_empty()) - { - gpu.gfx_target = gfx; - // Same rule as `apply_rocminfo_gpu_agents`: a target whose - // packaging this crate knows outranks the display name, an - // unknown one leaves that verdict alone. - if let Some(packaging) = gfx_target_packaging(&gpu.gfx_target) { - gpu.is_apu = Some(packaging == GfxPackaging::Integrated); - } - } - } + apply_hipinfo_gcn_arch_names(e, &out); } else { e.hipinfo_status = format!("error rc={rc}"); } @@ -2483,6 +2520,31 @@ fn probe_hip_sdk_windows(e: &mut Examination) { } } +/// Fold `hipInfo.exe`'s `gcnArchName:` lines into AMD GPUs the display probe +/// could not give a target, in order, against caller-supplied output. +/// +/// Split from the launch so it can be driven from a test: this only ever runs +/// on Windows, behind a `hipInfo.exe` that has to exist on disk. +fn apply_hipinfo_gcn_arch_names(e: &mut Examination, out: &str) { + for line in out.lines() { + if let Some(rest) = line.trim().strip_prefix("gcnArchName:") + && let Some(gfx) = crate::extract_first_gfx_token(rest) + && let Some(gpu) = e + .gpus + .iter_mut() + .find(|g| g.is_amd && g.gfx_target.is_empty()) + { + gpu.gfx_target = gfx; + // Same rule as `apply_rocminfo_gpu_agents`: a target whose + // packaging this crate knows outranks the display name, an unknown + // one leaves that verdict alone. + if let Some(packaging) = gfx_target_packaging(&gpu.gfx_target) { + gpu.is_apu = Some(packaging == GfxPackaging::Integrated); + } + } + } +} + fn probe_adrenalin_windows(e: &mut Examination) { let script = "(Get-CimInstance Win32_VideoController | Where-Object { $_.PNPDeviceID -match 'VEN_1002' -or $_.Name -match 'AMD|Radeon|Instinct' } | Select-Object -First 1).DriverVersion"; let (rc, out, _) = run("powershell", &["-NoProfile", "-Command", script], MEDIUM); @@ -3991,16 +4053,12 @@ mod tests { assert!(gfx_is_apu_family("gfx1033")); assert!(gfx_is_apu_family("gfx1035")); assert!(gfx_is_apu_family("gfx1036")); - // gfx90a is the MI200 accelerator, one character away from the Renoir - // iGPU above and emphatically not an APU. - assert!(!gfx_is_apu_family("gfx90a")); - // Unrelated families are never APUs. - assert!(!gfx_is_apu_family("gfx1200")); - assert!(!gfx_is_apu_family("gfx942")); // The targets no lookup table produces, so the cross-table drift guard - // cannot reach them: `gfx_target_from_gc_version` builds these straight - // out of the GC version in DRM ip-discovery. gfx1037 is Mendocino, - // which sells as a Radeon 610M exactly like gfx1036 does. + // cannot reach them: `gfx_target_from_gc_version` synthesises them from + // a GC version — KFD's packed `gfx_target_version` for all three + // (90002, 90009, 100307), and DRM ip-discovery for gfx1037 as well. + // gfx1037 is Mendocino, which sells as a Radeon 610M just as gfx1036 + // does. assert!(gfx_is_apu_family("gfx902")); assert!(gfx_is_apu_family("gfx909")); assert!(gfx_is_apu_family("gfx1037")); diff --git a/crates/rocm-core/src/examine_proptests.rs b/crates/rocm-core/src/examine_proptests.rs index 195754719..21202d8f3 100644 --- a/crates/rocm-core/src/examine_proptests.rs +++ b/crates/rocm-core/src/examine_proptests.rs @@ -15,7 +15,8 @@ //! implementation detail, so a failure names a user-visible defect. use super::{ - Examination, GFX_TARGET_PACKAGING, Gpu, apply_rocminfo_gpu_agents, classify_amd_marketing_name, + Examination, GFX_TARGET_PACKAGING, Gpu, apply_hipinfo_gcn_arch_names, apply_lspci_gpus, + apply_rocminfo_gpu_agents, apply_windows_display_rows, classify_amd_marketing_name, extract_lspci_name, gfx_is_apu_family, is_lspci_gpu_line, summarise_gpu_categories, }; use proptest::prelude::*; @@ -144,9 +145,10 @@ const AMD_MARKETING_NAMES: &[(&str, &str, bool)] = &[ /// /// Not every one of these is produced by a lookup table — gfx902, gfx909 and /// gfx1037 reach the CLI only through -/// [`crate::gfx_target_from_gc_version`], which synthesises a target from the -/// GC version DRM ip-discovery reports. That is exactly why they are here: the -/// cross-table drift guard cannot see them, so ground truth has to. +/// [`crate::gfx_target_from_gc_version`], which synthesises a target from a GC +/// version (KFD's packed `gfx_target_version`, or DRM ip-discovery). That is +/// exactly why they are here: the cross-table drift guard cannot see them, so +/// ground truth has to. const GFX_TARGETS: &[(&str, bool)] = &[ ("gfx900", false), ("gfx906", false), @@ -338,6 +340,50 @@ fn apu_lspci_devices() -> Vec<(&'static str, &'static str, &'static str, bool)> apus } +/// What the real PCI scan makes of one corpus device. +/// +/// Every test that needs a PCI-sourced GPU goes through this rather than +/// assembling one from the classifiers, because a test that re-implements the +/// probe's logic pins the re-implementation: when the probe stops reading the +/// device id, a hand-built row keeps passing. +fn pci_scanned(device: &str, device_id: &str, addr: &str) -> Gpu { + let line = format!( + "{addr} VGA compatible controller [0300]: Advanced Micro Devices, Inc. \ + [AMD/ATI] {device} [1002:{device_id}] (rev c1)" + ); + let mut e = Examination::default(); + apply_lspci_gpus(&mut e, &line); + assert_eq!(e.gpus.len(), 1, "{line:?} must enumerate as one GPU"); + e.gpus.remove(0) +} + +/// A `Win32_VideoController` row as `WIN_GPU_SCRIPT` prints it: name, driver +/// version, PNP device id. +fn windows_row(name: &str, device_id: &str) -> String { + format!("{name}\t32.0.1\tPCI\\VEN_1002&DEV_{device_id}&SUBSYS_00001002&REV_C1") +} + +/// What the real Windows display probe makes of one row. +fn windows_scanned(name: &str, device_id: &str) -> Gpu { + let row = windows_row(name, device_id); + let mut e = Examination::default(); + apply_windows_display_rows(&mut e, &row); + assert_eq!(e.gpus.len(), 1, "{row:?} must enumerate as one GPU"); + e.gpus.remove(0) +} + +/// Every PCI device id the crate's device-id table maps, with its target. +/// +/// The table is a `match`, not an iterable, so its whole input domain is swept. +fn every_mapped_pci_device_id() -> Vec<(String, &'static str)> { + let ids: Vec<_> = (0..=0xffff_u32) + .map(|id| format!("{id:04x}")) + .filter_map(|id| crate::gfx_target_from_amd_pci_device_id(&id).map(|t| (id, t))) + .collect(); + assert!(!ids.is_empty(), "the device-id table maps no id at all"); + ids +} + /// A `rocminfo` agent listing for the given targets, in the agent block shape /// the current parser can digest (no ISA sub-entries). fn rocminfo_output(agents: &[(&str, &str)]) -> String { @@ -447,23 +493,17 @@ proptest! { marketing in proptest::sample::select(&["", "AMD Radeon Graphics"][..]), ) { // A coherent host: the gfx target is the one this very device reports. - let name = format!("Advanced Micro Devices, Inc. [AMD/ATI] {}", device.0); - let (gfx_guess, is_apu_guess) = classify_amd_marketing_name(&name); + let gpu = pci_scanned(device.0, device.1, &addr); prop_assert!( - is_apu_guess, + gpu.is_apu == Some(true), "the PCI scan must already know {} is an APU before this property \ - can say anything about preserving that verdict", + can say anything about preserving that verdict: {:?}", device.0, + gpu, ); let mut e = Examination { - gpus: vec![Gpu { - name, - gfx_target: gfx_guess, - pci_id: addr, - is_apu: Some(true), - is_amd: true, - }], + gpus: vec![gpu], ..Examination::default() }; apply_rocminfo_gpu_agents(&mut e, &rocminfo_output(&[(device.2, marketing)])); @@ -517,76 +557,44 @@ proptest! { ); } - /// The Windows display probe and `examine`'s own Windows GPU enumeration - /// read the same row and must not disagree about the gfx target. + /// `examine` and the install-side display probe must read the same + /// adapter the same way. + /// + /// They share a decoder, so this is not two tables cross-checking each + /// other: it guards the *wiring*. The install side hands the decoder + /// `namepnp`, `examine` hands it `namedriverpnp`, and a + /// decoder that read the PNP id by column position rather than by content + /// would quietly resolve one and not the other. #[test] - fn windows_probes_agree_on_gfx_target( + fn examine_and_the_install_probe_read_a_windows_row_alike( entry in proptest::sample::select(AMD_MARKETING_NAMES), - subsys in 0u32..0x1_0000, ) { - // Only coherent rows: the PNP id must name the same part the marketing - // name does, or the two probes are being asked about different GPUs. let Some(device_id) = entry_device_id(entry.0) else { return Ok(()); }; - // A Win32_VideoController row as `probe_gpus_windows` reads it: name, - // driver version, PNP device id. - let pnp = format!("PCI\\VEN_1002&DEV_{device_id}&SUBSYS_{subsys:04x}1002&REV_C1"); - let row = format!("{}\t32.0.1\t{pnp}", entry.0); - let install_target = crate::parse_windows_display_gfx_target(&row); - let examine_target = classify_amd_marketing_name(entry.0).0; - if let Some(install_target) = install_target - && !examine_target.is_empty() - { - prop_assert_eq!( - &examine_target, - &install_target, - "the Windows display probe and examine disagree about {}", - entry.0, - ); - } - } - - /// `probe_gpus_windows` has the PNP device id in hand on every row, and the - /// crate already decodes it. When that decode names an APU target, the - /// report must not come back `is_apu=false` just because the marketing - /// name was not in `examine`'s own smaller table. - #[test] - fn the_windows_row_is_not_called_discrete_when_its_pnp_id_names_an_apu( - entry in proptest::sample::select(AMD_MARKETING_NAMES), - subsys in 0u32..0x1_0000, - ) { - let Some(device_id) = entry_device_id(entry.0) else { - return Ok(()); - }; - let pnp = format!("PCI\\VEN_1002&DEV_{device_id}&SUBSYS_{subsys:04x}1002&REV_C1"); - let row = format!("{}\t32.0.1\t{pnp}", entry.0); - let Some(install_target) = crate::parse_windows_display_gfx_target(&row) else { - return Ok(()); - }; - prop_assume!(gfx_is_apu_family(&install_target)); - let (_target, is_apu) = classify_amd_marketing_name(entry.0); - prop_assert!( - is_apu, - "{} has PNP id {} which this crate decodes to the APU target {}, \ - yet examine reports is_apu=false", + let install_text = format!( + "{}\tPCI\\VEN_1002&DEV_{device_id}&SUBSYS_00001002&REV_C1", + entry.0 + ); + let install_target = crate::parse_windows_display_gfx_target(&install_text); + let examine_target = windows_scanned(entry.0, device_id).gfx_target; + prop_assert_eq!( + install_target.unwrap_or_default(), + examine_target, + "the install probe and examine disagree about {}", entry.0, - pnp, - install_target, ); } - } /// Corpus entries whose `lspci` text names no model at all. /// /// `lspci` prints the `pci.ids` device string, and when that database has no -/// entry for an id it prints the bare word `Device`. A name-keyed lookup can -/// never classify such a row: the identifying information is in the -/// `[1002:xxxx]` id on the same line, which the PCI scan currently discards. -/// They are excluded from the name-classification sweeps below rather than -/// dropped from the corpus, because the parsing and totality properties still -/// have to survive them. +/// entry for an id it prints the bare word `Device`. The PCI scan then has only +/// the `[1002:xxxx]` id to go on, and these are ids the crate's device-id table +/// does not map yet (Strix Halo's, here), so nothing can classify them. They +/// are excluded from the APU sweeps below rather than dropped from the corpus, +/// because the parsing and totality properties still have to survive them. const LSPCI_DEVICES_WITHOUT_A_PCI_IDS_NAME: &[&str] = &["Device"]; /// The PCI device id for a marketing name, when the corpus pins one. @@ -631,10 +639,10 @@ fn every_known_apu_is_classified_as_an_apu() { if !is_apu || LSPCI_DEVICES_WITHOUT_A_PCI_IDS_NAME.contains(device) { continue; } - let name = format!("Advanced Micro Devices, Inc. [AMD/ATI] {device}"); - if !classify_amd_marketing_name(&name).1 { + let gpu = pci_scanned(device, id, "0000:04:00.0"); + if gpu.is_apu != Some(true) { misses.push(format!( - " lspci device {device:?} [1002:{id}]: examine says is_apu=false" + " lspci device {device:?} [1002:{id}]: the PCI scan says {gpu:?}" )); } } @@ -651,8 +659,7 @@ fn every_known_apu_is_classified_as_an_apu() { fn no_part_is_labelled_with_the_wrong_gfx_target() { let mut wrong: Vec = Vec::new(); for (device, id, gfx, _is_apu) in AMD_LSPCI_DEVICES { - let name = format!("Advanced Micro Devices, Inc. [AMD/ATI] {device}"); - let guess = classify_amd_marketing_name(&name).0; + let guess = pci_scanned(device, id, "0000:04:00.0").gfx_target; if !guess.is_empty() && guess != *gfx { wrong.push(format!( " lspci {device:?} [1002:{id}] is {gfx}, but examine reports {guess}" @@ -690,18 +697,18 @@ fn no_part_is_labelled_with_the_wrong_gfx_target() { #[test] fn the_apu_verdict_holds_on_both_sides_of_the_rocminfo_isa_name_defect() { let build = |with_isa: bool| { - let name = "Advanced Micro Devices, Inc. [AMD/ATI] Raphael".to_owned(); - let (gfx_target, is_apu) = classify_amd_marketing_name(&name); - assert_eq!(gfx_target, "gfx1036", "the PCI scan alone names the part"); - assert!(is_apu, "the PCI scan alone knows Raphael is an APU"); + let gpu = pci_scanned("Raphael", "164e", "0000:14:00.0"); + assert_eq!( + gpu.gfx_target, "gfx1036", + "the PCI scan alone names the part" + ); + assert_eq!( + gpu.is_apu, + Some(true), + "the PCI scan alone knows Raphael is an APU" + ); let mut e = Examination { - gpus: vec![Gpu { - name, - gfx_target, - pci_id: "0000:14:00.0".to_owned(), - is_apu: Some(is_apu), - is_amd: true, - }], + gpus: vec![gpu], ..Examination::default() }; apply_rocminfo_gpu_agents( @@ -746,17 +753,9 @@ fn the_apu_verdict_holds_on_both_sides_of_the_rocminfo_isa_name_defect() { /// discrete GPU, so the PCI scan's verdict has to survive untouched. #[test] fn folding_in_rocminfo_only_ever_adds_knowledge() { - let host = |lspci_name: &str, agent_target: &str, with_isa: bool| { - let name = format!("Advanced Micro Devices, Inc. [AMD/ATI] {lspci_name}"); - let (gfx_target, is_apu) = classify_amd_marketing_name(&name); + let host = |lspci_name: &str, device_id: &str, agent_target: &str, with_isa: bool| { let mut e = Examination { - gpus: vec![Gpu { - name, - gfx_target, - pci_id: "0000:66:00.0".to_owned(), - is_apu: Some(is_apu), - is_amd: true, - }], + gpus: vec![pci_scanned(lspci_name, device_id, "0000:66:00.0")], ..Examination::default() }; apply_rocminfo_gpu_agents( @@ -768,12 +767,12 @@ fn folding_in_rocminfo_only_ever_adds_knowledge() { }; // A nameless Strix Halo: nothing is known before the fold. - let before = host("Device", "gfx1151", true); + let before = host("Device", "1586", "gfx1151", true); assert_eq!(before.gpus[0].gfx_target, "", "{:?}", before.gpus); assert!(!before.has_apu, "{:?}", before.gpus); // Once the agent is readable the fold supplies both the target and the // packaging that follows from it. - let after = host("Device", "gfx1151", false); + let after = host("Device", "1586", "gfx1151", false); assert_eq!(after.gpus[0].gfx_target, "gfx1151", "{:?}", after.gpus); assert!( after.has_apu, @@ -785,7 +784,7 @@ fn folding_in_rocminfo_only_ever_adds_knowledge() { // A Rembrandt laptop whose agent names a target this crate has no packaging // entry for. The target is still worth recording; the verdict is not the // fold's to revise. - let unknown = host("Rembrandt [Radeon 680M]", "gfx1154", false); + let unknown = host("Rembrandt [Radeon 680M]", "1681", "gfx1154", false); assert_eq!(unknown.gpus[0].gfx_target, "gfx1154", "{:?}", unknown.gpus); assert!( unknown.has_apu, @@ -913,23 +912,12 @@ fn the_igpu_dgpu_collision_check_still_fires_on_a_raphael_plus_rx7900_host() { [AMD/ATI] Raphael [1002:164e] (rev c1)"; let dgpu = "0000:03:00.0 VGA compatible controller [0300]: Advanced Micro Devices, Inc. \ [AMD/ATI] Navi 31 [Radeon RX 7900 XT/7900 XTX/7900 GRE/7900M] [1002:744c]"; - let mut gpus = Vec::new(); - for (line, pci) in [(dgpu, "0000:03:00.0"), (igpu, "0000:14:00.0")] { - let name = extract_lspci_name(line); - let (gfx_target, is_apu) = classify_amd_marketing_name(&name); - gpus.push(Gpu { - name, - gfx_target, - pci_id: pci.to_owned(), - is_apu: Some(is_apu), - is_amd: true, - }); - } let mut e = Examination { os_family: "linux".to_owned(), - gpus, ..Examination::default() }; + apply_lspci_gpus(&mut e, &format!("{dgpu}\n{igpu}\n")); + assert_eq!(e.gpus.len(), 2, "{:?}", e.gpus); // rocminfo in KFD node order, matching the PCI order the scan produced. apply_rocminfo_gpu_agents( &mut e, @@ -972,19 +960,12 @@ fn no_known_apu_has_its_verdict_downgraded_by_rocminfo() { if !is_apu { continue; } - let name = format!("Advanced Micro Devices, Inc. [AMD/ATI] {device}"); - let (gfx_guess, is_apu_guess) = classify_amd_marketing_name(&name); - if !is_apu_guess { + let gpu = pci_scanned(device, id, "0000:04:00.0"); + if gpu.is_apu != Some(true) { continue; } let mut e = Examination { - gpus: vec![Gpu { - name: name.clone(), - gfx_target: gfx_guess, - pci_id: "0000:04:00.0".to_owned(), - is_apu: Some(true), - is_amd: true, - }], + gpus: vec![gpu], ..Examination::default() }; apply_rocminfo_gpu_agents(&mut e, &rocminfo_output(&[(gfx, "AMD Radeon Graphics")])); @@ -1032,25 +1013,20 @@ fn a_rembrandt_laptop_is_not_reported_as_a_discrete_gpu() { ) .expect("plant the GPU node properties"); - let line = "0000:04:00.0 VGA compatible controller [0300]: Advanced Micro Devices, Inc. \ - [AMD/ATI] Rembrandt [Radeon 680M] [1002:1681] (rev c8)"; - let name = extract_lspci_name(line); - let (gfx_guess, is_apu_guess) = classify_amd_marketing_name(&name); - assert!( - is_apu_guess, - "the PCI scan alone already knows Rembrandt is an APU" + // The whole Linux sequence from the PCI scan on: `apply_lspci_gpus` is the + // scan itself, minus only the process launch. + let mut e = Examination::default(); + apply_lspci_gpus( + &mut e, + "0000:04:00.0 VGA compatible controller [0300]: Advanced Micro Devices, Inc. \ + [AMD/ATI] Rembrandt [Radeon 680M] [1002:1681] (rev c8)", + ); + assert_eq!( + e.gpus.first().and_then(|gpu| gpu.is_apu), + Some(true), + "the PCI scan alone already knows Rembrandt is an APU: {:?}", + e.gpus ); - - let mut e = Examination { - gpus: vec![Gpu { - name, - gfx_target: gfx_guess, - pci_id: "0000:04:00.0".to_owned(), - is_apu: Some(is_apu_guess), - is_amd: true, - }], - ..Examination::default() - }; super::probe_gpus_after_lspci( &mut e, super::GpuProbeSources { @@ -1076,6 +1052,194 @@ fn a_rembrandt_laptop_is_not_reported_as_a_discrete_gpu() { ); } +/// The Linux PCI scan classifies a row by its device id when the name carries +/// nothing. +/// +/// `lspci` prints the bare word `Device` for an id `pci.ids` does not know, so +/// on these rows only the `[1002:xxxx]` tag can identify the part. Every id the +/// device-id table maps is swept, so a scan that falls back to reading the name +/// alone fails here for all of them at once. +#[test] +fn an_lspci_row_is_classified_by_its_device_id_when_its_name_says_nothing() { + let mut wrong: Vec = Vec::new(); + for (id, target) in every_mapped_pci_device_id() { + let gpu = pci_scanned("Device", &id, "0000:66:00.0"); + if gpu.gfx_target != target || gpu.is_apu != Some(gfx_is_apu_family(target)) { + wrong.push(format!(" [1002:{id}] is {target}, scanned as {gpu:?}")); + } + } + assert!( + wrong.is_empty(), + "lspci rows the device id identifies but the scan did not:\n{}", + wrong.join("\n") + ); +} + +/// The Windows display probe classifies a row by its PNP id when the name +/// carries nothing. +/// +/// "AMD Radeon(TM) Graphics" is not a placeholder: it is what Renoir, Rembrandt +/// and Raphael iGPUs actually report as their adapter name on Windows, so for +/// the commonest APUs the PNP id is the only thing on the row that names the +/// part. +#[test] +fn a_windows_row_is_classified_by_its_pnp_id_when_its_name_says_nothing() { + assert_eq!( + classify_amd_marketing_name("AMD Radeon(TM) Graphics").0, + "", + "the premise: the generic name alone must resolve nothing" + ); + let mut wrong: Vec = Vec::new(); + for (id, target) in every_mapped_pci_device_id() { + let gpu = windows_scanned("AMD Radeon(TM) Graphics", &id); + if gpu.gfx_target != target || gpu.is_apu != Some(gfx_is_apu_family(target)) { + wrong.push(format!(" DEV_{id} is {target}, scanned as {gpu:?}")); + } + } + assert!( + wrong.is_empty(), + "Windows rows the PNP id identifies but the probe did not:\n{}", + wrong.join("\n") + ); +} + +/// `hipInfo` revises the display probe's verdict when it reports a target whose +/// packaging is known, and only then. +/// +/// The first half is a host that exists: a Renoir laptop's adapter is named +/// "AMD Radeon(TM) Graphics" and its device id is one the table does not map, +/// so the display probe gives it no target and the PCI-row default, and only +/// `hipInfo`'s `gcnArchName` can say what it is. +/// +/// The second half pins the fold's contract on a state the Windows display +/// probe cannot currently produce — a GPU already called an APU, with no +/// target. The display probe only reaches `Some(true)` by resolving a target, +/// so through the real pipeline the "leave an unknown target's verdict alone" +/// rule changes no output today. It is asserted anyway because it is the rule +/// every other fold in this module follows, and the first new source of a +/// target-less verdict would otherwise inherit the downgrade silently. +#[test] +fn a_hipinfo_target_revises_a_verdict_only_when_its_packaging_is_known() { + let mut e = Examination::default(); + apply_windows_display_rows(&mut e, &windows_row("AMD Radeon(TM) Graphics", "1636")); + assert_eq!(e.gpus[0].gfx_target, "", "{:?}", e.gpus); + apply_hipinfo_gcn_arch_names(&mut e, "device# 0\n gcnArchName: gfx90c:xnack-\n"); + assert_eq!(e.gpus[0].gfx_target, "gfx90c", "{:?}", e.gpus); + assert_eq!( + e.gpus[0].is_apu, + Some(true), + "a known integrated target must revise the display probe's default: {:?}", + e.gpus + ); + + let mut e = Examination { + gpus: vec![Gpu { + name: "AMD Radeon(TM) Graphics".to_owned(), + is_amd: true, + is_apu: Some(true), + ..Gpu::default() + }], + ..Examination::default() + }; + apply_hipinfo_gcn_arch_names(&mut e, " gcnArchName: gfx1154\n"); + assert_eq!(e.gpus[0].gfx_target, "gfx1154", "{:?}", e.gpus); + assert_eq!( + e.gpus[0].is_apu, + Some(true), + "an unrecognised target is not evidence of a discrete GPU: {:?}", + e.gpus + ); +} + +/// An AMD PCI row nothing can identify is reported as discrete, deliberately, +/// and both sides of that trade are pinned here — see `pci_row_is_apu`. +/// +/// What the default buys: a Ryzen 7000 desktop with an RX 5700 XT. Navi 10 is +/// in neither SKU table, so its row resolves no target; reported as "cannot +/// say", it would clear `has_discrete_amd` and the iGPU+dGPU collision check +/// would never fire on the host it is written for. +/// +/// What it costs: a Mendocino laptop, whose iGPU likewise resolves nothing, is +/// reported with `has_discrete_amd` set. The collision check still needs +/// `has_apu`, which that host does not have, so no diagnosis fires — the cost +/// is a wrong field in `rocm examine --json`, which is asserted too rather +/// than left implicit. +#[test] +fn an_unidentifiable_pci_row_is_reported_as_discrete_and_this_is_what_that_costs() { + let raphael = "0000:14:00.0 VGA compatible controller [0300]: Advanced Micro Devices, \ + Inc. [AMD/ATI] Raphael [1002:164e] (rev c1)"; + let navi10 = "0000:03:00.0 VGA compatible controller [0300]: Advanced Micro Devices, \ + Inc. [AMD/ATI] Navi 10 [Radeon RX 5600 OEM/5600 XT / 5700/5700 XT] \ + [1002:731f] (rev c1)"; + let mendocino = "0000:04:00.0 VGA compatible controller [0300]: Advanced Micro Devices, \ + Inc. [AMD/ATI] Mendocino [1002:1506] (rev c1)"; + let host = |lines: &[&str]| { + let mut e = Examination { + os_family: "linux".to_owned(), + ..Examination::default() + }; + apply_lspci_gpus(&mut e, &lines.join("\n")); + summarise_gpu_categories(&mut e); + let fires = crate::diagnose::diagnose(&e, "my training run segfaults") + .matched + .iter() + .any(|d| d.id == "fix-9-igpu-dgpu" && d.score > 0); + (e, fires) + }; + + let (hybrid, fires) = host(&[navi10, raphael]); + assert_eq!( + hybrid.gpus[0].gfx_target, "", + "the premise: Navi 10 is unresolved" + ); + assert!( + hybrid.has_apu && hybrid.has_discrete_amd && fires, + "an unresolved discrete card beside a known iGPU must still raise the \ + collision diagnosis: {:?}", + hybrid.gpus + ); + + let (laptop, fires) = host(&[mendocino]); + assert_eq!( + laptop.gpus[0].gfx_target, "", + "the premise: Mendocino is unresolved" + ); + assert!( + laptop.has_discrete_amd && !laptop.has_apu, + "the known cost: an unresolved iGPU reads as discrete: {:?}", + laptop.gpus + ); + assert!( + !fires, + "and the cost stops there — no iGPU+dGPU diagnosis on a one-GPU laptop" + ); +} + +/// No discrete part in the corpus is ever classified as an APU. +/// +/// The APU sweeps above only look for misses; this is the other direction, +/// and it is what keeps a codename pattern in the marketing table from being +/// broader than the part it names. +#[test] +fn no_discrete_part_is_classified_as_an_apu() { + let mut wrong: Vec = Vec::new(); + for (name, gfx, is_apu) in AMD_MARKETING_NAMES { + if !is_apu && classify_amd_marketing_name(name).1 { + wrong.push(format!(" marketing name {name:?} ({gfx})")); + } + } + for (device, id, gfx, is_apu) in AMD_LSPCI_DEVICES { + if !is_apu && pci_scanned(device, id, "0000:03:00.0").is_apu == Some(true) { + wrong.push(format!(" lspci {device:?} [1002:{id}] ({gfx})")); + } + } + assert!( + wrong.is_empty(), + "discrete parts classified as APUs:\n{}", + wrong.join("\n") + ); +} + /// Measure how far the generators actually reach, by drawing from them /// directly and tallying which branches each draw lands in. ///