From d4caaf3736a3f6c4d8a77ded204f3ba950715f52 Mon Sep 17 00:00:00 2001 From: Charis Poag Date: Thu, 30 Jul 2026 21:19:47 -0500 Subject: [PATCH 1/5] fix(amdsmi): stop set_clk_freq from falsely returning INVAL The deep-sleep bitmask validation added in ROCM-9294 (#4707) rejected a clock-set with AMDSMI_STATUS_INVAL whenever the requested mask had any bit outside a settable range computed from a prior get_clk_freq read. Together with a pre-write read-only sysfs probe, this check-then-write sequence had a TOCTOU window that falsely failed on Navi48 (gfx1201) even though the set is supported (Navi31 runs the same flow and passes), surfacing as a TestFrequenciesReadWrite failure. Drop the strict up-front validation in favor of driver-like behavior: bitfield_to_freq_string() now discards bits above the highest settable level and applies the in-range subset, raising INVALID_ARGS only when no settable level remains. Deep-sleep ('S') is counted in num_supported but is not a writable pp_dpm_* label, so it is excluded from the settable range. The read-only check moves to the post-write PERMISSION path (removing the TOCTOU), and the clk-type lookup runs before the perf-level change so an invalid clock type no longer forces MANUAL. Tests: TestFrequenciesReadWrite accepts AMDSMI_STATUS_INVAL only when no level is settable, and TestBase::SavePerfLevels()/RestorePerfLevels() restore the device perf level so a forced MANUAL state cannot leak between tests. ROCM-28256 --- projects/amdsmi/include/amd_smi/amdsmi.h | 40 +++--- .../rocm_smi/include/rocm_smi/rocm_smi.h | 39 +++--- projects/amdsmi/rocm_smi/src/rocm_smi.cc | 90 ++++++------ .../gpu/clock/frequencies_read_write_test.cc | 129 +++++++++++++----- .../gpu/power/power_read_write_test.cc | 8 +- .../amdsmi/tests/amd_smi_test/test_base.cc | 66 +++++++++ .../amdsmi/tests/amd_smi_test/test_base.h | 5 + 7 files changed, 255 insertions(+), 122 deletions(-) diff --git a/projects/amdsmi/include/amd_smi/amdsmi.h b/projects/amdsmi/include/amd_smi/amdsmi.h index 2e64e59b4a8f..8bc60e7d3000 100644 --- a/projects/amdsmi/include/amd_smi/amdsmi.h +++ b/projects/amdsmi/include/amd_smi/amdsmi.h @@ -5457,31 +5457,33 @@ amdsmi_status_t amdsmi_set_gpu_overdrive_level(amdsmi_processor_handle processor * * @platform{gpu_bm_linux} * - * @details Given a processor handle @p processor_handle, a clock type @p clk_type, and a - * 64 bit bitmask @p freq_bitmask, this function will limit the set of - * allowable frequencies. If a bit in @p freq_bitmask has a value of 1, then - * the frequency (as ordered in an ::amdsmi_frequencies_t returned by - * amdsmi_get_clk_freq()) corresponding to that bit index will be - * allowed. - * - * This function will change the performance level to - * ::AMDSMI_DEV_PERF_LEVEL_MANUAL in order to modify the set of allowable - * frequencies. Caller will need to set to ::AMDSMI_DEV_PERF_LEVEL_AUTO in order - * to get back to default state. - * - * All bits with indices greater than or equal to - * ::amdsmi_frequencies_t::num_supported will be ignored. + * @details Given a processor handle @p processor_handle, this + * function will restricts clock @p clk_type to the frequencies + * selected in @p freq_bitmask. Bit N maps to DPM level N in the + * order returned by amdsmi_get_clk_freq(). Set a bit to 1 to + * enable and 0 to disable that level. + * + * Only settable DPM levels can be enabled. Bits outside the settable range are + * ignored, and the remaining in-range bits are still applied (matching driver + * behavior). The deep-sleep frequency (see ::amdsmi_frequencies_t::has_deep_sleep) + * is not settable, so when it is present the settable level count is + * ::amdsmi_frequencies_t::num_supported minus one. Without a deep-sleep + * frequency, all ::amdsmi_frequencies_t::num_supported levels (0 to + * num_supported - 1) are settable. The call returns ::AMDSMI_STATUS_INVAL if no + * bit selects a settable level. + * + * This call sets the performance level to ::AMDSMI_DEV_PERF_LEVEL_MANUAL. Set it + * back to ::AMDSMI_DEV_PERF_LEVEL_AUTO to restore the default behavior. * * @note This function requires admin/sudo privileges * * @param[in] processor_handle a processor handle * - * @param[in] clk_type the type of clock for which the set of frequencies - * will be modified + * @param[in] clk_type the type of clock to modify * - * @param[in] freq_bitmask A bitmask indicating the indices of the - * frequencies that are to be enabled (1) and disabled (0). Only the lowest - * ::amdsmi_frequencies_t.num_supported bits of this mask are relevant. + * @param[in] freq_bitmask Bitmask selecting which DPM levels to allow, where + * bit N enables (1) or disables (0) DPM level N. Bits above the highest settable + * level are ignored. See notes above on settable levels. * * @return ::amdsmi_status_t | ::AMDSMI_STATUS_SUCCESS on success, non-zero on fail */ diff --git a/projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi.h b/projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi.h index b77797ffaba7..298d48252d08 100644 --- a/projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi.h +++ b/projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi.h @@ -4038,29 +4038,30 @@ rsmi_status_t rsmi_dev_overdrive_level_set_v1(uint32_t dv_ind, uint32_t od); * @brief Control the set of allowed frequencies that can be used for the * specified clock. * - * @details Given a device index @p dv_ind, a clock type @p clk_type, and a - * 64 bit bitmask @p freq_bitmask, this function will limit the set of - * allowable frequencies. If a bit in @p freq_bitmask has a value of 1, then - * the frequency (as ordered in an ::rsmi_frequencies_t returned by - * rsmi_dev_gpu_clk_freq_get()) corresponding to that bit index will be - * allowed. - * - * This function will change the performance level to - * ::RSMI_DEV_PERF_LEVEL_MANUAL in order to modify the set of allowable - * frequencies. Caller will need to set to ::RSMI_DEV_PERF_LEVEL_AUTO in order - * to get back to default state. - * - * All bits with indices greater than or equal to - * ::rsmi_frequencies_t::num_supported will be ignored. + * @details Given a device index @p dv_ind, this function will restrict clock + * @p clk_type to the frequencies selected in @p freq_bitmask. Bit N maps to + * DPM level N in the order returned by rsmi_dev_gpu_clk_freq_get(). Set a bit + * to 1 to enable and 0 to disable that level. + * + * Only settable DPM levels can be enabled. Bits outside the settable range are + * ignored, and the remaining in-range bits are still applied (matching driver + * behavior). The deep-sleep frequency (see ::rsmi_frequencies_t::has_deep_sleep) + * is not settable, so when it is present the settable level count is + * ::rsmi_frequencies_t::num_supported minus one. Without a deep-sleep + * frequency, all ::rsmi_frequencies_t::num_supported levels (0 to + * num_supported - 1) are settable. The call returns ::RSMI_STATUS_INVALID_ARGS + * if no bit selects a settable level. + * + * This call sets the performance level to ::RSMI_DEV_PERF_LEVEL_MANUAL. Set it + * back to ::RSMI_DEV_PERF_LEVEL_AUTO to restore the default behavior. * * @param[in] dv_ind a device index * - * @param[in] clk_type the type of clock for which the set of frequencies - * will be modified + * @param[in] clk_type the type of clock to modify * - * @param[in] freq_bitmask A bitmask indicating the indices of the - * frequencies that are to be enabled (1) and disabled (0). Only the lowest - * ::rsmi_frequencies_t.num_supported bits of this mask are relevant. + * @param[in] freq_bitmask Bitmask selecting which DPM levels to allow, where + * bit N enables (1) or disables (0) DPM level N. Bits above the highest settable + * level are ignored. See notes above on settable levels. * * @retval ::RSMI_STATUS_SUCCESS is returned upon successful call. * @retval ::RSMI_STATUS_NOT_SUPPORTED installed software or hardware does not diff --git a/projects/amdsmi/rocm_smi/src/rocm_smi.cc b/projects/amdsmi/rocm_smi/src/rocm_smi.cc index f82ecaee4828..d8fb2e87e9a3 100644 --- a/projects/amdsmi/rocm_smi/src/rocm_smi.cc +++ b/projects/amdsmi/rocm_smi/src/rocm_smi.cc @@ -2085,20 +2085,31 @@ rsmi_status_t rsmi_dev_firmware_version_get(uint32_t dv_ind, rsmi_fw_block_t blo CATCH } -static std::string bitfield_to_freq_string(uint64_t bitf, uint32_t num_supported) { - std::string bf_str; - std::bitset bs(bitf); - +// Build the space-separated DPM-level string for pp_dpm_* from a bitmask: bit N +// selects level N (e.g. 0b101 -> "0 2 "). Bits above the highest settable level +// are dropped, matching how the driver ignores out-of-range indices. The +// deep-sleep level ('S') is counted in num_supported but is not writable, so it +// is excluded from the settable range. Throws RSMI_STATUS_INVALID_ARGS if no +// settable level remains. +static std::string bitfield_to_freq_string(uint64_t bitf, uint32_t num_supported, + bool has_deep_sleep = false) { if (num_supported > RSMI_MAX_NUM_FREQUENCIES) { throw amd::smi::rsmi_exception(RSMI_STATUS_INVALID_ARGS, __FUNCTION__); } - for (uint32_t i = 0; i < num_supported; ++i) { + uint32_t settable = (has_deep_sleep && num_supported > 0) ? num_supported - 1 : num_supported; + + std::string bf_str; + std::bitset bs(bitf); + for (uint32_t i = 0; i < settable; ++i) { if (bs[i]) { bf_str += std::to_string(i); bf_str += " "; } } + if (bf_str.empty()) { + throw amd::smi::rsmi_exception(RSMI_STATUS_INVALID_ARGS, __FUNCTION__); + } return bf_str; } @@ -2117,14 +2128,6 @@ rsmi_status_t rsmi_dev_gpu_clk_freq_set(uint32_t dv_ind, rsmi_clk_type_t clk_typ return RSMI_STATUS_INVALID_ARGS; } - amd::smi::DevInfoTypes dev_type; - const auto& clk_type_it = kClkTypeMap.find(clk_type); - if (clk_type_it != kClkTypeMap.end()) { - dev_type = clk_type_it->second; - } else { - return RSMI_STATUS_INVALID_ARGS; - } - ret = rsmi_dev_gpu_clk_freq_get(dv_ind, clk_type, &freqs); if (ret != RSMI_STATUS_SUCCESS) { @@ -2136,47 +2139,28 @@ rsmi_status_t rsmi_dev_gpu_clk_freq_set(uint32_t dv_ind, rsmi_clk_type_t clk_typ return RSMI_STATUS_UNEXPECTED_SIZE; } - // Deep sleep entry (index 0 in pp_dpm_sclk marked with 'S') is not a real - // DPM level; subtract it from the valid count so the bitmask only covers - // actual DPM levels. Bitmask bit i maps to sysfs DPM level i, which - // corresponds to freqs.frequency[i] when has_deep_sleep is false, or - // freqs.frequency[i+1] when has_deep_sleep is true. - if (freqs.num_supported > 0) { - uint32_t max_levels = freqs.num_supported; - if (freqs.has_deep_sleep) { - max_levels--; - } - uint64_t valid_mask = (max_levels > 0) ? (1ULL << max_levels) - 1 : 0; - if (freq_bitmask == 0 || (freq_bitmask & ~valid_mask)) { - return RSMI_STATUS_INVALID_ARGS; - } - } - amd::smi::RocmSMI& smi = amd::smi::RocmSMI::getInstance(); // Above call to rsmi_dev_get_gpu_clk_freq should have emitted an error if // assert below is not true assert(dv_ind < smi.devices().size()); + // The deep-sleep pseudo-level ('S') is counted in freqs.num_supported but is + // not a writable pp_dpm_* label, so it is excluded from the settable range. + std::string freq_enable_str = + bitfield_to_freq_string(freq_bitmask, freqs.num_supported, freqs.has_deep_sleep); + std::shared_ptr dev = smi.devices()[dv_ind]; assert(dev != nullptr); - // If the sysfs node is read-only, force DPM level is not supported - std::string sysfs_path = dev->get_sys_file_path_by_type(dev_type, true); - bool read_only = false; - int ro_ret = amd::smi::isReadOnlyForAll(sysfs_path, &read_only); - if (ro_ret != 0) { - return amd::smi::ErrnoToRsmiStatus(ro_ret); - } - if (read_only) { - return RSMI_STATUS_NOT_SUPPORTED; + amd::smi::DevInfoTypes dev_type; + const auto& clk_type_it = kClkTypeMap.find(clk_type); + if (clk_type_it != kClkTypeMap.end()) { + dev_type = clk_type_it->second; + } else { + return RSMI_STATUS_INVALID_ARGS; } - // bitfield_to_freq_string iterates [0, num_supported), but the validation - // above already guarantees no bits are set beyond the valid DPM range, so - // the extra iteration over the deep sleep index (if present) is harmless. - std::string freq_enable_str = bitfield_to_freq_string(freq_bitmask, freqs.num_supported); - ret = rsmi_dev_perf_level_set_v1(dv_ind, RSMI_DEV_PERF_LEVEL_MANUAL); if (ret != RSMI_STATUS_SUCCESS) { return ret; @@ -2184,11 +2168,25 @@ rsmi_status_t rsmi_dev_gpu_clk_freq_set(uint32_t dv_ind, rsmi_clk_type_t clk_typ rsmi_status_t status; status = amd::smi::ErrnoToRsmiStatus(dev->writeDevInfo(dev_type, freq_enable_str)); + ss << __PRETTY_FUNCTION__ << " | Attempted to set clock frequencies for device " << dv_ind + << " to: freq_enable_str=|" << freq_enable_str << "|" + << " (mask=0x" << std::hex << freq_bitmask << std::dec + << ", num_supported=" << freqs.num_supported + << ", deep_sleep=" << (freqs.has_deep_sleep ? "Y" : "N") << ", current=" << freqs.current + << ")" + << "; returned: " << amd::smi::getRSMIStatusString(status, false) << "\n"; + + ss << " | freqs[0.." << freqs.num_supported << "):"; + for (uint32_t fi = 0; fi < freqs.num_supported && fi < RSMI_MAX_NUM_FREQUENCIES; ++fi) { + ss << " [" << fi << "]=" << freqs.frequency[fi]; + } + LOG_DEBUG(ss); if (status == RSMI_STATUS_PERMISSION) { - bool post_read_only = false; - amd::smi::isReadOnlyForAll(sysfs_path, &post_read_only); - if (post_read_only) { + std::string sysfs_path = dev->get_sys_file_path_by_type(dev_type, true); + bool read_only = false; + amd::smi::isReadOnlyForAll(sysfs_path, &read_only); + if (read_only) { return RSMI_STATUS_NOT_SUPPORTED; } } diff --git a/projects/amdsmi/tests/amd_smi_test/functional/gpu/clock/frequencies_read_write_test.cc b/projects/amdsmi/tests/amd_smi_test/functional/gpu/clock/frequencies_read_write_test.cc index bdfc42d8e89d..3d10f7c2aa3a 100644 --- a/projects/amdsmi/tests/amd_smi_test/functional/gpu/clock/frequencies_read_write_test.cc +++ b/projects/amdsmi/tests/amd_smi_test/functional/gpu/clock/frequencies_read_write_test.cc @@ -8,7 +8,9 @@ #include #include #include +#include #include +#include #include #include "amd_smi/amdsmi.h" @@ -26,7 +28,9 @@ TestFrequenciesReadWrite::~TestFrequenciesReadWrite(void) {} void TestFrequenciesReadWrite::SetUp(void) { TestBase::SetUp(); - + // Capture the per-device performance level so Close() can restore it and + // avoid leaking a forced 'manual' perf state to later tests. + SavePerfLevels(); return; } @@ -38,11 +42,30 @@ void TestFrequenciesReadWrite::DisplayResults(void) const { } void TestFrequenciesReadWrite::Close() { + // Restore the performance level captured in SetUp() before amdsmi is shut + // down. Runs even if Run() aborted on a failed assertion, so this test cannot + // leak a forced 'manual' state to subsequent tests. + RestorePerfLevels(); // This will close handles opened within rsmitst utility calls and call // amdsmi_shut_down(), so it should be done after other hsa cleanup TestBase::Close(); } +// Number of writable DPM labels. The deep-sleep pseudo-level ('S') is listed in +// the table but has no settable numeric label, so it is excluded. +static uint32_t settable_level_count(const amdsmi_frequencies_t& f) { + return (f.has_deep_sleep && f.num_supported > 0) ? f.num_supported - 1 : f.num_supported; +} + +// The subset of a requested set-mask that can actually take effect: the bits +// within the settable label range [0, settable_level_count). amd-smi (and the +// driver) drop bits above that, so they never apply. +static uint64_t effective_level_mask(uint64_t requested_mask, const amdsmi_frequencies_t& f) { + uint32_t settable = settable_level_count(f); + uint64_t in_range = (settable >= 64) ? ~0ULL : ((1ULL << settable) - 1); + return requested_mask & in_range; +} + void TestFrequenciesReadWrite::Run(void) { amdsmi_status_t ret; amdsmi_frequencies_t f; @@ -67,9 +90,23 @@ void TestFrequenciesReadWrite::Run(void) { for (uint32_t dv_ind = 0; dv_ind < num_monitor_devs(); ++dv_ind) { PrintDeviceHeader(processor_handles_[dv_ind]); + // Optional debug filter: set AMDSMI_TEST_CLK to a clock name (e.g. "SYS", + // "GFX", "DF", "SOC", "MEM", "VCLK0", ...) to restrict this test to a + // single clock type. Useful for isolating which clock a set error comes + // from by running the test once per clock. Unset => exercise all clocks. + const char* only_clk_env = std::getenv("AMDSMI_TEST_CLK"); + for (uint32_t clk = AMDSMI_CLK_TYPE_FIRST; clk <= AMDSMI_CLK_TYPE__MAX; ++clk) { amdsmi_clk = (amdsmi_clk_type_t)clk; + if (only_clk_env != nullptr) { + auto name_it = clk_type_map.find(amdsmi_clk); + std::string clk_name = (name_it != clk_type_map.end()) ? name_it->second : ""; + if (clk_name != only_clk_env) { + continue; // skip clocks that don't match AMDSMI_TEST_CLK + } + } + auto freq_read = [&]() -> bool { // Skip AMDSMI_CLK_TYPE_PCIE, which does not supported in rocm-smi. if (auto it = clk_type_map.find(amdsmi_clk); it != clk_type_map.end()) { @@ -80,13 +117,32 @@ void TestFrequenciesReadWrite::Run(void) { std::cout << "amdsmi_get_clk_freq(" << it->second << ", f)" << std::endl; } - DISPLAY_AMDSMI_API("amdsmi_get_clk_freq", "gpu=" + std::to_string(dv_ind), VERB(STANDARD)); + DISPLAY_AMDSMI_API("amdsmi_get_clk_freq(" + std::string(FreqEnumToStr(amdsmi_clk)) + ")", + "gpu=" + std::to_string(dv_ind), VERB(STANDARD)); ret = amdsmi_get_clk_freq(processor_handles_[dv_ind], amdsmi_clk, &f); DISPLAY_AMDSMI_STATUS(VERB(STANDARD), __FILE__, __LINE__, ret, AMDSMI_STATUS_SUCCESS); if (auto it = clk_type_map.find(amdsmi_clk); it != clk_type_map.end()) { std::cout << ": " << smi_amdgpu_get_status_string(ret, false) << std::endl; } + IF_VERB(STANDARD) { + if (ret == AMDSMI_STATUS_SUCCESS) { + std::cout << "\t**" << FreqEnumToStr(amdsmi_clk) << ":\n" + << "\t\t**Is in deep sleep: " << (f.has_deep_sleep ? "Yes" : "No") << "\n" + << "\t\t**Number of supported frequencies: " << f.num_supported << "\n" + << "\t\t**Current frequency index: " << f.current << "\n" + << "\t\t**Supported frequencies: "; + for (uint32_t i = 0; i < f.num_supported; ++i) { + std::cout << f.frequency[i] << " Hz"; + if (i < f.num_supported - 1) { + std::cout << ", "; + } else { + std::cout << std::endl; + } + } + } + } + if (ret == AMDSMI_STATUS_NOT_SUPPORTED || ret == AMDSMI_STATUS_NOT_YET_IMPLEMENTED) { std::cout << "\t**Set " << FreqEnumToStr(amdsmi_clk) << ": Not supported on this machine" << std::endl; @@ -114,25 +170,7 @@ void TestFrequenciesReadWrite::Run(void) { // Skip AMDSMI_CLK_TYPE_PCIE, which does not supported in rocm-smi. if (amdsmi_clk == AMDSMI_CLK_TYPE_PCIE) return; - // Build the bitmask dynamically based on reported DPM levels. - // The deep sleep entry (if present) is not a valid DPM level, - // so exclude it from the count to stay within the valid range. - uint32_t dpm_levels = f.num_supported; - if (f.has_deep_sleep && dpm_levels > 0) { - dpm_levels--; - } - - if (dpm_levels < 2) { - // Not enough DPM levels to test a non-trivial bitmask; skip. - IF_VERB(STANDARD) { - std::cout << "\t**Set " << FreqEnumToStr(amdsmi_clk) << ": Only " << dpm_levels - << " DPM level(s), skipping write test." << std::endl; - } - return; - } - - // Pick two valid DPM levels: the second-to-last and the last. - freq_bitmask = (1ULL << (dpm_levels - 2)) | (1ULL << (dpm_levels - 1)); + freq_bitmask = 0b01100; // Try the 3rd and 4th DPM levels std::string freq_bm_str = std::bitset(freq_bitmask).to_string(); @@ -142,10 +180,13 @@ void TestFrequenciesReadWrite::Run(void) { std::cout << "Setting frequency mask for " << FreqEnumToStr(amdsmi_clk) << " to 0b" << freq_bm_str << " ..." << std::endl; } - DISPLAY_AMDSMI_API("amdsmi_set_clk_freq", "gpu=" + std::to_string(dv_ind), VERB(STANDARD)); + std::string set_api = "amdsmi_set_clk_freq(" + std::string(FreqEnumToStr(amdsmi_clk)) + + ", 0b" + freq_bm_str + ")"; + DISPLAY_AMDSMI_API(set_api, "gpu=" + std::to_string(dv_ind), VERB(STANDARD)); ret = amdsmi_set_clk_freq(processor_handles_[dv_ind], amdsmi_clk, freq_bitmask); DISPLAY_AMDSMI_STATUS(VERB(STANDARD), __FILE__, __LINE__, ret, AMDSMI_STATUS_SUCCESS, - AMDSMI_STATUS_NOT_SUPPORTED, AMDSMI_STATUS_NO_PERM); + AMDSMI_STATUS_NOT_SUPPORTED, AMDSMI_STATUS_NO_PERM, + AMDSMI_STATUS_INVAL); // Certain ASICs does not allow to set particular clocks. If set function for a clock // returns permission error despite root access, manually set ret value to success and // return @@ -159,35 +200,47 @@ void TestFrequenciesReadWrite::Run(void) { return; } - CHK_ERR_ASRT(ret) + // Lenient set contract: amd-smi applies the in-range subset of the + // requested mask and drops labels above the max settable level (like the + // driver), so a set only fails with INVAL when the mask selects no + // settable level at all. + if (ret == AMDSMI_STATUS_INVAL) { + std::cout << "\t**[Warn] AMDSMI_STATUS_INVAL - Could not set frequency mask for " + << FreqEnumToStr(amdsmi_clk) << " to 0b" << freq_bm_str << " on device [" + << dv_ind << "]!" << std::endl; + ASSERT_EQ(effective_level_mask(freq_bitmask, f), 0ULL); + } else { + CHK_ERR_ASRT(ret) + } + DISPLAY_AMDSMI_API("amdsmi_get_clk_freq", "gpu=" + std::to_string(dv_ind), VERB(STANDARD)); ret = amdsmi_get_clk_freq(processor_handles_[dv_ind], amdsmi_clk, &f); DISPLAY_AMDSMI_STATUS(VERB(STANDARD), __FILE__, __LINE__, ret, AMDSMI_STATUS_SUCCESS); if (ret != AMDSMI_STATUS_SUCCESS) { return; } - IF_VERB(STANDARD) { std::cout << "Frequency is now index " << f.current << std::endl; std::cout << "Resetting mask to all frequencies." << std::endl; } - // Build a valid "all DPM levels" bitmask instead of 0xFFFFFFFF, - // which would be rejected since it has bits beyond valid range. - uint32_t reset_levels = f.num_supported; - if (f.has_deep_sleep && reset_levels > 0) { - reset_levels--; - } - uint64_t all_levels_mask = (reset_levels > 0) ? (1ULL << reset_levels) - 1 : 0; - DISPLAY_AMDSMI_API("amdsmi_set_clk_freq", "gpu=" + std::to_string(dv_ind), VERB(STANDARD)); - ret = amdsmi_set_clk_freq(processor_handles_[dv_ind], amdsmi_clk, all_levels_mask); - DISPLAY_AMDSMI_STATUS(VERB(STANDARD), __FILE__, __LINE__, ret, AMDSMI_STATUS_SUCCESS); + + freq_bitmask = 0xFFFFFFFF; + set_api = "amdsmi_set_clk_freq(" + std::string(FreqEnumToStr(amdsmi_clk)) + ", 0xFFFFFFFF)"; + DISPLAY_AMDSMI_API(set_api, "gpu=" + std::to_string(dv_ind), VERB(STANDARD)); + ret = amdsmi_set_clk_freq(processor_handles_[dv_ind], amdsmi_clk, freq_bitmask); + DISPLAY_AMDSMI_STATUS(VERB(STANDARD), __FILE__, __LINE__, ret, AMDSMI_STATUS_SUCCESS, + AMDSMI_STATUS_INVAL); if (ret == AMDSMI_STATUS_NOT_SUPPORTED) { ret = AMDSMI_STATUS_SUCCESS; return; } - if (ret != AMDSMI_STATUS_SUCCESS) { + if (ret == AMDSMI_STATUS_INVAL) { + // Only tolerated for a clock with no settable level (e.g. deep-sleep + // only); otherwise the "enable all" reset must succeed. + ASSERT_EQ(effective_level_mask(freq_bitmask, f), 0ULL); return; } + CHK_ERR_ASRT(ret) DISPLAY_AMDSMI_API("amdsmi_set_gpu_perf_level", "gpu=" + std::to_string(dv_ind), VERB(STANDARD)); @@ -205,7 +258,9 @@ void TestFrequenciesReadWrite::Run(void) { continue; } freq_write(); - CHK_ERR_ASRT(ret) + if (ret != AMDSMI_STATUS_INVAL) { + CHK_ERR_ASRT(ret) + } } } } diff --git a/projects/amdsmi/tests/amd_smi_test/functional/gpu/power/power_read_write_test.cc b/projects/amdsmi/tests/amd_smi_test/functional/gpu/power/power_read_write_test.cc index 5bf62d1c31b9..03ab57162e4e 100644 --- a/projects/amdsmi/tests/amd_smi_test/functional/gpu/power/power_read_write_test.cc +++ b/projects/amdsmi/tests/amd_smi_test/functional/gpu/power/power_read_write_test.cc @@ -23,7 +23,9 @@ TestPowerReadWrite::~TestPowerReadWrite(void) {} void TestPowerReadWrite::SetUp(void) { TestBase::SetUp(); - + // Capture the per-device performance level so Close() can restore it and + // avoid leaking a forced 'manual' perf state to later tests. + SavePerfLevels(); return; } @@ -35,6 +37,10 @@ void TestPowerReadWrite::DisplayResults(void) const { } void TestPowerReadWrite::Close() { + // Restore the performance level captured in SetUp() before amdsmi is shut + // down. Runs even if Run() aborted on a failed assertion, so this test cannot + // leak a forced 'manual' state to subsequent tests. + RestorePerfLevels(); // This will close handles opened within amdsmitst utility calls and call // amdsmi_shut_down(), so it should be done after other hsa cleanup TestBase::Close(); diff --git a/projects/amdsmi/tests/amd_smi_test/test_base.cc b/projects/amdsmi/tests/amd_smi_test/test_base.cc index d5329dc9028c..03ebe475f636 100644 --- a/projects/amdsmi/tests/amd_smi_test/test_base.cc +++ b/projects/amdsmi/tests/amd_smi_test/test_base.cc @@ -30,6 +30,33 @@ static bool CheckModule(const std::string& fileName, const std::string& cond) { return (false); } +static std::string get_perf_level_string(amdsmi_dev_perf_level_t level) { + switch (level) { + case AMDSMI_DEV_PERF_LEVEL_AUTO: + return "AUTO"; + case AMDSMI_DEV_PERF_LEVEL_LOW: + return "LOW"; + case AMDSMI_DEV_PERF_LEVEL_HIGH: + return "HIGH"; + case AMDSMI_DEV_PERF_LEVEL_MANUAL: + return "MANUAL"; + case AMDSMI_DEV_PERF_LEVEL_STABLE_STD: + return "STABLE_STD"; + case AMDSMI_DEV_PERF_LEVEL_STABLE_PEAK: + return "STABLE_PEAK"; + case AMDSMI_DEV_PERF_LEVEL_STABLE_MIN_MCLK: + return "STABLE_MIN_MCLK"; + case AMDSMI_DEV_PERF_LEVEL_STABLE_MIN_SCLK: + return "STABLE_MIN_SCLK"; + case AMDSMI_DEV_PERF_LEVEL_DETERMINISM: + return "DETERMINISM"; + case AMDSMI_DEV_PERF_LEVEL_UNKNOWN: + return "UNKNOWN"; + default: + return "UNDEFINED_PERF_LEVEL"; + } +} + TestBase::TestBase() : setup_failed_(false) {} TestBase::~TestBase() = default; @@ -298,6 +325,45 @@ void TestBase::Close(void) { ASSERT_EQ(err, AMDSMI_STATUS_SUCCESS); } +void TestBase::SavePerfLevels(void) { + for (uint32_t i = 0; i < num_monitor_devs_; ++i) { + amdsmi_dev_perf_level_t lvl; + amdsmi_status_t err = amdsmi_get_gpu_perf_level(processor_handles_[i], &lvl); + if (err == AMDSMI_STATUS_SUCCESS) { + saved_perf_level_[i] = lvl; + saved_perf_level_valid_[i] = true; + IF_VERB(STANDARD) { + std::cout << "Saved original performance level for device " << i << ": " + << get_perf_level_string(lvl) << std::endl; + } + } else { + // Device does not expose a settable performance level; skip it so a + // later restore does not attempt an unsupported write. + saved_perf_level_valid_[i] = false; + } + } +} + +void TestBase::RestorePerfLevels(void) { + for (uint32_t i = 0; i < num_monitor_devs_; ++i) { + if (!saved_perf_level_valid_[i]) { + continue; // Original level was never captured for this device. + } + // Best-effort: intentionally ignore the return so that a restore failure on + // one device neither aborts the test nor masks the actual test outcome. + amdsmi_status_t err = amdsmi_set_gpu_perf_level(processor_handles_[i], saved_perf_level_[i]); + IF_VERB(STANDARD) { + if (err == AMDSMI_STATUS_SUCCESS) { + std::cout << "Restored perf level dev[" << i + << "]: " << get_perf_level_string(saved_perf_level_[i]) << std::endl; + } else { + std::cout << "Failed to restore perf level dev[" << i + << "]: " << get_perf_level_string(saved_perf_level_[i]) << std::endl; + } + } + } +} + void TestBase::DisplayResults(void) const { std::string label; IF_VERB(STANDARD) { diff --git a/projects/amdsmi/tests/amd_smi_test/test_base.h b/projects/amdsmi/tests/amd_smi_test/test_base.h index fd5005f11417..00cc1836dc2e 100644 --- a/projects/amdsmi/tests/amd_smi_test/test_base.h +++ b/projects/amdsmi/tests/amd_smi_test/test_base.h @@ -113,6 +113,11 @@ class TestBase { uint32_t socket_count_; ///< socket count std::vector sockets_; ///< sockets + void SavePerfLevels(void); + void RestorePerfLevels(void); + amdsmi_dev_perf_level_t saved_perf_level_[MAX_MONITOR_DEVICES]; ///< Restore original perf levels + bool saved_perf_level_valid_[MAX_MONITOR_DEVICES] = {false}; ///< Does perf need restoring? + private: std::string description_; std::string title_; ///< Displayed title of test From 1c7d05e5f7fbcf54ce29bec07baa3c310db12ea2 Mon Sep 17 00:00:00 2001 From: Charis Poag Date: Thu, 30 Jul 2026 22:35:30 -0500 Subject: [PATCH 2/5] fix(amdsmi): address PR review comments on set_clk_freq - rocm_smi.cc: only upgrade PERMISSION -> NOT_SUPPORTED when the read-only sysfs probe succeeds; otherwise keep the real write result. - amdsmi.h: fix doc grammar ("will restricts" -> "restricts"). ROCM-28256 --- projects/amdsmi/include/amd_smi/amdsmi.h | 2 +- projects/amdsmi/rocm_smi/src/rocm_smi.cc | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/projects/amdsmi/include/amd_smi/amdsmi.h b/projects/amdsmi/include/amd_smi/amdsmi.h index 8bc60e7d3000..91add552d125 100644 --- a/projects/amdsmi/include/amd_smi/amdsmi.h +++ b/projects/amdsmi/include/amd_smi/amdsmi.h @@ -5458,7 +5458,7 @@ amdsmi_status_t amdsmi_set_gpu_overdrive_level(amdsmi_processor_handle processor * @platform{gpu_bm_linux} * * @details Given a processor handle @p processor_handle, this - * function will restricts clock @p clk_type to the frequencies + * function restricts clock @p clk_type to the frequencies * selected in @p freq_bitmask. Bit N maps to DPM level N in the * order returned by amdsmi_get_clk_freq(). Set a bit to 1 to * enable and 0 to disable that level. diff --git a/projects/amdsmi/rocm_smi/src/rocm_smi.cc b/projects/amdsmi/rocm_smi/src/rocm_smi.cc index d8fb2e87e9a3..362b31d223a5 100644 --- a/projects/amdsmi/rocm_smi/src/rocm_smi.cc +++ b/projects/amdsmi/rocm_smi/src/rocm_smi.cc @@ -2185,8 +2185,9 @@ rsmi_status_t rsmi_dev_gpu_clk_freq_set(uint32_t dv_ind, rsmi_clk_type_t clk_typ if (status == RSMI_STATUS_PERMISSION) { std::string sysfs_path = dev->get_sys_file_path_by_type(dev_type, true); bool read_only = false; - amd::smi::isReadOnlyForAll(sysfs_path, &read_only); - if (read_only) { + // Only upgrade PERMISSION -> NOT_SUPPORTED when the probe succeeds; if it + // fails, keep the real write result (PERMISSION). + if (amd::smi::isReadOnlyForAll(sysfs_path, &read_only) == 0 && read_only) { return RSMI_STATUS_NOT_SUPPORTED; } } From 59d89af45e241508bd7a6a61f45446482b2c3b59 Mon Sep 17 00:00:00 2001 From: Charis Poag Date: Thu, 30 Jul 2026 22:37:59 -0500 Subject: [PATCH 3/5] test(amdsmi): re-enable TestFrequenciesReadWrite on Aldebaran The clock-set fix makes GpuFunctionalReadWrite.TestFrequenciesReadWrite pass on gfx90a / MI210, so remove it from FILTER[aldebaran] in amdsmitst.exclude (originally excluded under SWDEV-306889). Validated by CI on MI210. ROCM-28256 --- projects/amdsmi/tests/amd_smi_test/amdsmitst.exclude | 1 - 1 file changed, 1 deletion(-) diff --git a/projects/amdsmi/tests/amd_smi_test/amdsmitst.exclude b/projects/amdsmi/tests/amd_smi_test/amdsmitst.exclude index 044b6e48d5e4..9d120b76a0c9 100644 --- a/projects/amdsmi/tests/amd_smi_test/amdsmitst.exclude +++ b/projects/amdsmi/tests/amd_smi_test/amdsmitst.exclude @@ -47,7 +47,6 @@ $BLACKLIST_ALL_ASICS\ "GpuFunctionalReadOnly.TestVoltCurvRead:"\ "GpuFunctionalReadOnly.TestFrequenciesRead:"\ "GpuFunctionalReadWrite.FanReadWrite:"\ -"GpuFunctionalReadWrite.TestFrequenciesReadWrite:"\ "GpuFunctionalReadWrite.TestPciReadWrite:"\ "GpuFunctionalReadWrite.TestPowerReadWrite" From 9e3146c5360699ac36ad2978a92822c2cd51499f Mon Sep 17 00:00:00 2001 From: Charis Poag Date: Thu, 30 Jul 2026 23:26:03 -0500 Subject: [PATCH 4/5] docs(amdsmi): add changelog entry for set_clk_freq INVAL fix Record the lenient amdsmi_set_clk_freq behavior (drop out-of-range bits, INVAL only when nothing is settable, post-write read-only check) and the TestFrequenciesReadWrite re-enable on Aldebaran under the ROCm 7.15.0 Fixed section. ROCM-28256 --- projects/amdsmi/CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/projects/amdsmi/CHANGELOG.md b/projects/amdsmi/CHANGELOG.md index ba78e433e73b..c5f6fee87fc7 100644 --- a/projects/amdsmi/CHANGELOG.md +++ b/projects/amdsmi/CHANGELOG.md @@ -263,6 +263,9 @@ Full documentation for amd_smi_lib is available at [https://rocm.docs.amd.com/pr - The WSL backend returned success with a zeroed structure, so `rev_id` read as `0x0`, and where it did report the not-supported value Python rendered it as the raw `0xffffffff`. Python and the CLI now render it as `N/A`. - `amdsmi_asic_info_t` is now reset through one shared initializer used by every backend, so a field a backend cannot supply keeps its not-supported value rather than a plausible zero. +- **Fixed `amdsmi_set_clk_freq()` falsely returning `AMDSMI_STATUS_INVAL` on Navi48 (gfx1201)**. + - The deep-sleep bitmask validation added previously rejected the entire request when any requested bit fell outside a settable range computed from a prior read, so a supported clock set failed on gfx1201 even though the write is valid. The set is now lenient: out-of-range bits are dropped, and `AMDSMI_STATUS_INVAL` is only returned when no requested level is settable. The read-only check runs after the write, and `TestFrequenciesReadWrite` is re-enabled on Aldebaran (gfx90a / MI210). + ### Upcoming Changes - **UUIDs will be replaced by CUIDs in an upcoming version**. From 956ce8cf13c47f538343c658b84c54a64c37d215 Mon Sep 17 00:00:00 2001 From: Charis Poag Date: Thu, 1 Oct 2026 21:57:04 -0500 Subject: [PATCH 5/5] fix(amdsmi): close TOCTOU race in clk_freq test, document edge-case retvals - frequencies_read_write_test.cc: replace predict-before validation with a reactive post-hoc check (refresh f after an unexpected INVAL, assert the level is now out of the shrunk settable range) to close a TOCTOU race on live power-state changes (DF/FCLK flakiness). Extract refresh_f(), gpu_tag_str(), and to_bit_string() helpers to DRY up repeated patterns; derive the positive-case mask dynamically from settable_level_count(f) instead of a hardcoded 0b01100; add phase overview comments. - amdsmi.h / rocm_smi.h: document the deep-sleep pseudo-level's exclusion from the settable DPM level count, AMDSMI/RSMI_STATUS_UNEXPECTED_DATA for zero settable levels, and RSMI_STATUS_UNEXPECTED_SIZE for num_supported exceeding AMDSMI/RSMI_MAX_NUM_FREQUENCIES. Addresses review comments from bill-shuzhou-liu and gabrpham on PR #9536. --- projects/amdsmi/include/amd_smi/amdsmi.h | 29 ++- .../rocm_smi/include/rocm_smi/rocm_smi.h | 17 +- projects/amdsmi/rocm_smi/src/rocm_smi.cc | 89 ++++---- .../gpu/clock/frequencies_read_write_test.cc | 207 ++++++++++++------ 4 files changed, 228 insertions(+), 114 deletions(-) diff --git a/projects/amdsmi/include/amd_smi/amdsmi.h b/projects/amdsmi/include/amd_smi/amdsmi.h index 91add552d125..7f0ca6619e58 100644 --- a/projects/amdsmi/include/amd_smi/amdsmi.h +++ b/projects/amdsmi/include/amd_smi/amdsmi.h @@ -4089,7 +4089,13 @@ amdsmi_status_t amdsmi_get_gpu_pci_replay_counter(amdsmi_processor_handle proces * ::amdsmi_frequencies_t::num_supported (of ::amdsmi_pcie_bandwidth_t) bits of * this mask are relevant. * - * @return ::amdsmi_status_t | ::AMDSMI_STATUS_SUCCESS on success, non-zero on fail + * @retval ::AMDSMI_STATUS_SUCCESS call was successful + * @retval ::AMDSMI_STATUS_NOT_SUPPORTED installed software or hardware does not + * support this function with the given arguments + * @retval ::AMDSMI_STATUS_NO_PERM function requires root access + * @retval ::AMDSMI_STATUS_INVAL @p bw_bitmask selects no settable bandwidth + * @retval ::AMDSMI_STATUS_UNEXPECTED_DATA the device reports zero settable + * bandwidths */ amdsmi_status_t amdsmi_set_gpu_pci_bandwidth(amdsmi_processor_handle processor_handle, uint64_t bw_bitmask); @@ -5464,13 +5470,15 @@ amdsmi_status_t amdsmi_set_gpu_overdrive_level(amdsmi_processor_handle processor * enable and 0 to disable that level. * * Only settable DPM levels can be enabled. Bits outside the settable range are - * ignored, and the remaining in-range bits are still applied (matching driver - * behavior). The deep-sleep frequency (see ::amdsmi_frequencies_t::has_deep_sleep) + * dropped before the request is sent, so an out-of-range level is never + * requested; the remaining in-range bits are still applied. The deep-sleep + * frequency (see ::amdsmi_frequencies_t::has_deep_sleep) * is not settable, so when it is present the settable level count is * ::amdsmi_frequencies_t::num_supported minus one. Without a deep-sleep * frequency, all ::amdsmi_frequencies_t::num_supported levels (0 to - * num_supported - 1) are settable. The call returns ::AMDSMI_STATUS_INVAL if no - * bit selects a settable level. + * num_supported - 1) are settable. The call returns ::AMDSMI_STATUS_INVAL if + * @p freq_bitmask selects no settable level, or ::AMDSMI_STATUS_UNEXPECTED_DATA + * if the device reports zero settable levels for @p clk_type. * * This call sets the performance level to ::AMDSMI_DEV_PERF_LEVEL_MANUAL. Set it * back to ::AMDSMI_DEV_PERF_LEVEL_AUTO to restore the default behavior. @@ -5485,7 +5493,16 @@ amdsmi_status_t amdsmi_set_gpu_overdrive_level(amdsmi_processor_handle processor * bit N enables (1) or disables (0) DPM level N. Bits above the highest settable * level are ignored. See notes above on settable levels. * - * @return ::amdsmi_status_t | ::AMDSMI_STATUS_SUCCESS on success, non-zero on fail + * @retval ::AMDSMI_STATUS_SUCCESS call was successful + * @retval ::AMDSMI_STATUS_NOT_SUPPORTED installed software or hardware does not + * support this function with the given arguments + * @retval ::AMDSMI_STATUS_NO_PERM function requires root access + * @retval ::AMDSMI_STATUS_INVAL @p clk_type is not a valid ::amdsmi_clk_type_t, + * or @p freq_bitmask selects no settable DPM level + * @retval ::AMDSMI_STATUS_UNEXPECTED_SIZE the device reports more supported + * frequencies than AMDSMI_MAX_NUM_FREQUENCIES + * @retval ::AMDSMI_STATUS_UNEXPECTED_DATA the device reports zero settable DPM + * levels for @p clk_type */ amdsmi_status_t amdsmi_set_clk_freq(amdsmi_processor_handle processor_handle, amdsmi_clk_type_t clk_type, uint64_t freq_bitmask); diff --git a/projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi.h b/projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi.h index 298d48252d08..24bad84a36ef 100644 --- a/projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi.h +++ b/projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi.h @@ -2651,6 +2651,9 @@ rsmi_status_t rsmi_dev_pci_replay_counter_get(uint32_t dv_ind, uint64_t* counter * * @retval ::RSMI_STATUS_SUCCESS call was successful * @retval ::RSMI_STATUS_PERMISSION function requires root access + * @retval ::RSMI_STATUS_INVALID_ARGS @p bw_bitmask selects no settable bandwidth + * @retval ::RSMI_STATUS_UNEXPECTED_DATA the device reports zero settable + * bandwidths * */ rsmi_status_t rsmi_dev_pci_bandwidth_set(uint32_t dv_ind, uint64_t bw_bitmask); @@ -4044,13 +4047,15 @@ rsmi_status_t rsmi_dev_overdrive_level_set_v1(uint32_t dv_ind, uint32_t od); * to 1 to enable and 0 to disable that level. * * Only settable DPM levels can be enabled. Bits outside the settable range are - * ignored, and the remaining in-range bits are still applied (matching driver - * behavior). The deep-sleep frequency (see ::rsmi_frequencies_t::has_deep_sleep) + * dropped before the request is sent, so an out-of-range level is never + * requested; the remaining in-range bits are still applied. The deep-sleep + * frequency (see ::rsmi_frequencies_t::has_deep_sleep) * is not settable, so when it is present the settable level count is * ::rsmi_frequencies_t::num_supported minus one. Without a deep-sleep * frequency, all ::rsmi_frequencies_t::num_supported levels (0 to * num_supported - 1) are settable. The call returns ::RSMI_STATUS_INVALID_ARGS - * if no bit selects a settable level. + * if @p freq_bitmask selects no settable level, or ::RSMI_STATUS_UNEXPECTED_DATA + * if the device reports zero settable levels for @p clk_type. * * This call sets the performance level to ::RSMI_DEV_PERF_LEVEL_MANUAL. Set it * back to ::RSMI_DEV_PERF_LEVEL_AUTO to restore the default behavior. @@ -4067,6 +4072,12 @@ rsmi_status_t rsmi_dev_overdrive_level_set_v1(uint32_t dv_ind, uint32_t od); * @retval ::RSMI_STATUS_NOT_SUPPORTED installed software or hardware does not * support this function with the given arguments * @retval ::RSMI_STATUS_PERMISSION function requires root access + * @retval ::RSMI_STATUS_INVALID_ARGS @p clk_type is not a valid ::rsmi_clk_type_t, + * or @p freq_bitmask selects no settable DPM level + * @retval ::RSMI_STATUS_UNEXPECTED_SIZE the device reports more supported + * frequencies than RSMI_MAX_NUM_FREQUENCIES + * @retval ::RSMI_STATUS_UNEXPECTED_DATA the device reports zero settable DPM + * levels for @p clk_type * */ rsmi_status_t rsmi_dev_gpu_clk_freq_set(uint32_t dv_ind, rsmi_clk_type_t clk_type, diff --git a/projects/amdsmi/rocm_smi/src/rocm_smi.cc b/projects/amdsmi/rocm_smi/src/rocm_smi.cc index 362b31d223a5..af8ce8030af1 100644 --- a/projects/amdsmi/rocm_smi/src/rocm_smi.cc +++ b/projects/amdsmi/rocm_smi/src/rocm_smi.cc @@ -2087,10 +2087,12 @@ rsmi_status_t rsmi_dev_firmware_version_get(uint32_t dv_ind, rsmi_fw_block_t blo // Build the space-separated DPM-level string for pp_dpm_* from a bitmask: bit N // selects level N (e.g. 0b101 -> "0 2 "). Bits above the highest settable level -// are dropped, matching how the driver ignores out-of-range indices. The -// deep-sleep level ('S') is counted in num_supported but is not writable, so it -// is excluded from the settable range. Throws RSMI_STATUS_INVALID_ARGS if no -// settable level remains. +// are dropped before writing so an out-of-range level never gets requested. +// The deep-sleep level ('S') is counted in num_supported but is not writable, +// so it is excluded from the settable range. Throws RSMI_STATUS_UNEXPECTED_DATA +// if the device reports zero settable levels (this should not happen on any +// supported ASIC), or RSMI_STATUS_INVALID_ARGS if the mask selects no +// settable level. static std::string bitfield_to_freq_string(uint64_t bitf, uint32_t num_supported, bool has_deep_sleep = false) { if (num_supported > RSMI_MAX_NUM_FREQUENCIES) { @@ -2098,6 +2100,9 @@ static std::string bitfield_to_freq_string(uint64_t bitf, uint32_t num_supported } uint32_t settable = (has_deep_sleep && num_supported > 0) ? num_supported - 1 : num_supported; + if (settable == 0) { + throw amd::smi::rsmi_exception(RSMI_STATUS_UNEXPECTED_DATA, __FUNCTION__); + } std::string bf_str; std::bitset bs(bitf); @@ -2128,6 +2133,26 @@ rsmi_status_t rsmi_dev_gpu_clk_freq_set(uint32_t dv_ind, rsmi_clk_type_t clk_typ return RSMI_STATUS_INVALID_ARGS; } + amd::smi::DevInfoTypes dev_type; + const auto& clk_type_it = kClkTypeMap.find(clk_type); + if (clk_type_it != kClkTypeMap.end()) { + dev_type = clk_type_it->second; + } else { + return RSMI_STATUS_INVALID_ARGS; + } + + amd::smi::RocmSMI& smi = amd::smi::RocmSMI::getInstance(); + assert(dv_ind < smi.devices().size()); + std::shared_ptr dev = smi.devices()[dv_ind]; + assert(dev != nullptr); + + std::string sysfs_path = dev->get_sys_file_path_by_type(dev_type, true); + bool read_only = false; + // Dont try to set on pp_dpm* files that are read only + if (amd::smi::isReadOnlyForAll(sysfs_path, &read_only) == 0 && read_only) { + return RSMI_STATUS_NOT_SUPPORTED; + } + ret = rsmi_dev_gpu_clk_freq_get(dv_ind, clk_type, &freqs); if (ret != RSMI_STATUS_SUCCESS) { @@ -2139,27 +2164,26 @@ rsmi_status_t rsmi_dev_gpu_clk_freq_set(uint32_t dv_ind, rsmi_clk_type_t clk_typ return RSMI_STATUS_UNEXPECTED_SIZE; } - amd::smi::RocmSMI& smi = amd::smi::RocmSMI::getInstance(); - - // Above call to rsmi_dev_get_gpu_clk_freq should have emitted an error if - // assert below is not true - assert(dv_ind < smi.devices().size()); + // Logged before bitfield_to_freq_string()/perf-level-set so this context is + // captured even if a later step fails or throws (e.g. no settable level). + std::string clock_name = kClkStateMap.find(clk_type)->second; + ss << __PRETTY_FUNCTION__ << " | Requested clock frequency set for device " << dv_ind << "for " + << clock_name << "\n : mask=0x" << std::hex << freq_bitmask << std::dec + << ", num_supported=" << freqs.num_supported + << ", deep_sleep=" << (freqs.has_deep_sleep ? "Y" : "N") << ", current=" << freqs.current + << " | freqs[0.." << freqs.num_supported << "):"; + for (uint32_t fi = 0; fi < freqs.num_supported && fi < RSMI_MAX_NUM_FREQUENCIES; ++fi) { + ss << " [" << fi << "]=" << freqs.frequency[fi]; + } + LOG_DEBUG(ss); // The deep-sleep pseudo-level ('S') is counted in freqs.num_supported but is // not a writable pp_dpm_* label, so it is excluded from the settable range. std::string freq_enable_str = bitfield_to_freq_string(freq_bitmask, freqs.num_supported, freqs.has_deep_sleep); - - std::shared_ptr dev = smi.devices()[dv_ind]; - assert(dev != nullptr); - - amd::smi::DevInfoTypes dev_type; - const auto& clk_type_it = kClkTypeMap.find(clk_type); - if (clk_type_it != kClkTypeMap.end()) { - dev_type = clk_type_it->second; - } else { - return RSMI_STATUS_INVALID_ARGS; - } + ss << __PRETTY_FUNCTION__ << " | About to try writing clock frequency mask for device " << dv_ind + << " for " << clock_name << ": freq_enable_str=|" << freq_enable_str << "|"; + LOG_DEBUG(ss); ret = rsmi_dev_perf_level_set_v1(dv_ind, RSMI_DEV_PERF_LEVEL_MANUAL); if (ret != RSMI_STATUS_SUCCESS) { @@ -2168,30 +2192,11 @@ rsmi_status_t rsmi_dev_gpu_clk_freq_set(uint32_t dv_ind, rsmi_clk_type_t clk_typ rsmi_status_t status; status = amd::smi::ErrnoToRsmiStatus(dev->writeDevInfo(dev_type, freq_enable_str)); - ss << __PRETTY_FUNCTION__ << " | Attempted to set clock frequencies for device " << dv_ind - << " to: freq_enable_str=|" << freq_enable_str << "|" - << " (mask=0x" << std::hex << freq_bitmask << std::dec - << ", num_supported=" << freqs.num_supported - << ", deep_sleep=" << (freqs.has_deep_sleep ? "Y" : "N") << ", current=" << freqs.current - << ")" - << "; returned: " << amd::smi::getRSMIStatusString(status, false) << "\n"; - - ss << " | freqs[0.." << freqs.num_supported << "):"; - for (uint32_t fi = 0; fi < freqs.num_supported && fi < RSMI_MAX_NUM_FREQUENCIES; ++fi) { - ss << " [" << fi << "]=" << freqs.frequency[fi]; - } + ss << __PRETTY_FUNCTION__ << " | Wrote clock frequency mask for device " << dv_ind << " for " + << clock_name << ": freq_enable_str=|" << freq_enable_str << "|" + << "; returned: " << amd::smi::getRSMIStatusString(status, false); LOG_DEBUG(ss); - if (status == RSMI_STATUS_PERMISSION) { - std::string sysfs_path = dev->get_sys_file_path_by_type(dev_type, true); - bool read_only = false; - // Only upgrade PERMISSION -> NOT_SUPPORTED when the probe succeeds; if it - // fails, keep the real write result (PERMISSION). - if (amd::smi::isReadOnlyForAll(sysfs_path, &read_only) == 0 && read_only) { - return RSMI_STATUS_NOT_SUPPORTED; - } - } - return status; CATCH diff --git a/projects/amdsmi/tests/amd_smi_test/functional/gpu/clock/frequencies_read_write_test.cc b/projects/amdsmi/tests/amd_smi_test/functional/gpu/clock/frequencies_read_write_test.cc index 3d10f7c2aa3a..8c873e66af9e 100644 --- a/projects/amdsmi/tests/amd_smi_test/functional/gpu/clock/frequencies_read_write_test.cc +++ b/projects/amdsmi/tests/amd_smi_test/functional/gpu/clock/frequencies_read_write_test.cc @@ -66,6 +66,14 @@ static uint64_t effective_level_mask(uint64_t requested_mask, const amdsmi_frequ return requested_mask & in_range; } +// Binary string for a bitmask, trimmed of leading zeros (down to at least one +// digit) for display. +static std::string to_bit_string(uint64_t mask) { + std::string s = std::bitset(mask).to_string(); + s.erase(0, std::min(s.find_first_not_of('0'), s.size() - 1)); + return s; +} + void TestFrequenciesReadWrite::Run(void) { amdsmi_status_t ret; amdsmi_frequencies_t f; @@ -149,14 +157,6 @@ void TestFrequenciesReadWrite::Run(void) { return false; } - // special driver issue, shouldn't normally occur - if (ret == AMDSMI_STATUS_UNEXPECTED_DATA) { - std::cerr << "WARN: Clock file [" << FreqEnumToStr(amdsmi_clk) << "] exists on device [" - << dv_ind << "] but empty!" << std::endl; - std::cerr << " Likely a driver issue!" << std::endl; - } - - // CHK_ERR_ASRT(ret) IF_VERB(STANDARD) { std::cout << "Initial frequency for clock " << FreqEnumToStr(amdsmi_clk) << " is " << f.current << std::endl; @@ -165,76 +165,158 @@ void TestFrequenciesReadWrite::Run(void) { }; auto freq_write = [&]() { + // Exercises the lenient-set contract in four phases: + // (1) a mask entirely above the settable range must return INVAL, + // (2) every settable level, singly and paired, must be settable, + // (3) resetting to "all frequencies" must succeed (or legitimately INVAL if + // nothing is settable), + // (4) perf level is restored to AUTO. + // Set clocks to something other than the usual default of the lowest // frequency. // Skip AMDSMI_CLK_TYPE_PCIE, which does not supported in rocm-smi. if (amdsmi_clk == AMDSMI_CLK_TYPE_PCIE) return; - freq_bitmask = 0b01100; // Try the 3rd and 4th DPM levels - - std::string freq_bm_str = std::bitset(freq_bitmask).to_string(); + uint32_t settable = settable_level_count(f); + + // False when a set failure just means "unsupported on this ASIC", not + // a real test failure; also prints why the rest of the check is + // skipped. Shared by the negative case and the per-level loop below. + auto is_set_supported = [&](amdsmi_status_t set_ret) { + if ((set_ret == AMDSMI_STATUS_NO_PERM && geteuid() == 0) || + (set_ret == AMDSMI_STATUS_NOT_SUPPORTED)) { + std::cout << "\t**Set " << FreqEnumToStr(amdsmi_clk) + << ": Not supported on this machine. Skipping..." << std::endl; + return false; + } + return true; + }; + + // "amdsmi_set_clk_freq(, )", shared by every set call + // below so the clock name is only spelled out once. + auto set_api_str = [&](const std::string& value) { + return "amdsmi_set_clk_freq(" + std::string(FreqEnumToStr(amdsmi_clk)) + ", " + value + + ")"; + }; + + // "gpu=, VALID"/"gpu=, INVALID" tag for the API call + // lines below, so it's clear at a glance which outcome is expected. + auto gpu_tag_str = [&](bool valid) { + return "gpu=" + std::to_string(dv_ind) + (valid ? ", VALID" : ", INVALID"); + }; + + // Re-reads f (the settable range can shrink live); callers assert + // whatever f-derived condition justifies the INVAL they just saw. + auto refresh_f = [&]() { + DISPLAY_AMDSMI_API("amdsmi_get_clk_freq", "gpu=" + std::to_string(dv_ind), + VERB(STANDARD)); + amdsmi_status_t refresh_ret = + amdsmi_get_clk_freq(processor_handles_[dv_ind], amdsmi_clk, &f); + DISPLAY_AMDSMI_STATUS(VERB(STANDARD), __FILE__, __LINE__, refresh_ret, + AMDSMI_STATUS_SUCCESS); + return refresh_ret; + }; + + // Negative case: a mask entirely above the settable range must be + // rejected with INVAL, regardless of how many levels are settable. + if (settable > 0) { + uint64_t invalid_bitmask = 1ULL << settable; + std::string invalid_bm_str = to_bit_string(invalid_bitmask); + + IF_VERB(STANDARD) { + std::cout << "Setting frequency mask for " << FreqEnumToStr(amdsmi_clk) << " to 0b" + << invalid_bm_str << " (level " << settable + << ", out-of-range, expect AMDSMI_STATUS_INVAL) ..." << std::endl; + } + DISPLAY_AMDSMI_API(set_api_str("0b" + invalid_bm_str), gpu_tag_str(false), + VERB(STANDARD)); + amdsmi_status_t invalid_ret = + amdsmi_set_clk_freq(processor_handles_[dv_ind], amdsmi_clk, invalid_bitmask); + DISPLAY_AMDSMI_STATUS(VERB(STANDARD), __FILE__, __LINE__, invalid_ret, + AMDSMI_STATUS_INVAL, AMDSMI_STATUS_NOT_SUPPORTED, + AMDSMI_STATUS_NO_PERM); + ASSERT_TRUE(invalid_ret == AMDSMI_STATUS_INVAL || + invalid_ret == AMDSMI_STATUS_NOT_SUPPORTED || + invalid_ret == AMDSMI_STATUS_NO_PERM); + if (!is_set_supported(invalid_ret)) { + return; + } + ASSERT_EQ(invalid_ret, AMDSMI_STATUS_INVAL); + } - freq_bm_str.erase(0, std::min(freq_bm_str.find_first_not_of('0'), freq_bm_str.size() - 1)); + // Positive case: exercise every settable DPM level - bit i alone, or + // paired with i+1 when both are settable, so both single- and + // multi-bit masks get a real (non-INVAL) write on every device + // regardless of table size. + // f.frequency[] is positional: index 0 is the deep-sleep entry when + // present, so the first settable level is offset by 1 in that case. + uint32_t freq_idx_offset = f.has_deep_sleep ? 1 : 0; + std::string set_api; + for (uint32_t i = 0; i < settable; ++i) { + freq_bitmask = (1ULL << i); + if (i + 1 < settable) { + freq_bitmask |= (1ULL << (i + 1)); + } - IF_VERB(STANDARD) { - std::cout << "Setting frequency mask for " << FreqEnumToStr(amdsmi_clk) << " to 0b" - << freq_bm_str << " ..." << std::endl; - } - std::string set_api = "amdsmi_set_clk_freq(" + std::string(FreqEnumToStr(amdsmi_clk)) + - ", 0b" + freq_bm_str + ")"; - DISPLAY_AMDSMI_API(set_api, "gpu=" + std::to_string(dv_ind), VERB(STANDARD)); - ret = amdsmi_set_clk_freq(processor_handles_[dv_ind], amdsmi_clk, freq_bitmask); - DISPLAY_AMDSMI_STATUS(VERB(STANDARD), __FILE__, __LINE__, ret, AMDSMI_STATUS_SUCCESS, - AMDSMI_STATUS_NOT_SUPPORTED, AMDSMI_STATUS_NO_PERM, - AMDSMI_STATUS_INVAL); - // Certain ASICs does not allow to set particular clocks. If set function for a clock - // returns permission error despite root access, manually set ret value to success and - // return - // - // Sometimes setting clock frequencies is completely not supported - if ((ret == AMDSMI_STATUS_NO_PERM && geteuid() == 0) || - (ret == AMDSMI_STATUS_NOT_SUPPORTED)) { - std::cout << "\t**Set " << FreqEnumToStr(amdsmi_clk) - << ": Not supported on this machine. Skipping..." << std::endl; - ret = AMDSMI_STATUS_SUCCESS; - return; - } + std::string freq_bm_str = to_bit_string(freq_bitmask); - // Lenient set contract: amd-smi applies the in-range subset of the - // requested mask and drops labels above the max settable level (like the - // driver), so a set only fails with INVAL when the mask selects no - // settable level at all. - if (ret == AMDSMI_STATUS_INVAL) { - std::cout << "\t**[Warn] AMDSMI_STATUS_INVAL - Could not set frequency mask for " - << FreqEnumToStr(amdsmi_clk) << " to 0b" << freq_bm_str << " on device [" - << dv_ind << "]!" << std::endl; - ASSERT_EQ(effective_level_mask(freq_bitmask, f), 0ULL); - } else { + IF_VERB(STANDARD) { + std::cout << "Setting frequency mask for " << FreqEnumToStr(amdsmi_clk) << " to 0b" + << freq_bm_str << " (" << f.frequency[i + freq_idx_offset] << " Hz"; + if (i + 1 < settable) { + std::cout << ", " << f.frequency[i + 1 + freq_idx_offset] << " Hz"; + } + std::cout << ", expect AMDSMI_STATUS_SUCCESS) ..." << std::endl; + } + set_api = set_api_str("0b" + freq_bm_str); + DISPLAY_AMDSMI_API(set_api, gpu_tag_str(true), VERB(STANDARD)); + ret = amdsmi_set_clk_freq(processor_handles_[dv_ind], amdsmi_clk, freq_bitmask); + DISPLAY_AMDSMI_STATUS(VERB(STANDARD), __FILE__, __LINE__, ret, AMDSMI_STATUS_SUCCESS, + AMDSMI_STATUS_INVAL, AMDSMI_STATUS_NOT_SUPPORTED, + AMDSMI_STATUS_NO_PERM); + ASSERT_TRUE(ret == AMDSMI_STATUS_SUCCESS || ret == AMDSMI_STATUS_INVAL || + ret == AMDSMI_STATUS_NOT_SUPPORTED || ret == AMDSMI_STATUS_NO_PERM); + if (!is_set_supported(ret)) { + return; + } + if (ret == AMDSMI_STATUS_INVAL) { + // The settable range can shrink between planning this mask and the + // driver processing it (live power-state fluctuation, same as + // deep-sleep detection). Re-read after the fact - not before - so + // there's no window left for the state to change again before we + // judge whether this INVAL was actually justified. + ASSERT_EQ(refresh_f(), AMDSMI_STATUS_SUCCESS); + ASSERT_GE(i, settable_level_count(f)); + continue; + } CHK_ERR_ASRT(ret) } - DISPLAY_AMDSMI_API("amdsmi_get_clk_freq", "gpu=" + std::to_string(dv_ind), VERB(STANDARD)); - ret = amdsmi_get_clk_freq(processor_handles_[dv_ind], amdsmi_clk, &f); - DISPLAY_AMDSMI_STATUS(VERB(STANDARD), __FILE__, __LINE__, ret, AMDSMI_STATUS_SUCCESS); - if (ret != AMDSMI_STATUS_SUCCESS) { - return; - } + // Refresh f for the printout below; the reset step's own INVAL + // check re-reads again regardless, so this is mainly a post-loop + // sanity check plus display info, not load-bearing for the reset. + ASSERT_EQ(refresh_f(), AMDSMI_STATUS_SUCCESS); IF_VERB(STANDARD) { std::cout << "Frequency is now index " << f.current << std::endl; std::cout << "Resetting mask to all frequencies." << std::endl; } freq_bitmask = 0xFFFFFFFF; - set_api = "amdsmi_set_clk_freq(" + std::string(FreqEnumToStr(amdsmi_clk)) + ", 0xFFFFFFFF)"; - DISPLAY_AMDSMI_API(set_api, "gpu=" + std::to_string(dv_ind), VERB(STANDARD)); + set_api = set_api_str("0xFFFFFFFF"); + DISPLAY_AMDSMI_API(set_api, gpu_tag_str(true), VERB(STANDARD)); ret = amdsmi_set_clk_freq(processor_handles_[dv_ind], amdsmi_clk, freq_bitmask); DISPLAY_AMDSMI_STATUS(VERB(STANDARD), __FILE__, __LINE__, ret, AMDSMI_STATUS_SUCCESS, - AMDSMI_STATUS_INVAL); - if (ret == AMDSMI_STATUS_NOT_SUPPORTED) { - ret = AMDSMI_STATUS_SUCCESS; + AMDSMI_STATUS_INVAL, AMDSMI_STATUS_NOT_SUPPORTED, + AMDSMI_STATUS_NO_PERM); + ASSERT_TRUE(ret == AMDSMI_STATUS_SUCCESS || ret == AMDSMI_STATUS_INVAL || + ret == AMDSMI_STATUS_NOT_SUPPORTED || ret == AMDSMI_STATUS_NO_PERM); + if (!is_set_supported(ret)) { return; } if (ret == AMDSMI_STATUS_INVAL) { + // has_deep_sleep can change between reads, so re-read f here instead + // of reusing the snapshot taken before this reset attempt. + ASSERT_EQ(refresh_f(), AMDSMI_STATUS_SUCCESS); // Only tolerated for a clock with no settable level (e.g. deep-sleep // only); otherwise the "enable all" reset must succeed. ASSERT_EQ(effective_level_mask(freq_bitmask, f), 0ULL); @@ -245,10 +327,12 @@ void TestFrequenciesReadWrite::Run(void) { DISPLAY_AMDSMI_API("amdsmi_set_gpu_perf_level", "gpu=" + std::to_string(dv_ind), VERB(STANDARD)); ret = amdsmi_set_gpu_perf_level(processor_handles_[dv_ind], AMDSMI_DEV_PERF_LEVEL_AUTO); - DISPLAY_AMDSMI_STATUS(VERB(STANDARD), __FILE__, __LINE__, ret, AMDSMI_STATUS_SUCCESS); - if (ret == AMDSMI_STATUS_NOT_SUPPORTED) { - ret = AMDSMI_STATUS_SUCCESS; - return; + DISPLAY_AMDSMI_STATUS(VERB(STANDARD), __FILE__, __LINE__, ret, AMDSMI_STATUS_SUCCESS, + AMDSMI_STATUS_NOT_SUPPORTED, AMDSMI_STATUS_NO_PERM); + ASSERT_TRUE(ret == AMDSMI_STATUS_SUCCESS || ret == AMDSMI_STATUS_NOT_SUPPORTED || + ret == AMDSMI_STATUS_NO_PERM); + if (is_set_supported(ret)) { + CHK_ERR_ASRT(ret) } }; @@ -258,9 +342,6 @@ void TestFrequenciesReadWrite::Run(void) { continue; } freq_write(); - if (ret != AMDSMI_STATUS_INVAL) { - CHK_ERR_ASRT(ret) - } } } }