[marifamd]

Motivation

amdsmi_get_gpu_asic_info() reports rev_id, the PCI config-space revision, but nothing about the silicon itself. Consumers that need the ASIC revision (validation and qualification tooling, telemetry exporters tagging metrics per revision) had no in-band source for it.

The AMDGPU_INFO_DEV_INFO DRM query this function already issues carries two more revision values, and they answer different questions:

Field	amdgpu source	Meaning
rev_id	pci_rev	PCI config-space revision (already reported)
chip_rev_id	chip_rev	Internal chip revision, the silicon stepping. The kernel UAPI annotates it "Internal chip revision: A0, A1, etc."
external_rev_id	external_rev	Family-scoped revision. The same value recurs across unrelated ASIC families, so it must be paired with device_id

They are genuinely distinct on real hardware. On a Navi 21 the same part reports rev_id 0xc3, chip_rev_id 0x00, external_rev_id 0x28, so neither existing field can stand in for the others.

Technical Details

Both values are read from the dev_info structure the function already fetches, so no additional ioctl or syscall is introduced.

ABI: the two uint32_t fields come out of the amdsmi_asic_info_t reserved tail. reserved shrinks from 17 to 15 entries and moves; the struct stays 896 bytes and no pre-existing field offset changes.

Field	Offset
flags	816
physical_acc_id (existing)	824
chip_rev_id (new)	828
external_rev_id (new)	832
reserved[15]	836

Verified three ways: compiler offsetof, the regenerated ctypes wrapper, and the Rust offset_of! assertions. Both wrappers were regenerated with tools/update_wrapper.sh and tools/update_rust_wrapper.sh.

Behavior change to an existing field. Calling this out explicitly so it is not missed in review: rev_id was assigned from dev_info before the DRM query populated it, so every early-return path overwrote the not-supported value with a zero that reads like a real revision. rev_id now reports N/A on those paths instead of 0x0. Callers that check the return status are unaffected, since those paths already return a non-success status.

To support that, amdsmi_asic_info_t is now reset through a single init_asic_info_defaults() shared by the DRM and WSL backends. This is required by the feature, not incidental: the WSL backend previously memset the struct to zero, which would have reported both new fields as a plausible 0x00 rather than N/A.

The Python binding seeds both new fields with the not-supported value before the call, so a newer wrapper running against an older libamd_smi.so (supported via the wheel's system-library fallback) reports N/A instead of the ctypes zero.

The three commits are split to be reviewed in order: the shared initializer and rev_id fix, then the two new fields across the cascade, then the tests.

Issue Tracking

JIRA ID: ROCM-28626

Test Plan

• Unit, hardware-free: pin struct size and both new offsets, and the reporting contract through the public Python entry point against a stubbed library (value passthrough, two-digit padding, 0xFFFFFFFF to N/A, 0x0 staying a real value, and a library that predates the fields).
• Unit, C++: fill amdsmi_asic_info_t with garbage, call init_asic_info_defaults(), assert every field lands on its not-supported value.
• Functional, hardware: resolve each processor's render node, issue AMDGPU_INFO_DEV_INFO directly, and assert all three revisions match what amdsmi_get_gpu_asic_info() reports.
• CLI: assert both keys reach --json, and that the --csv column values match the JSON values.
• Build both the default configuration and -DENABLE_WSL_BACKEND=ON.

Test Result

All green on a two-GPU Navi system (Navi 21 and Navi 32).

• Build: 0 errors in both configurations, no new warnings on touched lines.
• Tests: 15 Python tests and 2 gtests pass. pre-commit passes on every changed file.
• Each of the three commits builds independently.
• Values match the DRM ioctl exactly:

Node	ASIC	rev_id	chip_rev_id	external_rev_id	vs ioctl
renderD128	Navi 21	0xc3	0x00	0x28	match
renderD129	Navi 32	0xc8	0x00	0x20	match

Previously measured on other silicon: MI210 chip_rev 0x01 / external_rev 0x3d, MI300A 0x01 / 0x47, MI350 0x00 / 0x50.

The swap-detection test was validated by injecting a real chip_rev_id/external_rev_id swap into the interface: the new assertion failed as intended while the other tests passed, confirming it catches a defect the earlier equal-value test could not.

Submission Checklist

• Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.

[therock-pr-bot]

✅ All Policy Checks Passed

Check	Status	Details
📝 PR Description	✅ Pass	—
⛔ Forbidden Files	✅ Pass	—
🧪 Unit Test	✅ Pass	—
🚫 Draft PR	🔜 To Be Enabled	—
🚩 Feature Flag	🔜 To Be Enabled	—
📊 Code Coverage	🔜 To Be Enabled	—

🎉 All policy checks passed!

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

[dmitrii-galantsev]

claude:

Approving with the WSL fix pushed on top.

ABI checks out: silicon_rev_id is appended at the tail of amdsmi_asic_info_t (offset 824) with reserved[18] shrunk to [17], so the struct is still 896 bytes and no pre-existing field moved. The Rust offset_of! static asserts are updated to 824/828 consistently, the Python struct_amdsmi_asic_info_t matches, and static.py:305 iterates every key of the returned dict, so the field reaches amd-smi static --asic in human-readable, JSON and CSV.

One caveat: no amdsmi build actually completed in this PR's CI — the failures were cancelled builds and unrelated projects. This PR touches C++, so please get a real green amdsmi build before merging.

[dmitrii-galantsev]

claude:

Rebased onto origin/develop (3bbe12e068e) — the branch was 332 commits behind and CONFLICTING. Re-approving after the force-push dismissed the previous approval.

Conflicts resolved: only CHANGELOG.md. develop opened a new ## amd_smi_lib for ROCm 10.1.0 section (#10391) after this branch was cut, so the entry was moved out of the now-closed 10.0.0 section into 10.1.0 / ### Added. No content change to the entry itself. Also converted the new test file to the two-line SPDX header now enforced by the amdsmi-license-headers pre-commit hook.

ABI re-verified on the rebased tree (not assumed to have survived) — develop has not added any field to amdsmi_asic_info_t, so the layout is unchanged from the original claim. Confirmed by compiling a TU of static_asserts against the rebased amdsmi.h:

• sizeof(amdsmi_asic_info_t) = 896 (unchanged), alignof = 8
• offsetof(silicon_rev_id) = 824 (the slot reserved used to start at)
• offsetof(reserved) = 828, reserved shrunk [18] -> [17] (68 bytes)
• every pre-existing offset unchanged: market_name 0, vendor_id 256, vendor_name 260, subvendor_id 516, device_id 520, rev_id 528, asic_serial 532, oam_id 788, num_of_compute_units 792, target_graphics_version 800, subsystem_id 808, flags 816

All three bindings agree on 896 / 824 / 828: the C header, the Rust offset_of! static asserts in rust-interface/src/amdsmi_wrapper.rs, and the Python struct_amdsmi_asic_info_t ctypes layout (evaluated directly out of the generated wrapper, not transcribed).

Other checks: git range-diff against the pre-rebase tip shows only the two intended deltas above — the WSL commit is byte-identical. g++ -fsyntax-only -std=c++17 -DENABLE_WSL_BACKEND clean on amd_smi_wsl_device.cc; clang-format --dry-run --Werror clean on all touched C++; ruff check / ruff format --check / py_compile clean on the test; pre-commit clean on all changed files (the one hook failure is tests/python/unit/gpu/test_topology.py, pre-existing on develop and untouched here).

Caveat unchanged from my earlier review: this touches C++ and I have only syntax-checked it. It needs a real green amdsmi build and a run on hardware before merge — in particular that dev_info.external_rev is what is actually wanted, and that the N/A path reports correctly on WSL.

[systems-assistant]

TheRock Submodule Bump Activity

Newest first

• 2026-09-03 — Included in TheRock via ROCm/TheRock#7862.