<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1"> <style type="text/css" style="display:none;"> P {margin-top:0;margin-bottom:0;} </style> </head> <body dir="ltr"> [AMD Official Use Only - Internal Distribution Only] <div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> </div> <div id="appendonsend"></div> <div style="font-family:Calibri,Arial,Helvetica,sans-serif; font-size:12pt; color:rgb(0,0,0)"> </div> <hr tabindex="-1" style="display:inline-block; width:98%"> <div id="divRplyFwdMsg" dir="ltr">From: Lazar, Lijo <Lijo.Lazar@amd.com> Sent: Thursday, May 13, 2021 7:53 PM To: Wang, Kevin(Yang) <Kevin1.Wang@amd.com>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org> Cc: Zhang, Hawking <Hawking.Zhang@amd.com>; Feng, Kenneth <Kenneth.Feng@amd.com> Subject: Re: [PATCH 2/3] drm/amd/pm: Fix showing incorrect frequencies on aldebaran <div> </div> </div> <div class="BodyFragment"> <div class="PlainText"> On 5/13/2021 5:06 PM, Wang, Kevin(Yang) wrote: > [AMD Official Use Only - Internal Distribution Only] > > > > > ------------------------------------------------------------------------ > *From:* Lazar, Lijo <Lijo.Lazar@amd.com> > *Sent:* Thursday, May 13, 2021 5:47 PM > *To:* amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org> > *Cc:* Zhang, Hawking <Hawking.Zhang@amd.com>; Feng, Kenneth > <Kenneth.Feng@amd.com>; Wang, Kevin(Yang) <Kevin1.Wang@amd.com> > *Subject:* [PATCH 2/3] drm/amd/pm: Fix showing incorrect frequencies on > aldebaran > > Use the current and custom pstate frequencies to track the current and > user-set min/max values in manual and determinism mode. Previously, only > actual_* value was used to track the currrent and user requested value. > The value will get reassigned whenever user requests a new value with > pp_od_clk_voltage node. Hence it will show incorrect values when user > requests an invalid value or tries a partial request without committing > the values. Separating out to custom and current variable fixes such > issues. > > Signed-off-by: Lijo Lazar <lijo.lazar@amd.com> > --- > .../drm/amd/pm/swsmu/smu13/aldebaran_ppt.c | 65 ++++++++++++------- > .../gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c | 18 ++++- > 2 files changed, 55 insertions(+), 28 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 5d04a1dfdfd8..d27ed2954705 100644 > --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c > @@ -78,8 +78,6 @@ > > #define smnPCIE_ESM_CTRL 0x111003D0 > > -#define CLOCK_VALID (1 << 31) > - > static const struct cmn2asic_msg_mapping > aldebaran_message_map[SMU_MSG_MAX_COUNT] = { > MSG_MAP(TestMessage, > PPSMC_MSG_TestMessage, 0), > MSG_MAP(GetSmuVersion, > PPSMC_MSG_GetSmuVersion, 1), > @@ -455,12 +453,18 @@ static int aldebaran_populate_umd_state_clk(struct > smu_context *smu) > > pstate_table->gfxclk_pstate.min = gfx_table->min; > pstate_table->gfxclk_pstate.peak = gfx_table->max; > + pstate_table->gfxclk_pstate.curr.min = gfx_table->min; > + pstate_table->gfxclk_pstate.curr.max = gfx_table->max; > > pstate_table->uclk_pstate.min = mem_table->min; > pstate_table->uclk_pstate.peak = mem_table->max; > + pstate_table->uclk_pstate.curr.min = mem_table->min; > + pstate_table->uclk_pstate.curr.max = mem_table->max; > > pstate_table->socclk_pstate.min = soc_table->min; > pstate_table->socclk_pstate.peak = soc_table->max; > + pstate_table->socclk_pstate.curr.min = soc_table->min; > + pstate_table->socclk_pstate.curr.max = soc_table->max; > > if (gfx_table->count > ALDEBARAN_UMD_PSTATE_GFXCLK_LEVEL && > mem_table->count > ALDEBARAN_UMD_PSTATE_MCLK_LEVEL && > @@ -669,6 +673,7 @@ static int aldebaran_print_clk_levels(struct > smu_context *smu, > { > int i, now, size = 0; > int ret = 0; > + struct smu_umd_pstate_table *pstate_table = &smu->pstate_table; > struct pp_clock_levels_with_latency clocks; > struct smu_13_0_dpm_table *single_dpm_table; > struct smu_dpm_context *smu_dpm = &smu->smu_dpm; > @@ -703,12 +708,8 @@ static int aldebaran_print_clk_levels(struct > smu_context *smu, > > display_levels = clocks.num_levels; > > - min_clk = smu->gfx_actual_hard_min_freq & CLOCK_VALID ? > - smu->gfx_actual_hard_min_freq & > ~CLOCK_VALID : > - single_dpm_table->dpm_levels[0].value; > - max_clk = smu->gfx_actual_soft_max_freq & CLOCK_VALID ? > - smu->gfx_actual_soft_max_freq & > ~CLOCK_VALID : > - single_dpm_table->dpm_levels[1].value; > + min_clk = pstate_table->gfxclk_pstate.curr.min; > + max_clk = pstate_table->gfxclk_pstate.curr.max; > > freq_values[0] = min_clk; > freq_values[1] = max_clk; > @@ -1134,9 +1135,6 @@ static int aldebaran_set_performance_level(struct > smu_context *smu, > && (level != > AMD_DPM_FORCED_LEVEL_PERF_DETERMINISM)) > smu_cmn_send_smc_msg(smu, SMU_MSG_DisableDeterminism, > NULL); > > - /* Reset user min/max gfx clock */ > - smu->gfx_actual_hard_min_freq = 0; > - smu->gfx_actual_soft_max_freq = 0; > > switch (level) { > > @@ -1163,6 +1161,7 @@ static int > aldebaran_set_soft_freq_limited_range(struct smu_context *smu, > { > struct smu_dpm_context *smu_dpm = &(smu->smu_dpm); > struct smu_13_0_dpm_context *dpm_context = smu_dpm->dpm_context; > + struct smu_umd_pstate_table *pstate_table = &smu->pstate_table; > struct amdgpu_device *adev = smu->adev; > uint32_t min_clk; > uint32_t max_clk; > @@ -1176,14 +1175,20 @@ static int > aldebaran_set_soft_freq_limited_range(struct smu_context *smu, > return -EINVAL; > > if (smu_dpm->dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL) { > - min_clk = max(min, dpm_context->dpm_tables.gfx_table.min); > - max_clk = min(max, dpm_context->dpm_tables.gfx_table.max); > - ret = smu_v13_0_set_soft_freq_limited_range(smu, SMU_GFXCLK, > - min_clk, > max_clk); > + if (min >= max) { > + dev_err(smu->adev->dev, > + "Minimum GFX clk should be less than the > maximum allowed clock\n"); > + return -EINVAL; > + } > > [kevin]: > why can these value not be equal in manual mode? We are intentionally avoiding user expectation to run at fixed clocks. The message is to make it clear that it is not a reasonable expectation on aldebaran. Thanks, Lijo</div> <div class="PlainText"> </div> <div class="PlainText">[kevin]:</div> <div class="PlainText">In fact, the SMU firmware allows driver to set the same clock,</div> <div class="PlainText">in other asic, we have the same code logic.</div> <div class="PlainText"> </div> <div class="PlainText">Regards,</div> <div class="PlainText">Kevin > + if ((min == pstate_table->gfxclk_pstate.curr.min) && > + (max == pstate_table->gfxclk_pstate.curr.max)) > + return 0; > + ret = smu_v13_0_set_soft_freq_limited_range(smu, SMU_GFXCLK, > + min, max); > if (!ret) { > - smu->gfx_actual_hard_min_freq = min_clk | > CLOCK_VALID; > - smu->gfx_actual_soft_max_freq = max_clk | > CLOCK_VALID; > + pstate_table->gfxclk_pstate.curr.min = min; > + pstate_table->gfxclk_pstate.curr.max = max; > } > return ret; > } > @@ -1209,10 +1214,8 @@ static int > aldebaran_set_soft_freq_limited_range(struct smu_context *smu, > dev_err(adev->dev, > "Failed to enable > determinism at GFX clock %d MHz\n", max); > } else { > - smu->gfx_actual_hard_min_freq = > - min_clk | CLOCK_VALID; > - smu->gfx_actual_soft_max_freq = > - max | CLOCK_VALID; > + pstate_table->gfxclk_pstate.curr.min = > min_clk; > + pstate_table->gfxclk_pstate.curr.max = max; > } > } > } > @@ -1225,6 +1228,7 @@ static int aldebaran_usr_edit_dpm_table(struct > smu_context *smu, enum PP_OD_DPM_ > { > struct smu_dpm_context *smu_dpm = &(smu->smu_dpm); > struct smu_13_0_dpm_context *dpm_context = smu_dpm->dpm_context; > + struct smu_umd_pstate_table *pstate_table = &smu->pstate_table; > uint32_t min_clk; > uint32_t max_clk; > int ret = 0; > @@ -1245,16 +1249,20 @@ static int aldebaran_usr_edit_dpm_table(struct > smu_context *smu, enum PP_OD_DPM_ > if (input[1] < > dpm_context->dpm_tables.gfx_table.min) { > dev_warn(smu->adev->dev, "Minimum GFX > clk (%ld) MHz specified is > less than the minimum allowed (%d) MHz\n", > input[1], > dpm_context->dpm_tables.gfx_table.min); > + pstate_table->gfxclk_pstate.custom.min = > + > pstate_table->gfxclk_pstate.curr.min; > return -EINVAL; > } > - smu->gfx_actual_hard_min_freq = input[1]; > + pstate_table->gfxclk_pstate.custom.min = input[1]; > } else if (input[0] == 1) { > if (input[1] > > dpm_context->dpm_tables.gfx_table.max) { > dev_warn(smu->adev->dev, "Maximum GFX > clk (%ld) MHz specified is > greater than the maximum allowed (%d) MHz\n", > input[1], > dpm_context->dpm_tables.gfx_table.max); > + pstate_table->gfxclk_pstate.custom.max = > + > pstate_table->gfxclk_pstate.curr.max; > return -EINVAL; > } > - smu->gfx_actual_soft_max_freq = input[1]; > + pstate_table->gfxclk_pstate.custom.max = input[1]; > } else { > return -EINVAL; > } > @@ -1276,8 +1284,15 @@ static int aldebaran_usr_edit_dpm_table(struct > smu_context *smu, enum PP_OD_DPM_ > dev_err(smu->adev->dev, "Input parameter > number not correct\n"); > return -EINVAL; > } else { > - min_clk = smu->gfx_actual_hard_min_freq; > - max_clk = smu->gfx_actual_soft_max_freq; > + if (!pstate_table->gfxclk_pstate.custom.min) > + pstate_table->gfxclk_pstate.custom.min = > + > pstate_table->gfxclk_pstate.curr.min; > + if (!pstate_table->gfxclk_pstate.custom.max) > + pstate_table->gfxclk_pstate.custom.max = > + > pstate_table->gfxclk_pstate.curr.max; > + min_clk = pstate_table->gfxclk_pstate.custom.min; > + max_clk = pstate_table->gfxclk_pstate.custom.max; > + > return > aldebaran_set_soft_freq_limited_range(smu, SMU_GFXCLK, > min_clk, max_clk); > } > break; > diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c > b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c > index 0864083e7435..755bddaf6c4b 100644 > --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c > @@ -1624,8 +1624,12 @@ int smu_v13_0_set_performance_level(struct > smu_context *smu, > SMU_GFXCLK, > sclk_min, > sclk_max); > - if (ret) > + if (ret) { > return ret; > + } else { > + pstate_table->gfxclk_pstate.curr.min = sclk_min; > + pstate_table->gfxclk_pstate.curr.max = sclk_max; > + } > } > > if (mclk_min && mclk_max) { > @@ -1633,8 +1637,12 @@ int smu_v13_0_set_performance_level(struct > smu_context *smu, > SMU_MCLK, > mclk_min, > mclk_max); > - if (ret) > + if (ret) { > return ret; > + } else { > + pstate_table->uclk_pstate.curr.min = mclk_min; > + pstate_table->uclk_pstate.curr.max = mclk_max; > + } > } > > if (socclk_min && socclk_max) { > @@ -1642,8 +1650,12 @@ int smu_v13_0_set_performance_level(struct > smu_context *smu, > SMU_SOCCLK, > socclk_min, > socclk_max); > - if (ret) > + if (ret) { > return ret; > + } else { > + pstate_table->socclk_pstate.curr.min = socclk_min; > + pstate_table->socclk_pstate.curr.max = socclk_max; > + } > } > > return ret; > -- > 2.17.1 > </div> </div> </div> </body> </html>