On Thu, Jun 18, 2026 at 9:37 AM Wang, Yang(Kevin)
<[email protected]> wrote:
>
> 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
Seems reasonable to me.
Thanks!
Alex
>
> 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;
> > > >>>>>
> > > >>>
> > > >
> > >
