From 87d1add21549d112ad19300ae969a2cb109b3309 Mon Sep 17 00:00:00 2001 From: Koushik Billakanti Date: Thu, 22 Oct 2026 12:39:56 -0500 Subject: [PATCH 1/8] fix(amdsmi): report empty DPM tables and reset FCLK cap Signed-off-by: Koushik Billakanti --- projects/amdsmi/CHANGELOG.md | 6 + .../amdsmi/amdsmi_cli/subcommands/reset.py | 33 +++++ projects/amdsmi/rocm_smi/src/rocm_smi.cc | 5 + projects/amdsmi/src/amd_smi/amd_smi_utils.cc | 11 ++ .../amd_smi_test/unit/gpu/clock_range_test.cc | 27 +++- .../unit/gpu/test_cli_reset_clocks_fclk.py | 122 ++++++++++++++++++ .../python/unit/system/test_cli_exit_codes.py | 14 +- 7 files changed, 210 insertions(+), 8 deletions(-) create mode 100644 projects/amdsmi/tests/python/unit/gpu/test_cli_reset_clocks_fclk.py diff --git a/projects/amdsmi/CHANGELOG.md b/projects/amdsmi/CHANGELOG.md index a8a2c9ffdf24..84ab7521e12d 100644 --- a/projects/amdsmi/CHANGELOG.md +++ b/projects/amdsmi/CHANGELOG.md @@ -218,6 +218,12 @@ Full documentation for amd_smi_lib is available at [https://rocm.docs.amd.com/pr - **Fixed GPU metrics returning no data on integrated GPUs**. - `amdsmi_get_gpu_metrics_info()` and `amdsmi_get_gpu_activity()` failed on APUs such as gfx1103, leaving PLX temperature, GPU utilization and `amd-smi metric` empty. Those devices now report the values they provide with the rest marked N/A, and any GPU without a PLX sensor now reports N/A there instead of a placeholder reading. +- **Fixed clock queries and `amd-smi set` on a clock without a DPM table returning an error instead of not supported**. + - On GPUs such as the Raphael iGPU, an empty `pp_dpm_fclk` made `amdsmi_get_clk_freq()` return `AMDSMI_STATUS_UNEXPECTED_DATA` and `amdsmi_get_clock_info()` return `AMDSMI_STATUS_IO`, so `amd-smi set --clk-level FCLK` and `--clk-limit FCLK` failed. They now return `AMDSMI_STATUS_NOT_SUPPORTED`, as for a missing `pp_dpm_*` file. + +- **Fixed `amd-smi reset --clocks` leaving a lowered FCLK cap in place**. + - On MI300X the cap from `amd-smi set --clk-limit FCLK MAX` survived the reset, because returning to the auto performance level restores only the GFX and memory clock ranges. The reset now raises the FCLK cap back to the top DPM level. + - **Fixed `amd-smi` printing a Python traceback when an unknown NIC or switch is selected**. - `amd-smi static --nic 999` and `--switch 999` failed while building the "device not found" error, so the command exited `1` with a traceback and no readable message. `--json` and `--csv` produced no parseable output. - Both now report `Can not find a device: NIC '999'` (or `SWITCH`) and exit `196`, matching `--gpu`, `--cpu`, and `--core`. diff --git a/projects/amdsmi/amdsmi_cli/subcommands/reset.py b/projects/amdsmi/amdsmi_cli/subcommands/reset.py index bad21cc0c0f9..2aa15e55fd55 100644 --- a/projects/amdsmi/amdsmi_cli/subcommands/reset.py +++ b/projects/amdsmi/amdsmi_cli/subcommands/reset.py @@ -14,6 +14,25 @@ class ResetCommands: + @staticmethod + def _restore_fclk_cap(gpu): + """Raise a lowered FCLK cap back to the top DPM level. + + Switching to AUTO restores the GFX and memory clock ranges but not FCLK, so + a cap from ``set --clk-limit FCLK MAX`` would otherwise outlive the reset. + + Returns: + bool: True when the cap was raised. + """ + clk_type = amdsmi_interface.AmdSmiClkType.DF + levels = amdsmi_interface.amdsmi_get_clk_freq(gpu, clk_type)["frequency"] + top_mhz = max((hz for hz in levels if hz > 0), default=0) // 1_000_000 + current_max = amdsmi_interface.amdsmi_get_clock_info(gpu, clk_type)["max_clk"] + if not top_mhz or not isinstance(current_max, int) or current_max >= top_mhz: + return False + amdsmi_interface.amdsmi_set_gpu_clk_limit(gpu, "fclk", "max", top_mhz) + return True + def reset( self, args, @@ -229,6 +248,20 @@ def reset( ) self.helpers.error_collector.record_library_error(e.get_error_code()) # continue to reset clocks and performance level + try: + self._restore_fclk_cap(args.gpu) + except amdsmi_exception.AmdSmiLibraryException as e: + if e.get_error_code() == amdsmi_interface.amdsmi_wrapper.AMDSMI_STATUS_NO_PERM: + raise PermissionError("Command requires elevation") from e + # A GPU without an FCLK table has no cap to restore. + if ( + e.get_error_code() + != amdsmi_interface.amdsmi_wrapper.AMDSMI_STATUS_NOT_SUPPORTED + ): + self.helpers.error_collector.record_library_error(e.get_error_code()) + logging.debug( + "Failed to restore fclk cap on gpu %s | %s", gpu_id, e.get_error_info() + ) try: level_auto = amdsmi_interface.AmdSmiDevPerfLevel.AUTO amdsmi_interface.amdsmi_set_gpu_perf_level(args.gpu, level_auto) diff --git a/projects/amdsmi/rocm_smi/src/rocm_smi.cc b/projects/amdsmi/rocm_smi/src/rocm_smi.cc index 79efa031016d..da04402990dc 100644 --- a/projects/amdsmi/rocm_smi/src/rocm_smi.cc +++ b/projects/amdsmi/rocm_smi/src/rocm_smi.cc @@ -1360,6 +1360,11 @@ static rsmi_status_t get_frequencies(amd::smi::DevInfoTypes type, rsmi_clk_type_ f->current = 0; ret = GetDevValueVec(type, dv_ind, &val_vec); + // An empty pp_dpm_* file (e.g. FCLK on an iGPU) lists no levels, the same as a + // missing one. + if (ret == RSMI_STATUS_UNEXPECTED_DATA) { + return RSMI_STATUS_NOT_SUPPORTED; + } if (ret != RSMI_STATUS_SUCCESS) { return ret; } diff --git a/projects/amdsmi/src/amd_smi/amd_smi_utils.cc b/projects/amdsmi/src/amd_smi/amd_smi_utils.cc index fcfa2831c548..8703680f708f 100644 --- a/projects/amdsmi/src/amd_smi/amd_smi_utils.cc +++ b/projects/amdsmi/src/amd_smi/amd_smi_utils.cc @@ -353,9 +353,16 @@ amdsmi_status_t smi_amdgpu_parse_dpm_ranges(std::istream& dpm_stream, unsigned int current_freq = 0; char str[10]; char single_char; + bool has_levels = false; for (std::string line; getline(dpm_stream, line);) { unsigned int dpm_level, freq; + // A clock without a DPM table reads back as a bare newline. + if (line.find_first_not_of(" \t\r") == std::string::npos) { + continue; + } + has_levels = true; + char firstChar = line[0]; if (firstChar == 'S') { if (sscanf(line.c_str(), "%c: %u%9s", &single_char, &sleep_freq, str) <= 2) { @@ -388,6 +395,10 @@ amdsmi_status_t smi_amdgpu_parse_dpm_ranges(std::istream& dpm_stream, dpm = dpm_level > dpm ? dpm_level : dpm; } } + // Nothing to report a range from: answer as for a missing pp_dpm_* file. + if (!has_levels && !od_range.present) { + return AMDSMI_STATUS_NOT_SUPPORTED; + } if (dpm == 0 && current_freq > 0) { // if the dpm level is 0, then the current frequency is the min/max frequency max = current_freq; diff --git a/projects/amdsmi/tests/amd_smi_test/unit/gpu/clock_range_test.cc b/projects/amdsmi/tests/amd_smi_test/unit/gpu/clock_range_test.cc index 29ef18d4ee37..e1c35359f275 100644 --- a/projects/amdsmi/tests/amd_smi_test/unit/gpu/clock_range_test.cc +++ b/projects/amdsmi/tests/amd_smi_test/unit/gpu/clock_range_test.cc @@ -252,15 +252,32 @@ TEST(GpuUnit, DpmRangesUnavailableSleepSentinelNotRejected) { EXPECT_EQ(ranges.sleep_state_freq, -1); } -// An empty pp_dpm_* stream with no overdrive range: min keeps its UINT_MAX -// "unavailable" sentinel and surfaces as -1 rather than failing the call. -TEST(GpuUnit, DpmRangesEmptyStreamKeepsUnavailableMin) { +// An empty pp_dpm_* stream with no overdrive range has no range to report, so it +// answers as a missing pp_dpm_* file does. +TEST(GpuUnit, DpmRangesEmptyStreamNotSupported) { std::istringstream dpm(""); SmiAmdgpuOdClkRange od_range; // not present -> derive from the (absent) levels SmiAmdgpuClkRanges ranges; + EXPECT_EQ(smi_amdgpu_parse_dpm_ranges(dpm, od_range, ranges), AMDSMI_STATUS_NOT_SUPPORTED); +} + +// The amdgpu driver prints a bare newline for a clock without a DPM table, e.g. +// pp_dpm_fclk on a Raphael iGPU. That line is not a malformed level. +TEST(GpuUnit, DpmRangesBlankLineNotSupported) { + std::istringstream dpm("\n"); + SmiAmdgpuOdClkRange od_range; + SmiAmdgpuClkRanges ranges; + EXPECT_EQ(smi_amdgpu_parse_dpm_ranges(dpm, od_range, ranges), AMDSMI_STATUS_NOT_SUPPORTED); +} + +// With an overdrive range the domain still has a range to report. +TEST(GpuUnit, DpmRangesBlankLineKeepsOdRange) { + std::istringstream dpm("\n"); + SmiAmdgpuOdClkRange od_range{true, 1800u, 1300u}; + SmiAmdgpuClkRanges ranges; EXPECT_EQ(smi_amdgpu_parse_dpm_ranges(dpm, od_range, ranges), AMDSMI_STATUS_SUCCESS); - EXPECT_EQ(ranges.max_freq, 0); - EXPECT_EQ(ranges.min_freq, -1); + EXPECT_EQ(ranges.max_freq, 1800); + EXPECT_EQ(ranges.min_freq, 1300); } // A genuinely out-of-range value (> INT_MAX, not the sentinel) is still diff --git a/projects/amdsmi/tests/python/unit/gpu/test_cli_reset_clocks_fclk.py b/projects/amdsmi/tests/python/unit/gpu/test_cli_reset_clocks_fclk.py new file mode 100644 index 000000000000..66cb15f9f666 --- /dev/null +++ b/projects/amdsmi/tests/python/unit/gpu/test_cli_reset_clocks_fclk.py @@ -0,0 +1,122 @@ +#!/usr/bin/env python3 +# Copyright Advanced Micro Devices, Inc. +# SPDX-License-Identifier: MIT + +"""Mock-based unit tests for the FCLK step of ``amd-smi reset --clocks``. + +The stubbed library lets these run without GPU hardware or the compiled +``amdsmi`` package. +""" + +import os +import types +import unittest + +try: + from common.common import ( + amdsmi_path, + cli_search_order, + fake_module, + find_cli_dir, + load_cli_module, + stub_modules, + ) +except (ImportError, FileNotFoundError): # pragma: no cover - harness/install unavailable + amdsmi_path = None + cli_search_order = None + find_cli_dir = None + +_CLI_DIR = ( + find_cli_dir(*cli_search_order(os.path.dirname(os.path.abspath(__file__)))) + if find_cli_dir and cli_search_order + else None +) +RESET_PATH = os.path.join(_CLI_DIR, "subcommands", "reset.py") if _CLI_DIR else "" + +_STATUS_NOT_SUPPORTED = 2 +_DF = "DF" + +# MI300X FCLK DPM table, in Hz. +_FCLK_HZ = [1300_000_000, 1550_000_000, 1800_000_000] + + +class _FakeLibraryException(Exception): + def __init__(self, err_code=_STATUS_NOT_SUPPORTED): + super().__init__("mock error") + self._err_code = err_code + + def get_error_code(self): + return self._err_code + + +def _build_fake_amdsmi(): + amdsmi_pkg = types.ModuleType("amdsmi") + interface = types.ModuleType("amdsmi.amdsmi_interface") + exception = types.ModuleType("amdsmi.amdsmi_exception") + wrapper = types.ModuleType("amdsmi.amdsmi_wrapper") + + wrapper.AMDSMI_STATUS_NOT_SUPPORTED = _STATUS_NOT_SUPPORTED + interface.amdsmi_wrapper = wrapper + interface.AmdSmiClkType = types.SimpleNamespace(DF=_DF) + exception.AmdSmiLibraryException = _FakeLibraryException + + amdsmi_pkg.amdsmi_interface = interface + amdsmi_pkg.amdsmi_exception = exception + return { + "amdsmi": amdsmi_pkg, + "amdsmi.amdsmi_interface": interface, + "amdsmi.amdsmi_exception": exception, + "amdsmi.amdsmi_wrapper": wrapper, + } + + +class TestRestoreFclkCap(unittest.TestCase): + """Unit tests for ``ResetCommands._restore_fclk_cap``.""" + + @classmethod + def setUpClass(cls): + if not RESET_PATH or not os.path.isfile(RESET_PATH): + raise unittest.SkipTest( + f"amd-smi CLI reset.py not found (looked in {_CLI_DIR or amdsmi_path})" + ) + modules = _build_fake_amdsmi() + modules["amdsmi_helpers"] = fake_module( + "amdsmi_helpers", AMDSMIHelpers=type("AMDSMIHelpers", (), {}) + ) + stub_modules(cls, modules) + cls.interface = modules["amdsmi.amdsmi_interface"] + cls.module = load_cli_module("reset_under_test", RESET_PATH, sys_path_dir=_CLI_DIR) + + def _restore(self, levels_hz, max_clk): + self.writes = [] + self.interface.amdsmi_get_clk_freq = lambda _gpu, clk: {"frequency": list(levels_hz)} + self.interface.amdsmi_get_clock_info = lambda _gpu, clk: {"max_clk": max_clk} + self.interface.amdsmi_set_gpu_clk_limit = lambda _gpu, clk, lim, val: self.writes.append( + (clk, lim, val) + ) + return self.module.ResetCommands._restore_fclk_cap(None) + + def test_lowered_cap_raised_to_top_level(self): + self.assertTrue(self._restore(_FCLK_HZ, 1550)) + self.assertEqual(self.writes, [("fclk", "max", 1800)]) + + def test_cap_at_top_level_not_written(self): + self.assertFalse(self._restore(_FCLK_HZ, 1800)) + self.assertEqual(self.writes, []) + + def test_unreadable_cap_not_written(self): + self.assertFalse(self._restore(_FCLK_HZ, "N/A")) + self.assertEqual(self.writes, []) + + def test_empty_table_not_written(self): + self.assertFalse(self._restore([], 1550)) + self.assertEqual(self.writes, []) + + def test_unsupported_clock_raises(self): + # The caller treats NOT_SUPPORTED as "no FCLK table" and moves on. + def _raise(_gpu, _clk): + raise _FakeLibraryException(_STATUS_NOT_SUPPORTED) + + self.interface.amdsmi_get_clk_freq = _raise + with self.assertRaises(_FakeLibraryException): + self.module.ResetCommands._restore_fclk_cap(None) diff --git a/projects/amdsmi/tests/python/unit/system/test_cli_exit_codes.py b/projects/amdsmi/tests/python/unit/system/test_cli_exit_codes.py index 23d1b6024a82..9dcb55846919 100644 --- a/projects/amdsmi/tests/python/unit/system/test_cli_exit_codes.py +++ b/projects/amdsmi/tests/python/unit/system/test_cli_exit_codes.py @@ -2010,10 +2010,18 @@ def gpureset(cmd, mode): return {"amdsmi_reset_gpu": fn} def clocks(cmd, mode): - # Overdrive is reset first, then perf level (twice) with the same - # handle; both must be stubbed so a fake handle never reaches hardware. + # Overdrive is reset first, then the FCLK cap, then perf level (twice) + # with the same handle; all must be stubbed so a fake handle never + # reaches hardware. fn = _raise_not_supported if mode == "fail" else _noop - return {"amdsmi_set_gpu_overdrive_level": fn, "amdsmi_set_gpu_perf_level": fn} + if mode == "fail": + fclk = {"amdsmi_get_clk_freq": fn} + else: + fclk = { + "amdsmi_get_clk_freq": lambda *_: {"frequency": [1800_000_000]}, + "amdsmi_get_clock_info": lambda *_: {"max_clk": 1800}, + } + return {"amdsmi_set_gpu_overdrive_level": fn, "amdsmi_set_gpu_perf_level": fn, **fclk} def fans(cmd, mode): fn = _raise_not_supported if mode == "fail" else _noop From 9b882704ddcc1a07950d56288b5ee9e87583b328 Mon Sep 17 00:00:00 2001 From: Koushik Billakanti Date: Tue, 27 Oct 2026 00:21:30 -0500 Subject: [PATCH 2/8] fix(amdsmi): harden mixed-GPU sweeps Signed-off-by: Koushik Billakanti --- projects/amdsmi/CHANGELOG.md | 6 +- .../amdsmi/amdsmi_cli/subcommands/reset.py | 29 ++- .../amdsmi/docs/how-to/amdsmi-cli-tool.md | 5 + .../amdsmi/rocm_smi/src/rocm_smi_device.cc | 5 + projects/amdsmi/src/amd_smi/amd_smi_utils.cc | 3 + .../amd_smi_test/unit/gpu/clock_range_test.cc | 112 +++++++++++ projects/amdsmi/tests/python/README.md | 11 ++ projects/amdsmi/tests/python/cli/base.py | 17 +- projects/amdsmi/tests/python/cli/test_set.py | 10 - .../unit/gpu/test_cli_reset_clocks_fclk.py | 9 +- .../python/unit/gpu/test_cli_set_sweep.py | 180 ++++++++++++++++++ .../python/unit/system/test_cli_exit_codes.py | 67 +++++++ 12 files changed, 418 insertions(+), 36 deletions(-) create mode 100644 projects/amdsmi/tests/python/unit/gpu/test_cli_set_sweep.py diff --git a/projects/amdsmi/CHANGELOG.md b/projects/amdsmi/CHANGELOG.md index 84ab7521e12d..0745a6fefb24 100644 --- a/projects/amdsmi/CHANGELOG.md +++ b/projects/amdsmi/CHANGELOG.md @@ -218,11 +218,11 @@ Full documentation for amd_smi_lib is available at [https://rocm.docs.amd.com/pr - **Fixed GPU metrics returning no data on integrated GPUs**. - `amdsmi_get_gpu_metrics_info()` and `amdsmi_get_gpu_activity()` failed on APUs such as gfx1103, leaving PLX temperature, GPU utilization and `amd-smi metric` empty. Those devices now report the values they provide with the rest marked N/A, and any GPU without a PLX sensor now reports N/A there instead of a placeholder reading. -- **Fixed clock queries and `amd-smi set` on a clock without a DPM table returning an error instead of not supported**. - - On GPUs such as the Raphael iGPU, an empty `pp_dpm_fclk` made `amdsmi_get_clk_freq()` return `AMDSMI_STATUS_UNEXPECTED_DATA` and `amdsmi_get_clock_info()` return `AMDSMI_STATUS_IO`, so `amd-smi set --clk-level FCLK` and `--clk-limit FCLK` failed. They now return `AMDSMI_STATUS_NOT_SUPPORTED`, as for a missing `pp_dpm_*` file. +- **Fixed clock queries and settings on GPUs without a clock table**. + - Unsupported clocks now report not supported instead of a data error, while malformed tables and read failures remain errors. - **Fixed `amd-smi reset --clocks` leaving a lowered FCLK cap in place**. - - On MI300X the cap from `amd-smi set --clk-limit FCLK MAX` survived the reset, because returning to the auto performance level restores only the GFX and memory clock ranges. The reset now raises the FCLK cap back to the top DPM level. + - The reset restores the top supported FCLK level and reports restoration failures instead of hiding them. - **Fixed `amd-smi` printing a Python traceback when an unknown NIC or switch is selected**. - `amd-smi static --nic 999` and `--switch 999` failed while building the "device not found" error, so the command exited `1` with a traceback and no readable message. `--json` and `--csv` produced no parseable output. diff --git a/projects/amdsmi/amdsmi_cli/subcommands/reset.py b/projects/amdsmi/amdsmi_cli/subcommands/reset.py index 2aa15e55fd55..b1e2b1fd875f 100644 --- a/projects/amdsmi/amdsmi_cli/subcommands/reset.py +++ b/projects/amdsmi/amdsmi_cli/subcommands/reset.py @@ -16,18 +16,17 @@ class ResetCommands: @staticmethod def _restore_fclk_cap(gpu): - """Raise a lowered FCLK cap back to the top DPM level. - - Switching to AUTO restores the GFX and memory clock ranges but not FCLK, so - a cap from ``set --clk-limit FCLK MAX`` would otherwise outlive the reset. - - Returns: - bool: True when the cap was raised. - """ + """Return whether a lowered FCLK cap was raised; AUTO alone leaves it set.""" clk_type = amdsmi_interface.AmdSmiClkType.DF - levels = amdsmi_interface.amdsmi_get_clk_freq(gpu, clk_type)["frequency"] + try: + levels = amdsmi_interface.amdsmi_get_clk_freq(gpu, clk_type)["frequency"] + current_max = amdsmi_interface.amdsmi_get_clock_info(gpu, clk_type)["max_clk"] + except amdsmi_exception.AmdSmiLibraryException as e: + if e.get_error_code() != amdsmi_interface.amdsmi_wrapper.AMDSMI_STATUS_NOT_SUPPORTED: + raise + logging.debug("FCLK cap is not available | %s", e.get_error_info()) + return False top_mhz = max((hz for hz in levels if hz > 0), default=0) // 1_000_000 - current_max = amdsmi_interface.amdsmi_get_clock_info(gpu, clk_type)["max_clk"] if not top_mhz or not isinstance(current_max, int) or current_max >= top_mhz: return False amdsmi_interface.amdsmi_set_gpu_clk_limit(gpu, "fclk", "max", top_mhz) @@ -253,12 +252,10 @@ def reset( except amdsmi_exception.AmdSmiLibraryException as e: if e.get_error_code() == amdsmi_interface.amdsmi_wrapper.AMDSMI_STATUS_NO_PERM: raise PermissionError("Command requires elevation") from e - # A GPU without an FCLK table has no cap to restore. - if ( - e.get_error_code() - != amdsmi_interface.amdsmi_wrapper.AMDSMI_STATUS_NOT_SUPPORTED - ): - self.helpers.error_collector.record_library_error(e.get_error_code()) + reset_clocks_results["fclk"] = ( + f"[{e.get_error_info(detailed=False)}] Unable to restore FCLK cap" + ) + self.helpers.error_collector.record_library_error(e.get_error_code()) logging.debug( "Failed to restore fclk cap on gpu %s | %s", gpu_id, e.get_error_info() ) diff --git a/projects/amdsmi/docs/how-to/amdsmi-cli-tool.md b/projects/amdsmi/docs/how-to/amdsmi-cli-tool.md index 30e895955776..b504f5219c6f 100644 --- a/projects/amdsmi/docs/how-to/amdsmi-cli-tool.md +++ b/projects/amdsmi/docs/how-to/amdsmi-cli-tool.md @@ -905,6 +905,11 @@ Command Modifiers: DEBUG, INFO, WARNING, ERROR, CRITICAL ``` +`reset --clocks` also restores a lowered FCLK cap to the highest supported DPM +level. GPUs without FCLK information skip this step. If restoring the cap fails, +the command reports the failure in `reset_clocks.fclk` and its exit status while +still attempting to reset the performance level. + (cmd-monitor)= ### amd-smi monitor diff --git a/projects/amdsmi/rocm_smi/src/rocm_smi_device.cc b/projects/amdsmi/rocm_smi/src/rocm_smi_device.cc index e33388b14fd4..35ff92338d97 100644 --- a/projects/amdsmi/rocm_smi/src/rocm_smi_device.cc +++ b/projects/amdsmi/rocm_smi/src/rocm_smi_device.cc @@ -1281,6 +1281,11 @@ int Device::readDevInfoMultiLineStr(DevInfoTypes type, std::vector* while (std::getline(fs, line)) { retVec->push_back(line); } + if (fs.bad() || (fs.fail() && !fs.eof())) { + ss << "Failed to read devInfoMultiLineStr for DevInfoType (" << get_type_string(type) << ")"; + LOG_ERROR(ss); + return EIO; + } fs.close(); if (retVec->empty()) { diff --git a/projects/amdsmi/src/amd_smi/amd_smi_utils.cc b/projects/amdsmi/src/amd_smi/amd_smi_utils.cc index 8703680f708f..eeb47832d389 100644 --- a/projects/amdsmi/src/amd_smi/amd_smi_utils.cc +++ b/projects/amdsmi/src/amd_smi/amd_smi_utils.cc @@ -395,6 +395,9 @@ amdsmi_status_t smi_amdgpu_parse_dpm_ranges(std::istream& dpm_stream, dpm = dpm_level > dpm ? dpm_level : dpm; } } + if (dpm_stream.bad() || (dpm_stream.fail() && !dpm_stream.eof())) { + return AMDSMI_STATUS_IO; + } // Nothing to report a range from: answer as for a missing pp_dpm_* file. if (!has_levels && !od_range.present) { return AMDSMI_STATUS_NOT_SUPPORTED; diff --git a/projects/amdsmi/tests/amd_smi_test/unit/gpu/clock_range_test.cc b/projects/amdsmi/tests/amd_smi_test/unit/gpu/clock_range_test.cc index e1c35359f275..67fd3f91ba76 100644 --- a/projects/amdsmi/tests/amd_smi_test/unit/gpu/clock_range_test.cc +++ b/projects/amdsmi/tests/amd_smi_test/unit/gpu/clock_range_test.cc @@ -10,13 +10,59 @@ #include #include +#include +#include +#include +#include #include +#include #include "amd_smi/amdsmi.h" #include "amd_smi/impl/amd_smi_clk_testing.h" +#include "rocm_smi/rocm_smi.h" +#include "rocm_smi/rocm_smi_device.h" +#include "rocm_smi/rocm_smi_main.h" namespace { +class FakeDpmDevice { + public: + FakeDpmDevice() { + auto path = (std::filesystem::temp_directory_path() / "amdsmi_dpm_XXXXXX").string(); + if (!mkdtemp(path.data())) { + throw std::runtime_error("cannot create DPM test directory"); + } + root_ = path; + std::filesystem::create_directory(root_ / "device"); + auto& devices = amd::smi::RocmSMI::getInstance().devices(); + index_ = static_cast(devices.size()); + devices.push_back(std::make_shared(root_.string(), nullptr)); + } + + ~FakeDpmDevice() { + auto& devices = amd::smi::RocmSMI::getInstance().devices(); + devices.erase(devices.begin() + index_); + std::filesystem::remove_all(root_); + } + + FakeDpmDevice(const FakeDpmDevice&) = delete; + FakeDpmDevice& operator=(const FakeDpmDevice&) = delete; + + uint32_t index() const { return index_; } + std::filesystem::path table() const { return root_ / "device" / "pp_dpm_fclk"; } + + void WriteTable(const char* contents) const { + std::ofstream out(table()); + ASSERT_TRUE(out.is_open()); + out << contents; + ASSERT_TRUE(out.good()); + } + + private: + std::filesystem::path root_; + uint32_t index_; +}; + // pp_od_clk_voltage as seen on MI45x: SCLK and MCLK sections plus an OD_RANGE // footer, but no OD_FCLK section. constexpr char kOdNoFclk[] = @@ -280,6 +326,72 @@ TEST(GpuUnit, DpmRangesBlankLineKeepsOdRange) { EXPECT_EQ(ranges.min_freq, 1300); } +TEST(GpuUnit, DpmRangesReadFailureIsNotAnEmptyTable) { + for (bool has_od_range : {false, true}) { + std::istringstream dpm("0: 1300Mhz\n"); + dpm.setstate(std::ios::badbit); + SmiAmdgpuOdClkRange od_range{has_od_range, 1800u, 1300u}; + SmiAmdgpuClkRanges ranges; + EXPECT_EQ(smi_amdgpu_parse_dpm_ranges(dpm, od_range, ranges), AMDSMI_STATUS_IO); + } +} + +TEST(GpuUnit, DpmFrequencyReadFailureIsNotAnEmptyTable) { + FakeDpmDevice device; + // Offset zero in our own proc mem file fails with EIO without reading memory. + std::filesystem::create_symlink("/proc/self/mem", device.table()); + rsmi_frequencies_t frequencies{}; + EXPECT_EQ(rsmi_dev_gpu_clk_freq_get(device.index(), RSMI_CLK_TYPE_DF, &frequencies), + RSMI_STATUS_UNEXPECTED_SIZE); +} + +TEST(GpuUnit, DpmFrequencyMissingTableNotSupported) { + FakeDpmDevice device; + rsmi_frequencies_t frequencies{}; + EXPECT_EQ(rsmi_dev_gpu_clk_freq_get(device.index(), RSMI_CLK_TYPE_DF, &frequencies), + RSMI_STATUS_NOT_SUPPORTED); +} + +TEST(GpuUnit, DpmFrequencyEmptyTablesNotSupported) { + FakeDpmDevice device; + for (const char* contents : {"", "\n", " \t\r\n", "\n\n"}) { + device.WriteTable(contents); + rsmi_frequencies_t frequencies{}; + EXPECT_EQ(rsmi_dev_gpu_clk_freq_get(device.index(), RSMI_CLK_TYPE_DF, &frequencies), + RSMI_STATUS_NOT_SUPPORTED); + EXPECT_EQ(frequencies.num_supported, 0u); + } +} + +TEST(GpuUnit, DpmFrequencyMalformedTableStillFails) { + FakeDpmDevice device; + device.WriteTable("0: 1300invalid\n"); + rsmi_frequencies_t frequencies{}; + EXPECT_EQ(rsmi_dev_gpu_clk_freq_get(device.index(), RSMI_CLK_TYPE_DF, &frequencies), + RSMI_STATUS_UNEXPECTED_DATA); +} + +TEST(GpuUnit, DpmFrequencyValidTableStillReportsLevels) { + FakeDpmDevice device; + device.WriteTable("0: 1300Mhz\n1: 1550Mhz *\n2: 1800Mhz\n"); + rsmi_frequencies_t frequencies{}; + ASSERT_EQ(rsmi_dev_gpu_clk_freq_get(device.index(), RSMI_CLK_TYPE_DF, &frequencies), + RSMI_STATUS_SUCCESS); + EXPECT_EQ(frequencies.num_supported, 3u); + EXPECT_EQ(frequencies.current, 1u); + EXPECT_EQ(frequencies.frequency[0], 1300000000u); + EXPECT_EQ(frequencies.frequency[2], 1800000000u); +} + +TEST(GpuUnit, DpmRangesWhitespaceDoesNotHideMalformedData) { + for (const char* contents : {"\ninvalid\n", " \t\r\ninvalid\n"}) { + std::istringstream dpm(contents); + SmiAmdgpuOdClkRange od_range; + SmiAmdgpuClkRanges ranges; + EXPECT_EQ(smi_amdgpu_parse_dpm_ranges(dpm, od_range, ranges), AMDSMI_STATUS_IO); + } +} + // A genuinely out-of-range value (> INT_MAX, not the sentinel) is still // rejected, preserving the protection added in #9549. constexpr char kDpmSclkHugeFreq[] = "0: 3000000000Mhz\n"; // 3e9 > INT_MAX diff --git a/projects/amdsmi/tests/python/README.md b/projects/amdsmi/tests/python/README.md index fc578f2e3387..b56fd477e626 100644 --- a/projects/amdsmi/tests/python/README.md +++ b/projects/amdsmi/tests/python/README.md @@ -82,6 +82,17 @@ graph TD All three share the same option set and the same GTest-style summary, because they all delegate to `common.run_test_dir()`. +The `set` sweep runs each setting with a numeric `--gpu` selector and derives +clock and power values from that GPU. Each device's exit code is checked +independently; aggregate errors are not added to the allowed codes. + +The sweep explicitly excludes memory-partition changes. A request can remain +queued until driver reload, and requesting the currently active mode does not +cancel it. Cleanup therefore does not claim to restore the original memory +partition. Test NPS transitions only in a separate, controlled driver-reload +workflow; the generic sweep neither reloads the driver nor clears changes +queued before the test. + ### Invocation From an installed package (recommended — avoids a shadowing `site-packages` copy): diff --git a/projects/amdsmi/tests/python/cli/base.py b/projects/amdsmi/tests/python/cli/base.py index 94ce6a8c5a8a..5c9083c9594f 100644 --- a/projects/amdsmi/tests/python/cli/base.py +++ b/projects/amdsmi/tests/python/cli/base.py @@ -121,7 +121,10 @@ class TestCliBase(unittest.TestCase): # TODO(amdsmi_team): drop these once the CLI tests supports automated # input SWEEP_EXCLUDED_ARGS = { - "set": {"--compute-partition": "waits for confirmation"}, + "set": { + "--compute-partition": "waits for confirmation", + "--memory-partition": "cannot cancel a queued change by restoring the active mode", + }, "reset": { # --gtt reaches reset.py's prompt_reboot(), which calls `systemctl reboot` on "y" "--gtt": "prompts to reboot the host", @@ -570,7 +573,7 @@ def FindArgs(self, cmd, match_str): def CreateCmds(self, cmd_name, list1_name, list2_name, list3_name, list4_name): cmd = f"amd-smi {cmd_name} --help" list1_args = self._order_sweep_args(cmd_name, self.FindArgs(cmd, list1_name)) - list2_args = self.FindArgs(cmd, list2_name) + list2_args = ["pass"] if cmd_name == "set" else self.FindArgs(cmd, list2_name) list3_args = self.FindArgs(cmd, list3_name) list4_args = self.FindArgs(cmd, list4_name) if self.Debug: @@ -624,6 +627,14 @@ def CreateCmds(self, cmd_name, list1_name, list2_name, list3_name, list4_name): ) ) + if cmd_name == "set": + # Select the device before resolving its clock and power limits. + cmds = [ + (f"{command} --gpu {gpu_index}", condition) + for command, condition in cmds + for gpu_index in range(len(self.common.processors)) + ] + # Calculate and substitute in dependent values # Removes cmds that are invalid for index, cmd_cond in enumerate(cmds): @@ -843,7 +854,7 @@ def CreateCmds(self, cmd_name, list1_name, list2_name, list3_name, list4_name): sub_arg = items[2] if sub_arg != found_sub_arg: # Whole word matches only: i.e. remove "--gpu", not "--gpureset" - if "--gpu" in items: + if "--gpu" in items and cmd_name != "set": cmd = "" # Remove all file and watch modifiers except for gpu 0 diff --git a/projects/amdsmi/tests/python/cli/test_set.py b/projects/amdsmi/tests/python/cli/test_set.py index dcfb3f746fac..6b2e73786e65 100644 --- a/projects/amdsmi/tests/python/cli/test_set.py +++ b/projects/amdsmi/tests/python/cli/test_set.py @@ -78,17 +78,7 @@ def _restore_starting_values(self): ) ) - # set --memory-partition defaults - # Safe to write back: the mode only takes effect after a driver reload. - memory_partition = self.partition_data["current_partition"][index]["memory"] - if memory_partition != "N/A": - cmds.append( - (f"amd-smi set --memory-partition {memory_partition} --gpu {index}", self.PASS) - ) - # set --compute-partition defaults - # Must come after memory partition sets, since the memory partition set can override - # this setting (their defaults may differ). accelerator_type = self.partition_data["current_partition"][index]["accelerator_type"] if accelerator_type != "N/A": cmds.append( diff --git a/projects/amdsmi/tests/python/unit/gpu/test_cli_reset_clocks_fclk.py b/projects/amdsmi/tests/python/unit/gpu/test_cli_reset_clocks_fclk.py index 66cb15f9f666..faa064c86af7 100644 --- a/projects/amdsmi/tests/python/unit/gpu/test_cli_reset_clocks_fclk.py +++ b/projects/amdsmi/tests/python/unit/gpu/test_cli_reset_clocks_fclk.py @@ -48,6 +48,9 @@ def __init__(self, err_code=_STATUS_NOT_SUPPORTED): def get_error_code(self): return self._err_code + def get_error_info(self, detailed=True): + return f"status {self._err_code}" + def _build_fake_amdsmi(): amdsmi_pkg = types.ModuleType("amdsmi") @@ -112,11 +115,9 @@ def test_empty_table_not_written(self): self.assertFalse(self._restore([], 1550)) self.assertEqual(self.writes, []) - def test_unsupported_clock_raises(self): - # The caller treats NOT_SUPPORTED as "no FCLK table" and moves on. + def test_unsupported_clock_is_skipped(self): def _raise(_gpu, _clk): raise _FakeLibraryException(_STATUS_NOT_SUPPORTED) self.interface.amdsmi_get_clk_freq = _raise - with self.assertRaises(_FakeLibraryException): - self.module.ResetCommands._restore_fclk_cap(None) + self.assertFalse(self.module.ResetCommands._restore_fclk_cap(None)) diff --git a/projects/amdsmi/tests/python/unit/gpu/test_cli_set_sweep.py b/projects/amdsmi/tests/python/unit/gpu/test_cli_set_sweep.py new file mode 100644 index 000000000000..87fbee7e2bf5 --- /dev/null +++ b/projects/amdsmi/tests/python/unit/gpu/test_cli_set_sweep.py @@ -0,0 +1,180 @@ +# Copyright Advanced Micro Devices, Inc. +# SPDX-License-Identifier: MIT + +"""Hardware-free regressions for the CLI set sweep.""" + +import types +import unittest +from unittest.mock import Mock + +from cli import base, test_set + + +class TestCliSetSweep(unittest.TestCase): + def setUp(self) -> None: + self.sweep = base.TestCliBase() + self.sweep.common = types.SimpleNamespace( + processors=[object(), object()], print=Mock(), cleaner_shader_resets_gpu=lambda: False + ) + + def _commands(self, options, command="set"): + arguments = { + "options": options, + "devices": ["--gpu all", "--gpu 0", "--gpu 1"], + "modifiers": ["pass"], + "": ["pass"], + } + self.sweep.FindArgs = lambda _command, section: arguments[section] + return self.sweep.CreateCmds(command, "options", "devices", "modifiers", "") + + def test_every_set_option_runs_once_per_gpu(self) -> None: + options = ["--perf-level AUTO", "--fan 50"] + for reduced in (True, False): + with self.subTest(reduced=reduced): + self.sweep.ReduceCmds = reduced + commands = self._commands(options) + self.assertCountEqual( + [command for command, _ in commands], + [ + f"amd-smi set {option} --loglevel DEBUG --gpu {gpu}" + for option in options + for gpu in range(2) + ], + ) + for _, allowed in commands: + self.assertEqual( + allowed, [self.sweep.PASS, base.amdsmi.AmdSmiStatus.NOT_SUPPORTED] + ) + + def test_memory_partition_changes_are_explicitly_excluded(self) -> None: + commands = self._commands( + ["--memory-partition NPS1", "--memory-partition NPS2", "--perf-level AUTO"] + ) + self.assertTrue(commands) + self.assertFalse(any("--memory-partition" in command for command, _ in commands)) + messages = [call[0][0] for call in self.sweep.common.print.call_args_list] + exclusions = [message for message in messages if "memory-partition" in message] + self.assertEqual(len(exclusions), 1) + self.assertIn("cannot cancel", exclusions[0]) + + def test_clock_values_come_from_the_selected_gpu(self) -> None: + self.sweep.clk_freq = [ + {"FCLK": {"num_supported": 3, "has_deep_sleep": False}}, + {"FCLK": {"num_supported": 3, "has_deep_sleep": True}}, + ] + self.sweep.metric_data = { + "gpu_data": [ + {"clock": {"fclk_0": {"min_clk": {"value": low}, "max_clk": {"value": high}}}} + for low, high in ((1200, 2000), (400, 800)) + ] + } + commands = self._commands( + [ + "--clk-level {clk_level_fclk}", + "--clk-limit {clk_limit_fclk_max}", + "--clk-limit {clk_limit_fclk_min}", + ] + ) + self.assertCountEqual( + [command for command, _ in commands], + [ + f"amd-smi set {option} --loglevel DEBUG --gpu {gpu}" + for gpu, options in ( + ( + 0, + ( + "--clk-level FCLK 0 1 2", + "--clk-limit FCLK MAX 1800", + "--clk-limit FCLK MIN 1400", + ), + ), + ( + 1, + ( + "--clk-level FCLK 0 1", + "--clk-limit FCLK MAX 700", + "--clk-limit FCLK MIN 500", + ), + ), + ) + for option in options + ], + ) + for _, allowed in commands: + self.assertEqual( + allowed, + [ + self.sweep.PASS, + base.amdsmi.AmdSmiStatus.NOT_SUPPORTED, + base.AmdSmiExitCode.INVALID_PARAMETER_VALUE, + ], + ) + + def test_unsupported_clock_does_not_hide_the_other_gpu(self) -> None: + self.sweep.clk_freq = [ + {"FCLK": {"num_supported": 3, "has_deep_sleep": False}}, + {"FCLK": None}, + ] + commands = self._commands(["--clk-level {clk_level_fclk}"]) + self.assertEqual( + [command for command, _ in commands], + ["amd-smi set --clk-level FCLK 0 1 2 --loglevel DEBUG --gpu 0"], + ) + + def test_mixed_supported_and_unsupported_devices_pass_individually(self) -> None: + commands = self._commands(["--fan 50"]) + self.sweep.util = types.SimpleNamespace( + RunCmdSync=Mock(side_effect=[(0, "", ""), (2, "", "")]) + ) + self.sweep.RunCmds(commands) + self.assertEqual(self.sweep.util.RunCmdSync.call_count, 2) + + def test_io_and_aggregate_errors_are_not_accepted(self) -> None: + commands = self._commands(["--fan 50"]) + for code in (12, 204): + with self.subTest(code=code): + self.sweep.util = types.SimpleNamespace( + RunCmdSync=Mock(side_effect=[(0, "", ""), (code, "", "")]) + ) + with self.assertRaisesRegex(AssertionError, f"--gpu 1.*rc={code}"): + self.sweep.RunCmds(commands) + self.assertEqual(self.sweep.util.RunCmdSync.call_count, 2) + + def test_read_sweep_keeps_all_gpu_coverage(self) -> None: + commands = self._commands(["--usage"], command="metric") + self.assertIn( + "amd-smi metric --usage --loglevel DEBUG", [command for command, _ in commands] + ) + self.assertIn( + "amd-smi metric --usage --gpu all --loglevel DEBUG", + [command for command, _ in commands], + ) + + def test_cleanup_does_not_claim_to_restore_memory_partitions(self) -> None: + self.sweep.metric_data = { + "gpu_data": [ + {"fan": {"max": "N/A", "speed": "N/A"}, "clock": {}, "perf_level": "N/A"} + for _ in range(2) + ] + } + self.sweep.static_data = { + "gpu_data": [ + { + "clock": {"sys": "N/A"}, + "partition": {"compute_partition_mem_alloc_mode": "N/A"}, + "soc_pstate": "N/A", + "xgmi_plpd": "N/A", + "limit": {"ptl_state": "N/A", "ptl_format": "N/A"}, + "process_isolation": "N/A", + } + for _ in range(2) + ] + } + self.sweep.partition_data = { + "current_partition": [{"memory": "NPS1", "accelerator_type": "N/A"} for _ in range(2)] + } + self.sweep.RunCmds = Mock() + test_set.TestSet._restore_starting_values(self.sweep) + commands = self.sweep.RunCmds.call_args[0][0] + self.assertTrue(commands) + self.assertFalse(any("--memory-partition" in command for command, _ in commands)) diff --git a/projects/amdsmi/tests/python/unit/system/test_cli_exit_codes.py b/projects/amdsmi/tests/python/unit/system/test_cli_exit_codes.py index 9dcb55846919..21e61a0dc8cb 100644 --- a/projects/amdsmi/tests/python/unit/system/test_cli_exit_codes.py +++ b/projects/amdsmi/tests/python/unit/system/test_cli_exit_codes.py @@ -2256,6 +2256,73 @@ def test_gpu_od_success_records_nothing(self): class TestResetGpuGAllFailureGuards(unittest.TestCase): """A per-device GPU `reset` failure must be recorded and must NOT crash.""" + def test_missing_fclk_table_keeps_existing_reset_output(self): + reset = _load_reset() + cmd = _make_reset_cmd(reset) + patches = _build_reset_specs(reset)["clocks"](cmd, "success") + patches["amdsmi_get_clk_freq"] = _raise_not_supported + with _patch_amdsmi_interface(reset, **patches): + cmd.reset(_reset_args(reset, clocks=True)) + self.assertEqual(cmd.helpers.error_collector.resolve_exit_code(), 0) + result = next(value for _, key, value in cmd.logger.stored if key == "reset_clocks") + self.assertEqual(set(result), {"overdrive", "clocks", "performance"}) + + def test_fclk_read_failure_is_reported(self): + reset = _load_reset() + cmd = _make_reset_cmd(reset) + + def fail_read(*args): + raise _DriverlessLibErr(amdsmi_wrapper.AMDSMI_STATUS_IO) + + patches = _build_reset_specs(reset)["clocks"](cmd, "success") + patches["amdsmi_get_clk_freq"] = fail_read + with _patch_amdsmi_interface(reset, **patches): + cmd.reset(_reset_args(reset, clocks=True)) + self.assertEqual( + cmd.helpers.error_collector.resolve_exit_code(), amdsmi_wrapper.AMDSMI_STATUS_IO + ) + result = next(value for _, key, value in cmd.logger.stored if key == "reset_clocks") + self.assertIn("Unable to restore FCLK cap", result["fclk"]) + + def test_fclk_permission_failure_requires_elevation(self): + reset = _load_reset() + + def no_permission(*args): + raise _DriverlessLibErr(amdsmi_wrapper.AMDSMI_STATUS_NO_PERM) + + for operation in ("amdsmi_get_clk_freq", "amdsmi_set_gpu_clk_limit"): + with self.subTest(operation=operation): + cmd = _make_reset_cmd(reset) + patches = _build_reset_specs(reset)["clocks"](cmd, "success") + patches["amdsmi_get_clock_info"] = lambda *_: {"max_clk": 1550} + patches[operation] = no_permission + with _patch_amdsmi_interface(reset, **patches): + with self.assertRaisesRegex(PermissionError, "requires elevation"): + cmd.reset(_reset_args(reset, clocks=True)) + + def test_fclk_write_failure_is_reported_and_auto_reset_continues(self): + reset = _load_reset() + for code in (amdsmi_wrapper.AMDSMI_STATUS_NOT_SUPPORTED, amdsmi_wrapper.AMDSMI_STATUS_IO): + with self.subTest(code=code): + cmd = _make_reset_cmd(reset) + perf_writes = [] + + def fail_write(*args): + raise _DriverlessLibErr(code) + + patches = _build_reset_specs(reset)["clocks"](cmd, "success") + patches.update( + amdsmi_get_clock_info=lambda *_: {"max_clk": 1550}, + amdsmi_set_gpu_clk_limit=fail_write, + amdsmi_set_gpu_perf_level=lambda *args: perf_writes.append(args), + ) + with _patch_amdsmi_interface(reset, **patches): + cmd.reset(_reset_args(reset, clocks=True)) + self.assertEqual(cmd.helpers.error_collector.resolve_exit_code(), code) + result = next(value for _, key, value in cmd.logger.stored if key == "reset_clocks") + self.assertIn("Unable to restore FCLK cap", result["fclk"]) + self.assertEqual(len(perf_writes), 2) + def test_every_reset_option_is_driven(self): """Forcing function: every per-device GPU `reset` option must have a driver in _build_reset_specs. Adding a new option without a spec fails From 6ca6edbe6442973a3f99e916d84835b76b4b54ee Mon Sep 17 00:00:00 2001 From: Koushik Billakanti Date: Tue, 27 Oct 2026 00:30:10 -0500 Subject: [PATCH 3/8] fix(amdsmi): reject invalid clock units Signed-off-by: Koushik Billakanti --- projects/amdsmi/rocm_smi/src/rocm_smi.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/projects/amdsmi/rocm_smi/src/rocm_smi.cc b/projects/amdsmi/rocm_smi/src/rocm_smi.cc index da04402990dc..79c8d98d3105 100644 --- a/projects/amdsmi/rocm_smi/src/rocm_smi.cc +++ b/projects/amdsmi/rocm_smi/src/rocm_smi.cc @@ -96,7 +96,6 @@ static uint64_t get_multiplier_from_char(char units_char) { break; default: - assert(false); // Unexpected units for frequency throw amd::smi::rsmi_exception(RSMI_STATUS_UNEXPECTED_DATA, __FUNCTION__); } return multiplier; From d46912ea789bbb1ee318159a2e5622a9c0aca2ee Mon Sep 17 00:00:00 2001 From: Koushik Billakanti Date: Tue, 27 Oct 2026 01:34:19 -0500 Subject: [PATCH 4/8] fix(amdsmi): avoid stale clock restores Signed-off-by: Koushik Billakanti --- projects/amdsmi/tests/python/README.md | 5 ++ projects/amdsmi/tests/python/cli/base.py | 2 - projects/amdsmi/tests/python/cli/test_set.py | 23 +------- .../python/unit/gpu/test_cli_set_sweep.py | 56 ++++++++++++++++++- 4 files changed, 63 insertions(+), 23 deletions(-) diff --git a/projects/amdsmi/tests/python/README.md b/projects/amdsmi/tests/python/README.md index b56fd477e626..79f2453bd95a 100644 --- a/projects/amdsmi/tests/python/README.md +++ b/projects/amdsmi/tests/python/README.md @@ -93,6 +93,11 @@ partition. Test NPS transitions only in a separate, controlled driver-reload workflow; the generic sweep neither reloads the driver nor clears changes queued before the test. +Clock cleanup uses `reset --clocks` rather than replaying captured limits that +can conflict with the sweep's current bounds. It resets clocks to defaults, +then reapplies each GPU's saved performance level; custom clock limits are +not preserved. + ### Invocation From an installed package (recommended — avoids a shadowing `site-packages` copy): diff --git a/projects/amdsmi/tests/python/cli/base.py b/projects/amdsmi/tests/python/cli/base.py index 5c9083c9594f..ebe615c60876 100644 --- a/projects/amdsmi/tests/python/cli/base.py +++ b/projects/amdsmi/tests/python/cli/base.py @@ -358,8 +358,6 @@ def __init__(self, *args, **kwargs): self.memory_partition_modes = ["NPS1", "NPS2", "NPS4", "NPS8"] self.power_types = ["ppt0", "ppt1"] self.ptl_formats = ["I8", "F16", "BF16", "F32", "F64", "F8", "VECTOR"] - self.clk_limits = ["SCLK", "MCLK", "FCLK"] - self.limit_types = ["MAX", "MIN"] self.clk_levels = ["SCLK", "MCLK", "FCLK", "SOCCLK", "PCIE"] # When parsing, ignore these entries as they are abnormal diff --git a/projects/amdsmi/tests/python/cli/test_set.py b/projects/amdsmi/tests/python/cli/test_set.py index 6b2e73786e65..adba27a31b0a 100644 --- a/projects/amdsmi/tests/python/cli/test_set.py +++ b/projects/amdsmi/tests/python/cli/test_set.py @@ -135,25 +135,9 @@ def _restore_starting_values(self): # TODO: get the right ptl-format cmds.append((f"amd-smi set --ptl-format {ptl_format} --gpu {index}", self.PASS)) - # set --clk-limit defaults - clock = self.metric_data["gpu_data"][index]["clock"] - for clk_type in self.clk_limits: - for limit_type in self.limit_types: - value = self._clk_limit_value(clock, clk_type, limit_type) - if value is not None: - # A readable min/max clock does not imply the device - # supports setting a limit on it. - cmds.append( - ( - f"amd-smi set --clk-limit {clk_type} {limit_type} {value} --gpu {index}", - [self.PASS, amdsmi.AmdSmiStatus.NOT_SUPPORTED], - ) - ) - # reset --clocks - # A clk-level write is a bitmask, so replaying the recorded level would pin DPM - # to that one level; only AUTO hands every level back. Overdrive is reset too, - # and reports NOT_SUPPORTED on parts that lack it even though the rest worked. + # Reset clears clock masks and limits; replaying stale bounds first can fail. + # Overdrive may be unsupported even when the clock reset succeeds. cmds.append( ( f"amd-smi reset --clocks --gpu {index}", @@ -162,8 +146,7 @@ def _restore_starting_values(self): ) # set --perf-level defaults - # Last of the clock writes: the reset above forces AUTO, and --clk-limit and - # --perf-determinism each leave the device on a level of their own. + # Reset forces AUTO, so restore the saved performance level last. perf_level = self.metric_data["gpu_data"][index]["perf_level"] if perf_level not in ("N/A", "AMDSMI_DEV_PERF_LEVEL_UNKNOWN"): perf_level = _strip_prefix(perf_level, "AMDSMI_DEV_PERF_LEVEL_") diff --git a/projects/amdsmi/tests/python/unit/gpu/test_cli_set_sweep.py b/projects/amdsmi/tests/python/unit/gpu/test_cli_set_sweep.py index 87fbee7e2bf5..ee6908c82e65 100644 --- a/projects/amdsmi/tests/python/unit/gpu/test_cli_set_sweep.py +++ b/projects/amdsmi/tests/python/unit/gpu/test_cli_set_sweep.py @@ -150,7 +150,7 @@ def test_read_sweep_keeps_all_gpu_coverage(self) -> None: [command for command, _ in commands], ) - def test_cleanup_does_not_claim_to_restore_memory_partitions(self) -> None: + def _prepare_cleanup(self) -> None: self.sweep.metric_data = { "gpu_data": [ {"fan": {"max": "N/A", "speed": "N/A"}, "clock": {}, "perf_level": "N/A"} @@ -173,8 +173,62 @@ def test_cleanup_does_not_claim_to_restore_memory_partitions(self) -> None: self.sweep.partition_data = { "current_partition": [{"memory": "NPS1", "accelerator_type": "N/A"} for _ in range(2)] } + + def test_cleanup_does_not_claim_to_restore_memory_partitions(self) -> None: + self._prepare_cleanup() self.sweep.RunCmds = Mock() test_set.TestSet._restore_starting_values(self.sweep) commands = self.sweep.RunCmds.call_args[0][0] self.assertTrue(commands) self.assertFalse(any("--memory-partition" in command for command, _ in commands)) + + def test_cleanup_resets_clocks_instead_of_replaying_stale_limits(self) -> None: + self._prepare_cleanup() + self.sweep.metric_data["gpu_data"][0]["clock"] = { + "gfx_0": {"min_clk": {"value": 500}, "max_clk": {"value": 500}} + } + self.sweep.static_data["gpu_data"][0]["clock"]["sys"] = { + "frequency_levels": {"Level 0": {"value": 500}, "Level 1": {"value": 1700}} + } + for gpu, level in enumerate(("AUTO", "HIGH")): + self.sweep.metric_data["gpu_data"][gpu]["perf_level"] = f"AMDSMI_DEV_PERF_LEVEL_{level}" + self.sweep.util = types.SimpleNamespace( + RunCmdSync=Mock( + side_effect=lambda cmd, **kwargs: ( + 198 if cmd == "amd-smi set --clk-limit SCLK MAX 500 --gpu 0" else 0, + "", + "", + ) + ) + ) + + test_set.TestSet._restore_starting_values(self.sweep) + + commands = [call[0][0] for call in self.sweep.util.RunCmdSync.call_args_list] + self.assertFalse(any("--clk-limit" in command for command in commands)) + for gpu, level in enumerate(("AUTO", "HIGH")): + self.assertLess( + commands.index(f"amd-smi reset --clocks --gpu {gpu}"), + commands.index(f"amd-smi set --perf-level {level} --gpu {gpu}"), + ) + + def test_cleanup_keeps_strict_reset_statuses_and_attempts_every_gpu(self) -> None: + for code in (0, 2, 12, 198, 204): + with self.subTest(code=code): + self._prepare_cleanup() + self.sweep.util = types.SimpleNamespace( + RunCmdSync=Mock( + side_effect=lambda cmd, **kwargs: ( + code if cmd == "amd-smi reset --clocks --gpu 0" else 0, + "", + "", + ) + ) + ) + if code in (0, 2): + test_set.TestSet._restore_starting_values(self.sweep) + else: + with self.assertRaisesRegex(AssertionError, f"rc={code}"): + test_set.TestSet._restore_starting_values(self.sweep) + commands = [call[0][0] for call in self.sweep.util.RunCmdSync.call_args_list] + self.assertIn("amd-smi reset --clocks --gpu 1", commands) From e93e89f6de25a1d9fddeddbdf9afbb853f4459ae Mon Sep 17 00:00:00 2001 From: Koushik Billakanti Date: Tue, 27 Oct 2026 11:53:22 -0500 Subject: [PATCH 5/8] fix(amdsmi): preserve empty DPM errors Signed-off-by: Koushik Billakanti --- projects/amdsmi/CHANGELOG.md | 4 +- .../amdsmi/docs/how-to/amdsmi-cli-tool.md | 6 +-- projects/amdsmi/rocm_smi/src/rocm_smi.cc | 5 --- projects/amdsmi/src/amd_smi/amd_smi_utils.cc | 5 +-- .../amd_smi_test/unit/gpu/clock_range_test.cc | 38 +++++++++---------- projects/amdsmi/tests/python/README.md | 1 + .../python/unit/system/test_cli_exit_codes.py | 30 ++++++++------- 7 files changed, 44 insertions(+), 45 deletions(-) diff --git a/projects/amdsmi/CHANGELOG.md b/projects/amdsmi/CHANGELOG.md index 16ec29a62b9a..e641fcaf8c45 100644 --- a/projects/amdsmi/CHANGELOG.md +++ b/projects/amdsmi/CHANGELOG.md @@ -203,8 +203,8 @@ Full documentation for amd_smi_lib is available at [https://rocm.docs.amd.com/pr - **Fixed GPU metrics returning no data on integrated GPUs**. - `amdsmi_get_gpu_metrics_info()` and `amdsmi_get_gpu_activity()` failed on APUs such as gfx1103, leaving PLX temperature, GPU utilization and `amd-smi metric` empty. Those devices now report the values they provide with the rest marked N/A, and any GPU without a PLX sensor now reports N/A there instead of a placeholder reading. -- **Fixed clock queries and settings on GPUs without a clock table**. - - Unsupported clocks now report not supported instead of a data error, while malformed tables and read failures remain errors. +- **Preserved errors from empty GPU clock tables**. + - Clock queries retain errors for empty or blank driver data instead of reporting the feature as unsupported. - **Fixed `amd-smi reset --clocks` leaving a lowered FCLK cap in place**. - The reset restores the top supported FCLK level and reports restoration failures instead of hiding them. diff --git a/projects/amdsmi/docs/how-to/amdsmi-cli-tool.md b/projects/amdsmi/docs/how-to/amdsmi-cli-tool.md index 4aa0882b7548..70711809c923 100644 --- a/projects/amdsmi/docs/how-to/amdsmi-cli-tool.md +++ b/projects/amdsmi/docs/how-to/amdsmi-cli-tool.md @@ -906,9 +906,9 @@ Command Modifiers: ``` `reset --clocks` also restores a lowered FCLK cap to the highest supported DPM -level. GPUs without FCLK information skip this step. If restoring the cap fails, -the command reports the failure in `reset_clocks.fclk` and its exit status while -still attempting to reset the performance level. +level. Unsupported FCLK queries skip this step. Clock-query and cap-restoration +errors are reported in `reset_clocks.fclk` and the exit status while the command +still attempts to reset the performance level. (cmd-monitor)= ### amd-smi monitor diff --git a/projects/amdsmi/rocm_smi/src/rocm_smi.cc b/projects/amdsmi/rocm_smi/src/rocm_smi.cc index 79c8d98d3105..68cbc39f8efb 100644 --- a/projects/amdsmi/rocm_smi/src/rocm_smi.cc +++ b/projects/amdsmi/rocm_smi/src/rocm_smi.cc @@ -1359,11 +1359,6 @@ static rsmi_status_t get_frequencies(amd::smi::DevInfoTypes type, rsmi_clk_type_ f->current = 0; ret = GetDevValueVec(type, dv_ind, &val_vec); - // An empty pp_dpm_* file (e.g. FCLK on an iGPU) lists no levels, the same as a - // missing one. - if (ret == RSMI_STATUS_UNEXPECTED_DATA) { - return RSMI_STATUS_NOT_SUPPORTED; - } if (ret != RSMI_STATUS_SUCCESS) { return ret; } diff --git a/projects/amdsmi/src/amd_smi/amd_smi_utils.cc b/projects/amdsmi/src/amd_smi/amd_smi_utils.cc index ec5cebf4dea2..30e7ccc20b1c 100644 --- a/projects/amdsmi/src/amd_smi/amd_smi_utils.cc +++ b/projects/amdsmi/src/amd_smi/amd_smi_utils.cc @@ -357,7 +357,6 @@ amdsmi_status_t smi_amdgpu_parse_dpm_ranges(std::istream& dpm_stream, for (std::string line; getline(dpm_stream, line);) { unsigned int dpm_level, freq; - // A clock without a DPM table reads back as a bare newline. if (line.find_first_not_of(" \t\r") == std::string::npos) { continue; } @@ -398,9 +397,9 @@ amdsmi_status_t smi_amdgpu_parse_dpm_ranges(std::istream& dpm_stream, if (dpm_stream.bad() || (dpm_stream.fail() && !dpm_stream.eof())) { return AMDSMI_STATUS_IO; } - // Nothing to report a range from: answer as for a missing pp_dpm_* file. + // Empty data does not establish whether the clock is supported. if (!has_levels && !od_range.present) { - return AMDSMI_STATUS_NOT_SUPPORTED; + return AMDSMI_STATUS_IO; } if (dpm == 0 && current_freq > 0) { // if the dpm level is 0, then the current frequency is the min/max frequency diff --git a/projects/amdsmi/tests/amd_smi_test/unit/gpu/clock_range_test.cc b/projects/amdsmi/tests/amd_smi_test/unit/gpu/clock_range_test.cc index 67fd3f91ba76..89d46a8e44b4 100644 --- a/projects/amdsmi/tests/amd_smi_test/unit/gpu/clock_range_test.cc +++ b/projects/amdsmi/tests/amd_smi_test/unit/gpu/clock_range_test.cc @@ -298,22 +298,17 @@ TEST(GpuUnit, DpmRangesUnavailableSleepSentinelNotRejected) { EXPECT_EQ(ranges.sleep_state_freq, -1); } -// An empty pp_dpm_* stream with no overdrive range has no range to report, so it -// answers as a missing pp_dpm_* file does. -TEST(GpuUnit, DpmRangesEmptyStreamNotSupported) { - std::istringstream dpm(""); - SmiAmdgpuOdClkRange od_range; // not present -> derive from the (absent) levels - SmiAmdgpuClkRanges ranges; - EXPECT_EQ(smi_amdgpu_parse_dpm_ranges(dpm, od_range, ranges), AMDSMI_STATUS_NOT_SUPPORTED); -} - -// The amdgpu driver prints a bare newline for a clock without a DPM table, e.g. -// pp_dpm_fclk on a Raphael iGPU. That line is not a malformed level. -TEST(GpuUnit, DpmRangesBlankLineNotSupported) { - std::istringstream dpm("\n"); - SmiAmdgpuOdClkRange od_range; - SmiAmdgpuClkRanges ranges; - EXPECT_EQ(smi_amdgpu_parse_dpm_ranges(dpm, od_range, ranges), AMDSMI_STATUS_NOT_SUPPORTED); +TEST(GpuUnit, DpmRangesEmptyTablesReportIoError) { + for (const char* contents : {"", "\n", " \t\r\n", "\n\n"}) { + std::istringstream dpm(contents); + SmiAmdgpuOdClkRange od_range; + SmiAmdgpuClkRanges ranges{1800, 1300, 2, 400}; + EXPECT_EQ(smi_amdgpu_parse_dpm_ranges(dpm, od_range, ranges), AMDSMI_STATUS_IO); + EXPECT_EQ(ranges.max_freq, 1800); + EXPECT_EQ(ranges.min_freq, 1300); + EXPECT_EQ(ranges.num_dpm, 2); + EXPECT_EQ(ranges.sleep_state_freq, 400); + } } // With an overdrive range the domain still has a range to report. @@ -352,13 +347,18 @@ TEST(GpuUnit, DpmFrequencyMissingTableNotSupported) { RSMI_STATUS_NOT_SUPPORTED); } -TEST(GpuUnit, DpmFrequencyEmptyTablesNotSupported) { +TEST(GpuUnit, DpmFrequencyEmptyTablesPreserveDataError) { FakeDpmDevice device; for (const char* contents : {"", "\n", " \t\r\n", "\n\n"}) { - device.WriteTable(contents); + device.WriteTable("0: 1300Mhz *\n"); rsmi_frequencies_t frequencies{}; + ASSERT_EQ(rsmi_dev_gpu_clk_freq_get(device.index(), RSMI_CLK_TYPE_DF, &frequencies), + RSMI_STATUS_SUCCESS); + ASSERT_EQ(frequencies.num_supported, 1u); + + device.WriteTable(contents); EXPECT_EQ(rsmi_dev_gpu_clk_freq_get(device.index(), RSMI_CLK_TYPE_DF, &frequencies), - RSMI_STATUS_NOT_SUPPORTED); + RSMI_STATUS_UNEXPECTED_DATA); EXPECT_EQ(frequencies.num_supported, 0u); } } diff --git a/projects/amdsmi/tests/python/README.md b/projects/amdsmi/tests/python/README.md index 79f2453bd95a..19cb68e7d8eb 100644 --- a/projects/amdsmi/tests/python/README.md +++ b/projects/amdsmi/tests/python/README.md @@ -85,6 +85,7 @@ they all delegate to `common.run_test_dir()`. The `set` sweep runs each setting with a numeric `--gpu` selector and derives clock and power values from that GPU. Each device's exit code is checked independently; aggregate errors are not added to the allowed codes. +Empty DPM data remains an error and is not added to the sweep's allowed statuses. The sweep explicitly excludes memory-partition changes. A request can remain queued until driver reload, and requesting the currently active mode does not diff --git a/projects/amdsmi/tests/python/unit/system/test_cli_exit_codes.py b/projects/amdsmi/tests/python/unit/system/test_cli_exit_codes.py index 21e61a0dc8cb..86460d10f1ef 100644 --- a/projects/amdsmi/tests/python/unit/system/test_cli_exit_codes.py +++ b/projects/amdsmi/tests/python/unit/system/test_cli_exit_codes.py @@ -2267,22 +2267,26 @@ def test_missing_fclk_table_keeps_existing_reset_output(self): result = next(value for _, key, value in cmd.logger.stored if key == "reset_clocks") self.assertEqual(set(result), {"overdrive", "clocks", "performance"}) - def test_fclk_read_failure_is_reported(self): + def test_fclk_read_failure_is_reported(self) -> None: reset = _load_reset() - cmd = _make_reset_cmd(reset) + for operation, code in ( + ("amdsmi_get_clk_freq", amdsmi_wrapper.AMDSMI_STATUS_IO), + ("amdsmi_get_clk_freq", amdsmi_wrapper.AMDSMI_STATUS_UNEXPECTED_DATA), + ("amdsmi_get_clock_info", amdsmi_wrapper.AMDSMI_STATUS_IO), + ): + with self.subTest(operation=operation, code=code): + cmd = _make_reset_cmd(reset) - def fail_read(*args): - raise _DriverlessLibErr(amdsmi_wrapper.AMDSMI_STATUS_IO) + def fail_read(*_args: object) -> None: + raise _DriverlessLibErr(code) - patches = _build_reset_specs(reset)["clocks"](cmd, "success") - patches["amdsmi_get_clk_freq"] = fail_read - with _patch_amdsmi_interface(reset, **patches): - cmd.reset(_reset_args(reset, clocks=True)) - self.assertEqual( - cmd.helpers.error_collector.resolve_exit_code(), amdsmi_wrapper.AMDSMI_STATUS_IO - ) - result = next(value for _, key, value in cmd.logger.stored if key == "reset_clocks") - self.assertIn("Unable to restore FCLK cap", result["fclk"]) + patches = _build_reset_specs(reset)["clocks"](cmd, "success") + patches[operation] = fail_read + with _patch_amdsmi_interface(reset, **patches): + cmd.reset(_reset_args(reset, clocks=True)) + self.assertEqual(cmd.helpers.error_collector.resolve_exit_code(), code) + result = next(value for _, key, value in cmd.logger.stored if key == "reset_clocks") + self.assertIn("Unable to restore FCLK cap", result["fclk"]) def test_fclk_permission_failure_requires_elevation(self): reset = _load_reset() From a0553405541786b09e0597d61b47ee01d2505d01 Mon Sep 17 00:00:00 2001 From: Koushik Billakanti Date: Sun, 1 Nov 2026 19:12:24 -0600 Subject: [PATCH 6/8] fix(amdsmi): guard FCLK restoration Signed-off-by: Koushik Billakanti --- .../amdsmi/amdsmi_cli/subcommands/reset.py | 6 ++++- .../amdsmi/docs/how-to/amdsmi-cli-tool.md | 3 ++- projects/amdsmi/tests/python/cli/base.py | 6 ++--- .../python/unit/gpu/test_cli_set_sweep.py | 6 +++++ .../python/unit/system/test_cli_exit_codes.py | 26 +++++++++++++++++++ 5 files changed, 41 insertions(+), 6 deletions(-) diff --git a/projects/amdsmi/amdsmi_cli/subcommands/reset.py b/projects/amdsmi/amdsmi_cli/subcommands/reset.py index b1e2b1fd875f..e976d40d34e8 100644 --- a/projects/amdsmi/amdsmi_cli/subcommands/reset.py +++ b/projects/amdsmi/amdsmi_cli/subcommands/reset.py @@ -29,7 +29,11 @@ def _restore_fclk_cap(gpu): top_mhz = max((hz for hz in levels if hz > 0), default=0) // 1_000_000 if not top_mhz or not isinstance(current_max, int) or current_max >= top_mhz: return False - amdsmi_interface.amdsmi_set_gpu_clk_limit(gpu, "fclk", "max", top_mhz) + try: + amdsmi_interface.amdsmi_set_gpu_clk_limit(gpu, "fclk", "max", top_mhz) + except amdsmi_exception.AmdSmiLibraryException as e: + logging.debug("Failed to restore FCLK cap to %s MHz | %s", top_mhz, e.get_error_info()) + raise return True def reset( diff --git a/projects/amdsmi/docs/how-to/amdsmi-cli-tool.md b/projects/amdsmi/docs/how-to/amdsmi-cli-tool.md index 70711809c923..a6531ccfa8e9 100644 --- a/projects/amdsmi/docs/how-to/amdsmi-cli-tool.md +++ b/projects/amdsmi/docs/how-to/amdsmi-cli-tool.md @@ -906,7 +906,8 @@ Command Modifiers: ``` `reset --clocks` also restores a lowered FCLK cap to the highest supported DPM -level. Unsupported FCLK queries skip this step. Clock-query and cap-restoration +level. Unsupported FCLK queries skip this step; an unsupported cap write remains +a reset failure. Clock-query and cap-restoration errors are reported in `reset_clocks.fclk` and the exit status while the command still attempts to reset the performance level. diff --git a/projects/amdsmi/tests/python/cli/base.py b/projects/amdsmi/tests/python/cli/base.py index ebe615c60876..d12553ae45ad 100644 --- a/projects/amdsmi/tests/python/cli/base.py +++ b/projects/amdsmi/tests/python/cli/base.py @@ -133,7 +133,7 @@ class TestCliBase(unittest.TestCase): } # TODO(amdsmi_team): User input is not a problem, we just need to test above ^ - PROMPT_ANSWERS = {"set": {"--fan": "y\n", "--memory-partition": "y\n"}} + PROMPT_ANSWERS = {"set": {"--fan": "y\n"}} # Commands whose sweep also accepts NOT_SUPPORTED. A write may legitimately # refuse a feature the ASIC lacks, but a read answering NOT_SUPPORTED is a @@ -142,12 +142,10 @@ class TestCliBase(unittest.TestCase): # Exit codes a single argument may also answer with, beyond NOT_SUPPORTED SWEEP_TOLERATED_CODES = { - # Not all ASICs support setting all modes, which is an invalid set option - "set": {"--memory-partition": amdsmi.AmdSmiStatus.INVAL}, # This reset is a read -- the kernel clears xgmi_error when it is read -- # so the read's errno becomes the reset's status, and some drivers reject # it with EINVAL. - "reset": {"--xgmierr": amdsmi.AmdSmiStatus.INVAL}, + "reset": {"--xgmierr": amdsmi.AmdSmiStatus.INVAL} } # Order for the amd-smi reset commands, putting --clocks last since diff --git a/projects/amdsmi/tests/python/unit/gpu/test_cli_set_sweep.py b/projects/amdsmi/tests/python/unit/gpu/test_cli_set_sweep.py index ee6908c82e65..dd3792c3f07a 100644 --- a/projects/amdsmi/tests/python/unit/gpu/test_cli_set_sweep.py +++ b/projects/amdsmi/tests/python/unit/gpu/test_cli_set_sweep.py @@ -57,6 +57,12 @@ def test_memory_partition_changes_are_explicitly_excluded(self) -> None: self.assertEqual(len(exclusions), 1) self.assertIn("cannot cancel", exclusions[0]) + def test_excluded_memory_partition_has_no_sweep_metadata(self) -> None: + command = "amd-smi set --memory-partition NPS2 --gpu 0" + for name in ("PROMPT_ANSWERS", "SWEEP_TOLERATED_CODES"): + with self.subTest(table=name): + self.assertIsNone(self.sweep._lookup(getattr(self.sweep, name), command)) + def test_clock_values_come_from_the_selected_gpu(self) -> None: self.sweep.clk_freq = [ {"FCLK": {"num_supported": 3, "has_deep_sleep": False}}, diff --git a/projects/amdsmi/tests/python/unit/system/test_cli_exit_codes.py b/projects/amdsmi/tests/python/unit/system/test_cli_exit_codes.py index 86460d10f1ef..78645b0d1feb 100644 --- a/projects/amdsmi/tests/python/unit/system/test_cli_exit_codes.py +++ b/projects/amdsmi/tests/python/unit/system/test_cli_exit_codes.py @@ -2256,6 +2256,32 @@ def test_gpu_od_success_records_nothing(self): class TestResetGpuGAllFailureGuards(unittest.TestCase): """A per-device GPU `reset` failure must be recorded and must NOT crash.""" + def test_fclk_setter_logs_target_and_preserves_library_error(self) -> None: + reset = _load_reset() + for code in ( + amdsmi_wrapper.AMDSMI_STATUS_NOT_SUPPORTED, + amdsmi_wrapper.AMDSMI_STATUS_IO, + amdsmi_wrapper.AMDSMI_STATUS_NO_PERM, + ): + with self.subTest(code=code): + error = _DriverlessLibErr(code) + + def fail_write(*_args: object) -> None: + raise error + + with _patch_amdsmi_interface( + reset, + amdsmi_get_clk_freq=lambda *_: {"frequency": [1300000000, 1800000000]}, + amdsmi_get_clock_info=lambda *_: {"max_clk": 1550}, + amdsmi_set_gpu_clk_limit=fail_write, + ): + with self.assertLogs(level="DEBUG") as logs: + with self.assertRaises(_DriverlessLibErr) as caught: + reset.ResetCommands._restore_fclk_cap("gpu0") + self.assertIs(caught.exception, error) + self.assertIn("Failed to restore FCLK cap to 1800 MHz", logs.output[0]) + self.assertIn(error.get_error_info(), logs.output[0]) + def test_missing_fclk_table_keeps_existing_reset_output(self): reset = _load_reset() cmd = _make_reset_cmd(reset) From 6076bf2865799fcbb840a55e4ae9d35de37862a9 Mon Sep 17 00:00:00 2001 From: Koushik Billakanti Date: Tue, 3 Nov 2026 14:14:24 -0600 Subject: [PATCH 7/8] test(amdsmi): narrow DPM coverage Signed-off-by: Koushik Billakanti --- projects/amdsmi/CHANGELOG.md | 6 - .../amdsmi/amdsmi_cli/subcommands/reset.py | 34 --- .../amdsmi/docs/how-to/amdsmi-cli-tool.md | 6 - .../amd_smi/impl/amd_smi_test_overrides.h | 4 +- projects/amdsmi/rocm_smi/src/rocm_smi.cc | 1 + .../amdsmi/rocm_smi/src/rocm_smi_device.cc | 5 - projects/amdsmi/src/amd_smi/amd_smi_utils.cc | 15 +- .../amd_smi_test/unit/gpu/clock_range_test.cc | 178 ++++++------- projects/amdsmi/tests/python/README.md | 17 -- projects/amdsmi/tests/python/cli/base.py | 25 +- projects/amdsmi/tests/python/cli/test_set.py | 33 ++- .../unit/gpu/test_cli_reset_clocks_fclk.py | 123 --------- .../python/unit/gpu/test_cli_set_sweep.py | 240 ------------------ .../python/unit/system/test_cli_exit_codes.py | 111 +------- 14 files changed, 139 insertions(+), 659 deletions(-) delete mode 100644 projects/amdsmi/tests/python/unit/gpu/test_cli_reset_clocks_fclk.py delete mode 100644 projects/amdsmi/tests/python/unit/gpu/test_cli_set_sweep.py diff --git a/projects/amdsmi/CHANGELOG.md b/projects/amdsmi/CHANGELOG.md index e641fcaf8c45..0382f674e1fd 100644 --- a/projects/amdsmi/CHANGELOG.md +++ b/projects/amdsmi/CHANGELOG.md @@ -203,12 +203,6 @@ Full documentation for amd_smi_lib is available at [https://rocm.docs.amd.com/pr - **Fixed GPU metrics returning no data on integrated GPUs**. - `amdsmi_get_gpu_metrics_info()` and `amdsmi_get_gpu_activity()` failed on APUs such as gfx1103, leaving PLX temperature, GPU utilization and `amd-smi metric` empty. Those devices now report the values they provide with the rest marked N/A, and any GPU without a PLX sensor now reports N/A there instead of a placeholder reading. -- **Preserved errors from empty GPU clock tables**. - - Clock queries retain errors for empty or blank driver data instead of reporting the feature as unsupported. - -- **Fixed `amd-smi reset --clocks` leaving a lowered FCLK cap in place**. - - The reset restores the top supported FCLK level and reports restoration failures instead of hiding them. - - **Fixed `amd-smi` printing a Python traceback when an unknown NIC or switch is selected**. - `amd-smi static --nic 999` and `--switch 999` failed while building the "device not found" error, so the command exited `1` with a traceback and no readable message. `--json` and `--csv` produced no parseable output. - Both now report `Can not find a device: NIC '999'` (or `SWITCH`) and exit `196`, matching `--gpu`, `--cpu`, and `--core`. diff --git a/projects/amdsmi/amdsmi_cli/subcommands/reset.py b/projects/amdsmi/amdsmi_cli/subcommands/reset.py index e976d40d34e8..bad21cc0c0f9 100644 --- a/projects/amdsmi/amdsmi_cli/subcommands/reset.py +++ b/projects/amdsmi/amdsmi_cli/subcommands/reset.py @@ -14,28 +14,6 @@ class ResetCommands: - @staticmethod - def _restore_fclk_cap(gpu): - """Return whether a lowered FCLK cap was raised; AUTO alone leaves it set.""" - clk_type = amdsmi_interface.AmdSmiClkType.DF - try: - levels = amdsmi_interface.amdsmi_get_clk_freq(gpu, clk_type)["frequency"] - current_max = amdsmi_interface.amdsmi_get_clock_info(gpu, clk_type)["max_clk"] - except amdsmi_exception.AmdSmiLibraryException as e: - if e.get_error_code() != amdsmi_interface.amdsmi_wrapper.AMDSMI_STATUS_NOT_SUPPORTED: - raise - logging.debug("FCLK cap is not available | %s", e.get_error_info()) - return False - top_mhz = max((hz for hz in levels if hz > 0), default=0) // 1_000_000 - if not top_mhz or not isinstance(current_max, int) or current_max >= top_mhz: - return False - try: - amdsmi_interface.amdsmi_set_gpu_clk_limit(gpu, "fclk", "max", top_mhz) - except amdsmi_exception.AmdSmiLibraryException as e: - logging.debug("Failed to restore FCLK cap to %s MHz | %s", top_mhz, e.get_error_info()) - raise - return True - def reset( self, args, @@ -251,18 +229,6 @@ def reset( ) self.helpers.error_collector.record_library_error(e.get_error_code()) # continue to reset clocks and performance level - try: - self._restore_fclk_cap(args.gpu) - except amdsmi_exception.AmdSmiLibraryException as e: - if e.get_error_code() == amdsmi_interface.amdsmi_wrapper.AMDSMI_STATUS_NO_PERM: - raise PermissionError("Command requires elevation") from e - reset_clocks_results["fclk"] = ( - f"[{e.get_error_info(detailed=False)}] Unable to restore FCLK cap" - ) - self.helpers.error_collector.record_library_error(e.get_error_code()) - logging.debug( - "Failed to restore fclk cap on gpu %s | %s", gpu_id, e.get_error_info() - ) try: level_auto = amdsmi_interface.AmdSmiDevPerfLevel.AUTO amdsmi_interface.amdsmi_set_gpu_perf_level(args.gpu, level_auto) diff --git a/projects/amdsmi/docs/how-to/amdsmi-cli-tool.md b/projects/amdsmi/docs/how-to/amdsmi-cli-tool.md index a6531ccfa8e9..4951bd627b46 100644 --- a/projects/amdsmi/docs/how-to/amdsmi-cli-tool.md +++ b/projects/amdsmi/docs/how-to/amdsmi-cli-tool.md @@ -905,12 +905,6 @@ Command Modifiers: DEBUG, INFO, WARNING, ERROR, CRITICAL ``` -`reset --clocks` also restores a lowered FCLK cap to the highest supported DPM -level. Unsupported FCLK queries skip this step; an unsupported cap write remains -a reset failure. Clock-query and cap-restoration -errors are reported in `reset_clocks.fclk` and the exit status while the command -still attempts to reset the performance level. - (cmd-monitor)= ### amd-smi monitor diff --git a/projects/amdsmi/include/amd_smi/impl/amd_smi_test_overrides.h b/projects/amdsmi/include/amd_smi/impl/amd_smi_test_overrides.h index 8b6a1863e94f..280f7badda4f 100644 --- a/projects/amdsmi/include/amd_smi/impl/amd_smi_test_overrides.h +++ b/projects/amdsmi/include/amd_smi/impl/amd_smi_test_overrides.h @@ -5,8 +5,8 @@ #include -// Library-local test seam for the sysfs root smi_amdgpu_get_enabled_blocks() -// (src/amd_smi/amd_smi_utils.cc) reads .../ras/features from. Defaults to the +// Library-local sysfs-root seam for clock ranges and RAS enabled blocks +// (src/amd_smi/amd_smi_utils.cc). Defaults to the // real "/sys/class/drm/" prefix -- e.g. production reads // "/sys/class/drm/card0/device/ras/features". A test that overrides this to // "/tmp/amdsmi_fake_sysfs_XXXXXX/" (see FakeSysfsTree in diff --git a/projects/amdsmi/rocm_smi/src/rocm_smi.cc b/projects/amdsmi/rocm_smi/src/rocm_smi.cc index 68cbc39f8efb..79efa031016d 100644 --- a/projects/amdsmi/rocm_smi/src/rocm_smi.cc +++ b/projects/amdsmi/rocm_smi/src/rocm_smi.cc @@ -96,6 +96,7 @@ static uint64_t get_multiplier_from_char(char units_char) { break; default: + assert(false); // Unexpected units for frequency throw amd::smi::rsmi_exception(RSMI_STATUS_UNEXPECTED_DATA, __FUNCTION__); } return multiplier; diff --git a/projects/amdsmi/rocm_smi/src/rocm_smi_device.cc b/projects/amdsmi/rocm_smi/src/rocm_smi_device.cc index 35ff92338d97..e33388b14fd4 100644 --- a/projects/amdsmi/rocm_smi/src/rocm_smi_device.cc +++ b/projects/amdsmi/rocm_smi/src/rocm_smi_device.cc @@ -1281,11 +1281,6 @@ int Device::readDevInfoMultiLineStr(DevInfoTypes type, std::vector* while (std::getline(fs, line)) { retVec->push_back(line); } - if (fs.bad() || (fs.fail() && !fs.eof())) { - ss << "Failed to read devInfoMultiLineStr for DevInfoType (" << get_type_string(type) << ")"; - LOG_ERROR(ss); - return EIO; - } fs.close(); if (retVec->empty()) { diff --git a/projects/amdsmi/src/amd_smi/amd_smi_utils.cc b/projects/amdsmi/src/amd_smi/amd_smi_utils.cc index 30e7ccc20b1c..bacb9394e4e1 100644 --- a/projects/amdsmi/src/amd_smi/amd_smi_utils.cc +++ b/projects/amdsmi/src/amd_smi/amd_smi_utils.cc @@ -353,15 +353,9 @@ amdsmi_status_t smi_amdgpu_parse_dpm_ranges(std::istream& dpm_stream, unsigned int current_freq = 0; char str[10]; char single_char; - bool has_levels = false; for (std::string line; getline(dpm_stream, line);) { unsigned int dpm_level, freq; - if (line.find_first_not_of(" \t\r") == std::string::npos) { - continue; - } - has_levels = true; - char firstChar = line[0]; if (firstChar == 'S') { if (sscanf(line.c_str(), "%c: %u%9s", &single_char, &sleep_freq, str) <= 2) { @@ -394,13 +388,6 @@ amdsmi_status_t smi_amdgpu_parse_dpm_ranges(std::istream& dpm_stream, dpm = dpm_level > dpm ? dpm_level : dpm; } } - if (dpm_stream.bad() || (dpm_stream.fail() && !dpm_stream.eof())) { - return AMDSMI_STATUS_IO; - } - // Empty data does not establish whether the clock is supported. - if (!has_levels && !od_range.present) { - return AMDSMI_STATUS_IO; - } if (dpm == 0 && current_freq > 0) { // if the dpm level is 0, then the current frequency is the min/max frequency max = current_freq; @@ -426,7 +413,7 @@ amdsmi_status_t smi_amdgpu_get_ranges(amd::smi::AMDSmiGPUDevice* device, amdsmi_ int* max_freq, int* min_freq, int* num_dpm, int* sleep_state_freq) { SMIGPUDEVICE_MUTEX(device->get_mutex()) - std::string fullpath = "/sys/class/drm/" + device->get_gpu_path() + "/device"; + std::string fullpath = smi_amdgpu_sysfs_drm_root() + device->get_gpu_path() + "/device"; std::string smclk_min_max_fullpath = ""; diff --git a/projects/amdsmi/tests/amd_smi_test/unit/gpu/clock_range_test.cc b/projects/amdsmi/tests/amd_smi_test/unit/gpu/clock_range_test.cc index 89d46a8e44b4..a5f6192ce65b 100644 --- a/projects/amdsmi/tests/amd_smi_test/unit/gpu/clock_range_test.cc +++ b/projects/amdsmi/tests/amd_smi_test/unit/gpu/clock_range_test.cc @@ -10,58 +10,24 @@ #include #include -#include -#include -#include -#include #include -#include +#include #include "amd_smi/amdsmi.h" #include "amd_smi/impl/amd_smi_clk_testing.h" +#include "amd_smi/impl/amd_smi_drm.h" +#include "amd_smi/impl/amd_smi_gpu_device.h" +#include "amd_smi/impl/amd_smi_test_overrides.h" #include "rocm_smi/rocm_smi.h" -#include "rocm_smi/rocm_smi_device.h" -#include "rocm_smi/rocm_smi_main.h" +#include "rocm_smi/rocm_smi_gpu_metrics.h" +#include "test_fixture_utils.h" namespace { -class FakeDpmDevice { - public: - FakeDpmDevice() { - auto path = (std::filesystem::temp_directory_path() / "amdsmi_dpm_XXXXXX").string(); - if (!mkdtemp(path.data())) { - throw std::runtime_error("cannot create DPM test directory"); - } - root_ = path; - std::filesystem::create_directory(root_ / "device"); - auto& devices = amd::smi::RocmSMI::getInstance().devices(); - index_ = static_cast(devices.size()); - devices.push_back(std::make_shared(root_.string(), nullptr)); - } - - ~FakeDpmDevice() { - auto& devices = amd::smi::RocmSMI::getInstance().devices(); - devices.erase(devices.begin() + index_); - std::filesystem::remove_all(root_); - } - - FakeDpmDevice(const FakeDpmDevice&) = delete; - FakeDpmDevice& operator=(const FakeDpmDevice&) = delete; - - uint32_t index() const { return index_; } - std::filesystem::path table() const { return root_ / "device" / "pp_dpm_fclk"; } - - void WriteTable(const char* contents) const { - std::ofstream out(table()); - ASSERT_TRUE(out.is_open()); - out << contents; - ASSERT_TRUE(out.good()); - } - - private: - std::filesystem::path root_; - uint32_t index_; -}; +using amd::smi::AMDSmiDrm; +using amd::smi::AMDSmiGPUDevice; +using amd::smi::testing::ScopedOverride; +using amd::smi::testing::ScopedProcessorRegistration; // pp_od_clk_voltage as seen on MI45x: SCLK and MCLK sections plus an OD_RANGE // footer, but no OD_FCLK section. @@ -298,8 +264,21 @@ TEST(GpuUnit, DpmRangesUnavailableSleepSentinelNotRejected) { EXPECT_EQ(ranges.sleep_state_freq, -1); } -TEST(GpuUnit, DpmRangesEmptyTablesReportIoError) { - for (const char* contents : {"", "\n", " \t\r\n", "\n\n"}) { +// An empty pp_dpm_* stream with no overdrive range: min keeps its UINT_MAX +// "unavailable" sentinel and surfaces as -1 rather than failing the call. +TEST(GpuUnit, DpmRangesEmptyStreamKeepsUnavailableMin) { + std::istringstream dpm(""); + SmiAmdgpuOdClkRange od_range; // not present -> derive from the (absent) levels + SmiAmdgpuClkRanges ranges; + EXPECT_EQ(smi_amdgpu_parse_dpm_ranges(dpm, od_range, ranges), AMDSMI_STATUS_SUCCESS); + EXPECT_EQ(ranges.max_freq, 0); + EXPECT_EQ(ranges.min_freq, -1); + EXPECT_EQ(ranges.sleep_state_freq, -1); +} + +TEST(GpuUnit, DpmRangesBlankTablesReportIoError) { + for (const char* contents : {"\n", " \t\r\n", "\n\n"}) { + SCOPED_TRACE(contents); std::istringstream dpm(contents); SmiAmdgpuOdClkRange od_range; SmiAmdgpuClkRanges ranges{1800, 1300, 2, 400}; @@ -311,69 +290,50 @@ TEST(GpuUnit, DpmRangesEmptyTablesReportIoError) { } } -// With an overdrive range the domain still has a range to report. -TEST(GpuUnit, DpmRangesBlankLineKeepsOdRange) { - std::istringstream dpm("\n"); - SmiAmdgpuOdClkRange od_range{true, 1800u, 1300u}; - SmiAmdgpuClkRanges ranges; - EXPECT_EQ(smi_amdgpu_parse_dpm_ranges(dpm, od_range, ranges), AMDSMI_STATUS_SUCCESS); - EXPECT_EQ(ranges.max_freq, 1800); - EXPECT_EQ(ranges.min_freq, 1300); -} - -TEST(GpuUnit, DpmRangesReadFailureIsNotAnEmptyTable) { - for (bool has_od_range : {false, true}) { - std::istringstream dpm("0: 1300Mhz\n"); - dpm.setstate(std::ios::badbit); - SmiAmdgpuOdClkRange od_range{has_od_range, 1800u, 1300u}; - SmiAmdgpuClkRanges ranges; +TEST(GpuUnit, DpmRangesBlankLinesReportIoWithOdRange) { + for (const char* contents : {"\n", " \t\r\n", "\n0: 1300Mhz\n", "0: 1300Mhz\n\n"}) { + SCOPED_TRACE(contents); + std::istringstream dpm(contents); + SmiAmdgpuOdClkRange od_range{true, 1800u, 1300u}; + SmiAmdgpuClkRanges ranges{1800, 1300, 2, 400}; EXPECT_EQ(smi_amdgpu_parse_dpm_ranges(dpm, od_range, ranges), AMDSMI_STATUS_IO); + EXPECT_EQ(ranges.max_freq, 1800); + EXPECT_EQ(ranges.min_freq, 1300); + EXPECT_EQ(ranges.num_dpm, 2); + EXPECT_EQ(ranges.sleep_state_freq, 400); } } -TEST(GpuUnit, DpmFrequencyReadFailureIsNotAnEmptyTable) { - FakeDpmDevice device; - // Offset zero in our own proc mem file fails with EIO without reading memory. - std::filesystem::create_symlink("/proc/self/mem", device.table()); - rsmi_frequencies_t frequencies{}; - EXPECT_EQ(rsmi_dev_gpu_clk_freq_get(device.index(), RSMI_CLK_TYPE_DF, &frequencies), - RSMI_STATUS_UNEXPECTED_SIZE); -} - TEST(GpuUnit, DpmFrequencyMissingTableNotSupported) { - FakeDpmDevice device; + FakeSysfsTree tree; + ScopedRocmSmiDevice device(tree.root() + "/card0"); rsmi_frequencies_t frequencies{}; EXPECT_EQ(rsmi_dev_gpu_clk_freq_get(device.index(), RSMI_CLK_TYPE_DF, &frequencies), RSMI_STATUS_NOT_SUPPORTED); } TEST(GpuUnit, DpmFrequencyEmptyTablesPreserveDataError) { - FakeDpmDevice device; + FakeSysfsTree tree; + ScopedRocmSmiDevice device(tree.root() + "/card0"); for (const char* contents : {"", "\n", " \t\r\n", "\n\n"}) { - device.WriteTable("0: 1300Mhz *\n"); + SCOPED_TRACE(contents); + tree.WriteFile("card0/device/pp_dpm_fclk", "0: 1300Mhz *\n"); rsmi_frequencies_t frequencies{}; ASSERT_EQ(rsmi_dev_gpu_clk_freq_get(device.index(), RSMI_CLK_TYPE_DF, &frequencies), RSMI_STATUS_SUCCESS); ASSERT_EQ(frequencies.num_supported, 1u); - device.WriteTable(contents); + tree.WriteFile("card0/device/pp_dpm_fclk", contents); EXPECT_EQ(rsmi_dev_gpu_clk_freq_get(device.index(), RSMI_CLK_TYPE_DF, &frequencies), RSMI_STATUS_UNEXPECTED_DATA); EXPECT_EQ(frequencies.num_supported, 0u); } } -TEST(GpuUnit, DpmFrequencyMalformedTableStillFails) { - FakeDpmDevice device; - device.WriteTable("0: 1300invalid\n"); - rsmi_frequencies_t frequencies{}; - EXPECT_EQ(rsmi_dev_gpu_clk_freq_get(device.index(), RSMI_CLK_TYPE_DF, &frequencies), - RSMI_STATUS_UNEXPECTED_DATA); -} - TEST(GpuUnit, DpmFrequencyValidTableStillReportsLevels) { - FakeDpmDevice device; - device.WriteTable("0: 1300Mhz\n1: 1550Mhz *\n2: 1800Mhz\n"); + FakeSysfsTree tree; + tree.WriteFile("card0/device/pp_dpm_fclk", "0: 1300Mhz\n1: 1550Mhz *\n2: 1800Mhz\n"); + ScopedRocmSmiDevice device(tree.root() + "/card0"); rsmi_frequencies_t frequencies{}; ASSERT_EQ(rsmi_dev_gpu_clk_freq_get(device.index(), RSMI_CLK_TYPE_DF, &frequencies), RSMI_STATUS_SUCCESS); @@ -383,6 +343,54 @@ TEST(GpuUnit, DpmFrequencyValidTableStillReportsLevels) { EXPECT_EQ(frequencies.frequency[2], 1800000000u); } +TEST(GpuUnit, DpmMixedDevicesPreservePublicApiErrors) { + ScopedAmdsmiInit smi_init; + FakeSysfsTree tree; + tree.WriteFile("card0/device/pp_dpm_fclk", "0: 1300Mhz\n1: 1550Mhz *\n2: 1800Mhz\n"); + tree.WriteFile("card1/device/pp_dpm_fclk", "\n"); + amd::smi::AMDGpuMetrics_v10_t metrics{}; + metrics.m_common_header = {sizeof(metrics), 1, 0}; + for (const char* card : {"card0", "card1"}) { + const std::string device_path = std::string(card) + "/device/"; + tree.WriteFile(device_path + "gpu_metrics", + std::string(reinterpret_cast(&metrics), sizeof(metrics))); + tree.WriteFile(device_path + "pp_od_clk_voltage", kOdNoFclk); + } + ScopedOverride root_guard( + smi_amdgpu_sysfs_drm_root, smi_amdgpu_set_sysfs_drm_root_for_testing, tree.root() + "/"); + ScopedRocmSmiDevice rocm_dgpu(tree.root() + "/card0"); + ScopedRocmSmiDevice rocm_igpu(tree.root() + "/card1"); + AMDSmiDrm drm; + AMDSmiGPUDevice dgpu(rocm_dgpu.index(), "card0", amdsmi_bdf_t{}, drm); + AMDSmiGPUDevice igpu(rocm_igpu.index(), "card1", amdsmi_bdf_t{}, drm); + ScopedProcessorRegistration dgpu_registration(&dgpu); + ScopedProcessorRegistration igpu_registration(&igpu); + for (const auto handle : {dgpu_registration.handle(), igpu_registration.handle()}) { + amdsmi_gpu_metrics_t result{}; + ASSERT_EQ(amdsmi_get_gpu_metrics_info(handle, &result), AMDSMI_STATUS_SUCCESS); + } + + amdsmi_frequencies_t frequencies{}; + ASSERT_EQ(amdsmi_get_clk_freq(dgpu_registration.handle(), AMDSMI_CLK_TYPE_DF, &frequencies), + AMDSMI_STATUS_SUCCESS); + EXPECT_EQ(frequencies.num_supported, 3u); + EXPECT_EQ(frequencies.current, 1u); + EXPECT_EQ(frequencies.frequency[0], 1300000000u); + EXPECT_EQ(frequencies.frequency[1], 1550000000u); + EXPECT_EQ(frequencies.frequency[2], 1800000000u); + amdsmi_clk_info_t info{}; + ASSERT_EQ(amdsmi_get_clock_info(dgpu_registration.handle(), AMDSMI_CLK_TYPE_DF, &info), + AMDSMI_STATUS_SUCCESS); + EXPECT_EQ(info.min_clk, 1300u); + EXPECT_EQ(info.max_clk, 1800u); + + EXPECT_EQ(amdsmi_get_clk_freq(igpu_registration.handle(), AMDSMI_CLK_TYPE_DF, &frequencies), + AMDSMI_STATUS_UNEXPECTED_DATA); + EXPECT_EQ(frequencies.num_supported, 0u); + EXPECT_EQ(amdsmi_get_clock_info(igpu_registration.handle(), AMDSMI_CLK_TYPE_DF, &info), + AMDSMI_STATUS_IO); +} + TEST(GpuUnit, DpmRangesWhitespaceDoesNotHideMalformedData) { for (const char* contents : {"\ninvalid\n", " \t\r\ninvalid\n"}) { std::istringstream dpm(contents); diff --git a/projects/amdsmi/tests/python/README.md b/projects/amdsmi/tests/python/README.md index 19cb68e7d8eb..fc578f2e3387 100644 --- a/projects/amdsmi/tests/python/README.md +++ b/projects/amdsmi/tests/python/README.md @@ -82,23 +82,6 @@ graph TD All three share the same option set and the same GTest-style summary, because they all delegate to `common.run_test_dir()`. -The `set` sweep runs each setting with a numeric `--gpu` selector and derives -clock and power values from that GPU. Each device's exit code is checked -independently; aggregate errors are not added to the allowed codes. -Empty DPM data remains an error and is not added to the sweep's allowed statuses. - -The sweep explicitly excludes memory-partition changes. A request can remain -queued until driver reload, and requesting the currently active mode does not -cancel it. Cleanup therefore does not claim to restore the original memory -partition. Test NPS transitions only in a separate, controlled driver-reload -workflow; the generic sweep neither reloads the driver nor clears changes -queued before the test. - -Clock cleanup uses `reset --clocks` rather than replaying captured limits that -can conflict with the sweep's current bounds. It resets clocks to defaults, -then reapplies each GPU's saved performance level; custom clock limits are -not preserved. - ### Invocation From an installed package (recommended — avoids a shadowing `site-packages` copy): diff --git a/projects/amdsmi/tests/python/cli/base.py b/projects/amdsmi/tests/python/cli/base.py index d12553ae45ad..94ce6a8c5a8a 100644 --- a/projects/amdsmi/tests/python/cli/base.py +++ b/projects/amdsmi/tests/python/cli/base.py @@ -121,10 +121,7 @@ class TestCliBase(unittest.TestCase): # TODO(amdsmi_team): drop these once the CLI tests supports automated # input SWEEP_EXCLUDED_ARGS = { - "set": { - "--compute-partition": "waits for confirmation", - "--memory-partition": "cannot cancel a queued change by restoring the active mode", - }, + "set": {"--compute-partition": "waits for confirmation"}, "reset": { # --gtt reaches reset.py's prompt_reboot(), which calls `systemctl reboot` on "y" "--gtt": "prompts to reboot the host", @@ -133,7 +130,7 @@ class TestCliBase(unittest.TestCase): } # TODO(amdsmi_team): User input is not a problem, we just need to test above ^ - PROMPT_ANSWERS = {"set": {"--fan": "y\n"}} + PROMPT_ANSWERS = {"set": {"--fan": "y\n", "--memory-partition": "y\n"}} # Commands whose sweep also accepts NOT_SUPPORTED. A write may legitimately # refuse a feature the ASIC lacks, but a read answering NOT_SUPPORTED is a @@ -142,10 +139,12 @@ class TestCliBase(unittest.TestCase): # Exit codes a single argument may also answer with, beyond NOT_SUPPORTED SWEEP_TOLERATED_CODES = { + # Not all ASICs support setting all modes, which is an invalid set option + "set": {"--memory-partition": amdsmi.AmdSmiStatus.INVAL}, # This reset is a read -- the kernel clears xgmi_error when it is read -- # so the read's errno becomes the reset's status, and some drivers reject # it with EINVAL. - "reset": {"--xgmierr": amdsmi.AmdSmiStatus.INVAL} + "reset": {"--xgmierr": amdsmi.AmdSmiStatus.INVAL}, } # Order for the amd-smi reset commands, putting --clocks last since @@ -356,6 +355,8 @@ def __init__(self, *args, **kwargs): self.memory_partition_modes = ["NPS1", "NPS2", "NPS4", "NPS8"] self.power_types = ["ppt0", "ppt1"] self.ptl_formats = ["I8", "F16", "BF16", "F32", "F64", "F8", "VECTOR"] + self.clk_limits = ["SCLK", "MCLK", "FCLK"] + self.limit_types = ["MAX", "MIN"] self.clk_levels = ["SCLK", "MCLK", "FCLK", "SOCCLK", "PCIE"] # When parsing, ignore these entries as they are abnormal @@ -569,7 +570,7 @@ def FindArgs(self, cmd, match_str): def CreateCmds(self, cmd_name, list1_name, list2_name, list3_name, list4_name): cmd = f"amd-smi {cmd_name} --help" list1_args = self._order_sweep_args(cmd_name, self.FindArgs(cmd, list1_name)) - list2_args = ["pass"] if cmd_name == "set" else self.FindArgs(cmd, list2_name) + list2_args = self.FindArgs(cmd, list2_name) list3_args = self.FindArgs(cmd, list3_name) list4_args = self.FindArgs(cmd, list4_name) if self.Debug: @@ -623,14 +624,6 @@ def CreateCmds(self, cmd_name, list1_name, list2_name, list3_name, list4_name): ) ) - if cmd_name == "set": - # Select the device before resolving its clock and power limits. - cmds = [ - (f"{command} --gpu {gpu_index}", condition) - for command, condition in cmds - for gpu_index in range(len(self.common.processors)) - ] - # Calculate and substitute in dependent values # Removes cmds that are invalid for index, cmd_cond in enumerate(cmds): @@ -850,7 +843,7 @@ def CreateCmds(self, cmd_name, list1_name, list2_name, list3_name, list4_name): sub_arg = items[2] if sub_arg != found_sub_arg: # Whole word matches only: i.e. remove "--gpu", not "--gpureset" - if "--gpu" in items and cmd_name != "set": + if "--gpu" in items: cmd = "" # Remove all file and watch modifiers except for gpu 0 diff --git a/projects/amdsmi/tests/python/cli/test_set.py b/projects/amdsmi/tests/python/cli/test_set.py index adba27a31b0a..dcfb3f746fac 100644 --- a/projects/amdsmi/tests/python/cli/test_set.py +++ b/projects/amdsmi/tests/python/cli/test_set.py @@ -78,7 +78,17 @@ def _restore_starting_values(self): ) ) + # set --memory-partition defaults + # Safe to write back: the mode only takes effect after a driver reload. + memory_partition = self.partition_data["current_partition"][index]["memory"] + if memory_partition != "N/A": + cmds.append( + (f"amd-smi set --memory-partition {memory_partition} --gpu {index}", self.PASS) + ) + # set --compute-partition defaults + # Must come after memory partition sets, since the memory partition set can override + # this setting (their defaults may differ). accelerator_type = self.partition_data["current_partition"][index]["accelerator_type"] if accelerator_type != "N/A": cmds.append( @@ -135,9 +145,25 @@ def _restore_starting_values(self): # TODO: get the right ptl-format cmds.append((f"amd-smi set --ptl-format {ptl_format} --gpu {index}", self.PASS)) + # set --clk-limit defaults + clock = self.metric_data["gpu_data"][index]["clock"] + for clk_type in self.clk_limits: + for limit_type in self.limit_types: + value = self._clk_limit_value(clock, clk_type, limit_type) + if value is not None: + # A readable min/max clock does not imply the device + # supports setting a limit on it. + cmds.append( + ( + f"amd-smi set --clk-limit {clk_type} {limit_type} {value} --gpu {index}", + [self.PASS, amdsmi.AmdSmiStatus.NOT_SUPPORTED], + ) + ) + # reset --clocks - # Reset clears clock masks and limits; replaying stale bounds first can fail. - # Overdrive may be unsupported even when the clock reset succeeds. + # A clk-level write is a bitmask, so replaying the recorded level would pin DPM + # to that one level; only AUTO hands every level back. Overdrive is reset too, + # and reports NOT_SUPPORTED on parts that lack it even though the rest worked. cmds.append( ( f"amd-smi reset --clocks --gpu {index}", @@ -146,7 +172,8 @@ def _restore_starting_values(self): ) # set --perf-level defaults - # Reset forces AUTO, so restore the saved performance level last. + # Last of the clock writes: the reset above forces AUTO, and --clk-limit and + # --perf-determinism each leave the device on a level of their own. perf_level = self.metric_data["gpu_data"][index]["perf_level"] if perf_level not in ("N/A", "AMDSMI_DEV_PERF_LEVEL_UNKNOWN"): perf_level = _strip_prefix(perf_level, "AMDSMI_DEV_PERF_LEVEL_") diff --git a/projects/amdsmi/tests/python/unit/gpu/test_cli_reset_clocks_fclk.py b/projects/amdsmi/tests/python/unit/gpu/test_cli_reset_clocks_fclk.py deleted file mode 100644 index faa064c86af7..000000000000 --- a/projects/amdsmi/tests/python/unit/gpu/test_cli_reset_clocks_fclk.py +++ /dev/null @@ -1,123 +0,0 @@ -#!/usr/bin/env python3 -# Copyright Advanced Micro Devices, Inc. -# SPDX-License-Identifier: MIT - -"""Mock-based unit tests for the FCLK step of ``amd-smi reset --clocks``. - -The stubbed library lets these run without GPU hardware or the compiled -``amdsmi`` package. -""" - -import os -import types -import unittest - -try: - from common.common import ( - amdsmi_path, - cli_search_order, - fake_module, - find_cli_dir, - load_cli_module, - stub_modules, - ) -except (ImportError, FileNotFoundError): # pragma: no cover - harness/install unavailable - amdsmi_path = None - cli_search_order = None - find_cli_dir = None - -_CLI_DIR = ( - find_cli_dir(*cli_search_order(os.path.dirname(os.path.abspath(__file__)))) - if find_cli_dir and cli_search_order - else None -) -RESET_PATH = os.path.join(_CLI_DIR, "subcommands", "reset.py") if _CLI_DIR else "" - -_STATUS_NOT_SUPPORTED = 2 -_DF = "DF" - -# MI300X FCLK DPM table, in Hz. -_FCLK_HZ = [1300_000_000, 1550_000_000, 1800_000_000] - - -class _FakeLibraryException(Exception): - def __init__(self, err_code=_STATUS_NOT_SUPPORTED): - super().__init__("mock error") - self._err_code = err_code - - def get_error_code(self): - return self._err_code - - def get_error_info(self, detailed=True): - return f"status {self._err_code}" - - -def _build_fake_amdsmi(): - amdsmi_pkg = types.ModuleType("amdsmi") - interface = types.ModuleType("amdsmi.amdsmi_interface") - exception = types.ModuleType("amdsmi.amdsmi_exception") - wrapper = types.ModuleType("amdsmi.amdsmi_wrapper") - - wrapper.AMDSMI_STATUS_NOT_SUPPORTED = _STATUS_NOT_SUPPORTED - interface.amdsmi_wrapper = wrapper - interface.AmdSmiClkType = types.SimpleNamespace(DF=_DF) - exception.AmdSmiLibraryException = _FakeLibraryException - - amdsmi_pkg.amdsmi_interface = interface - amdsmi_pkg.amdsmi_exception = exception - return { - "amdsmi": amdsmi_pkg, - "amdsmi.amdsmi_interface": interface, - "amdsmi.amdsmi_exception": exception, - "amdsmi.amdsmi_wrapper": wrapper, - } - - -class TestRestoreFclkCap(unittest.TestCase): - """Unit tests for ``ResetCommands._restore_fclk_cap``.""" - - @classmethod - def setUpClass(cls): - if not RESET_PATH or not os.path.isfile(RESET_PATH): - raise unittest.SkipTest( - f"amd-smi CLI reset.py not found (looked in {_CLI_DIR or amdsmi_path})" - ) - modules = _build_fake_amdsmi() - modules["amdsmi_helpers"] = fake_module( - "amdsmi_helpers", AMDSMIHelpers=type("AMDSMIHelpers", (), {}) - ) - stub_modules(cls, modules) - cls.interface = modules["amdsmi.amdsmi_interface"] - cls.module = load_cli_module("reset_under_test", RESET_PATH, sys_path_dir=_CLI_DIR) - - def _restore(self, levels_hz, max_clk): - self.writes = [] - self.interface.amdsmi_get_clk_freq = lambda _gpu, clk: {"frequency": list(levels_hz)} - self.interface.amdsmi_get_clock_info = lambda _gpu, clk: {"max_clk": max_clk} - self.interface.amdsmi_set_gpu_clk_limit = lambda _gpu, clk, lim, val: self.writes.append( - (clk, lim, val) - ) - return self.module.ResetCommands._restore_fclk_cap(None) - - def test_lowered_cap_raised_to_top_level(self): - self.assertTrue(self._restore(_FCLK_HZ, 1550)) - self.assertEqual(self.writes, [("fclk", "max", 1800)]) - - def test_cap_at_top_level_not_written(self): - self.assertFalse(self._restore(_FCLK_HZ, 1800)) - self.assertEqual(self.writes, []) - - def test_unreadable_cap_not_written(self): - self.assertFalse(self._restore(_FCLK_HZ, "N/A")) - self.assertEqual(self.writes, []) - - def test_empty_table_not_written(self): - self.assertFalse(self._restore([], 1550)) - self.assertEqual(self.writes, []) - - def test_unsupported_clock_is_skipped(self): - def _raise(_gpu, _clk): - raise _FakeLibraryException(_STATUS_NOT_SUPPORTED) - - self.interface.amdsmi_get_clk_freq = _raise - self.assertFalse(self.module.ResetCommands._restore_fclk_cap(None)) diff --git a/projects/amdsmi/tests/python/unit/gpu/test_cli_set_sweep.py b/projects/amdsmi/tests/python/unit/gpu/test_cli_set_sweep.py deleted file mode 100644 index dd3792c3f07a..000000000000 --- a/projects/amdsmi/tests/python/unit/gpu/test_cli_set_sweep.py +++ /dev/null @@ -1,240 +0,0 @@ -# Copyright Advanced Micro Devices, Inc. -# SPDX-License-Identifier: MIT - -"""Hardware-free regressions for the CLI set sweep.""" - -import types -import unittest -from unittest.mock import Mock - -from cli import base, test_set - - -class TestCliSetSweep(unittest.TestCase): - def setUp(self) -> None: - self.sweep = base.TestCliBase() - self.sweep.common = types.SimpleNamespace( - processors=[object(), object()], print=Mock(), cleaner_shader_resets_gpu=lambda: False - ) - - def _commands(self, options, command="set"): - arguments = { - "options": options, - "devices": ["--gpu all", "--gpu 0", "--gpu 1"], - "modifiers": ["pass"], - "": ["pass"], - } - self.sweep.FindArgs = lambda _command, section: arguments[section] - return self.sweep.CreateCmds(command, "options", "devices", "modifiers", "") - - def test_every_set_option_runs_once_per_gpu(self) -> None: - options = ["--perf-level AUTO", "--fan 50"] - for reduced in (True, False): - with self.subTest(reduced=reduced): - self.sweep.ReduceCmds = reduced - commands = self._commands(options) - self.assertCountEqual( - [command for command, _ in commands], - [ - f"amd-smi set {option} --loglevel DEBUG --gpu {gpu}" - for option in options - for gpu in range(2) - ], - ) - for _, allowed in commands: - self.assertEqual( - allowed, [self.sweep.PASS, base.amdsmi.AmdSmiStatus.NOT_SUPPORTED] - ) - - def test_memory_partition_changes_are_explicitly_excluded(self) -> None: - commands = self._commands( - ["--memory-partition NPS1", "--memory-partition NPS2", "--perf-level AUTO"] - ) - self.assertTrue(commands) - self.assertFalse(any("--memory-partition" in command for command, _ in commands)) - messages = [call[0][0] for call in self.sweep.common.print.call_args_list] - exclusions = [message for message in messages if "memory-partition" in message] - self.assertEqual(len(exclusions), 1) - self.assertIn("cannot cancel", exclusions[0]) - - def test_excluded_memory_partition_has_no_sweep_metadata(self) -> None: - command = "amd-smi set --memory-partition NPS2 --gpu 0" - for name in ("PROMPT_ANSWERS", "SWEEP_TOLERATED_CODES"): - with self.subTest(table=name): - self.assertIsNone(self.sweep._lookup(getattr(self.sweep, name), command)) - - def test_clock_values_come_from_the_selected_gpu(self) -> None: - self.sweep.clk_freq = [ - {"FCLK": {"num_supported": 3, "has_deep_sleep": False}}, - {"FCLK": {"num_supported": 3, "has_deep_sleep": True}}, - ] - self.sweep.metric_data = { - "gpu_data": [ - {"clock": {"fclk_0": {"min_clk": {"value": low}, "max_clk": {"value": high}}}} - for low, high in ((1200, 2000), (400, 800)) - ] - } - commands = self._commands( - [ - "--clk-level {clk_level_fclk}", - "--clk-limit {clk_limit_fclk_max}", - "--clk-limit {clk_limit_fclk_min}", - ] - ) - self.assertCountEqual( - [command for command, _ in commands], - [ - f"amd-smi set {option} --loglevel DEBUG --gpu {gpu}" - for gpu, options in ( - ( - 0, - ( - "--clk-level FCLK 0 1 2", - "--clk-limit FCLK MAX 1800", - "--clk-limit FCLK MIN 1400", - ), - ), - ( - 1, - ( - "--clk-level FCLK 0 1", - "--clk-limit FCLK MAX 700", - "--clk-limit FCLK MIN 500", - ), - ), - ) - for option in options - ], - ) - for _, allowed in commands: - self.assertEqual( - allowed, - [ - self.sweep.PASS, - base.amdsmi.AmdSmiStatus.NOT_SUPPORTED, - base.AmdSmiExitCode.INVALID_PARAMETER_VALUE, - ], - ) - - def test_unsupported_clock_does_not_hide_the_other_gpu(self) -> None: - self.sweep.clk_freq = [ - {"FCLK": {"num_supported": 3, "has_deep_sleep": False}}, - {"FCLK": None}, - ] - commands = self._commands(["--clk-level {clk_level_fclk}"]) - self.assertEqual( - [command for command, _ in commands], - ["amd-smi set --clk-level FCLK 0 1 2 --loglevel DEBUG --gpu 0"], - ) - - def test_mixed_supported_and_unsupported_devices_pass_individually(self) -> None: - commands = self._commands(["--fan 50"]) - self.sweep.util = types.SimpleNamespace( - RunCmdSync=Mock(side_effect=[(0, "", ""), (2, "", "")]) - ) - self.sweep.RunCmds(commands) - self.assertEqual(self.sweep.util.RunCmdSync.call_count, 2) - - def test_io_and_aggregate_errors_are_not_accepted(self) -> None: - commands = self._commands(["--fan 50"]) - for code in (12, 204): - with self.subTest(code=code): - self.sweep.util = types.SimpleNamespace( - RunCmdSync=Mock(side_effect=[(0, "", ""), (code, "", "")]) - ) - with self.assertRaisesRegex(AssertionError, f"--gpu 1.*rc={code}"): - self.sweep.RunCmds(commands) - self.assertEqual(self.sweep.util.RunCmdSync.call_count, 2) - - def test_read_sweep_keeps_all_gpu_coverage(self) -> None: - commands = self._commands(["--usage"], command="metric") - self.assertIn( - "amd-smi metric --usage --loglevel DEBUG", [command for command, _ in commands] - ) - self.assertIn( - "amd-smi metric --usage --gpu all --loglevel DEBUG", - [command for command, _ in commands], - ) - - def _prepare_cleanup(self) -> None: - self.sweep.metric_data = { - "gpu_data": [ - {"fan": {"max": "N/A", "speed": "N/A"}, "clock": {}, "perf_level": "N/A"} - for _ in range(2) - ] - } - self.sweep.static_data = { - "gpu_data": [ - { - "clock": {"sys": "N/A"}, - "partition": {"compute_partition_mem_alloc_mode": "N/A"}, - "soc_pstate": "N/A", - "xgmi_plpd": "N/A", - "limit": {"ptl_state": "N/A", "ptl_format": "N/A"}, - "process_isolation": "N/A", - } - for _ in range(2) - ] - } - self.sweep.partition_data = { - "current_partition": [{"memory": "NPS1", "accelerator_type": "N/A"} for _ in range(2)] - } - - def test_cleanup_does_not_claim_to_restore_memory_partitions(self) -> None: - self._prepare_cleanup() - self.sweep.RunCmds = Mock() - test_set.TestSet._restore_starting_values(self.sweep) - commands = self.sweep.RunCmds.call_args[0][0] - self.assertTrue(commands) - self.assertFalse(any("--memory-partition" in command for command, _ in commands)) - - def test_cleanup_resets_clocks_instead_of_replaying_stale_limits(self) -> None: - self._prepare_cleanup() - self.sweep.metric_data["gpu_data"][0]["clock"] = { - "gfx_0": {"min_clk": {"value": 500}, "max_clk": {"value": 500}} - } - self.sweep.static_data["gpu_data"][0]["clock"]["sys"] = { - "frequency_levels": {"Level 0": {"value": 500}, "Level 1": {"value": 1700}} - } - for gpu, level in enumerate(("AUTO", "HIGH")): - self.sweep.metric_data["gpu_data"][gpu]["perf_level"] = f"AMDSMI_DEV_PERF_LEVEL_{level}" - self.sweep.util = types.SimpleNamespace( - RunCmdSync=Mock( - side_effect=lambda cmd, **kwargs: ( - 198 if cmd == "amd-smi set --clk-limit SCLK MAX 500 --gpu 0" else 0, - "", - "", - ) - ) - ) - - test_set.TestSet._restore_starting_values(self.sweep) - - commands = [call[0][0] for call in self.sweep.util.RunCmdSync.call_args_list] - self.assertFalse(any("--clk-limit" in command for command in commands)) - for gpu, level in enumerate(("AUTO", "HIGH")): - self.assertLess( - commands.index(f"amd-smi reset --clocks --gpu {gpu}"), - commands.index(f"amd-smi set --perf-level {level} --gpu {gpu}"), - ) - - def test_cleanup_keeps_strict_reset_statuses_and_attempts_every_gpu(self) -> None: - for code in (0, 2, 12, 198, 204): - with self.subTest(code=code): - self._prepare_cleanup() - self.sweep.util = types.SimpleNamespace( - RunCmdSync=Mock( - side_effect=lambda cmd, **kwargs: ( - code if cmd == "amd-smi reset --clocks --gpu 0" else 0, - "", - "", - ) - ) - ) - if code in (0, 2): - test_set.TestSet._restore_starting_values(self.sweep) - else: - with self.assertRaisesRegex(AssertionError, f"rc={code}"): - test_set.TestSet._restore_starting_values(self.sweep) - commands = [call[0][0] for call in self.sweep.util.RunCmdSync.call_args_list] - self.assertIn("amd-smi reset --clocks --gpu 1", commands) diff --git a/projects/amdsmi/tests/python/unit/system/test_cli_exit_codes.py b/projects/amdsmi/tests/python/unit/system/test_cli_exit_codes.py index 78645b0d1feb..23d1b6024a82 100644 --- a/projects/amdsmi/tests/python/unit/system/test_cli_exit_codes.py +++ b/projects/amdsmi/tests/python/unit/system/test_cli_exit_codes.py @@ -2010,18 +2010,10 @@ def gpureset(cmd, mode): return {"amdsmi_reset_gpu": fn} def clocks(cmd, mode): - # Overdrive is reset first, then the FCLK cap, then perf level (twice) - # with the same handle; all must be stubbed so a fake handle never - # reaches hardware. + # Overdrive is reset first, then perf level (twice) with the same + # handle; both must be stubbed so a fake handle never reaches hardware. fn = _raise_not_supported if mode == "fail" else _noop - if mode == "fail": - fclk = {"amdsmi_get_clk_freq": fn} - else: - fclk = { - "amdsmi_get_clk_freq": lambda *_: {"frequency": [1800_000_000]}, - "amdsmi_get_clock_info": lambda *_: {"max_clk": 1800}, - } - return {"amdsmi_set_gpu_overdrive_level": fn, "amdsmi_set_gpu_perf_level": fn, **fclk} + return {"amdsmi_set_gpu_overdrive_level": fn, "amdsmi_set_gpu_perf_level": fn} def fans(cmd, mode): fn = _raise_not_supported if mode == "fail" else _noop @@ -2256,103 +2248,6 @@ def test_gpu_od_success_records_nothing(self): class TestResetGpuGAllFailureGuards(unittest.TestCase): """A per-device GPU `reset` failure must be recorded and must NOT crash.""" - def test_fclk_setter_logs_target_and_preserves_library_error(self) -> None: - reset = _load_reset() - for code in ( - amdsmi_wrapper.AMDSMI_STATUS_NOT_SUPPORTED, - amdsmi_wrapper.AMDSMI_STATUS_IO, - amdsmi_wrapper.AMDSMI_STATUS_NO_PERM, - ): - with self.subTest(code=code): - error = _DriverlessLibErr(code) - - def fail_write(*_args: object) -> None: - raise error - - with _patch_amdsmi_interface( - reset, - amdsmi_get_clk_freq=lambda *_: {"frequency": [1300000000, 1800000000]}, - amdsmi_get_clock_info=lambda *_: {"max_clk": 1550}, - amdsmi_set_gpu_clk_limit=fail_write, - ): - with self.assertLogs(level="DEBUG") as logs: - with self.assertRaises(_DriverlessLibErr) as caught: - reset.ResetCommands._restore_fclk_cap("gpu0") - self.assertIs(caught.exception, error) - self.assertIn("Failed to restore FCLK cap to 1800 MHz", logs.output[0]) - self.assertIn(error.get_error_info(), logs.output[0]) - - def test_missing_fclk_table_keeps_existing_reset_output(self): - reset = _load_reset() - cmd = _make_reset_cmd(reset) - patches = _build_reset_specs(reset)["clocks"](cmd, "success") - patches["amdsmi_get_clk_freq"] = _raise_not_supported - with _patch_amdsmi_interface(reset, **patches): - cmd.reset(_reset_args(reset, clocks=True)) - self.assertEqual(cmd.helpers.error_collector.resolve_exit_code(), 0) - result = next(value for _, key, value in cmd.logger.stored if key == "reset_clocks") - self.assertEqual(set(result), {"overdrive", "clocks", "performance"}) - - def test_fclk_read_failure_is_reported(self) -> None: - reset = _load_reset() - for operation, code in ( - ("amdsmi_get_clk_freq", amdsmi_wrapper.AMDSMI_STATUS_IO), - ("amdsmi_get_clk_freq", amdsmi_wrapper.AMDSMI_STATUS_UNEXPECTED_DATA), - ("amdsmi_get_clock_info", amdsmi_wrapper.AMDSMI_STATUS_IO), - ): - with self.subTest(operation=operation, code=code): - cmd = _make_reset_cmd(reset) - - def fail_read(*_args: object) -> None: - raise _DriverlessLibErr(code) - - patches = _build_reset_specs(reset)["clocks"](cmd, "success") - patches[operation] = fail_read - with _patch_amdsmi_interface(reset, **patches): - cmd.reset(_reset_args(reset, clocks=True)) - self.assertEqual(cmd.helpers.error_collector.resolve_exit_code(), code) - result = next(value for _, key, value in cmd.logger.stored if key == "reset_clocks") - self.assertIn("Unable to restore FCLK cap", result["fclk"]) - - def test_fclk_permission_failure_requires_elevation(self): - reset = _load_reset() - - def no_permission(*args): - raise _DriverlessLibErr(amdsmi_wrapper.AMDSMI_STATUS_NO_PERM) - - for operation in ("amdsmi_get_clk_freq", "amdsmi_set_gpu_clk_limit"): - with self.subTest(operation=operation): - cmd = _make_reset_cmd(reset) - patches = _build_reset_specs(reset)["clocks"](cmd, "success") - patches["amdsmi_get_clock_info"] = lambda *_: {"max_clk": 1550} - patches[operation] = no_permission - with _patch_amdsmi_interface(reset, **patches): - with self.assertRaisesRegex(PermissionError, "requires elevation"): - cmd.reset(_reset_args(reset, clocks=True)) - - def test_fclk_write_failure_is_reported_and_auto_reset_continues(self): - reset = _load_reset() - for code in (amdsmi_wrapper.AMDSMI_STATUS_NOT_SUPPORTED, amdsmi_wrapper.AMDSMI_STATUS_IO): - with self.subTest(code=code): - cmd = _make_reset_cmd(reset) - perf_writes = [] - - def fail_write(*args): - raise _DriverlessLibErr(code) - - patches = _build_reset_specs(reset)["clocks"](cmd, "success") - patches.update( - amdsmi_get_clock_info=lambda *_: {"max_clk": 1550}, - amdsmi_set_gpu_clk_limit=fail_write, - amdsmi_set_gpu_perf_level=lambda *args: perf_writes.append(args), - ) - with _patch_amdsmi_interface(reset, **patches): - cmd.reset(_reset_args(reset, clocks=True)) - self.assertEqual(cmd.helpers.error_collector.resolve_exit_code(), code) - result = next(value for _, key, value in cmd.logger.stored if key == "reset_clocks") - self.assertIn("Unable to restore FCLK cap", result["fclk"]) - self.assertEqual(len(perf_writes), 2) - def test_every_reset_option_is_driven(self): """Forcing function: every per-device GPU `reset` option must have a driver in _build_reset_specs. Adding a new option without a spec fails From 75a85b9c210a990ec05b76ef1ca17610083ec609 Mon Sep 17 00:00:00 2001 From: Koushik Billakanti Date: Tue, 10 Nov 2026 00:58:11 -0600 Subject: [PATCH 8/8] test(amdsmi): clarify DPM coverage Signed-off-by: Koushik Billakanti --- .../amd_smi_test/unit/gpu/clock_range_test.cc | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/projects/amdsmi/tests/amd_smi_test/unit/gpu/clock_range_test.cc b/projects/amdsmi/tests/amd_smi_test/unit/gpu/clock_range_test.cc index a5f6192ce65b..54a8b0aabfef 100644 --- a/projects/amdsmi/tests/amd_smi_test/unit/gpu/clock_range_test.cc +++ b/projects/amdsmi/tests/amd_smi_test/unit/gpu/clock_range_test.cc @@ -1,11 +1,8 @@ // Copyright Advanced Micro Devices, Inc. // SPDX-License-Identifier: MIT -// Unit tests for smi_amdgpu_parse_od_clk_range(), the pp_od_clk_voltage parser -// behind amd-smi's per-domain min/max clock. Driven over in-memory streams -- -// no GPU required. Guards the MI45x case where pp_od_clk_voltage carries no -// OD_FCLK section: the parser must report the domain absent so the caller falls -// back to pp_dpm_fclk instead of reporting a max of 0 MHz. +// Hardware-free tests for overdrive/DPM parsing and public clock APIs, +// using in-memory streams and fake sysfs devices. #include @@ -348,6 +345,7 @@ TEST(GpuUnit, DpmMixedDevicesPreservePublicApiErrors) { FakeSysfsTree tree; tree.WriteFile("card0/device/pp_dpm_fclk", "0: 1300Mhz\n1: 1550Mhz *\n2: 1800Mhz\n"); tree.WriteFile("card1/device/pp_dpm_fclk", "\n"); + // amdsmi_get_clock_info reads gpu_metrics even for FCLK. amd::smi::AMDGpuMetrics_v10_t metrics{}; metrics.m_common_header = {sizeof(metrics), 1, 0}; for (const char* card : {"card0", "card1"}) { @@ -384,6 +382,8 @@ TEST(GpuUnit, DpmMixedDevicesPreservePublicApiErrors) { EXPECT_EQ(info.min_clk, 1300u); EXPECT_EQ(info.max_clk, 1800u); + // These distinct errors capture current newline-only behavior, not a + // prescribed cross-API status contract. EXPECT_EQ(amdsmi_get_clk_freq(igpu_registration.handle(), AMDSMI_CLK_TYPE_DF, &frequencies), AMDSMI_STATUS_UNEXPECTED_DATA); EXPECT_EQ(frequencies.num_supported, 0u); @@ -391,15 +391,6 @@ TEST(GpuUnit, DpmMixedDevicesPreservePublicApiErrors) { AMDSMI_STATUS_IO); } -TEST(GpuUnit, DpmRangesWhitespaceDoesNotHideMalformedData) { - for (const char* contents : {"\ninvalid\n", " \t\r\ninvalid\n"}) { - std::istringstream dpm(contents); - SmiAmdgpuOdClkRange od_range; - SmiAmdgpuClkRanges ranges; - EXPECT_EQ(smi_amdgpu_parse_dpm_ranges(dpm, od_range, ranges), AMDSMI_STATUS_IO); - } -} - // A genuinely out-of-range value (> INT_MAX, not the sentinel) is still // rejected, preserving the protection added in #9549. constexpr char kDpmSclkHugeFreq[] = "0: 3000000000Mhz\n"; // 3e9 > INT_MAX