<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"> <head> <meta http-equiv="Content-Type" content="text/html; charset=us-ascii"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style></style> </head> <body lang="EN-US" link="#0563C1" vlink="#954F72" style="word-wrap:break-word"> <div class="WordSection1"> [Public]<o:p></o:p> <o:p> </o:p> > I don't think we want to force the performance level. This interface forces various fixed clock configurations for debugging and profiling.<o:p></o:p> Ah got it.<o:p></o:p> <o:p> </o:p> >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.<o:p></o:p> <o:p> </o:p> Which profile mapping you think make sense?<o:p></o:p> My guess would be:<o:p></o:p> “BOOTUP_DEFAULT” for balanced<o:p></o:p> “POWER_SAVING” for low-power<o:p></o:p> “3D_FULL_SCREEN” for performance<o:p></o:p> <o:p> </o:p> Since recently we removed that interface for YC, and some earlier APUs don’t do as much with it.<o:p></o:p> So I wonder if this is only really valuable to do this callback for !APU. <o:p></o:p> <o:p> </o:p> > 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.<o:p></o:p> <o:p> </o:p> Even if changing the heuristic for workload as Alex suggested?<o:p></o:p> <o:p> </o:p> > Also, not sure how to handle a case like, say a laptop with Intel CPU and AMD dgpu.<o:p></o:p> <o:p> </o:p> I was actually thinking this approach where there are platform profile callbacks is best because of that specifically.<o:p></o:p> It would allow an Intel CPU system to have a platform profile driver implemented by the OEM, but then still notify amdgpu dGPU to tune for power saving or performance workload.<o:p></o:p> <o:p> </o:p> <div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt"> <div> <div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"> From: Lazar, Lijo <Lijo.Lazar@amd.com> Sent: Wednesday, November 10, 2021 10:05 To: Alex Deucher <alexdeucher@gmail.com>; Limonciello, Mario <Mario.Limonciello@amd.com> Cc: amd-gfx list <amd-gfx@lists.freedesktop.org> Subject: Re: [RFC 2/2] drm/amd/pm: Add support for reacting to platform profile notification<o:p></o:p> </div> </div> <o:p> </o:p> [Public]<o:p></o:p> <o:p> </o:p> <div> <div> 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.<o:p></o:p> </div> <div> <o:p> </o:p> </div> <div> Also, not sure how to handle a case like, say a laptop with Intel CPU and AMD dgpu.<o:p></o:p> </div> <div id="ms-outlook-mobile-signature"> <div> <o:p> </o:p> </div> Thanks, Lijo<o:p></o:p> </div> <div id="id-f61421a9-d54c-4f81-aa61-517d64763a14"> <div> <o:p> </o:p> </div> <div class="MsoNormal" align="center" style="text-align:center"> <hr size="2" width="98%" align="center"> </div> <div id="divRplyFwdMsg"> From: amd-gfx <<a href="mailto:amd-gfx-bounces@lists.freedesktop.org">amd-gfx-bounces@lists.freedesktop.org</a>> on behalf of Alex Deucher <<a href="mailto:alexdeucher@gmail.com">alexdeucher@gmail.com</a>> 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<o:p></o:p> </div> <o:p> </o:p> <div> On Wed, Nov 10, 2021 at 1:24 AM Mario Limonciello <<a href="mailto:mario.limonciello@amd.com">mario.limonciello@amd.com</a>> 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 <<a href="mailto:mario.limonciello@amd.com">mario.limonciello@amd.com</a>> > --- > 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 ><o:p></o:p> </div> <o:p> </o:p> </div> </div> </div> </div> </body> </html>