Messages in this thread | | | Date | Fri, 23 Oct 2020 11:40:18 +0530 | From | Viresh Kumar <> | Subject | Re: [PATCH 1/2] cpufreq: intel_pstate: Avoid missing HWP max updates in passive mode |
| |
On 22-10-20, 13:57, Rafael J. Wysocki wrote: > Index: linux-pm/drivers/cpufreq/cpufreq.c > =================================================================== > --- linux-pm.orig/drivers/cpufreq/cpufreq.c > +++ linux-pm/drivers/cpufreq/cpufreq.c > @@ -2182,6 +2182,9 @@ int __cpufreq_driver_target(struct cpufr > pr_debug("target for CPU %u: %u kHz, relation %u, requested %u kHz\n", > policy->cpu, target_freq, relation, old_target_freq); > > + if (cpufreq_driver->target) > + return cpufreq_driver->target(policy, target_freq, relation); > + > /* > * This might look like a redundant call as we are checking it again > * after finding index. But it is left intentionally for cases where > @@ -2194,9 +2197,6 @@ int __cpufreq_driver_target(struct cpufr > /* Save last value to restore later on errors */ > policy->restore_freq = policy->cur; > > - if (cpufreq_driver->target) > - return cpufreq_driver->target(policy, target_freq, relation); > - > if (!cpufreq_driver->target_index) > return -EINVAL;
From what I understood, you want your driver to get notified about policy->min/max changes and right now they are making it work from within the target() callback. Your commit log talks about policy->max and powersave combination, I think the same will be true in case of policy->min and performance ? And also with any other governor (like schedutil) if the target_freq doesn't change for a while.
And IMHO, this change is more like a band-aid which is going to remove the check of target != cur for all target() type drivers (which aren't many) and it feels like a penalty on them (which is also there for intel-cpufreq without hwp), and that we will get into the same problem for target_index() drivers as well if they want to do something similar in future, i.e. skip checking for same-freq.
Maybe adding a new flag for the cpufreq-driver for force-updates would be a better solution ? Which will make this very much driver dependent.
-- viresh
| |