From 92465e58974ef10eea48877b3b26c548088090eb Mon Sep 17 00:00:00 2001 From: Koushik Billakanti Date: Tue, 11 Aug 2026 11:56:27 -0500 Subject: [PATCH 1/4] fix(amdsmi): FCLK max no longer reported as 0 --- projects/amdsmi/CHANGELOG.md | 3 +++ projects/amdsmi/src/amd_smi/amd_smi_utils.cc | 27 +++++++++++++++----- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/projects/amdsmi/CHANGELOG.md b/projects/amdsmi/CHANGELOG.md index 8cbaf2b9461e..94d7f2dbfd75 100644 --- a/projects/amdsmi/CHANGELOG.md +++ b/projects/amdsmi/CHANGELOG.md @@ -135,6 +135,9 @@ GPU: 0 ### Resolved Issues +- **Fixed FCLK maximum clock reported as 0 MHz**. + - `amdsmi_get_clock_info()` derived the FCLK (data-fabric) range from `pp_od_clk_voltage`, which on some GPUs (e.g. MI45x) lists only `OD_SCLK`/`OD_MCLK` and no FCLK section. The parsed maximum stayed at 0 and the code never fell back to `pp_dpm_fclk`, so `amd-smi metric --clock` showed `FCLK_0 MAX_CLK: 0 MHz`. The range now falls back to `pp_dpm_fclk` (and the analogous `pp_dpm_sclk`/`pp_dpm_mclk`) whenever the overdrive file omits that domain. + - **Fixed `amd-smi ras --cper --json` emitting nothing when there are no CPER entries**. - The common no-entries case printed empty output, so consumers feeding stdout to `json.loads` failed with `Expecting value: line 1 column 1 (char 0)`. The command now always emits exactly one valid JSON document: `[]` when there are no entries, or a single aggregated array across all GPUs when there are. `--follow` mode stays silent until entries appear. The human-readable primary-partition warning is also suppressed in JSON mode so it no longer corrupts the output. diff --git a/projects/amdsmi/src/amd_smi/amd_smi_utils.cc b/projects/amdsmi/src/amd_smi/amd_smi_utils.cc index 820ba98eac02..6e94e1c41aeb 100644 --- a/projects/amdsmi/src/amd_smi/amd_smi_utils.cc +++ b/projects/amdsmi/src/amd_smi/amd_smi_utils.cc @@ -402,15 +402,30 @@ amdsmi_status_t smi_amdgpu_get_ranges(amd::smi::AMDSmiGPUDevice* device, amdsmi_ } } + // pp_od_clk_voltage may omit a domain that lacks overdrive support + // (e.g. no OD_FCLK section on MI45x), leaving its parsed max at 0. In + // that case clear the flag so the pp_dpm_* loop below derives min/max. if (sclk) { - max = smax; - min = smin; + if (smax > 0) { + max = smax; + min = smin; + } else { + sclk = false; + } } else if (mclk) { - max = mmax; - min = mmin; + if (mmax > 0) { + max = mmax; + min = mmin; + } else { + mclk = false; + } } else if (fclk) { - max = fmax; - min = fmin; + if (fmax > 0) { + max = fmax; + min = fmin; + } else { + fclk = false; + } } smclk_ranges.close(); From 2cd045a5d6b2f0c13d714047601a2b32978dd043 Mon Sep 17 00:00:00 2001 From: Koushik Billakanti Date: Wed, 12 Aug 2026 10:01:44 -0500 Subject: [PATCH 2/4] test(amdsmi): cover FCLK overdrive-range fallback --- .../include/amd_smi/impl/amd_smi_utils.h | 7 + projects/amdsmi/src/amd_smi/amd_smi_utils.cc | 130 ++++++++---------- .../amd_smi_test/unit/gpu/clock_range_test.cc | 117 ++++++++++++++++ 3 files changed, 185 insertions(+), 69 deletions(-) create mode 100644 projects/amdsmi/tests/amd_smi_test/unit/gpu/clock_range_test.cc diff --git a/projects/amdsmi/include/amd_smi/impl/amd_smi_utils.h b/projects/amdsmi/include/amd_smi/impl/amd_smi_utils.h index 4a275c485211..2df7f7423c87 100644 --- a/projects/amdsmi/include/amd_smi/impl/amd_smi_utils.h +++ b/projects/amdsmi/include/amd_smi/impl/amd_smi_utils.h @@ -29,6 +29,7 @@ #include #include #include +#include #include #include #include @@ -50,6 +51,12 @@ amdsmi_status_t smi_amdgpu_get_power_cap(amd::smi::AMDSmiGPUDevice* device, uint amdsmi_status_t smi_amdgpu_get_ranges(amd::smi::AMDSmiGPUDevice* device, amdsmi_clk_type_t domain, int* max_freq, int* min_freq, int* num_dpm, int* sleep_state_freq); +// Parse a pp_od_clk_voltage stream for one clock domain's user-defined min/max. +// Returns true (writing *max_freq/*min_freq) when the domain's section exists; +// false when it is absent -- e.g. MI45x omits OD_FCLK -- so the caller falls +// back to pp_dpm_*. Pure over a stream to keep the fallback path unit-testable. +bool smi_amdgpu_parse_od_clk_range(std::istream& od_stream, amdsmi_clk_type_t domain, + unsigned int* max_freq, unsigned int* min_freq); amdsmi_status_t smi_amdgpu_get_enabled_blocks(amd::smi::AMDSmiGPUDevice* device, uint64_t* enabled_blocks); amdsmi_status_t smi_amdgpu_get_bad_page_info(amd::smi::AMDSmiGPUDevice* device, uint32_t* num_pages, diff --git a/projects/amdsmi/src/amd_smi/amd_smi_utils.cc b/projects/amdsmi/src/amd_smi/amd_smi_utils.cc index 6e94e1c41aeb..671df27b2cd9 100644 --- a/projects/amdsmi/src/amd_smi/amd_smi_utils.cc +++ b/projects/amdsmi/src/amd_smi/amd_smi_utils.cc @@ -296,6 +296,62 @@ amdsmi_status_t smi_amdgpu_get_power_cap(amd::smi::AMDSmiGPUDevice* device, uint return AMDSMI_STATUS_SUCCESS; } +bool smi_amdgpu_parse_od_clk_range(std::istream& od_stream, amdsmi_clk_type_t domain, + unsigned int* max_freq, unsigned int* min_freq) { + // Section header (and its GFXCLK/MCLK/FCLK alias) whose levels feed this + // domain's user-defined range. + const char* od_header = nullptr; + const char* alias_header = nullptr; + switch (domain) { + case AMDSMI_CLK_TYPE_GFX: + od_header = "OD_SCLK:"; + alias_header = "GFXCLK:"; + break; + case AMDSMI_CLK_TYPE_MEM: + od_header = "OD_MCLK:"; + alias_header = "MCLK:"; + break; + case AMDSMI_CLK_TYPE_DF: + od_header = "OD_FCLK:"; + alias_header = "FCLK:"; + break; + default: + return false; + } + + unsigned int max = 0; + unsigned int min = UINT_MAX; + bool in_domain = false; + bool found = false; + char str[10]; + unsigned int dpm_level, freq; + for (std::string line; getline(od_stream, line);) { + // A recognized header selects the active domain; a non-matching one ends it. + if (line.compare("GFXCLK:") == 0 || line.compare("OD_SCLK:") == 0 || + line.compare("MCLK:") == 0 || line.compare("OD_MCLK:") == 0 || line.compare("FCLK:") == 0 || + line.compare("OD_FCLK:") == 0) { + in_domain = line.compare(od_header) == 0 || line.compare(alias_header) == 0; + continue; + } + if (!in_domain) { + continue; + } + if (sscanf(line.c_str(), "%u: %d%9s", &dpm_level, &freq, str) <= 2) { + continue; // skip lines that don't conform to the format + } + found = true; + if (freq > max) max = freq; + if (freq < min) min = freq; + } + + if (!found || max == 0) { + return false; + } + *max_freq = max; + *min_freq = min; + return true; +} + amdsmi_status_t smi_amdgpu_get_ranges(amd::smi::AMDSmiGPUDevice* device, amdsmi_clk_type_t domain, int* max_freq, int* min_freq, int* num_dpm, int* sleep_state_freq) { @@ -355,80 +411,16 @@ amdsmi_status_t smi_amdgpu_get_ranges(amd::smi::AMDSmiGPUDevice* device, amdsmi_ dpm = 0; sleep_freq = UINT_MAX; current_freq = 0; - // if getting sclk, mclk or fclk info, read pp_od_clk_voltage for min and max info + // sclk/mclk/fclk expose a user-defined range in pp_od_clk_voltage. When that + // file is missing, or present but omits this domain's section (e.g. no + // OD_FCLK on MI45x), fall back to deriving min/max from the pp_dpm_* levels + // below. if (sclk || mclk || fclk) { std::ifstream smclk_ranges(smclk_min_max_fullpath.c_str()); - unsigned int smax = 0; - unsigned int mmax = 0; - unsigned int fmax = 0; - unsigned int smin = UINT_MAX; - unsigned int mmin = UINT_MAX; - unsigned int fmin = UINT_MAX; - - // if pp_od_clk_voltage is not found, then go back to using the original pp_dpm files - if (!smclk_ranges.is_open()) { + if (!smi_amdgpu_parse_od_clk_range(smclk_ranges, domain, &max, &min)) { sclk = false; mclk = false; fclk = false; - } else { - // using enum to switch between recording for sclk, mclk, or fclk - enum ClkType { PARSING_SCLK = 0, PARSING_MCLK = 1, PARSING_FCLK = 2 }; - ClkType current_clk_type = PARSING_SCLK; - unsigned int dpm_level, freq; - for (std::string line; getline(smclk_ranges, line);) { - if (line.compare("GFXCLK:") == 0 || line.compare("OD_SCLK:") == 0) { - current_clk_type = PARSING_SCLK; - continue; - } else if (line.compare("MCLK:") == 0 || line.compare("OD_MCLK:") == 0) { - current_clk_type = PARSING_MCLK; - continue; - } else if (line.compare("FCLK:") == 0 || line.compare("OD_FCLK:") == 0) { - current_clk_type = PARSING_FCLK; - continue; - } - if (sscanf(line.c_str(), "%u: %d%9s", &dpm_level, &freq, str) <= 2) { - // skip lines that don't conform to the format - continue; - } - if (current_clk_type == PARSING_SCLK) { - if (freq > smax) smax = freq; - if (freq < smin) smin = freq; - } else if (current_clk_type == PARSING_MCLK) { - if (freq > mmax) mmax = freq; - if (freq < mmin) mmin = freq; - } else if (current_clk_type == PARSING_FCLK) { - if (freq > fmax) fmax = freq; - if (freq < fmin) fmin = freq; - } - } - - // pp_od_clk_voltage may omit a domain that lacks overdrive support - // (e.g. no OD_FCLK section on MI45x), leaving its parsed max at 0. In - // that case clear the flag so the pp_dpm_* loop below derives min/max. - if (sclk) { - if (smax > 0) { - max = smax; - min = smin; - } else { - sclk = false; - } - } else if (mclk) { - if (mmax > 0) { - max = mmax; - min = mmin; - } else { - mclk = false; - } - } else if (fclk) { - if (fmax > 0) { - max = fmax; - min = fmin; - } else { - fclk = false; - } - } - - smclk_ranges.close(); } } // obtain rest of info from regular pp_dpm_* files. 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 new file mode 100644 index 000000000000..e10ca21f23d8 --- /dev/null +++ b/projects/amdsmi/tests/amd_smi_test/unit/gpu/clock_range_test.cc @@ -0,0 +1,117 @@ +/* + * Copyright (c) Advanced Micro Devices, Inc. All rights reserved. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +// 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. + +#include + +#include +#include + +#include "amd_smi/amdsmi.h" +#include "amd_smi/impl/amd_smi_utils.h" + +namespace { + +// 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[] = + "OD_SCLK:\n" + "0: 500Mhz\n" + "1: 2100Mhz\n" + "OD_MCLK:\n" + "0: 900Mhz\n" + "1: 1200Mhz\n" + "OD_RANGE:\n" + "SCLK: 500Mhz 2100Mhz\n" + "MCLK: 900Mhz 1200Mhz\n"; + +// Same layout with an explicit OD_FCLK section present. +constexpr char kOdWithFclk[] = + "OD_SCLK:\n" + "0: 500Mhz\n" + "1: 2100Mhz\n" + "OD_FCLK:\n" + "0: 1000Mhz\n" + "1: 1100Mhz\n"; + +TEST(GpuUnit, OdClkRangeReadsSclkSection) { + std::istringstream od(kOdNoFclk); + unsigned int max = 0; + unsigned int min = UINT_MAX; + EXPECT_TRUE(smi_amdgpu_parse_od_clk_range(od, AMDSMI_CLK_TYPE_GFX, &max, &min)); + EXPECT_EQ(max, 2100u); + EXPECT_EQ(min, 500u); +} + +TEST(GpuUnit, OdClkRangeReadsMclkSection) { + std::istringstream od(kOdNoFclk); + unsigned int max = 0; + unsigned int min = UINT_MAX; + EXPECT_TRUE(smi_amdgpu_parse_od_clk_range(od, AMDSMI_CLK_TYPE_MEM, &max, &min)); + EXPECT_EQ(max, 1200u); + EXPECT_EQ(min, 900u); +} + +// The regression: with no OD_FCLK section the parser reports "absent" (false) +// and leaves the out-params untouched, so the caller derives FCLK min/max from +// pp_dpm_fclk rather than reporting 0. +TEST(GpuUnit, OdClkRangeFallsBackWhenFclkSectionMissing) { + std::istringstream od(kOdNoFclk); + unsigned int max = 4242; + unsigned int min = 4242; + EXPECT_FALSE(smi_amdgpu_parse_od_clk_range(od, AMDSMI_CLK_TYPE_DF, &max, &min)); + EXPECT_EQ(max, 4242u); + EXPECT_EQ(min, 4242u); +} + +// With OD_FCLK present the range is read directly, no fallback. +TEST(GpuUnit, OdClkRangeReadsFclkSectionWhenPresent) { + std::istringstream od(kOdWithFclk); + unsigned int max = 0; + unsigned int min = UINT_MAX; + EXPECT_TRUE(smi_amdgpu_parse_od_clk_range(od, AMDSMI_CLK_TYPE_DF, &max, &min)); + EXPECT_EQ(max, 1100u); + EXPECT_EQ(min, 1000u); +} + +// A missing/empty pp_od_clk_voltage yields no section -> fall back. +TEST(GpuUnit, OdClkRangeFallsBackOnEmptyStream) { + std::istringstream od(""); + unsigned int max = 7; + unsigned int min = 7; + EXPECT_FALSE(smi_amdgpu_parse_od_clk_range(od, AMDSMI_CLK_TYPE_GFX, &max, &min)); +} + +// Domains that have no overdrive section (e.g. SOC) are never OD-backed. +TEST(GpuUnit, OdClkRangeRejectsNonOdDomain) { + std::istringstream od(kOdWithFclk); + unsigned int max = 7; + unsigned int min = 7; + EXPECT_FALSE(smi_amdgpu_parse_od_clk_range(od, AMDSMI_CLK_TYPE_SOC, &max, &min)); +} + +} // namespace From 0a465c0d10ad010e661b982995f74aceebd3cdfc Mon Sep 17 00:00:00 2001 From: Koushik Billakanti Date: Fri, 14 Aug 2026 14:31:25 -0500 Subject: [PATCH 3/4] fix(amdsmi): harden FCLK overdrive clock-range parsing per review --- projects/amdsmi/CHANGELOG.md | 2 +- .../include/amd_smi/impl/amd_smi_utils.h | 7 +- projects/amdsmi/src/amd_smi/amd_smi_utils.cc | 6 +- .../amd_smi_test/unit/gpu/clock_range_test.cc | 71 +++++++++++++++++++ 4 files changed, 79 insertions(+), 7 deletions(-) diff --git a/projects/amdsmi/CHANGELOG.md b/projects/amdsmi/CHANGELOG.md index 94d7f2dbfd75..ddaf7becf5bc 100644 --- a/projects/amdsmi/CHANGELOG.md +++ b/projects/amdsmi/CHANGELOG.md @@ -135,7 +135,7 @@ GPU: 0 ### Resolved Issues -- **Fixed FCLK maximum clock reported as 0 MHz**. +- **Fixed `amd-smi metric --clock` reporting FCLK `max_clk` as 0 MHz**. - `amdsmi_get_clock_info()` derived the FCLK (data-fabric) range from `pp_od_clk_voltage`, which on some GPUs (e.g. MI45x) lists only `OD_SCLK`/`OD_MCLK` and no FCLK section. The parsed maximum stayed at 0 and the code never fell back to `pp_dpm_fclk`, so `amd-smi metric --clock` showed `FCLK_0 MAX_CLK: 0 MHz`. The range now falls back to `pp_dpm_fclk` (and the analogous `pp_dpm_sclk`/`pp_dpm_mclk`) whenever the overdrive file omits that domain. - **Fixed `amd-smi ras --cper --json` emitting nothing when there are no CPER entries**. diff --git a/projects/amdsmi/include/amd_smi/impl/amd_smi_utils.h b/projects/amdsmi/include/amd_smi/impl/amd_smi_utils.h index 2df7f7423c87..f0e040569aee 100644 --- a/projects/amdsmi/include/amd_smi/impl/amd_smi_utils.h +++ b/projects/amdsmi/include/amd_smi/impl/amd_smi_utils.h @@ -52,9 +52,10 @@ 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); // Parse a pp_od_clk_voltage stream for one clock domain's user-defined min/max. -// Returns true (writing *max_freq/*min_freq) when the domain's section exists; -// false when it is absent -- e.g. MI45x omits OD_FCLK -- so the caller falls -// back to pp_dpm_*. Pure over a stream to keep the fallback path unit-testable. +// Returns true (writing *max_freq/*min_freq) when the domain's section exists +// and yields a nonzero max; false when the section is absent -- e.g. MI45x omits +// OD_FCLK -- or reports only zero levels, so the caller falls back to pp_dpm_*. +// Pure over a stream to keep the fallback path unit-testable. bool smi_amdgpu_parse_od_clk_range(std::istream& od_stream, amdsmi_clk_type_t domain, unsigned int* max_freq, unsigned int* min_freq); amdsmi_status_t smi_amdgpu_get_enabled_blocks(amd::smi::AMDSmiGPUDevice* device, diff --git a/projects/amdsmi/src/amd_smi/amd_smi_utils.cc b/projects/amdsmi/src/amd_smi/amd_smi_utils.cc index 671df27b2cd9..282b85d15bdf 100644 --- a/projects/amdsmi/src/amd_smi/amd_smi_utils.cc +++ b/projects/amdsmi/src/amd_smi/amd_smi_utils.cc @@ -336,7 +336,7 @@ bool smi_amdgpu_parse_od_clk_range(std::istream& od_stream, amdsmi_clk_type_t do if (!in_domain) { continue; } - if (sscanf(line.c_str(), "%u: %d%9s", &dpm_level, &freq, str) <= 2) { + if (sscanf(line.c_str(), "%u: %u%9s", &dpm_level, &freq, str) <= 2) { continue; // skip lines that don't conform to the format } found = true; @@ -429,7 +429,7 @@ amdsmi_status_t smi_amdgpu_get_ranges(amd::smi::AMDSmiGPUDevice* device, amdsmi_ char firstChar = line[0]; if (firstChar == 'S') { - if (sscanf(line.c_str(), "%c: %d%9s", &single_char, &sleep_freq, str) <= 2) { + if (sscanf(line.c_str(), "%c: %u%9s", &single_char, &sleep_freq, str) <= 2) { ranges.close(); return AMDSMI_STATUS_NO_DATA; } @@ -443,7 +443,7 @@ amdsmi_status_t smi_amdgpu_get_ranges(amd::smi::AMDSmiGPUDevice* device, amdsmi_ * incorrect min value. */ - if (sscanf(line.c_str(), "%u: %d%c", &dpm_level, &freq, str) <= 2) { + if (sscanf(line.c_str(), "%u: %u%c", &dpm_level, &freq, str) <= 2) { ranges.close(); return AMDSMI_STATUS_IO; } 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 e10ca21f23d8..5db067ba6c75 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 @@ -58,6 +58,31 @@ constexpr char kOdWithFclk[] = "0: 1000Mhz\n" "1: 1100Mhz\n"; +// Bare GFXCLK/MCLK/FCLK aliases in place of the OD_* headers, as some GPUs emit. +constexpr char kOdAliasHeaders[] = + "GFXCLK:\n" + "0: 500Mhz\n" + "1: 2100Mhz\n" + "MCLK:\n" + "0: 900Mhz\n" + "1: 1200Mhz\n" + "FCLK:\n" + "0: 1000Mhz\n" + "1: 1100Mhz\n"; + +// OD_FCLK present but every level reads 0 -- no usable range. +constexpr char kOdFclkAllZero[] = + "OD_FCLK:\n" + "0: 0Mhz\n" + "1: 0Mhz\n"; + +// OD_FCLK with a non-conforming line between two valid levels. +constexpr char kOdFclkGarbageLine[] = + "OD_FCLK:\n" + "0: 1000Mhz\n" + "not-a-level\n" + "1: 1100Mhz\n"; + TEST(GpuUnit, OdClkRangeReadsSclkSection) { std::istringstream od(kOdNoFclk); unsigned int max = 0; @@ -114,4 +139,50 @@ TEST(GpuUnit, OdClkRangeRejectsNonOdDomain) { EXPECT_FALSE(smi_amdgpu_parse_od_clk_range(od, AMDSMI_CLK_TYPE_SOC, &max, &min)); } +// The bare GFXCLK/MCLK/FCLK aliases are accepted just like the OD_* headers. +TEST(GpuUnit, OdClkRangeReadsAliasHeaders) { + unsigned int max = 0; + unsigned int min = UINT_MAX; + std::istringstream gfx(kOdAliasHeaders); + EXPECT_TRUE(smi_amdgpu_parse_od_clk_range(gfx, AMDSMI_CLK_TYPE_GFX, &max, &min)); + EXPECT_EQ(max, 2100u); + EXPECT_EQ(min, 500u); + + max = 0; + min = UINT_MAX; + std::istringstream mem(kOdAliasHeaders); + EXPECT_TRUE(smi_amdgpu_parse_od_clk_range(mem, AMDSMI_CLK_TYPE_MEM, &max, &min)); + EXPECT_EQ(max, 1200u); + EXPECT_EQ(min, 900u); + + max = 0; + min = UINT_MAX; + std::istringstream df(kOdAliasHeaders); + EXPECT_TRUE(smi_amdgpu_parse_od_clk_range(df, AMDSMI_CLK_TYPE_DF, &max, &min)); + EXPECT_EQ(max, 1100u); + EXPECT_EQ(min, 1000u); +} + +// Section present but every level parses to 0 -> reported absent so the caller +// still falls back to pp_dpm_*. +TEST(GpuUnit, OdClkRangeFallsBackWhenAllLevelsZero) { + std::istringstream od(kOdFclkAllZero); + unsigned int max = 555; + unsigned int min = 555; + EXPECT_FALSE(smi_amdgpu_parse_od_clk_range(od, AMDSMI_CLK_TYPE_DF, &max, &min)); + EXPECT_EQ(max, 555u); + EXPECT_EQ(min, 555u); +} + +// A non-conforming line inside the section is skipped, not treated as its end, +// so levels on both sides still contribute to the range. +TEST(GpuUnit, OdClkRangeSkipsMalformedLineWithinSection) { + std::istringstream od(kOdFclkGarbageLine); + unsigned int max = 0; + unsigned int min = UINT_MAX; + EXPECT_TRUE(smi_amdgpu_parse_od_clk_range(od, AMDSMI_CLK_TYPE_DF, &max, &min)); + EXPECT_EQ(max, 1100u); + EXPECT_EQ(min, 1000u); +} + } // namespace From 9b0aab764ea27b2cde205e1fedc0fbadd160d1cd Mon Sep 17 00:00:00 2001 From: Koushik Billakanti Date: Tue, 25 Aug 2026 14:43:06 -0500 Subject: [PATCH 4/4] fix(amdsmi): address FCLK clock-range review feedback --- .../amd_smi/impl/amd_smi_clk_testing.h | 39 +++++++++++ .../include/amd_smi/impl/amd_smi_utils.h | 7 -- projects/amdsmi/src/amd_smi/amd_smi_utils.cc | 37 +++++------ .../amd_smi_test/unit/gpu/clock_range_test.cc | 64 ++++++++++++++++++- 4 files changed, 118 insertions(+), 29 deletions(-) create mode 100644 projects/amdsmi/include/amd_smi/impl/amd_smi_clk_testing.h diff --git a/projects/amdsmi/include/amd_smi/impl/amd_smi_clk_testing.h b/projects/amdsmi/include/amd_smi/impl/amd_smi_clk_testing.h new file mode 100644 index 000000000000..a43fab10a472 --- /dev/null +++ b/projects/amdsmi/include/amd_smi/impl/amd_smi_clk_testing.h @@ -0,0 +1,39 @@ +/* + * Copyright (c) Advanced Micro Devices, Inc. All rights reserved. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +#pragma once + +#include + +#include "amd_smi/amdsmi.h" + +// Library-local test seam for the pp_od_clk_voltage parser behind amd-smi's +// per-domain min/max clock. Not amdsmi_-prefixed, so the linker version script +// keeps it out of libamd_smi.so; tests reach it through the static archive. +// Shared by the definition (src/amd_smi/amd_smi_utils.cc) and the unit tests so +// the signature stays in sync. +// +// Returns true and writes *max_freq/*min_freq when the domain's overdrive +// section has a nonzero max; false when the section is absent (MI45x has no +// OD_FCLK) or all levels read zero, so the caller falls back to pp_dpm_*. +bool smi_amdgpu_parse_od_clk_range(std::istream& od_stream, amdsmi_clk_type_t domain, + unsigned int* max_freq, unsigned int* min_freq); diff --git a/projects/amdsmi/include/amd_smi/impl/amd_smi_utils.h b/projects/amdsmi/include/amd_smi/impl/amd_smi_utils.h index f0e040569aee..4727e542a0fd 100644 --- a/projects/amdsmi/include/amd_smi/impl/amd_smi_utils.h +++ b/projects/amdsmi/include/amd_smi/impl/amd_smi_utils.h @@ -51,13 +51,6 @@ amdsmi_status_t smi_amdgpu_get_power_cap(amd::smi::AMDSmiGPUDevice* device, uint amdsmi_status_t smi_amdgpu_get_ranges(amd::smi::AMDSmiGPUDevice* device, amdsmi_clk_type_t domain, int* max_freq, int* min_freq, int* num_dpm, int* sleep_state_freq); -// Parse a pp_od_clk_voltage stream for one clock domain's user-defined min/max. -// Returns true (writing *max_freq/*min_freq) when the domain's section exists -// and yields a nonzero max; false when the section is absent -- e.g. MI45x omits -// OD_FCLK -- or reports only zero levels, so the caller falls back to pp_dpm_*. -// Pure over a stream to keep the fallback path unit-testable. -bool smi_amdgpu_parse_od_clk_range(std::istream& od_stream, amdsmi_clk_type_t domain, - unsigned int* max_freq, unsigned int* min_freq); amdsmi_status_t smi_amdgpu_get_enabled_blocks(amd::smi::AMDSmiGPUDevice* device, uint64_t* enabled_blocks); amdsmi_status_t smi_amdgpu_get_bad_page_info(amd::smi::AMDSmiGPUDevice* device, uint32_t* num_pages, diff --git a/projects/amdsmi/src/amd_smi/amd_smi_utils.cc b/projects/amdsmi/src/amd_smi/amd_smi_utils.cc index 282b85d15bdf..1010818b6750 100644 --- a/projects/amdsmi/src/amd_smi/amd_smi_utils.cc +++ b/projects/amdsmi/src/amd_smi/amd_smi_utils.cc @@ -51,6 +51,7 @@ #include #include +#include "amd_smi/impl/amd_smi_clk_testing.h" #include "amd_smi/impl/amd_smi_common.h" #include "amd_smi/impl/amd_smi_gpu_mutex.h" #include "amd_smi/impl/amd_smi_system.h" @@ -326,10 +327,10 @@ bool smi_amdgpu_parse_od_clk_range(std::istream& od_stream, amdsmi_clk_type_t do char str[10]; unsigned int dpm_level, freq; for (std::string line; getline(od_stream, line);) { - // A recognized header selects the active domain; a non-matching one ends it. - if (line.compare("GFXCLK:") == 0 || line.compare("OD_SCLK:") == 0 || - line.compare("MCLK:") == 0 || line.compare("OD_MCLK:") == 0 || line.compare("FCLK:") == 0 || - line.compare("OD_FCLK:") == 0) { + // Section headers end with ':'. This domain's header (or its alias) starts + // capture; any other header ends it, so an adjacent section such as + // OD_VDDC_CURVE is not folded into the range. + if (!line.empty() && line.back() == ':') { in_domain = line.compare(od_header) == 0 || line.compare(alias_header) == 0; continue; } @@ -360,19 +361,17 @@ amdsmi_status_t smi_amdgpu_get_ranges(amd::smi::AMDSmiGPUDevice* device, amdsmi_ std::string smclk_min_max_fullpath = ""; - bool sclk = false; - bool mclk = false; - bool fclk = false; + bool use_od_range = false; switch (domain) { case AMDSMI_CLK_TYPE_GFX: smclk_min_max_fullpath = fullpath + "/pp_od_clk_voltage"; fullpath += "/pp_dpm_sclk"; - sclk = true; + use_od_range = true; break; case AMDSMI_CLK_TYPE_MEM: smclk_min_max_fullpath = fullpath + "/pp_od_clk_voltage"; fullpath += "/pp_dpm_mclk"; - mclk = true; + use_od_range = true; break; case AMDSMI_CLK_TYPE_VCLK0: fullpath += "/pp_dpm_vclk"; @@ -392,7 +391,7 @@ amdsmi_status_t smi_amdgpu_get_ranges(amd::smi::AMDSmiGPUDevice* device, amdsmi_ case AMDSMI_CLK_TYPE_DF: smclk_min_max_fullpath = fullpath + "/pp_od_clk_voltage"; fullpath += "/pp_dpm_fclk"; - fclk = true; + use_od_range = true; break; default: return AMDSMI_STATUS_INVAL; @@ -411,16 +410,13 @@ amdsmi_status_t smi_amdgpu_get_ranges(amd::smi::AMDSmiGPUDevice* device, amdsmi_ dpm = 0; sleep_freq = UINT_MAX; current_freq = 0; - // sclk/mclk/fclk expose a user-defined range in pp_od_clk_voltage. When that - // file is missing, or present but omits this domain's section (e.g. no - // OD_FCLK on MI45x), fall back to deriving min/max from the pp_dpm_* levels - // below. - if (sclk || mclk || fclk) { + // GFX/MEM/DF expose a user-defined range in pp_od_clk_voltage; when it omits + // this domain's section (e.g. no OD_FCLK on MI45x) fall back to the pp_dpm_* + // levels below. + if (use_od_range) { std::ifstream smclk_ranges(smclk_min_max_fullpath.c_str()); if (!smi_amdgpu_parse_od_clk_range(smclk_ranges, domain, &max, &min)) { - sclk = false; - mclk = false; - fclk = false; + use_od_range = false; } } // obtain rest of info from regular pp_dpm_* files. @@ -453,9 +449,8 @@ amdsmi_status_t smi_amdgpu_get_ranges(amd::smi::AMDSmiGPUDevice* device, amdsmi_ current_freq = freq; } - // not * was detected so check for the min max if not sclk, mclk, or fclk, which are user - // defined - if (!sclk && !mclk && !fclk) { + // Domains without an OD range derive min/max from the dpm levels here. + if (!use_od_range) { max = freq > max ? freq : max; min = freq < min ? freq : min; } 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 5db067ba6c75..64a6bada1bcb 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 @@ -32,7 +32,7 @@ #include #include "amd_smi/amdsmi.h" -#include "amd_smi/impl/amd_smi_utils.h" +#include "amd_smi/impl/amd_smi_clk_testing.h" namespace { @@ -83,6 +83,36 @@ constexpr char kOdFclkGarbageLine[] = "not-a-level\n" "1: 1100Mhz\n"; +// "Old Format" layout: OD_FCLK is immediately followed by OD_VDDC_CURVE, whose +// "idx: freq volt" lines resemble level lines and must not fold into the range. +constexpr char kOdFclkThenCurve[] = + "OD_SCLK:\n" + "0: 500Mhz\n" + "1: 2100Mhz\n" + "OD_FCLK:\n" + "0: 1000Mhz\n" + "1: 1100Mhz\n" + "OD_VDDC_CURVE:\n" + "0: 500Mhz 700mV\n" + "1: 1354Mhz 860mV\n" + "2: 2000Mhz 1150mV\n"; + +// Two OD_FCLK sections: levels from every occurrence merge into one range. +constexpr char kOdFclkDuplicate[] = + "OD_FCLK:\n" + "0: 1000Mhz\n" + "1: 1100Mhz\n" + "OD_SCLK:\n" + "0: 500Mhz\n" + "OD_FCLK:\n" + "0: 900Mhz\n" + "1: 2000Mhz\n"; + +// A single-level (locked-clock) section: min == max. +constexpr char kOdFclkSingleLevel[] = + "OD_FCLK:\n" + "0: 1500Mhz\n"; + TEST(GpuUnit, OdClkRangeReadsSclkSection) { std::istringstream od(kOdNoFclk); unsigned int max = 0; @@ -185,4 +215,36 @@ TEST(GpuUnit, OdClkRangeSkipsMalformedLineWithinSection) { EXPECT_EQ(min, 1000u); } +// An unrecognized section header (OD_VDDC_CURVE) ends FCLK parsing, so its curve +// lines are not mistaken for FCLK levels and do not inflate the max. +TEST(GpuUnit, OdClkRangeStopsAtUnrecognizedSectionHeader) { + std::istringstream od(kOdFclkThenCurve); + unsigned int max = 0; + unsigned int min = UINT_MAX; + EXPECT_TRUE(smi_amdgpu_parse_od_clk_range(od, AMDSMI_CLK_TYPE_DF, &max, &min)); + EXPECT_EQ(max, 1100u); + EXPECT_EQ(min, 1000u); +} + +// Repeated section headers merge: levels from every OD_FCLK occurrence feed one +// range. No live sysfs file repeats a header; this pins the contract. +TEST(GpuUnit, OdClkRangeMergesRepeatedSectionHeaders) { + std::istringstream od(kOdFclkDuplicate); + unsigned int max = 0; + unsigned int min = UINT_MAX; + EXPECT_TRUE(smi_amdgpu_parse_od_clk_range(od, AMDSMI_CLK_TYPE_DF, &max, &min)); + EXPECT_EQ(max, 2000u); + EXPECT_EQ(min, 900u); +} + +// A single-level (locked-clock) section reports min == max. +TEST(GpuUnit, OdClkRangeReadsSingleLevelSection) { + std::istringstream od(kOdFclkSingleLevel); + unsigned int max = 0; + unsigned int min = UINT_MAX; + EXPECT_TRUE(smi_amdgpu_parse_od_clk_range(od, AMDSMI_CLK_TYPE_DF, &max, &min)); + EXPECT_EQ(max, 1500u); + EXPECT_EQ(min, 1500u); +} + } // namespace