Skip to content

feat(amdsmi): add NPM node power limit setting and node power reading - #11471

Merged
bill-shuzhou-liu merged 6 commits into
developfrom
users/shuzhliu/feature-set-npm-power-limit
Oct 2, 2026
Merged

bill-shuzhou-liu merged 6 commits into
developfrom
users/shuzhliu/feature-set-npm-power-limit

Conversation

@bill-shuzhou-liu

Copy link
Copy Markdown
Contributor

Motivation

Adds the ability to set a per-node power cap for NPM (Node Power Management) devices and read back the current instantaneous node power, per the "Set npm power limit" design doc.

Technical Details

Public API (include/amd_smi/amdsmi.h, src/amd_smi/amd_smi.cc):

  • Add amdsmi_set_npm_limit() to set the per-node power cap
  • Rename node_power → current_node_power on amdsmi_npm_info_t for reading instantaneous node power

RocM SMI layer (rocm_smi/src/rocm_smi.cc, rocm_smi/src/rocm_smi_npm.cc, rocm_smi/include/rocm_smi/rocm_smi_npm.h):

  • Implement the sysfs-backed set/get plumbing for the node power limit and current node power

Python bindings (py-interface/amdsmi_interface.py, py-interface/amdsmi_wrapper.py):

  • Expose amdsmi_set_npm_limit() and the renamed current_node_power field

CLI (amdsmi_cli/amdsmi_parser.py, amdsmi_cli/amdsmi_helpers.py, amdsmi_cli/subcommands/node.py, amdsmi_cli/subcommands/set_value.py):

  • Add amd-smi set --node-power-limit/-n to set the cap
  • Update amd-smi node -p output to report current_node_power

Rust bindings (rust-interface/src/amdsmi_wrapper.rs):

  • Bind the new/renamed API

Docs (docs/how-to/amdsmi-cli-tool.md, docs/reference/amdsmi-py-api.md, CHANGELOG.md):

  • Document the new CLI flag and renamed field

Issue Tracking

JIRA ID: ROCM-26304

Test Plan

  • tests/amd_smi_test/unit/gpu/amdsmi_npm_limit_test.cc, rocm_smi_npm_test.cc (C++ GTest)
  • tests/python/unit/gpu/test_cli_set_node_power_limit.py, test_cli_current_node_power_management.py, test_npm_info_current_node_power.py, test_npm_set_limit.py (Python unit)
  • Built locally with BUILD_TESTS=ON, installed, and exercised amd-smi set --node-power-limit / amd-smi node -p against real hardware

Test Result

All new and existing unit tests pass; pre-commit passes on all changed files.

Submission Checklist

@bill-shuzhou-liu
bill-shuzhou-liu requested a review from a team September 10, 2026 20:53
@github-actions github-actions Bot added documentation Improvements or additions to documentation project: amdsmi labels Sep 10, 2026
@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?

@bill-shuzhou-liu

Copy link
Copy Markdown
Contributor Author

Tested on MI450X and works. please refer jira ticket for screen capture.

bill-shuzhou-liu and others added 2 commits September 28, 2026 07:41
Adds amdsmi_set_npm_limit()/amd-smi node --set-power-limit for setting
the per-node power cap, and a current_node_power field on
amdsmi_npm_info_t/amd-smi node -p for reading instantaneous node power
(renamed from node_power per the "Set npm power limit" design doc
update).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- validate_and_set_node_power_limit() now checks amdsmi_npm_info_t::status
  up front and rejects the request with AmdSmiInvalidParameterValueException
  when NPM is disabled on the node, before evaluating the max-bound check
  or calling amdsmi_set_npm_limit()
- Add test_npm_disabled_rejects and thread a status key through the
  existing fake amdsmi_get_npm_info() fixtures in
  test_cli_set_node_power_limit.py
- Document the new rejection case in CHANGELOG.md and
  docs/reference/amdsmi-py-api.md

Signed-off-by: Bill Liu <shuzhliu@amd.com>
@bill-shuzhou-liu
bill-shuzhou-liu force-pushed the users/shuzhliu/feature-set-npm-power-limit branch from d352a61 to e2e5d5e Compare September 28, 2026 13:01
Comment thread projects/amdsmi/src/CMakeLists.txt Resolved

@oliveiradan oliveiradan 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.

@bill-shuzhou-liu , I just added a few comments for you to check

- Convert the parenthesized 3.10+-only `with (...)` blocks in
  test_cli_set_node_power_limit.py to a form valid back to Python 3.7,
  and pin `target-version = "py37"` in pyproject.toml so ruff format
  stops re-converting it on every pre-commit run
- Reject amdsmi_set_npm_limit() requests at the C-library layer
  (rsmi_dev_npm_limit_set()) when NPM is disabled on the node, so this
  is enforced for every caller rather than only amd-smi's CLI; document
  the new rejection case in amdsmi.h, the Python interface, docs, and
  CHANGELOG.md
- Fix the reserved[] doxygen comment to say `reserved[5]` and mention
  both max_node_power_limit and current_node_power
- Add SetNpmLimitRootRejectsWhenNpmDisabled GTest coverage for the new
  NPM-disabled rejection path

Signed-off-by: Bill Liu <shuzhliu@amd.com>
Comment thread projects/amdsmi/src/amd_smi/amd_smi.cc Resolved
Comment thread projects/amdsmi/pyproject.toml Resolved
@bill-shuzhou-liu
bill-shuzhou-liu dismissed marifamd’s stale review September 30, 2026 18:59

Question answered and resolved.

Migrate leftover manual sys.modules save/restore (_SAVED_MODULE_NAMES +
tearDownClass) to the shared stub_modules() helper, and complete the
fake amdsmi.amdsmi_wrapper stub in test_cli_set_node_power_limit.py
with the two status constants amdsmi_helpers.AMDSMIHelpers reads at
import time (AMDSMI_STATUS_UNEXPECTED_SIZE/_UNEXPECTED_DATA). Without
these, setUpClass raised before stub_modules' cleanup was registered,
leaking the fake modules into sys.modules for the rest of the process
and causing 66+ unrelated failures in test_cli_exit_codes.py.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@bill-shuzhou-liu
bill-shuzhou-liu merged commit db4847b into develop Oct 2, 2026
240 of 279 checks passed
@bill-shuzhou-liu
bill-shuzhou-liu deleted the users/shuzhliu/feature-set-npm-power-limit branch October 2, 2026 14:03
@systems-assistant

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

marifamd pushed a commit that referenced this pull request Oct 12, 2026
…#12942)

## Motivation

The ``AMDSMI ASAN`` workflow fails on ``develop``. Its ``ASAN unit
tests`` job runs ``--gtest_filter='*Unit*'`` in a container with no
amdgpu driver, and 16 ``GpuUnit`` NPM tests assert that
``amdsmi_init()`` succeeds. It returns
``AMDSMI_STATUS_DRIVER_NOT_LOADED`` there. The tests (#11471) and the
workflow (#12363) each passed CI alone and broke the job once both were
on ``develop``.

## Technical Details

**Tests** (``tests/amd_smi_test/unit/gpu/amdsmi_npm_limit_test.cc``):
- Add ``REQUIRE_AMDSMI_INIT``: skips the test when ``amdsmi_init()``
returns ``AMDSMI_STATUS_DRIVER_NOT_LOADED``, and still asserts
``AMDSMI_STATUS_SUCCESS`` for any other status
- Replace the 17 ``ASSERT_EQ(init.status(), AMDSMI_STATUS_SUCCESS)``
calls with it

No library, header, wrapper, CLI, or docs change, so no cascade.

## JIRA ID

JIRA ID: ROCM-999999

## Test Plan

- Build ``amdsmitst`` (Debug, ``-DADDRESS_SANITIZER=ON``,
``-DBUILD_TESTS=ON``) and run ``--gtest_filter='*Unit*'`` in an
``ubuntu:24.04`` container with ``/sys/module/amdgpu`` and
``/sys/class/kfd`` masked, once with the ``develop`` version of the test
file and once with this change
- Run the NPM and power-info ``GpuUnit`` cases on a host with the amdgpu
driver loaded

## Test Result

- Driverless, ``develop`` version: 180 passed, 16 failed (the 16
``GpuUnit`` NPM tests in the CI log)
- Driverless, this change: 180 passed, 0 failed, 16 skipped
- Driver loaded: 24 passed, 10 skipped, unchanged by this change
- Built with ``ENABLE_ESMI_LIB=OFF``, unlike CI; the NPM tests do not
use ESMI


Signed-off-by: Oliveira, Daniel <daniel.oliveira@amd.com>
Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
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>
marifamd added a commit that referenced this pull request Oct 30, 2026
## Motivation

The ABI compliance check never flags a break. `check_report_content()`
matches text that abi-compliance-checker 2.3 does not print, so every
report reads as clean, and major mode passes with a warning even when
the checker exits 1. It passed #8575 (removed
`amdsmi_gpu_driver_reload`) and #11821 (grew `amdsmi_driver_info_t` from
768 to 1792 bytes, reverted in #13926).

## Technical Details

**Report parser** (`tests/abi_check/abi_check.py`):
- Read the counts from the report's section headings (`<h2>Removed
Symbols <span class='failed'>&nbsp;1&nbsp;</span>`)
- Fail on removed symbols, High or Medium severity problems, and any
`Size of this type has been changed` row, naming the type
- The checker rates struct growth Low because it assumes the library
allocates the struct; amd-smi callers allocate their output structs, so
growth breaks binaries built against the old header
- Fail major mode when the checker exits non-zero; it used to finish
"with warnings" and pass

**Baseline** (`tests/abi_check/abi_check.py`):
- Compare against `git merge-base <base> HEAD` instead of the base
branch tip. Without this, once the check can fail, a PR that forked
before an API landed on `develop` is reported as removing it

**Tests and docs**:
- `tests/abi_check/abi_check_test.py`: fixtures are fragments of real
abi-compliance-checker 2.3 reports; adds merge-base and pass/fail tests
- `.pre-commit-config.yaml`: run `abi_check_test.py` when
`tests/abi_check/` changes, so the LSTT Formatting job runs it; no
workflow ran it before
- `tests/README.md`: describe both

**Not changed**: `abi-compliance-check.yml`. The check stays
non-blocking and still reports through the `MAJOR ABI BREAKAGE` / `MINOR
ABI BREAKAGE` labels. The workflow runs the base branch's
`abi_check.py`, so this PR's own ABI jobs still use the old parser.

## Issue Tracking

Related: #11821, #13926

## Test Plan

- `abi_check_test.py` on Python 3.6, 3.10 and 3.14, against `develop`'s
`abi_check.py` and this branch
- `develop`'s and this branch's `abi_check.py` with
abi-compliance-checker 2.3 on Ubuntu 22.04 (the CI job's tool and OS),
major and minor mode, on real `amdsmi.h` pairs, each in a throwaway git
repo so `git merge-base` and `git show` behave as in CI
- `pre-commit run --from-ref origin/develop --to-ref HEAD` from
`projects/amdsmi`, as the LSTT Formatting job runs it

## Test Result

- `abi_check_test.py`: the new tests fail on `develop`'s `abi_check.py`
(3 failures, 5 errors) and all 16 pass with this change on every Python
version

Real abi-compliance-checker 2.3 runs, exit code (major / minor):

| `amdsmi.h` change | `develop` | This PR | This PR reports |
|---|---|---|---|
| #8575: removed `amdsmi_gpu_driver_reload` | 0 / 1 | **1** / 1 |
Removed Symbols: 1 |
| #11821: grew `amdsmi_driver_info_t` | 0 / 1 | **1** / 1 |
`amdsmi_driver_info_t` 768 to 1792 bytes |
| `release/therock-10.0` to `release/therock-10.1` | 0 / 1 | **1** / 1 |
`amdsmi_enumeration_info_t` 276 to 280 bytes |
| `release/therock-10.1` to `release/therock-10.2` | 0 / 1 | **1** / 1 |
Medium data type problems; `amdsmi_driver_info_t`,
`amdsmi_fabric_info_t` and its `fabric_info_` union grew |
| #12804: added APIs | 0 / 0 | 0 / 0 | |
| #11471: added APIs | 0 / 1 | 0 / 1 | |
| `AMDSMI_LIB_VERSION_MINOR` bump | 0 / 1 | 0 / 1 | |
| No change | 0 / 0 | 0 / 0 | |
| PR forked before #12804, base branch then gained it | 0 / 1 | 0 / 0 |
|

Minor mode keeps its meaning (any change the checker reports with
`-strict`); only its baseline changed.

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