Repository navigation
fix(amdsmi): report APU utilization as whole percent - #13540
Merged
Merged
Conversation
Every gpu_metrics v2.x table carries GFX and MM activity in hundredths of a percent. The SMU headers mark GfxActivity and UvdActivity "[centi]" for each ASIC that publishes one -- smu_v13_0_4, smu_v13_0_5 and yellow_carp for v2.1, renoir and vangogh for v2.2 to v2.4 -- and amdgpu stores the value unscaled, dividing by 100 only on the gpu_busy_percent path. amd-smi copied it straight into the public whole-percent fields, so amdsmi_get_gpu_activity() and amd-smi metric --usage reported a GPU at 45% busy as 4567. Convert on the way to the public fields, leaving the not-applicable sentinel alone so a value the device never answered for does not become 655. The APU sub-struct keeps the units the firmware published, as its power fields already do. A v3.0 table reports whole percent to begin with and is untouched. Two further fixes to the same path: - A per-metric query turned an unreadable metrics revision into unexpected data, contradicting the unsupported status the whole-table query reports for the same device. Keep the reason setup gave. - readDevInfoBinary() passed every errno from a refused read through to the caller, where EINVAL became INVALID_ARGS and blamed the caller for a fault on the driver's side. Pass on only the errnos that name a condition the caller can act on -- EBUSY, EPERM, EACCES, EINTR, and EOPNOTSUPP, which is what the SMU answers when the firmware does not know the metrics command at all -- and report the rest as the read failure they are. The APU metrics pointer handed back with the table pointed into the device's own metrics object, which a concurrent call on the same device could replace while the caller was still reading it. Copy it into thread-local storage while the device lock is still held, which is the lifetime the headers already promise for that pointer.
- The oversized-header case declared 4 KiB more than the layout we model but wrote only a table-sized file, so dropping the read clamp ended in a short read rather than an overrun: the case never reached past the buffer and ASAN saw nothing. The blob builder now always writes at least what the header declares, so the dangerous configuration is no longer reachable by leaving an argument out, and removing the clamp reports a heap-buffer-overflow. - Nothing covered the branch that tells a read the driver refused from a node that is simply empty -- both return zero bytes, and only the stream's error flag separates them. A directory in place of the node reproduces the refusal. The test checks that premise before drawing any conclusion from the status, so a platform where a directory stops behaving that way says so instead of passing for the wrong reason. - Nothing covered the activity unit conversion. One case walks three readings chosen to separate truncation, rounding away from zero, and rounding half to even, and gives the two fields different values so a crossed-over conversion cannot hide. Another checks the not-applicable sentinel is not scaled into a plausible-looking 655; it carries a live reading beside the sentinel, because the destination is pre-filled with that same sentinel and the check would otherwise also pass if the read had populated nothing at all. - The APU field-table test built its expectation with the same expression as the code it tested, so it held for whatever that table happened to contain. Spell the v2.4 layout out instead. - The empty-clock-file skip is gated on the driver's APU bit, which MI300A also carries. That is intentional, since those parts power-gate domains too, so say so rather than leaving a comment that reads as small-APU-only.
- The return documentation for both metrics getters listed only success, an unsupported revision and an unexpected size, while the call can also report a busy device or a permission problem. List what a caller can see. - Both getters carried the same APU-metrics paragraph, but a partition metrics table resolves only the XCP revisions and never carries APU metrics, so the pointer that paragraph describes is always null for that call. Say so, and keep the part that still applies: the call invalidates a pointer an earlier whole-table query handed out on the same thread. - Record the unit the APU sub-struct reports activity in, beside the existing note that its power fields stay in milliwatts. It follows the firmware, so it is hundredths of a percent on a v2.x table and whole percent on v3.0, while the top-level fields are whole percent for every revision. Say that the not-applicable sentinel is never scaled, since callers test for it by value. - A comment described the shorter APU revisions as byte-prefix supersets of the v2.4 struct, which is backwards and contradicted the paragraph stacked under it. They are prefixes of it. Fold the two into one and note that v3.0 is a separate layout rather than a prefix. - Restore three logging lines that were re-wrapped by hand while unrelated to the change that touched them. The pinned clang-format leaves both forms alone, so the churn was never required. - The changelog entry named status constants and described the change as APU-only. Say what a user sees instead, and cover the discrete-GPU PLX change it left out.
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The targeted fixes and strengthened regression tests support approval, with no unresolved blocking findings.
0 open findings
What changed in this PR
Corrects APU utilization reporting in amd-smi, following #11075, while improving metrics error handling and pointer lifetime safety.
Changes:
- Converts v2.x activity to whole percent while preserving unavailable-value sentinels.
- Preserves meaningful read errors and copies APU metrics into thread-local storage.
- Strengthens regression tests and clarifies API documentation.
| File | Description |
|---|---|
projects/amdsmi/tests/python/unit/gpu/test_cli_metrics_field_support.py |
Makes expected APU fields independent of production data. |
projects/amdsmi/tests/amd_smi_test/unit/gpu/dynamic_metrics_test.cc |
Tests scaling, sentinels, refused reads, and oversized tables. |
projects/amdsmi/tests/amd_smi_test/functional/gpu/clock/frequencies_read_write_test.cc |
Clarifies the existing APU skip rationale in comments. |
projects/amdsmi/src/amd_smi/amd_smi.cc |
Reverts unrelated logging-format changes. |
projects/amdsmi/rocm_smi/src/rocm_smi_gpu_metrics.cc |
Fixes activity units, status propagation, and APU pointer lifetime. |
projects/amdsmi/rocm_smi/src/rocm_smi_device.cc |
Limits read-error propagation to meaningful statuses. |
projects/amdsmi/include/amd_smi/amdsmi.h |
Documents units, sentinels, return statuses, and partition behavior. |
projects/amdsmi/CHANGELOG.md |
Records the metrics fixes. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The APU sub-struct kept v2.x GFX and MM activity in hundredths of a percent, while v3.0 reports whole percent and the Python interface and the CLI print both with a % sign. On a v2.1 APU more than 1% busy, the APU activity rows were dropped as N/A; below that they read up to 100 times too high. Convert them the same way as the top-level fields. Drop the per-metric query status change: another open pull request changes the same lines in a different way, so that change is left to it. Signed-off-by: Adam360x <Adam.pryor@amd.com>
dmitrii-galantsev
approved these changes
Oct 15, 2026
- Leave v2.3/v2.4 GFX/MM activity as reported: only Van Gogh publishes them, its current firmware interface already reports whole percent, and dividing turned 48% GFX busy into 0% - Fold the 100x CHANGELOG entry into the integrated GPU metrics fix, since no release reported those APUs' activity before Old Van Gogh firmware can still pair v2.2 with whole percent or v2.3 with hundredths; the table carries nothing else to tell them apart. Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
marifamd
force-pushed
the
users/adam360x/ROCM-29548-followup
branch
from
October 16, 2026 14:19
2e1738c to
6db10cc
Compare
marifamd
approved these changes
Oct 16, 2026
- Divide v3.0 average_vcn_activity by 100 with the rounding helper and keep N/A: both v3.0 kernel producers report VCN in hundredths and divide it by 100 for vcn_busy_percent - Document the field as whole percent in amdsmi.h - Add a Resolved Issues entry, since v3.0 APU metrics shipped in 7.13.0 Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
1 task done
1 task done
This was referenced Oct 21, 2026
dgaliffiAMD
pushed a commit
that referenced
this pull request
Oct 21, 2026
## Motivation On integrated GPUs with a `gpu_metrics` v2.x table, GPU and multimedia activity read 100 times too high. A GPU at 45% busy read as 4545 in the C API; `amd-smi metric --usage` showed N/A, or up to 100% when the GPU was nearly idle. The APU firmware reports activity in hundredths of a percent (the SMU headers mark `GfxActivity`/`UvdActivity` `[centi]`; amdgpu divides by 100 only for `gpu_busy_percent`), and amd-smi passed the value on as whole percent. #11075 made these tables readable on v2.1-v2.3 APUs; this follows up on its post-merge review. v3.0 tables already report whole percent. Discrete GPUs are not affected. ## Technical Details - **Behavior change:** v2.x GFX and MM activity are whole percent in `amdsmi_gpu_metrics_t` and in its APU sub-struct (was hundredths), so `amdsmi_get_gpu_activity()` follows. The not-applicable value is kept as is. - `readDevInfoBinary()`: passes on only the errnos that name a device state (suspended or resetting, runtime-inactive, access denied, interrupted, firmware without the metrics command); others report a failed read (`EINVAL` was reported as a caller error). - The APU metrics pointer is copied to thread-local storage while the device lock is held (a concurrent call could replace it while the caller read it). - `amdsmi.h`: return statuses of both metrics calls, the activity unit, and the partition call never returning APU metrics. - `amd_smi.cc`: three log statements rewrapped (formatting only). - Tests: `GpuUnit.ApuActivityReachesThePublicFieldsAsWholePercent`, `NotApplicableApuActivityIsStillReportedAsNotApplicable`, `DriverRefusedMetricsReadIsNotReportedAsUnsupported`; the oversized-header test now catches a missing read-size clamp; the Python APU field test spells out the v2.4 layout. - CHANGELOG: one Resolved Issues entry; the #11075 entry no longer names status constants. - Not changed here: per-metric query statuses, which #13412 changes. ## Issue Tracking JIRA ID: ROCM-29548 ## Test Plan - `amdsmitst --gtest_filter='*Unit*'` and the Python unit suite; each new test with its fix reverted. - Hawk Point APU (gfx1103, `gpu_metrics` v2.1) under a GPU load at a measured 45.9% duty. One captured table was bind-mounted over the sysfs node, so develop, the previous push and this branch read the same bytes: C API, Python and `amd-smi metric --usage`. ## Test Result - Unit: 298 passed (18 skipped); Python: 485 passed. Each new test fails with its fix reverted. - Table holding 4545 hundredths (45.45%): | | C API (top / APU) | `GFX_ACTIVITY` | APU GFX row | |---|---|---|---| | develop | 4545 / 4545 | N/A | missing | | previous push | 45 / 4545 | 45 % | missing | | this branch | 45 / 45 | 45 % | 45 % | - Nearly idle table (18 hundredths): develop showed 18 % in both rows; this branch shows 0 %. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. --------- Signed-off-by: Adam360x <Adam.pryor@amd.com> Signed-off-by: Maisam Arif <Maisam.Arif@amd.com> Co-authored-by: Maisam Arif <Maisam.Arif@amd.com>
Contributor
TheRock Submodule Bump ActivityNewest first
|
This was referenced Oct 27, 2026
dmitrii-galantsev
added a commit
that referenced
this pull request
Oct 30, 2026
Callers allocate most amdsmi structs and load the library by name, so a changed layout corrupts memory instead of failing to load. Public types changed size 46 times between ROCm 6.0 and 10.2, 19 of them within one SONAME; #11741 and #11821 are the latest. The ABI Compliance Check only labels such PRs. abi_gate.py parses amdsmi.h with libclang and records every public type's size, alignment and field offsets, enumerator values, function signatures, integer macros and the units, sentinels and directions in field and parameter doc comments. It snapshots a commit, its base and the latest release/therock-* branch from git, compares them, and prints each break as type, field, old and new value. gcc and g++ also compile static asserts of the base's layout against the new header. Additions and fields carved from reserved arrays pass. A break passes only when projects/amdsmi/abi/abi_exceptions.json gains an entry for that exact path, old and new value, and CHANGELOG.md names it, so it is reviewed by the owners of projects/amdsmi/. "abi_gate.py acknowledge" writes the entries. The file also lists append_only structs and notes on meaning the docs cannot express; today it holds the three % units #13540 added after release/therock-10.2. amdsmi-abi-gate.yml runs it on every PR to develop and release/therock-*, with no paths filter so it can become a required check, plus merge groups and pushes to develop. It runs the checker from the base commit, so a PR cannot edit it to pass; the workflow file itself still comes from the PR and needs a "require workflows" ruleset before the check is made required. Replayed on the 73 develop commits that changed amdsmi.h in the last year, it blocks all 19 layout and API breaks and misses none; it also blocks 4 doc-only unit corrections, which pass once acknowledged.
1 task
dmitrii-galantsev
added a commit
that referenced
this pull request
Oct 30, 2026
Callers allocate most amdsmi structs and load the library by name, so a changed layout corrupts memory instead of failing to load. Public types changed size 46 times between ROCm 6.0 and 10.2, 19 of them within one SONAME; #11741 and #11821 are the latest. The ABI Compliance Check only labels such PRs. abi_gate.py parses amdsmi.h with libclang and records every public type's size, alignment and field offsets, enumerator values, function signatures, integer macros and the units, sentinels and directions in field and parameter doc comments. It snapshots a commit, its base and the latest release/therock-* branch from git, compares them, and prints each break as type, field, old and new value. gcc and g++ also compile static asserts of the base's layout against the new header. Additions and fields carved from reserved arrays pass. A break passes only when projects/amdsmi/abi/abi_exceptions.json gains an entry for that exact path, old and new value, and CHANGELOG.md names it, so it is reviewed by the owners of projects/amdsmi/. "abi_gate.py acknowledge" writes the entries. The file also lists append_only structs and notes on meaning the docs cannot express; today it holds the three % units #13540 added after release/therock-10.2. amdsmi-abi-gate.yml runs it on every PR to develop and release/therock-*, with no paths filter so it can become a required check, plus merge groups and pushes to develop. It runs the checker from the base commit, so a PR cannot edit it to pass; the workflow file itself still comes from the PR and needs a "require workflows" ruleset before the check is made required. Replayed on the 73 develop commits that changed amdsmi.h in the last year, it blocks all 19 layout and API breaks and misses none; it also blocks 4 doc-only unit corrections, which pass once acknowledged.
dmitrii-galantsev
added a commit
that referenced
this pull request
Oct 30, 2026
Callers allocate most amdsmi structs and load the library by name, so a changed layout corrupts memory instead of failing to load. Public types changed size 46 times between ROCm 6.0 and 10.2, 19 of them within one SONAME; #11741 and #11821 are the latest. The ABI Compliance Check only labels such PRs. abi_gate.py parses amdsmi.h with libclang and records every public type's size, alignment and field offsets, enumerator values, function signatures, integer macros and the units, sentinels and directions in field and parameter doc comments. It snapshots a commit, its base and the latest release/therock-* branch from git, compares them, and prints each break as type, field, old and new value. gcc and g++ also compile static asserts of the base's layout against the new header. Additions and fields carved from reserved arrays or bitfields pass. The header must also give C++ the same layouts, field types and functions, with C linkage. A break passes only when projects/amdsmi/abi/abi_exceptions.json gains an entry for that exact path, old and new value, and CHANGELOG.md names it, so it is reviewed by the owners of projects/amdsmi/. "abi_gate.py acknowledge" writes the entries. The file also lists append_only structs and notes on meaning the docs cannot express; today it holds the three % units #13540 added after release/therock-10.2. amdsmi-abi-gate.yml runs it on every PR to develop and release/therock-*, with no paths filter so it can become a required check, plus merge groups and pushes to develop. It runs the checker from the base commit, or from develop when the base has none, so a PR cannot edit it to pass; the workflow file itself still comes from the PR and needs a "require workflows" ruleset before the check is made required. Replayed on the 73 develop commits that changed amdsmi.h in the last year, it blocks all 19 layout and API breaks and misses none; it also blocks 4 doc-only unit corrections, which pass once acknowledged.
erman-gurses
pushed a commit
that referenced
this pull request
Nov 4, 2026
## Motivation Callers allocate most amdsmi structs and load the library by name, so a changed layout corrupts memory in programs built against the old header instead of failing to load. Recent examples: - #11741 grew `amdsmi_fabric_info_t` from 320 to 552 bytes. - #11821 grew `amdsmi_driver_info_t` from 768 to 1792 bytes. - #10226 grew `amdsmi_enumeration_info_t` from 276 to 280 bytes. All three landed within SONAME 27. #14425 made the existing `ABI Compliance Check` report such changes, but it only labels a PR and cannot block it. This PR also covers the follow-ups from #14425's review. ## Technical Details **ABI Compliance Check (#14425 follow-ups)** - `abi_check.py` exits 1 for a break and 2 when it could not compare. That covers a checker error, a missing merge base, an old header it cannot fetch, or a report without the 2.3 summary table. `abi-compliance-check.yml` records exit 2 as `error`, which leaves the labels unchanged instead of adding a false `MAJOR ABI BREAKAGE`. - It defines `ENABLE_ESMI_LIB` before parsing, so the CPU API (69 functions, 12 types) is checked. - It fails on a changed non-string constant, such as `AMDSMI_MAX_MM_IP_COUNT` going from 8 to 16. - Smaller fixes: - a size row no longer inherits a type name from an earlier section; - it works on Python 3.6; - the patterns point at `ACC_COMMIT`; - new tests cover High severity, symbol problems and Low type problems that are not size changes. **AMDSMI ABI Gate (new, blocking once required)** - `tests/abi_check/abi_gate.py` parses `amdsmi.h` with libclang and records: - each type's size, alignment and field offsets; - enumerator values and function signatures; - integer macros; - the units, sentinel values and in/out directions in field and parameter doc comments. It compares the PR with its base and the newest `release/therock-*` branch, each read from git. Additions pass, and so do fields carved from `reserved*` arrays or bitfields when the struct keeps its size. - An intended break is recorded with `abi_gate.py acknowledge --why "#NNNN: ..."`, which adds one line per change (path, old and new value, reason) to `projects/amdsmi/abi/abi_exceptions.json`, together with a CHANGELOG line naming it. The owners of `projects/amdsmi/` review both. The file starts with the three `%` units #13540 documented on develop that 10.2 does not have. - `.github/workflows/amdsmi-abi-gate.yml` runs on every PR to develop and `release/therock-*`, on merge groups, and on pushes to develop. - It has a read-only token and no secrets. - It runs the checker from the base commit, or from develop when the base has none, such as an older release branch. - There is no `paths:` filter, so it can become a required check. PRs that do not touch amdsmi's headers pass in seconds. **TheRock CI** - `.github/skip-ci-config.toml` skips the TheRock build for `projects/amdsmi/abi/*` and `projects/amdsmi/tests/abi_check/*`, which no build reads. This PR does not make the gate required. That is a ruleset change after an advisory period, together with a "require workflows" rule, because on pull requests GitHub runs the PR's own copy of the workflow file. ## Issue Tracking JIRA ID: TBD Follows up #14425. ## Test Plan - `abi_check_test.py`, `abi_gate_test.py` and the amdsmi pre-commit hooks. - `abi_check.py` with abi-compliance-checker 2.3 on real headers: no change, an enum value change, a header that does not compile, and a missing base ref. - Gate replay on the 73 develop commits that changed `amdsmi.h` in the last 12 months, the `release/therock-*` transitions, and 25 bypass fixtures. - The gate workflow run locally with `act` for same-repo and fork PRs, merge groups and pushes to develop. ## Test Result - 29 and 86 unit tests pass. - `abi_check.py` exits 0, 1, 2 and 2 for the four headers. - The gate blocks all 19 layout and API breaks in the replay and misses none. Among them: #11741, #11821, #13926, #8575, #10226, the `AMDSMI_LINK_TYPE_PCIE`/`XGMI` value swap, and the same-size `amdsmi_version_t` change. - It also blocks 4 doc-only unit corrections, which pass once acknowledged. - A PR that replaces the checker with `exit 0` still fails. ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. --------- Signed-off-by: Maisam Arif <Maisam.Arif@amd.com> Co-authored-by: Maisam Arif <Maisam.Arif@amd.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
On integrated GPUs with a
gpu_metricsv2.x table, GPU and multimedia activity read 100 times too high. A GPU at 45% busy read as 4545 in the C API;amd-smi metric --usageshowed N/A, or up to 100% when the GPU was nearly idle.The APU firmware reports activity in hundredths of a percent (the SMU headers mark
GfxActivity/UvdActivity[centi]; amdgpu divides by 100 only forgpu_busy_percent), and amd-smi passed the value on as whole percent.#11075 made these tables readable on v2.1-v2.3 APUs; this follows up on its post-merge review. v3.0 tables already report whole percent. Discrete GPUs are not affected.
Technical Details
amdsmi_gpu_metrics_tand in its APU sub-struct (was hundredths), soamdsmi_get_gpu_activity()follows. The not-applicable value is kept as is.readDevInfoBinary(): passes on only the errnos that name a device state (suspended or resetting, runtime-inactive, access denied, interrupted, firmware without the metrics command); others report a failed read (EINVALwas reported as a caller error).amdsmi.h: return statuses of both metrics calls, the activity unit, and the partition call never returning APU metrics.amd_smi.cc: three log statements rewrapped (formatting only).GpuUnit.ApuActivityReachesThePublicFieldsAsWholePercent,NotApplicableApuActivityIsStillReportedAsNotApplicable,DriverRefusedMetricsReadIsNotReportedAsUnsupported; the oversized-header test now catches a missing read-size clamp; the Python APU field test spells out the v2.4 layout.Issue Tracking
JIRA ID: ROCM-29548
Test Plan
amdsmitst --gtest_filter='*Unit*'and the Python unit suite; each new test with its fix reverted.gpu_metricsv2.1) under a GPU load at a measured 45.9% duty. One captured table was bind-mounted over the sysfs node, so develop, the previous push and this branch read the same bytes: C API, Python andamd-smi metric --usage.Test Result
GFX_ACTIVITYSubmission Checklist