Patch is missing your signed-off-by. Please address Evan's comments
and the signed off by and I'll apply it.
Thanks!
Alex
On Sun, Nov 10, 2019 at 11:03 PM Quan, Evan <Evan.Quan@xxxxxxx> wrote:
>
> If smu_get_pptable_power_limit() is designed to be used internally, the second argument "lock_needed" can be dropped.
> Except that, the patch is reviewed-by: Evan Quan <evan.quan@xxxxxxx>
>
> > -----Original Message-----
> > From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Matt
> > Coffin
> > Sent: Saturday, November 9, 2019 7:54 AM
> > To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> > Cc: Matt Coffin <mcoffin13@xxxxxxxxx>
> > Subject: [PATCH] drm/amdgpu/smu_v11: Unify and fix power limits
> >
> > [Why]
> > On Navi10, and presumably arcterus, updating pp_table via sysfs would
> > not re-scale the maximum possible power limit one can set. On navi10,
> > the SMU code ignored the power percentage overdrive setting entirely,
> > and would not allow you to exceed the default power limit at all.
> >
> > [How]
> > Adding a function to the SMU interface to get the pptable version of the
> > default power limit allows ASIC-specific code to provide the correct
> > maximum-settable power limit for the current pptable.
> > ---
> > drivers/gpu/drm/amd/powerplay/amdgpu_smu.c | 18 ++++++++-
> > drivers/gpu/drm/amd/powerplay/arcturus_ppt.c | 22 +++++-----
> > .../gpu/drm/amd/powerplay/inc/amdgpu_smu.h | 4 +-
> > drivers/gpu/drm/amd/powerplay/inc/smu_v11_0.h | 2 +
> > .../drm/amd/powerplay/inc/smu_v11_0_pptable.h | 2 +
> > drivers/gpu/drm/amd/powerplay/navi10_ppt.c | 22 +++++-----
> > drivers/gpu/drm/amd/powerplay/smu_v11_0.c | 40 +++++++++++++++++--
> > drivers/gpu/drm/amd/powerplay/vega20_ppt.c | 1 -
> > 8 files changed, 83 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> > b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> > index 66faea66a8e9..6bf940f1edfb 100644
> > --- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> > +++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> > @@ -1107,7 +1107,7 @@ static int smu_smc_table_hw_init(struct
> > smu_context *smu,
> > if (ret)
> > return ret;
> >
> > - ret = smu_get_power_limit(smu, &smu->default_power_limit,
> > true, false);
> > + ret = smu_get_power_limit(smu, &smu->default_power_limit,
> > false, false);
> > if (ret)
> > return ret;
> > }
> > @@ -2509,3 +2509,19 @@ int smu_get_dpm_clock_table(struct smu_context
> > *smu,
> >
> > return ret;
> > }
> > +
> > +uint32_t smu_get_pptable_power_limit(struct smu_context *smu, bool
> > lock_needed)
> > +{
> > + uint32_t ret = 0;
> > +
> > + if (lock_needed)
> > + mutex_lock(&smu->mutex);
> > +
> > + if (smu->ppt_funcs->get_pptable_power_limit)
> > + ret = smu->ppt_funcs->get_pptable_power_limit(smu);
> > +
> > + if (lock_needed)
> > + mutex_unlock(&smu->mutex);
> > +
> > + return ret;
> > +}
> > diff --git a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> > b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> > index 3099ac256bd3..ebb9c8064867 100644
> > --- a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> > +++ b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> > @@ -1261,15 +1261,14 @@ arcturus_get_profiling_clk_mask(struct
> > smu_context *smu,
> >
> > static int arcturus_get_power_limit(struct smu_context *smu,
> > uint32_t *limit,
> > - bool asic_default)
> > + bool cap)
> > {
> > PPTable_t *pptable = smu->smu_table.driver_pptable;
> > uint32_t asic_default_power_limit = 0;
> > int ret = 0;
> > int power_src;
> >
> > - if (!smu->default_power_limit ||
> > - !smu->power_limit) {
> > + if (!smu->power_limit) {
> > if (smu_feature_is_enabled(smu, SMU_FEATURE_PPT_BIT)) {
> > power_src = smu_power_get_index(smu,
> > SMU_POWER_SOURCE_AC);
> > if (power_src < 0)
> > @@ -1292,17 +1291,11 @@ static int arcturus_get_power_limit(struct
> > smu_context *smu,
> > pptable-
> > >SocketPowerLimitAc[PPT_THROTTLER_PPT0];
> > }
> >
> > - if (smu->od_enabled) {
> > - asic_default_power_limit *= (100 + smu-
> > >smu_table.TDPODLimit);
> > - asic_default_power_limit /= 100;
> > - }
> > -
> > - smu->default_power_limit = asic_default_power_limit;
> > smu->power_limit = asic_default_power_limit;
> > }
> >
> > - if (asic_default)
> > - *limit = smu->default_power_limit;
> > + if (cap)
> > + *limit = smu_v11_0_get_max_power_limit(smu);
> > else
> > *limit = smu->power_limit;
> >
> > @@ -2070,6 +2063,12 @@ static void arcturus_i2c_eeprom_control_fini(struct
> > i2c_adapter *control)
> > i2c_del_adapter(control);
> > }
> >
> > +static uint32_t arcterus_get_pptable_power_limit(struct smu_context *smu)
> > +{
> > + PPTable_t *pptable = smu->smu_table.driver_pptable;
> > + return pptable->SocketPowerLimitAc[PPT_THROTTLER_PPT0];
> > +}
> > +
> > static const struct pptable_funcs arcturus_ppt_funcs = {
> > /* translate smu index into arcturus specific index */
> > .get_smu_msg_index = arcturus_get_smu_msg_index,
> > @@ -2160,6 +2159,7 @@ static const struct pptable_funcs arcturus_ppt_funcs
> > = {
> > .get_dpm_ultimate_freq = smu_v11_0_get_dpm_ultimate_freq,
> > .set_soft_freq_limited_range =
> > smu_v11_0_set_soft_freq_limited_range,
> > .override_pcie_parameters = smu_v11_0_override_pcie_parameters,
> > + .get_pptable_power_limit = arcterus_get_pptable_power_limit,
> > };
> >
> > void arcturus_set_ppt_funcs(struct smu_context *smu)
> > diff --git a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
> > b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
> > index 8120e7587585..9d0193569b05 100644
> > --- a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
> > +++ b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
> > @@ -261,7 +261,6 @@ struct smu_table_context
> > struct smu_table *tables;
> > struct smu_table memory_pool;
> > uint8_t thermal_controller_type;
> > - uint16_t TDPODLimit;
> >
> > void *overdrive_table;
> > };
> > @@ -548,6 +547,7 @@ struct pptable_funcs {
> > int (*get_dpm_ultimate_freq)(struct smu_context *smu, enum
> > smu_clk_type clk_type, uint32_t *min, uint32_t *max);
> > int (*set_soft_freq_limited_range)(struct smu_context *smu, enum
> > smu_clk_type clk_type, uint32_t min, uint32_t max);
> > int (*override_pcie_parameters)(struct smu_context *smu);
> > + uint32_t (*get_pptable_power_limit)(struct smu_context *smu);
> > };
> >
> > int smu_load_microcode(struct smu_context *smu);
> > @@ -717,4 +717,6 @@ int smu_get_uclk_dpm_states(struct smu_context *smu,
> > int smu_get_dpm_clock_table(struct smu_context *smu,
> > struct dpm_clocks *clock_table);
> >
> > +uint32_t smu_get_pptable_power_limit(struct smu_context *smu, bool
> > lock_needed);
> > +
> > #endif
> > diff --git a/drivers/gpu/drm/amd/powerplay/inc/smu_v11_0.h
> > b/drivers/gpu/drm/amd/powerplay/inc/smu_v11_0.h
> > index 154b57a4dbbb..0ba7a7292bea 100644
> > --- a/drivers/gpu/drm/amd/powerplay/inc/smu_v11_0.h
> > +++ b/drivers/gpu/drm/amd/powerplay/inc/smu_v11_0.h
> > @@ -252,4 +252,6 @@ int smu_v11_0_override_pcie_parameters(struct
> > smu_context *smu);
> >
> > int smu_v11_0_set_default_od_settings(struct smu_context *smu, bool
> > initialize, size_t overdrive_table_size);
> >
> > +uint32_t smu_v11_0_get_max_power_limit(struct smu_context *smu);
> > +
> > #endif
> > diff --git a/drivers/gpu/drm/amd/powerplay/inc/smu_v11_0_pptable.h
> > b/drivers/gpu/drm/amd/powerplay/inc/smu_v11_0_pptable.h
> > index 86cdc3393eac..b2f96a101124 100644
> > --- a/drivers/gpu/drm/amd/powerplay/inc/smu_v11_0_pptable.h
> > +++ b/drivers/gpu/drm/amd/powerplay/inc/smu_v11_0_pptable.h
> > @@ -141,7 +141,9 @@ struct smu_11_0_powerplay_table
> > struct smu_11_0_power_saving_clock_table power_saving_clock;
> > struct smu_11_0_overdrive_table overdrive_table;
> >
> > +#ifndef SMU_11_0_PARTIAL_PPTABLE
> > PPTable_t smc_pptable; //PPTable_t in smu11_driver_if.h
> > +#endif
> > } __attribute__((packed));
> >
> > #endif
> > diff --git a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
> > b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
> > index 4fbdf0e507f3..36cf313754e4 100644
> > --- a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
> > +++ b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
> > @@ -1633,17 +1633,22 @@ static int
> > navi10_display_disable_memory_clock_switch(struct smu_context *smu,
> > return ret;
> > }
> >
> > +static uint32_t navi10_get_pptable_power_limit(struct smu_context *smu)
> > +{
> > + PPTable_t *pptable = smu->smu_table.driver_pptable;
> > + return pptable->SocketPowerLimitAc[PPT_THROTTLER_PPT0];
> > +}
> > +
> > static int navi10_get_power_limit(struct smu_context *smu,
> > uint32_t *limit,
> > - bool asic_default)
> > + bool cap)
> > {
> > PPTable_t *pptable = smu->smu_table.driver_pptable;
> > uint32_t asic_default_power_limit = 0;
> > int ret = 0;
> > int power_src;
> >
> > - if (!smu->default_power_limit ||
> > - !smu->power_limit) {
> > + if (!smu->power_limit) {
> > if (smu_feature_is_enabled(smu, SMU_FEATURE_PPT_BIT)) {
> > power_src = smu_power_get_index(smu,
> > SMU_POWER_SOURCE_AC);
> > if (power_src < 0)
> > @@ -1666,17 +1671,11 @@ static int navi10_get_power_limit(struct
> > smu_context *smu,
> > pptable-
> > >SocketPowerLimitAc[PPT_THROTTLER_PPT0];
> > }
> >
> > - if (smu->od_enabled) {
> > - asic_default_power_limit *= (100 + smu-
> > >smu_table.TDPODLimit);
> > - asic_default_power_limit /= 100;
> > - }
> > -
> > - smu->default_power_limit = asic_default_power_limit;
> > smu->power_limit = asic_default_power_limit;
> > }
> >
> > - if (asic_default)
> > - *limit = smu->default_power_limit;
> > + if (cap)
> > + *limit = smu_v11_0_get_max_power_limit(smu);
> > else
> > *limit = smu->power_limit;
> >
> > @@ -2024,6 +2023,7 @@ static const struct pptable_funcs navi10_ppt_funcs =
> > {
> > .override_pcie_parameters = smu_v11_0_override_pcie_parameters,
> > .set_default_od_settings = navi10_set_default_od_settings,
> > .od_edit_dpm_table = navi10_od_edit_dpm_table,
> > + .get_pptable_power_limit = navi10_get_pptable_power_limit,
> > };
> >
> > void navi10_set_ppt_funcs(struct smu_context *smu)
> > diff --git a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
> > b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
> > index c4f67ee8840e..814d2256ff8e 100644
> > --- a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
> > +++ b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
> > @@ -24,6 +24,8 @@
> > #include <linux/module.h>
> > #include <linux/pci.h>
> >
> > +#define SMU_11_0_PARTIAL_PPTABLE
> > +
> > #include "pp_debug.h"
> > #include "amdgpu.h"
> > #include "amdgpu_smu.h"
> > @@ -31,6 +33,7 @@
> > #include "atomfirmware.h"
> > #include "amdgpu_atomfirmware.h"
> > #include "smu_v11_0.h"
> > +#include "smu_v11_0_pptable.h"
> > #include "soc15_common.h"
> > #include "atom.h"
> > #include "amd_pcie.h"
> > @@ -1045,13 +1048,44 @@ int smu_v11_0_init_max_sustainable_clocks(struct
> > smu_context *smu)
> > return 0;
> > }
> >
> > +uint32_t smu_v11_0_get_max_power_limit(struct smu_context *smu) {
> > + uint32_t od_limit, max_power_limit;
> > + struct smu_11_0_powerplay_table *powerplay_table = NULL;
> > + struct smu_table_context *table_context = &smu->smu_table;
> > + powerplay_table = table_context->power_play_table;
> > +
> > + max_power_limit = smu_get_pptable_power_limit(smu, false);
> > +
> > + if (!max_power_limit) {
> > + // If we couldn't get the table limit, fall back on first-read value
> > + if (!smu->default_power_limit)
> > + smu->default_power_limit = smu->power_limit;
> > + max_power_limit = smu->default_power_limit;
> > + }
> > +
> > + if (smu->od_enabled) {
> > + od_limit = le32_to_cpu(powerplay_table-
> > >overdrive_table.max[SMU_11_0_ODSETTING_POWERPERCENTAGE]);
> > +
> > + pr_debug("ODSETTING_POWERPERCENTAGE: %d
> > (default: %d)\n", od_limit, smu->default_power_limit);
> > +
> > + max_power_limit *= (100 + od_limit);
> > + max_power_limit /= 100;
> > + }
> > +
> > + return max_power_limit;
> > +}
> > +
> > int smu_v11_0_set_power_limit(struct smu_context *smu, uint32_t n)
> > {
> > int ret = 0;
> > + uint32_t max_power_limit;
> > +
> > + max_power_limit = smu_v11_0_get_max_power_limit(smu);
> >
> > - if (n > smu->default_power_limit) {
> > - pr_err("New power limit is over the max allowed %d\n",
> > - smu->default_power_limit);
> > + if (n > max_power_limit) {
> > + pr_err("New power limit (%d) is over the max allowed %d\n",
> > + n,
> > + max_power_limit);
> > return -EINVAL;
> > }
> >
> > diff --git a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c
> > b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c
> > index 5b21386f558d..0b4892833808 100644
> > --- a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c
> > +++ b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c
> > @@ -466,7 +466,6 @@ static int vega20_store_powerplay_table(struct
> > smu_context *smu)
> > sizeof(PPTable_t));
> >
> > table_context->thermal_controller_type = powerplay_table-
> > >ucThermalControllerType;
> > - table_context->TDPODLimit = le32_to_cpu(powerplay_table-
> > >OverDrive8Table.ODSettingsMax[ATOM_VEGA20_ODSETTING_POWERPERCE
> > NTAGE]);
> >
> > return 0;
> > }
> > --
> > 2.23.0
> >
> > _______________________________________________
> > amd-gfx mailing list
> > amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
