fix(amdsmi): stop `amdsmi_set_clk_freq()` from falsely returning INVAL by charis-poag-amd · Pull Request #9536 · ROCm/rocm-systems

charis-poag-amd · 2026-07-31T02:41:33Z

Summary

amdsmi_set_clk_freq() / rsmi_dev_gpu_clk_freq_set() could return
AMDSMI_STATUS_INVAL for a clock-set that the hardware actually supports. On
Navi48 (gfx1201) this fails TestFrequenciesReadWrite, even though Navi31
(gfx1100) runs the identical flow and passes.

Root cause

The deep-sleep-aware bitmask validation added in ROCM-9294 (#4707) rejects the
entire request with INVAL as soon as the caller's mask has any bit outside a
"settable" range computed from a prior get_clk_freq read. Because that
acceptable range is derived from a separate read and then acted on
(check-then-use), it doesn't always agree with what the driver will actually
accept, so a supported clock-set is falsely rejected on Navi48.

Fix

bitfield_to_freq_string() now mirrors the amdgpu driver: it silently drops
bits above the highest settable DPM level and applies the in-range subset,
raising INVALID_ARGS only when the requested mask has no settable level.
Building the write payload this way — instead of validating the mask against a
separately-read range and then writing — removes the check-then-use mismatch
that produced the false INVAL.
Deep-sleep ('S') is counted in num_supported but is not a writable
pp_dpm_* label, so it is excluded from the settable range (centralized in
bitfield_to_freq_string()).
The read-only sysfs check is now done only after an attempted write returns
PERMISSION, instead of probing before the write. A genuinely read-only node
still returns NOT_SUPPORTED; the result just comes from the actual write
outcome rather than a separate pre-check.
The clk-type lookup runs before the perf-level change so an invalid clock type
no longer forces MANUAL.
Doc comments for amdsmi_set_clk_freq / rsmi_dev_gpu_clk_freq_set updated to
describe the lenient contract.

Tests

TestFrequenciesReadWrite accepts AMDSMI_STATUS_INVAL only when no level is
settable, and exercises both the in-range-subset apply and the "enable all"
(0xFFFFFFFF) reset paths.
New TestBase::SavePerfLevels() / RestorePerfLevels() restore the per-device
performance level in Close() so a forced MANUAL state cannot leak between
tests (wired into the frequencies and power read/write tests).

Validation

Navi48 / gfx1201 — TestFrequenciesReadWrite now passes
Navi31 / gfx1100 — no regression
MI3x (gfx942 & gfx950)

Notes

Partially reverts the validation added in [ROCM-9294] Fix bitmask validation in rsmi_dev_gpu_clk_freq_set to exclude deep sleep from DPM levels #4707 (ROCM-9294); the lenient
behavior supersedes the strict up-front check.
Re-enables GpuFunctionalReadWrite.TestFrequenciesReadWrite on Aldebaran
(gfx90a / MI210) by removing it from FILTER[aldebaran] in
amdsmitst.exclude (was excluded under SWDEV-306889); the fix makes it pass,
validated by CI. Other Aldebaran exclusions are unchanged.
The ticket's secondary item (no gfx1201 mapping in detect_asic_filter.sh /
amdsmitst.exclude) is intentionally not addressed here — fixing the
underlying behavior makes the set succeed, so no Navi48 test exclusion is
needed.

Fixes ROCM-28256

therock-pr-bot · 2026-07-31T02:42:30Z

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

Copilot

Pull request overview

This PR adjusts AMD SMI / ROCm SMI clock-frequency setting behavior to avoid falsely rejecting supported clock masks (notably on Navi48), aligning mask handling more closely with the amdgpu driver by applying the in-range subset and only returning invalid-args when no settable levels are requested. It also updates tests and docs to reflect the more lenient contract and adds test infrastructure to prevent leaking forced performance-level state across tests.

Changes:

Make bitfield_to_freq_string() drop out-of-range DPM bits (and exclude deep-sleep from the settable range), rejecting only when the resulting set is empty.
Reorder/adjust rsmi_dev_gpu_clk_freq_set() logic (clk-type lookup ordering, post-write read-only check) and expand debug logging.
Update functional tests and base test fixture to save/restore per-device performance levels and to tolerate *_STATUS_INVAL only when no settable level exists.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
projects/amdsmi/tests/amd_smi_test/test_base.h	Adds perf-level save/restore APIs and state for test cleanup.
projects/amdsmi/tests/amd_smi_test/test_base.cc	Implements best-effort perf-level save/restore with verbose logging.
projects/amdsmi/tests/amd_smi_test/functional/gpu/power/power_read_write_test.cc	Saves/restores perf level around the test to avoid cross-test leakage.
projects/amdsmi/tests/amd_smi_test/functional/gpu/clock/frequencies_read_write_test.cc	Updates frequency write/reset expectations for the lenient mask contract; adds optional clock filter.
projects/amdsmi/rocm_smi/src/rocm_smi.cc	Implements lenient bitmask-to-string handling and adjusts clk freq set flow/logging.
projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi.h	Updates API docs to describe the new lenient behavior and deep-sleep handling.
projects/amdsmi/include/amd_smi/amdsmi.h	Updates API docs for the new lenient behavior and deep-sleep handling.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

gabrpham

Looks good, but just need a few additional docs updates. Still need an update to the Python API docs since it still says "only lowest num_supported bits are relevant" and the example sets freq_bitmask = 0, which now returns AMDSMI_STATUS_INVAL.

github-actions · 2026-08-30T00:25:40Z

This pull request has been inactive for 25 days and will be marked as stale.

If you would like to keep this PR open, please:

Add new commits
Add a comment explaining why it should remain open

This PR will be automatically closed in 5 days if no further activity occurs.

github-actions · 2026-09-05T00:22:26Z

This pull request has been automatically closed due to inactivity (30 days with no updates).

If you'd like to continue working on this, feel free to reopen the PR or create a new one.

marifamd · 2026-09-14T18:48:18Z

@charis-poag-amd Please follow up with the comments and move this ticket along

gabrpham

Previous comment has been answered. Potential follow up possible, but can go ahead and approve now.

…etvals - frequencies_read_write_test.cc: replace predict-before validation with a reactive post-hoc check (refresh f after an unexpected INVAL, assert the level is now out of the shrunk settable range) to close a TOCTOU race on live power-state changes (DF/FCLK flakiness). Extract refresh_f(), gpu_tag_str(), and to_bit_string() helpers to DRY up repeated patterns; derive the positive-case mask dynamically from settable_level_count(f) instead of a hardcoded 0b01100; add phase overview comments. - amdsmi.h / rocm_smi.h: document the deep-sleep pseudo-level's exclusion from the settable DPM level count, AMDSMI/RSMI_STATUS_UNEXPECTED_DATA for zero settable levels, and RSMI_STATUS_UNEXPECTED_SIZE for num_supported exceeding AMDSMI/RSMI_MAX_NUM_FREQUENCIES. Addresses review comments from bill-shuzhou-liu and gabrpham on PR #9536.

The deep-sleep bitmask validation added in ROCM-9294 (#4707) rejected a clock-set with AMDSMI_STATUS_INVAL whenever the requested mask had any bit outside a settable range computed from a prior get_clk_freq read. Together with a pre-write read-only sysfs probe, this check-then-write sequence had a TOCTOU window that falsely failed on Navi48 (gfx1201) even though the set is supported (Navi31 runs the same flow and passes), surfacing as a TestFrequenciesReadWrite failure. Drop the strict up-front validation in favor of driver-like behavior: bitfield_to_freq_string() now discards bits above the highest settable level and applies the in-range subset, raising INVALID_ARGS only when no settable level remains. Deep-sleep ('S') is counted in num_supported but is not a writable pp_dpm_* label, so it is excluded from the settable range. The read-only check moves to the post-write PERMISSION path (removing the TOCTOU), and the clk-type lookup runs before the perf-level change so an invalid clock type no longer forces MANUAL. Tests: TestFrequenciesReadWrite accepts AMDSMI_STATUS_INVAL only when no level is settable, and TestBase::SavePerfLevels()/RestorePerfLevels() restore the device perf level so a forced MANUAL state cannot leak between tests. ROCM-28256

- rocm_smi.cc: only upgrade PERMISSION -> NOT_SUPPORTED when the read-only sysfs probe succeeds; otherwise keep the real write result. - amdsmi.h: fix doc grammar ("will restricts" -> "restricts"). ROCM-28256

The clock-set fix makes GpuFunctionalReadWrite.TestFrequenciesReadWrite pass on gfx90a / MI210, so remove it from FILTER[aldebaran] in amdsmitst.exclude (originally excluded under SWDEV-306889). Validated by CI on MI210. ROCM-28256

Record the lenient amdsmi_set_clk_freq behavior (drop out-of-range bits, INVAL only when nothing is settable, post-write read-only check) and the TestFrequenciesReadWrite re-enable on Aldebaran under the ROCm 7.15.0 Fixed section. ROCM-28256

…etvals - frequencies_read_write_test.cc: replace predict-before validation with a reactive post-hoc check (refresh f after an unexpected INVAL, assert the level is now out of the shrunk settable range) to close a TOCTOU race on live power-state changes (DF/FCLK flakiness). Extract refresh_f(), gpu_tag_str(), and to_bit_string() helpers to DRY up repeated patterns; derive the positive-case mask dynamically from settable_level_count(f) instead of a hardcoded 0b01100; add phase overview comments. - amdsmi.h / rocm_smi.h: document the deep-sleep pseudo-level's exclusion from the settable DPM level count, AMDSMI/RSMI_STATUS_UNEXPECTED_DATA for zero settable levels, and RSMI_STATUS_UNEXPECTED_SIZE for num_supported exceeding AMDSMI/RSMI_MAX_NUM_FREQUENCIES. Addresses review comments from bill-shuzhou-liu and gabrpham on PR #9536.

systems-assistant · 2026-10-14T00:21:38Z

TheRock Submodule Bump Activity

Newest first

2026-10-14 — Included in TheRock via ROCm/TheRock#8956.

## Motivation `GpuFunctionalReadWrite.TestPciReadWrite` fails on GPUs with a single PCIe bandwidth level, such as MI100. Since #9536, `amdsmi_set_gpu_pci_bandwidth()` returns `AMDSMI_STATUS_INVAL` for a mask that selects no level, and the test's "every level except the current one" mask is empty on those GPUs. ## Technical Details - `pci_read_write_test.cc`: when that mask is empty, expect `AMDSMI_STATUS_INVAL` (or `NOT_SUPPORTED` / `NO_PERM`, as elsewhere in the test) and skip the rest of the set test for that GPU. The API rejects the mask before it changes anything, so there is nothing to restore. ## Issue Tracking JIRA ID: AILITOOLS-357 ## Test Plan - Build the changed file with the project's flags; run the amd-smi pre-commit hooks. - The Zuul AddressSanitizer job on MI100, which runs `amdsmitst` as root. ## Test Result - The file builds and pre-commit passes. - Before the fix, on develop with #9536, MI100 fails with `AMDSMI_STATUS_INVAL` for mask `0b0`. The test passed there before #9536 merged. Signed-off-by: Adam360x <Adam.pryor@amd.com>

#13460) ## Motivation Two `amdsmitst` read-write tests fail when they run as root in the Zuul AddressSanitizer jobs: `TestPerfCntrReadWrite` on Vega 20 and `TestFrequenciesReadWrite` on Navi 10. The regular Zuul jobs run `amdsmitst` without root, so they skip both tests. ## Technical Details - `perf_cntr_read_write_test.cc`: `CountEvents` no longer checks the result of freeing a counter whose event cannot start (added in #13307). - Stopping a counter that never started fails with `AMDSMI_STATUS_FILE_ERROR`, but the library frees the counter anyway. - The failed check returned instead of throwing, so the test went on into the simultaneous-events case. That case fails to start the same events and returns without freeing the counters it created. Before #13307 the throw skipped that case. - `frequencies_read_write_test.cc`: the out-of-range check now sets level `AMDSMI_MAX_NUM_FREQUENCIES`, which no device can have, instead of the first level above the levels the test read (added in #9536). - A clock with fine-grained DPM, like the Navi 10 data fabric clock, lists only its minimum and maximum, plus its current frequency while that is in between, so it shows 2 or 3 levels. - `amdsmi_set_clk_freq()` reads the levels again before the write. If the clock shows a third level by then, the mask selects a real level and the write succeeds. - `to_bit_string()` prints one more bit than the level count (`AMDSMI_MAX_NUM_FREQUENCIES + 1`), so the out-of-range bit shows in the log instead of `0b0`. ## Issue Tracking JIRA ID: AILITOOLS-357 ## Test Plan - Build both files with the project's flags; run the amd-smi pre-commit hooks. - The Zuul ASAN jobs on Vega 20 and Navi 10 (`test-baremetal-fedora-amdsmi-asan-ubuntu-26.04-vg20`, `-nv10`), which found both failures. ## Test Result - Both files build and pre-commit passes. - Before the fix, on Vega 20, the free check fails 24 times with `AMDSMI_STATUS_FILE_ERROR`, the simultaneous-events case fails to start an event, and LeakSanitizer reports 1268 bytes in 10 allocations, all from the simultaneous-events case. No leak comes from `CountEvents`, so the library did free those counters. - Before the fix, on Navi 10, the test read 2 data fabric levels (506 and 1266 MHz), and the mask `0b100` returned `AMDSMI_STATUS_SUCCESS`. In an earlier run the clock showed 3 levels, the mask `0b1000` got `AMDSMI_STATUS_INVAL`, and the test passed. - Zuul cannot test an unmerged GitHub PR, so the next ASAN run on Zuul confirms the fix after merge. --------- Signed-off-by: Adam360x <Adam.pryor@amd.com>

charis-poag-amd requested review from a team and Copilot July 31, 2026 02:41

github-actions Bot added the project: amdsmi label Jul 31, 2026

Copilot started reviewing on behalf of charis-poag-amd July 31, 2026 02:42

View session

Copilot AI reviewed Jul 31, 2026

View reviewed changes

Comment thread projects/amdsmi/rocm_smi/src/rocm_smi.cc Resolved

Comment thread projects/amdsmi/include/amd_smi/amdsmi.h Resolved

charis-poag-amd force-pushed the charpoag/ROCM-28256-clk-freq-lenient-set branch from 9762d08 to dcc8fbc Compare July 31, 2026 02:53

systems-assistant Bot added the organization: ROCm label Jul 31, 2026

charis-poag-amd force-pushed the charpoag/ROCM-28256-clk-freq-lenient-set branch 2 times, most recently from 952b1b2 to ca3dd73 Compare July 31, 2026 03:53

charis-poag-amd changed the title ~~fix(amdsmi): stop set_clk_freq from falsely returning INVAL~~ fix(amdsmi): stop amdsmi_set_clk_freq() from falsely returning INVAL Jul 31, 2026

charis-poag-amd force-pushed the charpoag/ROCM-28256-clk-freq-lenient-set branch from ca3dd73 to bf8f58b Compare July 31, 2026 04:26

marifamd requested a review from gabrpham August 3, 2026 00:37

bill-shuzhou-liu reviewed Aug 4, 2026

View reviewed changes

gabrpham reviewed Aug 4, 2026

View reviewed changes

Comment thread projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi.h Resolved

github-actions Bot added the Stale PR has no activity for 25+ days label Aug 30, 2026

github-actions Bot closed this Sep 5, 2026

marifamd reopened this Sep 14, 2026

github-actions Bot removed the Stale PR has no activity for 25+ days label Sep 15, 2026

gabrpham approved these changes Sep 29, 2026

View reviewed changes

Comment thread projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi.h Resolved

charis-poag-amd force-pushed the charpoag/ROCM-28256-clk-freq-lenient-set branch from 3325b85 to 00ae9f6 Compare October 2, 2026 02:57

charis-poag-amd force-pushed the charpoag/ROCM-28256-clk-freq-lenient-set branch from 00ae9f6 to 729cf64 Compare October 2, 2026 03:27

bill-shuzhou-liu approved these changes Oct 2, 2026

View reviewed changes

charis-poag-amd force-pushed the charpoag/ROCM-28256-clk-freq-lenient-set branch from 729cf64 to b4f2d4b Compare October 5, 2026 16:49

charis-poag-amd force-pushed the charpoag/ROCM-28256-clk-freq-lenient-set branch from b4f2d4b to 351bdcc Compare October 6, 2026 15:02

charis-poag-amd force-pushed the charpoag/ROCM-28256-clk-freq-lenient-set branch from 351bdcc to 08549ff Compare October 6, 2026 20:19

charis-poag-amd added 5 commits October 12, 2026 12:37

fix(amdsmi): address PR review comments on set_clk_freq

1c7d05e

- rocm_smi.cc: only upgrade PERMISSION -> NOT_SUPPORTED when the read-only sysfs probe succeeds; otherwise keep the real write result. - amdsmi.h: fix doc grammar ("will restricts" -> "restricts"). ROCM-28256

marifamd force-pushed the charpoag/ROCM-28256-clk-freq-lenient-set branch from 08549ff to 956ce8c Compare October 12, 2026 17:37

marifamd merged commit a6583ef into develop Oct 12, 2026
111 of 114 checks passed

marifamd deleted the charpoag/ROCM-28256-clk-freq-lenient-set branch October 12, 2026 20:04

marifamd mentioned this pull request Oct 12, 2026

docs(amdsmi): tighten 10.1.0 and 10.2.0 changelog entries for users #13305

Closed

1 task done

adam360x mentioned this pull request Oct 12, 2026

test(amdsmi): expect INVAL for an empty PCIe bandwidth mask #13309

Merged

1 task done

adam360x mentioned this pull request Oct 14, 2026

test(amdsmi): fix perf counter and clock mask checks that fail as root #13460

Merged

1 task done

pmoutsias-amd mentioned this pull request Oct 15, 2026

docs(amdsmi): move changelog entry to the next release section #13545

Closed

1 task

dmitrii-galantsev mentioned this pull request Oct 15, 2026

test(amdsmi): Add test filter standardization #8995

Open

marifamd mentioned this pull request Oct 27, 2026

chore(amdsmi): bump version to 27.3.0 and correct the 10.x changelog #14390

Open

1 task done

pmoutsias-amd mentioned this pull request Nov 2, 2026

Move AMD SMI changelog entries for code merged after the ROCm 10.2.0 branch to the amd_smi_lib for ROCm 10.3.0 section #14866

Closed

Conversation

charis-poag-amd commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Root cause

Fix

Tests

Validation

Notes

Uh oh!

therock-pr-bot Bot commented Jul 31, 2026

✅ All Policy Checks Passed

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gabrpham left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Aug 30, 2026

Uh oh!

github-actions Bot commented Sep 5, 2026

Uh oh!

marifamd commented Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gabrpham left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

systems-assistant Bot commented Oct 14, 2026

TheRock Submodule Bump Activity

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

charis-poag-amd commented Jul 31, 2026 •

edited

Loading

marifamd commented Sep 14, 2026 •

edited

Loading