Re: [PATCH v2 3/5] cpufreq/amd-pstate: Add support for platform profile class

[Gautham R. Shenoy]

[...snip...]

On Fri, Mar 07, 2025 at 10:30:25PM -0600, Mario Limonciello wrote:
> On 3/7/2025 10:55, Mario Limonciello wrote:
> > On 3/7/2025 10:22, Gautham R. Shenoy wrote:
> > > > +static int amd_pstate_profile_set(struct device *dev,
> > > > +                  enum platform_profile_option profile)
> > > > +{
> > > > +    struct amd_cpudata *cpudata = dev_get_drvdata(dev);
> > > > +    struct cpufreq_policy *policy __free(put_cpufreq_policy) =
> > > > cpufreq_cpu_get(cpudata->cpu);
> > > > +    int ret;
> > > > +
> > > > +    switch (profile) {
> > > > +    case PLATFORM_PROFILE_LOW_POWER:
> > > > +        if (cpudata->policy != CPUFREQ_POLICY_POWERSAVE)
> > > > +            cpudata->policy = CPUFREQ_POLICY_POWERSAVE;
> > > 
> > > So prior to the patch, cpudata->policy is supposed to mirror
> > > policy->policy.  With this patch, this assumption is no longer
> > > true. So it is possible for the user to again override the choice of
> > > EPP set via platform profile by changing the cpufreq governor ?
> > > 
> > > Is this the expected behaviour?
> > > 
> > > The bigger concern is, if the governor was previously "performance"
> > > and then the platform profile requested "low power", "cat
> > > /sys/devices/system/cpu/cpuX/cpufreq/scaling_governor" would still
> > > show "performance", which is inconsistent with the behaviour.
> > > 
> > > 
> > 
> > This ties back to the previous patches for dynamic EPP.  My expectation
> > was that when dynamic EPP is enabled that users can't manually set the
> > EPP anymore (it will return -EBUSY) and likewise turning on dynamic EPP
> > should keep the governor as powersave.
> > 
> > I'll double check all those are properly enforced; but that's at least
> > the intent.
> 
> FWIW - I double checked and confirmed that this is working as intended.
> * I couldn't change from powersave to performance when dynamic_epp was
> enabled (-EBUSY)
> * I couldn't change energy_performance_preference when dynamic_epp was
> enabled (-EBUSY)

Thanks for double checking this. 


> 
> > 
> > IMO this "should" all work because turning on Dynamic EPP sysfs file
> > forces the driver to go through a state transition that it will tear
> > everything down and back up.  The policy will come back up in
> > "powersave" even if it was previously in "performance" when the dynamic
> > EPP sysfs file was turned on.
> > 
> > Longer term; I also envision the scheduler influencing EPP values when
> > dynamic_epp is turned on.  The "platform profile" would be an "input" to
> > that decision making process (maybe giving a weighting?), not the only
> > lever.

Yes, the scheduler influencing the EPP values is something that I have
been wanting to explore as well. My idea was to use the nature of the
task + the load on the rq to determine the EPP value.

> > 
> > I haven't given any serious look at how to do this with the scheduler, I
> > wanted to lay the foundation first being dynamic EPP and raw EPP.
> >
> > So even if dynamic_epp isn't interesting "right now" for server because
> > the focus is around behavior for AC/DC, don't write it off just yet.
>

Fair enough.


-- 
Thanks and Regards
gautham.