AMD General > -----Original Message----- > From: Alex Deucher <[email protected]> > Sent: Monday, June 15, 2026 11:26 PM > To: Lazar, Lijo <[email protected]> > Cc: Wang, Yang(Kevin) <[email protected]>; amd- > [email protected]; Deucher, Alexander > <[email protected]>; Zhang, Hawking > <[email protected]>; Feng, Kenneth <[email protected]>; > Liu, Shuzhou (Bill) <[email protected]>; Arif, Maisam > <[email protected]> > Subject: Re: [PATCH] drm/amd/pm: refactor DPM clock level reporting > > On Tue, Jun 9, 2026 at 8:20 AM Lazar, Lijo <[email protected]> wrote: > > > > > > > > On 09-Jun-26 5:35 PM, Wang, Yang(Kevin) wrote: > > > AMD General > > > > > >> -----Original Message----- > > >> From: Lazar, Lijo <[email protected]> > > >> Sent: Tuesday, June 9, 2026 7:12 PM > > >> To: Wang, Yang(Kevin) <[email protected]>; amd- > > >> [email protected] > > >> Cc: Deucher, Alexander <[email protected]>; Zhang, > Hawking > > >> <[email protected]>; Feng, Kenneth > <[email protected]>; Liu, > > >> Shuzhou (Bill) <[email protected]>; Arif, Maisam > > >> <[email protected]> > > >> Subject: Re: [PATCH] drm/amd/pm: refactor DPM clock level reporting > > >> > > >> > > >> > > >> On 09-Jun-26 3:49 PM, Wang, Yang(Kevin) wrote: > > >>> AMD General > > >>> > > >>>> -----Original Message----- > > >>>> From: Lazar, Lijo <[email protected]> > > >>>> Sent: Tuesday, June 9, 2026 4:11 PM > > >>>> To: Wang, Yang(Kevin) <[email protected]>; amd- > > >>>> [email protected] > > >>>> Cc: Deucher, Alexander <[email protected]>; Zhang, > > >>>> Hawking <[email protected]>; Feng, Kenneth > > >>>> <[email protected]> > > >>>> 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 <[email protected]> > > >>>>> --- > > >>>>> 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 > > > > > >>> Is that to always show 3 levels in fine grained? > > > yes, this part indeed works differently from the prior logic. > > > > > > The major problem with the current fine-grained implementation is > > > that when the current clock does not hit min/max, the current clock is > shown current clock at position "1" (total 3 lines output) This tends to > confuse > users, who may wonder whether to use position 1 or 2 to set the maximum > value, but 1 is expected value by driver. > > > Therefore, this is one of the issues addressed by this patch. > > > > > > Back to your question: Either adopting a fixed 3-level display or > > > retaining the old logic is reasonable ( which one is your prefer ?) The > > > label > "F" stands for both fine-grained and frequency. > > > > > > > I prefer the new approach to separate out current clock from the levels. > > > > For ex: user space sees F label, uses the value as current clock. Rest > > of them used for level information. If it doesn't see F, try to parse > > the legacy way. > > > > Alex, do you have any comments? > > I was never crazy about having the value in the middle be outside of the > min/max, but IIRC, it was done that way for compatibility. > Whatever is the least disruptive for existing tools. > > Alex
Hi Alex, What do you think of this approach? We add a new label 'F' to display the current frequency value when fine-grain mode is enabled. This patch fixes the below issue related to the new DPM level selection logic. (always print '*' to avoid this kind of issue) https://gitlab.freedesktop.org/drm/amd/-/work_items/5295 https://gitlab.freedesktop.org/drm/amd/-/work_items/5371 Best Regards, Kevin > > > > > As a minimal representation of user space - > > Bill/Arif, is it possible to have this changed in amd-smi? > > > > Thanks, > > Lijo > > > > > Best Regards, > > > Kevin > > >> > > >>> 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; > > >>>>> > > >>> > > > > >
