[Public]
I feel it's better to leave to platform vendors. For ex: for APU cases they may have implementations in which their BIOSes talk to PMFW and this might be driving something else here.
Also, not sure how to handle a case like, say a laptop with Intel CPU and AMD dgpu.
Lijo
From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> on behalf of Alex Deucher <alexdeucher@xxxxxxxxx>
Sent: Wednesday, 10 November 2021, 8:44 pm
To: Limonciello, Mario
Cc: amd-gfx list
Subject: Re: [RFC 2/2] drm/amd/pm: Add support for reacting to platform profile notification
Sent: Wednesday, 10 November 2021, 8:44 pm
To: Limonciello, Mario
Cc: amd-gfx list
Subject: Re: [RFC 2/2] drm/amd/pm: Add support for reacting to platform profile notification
On Wed, Nov 10, 2021 at 1:24 AM Mario Limonciello
<mario.limonciello@xxxxxxx> wrote:
>
> Various drivers provide platform profile support to let users set a hint
> in their GUI whether they want to run in a high performance, low battery
> life or balanced configuration.
>
> Drivers that provide this typically work with the firmware on their system
> to configure hardware. In the case of AMDGPU however, the notification
> path doesn't come through firmware and can instead be provided directly
> to the driver from a notification chain.
>
> Use the information of the newly selected profile to tweak
> `dpm_force_performance_level` to that profile IFF the user hasn't manually
> selected `manual` or any other `profile_*` options.
I don't think we want to force the performance level. This interface
forces various fixed clock configurations for debugging and profiling.
I think what we'd want to select here is the power profile (see
amdgpu_set_pp_power_profile_mode()). For this interface you can
select various profiles (BOOTUP_DEFAULT, 3D_FULL_SCREEN, POWER_SAVING,
VIDEO, VR, COMPUTE, etc.). These still use dynamic power management,
but they adjust the heuristics used by the GPU to select power states
so the GPU performance ramps up/down more or less aggressively.
Alex
>
> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 3 +
> drivers/gpu/drm/amd/pm/amdgpu_pm.c | 105 +++++++++++++++++++++++-----
> 2 files changed, 90 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index b85b67a88a3d..27b0be23b6ac 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1097,6 +1097,9 @@ struct amdgpu_device {
>
> struct amdgpu_reset_control *reset_cntl;
> uint32_t ip_versions[HW_ID_MAX][HWIP_MAX_INSTANCE];
> +
> + /* platform profile notifications */
> + struct notifier_block platform_profile_notifier;
> };
>
> static inline struct amdgpu_device *drm_to_adev(struct drm_device *ddev)
> diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> index 41472ed99253..33fc52b90d4c 100644
> --- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> +++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> @@ -32,6 +32,7 @@
> #include <linux/hwmon.h>
> #include <linux/hwmon-sysfs.h>
> #include <linux/nospec.h>
> +#include <linux/platform_profile.h>
> #include <linux/pm_runtime.h>
> #include <asm/processor.h>
> #include "hwmgr.h"
> @@ -200,6 +201,33 @@ static ssize_t amdgpu_set_power_dpm_state(struct device *dev,
> return count;
> }
>
> +static int amdgpu_get_forced_level(struct device *dev, enum amd_dpm_forced_level *level)
> +{
> + struct drm_device *ddev = dev_get_drvdata(dev);
> + struct amdgpu_device *adev = drm_to_adev(ddev);
> + int ret;
> +
> + if (amdgpu_in_reset(adev))
> + return -EPERM;
> + if (adev->in_suspend && !adev->in_runpm)
> + return -EPERM;
> +
> + ret = pm_runtime_get_sync(ddev->dev);
> + if (ret < 0) {
> + pm_runtime_put_autosuspend(ddev->dev);
> + return ret;
> + }
> +
> + if (adev->powerplay.pp_funcs->get_performance_level)
> + *level = amdgpu_dpm_get_performance_level(adev);
> + else
> + *level = adev->pm.dpm.forced_level;
> +
> + pm_runtime_mark_last_busy(ddev->dev);
> + pm_runtime_put_autosuspend(ddev->dev);
> +
> + return 0;
> +}
>
> /**
> * DOC: power_dpm_force_performance_level
> @@ -264,29 +292,13 @@ static ssize_t amdgpu_get_power_dpm_force_performance_level(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> {
> - struct drm_device *ddev = dev_get_drvdata(dev);
> - struct amdgpu_device *adev = drm_to_adev(ddev);
> enum amd_dpm_forced_level level = 0xff;
> int ret;
>
> - if (amdgpu_in_reset(adev))
> - return -EPERM;
> - if (adev->in_suspend && !adev->in_runpm)
> - return -EPERM;
> + ret = amdgpu_get_forced_level(dev, &level);
>
> - ret = pm_runtime_get_sync(ddev->dev);
> - if (ret < 0) {
> - pm_runtime_put_autosuspend(ddev->dev);
> + if (ret < 0)
> return ret;
> - }
> -
> - if (adev->powerplay.pp_funcs->get_performance_level)
> - level = amdgpu_dpm_get_performance_level(adev);
> - else
> - level = adev->pm.dpm.forced_level;
> -
> - pm_runtime_mark_last_busy(ddev->dev);
> - pm_runtime_put_autosuspend(ddev->dev);
>
> return sysfs_emit(buf, "%s\n",
> (level == AMD_DPM_FORCED_LEVEL_AUTO) ? "auto" :
> @@ -405,6 +417,59 @@ static ssize_t amdgpu_set_power_dpm_force_performance_level(struct device *dev,
> return count;
> }
>
> +static void amdgpu_update_profile(struct device *dev, enum platform_profile_option *profile)
> +{
> + enum amd_dpm_forced_level level;
> + const char *str;
> + int ret;
> +
> + ret = amdgpu_get_forced_level(dev, &level);
> + if (ret < 0)
> + return;
> +
> + /* only update profile if we're in fixed modes right now that need updating */
> + switch (level) {
> + case AMD_DPM_FORCED_LEVEL_LOW:
> + if (*profile < PLATFORM_PROFILE_BALANCED)
> + return;
> + break;
> + case AMD_DPM_FORCED_LEVEL_HIGH:
> + if (*profile > PLATFORM_PROFILE_BALANCED)
> + return;
> + break;
> + case AMD_DPM_FORCED_LEVEL_AUTO:
> + if (*profile == PLATFORM_PROFILE_BALANCED)
> + return;
> + break;
> + default:
> + dev_dbg(dev, "refusing to update amdgpu profile from %d\n", level);
> + return;
> + }
> + if (*profile > PLATFORM_PROFILE_BALANCED)
> + str = "high";
> + else if (*profile < PLATFORM_PROFILE_BALANCED)
> + str = "low";
> + else
> + str = "auto";
> +
> + dev_dbg(dev, "updating platform profile to %s\n", str);
> + amdgpu_set_power_dpm_force_performance_level(dev, NULL, str, 0);
> +}
> +
> +static int amdgpu_platform_profile_notifier_call(struct notifier_block *nb,
> + unsigned long action, void *data)
> +{
> + if (action == PLATFORM_PROFILE_CHANGED) {
> + enum platform_profile_option *profile = ""> > + struct amdgpu_device *adev;
> +
> + adev = container_of(nb, struct amdgpu_device, platform_profile_notifier);
> + amdgpu_update_profile(adev->dev, profile);
> + }
> +
> + return NOTIFY_OK;
> +}
> +
> static ssize_t amdgpu_get_pp_num_states(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> @@ -3506,6 +3571,9 @@ int amdgpu_pm_sysfs_init(struct amdgpu_device *adev)
> if (ret)
> return ret;
>
> + adev->platform_profile_notifier.notifier_call = amdgpu_platform_profile_notifier_call;
> + platform_profile_register_notifier(&adev->platform_profile_notifier);
> +
> adev->pm.sysfs_initialized = true;
>
> return 0;
> @@ -3519,6 +3587,7 @@ void amdgpu_pm_sysfs_fini(struct amdgpu_device *adev)
> if (adev->pm.int_hwmon_dev)
> hwmon_device_unregister(adev->pm.int_hwmon_dev);
>
> + platform_profile_unregister_notifier(&adev->platform_profile_notifier);
> amdgpu_device_attr_remove_groups(adev, &adev->pm.pm_attr_list);
> }
>
> --
> 2.25.1
>
<mario.limonciello@xxxxxxx> wrote:
>
> Various drivers provide platform profile support to let users set a hint
> in their GUI whether they want to run in a high performance, low battery
> life or balanced configuration.
>
> Drivers that provide this typically work with the firmware on their system
> to configure hardware. In the case of AMDGPU however, the notification
> path doesn't come through firmware and can instead be provided directly
> to the driver from a notification chain.
>
> Use the information of the newly selected profile to tweak
> `dpm_force_performance_level` to that profile IFF the user hasn't manually
> selected `manual` or any other `profile_*` options.
I don't think we want to force the performance level. This interface
forces various fixed clock configurations for debugging and profiling.
I think what we'd want to select here is the power profile (see
amdgpu_set_pp_power_profile_mode()). For this interface you can
select various profiles (BOOTUP_DEFAULT, 3D_FULL_SCREEN, POWER_SAVING,
VIDEO, VR, COMPUTE, etc.). These still use dynamic power management,
but they adjust the heuristics used by the GPU to select power states
so the GPU performance ramps up/down more or less aggressively.
Alex
>
> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 3 +
> drivers/gpu/drm/amd/pm/amdgpu_pm.c | 105 +++++++++++++++++++++++-----
> 2 files changed, 90 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index b85b67a88a3d..27b0be23b6ac 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1097,6 +1097,9 @@ struct amdgpu_device {
>
> struct amdgpu_reset_control *reset_cntl;
> uint32_t ip_versions[HW_ID_MAX][HWIP_MAX_INSTANCE];
> +
> + /* platform profile notifications */
> + struct notifier_block platform_profile_notifier;
> };
>
> static inline struct amdgpu_device *drm_to_adev(struct drm_device *ddev)
> diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> index 41472ed99253..33fc52b90d4c 100644
> --- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> +++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> @@ -32,6 +32,7 @@
> #include <linux/hwmon.h>
> #include <linux/hwmon-sysfs.h>
> #include <linux/nospec.h>
> +#include <linux/platform_profile.h>
> #include <linux/pm_runtime.h>
> #include <asm/processor.h>
> #include "hwmgr.h"
> @@ -200,6 +201,33 @@ static ssize_t amdgpu_set_power_dpm_state(struct device *dev,
> return count;
> }
>
> +static int amdgpu_get_forced_level(struct device *dev, enum amd_dpm_forced_level *level)
> +{
> + struct drm_device *ddev = dev_get_drvdata(dev);
> + struct amdgpu_device *adev = drm_to_adev(ddev);
> + int ret;
> +
> + if (amdgpu_in_reset(adev))
> + return -EPERM;
> + if (adev->in_suspend && !adev->in_runpm)
> + return -EPERM;
> +
> + ret = pm_runtime_get_sync(ddev->dev);
> + if (ret < 0) {
> + pm_runtime_put_autosuspend(ddev->dev);
> + return ret;
> + }
> +
> + if (adev->powerplay.pp_funcs->get_performance_level)
> + *level = amdgpu_dpm_get_performance_level(adev);
> + else
> + *level = adev->pm.dpm.forced_level;
> +
> + pm_runtime_mark_last_busy(ddev->dev);
> + pm_runtime_put_autosuspend(ddev->dev);
> +
> + return 0;
> +}
>
> /**
> * DOC: power_dpm_force_performance_level
> @@ -264,29 +292,13 @@ static ssize_t amdgpu_get_power_dpm_force_performance_level(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> {
> - struct drm_device *ddev = dev_get_drvdata(dev);
> - struct amdgpu_device *adev = drm_to_adev(ddev);
> enum amd_dpm_forced_level level = 0xff;
> int ret;
>
> - if (amdgpu_in_reset(adev))
> - return -EPERM;
> - if (adev->in_suspend && !adev->in_runpm)
> - return -EPERM;
> + ret = amdgpu_get_forced_level(dev, &level);
>
> - ret = pm_runtime_get_sync(ddev->dev);
> - if (ret < 0) {
> - pm_runtime_put_autosuspend(ddev->dev);
> + if (ret < 0)
> return ret;
> - }
> -
> - if (adev->powerplay.pp_funcs->get_performance_level)
> - level = amdgpu_dpm_get_performance_level(adev);
> - else
> - level = adev->pm.dpm.forced_level;
> -
> - pm_runtime_mark_last_busy(ddev->dev);
> - pm_runtime_put_autosuspend(ddev->dev);
>
> return sysfs_emit(buf, "%s\n",
> (level == AMD_DPM_FORCED_LEVEL_AUTO) ? "auto" :
> @@ -405,6 +417,59 @@ static ssize_t amdgpu_set_power_dpm_force_performance_level(struct device *dev,
> return count;
> }
>
> +static void amdgpu_update_profile(struct device *dev, enum platform_profile_option *profile)
> +{
> + enum amd_dpm_forced_level level;
> + const char *str;
> + int ret;
> +
> + ret = amdgpu_get_forced_level(dev, &level);
> + if (ret < 0)
> + return;
> +
> + /* only update profile if we're in fixed modes right now that need updating */
> + switch (level) {
> + case AMD_DPM_FORCED_LEVEL_LOW:
> + if (*profile < PLATFORM_PROFILE_BALANCED)
> + return;
> + break;
> + case AMD_DPM_FORCED_LEVEL_HIGH:
> + if (*profile > PLATFORM_PROFILE_BALANCED)
> + return;
> + break;
> + case AMD_DPM_FORCED_LEVEL_AUTO:
> + if (*profile == PLATFORM_PROFILE_BALANCED)
> + return;
> + break;
> + default:
> + dev_dbg(dev, "refusing to update amdgpu profile from %d\n", level);
> + return;
> + }
> + if (*profile > PLATFORM_PROFILE_BALANCED)
> + str = "high";
> + else if (*profile < PLATFORM_PROFILE_BALANCED)
> + str = "low";
> + else
> + str = "auto";
> +
> + dev_dbg(dev, "updating platform profile to %s\n", str);
> + amdgpu_set_power_dpm_force_performance_level(dev, NULL, str, 0);
> +}
> +
> +static int amdgpu_platform_profile_notifier_call(struct notifier_block *nb,
> + unsigned long action, void *data)
> +{
> + if (action == PLATFORM_PROFILE_CHANGED) {
> + enum platform_profile_option *profile = ""> > + struct amdgpu_device *adev;
> +
> + adev = container_of(nb, struct amdgpu_device, platform_profile_notifier);
> + amdgpu_update_profile(adev->dev, profile);
> + }
> +
> + return NOTIFY_OK;
> +}
> +
> static ssize_t amdgpu_get_pp_num_states(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> @@ -3506,6 +3571,9 @@ int amdgpu_pm_sysfs_init(struct amdgpu_device *adev)
> if (ret)
> return ret;
>
> + adev->platform_profile_notifier.notifier_call = amdgpu_platform_profile_notifier_call;
> + platform_profile_register_notifier(&adev->platform_profile_notifier);
> +
> adev->pm.sysfs_initialized = true;
>
> return 0;
> @@ -3519,6 +3587,7 @@ void amdgpu_pm_sysfs_fini(struct amdgpu_device *adev)
> if (adev->pm.int_hwmon_dev)
> hwmon_device_unregister(adev->pm.int_hwmon_dev);
>
> + platform_profile_unregister_notifier(&adev->platform_profile_notifier);
> amdgpu_device_attr_remove_groups(adev, &adev->pm.pm_attr_list);
> }
>
> --
> 2.25.1
>
