Skip to content

fix(amdsmi): correct empty and mislabeled APU metric output - #11009

Open
marifamd wants to merge 2 commits into
developfrom
fix/metric-apu-explicit-sections
Open

fix(amdsmi): correct empty and mislabeled APU metric output#11009
marifamd wants to merge 2 commits into
developfrom
fix/metric-apu-explicit-sections

Conversation

@marifamd

Copy link
Copy Markdown
Contributor

Motivation

On APU parts (gfx1151 / Strix), amd-smi metric --energy printed only the GPU header and exited 0, with no energy key in --json or --csv output. Seven other section flags behaved the same way, so a user could not tell "unsupported" from "tool bug".

An audit of the remaining metric arguments on the same part found two more reporting defects: amdsmi_get_clock_info() returning an unavailable clock as a literal 65535 MHz, and the APU IPU bandwidth counters printing without a unit.

Technical Details

CLI metric sections (amdsmi_cli/subcommands/metric.py):

  • APUs do not expose the discrete-GPU sensors, so those sections are dropped from the default dump to keep it readable. The same gate also suppressed a section the user named explicitly, so nothing was printed at all.
  • Suppression is now scoped to the default dump. apu_suppressed is true only when no section was named, so an explicitly named section reports N/A.
  • The end-of-pass APU filter deleted any section whose value was the scalar string "N/A", which removed energy, ecc_blocks, overdrive and xgmi_err after they had been populated. It now applies the same default-dump-only rule, and a section emptied by the APU key-stripping reports N/A instead of disappearing.
  • Affects --energy, --ecc-blocks, --overdrive, --xgmi-err, --pcie, --voltage-curve, --voltage and --fan.

Clock reporting (src/amd_smi/amd_smi.cc, include/amd_smi/amdsmi.h):

  • amdsmi_get_clock_info copied the uint16_t gpu_metrics clock straight into the uint32_t info->clk, so the 16-bit unavailable marker widened to a literal 65535 MHz reading for GFX, MEM, SOC, VCLK and DCLK. The DF case in the same switch already returned UINT32_MAX.
  • All domains now report UINT32_MAX when the clock is unavailable, which the Python interface maps to N/A. amdsmi_clk_info_t.clk documents the convention.
  • No struct layout change, so no ABI impact. No in-tree CLI consumer reads this field: metric.py takes clocks from gpu_metrics and uses amdsmi_get_clock_info only for min_clk/max_clk/clk_deep_sleep, and set_value.py uses only min_clk/max_clk.

APU bandwidth units (amdsmi_cli/subcommands/metric.py, include/amd_smi/amdsmi.h):

  • average_ipu_reads and average_ipu_writes printed bare numbers while the adjacent DRAM counters carried MB/s. All four bandwidth counters now report MB/s, and each is annotated [MB/s] in the header.

Tests (tests/python/unit/gpu/test_cli_metric_apu_sections.py):

  • Hardware-free unit tests that drive MetricCommands.metric_gpu with the C library, logger and helpers stubbed, covering explicitly named sections, the unchanged default dump, and the bandwidth units.

Issue Tracking

JIRA ID: ROCM-9454

Test Plan

  • New unit tests in tests/python/unit/gpu/test_cli_metric_apu_sections.py, run before and after the fix.
  • Full Python unit suite (tests/python/unit) compared against a baseline run with the new file excluded.
  • Hardware validation on a gfx1151 Strix APU:
    • every amd-smi metric argument before and after, in human-readable, JSON and CSV.
    • amdsmi_get_clock_info before and after, with a locally built library loaded through the AMDSMI_LIB_OVERRIDE hook.
  • pre-commit (clang-format, ruff format, SPDX headers).

Test Result

New tests: 7 passed, 11 subtests passed. Confirmed failing before the fix (AssertionError: 'energy' not found in {} and 12 != '12 MB/s').

Full unit suite matches the baseline exactly (no new failures or errors) and adds 7 passes.

Explicitly named sections on Strix:

Argument Before After
--energy (empty) ENERGY: N/A
--ecc-blocks, --overdrive, --xgmi-err (empty) N/A
--pcie, --voltage-curve, --fan, --voltage (empty) section with N/A fields
--energy --json {"gpu": 0} {"gpu": 0, "energy": "N/A"}
--energy --csv gpu / 0 gpu,energy / 0,N/A

The default amd-smi metric dump is structurally identical to before (84 fields, same 9 JSON sections); the only textual differences between runs are live power and activity telemetry.

amdsmi_get_clock_info on Strix: GFX, MEM and SOC clk changed from 65535 to N/A; DF unchanged; every min_clk and max_clk unchanged.

APU_AVERAGE_IPU_READS: 0 now reads 0 MB/s; the DRAM counters are unchanged.

Submission Checklist

- Gate the APU section suppression on the default dump only, so a section
  named on the command line reports N/A instead of printing nothing and
  exiting 0
- Apply the same rule to the end-of-pass filter that dropped scalar N/A
  sections, which removed energy, ecc_blocks, overdrive and xgmi_err
  after they had been populated
- Report N/A for the APU fan section when apu_metrics.fan_pwm is absent
- Add hardware-free unit tests covering the explicitly named sections and
  the unchanged default dump

Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
- Widen the uint16 unavailable marker to UINT32_MAX in
  amdsmi_get_clock_info, so GFX, MEM, SOC, VCLK and DCLK no longer report
  a literal 65535 MHz reading; DF already returned the 32-bit marker
- Document that amdsmi_clk_info_t.clk is UINT32_MAX when unavailable
- Report the APU IPU read and write counters in MB/s, matching the
  adjacent DRAM counters, and annotate all four in the header
- Cover the bandwidth units in the APU metric unit tests

Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
@marifamd
marifamd requested a review from a team August 31, 2026 23:30
@therock-pr-bot

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?

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.

1 participant