[AMD Official Use Only] > > > > Even if changing the heuristic for workload as Alex suggested? > > > > Yes. I think this is meant to be BIOS driven for APU platforms and AMD > APU + AMD dGPU with smartshift. > So then it sounds like if any - it would make sense only on dGPU that isn't using smart shift. > > After seeing that this is coming under ACPI, I thought the intention is > to have this driven by firmware primarily. The purpose of platform > driver itself could be to optimize for those power profiles and while > doing that it should have considered all the components in the whole > platform (assuming platform driver testing covers the behavior of these > modes on a particular platform). > > I am not sure if it's appropriate for another driver to plug-in to this > automatically and tinker an 'expected-to-be-well-tuned' setting by the > platform driver. The modes selected by another driver may or may not > match with the conditions assumed by platform driver - for ex: some > profile could mean fans running quieter with EC control and then the > profile chosen by another driver could disturb that intended setting. > The key here is that any non-APU program won't have a path to influence dGPU behavior via FW or smartshift will it? So it could be useful to respond to a limited subset of hints that can be guaranteed to mean the same. Like maybe low power mode and balanced only, but don't make changes going to high power mode? > Thanks, > Lijo > > > *From:* Lazar, Lijo <Lijo.Lazar@xxxxxxx> > > *Sent:* Wednesday, November 10, 2021 10:05 > > *To:* Alex Deucher <alexdeucher@xxxxxxxxx>; Limonciello, Mario > > <Mario.Limonciello@xxxxxxx> > > *Cc:* amd-gfx list <amd-gfx@xxxxxxxxxxxxxxxxxxxxx> > > *Subject:* Re: [RFC 2/2] drm/amd/pm: Add support for reacting to > > platform profile notification > > > > [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. > > > > Thanks, > > Lijo > > > > ------------------------------------------------------------------------ > > > > *From:* amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx > > <mailto:amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx>> on behalf of Alex > > Deucher <alexdeucher@xxxxxxxxx <mailto: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 > > > > On Wed, Nov 10, 2021 at 1:24 AM Mario Limonciello > > <mario.limonciello@xxxxxxx <mailto: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 > > <mailto: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 = data; > > > + 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 > > > > >
