Re: [PATCH v2 1/2] cpufreq/amd-pstate: Fix EPP initialization for shared memory systems

[Mario Limonciello]

On 6/4/26 01:56, Marco Scardovi wrote:
> In data giovedì 4 giugno 2026 05:56:20 Ora legale dell’Europa centrale, K
> Prateek Nayak ha scritto:
> > Hello Mario,
> >
> > On 6/4/2026 12:23 AM, Mario Limonciello wrote:
> > > > --- a/drivers/cpufreq/amd-pstate.h
> > > > +++ b/drivers/cpufreq/amd-pstate.h
> > > > @@ -128,6 +128,8 @@ struct amd_cpudata {
> > > >        u8    epp_default_dc;
> > > >        bool    dynamic_epp;
> > > >        bool    raw_epp;
> > > > +    /* Indicates that EPP has been successfully programmed at least once
> > > > since boot. */ +    bool    epp_hw_programmed;
> > > >        struct notifier_block power_nb;
> > > >          /* platform profile */
> > >
> > > I'm having a hard time following why this is needed.  Let me explain my
> > > chain of thought.
> > >
> > > We start at amd_pstate_epp_cpu_init().
> > > * cpudata (and thus cppc_req_cached) is initalized to 0 via kzalloc()
> > >     - On a server we initialize "epp_default" via a lookup to the HW
> > > (amd_pstate_get_epp). - On a non-server we initialize "epp_default" to
> > > 0x80.
> > > * we call amd_pstate_set_epp() with epp_deafult as the argument.
> > >
> > > Now in the shared mem backend (shmem_set_epp) we do a lookup of epp_cached
> > > and it should be 0.  We do a comparison of the argument sent to the
> > > function, and this should be 0x80.
> > >
> > > So how do we get into a case that amd_pstate_set_epp() is actually called
> > > with 0?
> > This was the path I traced yesterday considering amd_dynamic_epp enabled for
> > a shared memory system that is plugged into the wall:
> >
> >    amd_pstate_epp_cpu_init()
> >      cpudata->epp_default_ac = AMD_CPPC_EPP_PERFORMANCE;
> >      cpudata->epp_default_dc = AMD_CPPC_EPP_BALANCE_PERFORMANCE;
> >      cpudata->current_profile = PLATFORM_PROFILE_BALANCED;
> >
> >      /* cpudata->cppc_req_cached has epp_cached as 0 */
> >
> >      amd_pstate_set_dynamic_epp(policy)
> >        epp = amd_pstate_get_balanced_epp(policy); /* returns
> > cpudata->epp_default_ac which is EPP_PERFORMANCE */
> >
> >        amd_pstate_set_epp(policy, epp /* AMD_CPPC_EPP_PERFORMANCE */)
> >          if (epp == epp_cached)
> >            return;
> >
> >          /*
> >           * Skips cppc_set_epp_perf(cpudata->cpu, &perf_ctrls, 1)
> >           * where the last argument "enable" enables EPP via the
> >           * AUTO_SEL_ENABLE path.
> >           */
> >
> > > There are some trace points specifically for this purpose - you could
> > > confirm there really is such a call by using them at bootup (or by
> > > starting in passive and with a mode change to active at runtime).
> > It should show up in the traces yes!
> >
> > > Then we call amd_pstate_set_epp() with that epp_default value.
> >
> > I think this patch has some merit, although the second patch, I'm not
> > too sure of. After some digging, it seems we always had AUTO_SEL_ENABLE
> > and ENERGY_PERF right from the time CPPC was introduced (including Zen1
> > systems that had CPPC support).
> >
> > Marco, do you have actually have a system that neither has
> > AUTO_SEL_ENABLE, nor ENERGY_PERF? Seems very unlikely a configuration
> > like that made it out.
>
> Hi Prateek, Mario,
>
> No, I don't have a system lacking AUTO_SEL_ENABLE or ENERGY_PERF support.
> The only AMD system I currently have access to is an 8940HX.
>
> I think I understand the reasoning behind both of your comments. My
> understanding is that the main question is whether there is a real boot path
> where amd_pstate_set_epp() returns early before EPP has ever been programmed
> in hardware, which would justify tracking that state separately from the
> cached EPP value.
>
> However, I'm not familiar enough with this part of the driver to confidently
> turn that reasoning into a correct implementation, so I'd prefer to defer to
> your judgement on whether the additional state tracking is the right approach.
>
> Thanks,
> Marco

So at least for patch 1 if you run dynamic epp, and are plugged into a 
wall you start out on EPP 0, but I'm pretty sure that's usually the 
firmware default anyway.  But this gives me a (better?) idea.  How about 
we just read the EPP from the firmware initially at startup?

Something like this:

╰─❯ git diff
diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 3f06e33f47120..0e32f7f92651b 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -1939,6 +1939,8 @@ static int amd_pstate_epp_cpu_init(struct 
cpufreq_policy *policy)

        policy->boost_supported = READ_ONCE(cpudata->boost_supported);

+       WRITE_ONCE(cpudata->cppc_req_cached, 
FIELD_PREP(AMD_CPPC_EPP_PERF_MASK, amd_pstate_get_epp(cpudata)));
+
        /*
         * Set the policy to provide a valid fallback value in case
         * the default cpufreq governor is neither powersave nor 
performance.