RE: [RFC 2/2] drm/amd/pm: Add support for reacting to platform profile notification

["Limonciello, Mario" <Mario.Limonciello@amd.com>]

[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@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
> >
> > [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@lists.freedesktop.org
> > <mailto:amd-gfx-bounces@lists.freedesktop.org>> on behalf of Alex
> > Deucher <alexdeucher@gmail.com <mailto:alexdeucher@gmail.com>>
> > *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@amd.com <mailto:mario.limonciello@amd.com>> 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@amd.com
> > <mailto:mario.limonciello@amd.com>>
> >  > ---
> >  >  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
> >  >
> >