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