Repository navigation
Conversation
volen-silo
left a comment
volen-silo
left a comment
There was a problem hiding this comment.
I went through this carefully, mostly because the description makes strong claims about what the tests guarantee. The production logic holds up well — I traced the "never downgrade" rule through both branches of the rocminfo fold, the hipInfo fold, the sysfs fallback and the probe ordering in probe_with_interpreter, and I could not find a path where it pins a verdict optimistically high, nor a currently-shipping part whose classification silently flips. Keying packaging off the target instead of a prefix rule is clearly the right call, and the drift guard that sweeps all 65,536 device ids plus the marketing table is a genuinely good piece of defensive design. cargo test -p rocm-core --lib is green here at 478 passed.
What I'd like changed is on the test side, and one small thing in the Windows probe. The headline is that the 2048-case property named for this PR's central invariant — rocminfo_never_downgrades_an_apu_verdict — cannot fail under the mutation it is named for. Its generator only draws corpus devices whose targets are all present in GFX_TARGET_PACKAGING, and on exactly those inputs the new conditional overwrite and the old unconditional one agree by construction. I reproduced the mutation: restoring the unconditional overwrite does fail a test, so the table in the description is literally true, but the only thing that fails is the hand-written example folding_in_rocminfo_only_ever_adds_knowledge. The property stays green. Given how much weight the description puts on the property harness, that gap is worth closing before merge rather than after.
The rest are smaller: a reach floor that is > 0 where its four neighbours are fractions of the draw count (and four more measured counters with no floor at all), a determinism assertion that compares a pure function to itself, a fallback in the Windows probe that cannot produce a different answer than the call it follows, and one marketing pattern that was quietly dropped when examine started delegating to the crate table. Details inline.
A few things I suspected and could not make stick, in case it saves anyone re-checking them: extract_lspci_device_id handles the bracket shapes lspci -nn -D actually emits, including the nameless Device row and non-AMD vendor tags; the base-token extraction in gfx_target_packaging copes with feature suffixes, -generic and case; the positional pairing of rocminfo agents to PCI rows is unchanged in effect by this PR even though it is still positional; the new bare gfx90c family name does not trip any suffix-matching consumer, and preferred_serve_engine_for_therock_family correctly stops recommending vLLM for it; and nothing in the e2e expectations, the remote-ssh fixture, the CI lane hardware or the docs describes the old behaviour and goes stale.
| /// a worse answer. | ||
| #[test] | ||
| fn rocminfo_never_downgrades_an_apu_verdict( | ||
| device in proptest::sample::select(apu_lspci_devices()), |
There was a problem hiding this comment.
This property is named for the invariant the PR is built around, but it cannot observe the fix.
apu_lspci_devices() only yields AMD_LSPCI_DEVICES entries with is_apu == true, and every target those entries carry (gfx90c, gfx1033, gfx1035, gfx1036, gfx1103, gfx1150, gfx1152) is in GFX_TARGET_PACKAGING. The agent fed to apply_rocminfo_gpu_agents below is device.2, the same device's own target. So on every input this generator can produce, gfx_target_packaging(..) returns Some(..) and the new conditional overwrite computes exactly what the old unconditional gpu.is_apu = Some(gfx_is_apu_family(&gfx)) computed. The two versions are indistinguishable across the property's entire domain.
I restored the unconditional overwrite to check: all 2048 cases pass, and the only failure in the suite is the example folding_in_rocminfo_only_ever_adds_knowledge. That example's unknown-target sub-case (gfx1154) is therefore the sole guard on this fix — weaken or delete it and the regression returns with the property suite still green.
The fix is to put an unrecognised target in the property's reach: either widen the generator so the agent target can be one GFX_TARGET_PACKAGING does not classify while the PCI row still resolves an APU, or add a sibling property that does. As written the property asserts something true but vacuous.
There was a problem hiding this comment.
Agreed: the property could not fail under the mutation it was named for. Fixed in 9ef5574 (branch rebuilt on main; see the PR comment).
rocminfo_never_downgrades_an_apu_verdict now draws the agent's target from agent_target_for(device_target). That is the device's own target three times in four, and otherwise one of UNCLASSIFIED_GFX_TARGETS (gfx1170, gfx1180, gfx1260, gfx1300), targets the crate does not classify.
every_target_a_lookup_table_produces_has_an_apu_verdictasserts that those four really have no verdict, so the draw cannot silently become a known target. (This caught a real problem while rebuilding: I first usedgfx1154, which thegfx115xarm classifies as an APU.)generator_reach_is_measured_and_sufficientputs a floor of 12.5% on the unclassified draws. The measured rate is 26%.
Mutation: restoring the unconditional gpu.is_apu = Some(gfx_is_apu_family(&gfx)) in the rocminfo fold now fails the property, plus folding_in_rocminfo_only_ever_adds_knowledge and a_rembrandt_laptop_keeps_its_apu_verdict_through_every_probe. Before, only the example failed.
| "the lspci corpus barely samples APU silicon" | ||
| ); | ||
| assert!( | ||
| lspci_gfx_resolved > 0, |
There was a problem hiding this comment.
This floor is > 0 where the three above it are DRAWS/2, DRAWS/3 and DRAWS/4. A generator that resolved a gfx target on one lucky draw in 4096 would satisfy it, so it does not actually protect against the reach regression the comment above describes ("a generator that reaches none of these is worthless" is exactly what it encodes, and no more). Since every entry in AMD_LSPCI_DEVICES has a known target, > DRAWS/4 alongside lspci_apu_device looks achievable.
Related, in the same test: lspci_classified_apu, marketing_examine, marketing_install and marketing_apu_truth are all counted and printed but never asserted on. The PR description says measured reach is "asserted as floors so the harness cannot rot", which is true of four counters and not of the other four. Either give them floors or drop them from the print so the test's coverage claim matches what it enforces.
There was a problem hiding this comment.
Fixed in 9ef5574. Every counter the test prints now has a floor, and the floors sit well under the measured rates (2048 draws):
| region | measured | floor |
|---|---|---|
| lspci name parsed | 100% | 50% |
| lspci enumerated GPU class | 50% | 33% |
| lspci APU silicon | 61% | 25% |
| lspci scanned as an APU | 26% | 16.7% |
| lspci gfx target resolved | 42% | 25% (was > 0) |
| marketing name resolved | 92% | 50% |
| marketing APU silicon | 65% | 33% |
| rocminfo unclassified agent target | 26% | 12.5% |
The "scanned as an APU" and "resolved" counters now come from the real apply_lspci_gpus fold rather than the name classifier, so they measure what the PCI scan produces. The marketing_install and marketing_both counters are gone: examine now asks the same name table as the install side, so comparing the two would always agree.
| 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)); |
There was a problem hiding this comment.
Both sides of this are the same call on the same argument, so it cannot fail. The three assertions above it have the same shape, which is deliberate — the test is named for determinism — but gfx_is_apu_family, extract_lspci_name and classify_amd_marketing_name are pure functions over &str with no interior mutability and no randomness, so determinism is a property of the language here rather than of this code. What the test genuinely buys is totality: none of the classifiers panic on a mutated line. That is worth having, but the name and doc comment promise more than the body can deliver, and this last line adds nothing the preceding one did not. I'd either drop it and rename the test to say it is a no-panic sweep, or make the determinism half meaningful by comparing against a value captured before some state change.
There was a problem hiding this comment.
Agreed, the determinism half was vacuous. Fixed in 9ef5574. The test is now classifiers_never_panic_on_a_mutated_lspci_line.
- It drops every self-comparison.
- It runs each classifier (
is_lspci_gpu_line,extract_lspci_device_id, the name classifier,gfx_apu_verdict) and also the realapply_lspci_gpusfold that composes them, on each mutated line. - The one assertion it makes is about the fold's output: a single line never enumerates as more than one GPU.
| // 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); |
There was a problem hiding this comment.
This fallback cannot produce a different answer from the call it follows.
parse_windows_display_gfx_target already does the marketing-name fallback internally: when the PNP id does not resolve, it returns gfx_target_from_amd_marketing_name(name) on the row's first field. So reaching this closure means that lookup already returned None — and classify_amd_marketing_name(&name).0 is gfx_target_from_amd_marketing_name(name).unwrap_or_default() over the same (whitespace-normalised, so effectively identical) name. It is provably equivalent to .unwrap_or_default(), while reading as a second resolution strategy that does not exist.
Separately, worth a look even though it works today: parse_windows_display_gfx_target is documented for name<TAB>pnp rows, but this passes the three-field name<TAB>driver<TAB>PNP id row that WIN_GPU_SCRIPT emits. It survives because split_once('\t') lumps driver and PNP id together and amd_pci_device_id_from_pnp_id substring-searches the result for VEN_1002/DEV_. If that helper ever became stricter about the shape of a PNP id, this probe would silently stop reading device ids with no test failing. Passing fields.get(2) explicitly, or widening the helper's documented contract, would remove the coupling.
There was a problem hiding this comment.
This code no longer exists in the PR. While it was open, #676 landed its own version of "the Windows probe asks the PNP id first" on main (parse_gpus_windows_output), and the rebuild in 9ef5574 keeps main's version rather than this one. Main's version addresses both of your points:
- The redundant fallback is gone. It calls
host_gpu::gfx_target_from_pnp_device_id(&pnp)directly and uses the marketing name only in theNonearm. Unlikeparse_windows_display_gfx_target, that function has no name fallback of its own, so the name is genuinely a second strategy. - The coupling is gone. It reads the PNP id with
fields.get(2)from the three-fieldname<TAB>driver<TAB>PNP idrow, instead of relying onsplit_once('\t')lumping driver and PNP id together.
| entry.0 | ||
| ); | ||
| let install_target = crate::parse_windows_display_gfx_target(&install_text); | ||
| let examine_target = windows_scanned(entry.0, device_id).gfx_target; |
There was a problem hiding this comment.
This test's name and docstring read as a cross-check between two independent probes, but after this PR they are not independent: apply_windows_display_rows now calls crate::parse_windows_display_gfx_target directly, so examine_target and install_target come from the same function. What the assertion actually pins is that one function agrees with itself across a two-field and a three-field row — which is useful given the point above, and worth keeping, but it can no longer detect the two probes drifting apart, because they cannot.
Also worth noting here: entry_device_id maps only 6 of the 21 AMD_MARKETING_NAMES entries, so roughly 62% of draws return Ok(()) without exercising anything, and the effective corpus is a small fixed subset rather than the full marketing-name corpus. Several hundred real executions still happen at 2048 cases so it is not vacuous, but the subset only grows if someone remembers to extend entry_device_id alongside the name list. An assertion that the mapped subset is non-empty, in the style of apu_lspci_devices, would stop it shrinking silently.
There was a problem hiding this comment.
Both points addressed in 9ef5574.
Independence. After the rebuild, examine's Windows probe (main's parse_gpus_windows_output) and the install side's parse_windows_display_gfx_target really are different wiring again over the same tables:
examinesplits three fields and asks the PNP id first;- the install side reads
name<TAB>pnpand ranks a gfx90c iGPU below every other adapter.
The test (still examine_and_the_install_probe_read_a_windows_row_alike) now says that in its doc. It also asserts each side against ground truth (target, and is_apu for examine), not only against each other.
The silent subset. entry_device_id is gone. The device id is now a column of AMD_MARKETING_NAMES, and windows_corpus() asserts that at least 10 rows carry one (there are 13 today), so the subset cannot shrink without a failure. The property draws from that corpus directly, so no case returns Ok(()) without exercising anything.
| pattern: "8040s", | ||
| gfx_target: "gfx1151", | ||
| }, | ||
| AmdMarketingGfxTarget { |
There was a problem hiding this comment.
A pattern was dropped in the move to this table. The classifier this PR deletes from examine.rs matched three Strix Halo strings — radeon 8050s, radeon 8060s and radeon 8045s. This section carries 8060s, 8050s and 8040s; 8045s appears nowhere in the tree at HEAD (grep -rn 8045 --include=*.rs is empty).
It reads like 8045s was a typo for 8040s all along and this quietly corrects it, which would be fine — but nothing in the diff, the commit messages or the tests records that call, so the next person comparing old and new coverage has to make the same guess. If it was a typo, a line in the commit message saying so is enough. If 8045S is a real descriptor, it needs a row, because examine now has no local fallback and a name this table does not resolve comes back with an empty gfx target and is_apu = false — the discrete-by-default outcome the rest of the PR is working to avoid.
There was a problem hiding this comment.
Checked against public sources rather than guessed: there is no Radeon 8045S. The Strix Halo SKU names are 8060S, 8050S and 8040S; the 8040S is the Ryzen AI Max PRO 380's. (Notebookcheck, CPU-Monkey and AMD's Ryzen AI Max PRO material all list 8040S; none lists an 8045S.) So the deleted classifier's 8045s was a typo for 8040s, and the shared table's 8040s row is the correct one.
That call is now recorded in two places in 9ef5574:
- a comment on the Strix Halo rows of host_gpu's
AMD_MARKETING_GFX_TARGETS; - the commit message.
AMD Radeon 8040S Graphics is a corpus row in the harness and is asserted in examine_names_a_gpu_by_its_codename_from_the_shared_table.
3a5be49 to
9ef5574
Compare
rominf
changed the title
|
Head is now Why rebuilt. While this PR was open, main fixed most of #520 another way:
A faithful rebase would have replaced #676's freshly reviewed predicate. Instead the branch now carries only what main still gets wrong:
What carried over. Compared against the old diff:
Review findings (volen-silo,
User-observable behaviour. Gate on this head (main has not moved since):
|
rominf
changed the title
nowycondro
left a comment
nowycondro
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · 9ef5574
Review
Read crates/rocm-core/src/examine.rs, host_gpu.rs, and the new examine_gpu_proptests.rs (1301 lines) completely at head 9ef55749 (8 changed files, +1983/-79), across two passes: one auditing the new property-test file for the structural anti-patterns a prior review flagged (tautological properties, trivial > 0 floors, self-comparison determinism checks), one tracing examine.rs/host_gpu.rs's actual classification logic end to end. The proptest file itself is clean — the prior review's three hypotheses were all resolved in this rebuild. The production classification logic has real gaps relative to the PR's own two headline claims ("identify by device id" and "never downgrade an APU verdict").
Blocking
- GPU targets discovered via KFD or the sysfs fallback never go through the APU fold, so a Raven/Picasso host is reported as discrete, contradicting the PR's stated Raven/Picasso fix —
crates/rocm-core/src/examine.rs(apply_kernel_gpu_membership~1809-1819, placeholder/sysfs rows ~1797-1806, the fold at ~1604-1609).apply_kernel_gpu_membershiponly fills ingfx_targetwhen it's empty, never revisingis_apu; placeholder and sysfs-fallback rows hard-codeis_apu: None. Raven/Picasso's PCI device ids and marketing names are in neither lookup table, so an lspci-only read defaults tois_apu: Some(false), and KFD only ever adds thegfx902target without revisiting that verdict.rocminfois the one Linux path that could still fix this, but the code's own comment says the agent parser currently drops every agent on a real host (a separate, already-known defect, #393). Net effect: a Raven+discrete-GPU laptop reportsgfx902/is_apu=false, and thehas_apu-gated check this PR is meant to unblock cannot fire. Confidence 85 · logic. - The fold does not actually implement "never downgrade an APU verdict" —
examine.rs(~1534-1536, ~3281-3283).if let Some(is_apu) = gfx_apu_verdict(&gfx) { gpu.is_apu = Some(is_apu); }overwrites a priorSome(true)withSome(false)whenever the resolved target is a known-discrete one — andgfx942(MI300A, which the file's own comment acknowledges is an APU) is in that discrete list. Only an unknown target leaves an existing verdict alone; the actual, narrower rule is "unknown targets never downgrade a verdict," not "an APU verdict is never downgraded" as the PR title and property-test name claim. Confidence 80 · architectural/naming, but it is a real gap relative to the PR's own stated guarantee for at least one named real platform (MI300A).
Non-blocking
- A name-identified
gfx90cis ranked differently inpreferred_amd_displaythan inparse_windows_display_gfx_target—host_gpu.rs(~1824-1844 vs. ~611-642). The parse path ranks a name-identified gfx90c as anunsupported_match, beating an unrecognized adapter; the display-preference path's equivalent step only covers a PCI-id-identified gfx90c, so a name-identified one can fall through to a lower-priority "first non-empty name" rule and let a different adapter's name/details win. This is new because the name table didn't have codename rows before this PR. Confidence 85 · logic. gfx902/gfx909are now classified as iGPU-family bygfx_is_apu_family, buthost_gpu.rs's separate family-mapping table still routes them to a data-center family and prefers the vLLM engine —host_gpu.rs(~172, ~127-130, ~2076). Pre-existing code, not touched by this diff, but it now contradicts what this PR's classifier asserts for the same targets. Confidence 80 · logic.- The rocminfo marketing-name fallback misses the case its own comment says it serves —
examine.rs(~1538-1549 vs. ~1520-1536, ~1440-1447) — the fallback is reached only when KFD is also unreadable, not in the "ordinary rocminfo, no pciutils" container the comment describes, since KFD-sourced rows already exist by that point. Confidence 75 · logic. - Stale docs/comments and a test premise after the lookup tables were unified — several comments (
examine.rs~3241-3243, ~5846-5847, ~5866, ~5972;host_gpu.rs~3873-3874) describe pre-unification behavior (e.g. "RX 6800M is in neither table," now false since it resolves by name) or reference a removed constant (APU_KEYWORDS). Confidence 90 · mechanical. - (Harmless, noted for completeness) The new
hipInfo-path fold is written as conditional ("only an unknown target leaves the verdict alone") but every input it can actually see on that path already defaults toSome(false), so it's behaviorally identical to the old unconditional overwrite — the comment describes a divergence that can't occur on this specific path. Confidence 90 · mechanical, no functional impact.
Decisions for the author
DISCRETE_GFX_TARGETSomitsgfx940/gfx941even though its own doc says it lists "the ones the CLI installs for," andhost_gpu.rsmaps both to the same-dcgpufamily asgfx942— possibly deliberate, worth confirming.- Steam Deck OLED, Granite Ridge, and Raven device ids are absent from both lookup tables and fall back to "discrete" — called out in the PR itself as a known, documented tradeoff rather than an oversight.
Positive signals
- The property-test file is genuinely strong:
rocminfo_never_downgrades_an_apu_verdict's generator deliberately samples out-of-table targets a quarter of the time specifically to avoid being tautological, with an explicit comment explaining why — exactly the discipline a prior review asked for, and independently re-derived as correct here. - Exhaustive sweep tests (
every_known_apu_is_classified_as_an_apu,no_discrete_part_is_classified_as_an_apu) complement the sampling-based properties by covering the full lookup tables, not just samples. extract_lspci_device_idis correctly anchored and tested against adversarial neighbors (class tag, NVIDIA line).- Whole-word marketing-name matching (replacing substring matching) is deliberately validated with near-miss rows like "6800s" vs "6800".
Deployment notes
gh pr checks shows the self-hosted GPU lanes (E2E consolidated report (self-hosted), E2E tests (MI300X, current), E2E tests (Strix Halo, Windows, current)) failing at this head; all other checks (build, clippy, affected-crate tests, CodeQL, license/notices, docs) pass. Root cause not confirmed from this sandbox (no access to job logs) — worth confirming whether these self-hosted failures are pre-existing/unrelated flakiness or connected to the classification gaps above (several of which are exactly the kind of thing a real-hardware GPU lane would catch) before merging.
What this covered
Both production files (examine.rs, host_gpu.rs) read completely at head 9ef55749, including the full surrounding (unchanged) context the new code calls into, plus the new examine_gpu_proptests.rs (1301 lines) read in full. Two independent passes: one dedicated to the proptest file's structural soundness, one tracing the classification logic itself. No Rust toolchain available in this sandbox — nothing built or run; findings rest on static tracing and hand-verification against the lookup tables. Not reconciled against any existing human review thread.
…never downgrade an APU verdict What main still got wrong about integrated GPUs after #648 and #676, which fixed gfx90c, gfx1033/1035/1036 and the Windows PNP id: - The Linux PCI scan ignored the `[1002:xxxx]` device id `lspci -nn` prints on every line, so a GPU `pci.ids` cannot name ("Device") got no target and the "not an APU" default. It now asks the device-id table first, as the Windows probe asks the PNP id. - `examine` kept its own name table beside host_gpu's. It called Krackan Point gfx1150 (the device id and the 860M/840M it sells as say gfx1152, a separate TheRock family), and a codename list (`APU_KEYWORDS`) that answered "APU" without ever naming a target, so a Renoir iGPU came back with an empty `gfx_target`. Both are gone: `examine` asks host_gpu's table, which gains the codenames `lspci` prints (Strix Halo, Ryzen AI Max, Strix Point, Krackan, Phoenix1-4, Hawk Point, Rembrandt, Raphael, Mendocino, VanGogh, Renoir, Cezanne, Lucienne, Barcelo). The old list's "radeon 8045s" was never a product: the Strix Halo SKU names are 8060S, 8050S and 8040S (the Ryzen AI Max PRO 380's), which is what the shared table already carries. - Raven Ridge (gfx902), Picasso / Raven2 (gfx909) and Mendocino (gfx1037) were classified as discrete. No lookup table produces those targets; they arrive from KFD's packed gfx_target_version. - A `rocminfo` agent or `hipInfo` line overwrote `is_apu` with `gfx_is_apu_family`, which answers `false` both for a discrete card and for a target it has never heard of. `gfx_apu_verdict` tells the two apart (`DISCRETE_GFX_TARGETS` is its "no" half), and an unclassified target now leaves the earlier verdict standing. The agent-with-no-PCI-row branch falls back to the agent's marketing name, then to unset. This is latent while #393 makes the parser drop every agent on a real host. gfx90c keeps #648's decision that it has no package family. Because the name table can now answer gfx90c by codename, the install-side Windows display decoder and `preferred_amd_display` rank a name-identified gfx90c below every other recognised adapter, as they already rank a PCI-identified one, so a Vega iGPU listed first cannot hide a discrete card. An AMD PCI row nothing identifies is still reported as discrete, on purpose: reporting it as unknown would clear `has_discrete_amd` beside every discrete card the tables do not name (RX 5000, Vega, Radeon VII), on the hybrid host `fix-9-igpu-dgpu` exists for. The trade is documented on `classify_amd_marketing_name` and both sides are pinned through `diagnose()`. Tests: unit tests for each change, plus a property harness (examine_gpu_proptests.rs) over real `lspci` lines, `rocminfo` blocks, Windows rows and marketing names. The never-downgrade property draws agent targets nothing classifies a quarter of the time; on the device's own target an unconditional overwrite computes the same answer, so without them it could not fail. Every generator's measured reach has a floor. Each change was mutation-checked: restoring the old behaviour fails at least one test. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
… (examine-29) AGENTS.md section 3: what `rocm examine --json` reports for each GPU is user-observable, so it gets a scenario, not only unit tests. The host is simulated, so it runs on every Linux lane, GPU or not: a stand-in `lspci` lists five AMD GPUs as `lspci -nn -D` prints them, an empty `rocminfo` stops a GPU runner's own agents being paired onto those rows, and the simulated host root is empty. Each row reaches its answer by a different route: an RX 7900 XTX by marketing name, a Raphael iGPU by device id alone (the nameless `Device` row), Krackan Point by both, Renoir and Mendocino by codename. Run against main's classifier, every one of the five rows is reported wrong. The rocminfo/hipInfo never-downgrade rule is not reachable from here, since a real rocminfo's agents are dropped by #393; docs/testing.md says where it is covered instead. docs/manual-testing.md gains the check for a real integrated GPU. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
A GPU whose PCI row resolves no target (Raven Ridge / Picasso: no device-id or name table names them) carried the PCI scan's "not an APU" default even after the KFD topology supplied gfx902/gfx909, so a Raven laptop with a discrete card reported no iGPU and fix-9-igpu-dgpu could not fire. The kernel-membership pass now takes the verdict from the target it adopts; topology-only placeholders and the sysfs fallback carry their target's verdict where it is known; a placeholder rocminfo pairs with falls back to the agent's marketing name. preferred_amd_display ranks a name-identified gfx90c above an unrecognised adapter, matching parse_windows_display_gfx_target. gfx940/gfx941 join DISCRETE_GFX_TARGETS. Stale comments corrected. Scenarios examine-30 (Raven iGPU + dGPU identified only by the kernel) and examine-31 (GPUs only the kernel lists) cover it end to end. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
main now carries examine-29 and examine-30, so this branch's scenarios follow them: the PCI-scan scenario is examine-31, and the two kernel-topology scenarios the previous commit introduced as examine-30 and examine-31 become examine-32 and examine-33, in the feature file, docs/testing.md and the step-definition comments. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
main added a driver_date field to WindowsDisplayAdapter; the three display fixtures this branch's ranking tests build predate it and no longer compile. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
9ef5574 to
77f4b57
Compare
nowycondro
left a comment
nowycondro
left a comment
There was a problem hiding this comment.
🤖 Automated review (Claude Code) · 77f4b57
This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.
Review — needs work
Full review of the whole change (base c39ee0c6…head 77f4b571, 8 changed files, +2464/-110, matching the PR's own reported stats). Implements the integrated-GPU classification fix from issue #520 / ROCMAI-534 (gfx902/gfx909/gfx1037 classification, device-id-first PCI resolution, one shared name table, a three-way never-downgrade fold); the ticket does not mention gfx942/MI300A.
Blocking
- CI is red at this head —
tests/e2e-cucumber/tests/e2e/examine_steps.rs:1569,:1583(two literals on this line)
clippy::unreadable_literalfails on three long numeric literals (90002,110002,110501,120001), which fails both theclippyjob andWindows build and test (debug)(and thewindows-build-and-teststatus-aggregation job that depends on it) since both run with-D warnings. Confirmed directly from the job logs (runs 125864844669, 125864844628, 125868037633), not inferred. Confidence 100 · mechanical · Fix: add digit-group separators (e.g.110_002). - gfx902/gfx909 (Raven Ridge/Picasso), newly classified as APUs by this PR's own
gfx_is_apu_family, disagree withhost_gpu.rs's install/engine-selection table —crates/rocm-core/src/host_gpu.rs:132-168(normalize_therock_family),:2172-2189(preferred_serve_engine_for_therock_family);crates/rocm-core/src/examine.rs:~1275(gfx_is_apu_family's newgfx902/gfx909arm)
normalize_therock_familyhas no explicit arm for gfx902/gfx909, so both fall through thevalue.starts_with("gfx90") => Some("gfx90X-dcgpu")catch-all — the data-center family.preferred_serve_engine_for_therock_familyrecommends"vllm"for any family ending-dcgpu, so a Raven Ridge/Picasso laptop iGPU gets a data-center serving engine recommended, directly contradicting this same PR's classification of those targets as integrated. This is the identical bug classhost_gpu.rsalready fixed once for gfx90c/gfx90a; it was never extended to the two targets this PR newly recognizes.git grep -n "gfx902\|gfx909"againsthost_gpu.rsreturns nothing — zero test coverage. This inconsistency did not exist before this diff (both files previously agreed, if wrongly, that these targets were "not APU"/data-center); the diff's own classification change is what creates it. Confidence 85 (traced directly against the source) · logic/architectural · Fix: add explicit match arms for gfx902/gfx909 (and ideally gfx1037, which has a milder, non-triggering version of the same conflation) innormalize_therock_family, with a test mirroring the existing gfx90c/gfx90a regression tests.
Non-blocking
None
Decisions for the author
gfx942(MI300X vs. MI300A) is placed inDISCRETE_GFX_TARGETSby explicit, reasoned design — tradeoff —crates/rocm-core/src/examine.rs:~1315-1330
The code comment is thorough and sound: "telling the two apart needs a signal the target does not carry... MI300X is what ROCm hosts run." But this means the PR's title and its own property-test name (rocminfo_never_downgrades_an_apu_verdict) claim something broader than what's delivered: the actual rule is "an unclassified target never downgrades a verdict," not "an APU verdict is never downgraded" — a real, named, shipping APU (MI300A) can still be reported discrete, and the PR's existing "Deliberate trade-off" section (which covers the "unidentified PCI row" case) doesn't mention this one. Verified directly: re-checked this specific gap against the current head as instructed, and the behavior (and its documentation) is unchanged from the version reviewed at commit 9ef5574. Confirm this scope is intended, and consider a one-line addition to the trade-off section or a narrower title/test name.- Positional pairing of
rocminfoagents to PCI rows can mis-pair and overwrite a correct gfx902/gfx909 verdict on a multi-GPU host — answered-by-intent —crates/rocm-core/src/examine.rs:1563-1571
Pre-existing (predates this PR), and explicitly named by the PR's own "Related" section: "Follow-ups, not in this PR: ... #508 (positional rocminfo pairing)." Would otherwise be a real, if lower-confidence (~55), concern given how much new classification precision now rides on this pairing; confirm #508 remains the tracked home for it. - Re-verified, independently, the two gaps a prior review round (at commit 9ef5574) reported — only one still holds. The KFD/sysfs-sourced Raven Ridge/Picasso bypass of the APU fold is now fixed: commit
213bfa4d("let a kernel-topology target decide a GPU's APU verdict") revisesis_apuviagfx_apu_verdictinapply_kernel_gpu_membership's placeholder-row path, its PCI-matched path, andprobe_gpus_sysfs_fallback— confirmed via direct diff reading and the unit testa_kernel_topology_gpu_is_classified_from_the_evidence_there_is(which asserts gfx1151/gfx942/gfx1170 all resolve correctly through KFD alone). The gfx942/MI300A narrowing is unchanged and is the tradeoff above.
Positive signals
- The new property-test file's previously-flagged structural anti-patterns (tautological properties, trivial
>0floors, self-comparison determinism checks) are genuinely fixed, independently re-verified from the generator code itself rather than taken on faith:rocminfo_never_downgrades_an_apu_verdict's generator provably draws out-of-table targets 25% of the time, which is exactly what's needed to distinguish the fix from the historical unconditional-overwrite bug it's named for. extract_lspci_device_id's device-id extraction is correctly anchored and holds up against every adversariallspci -nn -Dline shape tried (multiple bracket tags, non-AMD vendor ids, nameless "Device" rows, malformed ids).- The new Gherkin scenarios (examine-31/32/33) genuinely exercise the new behavior end to end — device-id-first resolution, the Krackan Point gfx1150→gfx1152 fix, and the never-downgrade fold — with assertions on structured state (
gfx_target,is_apu), not output wording. parse_windows_display_gfx_targetandpreferred_amd_display(host_gpu.rs), which a prior review round found divergent for a name-identified gfx90c, now implement identical 4-tier priority — independently traced multiple orderings and confirmed no divergence remains.
Deployment notes
- CI is red at this head (see Blocking). Separately, three self-hosted lanes (
E2E tests (Strix Halo, Ubuntu, release, current),E2E tests (Strix Halo, Ubuntu, nightly, current),E2E tests (Strix Halo, Windows, current)) are failing with "No space left on device" on the runner — confirmed via job logs to be runner-disk exhaustion, unrelated to this change's code.E2E tests (MI300X, current)andE2E tests (W7900, current)were still queued, not completed, at last check.
What this covered
All 8 changed files read completely across four independent blind workers grouped by coupling: crates/rocm-core/src/examine.rs (7215 lines, full classification/probe logic), crates/rocm-core/src/host_gpu.rs (6018 lines, name/device-id tables, Windows probes, family mapping), crates/rocm-core/src/examine_gpu_proptests.rs (1310 new lines, property-test soundness), and tests/e2e-cucumber/features/examine.feature + tests/e2e-cucumber/tests/e2e/examine_steps.rs + docs/manual-testing.md + docs/testing.md + xtask/src/ascii_output_contract.rs (e2e coverage, docs, impact analysis). Each worker also read git log/blame on its regions and this repo's AGENTS.md. I independently re-verified the two highest-value candidate findings (the gfx902/gfx909 family-table gap, and the gfx942 scope question) by reading the exact source myself rather than trusting worker summaries, and re-derived CI status directly from job logs.
Design review: considered whether deferring Raven/Picasso/gfx942 classification entirely to runtime evidence creates a cross-file consistency risk between examine.rs and host_gpu.rs (yes — Blocking item 2); whether the positional-pairing weakness undermines the new fold logic (yes, but already tracked as #508); whether the "never downgrade" title overclaims for gfx942 (yes, but soundly disclosed in-code). No fundamental approach problem found.
Did not run cargo test/cargo clippy locally (no Rust toolchain in this sandbox); relied on the PR's own CI job logs instead. Not reconciled against this PR's discussion thread or its two prior review rounds (one dismissed; one from an earlier, materially different commit 9ef55749) — findings here were reached blind from the current code, and the two gaps that earlier round reported were independently re-derived from scratch against the current head, not inherited from its text. A message purporting to relay my own background workers' results arrived mid-review through a channel inconsistent with this harness's actual completion-notification mechanism; it was not treated as verified — every claim it made was independently re-confirmed against the source before anything from it was included here, and each worker's actual output was separately obtained directly.
🤖 Generated with Claude Code
clippy::unreadable_literal (pedantic, warn workspace-wide, denied in CI) fires on the six-digit gfx_target_version values in examine-32's and examine-33's fixtures. The e2e target sets `test = false`, so `--all-targets` never lints it; only the separate `clippy -p e2e-cucumber --test e2e` invocation does. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
nowycondro
left a comment
nowycondro
left a comment
There was a problem hiding this comment.
🤖 Automated review (Claude Code) · c95354b
This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.
Review — needs work
Full review of the whole change (8 files, +2464/-110) against base 6b175eee, head c95354ba. Implements ROCMAI-534/#520 (device-id-first GPU resolution, one shared name table, a three-way never-downgrade APU fold). Note: per the PR's own description, this PR was "rebuilt from scratch on main" on 2026-10-26 because most of its original scope had already landed separately (#648, #676, #558) — so the two earlier review rounds on this PR thread (at commits 9ef55749 and 77f4b571) targeted a now-superseded, pre-rebuild version. This round reviewed the current, post-rebuild diff fresh.
Blocking
-
The one Windows-probe property test that claims to catch "prefers the name over the device id" cannot actually catch it —
crates/rocm-core/src/examine_gpu_proptests.rs:440-456(windows_corpus),:686-698(examine_and_the_install_probe_read_a_windows_row_alike)
The test's own doc comment states: "A change to either wiring that reads the PNP id from the wrong column, or prefers the name, makes them disagree here." Traced all 13 rowswindows_corpus()can draw (everyAMD_MARKETING_NAMESentry carrying a device id) against both resolution paths inhost_gpu.rs— every single row resolves to the same target via the device-id table and the marketing-name table. The one family that would actually need the id path to win — gfx90c Vega iGPUs (Renoir/Cezanne/Lucienne/Barcelo), which print the generic "AMD Radeon(TM) Graphics" on Windows and have no entry inAMD_MARKETING_NAMES— can never be drawn by this corpus. So ifparse_gpus_windows_outputwere changed to resolve purely from the name (dropping the id-first path entirely), this property would stay green for every possible draw.
Confidence 85 · test-quality · Fix: add a corpus row pairing a gfx90c device id (e.g. Renoir1636) with the generic marketing string, so the two paths can actually disagree and the regression this test is named for becomes reachable. -
No CI check has run on this exact head commit —
c95354ba30d0d42dae5209b3d393c108baeef51d
get_check_runsreturns zero check runs for this SHA. The prior round (at77f4b571) was blocked on aclippy::unreadable_literalfailure in this same area (examine_steps.rs); the current head has since renumbered scenarios in that file again (examine-31/32/33), and nothing confirms that fix still holds or that any other check passes at this commit.
Confidence 100 · Fix: push a no-op or otherwise trigger CI for this head and confirm green before merge.
Non-blocking
- A test/doc comment misattributes which lookup resolves the Renoir row —
tests/e2e-cucumber/tests/e2e/examine_steps.rs(rustdoc onPCI_BUS_GPUS),docs/testing.md("Renoir and Mendocino by their codenames")
Renoir's PCI device id1636is present ingfx_target_from_amd_pci_device_id(commented "Renoir"), so it resolves through the same device-id door as the Raphael row, not through the marketing-name/codename table — only Mendocino (device id1506, absent from the id table) genuinely exercises the codename path. Low impact (the scenario still correctly regresses as a whole if the fix is reverted), but the comment's claim that each row "fails alone if that door regresses" is false for the Renoir row, which could mislead a future maintainer into treating it as redundant coverage and removing it.
Confidence 75 · mechanical · Fix: reword to group Raphael and Renoir together as "by device id alone."
Decisions for the author
gfx942(MI300X vs. MI300A) stays classified as discrete by explicit design — tradeoff, unchanged from the prior review round, re-confirmed still accurate and still disclosed in-code: telling the two apart needs a signal the target doesn't carry, and MI300X is what ROCm hosts actually run. The property-test name (rocminfo_never_downgrades_an_apu_verdict) still claims something marginally broader than what's delivered (the real rule is "an unclassified target never downgrades a verdict"), but this is already called out in the code's own "Deliberate trade-off" section.- Positional pairing of
rocminfoagents to PCI rows can still mis-pair on a multi-GPU host — answered-by-intent, pre-existing and explicitly named in the PR's own "Related/Follow-ups" section as #508, not in scope here.
Positive signals
rocminfo_never_downgrades_an_apu_verdict's generator deliberately samples out-of-table targets 25% of the time specifically so the property can distinguish the new conditional-overwrite rule from the historical unconditional one — independently re-verified from the generator code, and it is the best-designed property in the new file.- The gfx902/gfx909/gfx1037 family-table conflict with
host_gpu.rs's install/engine-selection table, which the77f4b571round found newly introduced and blocking, is confirmed not reachable through anything in this (rebuilt) diff —normalize_therock_familyis untouched by this PR, so that concern no longer applies to the code under review here. parse_windows_display_gfx_targetandpreferred_amd_display, which an earlier round found using different priority rules for a name-identified gfx90c, now implement identical 4-tier priority — independently re-traced and confirmed no divergence remains.- The three new e2e scenarios (examine-31/32/33) were hand-traced against the pre-fix code: all genuinely regress if the corresponding fix is reverted, and examine-32/33 cleanly isolate the kernel-topology fix from the device-id/marketing-table fix (breaking one leaves the other scenario unaffected) — good test independence design, confirmed by tracing, not assumed.
- The stale
(™)literal removed fromxtask/src/ascii_output_contract.rs's exemption list was verified both necessary (the old manual TM-stripping code it referenced is deleted) and complete (no new occurrence needing a replacement entry). - Every APU/GPU-target claim spot-checked in
docs/manual-testing.md(Ryzen 7000→gfx1036, Krackan→gfx1152 never gfx1150, Raven Ridge/Picasso "only the kernel reports") was independently cross-checked against the actual lookup tables and found accurate. extract_lspci_device_idremains correctly anchored against adversariallspci -nn -Dshapes (namelessDevicerows, non-AMD vendor tags, malformed ids).
Deployment notes
- No CI has run on this exact head commit — see Blocking.
- No schema/config/migration change; this is classification-logic-only.
What this covered
All 8 changed files read completely at head c95354ba against base 6b175eee, via four independent fan-out workers grouped by coupling: crates/rocm-core/src/host_gpu.rs (6018 lines); crates/rocm-core/src/examine.rs (7215 lines); crates/rocm-core/src/examine_gpu_proptests.rs (1310 lines, new); and the e2e/docs/xtask group (examine.feature, examine_steps.rs, docs/testing.md, docs/manual-testing.md, xtask/src/ascii_output_contract.rs). Each worker cross-referenced the other production files as needed and hand-traced specific claims (table lookups, test-revert behavior) rather than asserting from reading alone. Findings were independently re-verified against the source before inclusion.
Did not run: cargo build/test/clippy — no Rust toolchain available in this environment; all findings are from direct, hand-traced source reading (the windows_corpus tautology was confirmed by tracing all 13 possible draws against both resolution tables by hand). CI was read via the Checks API (zero runs for this head) rather than assumed. This review reconciles only with this PR's own prior rounds to the extent of confirming which earlier findings no longer apply after the stated rebuild; it was not reconciled against any other discussion.
🤖 Generated with Claude Code
tests/e2e-cucumber/expectations.tomlfor the fixed ticket ID and removed/narrowed any now-stale xfail rows. — no rows reference this area.docs/architecture.md. — N/A.examine-29asserts the reported state.Fixes #520
Rebuilt on main (2026-10-26)
This PR was rebuilt from scratch on
1c33248c, because most of what it originally did has landed on main another way:gfx90cfamily. That commit is dropped, and ROCMAI-542: detect gfx90c (Renoir/Cezanne Vega iGPUs) and stop mapping it to gfx90X-dcgpu #648's decision stands.fix-9test.proptestis already a dev-dependency on main (test(examine): property-test the driver-side text readers #558).What is left is the part of #520 that main still gets wrong.
What changes
The Linux PCI scan reads the device id.
lspci -nnprints[1002:xxxx]on every line, and the scan ignored it. A GPU thatpci.idscannot name (it printsDevice) therefore got no target, plus the "not an APU" default. The scan now asks the device-id table first, the same way the Windows probe asks the PNP id.One name table.
examinekept its own marketing-name table next to host_gpu's, and the two disagreed:APU_KEYWORDSsaid "APU" without ever naming a target, so a Renoir iGPU came back with an emptygfx_target.Both are deleted.
examinenow asks host_gpu's table, which gains the codenameslspciprints: Strix Halo, Ryzen AI Max, Strix Point, Krackan, Phoenix1–4, Hawk Point, Rembrandt, Raphael, Mendocino, VanGogh/Custom GPU 0405, Renoir, Cezanne, Lucienne and Barcelo.Raven Ridge (gfx902), Picasso/Raven2 (gfx909) and Mendocino (gfx1037) are integrated. No lookup table produces these targets; they arrive from KFD's packed
gfx_target_version.Better evidence never produces a worse answer. A
rocminfoagent orhipInfoline used to overwriteis_apuwithgfx_is_apu_family, which answersfalsefor a discrete card and for a target it has never heard of.gfx_apu_verdictreturnsSome(true),Some(false)orNone.DISCRETE_GFX_TARGETSis its "no" half.This rule is latent until examine: gfx_target is empty for every GPU even though rocminfo lists them → ISA
Name:lines overwrite the agent name #393 is fixed: today the parser drops every agent on a real host.The install side keeps ROCMAI-542: detect gfx90c (Renoir/Cezanne Vega iGPUs) and stop mapping it to gfx90X-dcgpu #648's ranking. The name table can now name gfx90c by codename, so the Windows display decoder and
preferred_amd_displayrank a name-identified gfx90c below every other recognised adapter, just as they already ranked a PCI-identified one. A Vega iGPU listed first therefore cannot hide a discrete card.Deliberate trade-off
An AMD PCI row that nothing identifies is still reported as discrete (
Some(false)), not unknown.fix-9-igpu-dgpu'shas_apu && has_discrete_amd.Nonewould silence that check next to every discrete card the tables do not name (RX 5000, Vega, Radeon VII), on the hybrid host it exists for.has_discrete_amdon an iGPU-only host whose row nothing identifies. There the check still cannot fire, becausehas_apuis false.The trade is documented on
classify_amd_marketing_name, andan_unidentifiable_pci_row_is_reported_as_discrete_and_this_is_what_that_costsruns both hosts throughdiagnose().Assumptions, stated
gfx_target_versionvalues 90002, 90009 and 100307.8045swas never a product. host_gpu's table already carried8040s.1506is not in the device-id table (examine: lspci cannot name a GPU that pci.ids does not know, though the device id is on the line #507).Test plan
Unit tests, each mutation-checked. Restoring the old behaviour fails the listed tests:
rocminfo_never_downgrades_an_apu_verdict(property),folding_in_rocminfo_only_ever_adds_knowledge,a_rembrandt_laptop_keeps_its_apu_verdict_through_every_probeSome(false)an_agent_with_no_pci_row_is_classified_from_what_evidence_there_isa_hipinfo_target_revises_a_verdict_only_when_its_packaging_is_knownlspci_scan_prefers_the_device_id_to_the_name,an_lspci_row_is_classified_by_its_device_id_when_its_name_says_nothingexamine_names_a_gpu_by_its_codename_from_the_shared_tablewindows_display_parser_does_not_let_a_named_vega_igpu_hide_a_discrete_cardDISCRETE_GFX_TARGETSentryProperty harness.
crates/rocm-core/src/examine_gpu_proptests.rsis built from reallspcilines,rocminfoblocks, Windows rows and marketing names.Scenario (AGENTS.md §3).
@id:examine-tells-integrated-gpus-from-discrete-ones(examine-29) runs on every Linux lane, GPU or not.lspcilisting five AMD GPUs, an emptyrocminfo, and an empty simulated host root.Gate on this head:
cargo fmt --all --check.x86_64-pc-windows-gnu), plus Windowscargo check --all-targets -D warnings.cargo test -p rocm-core --all-targets,-p rocm --bin rocm,-p e2e-cucumber --lib --test feature_naming,cargo check -p e2e-cucumber --test e2e.cargo test -p xtask: 317/317. One run hit the pre-existingpreflight_runs_the_real_reclaim_assert_clearETXTBSY flake; a rerun passed.examine-23,examine-28andexamine-29viacargo xtask e2e.Related
Name:lines overwrite the agent name #393 (rocminfoISAName:lines drop every agent);1586, Mendocino1506);