Re: [PATCH] drm/amdgpu/smu_v11: Unify and fix power limits

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux