[PATCH] cpufreq: Apply default governor for setpolicy drivers

[PATCH] cpufreq: Apply default governor for setpolicy drivers

[Jason Baron]

When configuring a default governor (via CONFIG_CPU_FREQ_DEFAULT_*) with the
'intel_pstate' driver, I found that the default is not honored. For example,
configure 'CONFIG_CPU_FREQ_GOV_PERFORMANCE', and then do:

# cat /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor
powersave

However, I can do:

echo "performance" > /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor

and it takes properly.

Fix by setting the default governor, if its either 'powersave' or 'performance'.
Otherwise, fall back to what the driver originally set via its 'init' routine.

Signed-off-by: Jason Baron <jbaron@akamai.com>
---
 drivers/cpufreq/cpufreq.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 02d534d..931fa67 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -826,8 +826,19 @@ static void cpufreq_init_policy(struct cpufreq_policy *policy)
 {
 	struct cpufreq_policy new_policy;
 	int ret = 0;
+	int init_policy;
 
 	memcpy(&new_policy, policy, sizeof(*policy));
+
+	/* honor the default governor policy, unless its invalid */
+	if (cpufreq_driver->setpolicy) {
+		init_policy = new_policy.policy;
+		if (cpufreq_parse_governor(policy->governor->name,
+				&new_policy.policy, &new_policy.governor)) {
+			new_policy.policy = init_policy;
+		}
+	}
+
 	/* assure that the starting sequence is run in cpufreq_set_policy */
 	policy->governor = NULL;
 
-- 
1.8.2.rc2

Re: [PATCH] cpufreq: Apply default governor for setpolicy drivers

[Jason Baron]

On 12/18/2013 04:38 PM, Rafael J. Wysocki wrote:
> On Wednesday, December 18, 2013 08:31:02 PM Jason Baron wrote:
>> When configuring a default governor (via CONFIG_CPU_FREQ_DEFAULT_*) with the
>> 'intel_pstate' driver, I found that the default is not honored. For example,
>> configure 'CONFIG_CPU_FREQ_GOV_PERFORMANCE', and then do:
> intel_pstate doesn't use any cpufreq governors, so all of this is pointless
> for that particular driver anyway.
>
> Thanks!
>

Ok, but if I look at 'intel_pstate', I see a 'set_policy' call that does different
things if the policy is 'performance' vs. 'powersave'. Same for the 'longrun'
driver. So yes, they don't use the 'governors', but changing the governors
at runtime does appear to change change how they behave. What am
I missing?

Thanks,

-Jason

Re: [PATCH] cpufreq: Apply default governor for setpolicy drivers

[Rafael J. Wysocki]

On Wednesday, December 18, 2013 08:31:02 PM Jason Baron wrote:
> When configuring a default governor (via CONFIG_CPU_FREQ_DEFAULT_*) with the
> 'intel_pstate' driver, I found that the default is not honored. For example,
> configure 'CONFIG_CPU_FREQ_GOV_PERFORMANCE', and then do:

intel_pstate doesn't use any cpufreq governors, so all of this is pointless
for that particular driver anyway.

Thanks!

> # cat /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor
> powersave
> 
> However, I can do:
> 
> echo "performance" > /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor
> 
> and it takes properly.
> 
> Fix by setting the default governor, if its either 'powersave' or 'performance'.
> Otherwise, fall back to what the driver originally set via its 'init' routine.
> 
> Signed-off-by: Jason Baron <jbaron@akamai.com>
> ---
>  drivers/cpufreq/cpufreq.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 02d534d..931fa67 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -826,8 +826,19 @@ static void cpufreq_init_policy(struct cpufreq_policy *policy)
>  {
>  	struct cpufreq_policy new_policy;
>  	int ret = 0;
> +	int init_policy;
>  
>  	memcpy(&new_policy, policy, sizeof(*policy));
> +
> +	/* honor the default governor policy, unless its invalid */
> +	if (cpufreq_driver->setpolicy) {
> +		init_policy = new_policy.policy;
> +		if (cpufreq_parse_governor(policy->governor->name,
> +				&new_policy.policy, &new_policy.governor)) {
> +			new_policy.policy = init_policy;
> +		}
> +	}
> +
>  	/* assure that the starting sequence is run in cpufreq_set_policy */
>  	policy->governor = NULL;
>  
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] cpufreq: Apply default governor for setpolicy drivers

[Rafael J. Wysocki]

On Wednesday, December 18, 2013 04:35:04 PM Jason Baron wrote:
> On 12/18/2013 04:38 PM, Rafael J. Wysocki wrote:
> > On Wednesday, December 18, 2013 08:31:02 PM Jason Baron wrote:
> >> When configuring a default governor (via CONFIG_CPU_FREQ_DEFAULT_*) with the
> >> 'intel_pstate' driver, I found that the default is not honored. For example,
> >> configure 'CONFIG_CPU_FREQ_GOV_PERFORMANCE', and then do:
> > intel_pstate doesn't use any cpufreq governors, so all of this is pointless
> > for that particular driver anyway.
> >
> > Thanks!
> >
> 
> Ok, but if I look at 'intel_pstate', I see a 'set_policy' call that does different
> things if the policy is 'performance' vs. 'powersave'. Same for the 'longrun'
> driver. So yes, they don't use the 'governors', but changing the governors
> at runtime does appear to change change how they behave.

OK, so you want the initial policy to be set in accordance with the
CONFIG_CPU_FREQ_DEFAULT_* setting.  That makes sense, although it is not
immediately clear from the patch changelog.

That said the change in the patch looks kind of overly complicated.
What about adding something like this instead?

+       if (cpufreq_driver->setpolicy) {
+               unsigned int ret;
+
+       	/* Use the default policyt if it is valid. */
+               if (!cpufreq_parse_governor(policy->governor->name, &ret, NULL))
+                       new_policy.policy = ret;
+       }

Rafael

Re: [PATCH] cpufreq: Apply default governor for setpolicy drivers

[Jason Baron]

On 12/18/2013 06:33 PM, Rafael J. Wysocki wrote:
> On Wednesday, December 18, 2013 04:35:04 PM Jason Baron wrote:
>> On 12/18/2013 04:38 PM, Rafael J. Wysocki wrote:
>>> On Wednesday, December 18, 2013 08:31:02 PM Jason Baron wrote:
>>>> When configuring a default governor (via CONFIG_CPU_FREQ_DEFAULT_*) with the
>>>> 'intel_pstate' driver, I found that the default is not honored. For example,
>>>> configure 'CONFIG_CPU_FREQ_GOV_PERFORMANCE', and then do:
>>> intel_pstate doesn't use any cpufreq governors, so all of this is pointless
>>> for that particular driver anyway.
>>>
>>> Thanks!
>>>
>> Ok, but if I look at 'intel_pstate', I see a 'set_policy' call that does different
>> things if the policy is 'performance' vs. 'powersave'. Same for the 'longrun'
>> driver. So yes, they don't use the 'governors', but changing the governors
>> at runtime does appear to change change how they behave.
> OK, so you want the initial policy to be set in accordance with the
> CONFIG_CPU_FREQ_DEFAULT_* setting.  That makes sense, although it is not
> immediately clear from the patch changelog.
>
> That said the change in the patch looks kind of overly complicated.
> What about adding something like this instead?

Agreed - this is better. Will you just pull the below patch, or should
I re-post with a better changelog?

Thanks,

-Jason

> +       if (cpufreq_driver->setpolicy) {
> +               unsigned int ret;
> +
> +       	/* Use the default policyt if it is valid. */
> +               if (!cpufreq_parse_governor(policy->governor->name, &ret, NULL))
> +                       new_policy.policy = ret;
> +       }
>
> Rafael
>

Re: [PATCH] cpufreq: Apply default governor for setpolicy drivers

[Viresh Kumar]

On 19 December 2013 05:03, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> +       if (cpufreq_driver->setpolicy) {
> +               unsigned int ret;
> +
> +               /* Use the default policyt if it is valid. */
> +               if (!cpufreq_parse_governor(policy->governor->name, &ret, NULL))
> +                       new_policy.policy = ret;
> +       }

What about this instead as cpufreq_parse_governor isn't supposed to change
value of ret in case of errors.

> +       if (cpufreq_driver->setpolicy)
> +               cpufreq_parse_governor(policy->governor->name, &new_policy.policy, NULL);

Re: [PATCH] cpufreq: Apply default governor for setpolicy drivers

[Rafael J. Wysocki]

On Thursday, December 19, 2013 10:57:06 AM Viresh Kumar wrote:
> On 19 December 2013 05:03, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > +       if (cpufreq_driver->setpolicy) {
> > +               unsigned int ret;
> > +
> > +               /* Use the default policyt if it is valid. */
> > +               if (!cpufreq_parse_governor(policy->governor->name, &ret, NULL))
> > +                       new_policy.policy = ret;
> > +       }
> 
> What about this instead as cpufreq_parse_governor isn't supposed to change
> value of ret in case of errors.
> 
> > +       if (cpufreq_driver->setpolicy)
> > +               cpufreq_parse_governor(policy->governor->name, &new_policy.policy, NULL);

Right, this is even better as far as the code goes, but adding a comment
describing the role of it would still be nice.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] cpufreq: Apply default governor for setpolicy drivers

[Rafael J. Wysocki]

On Wednesday, December 18, 2013 10:51:26 PM Jason Baron wrote:
> On 12/18/2013 06:33 PM, Rafael J. Wysocki wrote:
> > On Wednesday, December 18, 2013 04:35:04 PM Jason Baron wrote:
> >> On 12/18/2013 04:38 PM, Rafael J. Wysocki wrote:
> >>> On Wednesday, December 18, 2013 08:31:02 PM Jason Baron wrote:
> >>>> When configuring a default governor (via CONFIG_CPU_FREQ_DEFAULT_*) with the
> >>>> 'intel_pstate' driver, I found that the default is not honored. For example,
> >>>> configure 'CONFIG_CPU_FREQ_GOV_PERFORMANCE', and then do:
> >>> intel_pstate doesn't use any cpufreq governors, so all of this is pointless
> >>> for that particular driver anyway.
> >>>
> >>> Thanks!
> >>>
> >> Ok, but if I look at 'intel_pstate', I see a 'set_policy' call that does different
> >> things if the policy is 'performance' vs. 'powersave'. Same for the 'longrun'
> >> driver. So yes, they don't use the 'governors', but changing the governors
> >> at runtime does appear to change change how they behave.
> > OK, so you want the initial policy to be set in accordance with the
> > CONFIG_CPU_FREQ_DEFAULT_* setting.  That makes sense, although it is not
> > immediately clear from the patch changelog.
> >
> > That said the change in the patch looks kind of overly complicated.
> > What about adding something like this instead?
> 
> Agreed - this is better. Will you just pull the below patch, or should
> I re-post with a better changelog?

I think the Viresh' version is the best one, but it needs a comment about what
it is for and a better changelog would be welcome. :-)

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.