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.
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;
> >>>
> >
