Skip to content

fix(amdsmi): stop dropping single-instance xGMI data counters - #12144

Merged
marifamd merged 2 commits into
developfrom
users/adam360x/ROCM-31364
Oct 6, 2026
Merged

marifamd merged 2 commits into
developfrom
users/adam360x/ROCM-31364

Conversation

@adam360x

@adam360x adam360x commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

On MI450 the GPU-CPU link is xGMI, and amdgpu publishes that link's read/write traffic in gpu_metrics v1.9 as a single-instance counter. amd-smi skipped single-instance rows when copying into per-link arrays, so xgmi_read_data_acc / xgmi_write_data_acc and the link read / write reported N/A. (The PCIe link fields are N/A by design on MI450: there is no PCIe host link.)

Technical Details

Dynamic metrics mapper (rocm_smi/src/rocm_smi_gpu_metrics.cc, assign_vector):

  • Store a single-instance row as element 0 of its per-link / per-instance array instead of skipping it
  • Accept rows of any unsigned width; keep an all-ones "not reported" value, and values that do not fit the field, unset

Tests (tests/amd_smi_test/unit/gpu/dynamic_metrics_test.cc):

  • Single-instance, per-link, narrower-type, narrower-unset, and out-of-range rows

Changelog (CHANGELOG.md):

  • Resolved Issues entry for the MI450 xGMI counters

No API or ABI change; wrapper, interface, and CLI are untouched.

Issue Tracking

JIRA ID: ROCM-31364

Test Plan

  • amdsmitst --gtest_filter='*Unit.*' (hardware-free)
  • Mutation checks: the previous mapper and a naive cast each fail the new tests
  • MI455X (gfx1250, gpu_metrics v1.9): amdsmi_get_gpu_metrics_info() and amdsmi_get_link_metrics(), develop vs this branch via AMDSMI_LIB_OVERRIDE

Test Result

  • 158/158 unit tests pass
  • MI455X: xGMI read/write[0] N/A -> 578484 KB / 0, matching the raw driver table; other array metrics unchanged
  • amd-smi xgmi still shows read/write as N/A: it lists only peer-GPU hive links, and this system has none (num_links=0)
  • Separately, xgmi_link_speed reads 32768 on MI450 because the driver copies the Q10 firmware value unrounded; reported to the driver team

Submission Checklist

@adam360x
adam360x requested review from a team and a lite review from Copilot September 23, 2026 20:03
@therock-pr-bot

therock-pr-bot Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

✅ 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?

@therock-pr-bot

Copy link
Copy Markdown

🚫 Please fix the failed policies before requesting reviews.

The following policy checks failed:

  • ❌ PR Description

The Not ready to Review label has been added to this PR.
Once all policies pass, the label will be removed automatically.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Scalar values with integral types differing from the exact target type are not yet handled.

Review effort: Lite
Findings: None

What changed in this PR

Updates AMD SMI dynamic GPU metrics mapping so single-instance xGMI counters populate link 0.

Changes:

  • Adds scalar-to-array mapping.
  • Adds xGMI mapping tests.
  • Documents the MI450 fix.
File Summary
projects/​amdsmi/​tests/​amd_smi_test/​unit/​gpu/​dynamic_metrics_test.cc Adds single-instance and per-link xGMI tests.
projects/​amdsmi/​rocm_smi/​src/​rocm_smi_gpu_metrics.cc Maps scalar rows to array element 0.
projects/​amdsmi/​CHANGELOG.md Documents the resolved issue.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@adam360x

Copy link
Copy Markdown
Contributor Author

Re: the Copilot review note, "Scalar values with integral types differing from the exact target type are not yet handled". Agreed, addressed in 1f31955.

The single-instance path matched only a scalar of the field's exact type, so a driver that declares the counter narrower (e.g. U32 into a uint64_t field) would still have been dropped.

  • Single-instance rows of any unsigned width now map to element 0.
  • A plain static_cast would reintroduce a sentinel bug: a narrower all-ones "not reported" value (e.g. UINT32_MAX) would widen into a real-looking 4294967295, and a wider value would truncate. Both are now kept unset.
  • Signed rows are still skipped, as before: none of these per-instance fields is signed, and a negative value has no valid unsigned mapping.
  • New tests: narrower-type, narrower-unset, and out-of-range. The previous mapper fails the first; a naive cast fails the other two (4294967295, 4464).
  • 158/158 unit tests pass. Re-verified on MI455X: xGMI read/write[0] go from N/A to 578484 KB / 0, and the other array metrics are unchanged.

@charis-poag-amd charis-poag-amd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre-approving, did have one question though. See inline.

- Store a single-instance dynamic gpu_metrics row as element 0 of its
  per-link array; the mapper accepted only multi-value rows, so the
  one xGMI read/write data counter MI450 reports for its GPU-CPU link
  was dropped and every link read as unavailable
- Add unit tests for single-instance and per-link xGMI rows

Signed-off-by: Adam360x <Adam.pryor@amd.com>
- Accept a single-instance dynamic gpu_metrics row of any unsigned
  width, not only the field's exact type, so a driver that declares a
  narrower counter still fills element 0
- Keep an all-ones "not reported" value, and values that do not fit
  the field, unset instead of widening or truncating them into
  plausible readings
- Cover narrower, narrower-unset, and out-of-range rows in unit tests

Signed-off-by: Adam360x <Adam.pryor@amd.com>
@adam360x
adam360x force-pushed the users/adam360x/ROCM-31364 branch from 67e19fd to 3bd95f2 Compare October 6, 2026 08:40
@marifamd
marifamd merged commit f1e1a4b into develop Oct 6, 2026
227 of 232 checks passed
@marifamd
marifamd deleted the users/adam360x/ROCM-31364 branch October 6, 2026 15:40
@systems-assistant

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

marifamd added a commit that referenced this pull request Oct 12, 2026
## Motivation

ROCm 10.1 has branched to `release/therock-10.1`. This opens the 10.2
development cycle: it bumps the library minor version to 27.2.0 and
fills the `10.2.0` changelog section. While opening it, the `10.1.0`
section on `develop` was audited against the release branch, and entries
that will not ship in 10.1 were moved to `10.2.0`.

## Technical Details

**Version** (`include/amd_smi/amdsmi.h`,
`rust-interface/src/amdsmi_wrapper.rs`):
- Bump `AMDSMI_LIB_VERSION_MINOR` `1 -> 2` (27.2.0). SONAME stays
`libamd_smi.so.27` (`SOVERSION` is `MAJOR` only).
- Regenerate the Rust binding constant to match. The Python wrapper and
Go shim carry no minor-version constant.

**Changelog** (`CHANGELOG.md`):
- Extend the existing `## amd_smi_lib for ROCm 10.2.0` section.
- Move the `10.1.0` entries whose source PR is not on
`release/therock-10.1`, and add entries for three develop-only PRs whose
fixes merged without one:

| Action | Source PR | Entries |
|--------|-----------|---------|
| Move `10.1.0` → `10.2.0` | #8624 | CLI exit-code rework: 5 Changed, 1
Removed, 2 Resolved Issues |
| Move `10.1.0` → `10.2.0` | #11494 | `AMDSMI_VRAM_TYPE_HBM4` (Added) |
| Move `10.1.0` → `10.2.0` | #11066 | fwupd UMA carveout (Changed) |
| Move `10.1.0` → `10.2.0` | #11471 | NPM node power limit: 1 Added, 2
Changed |
| Move `10.1.0` → `10.2.0` | #10678 | Version-absent `amd-smi metric`
fields omitted from human output (Changed; was under Removed, but it
changes output) |
| Move `10.1.0` → `10.2.0` | #12144 | xGMI data counters reading as
unavailable on MI450 (Resolved Issues) |
| Move `10.1.0` → `10.2.0` | #12804 | NPM balancing mode get/set (Added)
|
| New in `10.2.0` | #11474 | Topology APIs returned uninitialized NUMA
data (Resolved Issues) |
| New in `10.2.0` | #11045 | `amdsmi_get_gpu_fabric_info()` rejected
partial UALink sysfs data (Resolved Issues) |
| New in `10.2.0` | #12999 | NULL output pointers, CPER paging after a
large record, `amd-smi static --clock` traceback (3 Resolved Issues) |

- Drop the `### Removed` heading left empty in `10.1.0`.
- Keep the #11628 entry in `10.1.0` (cherry-picked to the release branch
as #12032) and the `10.0.0` section as is (it differs from
`release/therock-10.0` only by the #11046 placement fixes).

**Docs** (`docs/how-to/amdsmi-cli-tool.md`):
- Update the `amd-smi --help` banner and default-view header examples to
27.2.0 / ROCm 10.2.0 (stale since 27.0.0).

## Issue Tracking

N/A

## Test Plan

- Configure and build with `-DBUILD_TESTS=ON`; confirm CMake reports
27.2.0.
- Run `pre-commit` (amdsmi config) on the changed files.
- Check `10.2.0`, `10.1.0` and `10.0.0` against changelog conventions:
allowed subsection headings, two-space line breaks on headline bullets,
no JIRA IDs in entry text.
- Diff the `10.1.0` entries and headings against `release/therock-10.1`.
- No GPU run or new unit test: no runtime code changed.

## Test Result

- Build: pass (`amdsmitst` links), CMake reports 27.2.0
- `pre-commit`: all hooks passed
- Changelog conventions: clean for `10.2.0`, `10.1.0`, `10.0.0`
- `10.1.0` vs `release/therock-10.1`: identical entries and headings


---------

Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants