Re: [PATCH v2 1/5] cpufreq/amd-pstate: Add dynamic energy performance preference

[Mario Limonciello]

On 3/19/2025 11:50, Mario Limonciello wrote:
> On 3/18/2025 22:43, Dhananjay Ugwekar wrote:
> > On 3/19/2025 1:06 AM, Mario Limonciello wrote:
> > > On 3/12/2025 07:16, Dhananjay Ugwekar wrote:
> > > > On 3/4/2025 8:53 PM, Mario Limonciello wrote:
> > > > > From: Mario Limonciello <mario.limonciello@xxxxxxx>
> > > > >
> > > > > Dynamic energy performance preference will change the EPP profile
> > > > > based on whether the machine is running on AC or DC power.
> > > > >
> > > > > A notification chain from the power supply core is used to adjust
> > > > > EPP values on plug in or plug out events.
> > > > >
> > > > > For non-server systems:
> > > > >       * the default EPP for AC mode is `performance`.
> > > > >       * the default EPP for DC mode is `balance_performance`.
> > > > >
> > > > > Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> > > > > ---
> > > > > v1->v2:
> > > > >    * Change defaults to performance (AC) and balance_performance (DC)
> > > > >    * Default Kconfig to disabled for now
> > > > >    * Rebase on latest branch
> > > > > ---
> > > > >    Documentation/admin-guide/pm/amd-pstate.rst |  18 ++-
> > > > >    drivers/cpufreq/Kconfig.x86                 |  12 ++
> > > > >    drivers/cpufreq/amd-pstate.c                | 129 ++++++++++++++ 
> > > > > ++++--
> > > > >    drivers/cpufreq/amd-pstate.h                |   5 +-
> > > > >    4 files changed, 155 insertions(+), 9 deletions(-)
> > > > [Snip]
> > > > > @@ -1556,6 +1667,10 @@ static int amd_pstate_epp_set_policy(struct 
> > > > > cpufreq_policy *policy)
> > > > >        if (!policy->cpuinfo.max_freq)
> > > > >            return -ENODEV;
> > > > >    +    /* policy can't be changed to performance policy while 
> > > > > dynamic epp is enabled */
> > > > > +    if (policy->policy == CPUFREQ_POLICY_PERFORMANCE && cpudata- 
> > > > > >dynamic_epp)
> > > > > +        return -EBUSY;
> > > >
> > > > We might need to tweak this condition, because if we enable 
> > > > "CONFIG_X86_AMD_PSTATE_DYNAMIC_EPP" in config
> > > > and boot with "amd_pstate=active" it lands here (cpufreq_online()- 
> > > > >amd_pstate_epp_set_policy()) driver init fails
> > > > as the default governor is performance.
> > >
> > > The check is important to make sure that you can't go to performance 
> > > mode after init.
> > >
> > > I think this is the way I would want to solve it.
> > > Set policy to powersave before enabling dynamic epp for 
> > > amd_pstate_epp_cpu_init().
> > >
> > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> > > index 824756ac0010e..4a0f561d0e2d1 100644
> > > --- a/drivers/cpufreq/amd-pstate.c
> > > +++ b/drivers/cpufreq/amd-pstate.c
> > > @@ -1729,8 +1729,10 @@ static int amd_pstate_epp_cpu_init(struct 
> > > cpufreq_policy *policy)
> > >                  WRITE_ONCE(cpudata->cppc_req_cached, value);
> > >          }
> > >
> > > -       if (dynamic_epp)
> > > +       if (dynamic_epp) {
> > > +               policy->policy = CPUFREQ_POLICY_POWERSAVE;
> > >                  ret = amd_pstate_set_dynamic_epp(policy);
> > > +       }
> > >          else
> > >                  ret = amd_pstate_set_epp(policy, 
> > > amd_pstate_get_balanced_epp(policy));
> >             ^^^^^^^^^^^^ (mentioned below)
> > >          if (ret)
> > >
> > > Thoughts?
> >
> > Yes, this looks good, because anyway there is no point in having 
> > performance governor and dynamic
> > epp set at the same time.
> >
> > I found one related quirk though, we are setting performance governor 
> > for server platforms in
> > amd_pstate_epp_cpu_init() and then setting epp at the line highlighted 
> > above. We dont have a
> > check in *_set_epp() functions for performance governor. This could 
> > alter the performance governor
> > behavior if we set a "balanced" epp for it. I haven't tested this part 
> > yet.
>
> In that case we probably want the "default" ACPI platform profile to be 
> "performance" when on a server instead of balanced.

Actually this is already done:

		cpudata->current_profile = PLATFORM_PROFILE_PERFORMANCE;

I'll post an updated patch series for you to test.

> > Thanks,
> > Dhananjay
> >
> > > > > +
> > > > >        cpudata->policy = policy->policy;
> > > > >          ret = amd_pstate_epp_update_limit(policy);
> > > > > diff --git a/drivers/cpufreq/amd-pstate.h b/drivers/cpufreq/amd- 
> > > > > pstate.h
> > > > > index fbe1c08d3f061..6882876f895de 100644
> > > > > --- a/drivers/cpufreq/amd-pstate.h
> > > > > +++ b/drivers/cpufreq/amd-pstate.h
> > > > > @@ -104,7 +104,10 @@ struct amd_cpudata {
> > > > >        /* EPP feature related attributes*/
> > > > >        u32    policy;
> > > > >        bool    suspended;
> > > > > -    u8    epp_default;
> > > > > +    u8    epp_default_ac;
> > > > > +    u8    epp_default_dc;
> > > > > +    bool    dynamic_epp;
> > > > > +    struct notifier_block power_nb;
> > > > >    };
> > > > >      /*