* [PATCH 0/3] Add limit_type to (pptable_funcs)->set_power_limit signature
@ 2021-10-03 4:46 Darren Powell
2021-10-03 4:46 ` [PATCH 1/3] amdgpu/pm: add " Darren Powell
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Darren Powell @ 2021-10-03 4:46 UTC (permalink / raw)
To: amd-gfx; +Cc: Darren Powell
=== Description ===
Add limit_type to (pptable_funcs)->set_power_limit signature
plus two small patches
Fix for incorrect power limit readback in smu11 if POWER_SOURCE_DC
Explicit initialization of cached power limits in smu struct
=== Test System ===
* DESKTOP(AMD FX-8350 + NAVI10(731F/ca), BIOS: F2)
+ ISO(Ubuntu 20.04.3 LTS)
+ Kernel(5.13.0-g9e50250d2084-fdoagd5f)
=== Patch Summary ===
linux: (git@gitlab.freedesktop.org:agd5f) origin/amd-staging-drm-next @ d6119c68a46e
+ ca5057580168 amdgpu/pm: add limit_type to (pptable_funcs)->set_power_limit signature
+ 6e269294d363 drm/amd/pm: Fix incorrect power limit readback in smu11 if POWER_SOURCE_DC
+ 50715de0a584 drm/amd/pm: explicitly initialize cached power limits in smu struct
Darren Powell (3):
amdgpu/pm: add limit_type to (pptable_funcs)->set_power_limit
signature
drm/amd/pm: Fix incorrect power limit readback in smu11 if
POWER_SOURCE_DC
drm/amd/pm: explicitly initialize cached power limits in smu struct
drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h | 4 +++-
drivers/gpu/drm/amd/pm/inc/smu_v11_0.h | 4 +++-
drivers/gpu/drm/amd/pm/inc/smu_v13_0.h | 4 +++-
drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 9 +++++++--
drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c | 15 +++++++++------
drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c | 7 ++++---
.../gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c | 6 ++++--
drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c | 8 +++++---
8 files changed, 38 insertions(+), 19 deletions(-)
base-commit: d6119c68a46eae0b48c77353aa81e6e38b009d24
--
2.33.0
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/3] amdgpu/pm: add limit_type to (pptable_funcs)->set_power_limit signature 2021-10-03 4:46 [PATCH 0/3] Add limit_type to (pptable_funcs)->set_power_limit signature Darren Powell @ 2021-10-03 4:46 ` Darren Powell 2021-10-04 10:27 ` Lazar, Lijo 2021-10-03 4:46 ` [PATCH 2/3] drm/amd/pm: Fix incorrect power limit readback in smu11 if POWER_SOURCE_DC Darren Powell 2021-10-03 4:46 ` [PATCH 3/3] drm/amd/pm: explicitly initialize cached power limits in smu struct Darren Powell 2 siblings, 1 reply; 10+ messages in thread From: Darren Powell @ 2021-10-03 4:46 UTC (permalink / raw) To: amd-gfx; +Cc: Darren Powell modify (pptable_funcs)->set_power_limit signature modify smu11 set_power_limit signature (arcturus, navi10, sienna_cichlid) modify smu13 set_power_limit signature (aldabaran) modify vangogh_set_power_limit signature (vangogh) === Test === sudo bash 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} LOGFILE=pp_show_power_cap.log cp $LOGFILE{,.old} lspci -nn | grep "VGA\|Display" > $LOGFILE FILES=" power1_cap power2_cap" for f in $FILES do if test -f "$HWMON_DIR/$f"; then echo === $f === >> $LOGFILE cat $HWMON_DIR/$f >> $LOGFILE RESTORE_VALUE=`cat $HWMON_DIR/$f` 2>&1 >> $LOGFILE echo RESTORE_VALUE $RESTORE_VALUE >> $LOGFILE echo 120000000 > $HWMON_DIR/$f sleep 3 cat $HWMON_DIR/$f >> $LOGFILE echo $RESTORE_VALUE > $HWMON_DIR/$f sleep 3 cat $HWMON_DIR/$f >> $LOGFILE else echo === $f === >> $LOGFILE echo File Not Found >> $LOGFILE fi done cat $LOGFILE Signed-off-by: Darren Powell <darren.powell@amd.com> --- drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h | 4 +++- drivers/gpu/drm/amd/pm/inc/smu_v11_0.h | 4 +++- drivers/gpu/drm/amd/pm/inc/smu_v13_0.h | 4 +++- drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 5 +++-- drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c | 14 ++++++++------ drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c | 7 ++++--- drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c | 6 ++++-- drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c | 8 +++++--- 8 files changed, 33 insertions(+), 19 deletions(-) diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h index 8156729c370b..3557f4e7fc30 100644 --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h @@ -1008,7 +1008,9 @@ struct pptable_funcs { /** * @set_power_limit: Set power limit in watts. */ - int (*set_power_limit)(struct smu_context *smu, uint32_t n); + int (*set_power_limit)(struct smu_context *smu, + enum smu_ppt_limit_type limit_type, + uint32_t limit); /** * @init_max_sustainable_clocks: Populate max sustainable clock speed diff --git a/drivers/gpu/drm/amd/pm/inc/smu_v11_0.h b/drivers/gpu/drm/amd/pm/inc/smu_v11_0.h index cbdae8a2c698..2d422e6a9feb 100644 --- a/drivers/gpu/drm/amd/pm/inc/smu_v11_0.h +++ b/drivers/gpu/drm/amd/pm/inc/smu_v11_0.h @@ -197,7 +197,9 @@ int smu_v11_0_notify_display_change(struct smu_context *smu); int smu_v11_0_get_current_power_limit(struct smu_context *smu, uint32_t *power_limit); -int smu_v11_0_set_power_limit(struct smu_context *smu, uint32_t n); +int smu_v11_0_set_power_limit(struct smu_context *smu, + enum smu_ppt_limit_type limit_type, + uint32_t limit); int smu_v11_0_init_max_sustainable_clocks(struct smu_context *smu); diff --git a/drivers/gpu/drm/amd/pm/inc/smu_v13_0.h b/drivers/gpu/drm/amd/pm/inc/smu_v13_0.h index dc91eb608791..e5d3b0d1a032 100644 --- a/drivers/gpu/drm/amd/pm/inc/smu_v13_0.h +++ b/drivers/gpu/drm/amd/pm/inc/smu_v13_0.h @@ -163,7 +163,9 @@ int smu_v13_0_notify_display_change(struct smu_context *smu); int smu_v13_0_get_current_power_limit(struct smu_context *smu, uint32_t *power_limit); -int smu_v13_0_set_power_limit(struct smu_context *smu, uint32_t n); +int smu_v13_0_set_power_limit(struct smu_context *smu, + enum smu_ppt_limit_type limit_type, + uint32_t limit); int smu_v13_0_init_max_sustainable_clocks(struct smu_context *smu); diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c index a2a2a8398cd7..faa78a048b1f 100644 --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c @@ -2342,9 +2342,10 @@ static int smu_set_power_limit(void *handle, uint32_t limit) mutex_lock(&smu->mutex); + limit &= (1<<24)-1; if (limit_type != SMU_DEFAULT_PPT_LIMIT) if (smu->ppt_funcs->set_power_limit) { - ret = smu->ppt_funcs->set_power_limit(smu, limit); + ret = smu->ppt_funcs->set_power_limit(smu, limit_type, limit); goto out; } @@ -2360,7 +2361,7 @@ static int smu_set_power_limit(void *handle, uint32_t limit) limit = smu->current_power_limit; if (smu->ppt_funcs->set_power_limit) { - ret = smu->ppt_funcs->set_power_limit(smu, limit); + ret = smu->ppt_funcs->set_power_limit(smu, limit_type, limit); if (!ret && !(smu->user_dpm_profile.flags & SMU_DPM_USER_PROFILE_RESTORE)) smu->user_dpm_profile.power_limit = limit; } diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c index 3470c33ee09d..aedaa4bb15c2 100644 --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c @@ -978,7 +978,9 @@ int smu_v11_0_get_current_power_limit(struct smu_context *smu, return ret; } -int smu_v11_0_set_power_limit(struct smu_context *smu, uint32_t n) +int smu_v11_0_set_power_limit(struct smu_context *smu, + enum smu_ppt_limit_type limit_type, + uint32_t limit) { int power_src; int ret = 0; @@ -1001,16 +1003,16 @@ int smu_v11_0_set_power_limit(struct smu_context *smu, uint32_t n) * BIT 16-23: PowerSource * BIT 0-15: PowerLimit */ - n &= 0xFFFF; - n |= 0 << 24; - n |= (power_src) << 16; - ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_SetPptLimit, n, NULL); + limit &= 0xFFFF; + limit |= 0 << 24; + limit |= (power_src) << 16; + ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_SetPptLimit, limit, NULL); if (ret) { dev_err(smu->adev->dev, "[%s] Set power limit Failed!\n", __func__); return ret; } - smu->current_power_limit = n; + smu->current_power_limit = limit; return 0; } diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c index f6ef0ce6e9e2..eba516428f1b 100644 --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c @@ -2144,11 +2144,12 @@ static int vangogh_get_ppt_limit(struct smu_context *smu, return 0; } -static int vangogh_set_power_limit(struct smu_context *smu, uint32_t ppt_limit) +static int vangogh_set_power_limit(struct smu_context *smu, + enum smu_ppt_limit_type limit_type, + uint32_t ppt_limit) { struct smu_11_5_power_context *power_context = - smu->smu_power.power_context; - uint32_t limit_type = ppt_limit >> 24; + smu->smu_power.power_context; int ret = 0; if (!smu_cmn_feature_is_enabled(smu, SMU_FEATURE_PPT_BIT)) { 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 5019903db492..59a7d276541d 100644 --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c @@ -1241,11 +1241,13 @@ static int aldebaran_get_power_limit(struct smu_context *smu, return 0; } -static int aldebaran_set_power_limit(struct smu_context *smu, uint32_t n) +static int aldebaran_set_power_limit(struct smu_context *smu, + enum smu_ppt_limit_type limit_type, + uint32_t limit) { /* Power limit can be set only through primary die */ if (aldebaran_is_primary(smu)) - return smu_v13_0_set_power_limit(smu, n); + return smu_v13_0_set_power_limit(smu, limit_type, limit); return -EINVAL; } 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 05c5e61f3506..58d837d9a414 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 @@ -945,7 +945,9 @@ int smu_v13_0_get_current_power_limit(struct smu_context *smu, return ret; } -int smu_v13_0_set_power_limit(struct smu_context *smu, uint32_t n) +int smu_v13_0_set_power_limit(struct smu_context *smu, + enum smu_ppt_limit_type limit_type, + uint32_t limit) { int ret = 0; @@ -954,13 +956,13 @@ int smu_v13_0_set_power_limit(struct smu_context *smu, uint32_t n) return -EOPNOTSUPP; } - ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_SetPptLimit, n, NULL); + ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_SetPptLimit, limit, NULL); if (ret) { dev_err(smu->adev->dev, "[%s] Set power limit Failed!\n", __func__); return ret; } - smu->current_power_limit = n; + smu->current_power_limit = limit; return 0; } -- 2.33.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] amdgpu/pm: add limit_type to (pptable_funcs)->set_power_limit signature 2021-10-03 4:46 ` [PATCH 1/3] amdgpu/pm: add " Darren Powell @ 2021-10-04 10:27 ` Lazar, Lijo 0 siblings, 0 replies; 10+ messages in thread From: Lazar, Lijo @ 2021-10-04 10:27 UTC (permalink / raw) To: Darren Powell, amd-gfx On 10/3/2021 10:16 AM, Darren Powell wrote: > modify (pptable_funcs)->set_power_limit signature > modify smu11 set_power_limit signature (arcturus, navi10, sienna_cichlid) > modify smu13 set_power_limit signature (aldabaran) > modify vangogh_set_power_limit signature (vangogh) > > === Test === > sudo bash > > 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} > LOGFILE=pp_show_power_cap.log > > cp $LOGFILE{,.old} > lspci -nn | grep "VGA\|Display" > $LOGFILE > FILES=" > power1_cap > power2_cap" > > for f in $FILES > do > if test -f "$HWMON_DIR/$f"; then > echo === $f === >> $LOGFILE > cat $HWMON_DIR/$f >> $LOGFILE > RESTORE_VALUE=`cat $HWMON_DIR/$f` 2>&1 >> $LOGFILE > echo RESTORE_VALUE $RESTORE_VALUE >> $LOGFILE > echo 120000000 > $HWMON_DIR/$f > sleep 3 > cat $HWMON_DIR/$f >> $LOGFILE > echo $RESTORE_VALUE > $HWMON_DIR/$f > sleep 3 > cat $HWMON_DIR/$f >> $LOGFILE > else > echo === $f === >> $LOGFILE > echo File Not Found >> $LOGFILE > fi > done > cat $LOGFILE > > Signed-off-by: Darren Powell <darren.powell@amd.com> > --- > drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h | 4 +++- > drivers/gpu/drm/amd/pm/inc/smu_v11_0.h | 4 +++- > drivers/gpu/drm/amd/pm/inc/smu_v13_0.h | 4 +++- > drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 5 +++-- > drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c | 14 ++++++++------ > drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c | 7 ++++--- > drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c | 6 ++++-- > drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c | 8 +++++--- > 8 files changed, 33 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h > index 8156729c370b..3557f4e7fc30 100644 > --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h > +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h > @@ -1008,7 +1008,9 @@ struct pptable_funcs { > /** > * @set_power_limit: Set power limit in watts. > */ > - int (*set_power_limit)(struct smu_context *smu, uint32_t n); > + int (*set_power_limit)(struct smu_context *smu, > + enum smu_ppt_limit_type limit_type, > + uint32_t limit); > > /** > * @init_max_sustainable_clocks: Populate max sustainable clock speed > diff --git a/drivers/gpu/drm/amd/pm/inc/smu_v11_0.h b/drivers/gpu/drm/amd/pm/inc/smu_v11_0.h > index cbdae8a2c698..2d422e6a9feb 100644 > --- a/drivers/gpu/drm/amd/pm/inc/smu_v11_0.h > +++ b/drivers/gpu/drm/amd/pm/inc/smu_v11_0.h > @@ -197,7 +197,9 @@ int smu_v11_0_notify_display_change(struct smu_context *smu); > int smu_v11_0_get_current_power_limit(struct smu_context *smu, > uint32_t *power_limit); > > -int smu_v11_0_set_power_limit(struct smu_context *smu, uint32_t n); > +int smu_v11_0_set_power_limit(struct smu_context *smu, > + enum smu_ppt_limit_type limit_type, > + uint32_t limit); > > int smu_v11_0_init_max_sustainable_clocks(struct smu_context *smu); > > diff --git a/drivers/gpu/drm/amd/pm/inc/smu_v13_0.h b/drivers/gpu/drm/amd/pm/inc/smu_v13_0.h > index dc91eb608791..e5d3b0d1a032 100644 > --- a/drivers/gpu/drm/amd/pm/inc/smu_v13_0.h > +++ b/drivers/gpu/drm/amd/pm/inc/smu_v13_0.h > @@ -163,7 +163,9 @@ int smu_v13_0_notify_display_change(struct smu_context *smu); > int smu_v13_0_get_current_power_limit(struct smu_context *smu, > uint32_t *power_limit); > > -int smu_v13_0_set_power_limit(struct smu_context *smu, uint32_t n); > +int smu_v13_0_set_power_limit(struct smu_context *smu, > + enum smu_ppt_limit_type limit_type, > + uint32_t limit); > > int smu_v13_0_init_max_sustainable_clocks(struct smu_context *smu); > > diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c > index a2a2a8398cd7..faa78a048b1f 100644 > --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c > +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c > @@ -2342,9 +2342,10 @@ static int smu_set_power_limit(void *handle, uint32_t limit) > > mutex_lock(&smu->mutex); > > + limit &= (1<<24)-1; > if (limit_type != SMU_DEFAULT_PPT_LIMIT) > if (smu->ppt_funcs->set_power_limit) { > - ret = smu->ppt_funcs->set_power_limit(smu, limit); > + ret = smu->ppt_funcs->set_power_limit(smu, limit_type, limit); > goto out; > } > > @@ -2360,7 +2361,7 @@ static int smu_set_power_limit(void *handle, uint32_t limit) > limit = smu->current_power_limit; > > if (smu->ppt_funcs->set_power_limit) { > - ret = smu->ppt_funcs->set_power_limit(smu, limit); > + ret = smu->ppt_funcs->set_power_limit(smu, limit_type, limit); > if (!ret && !(smu->user_dpm_profile.flags & SMU_DPM_USER_PROFILE_RESTORE)) > smu->user_dpm_profile.power_limit = limit; > } > diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c > index 3470c33ee09d..aedaa4bb15c2 100644 > --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c > @@ -978,7 +978,9 @@ int smu_v11_0_get_current_power_limit(struct smu_context *smu, > return ret; > } > > -int smu_v11_0_set_power_limit(struct smu_context *smu, uint32_t n) > +int smu_v11_0_set_power_limit(struct smu_context *smu, > + enum smu_ppt_limit_type limit_type, > + uint32_t limit) > { > int power_src; > int ret = 0; > @@ -1001,16 +1003,16 @@ int smu_v11_0_set_power_limit(struct smu_context *smu, uint32_t n) > * BIT 16-23: PowerSource > * BIT 0-15: PowerLimit > */ > - n &= 0xFFFF; > - n |= 0 << 24; > - n |= (power_src) << 16; > - ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_SetPptLimit, n, NULL); Since limit_type is introduced as arg, could you also add below to smuv11/v13? Currently, anything other than default is used only in vangogh. if (limit_type != SMU_DEFAULT_PPT_LIMIT) return -EINVAL; That will also avoid any 'unused variable' warning. Thanks, Lijo > + limit &= 0xFFFF; > + limit |= 0 << 24; > + limit |= (power_src) << 16; > + ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_SetPptLimit, limit, NULL); > if (ret) { > dev_err(smu->adev->dev, "[%s] Set power limit Failed!\n", __func__); > return ret; > } > > - smu->current_power_limit = n; > + smu->current_power_limit = limit; > > return 0; > } > diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c > index f6ef0ce6e9e2..eba516428f1b 100644 > --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c > @@ -2144,11 +2144,12 @@ static int vangogh_get_ppt_limit(struct smu_context *smu, > return 0; > } > > -static int vangogh_set_power_limit(struct smu_context *smu, uint32_t ppt_limit) > +static int vangogh_set_power_limit(struct smu_context *smu, > + enum smu_ppt_limit_type limit_type, > + uint32_t ppt_limit) > { > struct smu_11_5_power_context *power_context = > - smu->smu_power.power_context; > - uint32_t limit_type = ppt_limit >> 24; > + smu->smu_power.power_context; > int ret = 0; > > if (!smu_cmn_feature_is_enabled(smu, SMU_FEATURE_PPT_BIT)) { > 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 5019903db492..59a7d276541d 100644 > --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c > @@ -1241,11 +1241,13 @@ static int aldebaran_get_power_limit(struct smu_context *smu, > return 0; > } > > -static int aldebaran_set_power_limit(struct smu_context *smu, uint32_t n) > +static int aldebaran_set_power_limit(struct smu_context *smu, > + enum smu_ppt_limit_type limit_type, > + uint32_t limit) > { > /* Power limit can be set only through primary die */ > if (aldebaran_is_primary(smu)) > - return smu_v13_0_set_power_limit(smu, n); > + return smu_v13_0_set_power_limit(smu, limit_type, limit); > > return -EINVAL; > } > 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 05c5e61f3506..58d837d9a414 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 > @@ -945,7 +945,9 @@ int smu_v13_0_get_current_power_limit(struct smu_context *smu, > return ret; > } > > -int smu_v13_0_set_power_limit(struct smu_context *smu, uint32_t n) > +int smu_v13_0_set_power_limit(struct smu_context *smu, > + enum smu_ppt_limit_type limit_type, > + uint32_t limit) > { > int ret = 0; > > @@ -954,13 +956,13 @@ int smu_v13_0_set_power_limit(struct smu_context *smu, uint32_t n) > return -EOPNOTSUPP; > } > > - ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_SetPptLimit, n, NULL); > + ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_SetPptLimit, limit, NULL); > if (ret) { > dev_err(smu->adev->dev, "[%s] Set power limit Failed!\n", __func__); > return ret; > } > > - smu->current_power_limit = n; > + smu->current_power_limit = limit; > > return 0; > } > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/3] drm/amd/pm: Fix incorrect power limit readback in smu11 if POWER_SOURCE_DC 2021-10-03 4:46 [PATCH 0/3] Add limit_type to (pptable_funcs)->set_power_limit signature Darren Powell 2021-10-03 4:46 ` [PATCH 1/3] amdgpu/pm: add " Darren Powell @ 2021-10-03 4:46 ` Darren Powell 2021-10-03 5:31 ` Nils Wallménius 2021-10-03 4:46 ` [PATCH 3/3] drm/amd/pm: explicitly initialize cached power limits in smu struct Darren Powell 2 siblings, 1 reply; 10+ messages in thread From: Darren Powell @ 2021-10-03 4:46 UTC (permalink / raw) To: amd-gfx; +Cc: Darren Powell when smu->adev->pm.ac_power == 0, message parameter with bit 16 set is saved to smu->current_power_limit. Fixes: 0cb4c62125a9 ("drm/amd/pm: correct power limit setting for SMU V11)" Signed-off-by: Darren Powell <darren.powell@amd.com> --- drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c index aedaa4bb15c2..9bb6da99d5b5 100644 --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c @@ -984,6 +984,7 @@ int smu_v11_0_set_power_limit(struct smu_context *smu, { int power_src; int ret = 0; + uint32_t limit_param; if (!smu_cmn_feature_is_enabled(smu, SMU_FEATURE_PPT_BIT)) { dev_err(smu->adev->dev, "Setting new power limit is not supported!\n"); @@ -1003,10 +1004,10 @@ int smu_v11_0_set_power_limit(struct smu_context *smu, * BIT 16-23: PowerSource * BIT 0-15: PowerLimit */ - limit &= 0xFFFF; - limit |= 0 << 24; - limit |= (power_src) << 16; - ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_SetPptLimit, limit, NULL); + limit_param = (limit & 0xFFFF); + limit_param |= 0 << 24; + limit_param |= (power_src) << 16; + ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_SetPptLimit, limit_param, NULL); if (ret) { dev_err(smu->adev->dev, "[%s] Set power limit Failed!\n", __func__); return ret; -- 2.33.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] drm/amd/pm: Fix incorrect power limit readback in smu11 if POWER_SOURCE_DC 2021-10-03 4:46 ` [PATCH 2/3] drm/amd/pm: Fix incorrect power limit readback in smu11 if POWER_SOURCE_DC Darren Powell @ 2021-10-03 5:31 ` Nils Wallménius 2021-10-04 1:15 ` Powell, Darren 0 siblings, 1 reply; 10+ messages in thread From: Nils Wallménius @ 2021-10-03 5:31 UTC (permalink / raw) To: Darren Powell; +Cc: amd-gfx [-- Attachment #1: Type: text/plain, Size: 1932 bytes --] Hi, sorry for the drive-by comment but limit_param |= 0 << 24; Doesn't do anything. Best regards Nils Den sön 3 okt. 2021 06:47Darren Powell <darren.powell@amd.com> skrev: > when smu->adev->pm.ac_power == 0, message parameter with bit 16 set is > saved > to smu->current_power_limit. > > Fixes: 0cb4c62125a9 ("drm/amd/pm: correct power limit setting for SMU > V11)" > > Signed-off-by: Darren Powell <darren.powell@amd.com> > --- > drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c > b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c > index aedaa4bb15c2..9bb6da99d5b5 100644 > --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c > @@ -984,6 +984,7 @@ int smu_v11_0_set_power_limit(struct smu_context *smu, > { > int power_src; > int ret = 0; > + uint32_t limit_param; > > if (!smu_cmn_feature_is_enabled(smu, SMU_FEATURE_PPT_BIT)) { > dev_err(smu->adev->dev, "Setting new power limit is not > supported!\n"); > @@ -1003,10 +1004,10 @@ int smu_v11_0_set_power_limit(struct smu_context > *smu, > * BIT 16-23: PowerSource > * BIT 0-15: PowerLimit > */ > - limit &= 0xFFFF; > - limit |= 0 << 24; > - limit |= (power_src) << 16; > - ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_SetPptLimit, > limit, NULL); > + limit_param = (limit & 0xFFFF); > + limit_param |= 0 << 24; > + limit_param |= (power_src) << 16; > + ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_SetPptLimit, > limit_param, NULL); > if (ret) { > dev_err(smu->adev->dev, "[%s] Set power limit Failed!\n", > __func__); > return ret; > -- > 2.33.0 > > [-- Attachment #2: Type: text/html, Size: 2946 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] drm/amd/pm: Fix incorrect power limit readback in smu11 if POWER_SOURCE_DC 2021-10-03 5:31 ` Nils Wallménius @ 2021-10-04 1:15 ` Powell, Darren 0 siblings, 0 replies; 10+ messages in thread From: Powell, Darren @ 2021-10-04 1:15 UTC (permalink / raw) To: Nils Wallménius; +Cc: amd-gfx [-- Attachment #1: Type: text/plain, Size: 2465 bytes --] [AMD Official Use Only] Yup, agreed, but I rationalized that it: - minimizes code changes, leave existing code in place - reinforces the comment, which states that only 0 is supported in this bitfield at this time Thanks Darren ________________________________ From: Nils Wallménius <nils.wallmenius@gmail.com> Sent: Sunday, October 3, 2021 1:31 AM To: Powell, Darren <Darren.Powell@amd.com> Cc: amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org> Subject: Re: [PATCH 2/3] drm/amd/pm: Fix incorrect power limit readback in smu11 if POWER_SOURCE_DC Hi, sorry for the drive-by comment but limit_param |= 0 << 24; Doesn't do anything. Best regards Nils Den sön 3 okt. 2021 06:47Darren Powell <darren.powell@amd.com<mailto:darren.powell@amd.com>> skrev: when smu->adev->pm.ac_power == 0, message parameter with bit 16 set is saved to smu->current_power_limit. Fixes: 0cb4c62125a9 ("drm/amd/pm: correct power limit setting for SMU V11)" Signed-off-by: Darren Powell <darren.powell@amd.com<mailto:darren.powell@amd.com>> --- drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c index aedaa4bb15c2..9bb6da99d5b5 100644 --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c @@ -984,6 +984,7 @@ int smu_v11_0_set_power_limit(struct smu_context *smu, { int power_src; int ret = 0; + uint32_t limit_param; if (!smu_cmn_feature_is_enabled(smu, SMU_FEATURE_PPT_BIT)) { dev_err(smu->adev->dev, "Setting new power limit is not supported!\n"); @@ -1003,10 +1004,10 @@ int smu_v11_0_set_power_limit(struct smu_context *smu, * BIT 16-23: PowerSource * BIT 0-15: PowerLimit */ - limit &= 0xFFFF; - limit |= 0 << 24; - limit |= (power_src) << 16; - ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_SetPptLimit, limit, NULL); + limit_param = (limit & 0xFFFF); + limit_param |= 0 << 24; + limit_param |= (power_src) << 16; + ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_SetPptLimit, limit_param, NULL); if (ret) { dev_err(smu->adev->dev, "[%s] Set power limit Failed!\n", __func__); return ret; -- 2.33.0 [-- Attachment #2: Type: text/html, Size: 5268 bytes --] ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/3] drm/amd/pm: explicitly initialize cached power limits in smu struct 2021-10-03 4:46 [PATCH 0/3] Add limit_type to (pptable_funcs)->set_power_limit signature Darren Powell 2021-10-03 4:46 ` [PATCH 1/3] amdgpu/pm: add " Darren Powell 2021-10-03 4:46 ` [PATCH 2/3] drm/amd/pm: Fix incorrect power limit readback in smu11 if POWER_SOURCE_DC Darren Powell @ 2021-10-03 4:46 ` Darren Powell 2021-10-04 10:43 ` Lazar, Lijo 2 siblings, 1 reply; 10+ messages in thread From: Darren Powell @ 2021-10-03 4:46 UTC (permalink / raw) To: amd-gfx; +Cc: Darren Powell Code appears to initialize values but macro will exit without error or initializing value if function is not implmented Signed-off-by: Darren Powell <darren.powell@amd.com> --- drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c index faa78a048b1f..210f047e136d 100644 --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c @@ -712,6 +712,10 @@ static int smu_late_init(void *handle) return ret; } + smu->current_power_limit = 0; + smu->default_power_limit = 0; + smu->max_power_limit = 0; + ret = smu_get_asic_power_limits(smu, &smu->current_power_limit, &smu->default_power_limit, -- 2.33.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] drm/amd/pm: explicitly initialize cached power limits in smu struct 2021-10-03 4:46 ` [PATCH 3/3] drm/amd/pm: explicitly initialize cached power limits in smu struct Darren Powell @ 2021-10-04 10:43 ` Lazar, Lijo 2021-10-05 5:04 ` Powell, Darren 0 siblings, 1 reply; 10+ messages in thread From: Lazar, Lijo @ 2021-10-04 10:43 UTC (permalink / raw) To: Darren Powell, amd-gfx On 10/3/2021 10:16 AM, Darren Powell wrote: > Code appears to initialize values but macro will exit without error > or initializing value if function is not implmented > > Signed-off-by: Darren Powell <darren.powell@amd.com> > --- > drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c > index faa78a048b1f..210f047e136d 100644 > --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c > +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c > @@ -712,6 +712,10 @@ static int smu_late_init(void *handle) > return ret; > } > > + smu->current_power_limit = 0; > + smu->default_power_limit = 0; > + smu->max_power_limit = 0; > + If this is only about first-time init - smu_context is part of adev, it will be zero initialized when adev is allocated. Thanks, Lijo > ret = smu_get_asic_power_limits(smu, > &smu->current_power_limit, > &smu->default_power_limit, > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] drm/amd/pm: explicitly initialize cached power limits in smu struct 2021-10-04 10:43 ` Lazar, Lijo @ 2021-10-05 5:04 ` Powell, Darren 2021-10-05 5:46 ` Lazar, Lijo 0 siblings, 1 reply; 10+ messages in thread From: Powell, Darren @ 2021-10-05 5:04 UTC (permalink / raw) To: Lazar, Lijo, amd-gfx [-- Attachment #1: Type: text/plain, Size: 2453 bytes --] [AMD Official Use Only] I'm just looking to clarify this code. The macro eventually expands to look like this if ((smu)->ppt_funcs) { if ((smu)->ppt_funcs->get_power_limit) (smu)->ppt_funcs->get_power_limit(smu, &smu->current_power_limit, &smu->default_power_limit, &smu->max_power_limit); else return 0; } else return -EINVAL; But you have to dig to realize that it's a macro, and that it makes no modification if the function is not defined. It's not clear without then searching and following the function pointers which platforms are using the saved value. I thought of inserting the following comment or should I just drop this altogether? /* seed the cached smu power limit values iff get_power_limit is defined, otherwise they remain 0 */ Thanks Darren ________________________________ From: Lazar, Lijo <Lijo.Lazar@amd.com> Sent: Monday, October 4, 2021 6:43 AM To: Powell, Darren <Darren.Powell@amd.com>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org> Subject: Re: [PATCH 3/3] drm/amd/pm: explicitly initialize cached power limits in smu struct On 10/3/2021 10:16 AM, Darren Powell wrote: > Code appears to initialize values but macro will exit without error > or initializing value if function is not implmented > > Signed-off-by: Darren Powell <darren.powell@amd.com> > --- > drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c > index faa78a048b1f..210f047e136d 100644 > --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c > +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c > @@ -712,6 +712,10 @@ static int smu_late_init(void *handle) > return ret; > } > > + smu->current_power_limit = 0; > + smu->default_power_limit = 0; > + smu->max_power_limit = 0; > + If this is only about first-time init - smu_context is part of adev, it will be zero initialized when adev is allocated. Thanks, Lijo > ret = smu_get_asic_power_limits(smu, > &smu->current_power_limit, > &smu->default_power_limit, > [-- Attachment #2: Type: text/html, Size: 7433 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] drm/amd/pm: explicitly initialize cached power limits in smu struct 2021-10-05 5:04 ` Powell, Darren @ 2021-10-05 5:46 ` Lazar, Lijo 0 siblings, 0 replies; 10+ messages in thread From: Lazar, Lijo @ 2021-10-05 5:46 UTC (permalink / raw) To: Powell, Darren, amd-gfx On 10/5/2021 10:34 AM, Powell, Darren wrote: > [AMD Official Use Only] > > > I'm just looking to clarify this code. The macro eventually expands to > look like this > > if ((smu)->ppt_funcs) > { > if ((smu)->ppt_funcs->get_power_limit) > (smu)->ppt_funcs->get_power_limit(smu, > &smu->current_power_limit, > &smu->default_power_limit, > &smu->max_power_limit); > else > return 0; > } > else > return -EINVAL; > > But you have to dig to realize that it's a macro, and that it makes no > modification if the function is not defined. It's not clear without then > searching and following the function pointers which platforms are using > the saved value. I thought of inserting the following comment or should > I just drop this altogether? Yes. The limit values are by default initialized to 0. If the function is not supported, it's not considered an error to fail late_init (it only reports 0 values in hwmon). late_init also gets called on other occasions like resume or after a GPU-only reset. If you want to change the invalid limit value in hwmon from 0 to something else (to differentiate unsupported vs API returning 0 value), then better to do in sw_init. Thanks, Lijo > /* seed the cached smu power limit values iff get_power_limit is > defined, otherwise they remain 0 */ > > Thanks > Darren > > ------------------------------------------------------------------------ > *From:* Lazar, Lijo <Lijo.Lazar@amd.com> > *Sent:* Monday, October 4, 2021 6:43 AM > *To:* Powell, Darren <Darren.Powell@amd.com>; > amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org> > *Subject:* Re: [PATCH 3/3] drm/amd/pm: explicitly initialize cached > power limits in smu struct > > > On 10/3/2021 10:16 AM, Darren Powell wrote: >> Code appears to initialize values but macro will exit without error >> or initializing value if function is not implmented >> >> Signed-off-by: Darren Powell <darren.powell@amd.com> >> --- >> drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c >> index faa78a048b1f..210f047e136d 100644 >> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c >> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c >> @@ -712,6 +712,10 @@ static int smu_late_init(void *handle) >> return ret; >> } >> >> + smu->current_power_limit = 0; >> + smu->default_power_limit = 0; >> + smu->max_power_limit = 0; >> + > > If this is only about first-time init - smu_context is part of adev, it > will be zero initialized when adev is allocated. > > > Thanks, > Lijo > >> ret = smu_get_asic_power_limits(smu, >> &smu->current_power_limit, >> &smu->default_power_limit, >> ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-10-05 5:46 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-03 4:46 [PATCH 0/3] Add limit_type to (pptable_funcs)->set_power_limit signature Darren Powell 2021-10-03 4:46 ` [PATCH 1/3] amdgpu/pm: add " Darren Powell 2021-10-04 10:27 ` Lazar, Lijo 2021-10-03 4:46 ` [PATCH 2/3] drm/amd/pm: Fix incorrect power limit readback in smu11 if POWER_SOURCE_DC Darren Powell 2021-10-03 5:31 ` Nils Wallménius 2021-10-04 1:15 ` Powell, Darren 2021-10-03 4:46 ` [PATCH 3/3] drm/amd/pm: explicitly initialize cached power limits in smu struct Darren Powell 2021-10-04 10:43 ` Lazar, Lijo 2021-10-05 5:04 ` Powell, Darren 2021-10-05 5:46 ` Lazar, Lijo
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.