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

[Marco Scardovi <scardracs@disroot.org>]

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