[AMD Official Use Only - General]
Series is acked-by: Evan Quan <evan.quan@xxxxxxx>
> -----Original Message-----
> From: Powell, Darren <Darren.Powell@xxxxxxx>
> Sent: Friday, May 13, 2022 11:15 AM
> To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Quan, Evan <Evan.Quan@xxxxxxx>; Wang, Yang(Kevin)
> <KevinYang.Wang@xxxxxxx>; david.nieto@xxxxxxx; Lazar, Lijo
> <Lijo.Lazar@xxxxxxx>; Feng, Kenneth <Kenneth.Feng@xxxxxxx>; Yu,
> Lang <Lang.Yu@xxxxxxx>; Powell, Darren <Darren.Powell@xxxxxxx>
> 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 <darren.powell@xxxxxxx>
> ---
> .../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
