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.
…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>
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.
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);