On 06-11-20, 18:02, Rafael J. Wysocki wrote: > On Fri, Nov 6, 2020 at 11:07 AM Viresh Kumar <viresh.ku...@linaro.org> wrote: > > > > On 05-11-20, 19:23, Rafael J. Wysocki wrote: > > > Index: linux-pm/include/linux/cpufreq.h > > > =================================================================== > > > --- linux-pm.orig/include/linux/cpufreq.h > > > +++ linux-pm/include/linux/cpufreq.h > > > @@ -63,6 +63,8 @@ struct cpufreq_policy { > > > > > > unsigned int min; /* in kHz */ > > > unsigned int max; /* in kHz */ > > > + unsigned int target_min; /* in kHz */ > > > + unsigned int target_max; /* in kHz */ > > > unsigned int cur; /* in kHz, only needed if cpufreq > > > * governors are used */ > > > unsigned int suspend_freq; /* freq to set during suspend > > > */ > > > > Rafael, honestly speaking I didn't like this patch very much. > > So what's the concern, specifically? > > > We need to fix a very specific problem with the intel-pstate driver when it > > is > > used with powersave/performance governor to make sure the hard limits > > are enforced. And this is something which no one else may face as > > well. > > Well, I predict that the CPPC driver will face this problem too at one point. > > As well as any other driver which doesn't select OPPs directly for > that matter, at least to some extent (note that intel_pstate in the > "passive" mode without HWP has it too, but since there is no way to > enforce the target max in that case, it is not relevant). > > > What about doing something like this instead in the intel_pstate > > driver only to get this fixed ? > > > > if (!strcmp(policy->governor->name, "powersave") || > > !strcmp(policy->governor->name, "performance")) > > hard-limit-to-be-enforced; > > > > This would be a much simpler and contained approach IMHO. > > I obviously prefer to do it the way I did in this series, because it > is more general and it is based on the governor telling the driver > what is needed instead of the driver trying to figure out what the > governor is and guessing what may be needed because of that. > > But if you have a very specific technical concern regarding my > approach, I can do it the other way too.
I was concerned about adding those fields in the policy structure, but I get that you want to do it in a more generic way. What about adding a field name "fixed" (or something else) in the governor's structure which tells us that the frequency is fixed and must be honored by the driver. -- viresh