[PATCH 2/6] amdgpu/pm: clean up smu_get_power_limit function signature
Powell, Darren
Darren.Powell at amd.com
Thu Jun 3 02:47:40 UTC 2021
[Public]
I think both of those candidates have drawbacks
** option #1 has a clash with the limit_level
eg.
enum pp_power_limit_level limit_level;
enum pp_power_limit power_limit;
power_limit = PP_PWR_LIMIT_DEFAULT; // name clash
** option #2 doesn't describe the usage, and might as well just be a uint
power_limit = PP_PWR_LIMIT_0; // no use in reading code later
Would you accept "category" as a suitable solution?
eg.
enum pp_power_category
{
PP_PWR_CATEGORY_DEFAULT,
PP_PWR_CATEGORY_FAST,
PP_PWR_CATEGORY_APU,
PP_PWR_CATEGORY_PLATFORM,
};
Other possibilities may be
** class
** subsystem
** type
** element
Thanks
Darren
________________________________
From: Lazar, Lijo <Lijo.Lazar at amd.com>
Sent: Wednesday, June 2, 2021 12:32 AM
To: Powell, Darren <Darren.Powell at amd.com>; amd-gfx at lists.freedesktop.org <amd-gfx at lists.freedesktop.org>
Subject: RE: [PATCH 2/6] amdgpu/pm: clean up smu_get_power_limit function signature
[Public]
I’m looking for an appropriate name that can accommodate more limits like
enum pp_power_limit
{
PP_PWR_LIMIT_DEFAULT,
PP_PWR_LIMIT_FAST,
PP_PWR_LIMIT_APU,
PP_PWR_LIMIT_PLATFORM,
};
Or simply different limits where the meaning may change based on ASIC - LIMIT2 could be platform power limit for ASIC1 or LIMIT2 could be max power allocation for a domain (say memory) for ASIC2
enum pp_power_limit
{
PP_PWR_LIMIT_0,
PP_PWR_LIMIT_1,
PP_PWR_LIMIT_2,
PP_PWR_LIMIT_3,
};
Thanks,
Lijo
From: Powell, Darren <Darren.Powell at amd.com>
Sent: Wednesday, June 2, 2021 9:25 AM
To: Lazar, Lijo <Lijo.Lazar at amd.com>; amd-gfx at lists.freedesktop.org
Subject: Re: [PATCH 2/6] amdgpu/pm: clean up smu_get_power_limit function signature
[Public]
I'm not sure exactly what you are looking for. The enums sample_window and limit_level map to power{1,2} and cap{min,max,default,current} respectively. I added the enums to make the function signatures more readable and stop the use of value as an input and output variable.
Please give more specific example?
Darren
= HWMON Mapping =
== Read ==
SENSOR(power1_average) amdgpu_hwmon_show_power_avg(0) amdgpu_dpm_read_sensor(AMDGPU_PP_SENSOR_GPU_POWER)
SENSOR(power1_cap_max) amdgpu_hwmon_show_power_cap_max(0) get_power_limit(PP_PWR_LIMIT_MAX, PP_PWR_WINDOW_DEFAULT)
SENSOR(power1_cap_min) amdgpu_hwmon_show_power_cap_min(0) 0
SENSOR(power1_cap) amdgpu_hwmon_show_power_cap(0) get_power_limit(PP_PWR_LIMIT_CURRENT, PP_PWR_WINDOW_DEFAULT)
SENSOR(power1_cap_default) amdgpu_hwmon_show_power_cap_default(0) get_power_limit(PP_PWR_LIMIT_DEFAULT, PP_PWR_WINDOW_DEFAULT)
SENSOR(power1_label) amdgpu_hwmon_show_power_label(0) "slowPPT"
SENSOR(power2_average) amdgpu_hwmon_show_power_avg(1) amdgpu_dpm_read_sensor(AMDGPU_PP_SENSOR_GPU_POWER)
SENSOR(power2_cap_max) amdgpu_hwmon_show_power_cap_max(1) get_power_limit(PP_PWR_LIMIT_MAX, PP_PWR_WINDOW_FAST)
SENSOR(power2_cap_min) amdgpu_hwmon_show_power_cap_min(1) 0
SENSOR(power2_cap) amdgpu_hwmon_show_power_cap(1) get_power_limit(PP_PWR_LIMIT_CURRENT, PP_PWR_WINDOW_FAST)
SENSOR(power2_cap_default) amdgpu_hwmon_show_power_cap_default(1) get_power_limit(PP_PWR_LIMIT_DEFAULT, PP_PWR_WINDOW_FAST)
SENSOR(power2_label) amdgpu_hwmon_show_power_label(1) "fastPPT"
== Write ==
SENSOR(power1_cap) amdgpu_hwmon_set_power_cap(0,value) set_power_limit( (0<<24) || value )
SENSOR(power2_cap) amdgpu_hwmon_set_power_cap(1,value) set_power_limit( (1<<24) || value )
= Summary =
power1 => PP_PWR_WINDOW_DEFAULT ( label ("slowPPT"))
power2 => PP_PWR_WINDOW_FAST ( label ("fastPPT"))
power_avg => AMDGPU_PP_SENSOR_GPU_POWER
power_cap_max => PP_PWR_LIMIT_MAX
power_cap_min => PP_PWR_LIMIT_MIN (optimized to 0)
power_cap => PP_PWR_LIMIT_CURRENT
power_cap_default => PP_PWR_LIMIT_DEFAULT
________________________________
From: Lazar, Lijo <Lijo.Lazar at amd.com<mailto:Lijo.Lazar at amd.com>>
Sent: Tuesday, June 1, 2021 12:56 AM
To: Powell, Darren <Darren.Powell at amd.com<mailto:Darren.Powell at amd.com>>; amd-gfx at lists.freedesktop.org<mailto:amd-gfx at lists.freedesktop.org> <amd-gfx at lists.freedesktop.org<mailto:amd-gfx at lists.freedesktop.org>>
Subject: RE: [PATCH 2/6] amdgpu/pm: clean up smu_get_power_limit function signature
[Public]
May be just call it power_limit or power_cap similar to hwmon. The various limits correspond to hwmon power[1-*]_cap and levels correspond to min/ max etc.
Thanks,
Lijo
From: Powell, Darren <Darren.Powell at amd.com<mailto:Darren.Powell at amd.com>>
Sent: Tuesday, June 1, 2021 4:50 AM
To: Lazar, Lijo <Lijo.Lazar at amd.com<mailto:Lijo.Lazar at amd.com>>; amd-gfx at lists.freedesktop.org<mailto:amd-gfx at lists.freedesktop.org>
Subject: Re: [PATCH 2/6] amdgpu/pm: clean up smu_get_power_limit function signature
[Public]
>< > The limits are not limited to sample window. There are limits like APU only limit, platform limit and totally obscure ones like PPT0/PPT1 etc.
>It's better that the new enum takes care of those as well in case there is a need to make them available through sysfs.
I think you mean something more like this?
+ enum pp_power_constraints
+{
+ PP_PWR_CONSTRAINT_DEFAULT,
+ PP_PWR_CONSTRAINT_FASTWINDOW,
+};
+
________________________________
From: Lazar, Lijo <Lijo.Lazar at amd.com<mailto:Lijo.Lazar at amd.com>>
Sent: Monday, May 31, 2021 2:04 AM
To: Powell, Darren <Darren.Powell at amd.com<mailto:Darren.Powell at amd.com>>; amd-gfx at lists.freedesktop.org<mailto:amd-gfx at lists.freedesktop.org> <amd-gfx at lists.freedesktop.org<mailto:amd-gfx at lists.freedesktop.org>>
Subject: RE: [PATCH 2/6] amdgpu/pm: clean up smu_get_power_limit function signature
[Public]
-----Original Message-----
From: Powell, Darren <Darren.Powell at amd.com<mailto:Darren.Powell at amd.com>>
Sent: Saturday, May 29, 2021 4:36 AM
To: amd-gfx at lists.freedesktop.org<mailto:amd-gfx at lists.freedesktop.org>
Cc: Powell, Darren <Darren.Powell at amd.com<mailto:Darren.Powell at amd.com>>
Subject: [PATCH 2/6] amdgpu/pm: clean up smu_get_power_limit function signature
add two new powerplay enums (limit_level, sample_window) add enums to smu_get_power_limit signature remove input bitfield stuffing of output variable limit update calls to smu_get_power_limit
* Test
AMDGPU_PCI_ADDR=`lspci -nn | grep "VGA\|Display" | cut -d " " -f 1` AMDGPU_HWMON=`ls -la /sys/class/hwmon | grep $AMDGPU_PCI_ADDR | cut -d " " -f 10` HWMON_DIR=/sys/class/hwmon/${AMDGPU_HWMON}
lspci -nn | grep "VGA\|Display" ; \
echo "=== power1 cap ===" ; cat $HWMON_DIR/power1_cap ; \
echo "=== power1 cap max ===" ; cat $HWMON_DIR/power1_cap_max ; \
echo "=== power1 cap def ===" ; cat $HWMON_DIR/power1_cap_default
Signed-off-by: Darren Powell <darren.powell at amd.com<mailto:darren.powell at amd.com>>
---
.../gpu/drm/amd/include/kgd_pp_interface.h | 14 ++++++++
drivers/gpu/drm/amd/pm/amdgpu_pm.c | 18 +++++-----
drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h | 3 +-
drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 34 +++++++++++++++++--
4 files changed, 57 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/amd/include/kgd_pp_interface.h b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
index b1cd52a9d684..ddbf802ea8ad 100644
--- a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
+++ b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
@@ -192,6 +192,20 @@ enum pp_df_cstate {
DF_CSTATE_ALLOW,
};
+enum pp_power_limit_level
+{
+ PP_PWR_LIMIT_MIN = -1,
+ PP_PWR_LIMIT_CURRENT,
+ PP_PWR_LIMIT_DEFAULT,
+ PP_PWR_LIMIT_MAX,
+};
+
+ enum pp_power_sample_window
+{
+ PP_PWR_WINDOW_DEFAULT,
+ PP_PWR_WINDOW_FAST,
+};
+
< > The limits are not limited to sample window. There are limits like APU only limit, platform limit and totally obscure ones like PPT0/PPT1 etc.
It's better that the new enum takes care of those as well in case there is a need to make them available through sysfs.
Thanks,
Lijo
#define PP_GROUP_MASK 0xF0000000
#define PP_GROUP_SHIFT 28
diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
index 13da377888d2..f7b45803431d 100644
--- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
@@ -2717,8 +2717,8 @@ static ssize_t amdgpu_hwmon_show_power_cap_max(struct device *dev, {
struct amdgpu_device *adev = dev_get_drvdata(dev);
const struct amd_pm_funcs *pp_funcs = adev->powerplay.pp_funcs;
- int limit_type = to_sensor_dev_attr(attr)->index;
- uint32_t limit = limit_type << 24;
+ enum pp_power_sample_window sample_window = to_sensor_dev_attr(attr)->index;
+ uint32_t limit;
uint32_t max_limit = 0;
ssize_t size;
int r;
@@ -2735,7 +2735,7 @@ static ssize_t amdgpu_hwmon_show_power_cap_max(struct device *dev,
}
if (is_support_sw_smu(adev)) {
- smu_get_power_limit(&adev->smu, &limit, SMU_PPT_LIMIT_MAX);
+ smu_get_power_limit(&adev->smu, &limit, PP_PWR_LIMIT_MAX,
+sample_window);
size = snprintf(buf, PAGE_SIZE, "%u\n", limit * 1000000);
} else if (pp_funcs && pp_funcs->get_power_limit) {
pp_funcs->get_power_limit(adev->powerplay.pp_handle,
@@ -2757,8 +2757,8 @@ static ssize_t amdgpu_hwmon_show_power_cap(struct device *dev, {
struct amdgpu_device *adev = dev_get_drvdata(dev);
const struct amd_pm_funcs *pp_funcs = adev->powerplay.pp_funcs;
- int limit_type = to_sensor_dev_attr(attr)->index;
- uint32_t limit = limit_type << 24;
+ enum pp_power_sample_window sample_window = to_sensor_dev_attr(attr)->index;
+ uint32_t limit;
ssize_t size;
int r;
@@ -2774,7 +2774,7 @@ static ssize_t amdgpu_hwmon_show_power_cap(struct device *dev,
}
if (is_support_sw_smu(adev)) {
- smu_get_power_limit(&adev->smu, &limit, SMU_PPT_LIMIT_CURRENT);
+ smu_get_power_limit(&adev->smu, &limit, PP_PWR_LIMIT_CURRENT,
+sample_window);
size = snprintf(buf, PAGE_SIZE, "%u\n", limit * 1000000);
} else if (pp_funcs && pp_funcs->get_power_limit) {
pp_funcs->get_power_limit(adev->powerplay.pp_handle,
@@ -2796,8 +2796,8 @@ static ssize_t amdgpu_hwmon_show_power_cap_default(struct device *dev, {
struct amdgpu_device *adev = dev_get_drvdata(dev);
const struct amd_pm_funcs *pp_funcs = adev->powerplay.pp_funcs;
- int limit_type = to_sensor_dev_attr(attr)->index;
- uint32_t limit = limit_type << 24;
+ enum pp_power_sample_window sample_window = to_sensor_dev_attr(attr)->index;
+ uint32_t limit;
ssize_t size;
int r;
@@ -2813,7 +2813,7 @@ static ssize_t amdgpu_hwmon_show_power_cap_default(struct device *dev,
}
if (is_support_sw_smu(adev)) {
- smu_get_power_limit(&adev->smu, &limit, SMU_PPT_LIMIT_DEFAULT);
+ smu_get_power_limit(&adev->smu, &limit, PP_PWR_LIMIT_DEFAULT,
+sample_window);
size = snprintf(buf, PAGE_SIZE, "%u\n", limit * 1000000);
} else if (pp_funcs && pp_funcs->get_power_limit) {
pp_funcs->get_power_limit(adev->powerplay.pp_handle,
diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
index 523f9d2982e9..b97b960c2eac 100644
--- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
+++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
@@ -1262,7 +1262,8 @@ enum smu_cmn2asic_mapping_type { #if !defined(SWSMU_CODE_LAYER_L2) && !defined(SWSMU_CODE_LAYER_L3) && !defined(SWSMU_CODE_LAYER_L4) int smu_get_power_limit(struct smu_context *smu,
uint32_t *limit,
- enum smu_ppt_limit_level limit_level);
+ enum pp_power_limit_level pp_limit_level,
+ enum pp_power_sample_window sample_window);
bool smu_mode1_reset_is_support(struct smu_context *smu); bool smu_mode2_reset_is_support(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 8aff67a667fa..44c1baa2748d 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
@@ -2168,14 +2168,44 @@ static int smu_set_fan_speed_rpm(void *handle, uint32_t speed)
int smu_get_power_limit(struct smu_context *smu,
uint32_t *limit,
- enum smu_ppt_limit_level limit_level)
+ enum pp_power_limit_level pp_limit_level,
+ enum pp_power_sample_window sample_window)
{
- uint32_t limit_type = *limit >> 24;
+ enum smu_ppt_limit_level limit_level;
+ uint32_t limit_type;
int ret = 0;
if (!smu->pm_enabled || !smu->adev->pm.dpm_enabled)
return -EOPNOTSUPP;
+ switch(sample_window) {
+ case PP_PWR_WINDOW_DEFAULT:
+ limit_type = SMU_DEFAULT_PPT_LIMIT;
+ break;
+ case PP_PWR_WINDOW_FAST:
+ limit_type = SMU_FAST_PPT_LIMIT;
+ break;
+ default:
+ return -EOPNOTSUPP;
+ break;
+ }
+
+ switch(pp_limit_level){
+ case PP_PWR_LIMIT_CURRENT:
+ limit_level = SMU_PPT_LIMIT_CURRENT;
+ break;
+ case PP_PWR_LIMIT_DEFAULT:
+ limit_level = SMU_PPT_LIMIT_DEFAULT;
+ break;
+ case PP_PWR_LIMIT_MAX:
+ limit_level = SMU_PPT_LIMIT_MAX;
+ break;
+ case PP_PWR_LIMIT_MIN:
+ default:
+ return -EOPNOTSUPP;
+ break;
+ }
+
mutex_lock(&smu->mutex);
if (limit_type != SMU_DEFAULT_PPT_LIMIT) {
--
2.25.1
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20210603/6ee495e0/attachment-0001.htm>
More information about the amd-gfx
mailing list