Re: [PATCH] drm/amd/pm: refactor DPM clock level reporting

[Lazar, Lijo]

On 09-Jun-26 3:49 PM, Wang, Yang(Kevin) wrote:
AMD General

-----Original Message-----
From: Lazar, Lijo <lijo.la...@amd.com>
Sent: Tuesday, June 9, 2026 4:11 PM
To: Wang, Yang(Kevin) <kevinyang.w...@amd.com>; amd-
g...@lists.freedesktop.org
Cc: Deucher, Alexander <alexander.deuc...@amd.com>; Zhang, Hawking
<hawking.zh...@amd.com>; Feng, Kenneth <kenneth.f...@amd.com>
Subject: Re: [PATCH] drm/amd/pm: refactor DPM clock level reporting



On 09-Jun-26 12:21 PM, Yang Wang wrote:
Refactor smu_cmn_print_dpm_clk_levels() to build clock entries before
emitting sysfs output.

For discrete DPM tables, mark the level closest to the reported
current clock. This avoids losing the active '*' marker when the
SMU-reported clock does not fall within the previous fixed tolerance.

Keep fine-grained output explicit by reporting the current clock on an
'F' line, and keep deep sleep represented by the 'S' line without
marking a discrete level.

Active marker placement:

| Mode         | '*' marker location       | Reason                    |
| ------------ | ------------------------- | ------------------------- |
| discrete     | closest/current DPM level | entries are real levels   |
| fine-grained | 'F:' current clock line   | min/max are range bounds  |
| deep sleep   | 'S:' line                 | outside normal DPM range  |

Closes: https://gitlab.freedesktop.org/drm/amd/-/work_items/5295
Signed-off-by: Yang Wang <kevinyang.w...@amd.com>
---
   drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c | 148 +++++++++++++++++--
------
   1 file changed, 101 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
index d365f06ac1ac..872c0328f290 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
@@ -1376,77 +1376,131 @@ void smu_cmn_reset_custom_level(struct
smu_context *smu)
     pstate_table->uclk_pstate.custom.max = 0;
   }

-static inline bool smu_cmn_freqs_match(uint32_t freq1, uint32_t
freq2)
+struct smu_clk_print_entry {
+   uint32_t freq;
+   bool selected;
+};
+
+static inline uint32_t smu_cmn_freq_distance(uint32_t freq1, uint32_t
+freq2) {
+   return freq1 > freq2 ? freq1 - freq2 : freq2 - freq1; }
+
+static inline uint32_t smu_cmn_get_dpm_level_count(struct
+smu_dpm_table *dpm_table) {
+   return min_t(uint32_t, dpm_table->count,
SMU_MAX_DPM_LEVELS); }
+
+static uint32_t smu_cmn_get_closest_clk_level(struct smu_dpm_table
+*dpm_table, uint32_t cur_clk) {
+   uint32_t min_distance, distance;
+   uint32_t closest_level = 0;
+   uint32_t count;
+   uint32_t i;
+
+   count = smu_cmn_get_dpm_level_count(dpm_table);
+   if (!count)
+           return SMU_MAX_DPM_LEVELS;
+
+   min_distance = smu_cmn_freq_distance(cur_clk, dpm_table-
dpm_levels[0].value);
+   for (i = 1; i < count; i++) {
+           distance = smu_cmn_freq_distance(cur_clk, dpm_table-
dpm_levels[i].value);
+           if (distance < min_distance) {
+                   min_distance = distance;
+                   closest_level = i;
+           }
+   }
+
+   return closest_level;
+}
+
+static inline int smu_cmn_emit_clk_line(char *buf, int size,
+                                   int level_index, uint32_t freq, bool
selected) {
+   return sysfs_emit_at(buf, size, "%d: %uMhz %s\n",
+                        level_index, freq, selected ? "*" : ""); }
+
+static void smu_cmn_build_fine_grained_levels(uint32_t min_clk, uint32_t
max_clk,
+                                         struct smu_clk_print_entry
*entries,
+                                         uint32_t *entry_count)
+{
+   *entry_count = 2;
+   entries[0].freq = min_clk;
+   entries[0].selected = false;
+   entries[1].freq = max_clk;
+   entries[1].selected = false;
+}
+
+static void smu_cmn_build_discrete_levels(struct smu_dpm_table
*dpm_table,
+                                     uint32_t selected_level,
+                                     struct smu_clk_print_entry *entries,
+                                     uint32_t *entry_count)
+{
+   uint32_t i;
+
+   *entry_count = smu_cmn_get_dpm_level_count(dpm_table);
+
+   for (i = 0; i < *entry_count; i++) {
+           entries[i].freq = dpm_table->dpm_levels[i].value;
+           entries[i].selected = (i == selected_level);
+   }
+}
+
+static int smu_cmn_emit_clk_prefix(char *buf, int size,
+                              bool is_fine_grained, bool is_deep_sleep,
+                              uint32_t cur_clk)
   {
-   /* Frequencies within 25 MHz are considered equal */
-   return (abs((int)freq1 - (int)freq2) <= 25);
+   if (is_deep_sleep)
+           size += sysfs_emit_at(buf, size, "S: %uMhz *\n", cur_clk);
+   else if (is_fine_grained)
+           size += sysfs_emit_at(buf, size, "F: %uMhz *\n", cur_clk);

What about keeping the else part as C: <cur_clk> in all cases - instead of just
fine grained? * indicates the closest level matched and cur_clk will give the
exact frequency.

Thanks,
Lijo

This is a good idea. However, for now I'd like to retain the existing logic to 
stay compatible with current parsing tools and prevent potential regressions.
Also, note that "F" and "S" are optional labels, which are only shown for 
unmatched DPM LEVEL entries.


+Bill/Maisam

What about the case when current clock matches min/max in fine grained 
scenario? In the new logic, F is the label for current clock always for 
fine grained clocks which is a deviation from the old logic. Is that to 
always show 3 levels in fine grained?

Thanks,
Lijo

Best Regards,
Kevin

+
+   return size;
   }

   int smu_cmn_print_dpm_clk_levels(struct smu_context *smu,
                              struct smu_dpm_table *dpm_table,
                              uint32_t cur_clk, char *buf, int *offset)
   {
-   uint32_t min_clk, max_clk, level_index, count;
-   uint32_t freq_values[3];
-   int size, lvl, i;
+   struct smu_clk_print_entry entries[SMU_MAX_DPM_LEVELS];
+   uint32_t min_clk, max_clk, count, entry_count = 0;
+   uint32_t selected_level = SMU_MAX_DPM_LEVELS;
+   int size, i;
     bool is_fine_grained;
     bool is_deep_sleep;
-   bool freq_match;

     if (!dpm_table || !buf)
             return -EINVAL;

-   level_index = 0;
     size = *offset;
-   count = dpm_table->count;
     is_fine_grained = dpm_table->flags &
SMU_DPM_TABLE_FINE_GRAINED;
-   min_clk = SMU_DPM_TABLE_MIN(dpm_table);
-   max_clk = SMU_DPM_TABLE_MAX(dpm_table);
+   count = smu_cmn_get_dpm_level_count(dpm_table);
+   min_clk = count ? dpm_table->dpm_levels[0].value : 0;
+   max_clk = count ? dpm_table->dpm_levels[count - 1].value : 0;

     /* Deep sleep - current clock < min_clock/2, TBD: cur_clk = 0 as
GFXOFF */
     is_deep_sleep = cur_clk < min_clk / 2;
-   if (is_deep_sleep) {
-           size += sysfs_emit_at(buf, size, "S: %uMhz *\n", cur_clk);
-           level_index = 1;
-   }

     if (!is_fine_grained || count == 1) {
-           for (i = 0; i < count; i++) {
-                   freq_match = !is_deep_sleep &&
-                                smu_cmn_freqs_match(
-                                        cur_clk,
-                                        dpm_table->dpm_levels[i].value);
-                   size += sysfs_emit_at(buf, size, "%d: %uMhz %s\n",
-                                         level_index + i,
-                                         dpm_table->dpm_levels[i].value,
-                                         freq_match ? "*" : "");
+           if (!is_deep_sleep) {
+                   selected_level =
+                           smu_cmn_get_closest_clk_level(dpm_table,
cur_clk);
             }
+           smu_cmn_build_discrete_levels(dpm_table, selected_level,
+                                                 entries, &entry_count);
     } else {
-           count = 2;
-           freq_values[0] = min_clk;
-           freq_values[1] = max_clk;
+           smu_cmn_build_fine_grained_levels(min_clk, max_clk,
+                                             entries, &entry_count);
+   }

-           if (!is_deep_sleep) {
-                   if (smu_cmn_freqs_match(cur_clk, min_clk)) {
-                           lvl = 0;
-                   } else if (smu_cmn_freqs_match(cur_clk, max_clk)) {
-                           lvl = 1;
-                   } else {
-                           /* NOTE: use index '1' to show current clock
value */
-                           lvl = 1;
-                           count = 3;
-                           freq_values[1] = cur_clk;
-                           freq_values[2] = max_clk;
-                   }
-           }
+   size = smu_cmn_emit_clk_prefix(buf, size, is_fine_grained,
+                                  is_deep_sleep, cur_clk);

-           for (i = 0; i < count; i++) {
-                   size += sysfs_emit_at(
-                           buf, size, "%d: %uMhz %s\n", level_index + i,
-                           freq_values[i],
-                           (!is_deep_sleep && i == lvl) ? "*" : "");
-           }
-   }
+   for (i = 0; i < entry_count; i++)
+           size += smu_cmn_emit_clk_line(buf, size, i,
+                                        entries[i].freq,
+                                        entries[i].selected);

     *offset = size;