[AMD Official Use Only - General] Series is acked-by: Evan Quan <[email protected]>
> -----Original Message----- > From: Powell, Darren <[email protected]> > Sent: Friday, May 13, 2022 11:15 AM > To: [email protected] > Cc: Quan, Evan <[email protected]>; Wang, Yang(Kevin) > <[email protected]>; [email protected]; Lazar, Lijo > <[email protected]>; Feng, Kenneth <[email protected]>; Yu, > Lang <[email protected]>; Powell, Darren <[email protected]> > Subject: [PATCH v1 4/4] amdgpu/pm: Optimize aldebaran_emit_clk_levels > > aldebaran_get_clk_table cannot fail so convert to void function > aldebaran_freqs_in_same_level now returns bool > aldebaran_emit_clk_levels optimized: > split into two switch statements to gather vars, then use common output > removed impossible error messages for failure of get_clk_table > reduce size of string literals by creating static string var > changed unsafe loop iterator from single_dpm_table->count to > clocks.num_levels > in case MAX_DPM_LEVELS > PP_MAX_CLOCK_LEVELS in future code > added clock_mhz to remove double divide by 1000 > collapse duplicate case statements in second switch statement > simplified code to output detect frequency level match to current level > > == Test == > LOGFILE=aldebaran.test.log > AMDGPU_PCI_ADDR=`lspci -nn | grep "VGA\|Display" | cut -d " " -f 1` > AMDGPU_HWMON=`ls -la /sys/class/hwmon | grep $AMDGPU_PCI_ADDR | > awk '{print $9}'` > HWMON_DIR=/sys/class/hwmon/${AMDGPU_HWMON} > > lspci -nn | grep "VGA\|Display" > $LOGFILE > FILES="pp_dpm_sclk > pp_dpm_mclk > pp_dpm_pcie > pp_dpm_socclk > pp_dpm_fclk > pp_dpm_vclk > pp_dpm_dclk " > > for f in $FILES > do > echo === $f === >> $LOGFILE > cat $HWMON_DIR/device/$f >> $LOGFILE > done > cat $LOGFILE > > Signed-off-by: Darren Powell <[email protected]> > --- > .../drm/amd/pm/swsmu/smu13/aldebaran_ppt.c | 173 +++++++----------- > 1 file changed, 62 insertions(+), 111 deletions(-) > > diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c > b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c > index e593878bc173..23a87bfb4429 100644 > --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c > @@ -545,9 +545,9 @@ static int aldebaran_populate_umd_state_clk(struct > smu_context *smu) > return 0; > } > > -static int aldebaran_get_clk_table(struct smu_context *smu, > - struct pp_clock_levels_with_latency *clocks, > - struct smu_13_0_dpm_table *dpm_table) > +static void aldebaran_get_clk_table(struct smu_context *smu, > + struct pp_clock_levels_with_latency > *clocks, > + struct smu_13_0_dpm_table *dpm_table) > { > int i, count; > > @@ -560,11 +560,11 @@ static int aldebaran_get_clk_table(struct > smu_context *smu, > clocks->data[i].latency_in_us = 0; > } > > - return 0; > + return; > } > > -static int aldebaran_freqs_in_same_level(int32_t frequency1, > - int32_t frequency2) > +static bool aldebaran_freqs_in_same_level(int32_t frequency1, > + int32_t frequency2) > { > return (abs(frequency1 - frequency2) <= EPSILON); > } > @@ -738,9 +738,12 @@ static int aldebaran_emit_clk_levels(struct > smu_context *smu, > struct smu_13_0_dpm_table *single_dpm_table; > struct smu_dpm_context *smu_dpm = &smu->smu_dpm; > struct smu_13_0_dpm_context *dpm_context = NULL; > - uint32_t i, display_levels; > + uint32_t i, cur_value = 0; > uint32_t freq_values[3] = {0}; > - uint32_t min_clk, max_clk, cur_value = 0; > + uint32_t min_clk, max_clk, display_levels; > + bool freq_match; > + unsigned int clock_mhz; > + static const char attempt_string[] = "Attempt to get current"; > > if (amdgpu_ras_intr_triggered()) { > *offset += sysfs_emit_at(buf, *offset, "unavailable\n"); > @@ -750,23 +753,18 @@ static int aldebaran_emit_clk_levels(struct > smu_context *smu, > dpm_context = smu_dpm->dpm_context; > > switch (type) { > - > case SMU_OD_SCLK: > *offset += sysfs_emit_at(buf, *offset, "%s:\n", "GFXCLK"); > fallthrough; > case SMU_SCLK: > ret = aldebaran_get_current_clk_freq_by_table(smu, > SMU_GFXCLK, &cur_value); > if (ret) { > - dev_err(smu->adev->dev, "Attempt to get current > gfx clk Failed!"); > + dev_err(smu->adev->dev, "%s gfx clk Failed!", > attempt_string); > return ret; > } > > single_dpm_table = &(dpm_context- > >dpm_tables.gfx_table); > - ret = aldebaran_get_clk_table(smu, &clocks, > single_dpm_table); > - if (ret) { > - dev_err(smu->adev->dev, "Attempt to get gfx clk > levels Failed!"); > - return ret; > - } > + aldebaran_get_clk_table(smu, &clocks, single_dpm_table); > > display_levels = clocks.num_levels; > > @@ -782,152 +780,105 @@ static int aldebaran_emit_clk_levels(struct > smu_context *smu, > freq_values[2] = max_clk; > freq_values[1] = cur_value; > } > - > - /* > - * For DPM disabled case, there will be only one clock level. > - * And it's safe to assume that is always the current clock. > - */ > - if (display_levels == clocks.num_levels) { > - for (i = 0; i < clocks.num_levels; i++) > - *offset += sysfs_emit_at(buf, *offset, > "%d: %uMhz %s\n", i, > - freq_values[i], > - (clocks.num_levels == 1) ? > - "*" : > - > (aldebaran_freqs_in_same_level( > - freq_values[i], > cur_value) ? > - "*" : > - "")); > - } else { > - for (i = 0; i < display_levels; i++) > - *offset += sysfs_emit_at(buf, *offset, > "%d: %uMhz %s\n", i, > - freq_values[i], i == 1 ? "*" : > ""); > - } > - > break; > - > case SMU_OD_MCLK: > *offset += sysfs_emit_at(buf, *offset, "%s:\n", "MCLK"); > fallthrough; > case SMU_MCLK: > ret = aldebaran_get_current_clk_freq_by_table(smu, > SMU_UCLK, &cur_value); > if (ret) { > - dev_err(smu->adev->dev, "Attempt to get current > mclk Failed!"); > + dev_err(smu->adev->dev, "%s mclk Failed!", > attempt_string); > return ret; > } > > single_dpm_table = &(dpm_context- > >dpm_tables.uclk_table); > - ret = aldebaran_get_clk_table(smu, &clocks, > single_dpm_table); > - if (ret) { > - dev_err(smu->adev->dev, "Attempt to get memory > clk levels Failed!"); > - return ret; > - } > - > - for (i = 0; i < clocks.num_levels; i++) > - *offset += sysfs_emit_at(buf, *offset, > "%d: %uMhz %s\n", > - i, clocks.data[i].clocks_in_khz / 1000, > - (clocks.num_levels == 1) ? "*" : > - (aldebaran_freqs_in_same_level( > - > clocks.data[i].clocks_in_khz / 1000, > - > cur_value) ? "*" : "")); > + aldebaran_get_clk_table(smu, &clocks, single_dpm_table); > break; > - > case SMU_SOCCLK: > ret = aldebaran_get_current_clk_freq_by_table(smu, > SMU_SOCCLK, &cur_value); > if (ret) { > - dev_err(smu->adev->dev, "Attempt to get current > socclk Failed!"); > + dev_err(smu->adev->dev, "%s socclk Failed!", > attempt_string); > return ret; > } > > single_dpm_table = &(dpm_context- > >dpm_tables.soc_table); > - ret = aldebaran_get_clk_table(smu, &clocks, > single_dpm_table); > - if (ret) { > - dev_err(smu->adev->dev, "Attempt to get socclk > levels Failed!"); > - return ret; > - } > - > - for (i = 0; i < clocks.num_levels; i++) > - *offset += sysfs_emit_at(buf, *offset, > "%d: %uMhz %s\n", > - i, clocks.data[i].clocks_in_khz / 1000, > - (clocks.num_levels == 1) ? "*" : > - (aldebaran_freqs_in_same_level( > - > clocks.data[i].clocks_in_khz / 1000, > - > cur_value) ? "*" : "")); > + aldebaran_get_clk_table(smu, &clocks, single_dpm_table); > break; > - > case SMU_FCLK: > ret = aldebaran_get_current_clk_freq_by_table(smu, > SMU_FCLK, &cur_value); > if (ret) { > - dev_err(smu->adev->dev, "Attempt to get current > fclk Failed!"); > + dev_err(smu->adev->dev, "%s fclk Failed!", > attempt_string); > return ret; > } > > single_dpm_table = &(dpm_context- > >dpm_tables.fclk_table); > - ret = aldebaran_get_clk_table(smu, &clocks, > single_dpm_table); > - if (ret) { > - dev_err(smu->adev->dev, "Attempt to get fclk levels > Failed!"); > - return ret; > - } > - > - for (i = 0; i < single_dpm_table->count; i++) > - *offset += sysfs_emit_at(buf, *offset, > "%d: %uMhz %s\n", > - i, single_dpm_table- > >dpm_levels[i].value, > - (clocks.num_levels == 1) ? "*" : > - (aldebaran_freqs_in_same_level( > - > clocks.data[i].clocks_in_khz / 1000, > - > cur_value) ? "*" : "")); > + aldebaran_get_clk_table(smu, &clocks, single_dpm_table); > break; > - > case SMU_VCLK: > ret = aldebaran_get_current_clk_freq_by_table(smu, > SMU_VCLK, &cur_value); > if (ret) { > - dev_err(smu->adev->dev, "Attempt to get current > vclk Failed!"); > + dev_err(smu->adev->dev, "%s vclk Failed!", > attempt_string); > return ret; > } > > single_dpm_table = &(dpm_context- > >dpm_tables.vclk_table); > - ret = aldebaran_get_clk_table(smu, &clocks, > single_dpm_table); > - if (ret) { > - dev_err(smu->adev->dev, "Attempt to get vclk > levels Failed!"); > - return ret; > - } > - > - for (i = 0; i < single_dpm_table->count; i++) > - *offset += sysfs_emit_at(buf, *offset, > "%d: %uMhz %s\n", > - i, single_dpm_table- > >dpm_levels[i].value, > - (clocks.num_levels == 1) ? "*" : > - (aldebaran_freqs_in_same_level( > - > clocks.data[i].clocks_in_khz / 1000, > - > cur_value) ? "*" : "")); > + aldebaran_get_clk_table(smu, &clocks, single_dpm_table); > break; > - > case SMU_DCLK: > ret = aldebaran_get_current_clk_freq_by_table(smu, > SMU_DCLK, &cur_value); > if (ret) { > - dev_err(smu->adev->dev, "Attempt to get current > dclk Failed!"); > + dev_err(smu->adev->dev, "%s dclk Failed!", > attempt_string); > return ret; > } > > single_dpm_table = &(dpm_context- > >dpm_tables.dclk_table); > - ret = aldebaran_get_clk_table(smu, &clocks, > single_dpm_table); > - if (ret) { > - dev_err(smu->adev->dev, "Attempt to get dclk > levels Failed!"); > - return ret; > + aldebaran_get_clk_table(smu, &clocks, single_dpm_table); > + break; > + default: > + return -EINVAL; > + } > + > + switch (type) { > + case SMU_OD_SCLK: > + case SMU_SCLK: > + /* > + * For DPM disabled case, there will be only one clock level. > + * And it's safe to assume that is always the current clock. > + */ > + if (display_levels == clocks.num_levels) { > + for (i = 0; i < clocks.num_levels; i++) { > + clock_mhz = freq_values[i]; > + freq_match = > aldebaran_freqs_in_same_level(clock_mhz, cur_value); > + freq_match |= (clocks.num_levels == 1); > + *offset += sysfs_emit_at(buf, *offset, > "%d: %uMhz %s\n", i, > + clock_mhz, > + freq_match ? "*" : > ""); > + } > + } else { > + for (i = 0; i < display_levels; i++) > + *offset += sysfs_emit_at(buf, *offset, > "%d: %uMhz %s\n", i, > + freq_values[i], i == 1 > ? > "*" : ""); > } > > - for (i = 0; i < single_dpm_table->count; i++) > + break; > + case SMU_OD_MCLK: > + case SMU_MCLK: > + case SMU_SOCCLK: > + case SMU_FCLK: > + case SMU_VCLK: > + case SMU_DCLK: > + for (i = 0; i < clocks.num_levels; i++) { > + clock_mhz = clocks.data[i].clocks_in_khz / 1000; > + freq_match = > aldebaran_freqs_in_same_level(clock_mhz, cur_value); > + freq_match |= (clocks.num_levels == 1); > *offset += sysfs_emit_at(buf, *offset, > "%d: %uMhz %s\n", > - i, single_dpm_table- > >dpm_levels[i].value, > - (clocks.num_levels == 1) ? "*" : > - (aldebaran_freqs_in_same_level( > - > clocks.data[i].clocks_in_khz / 1000, > - > cur_value) ? "*" : "")); > + i, clock_mhz, > + freq_match ? "*" : ""); > + } > break; > - > default: > return -EINVAL; > - break; > } > - > return 0; > } > > -- > 2.35.1
