[PATCH v2 0/4] cpufreq: intel_pstate: Handle powersave governor correctly in the passive mode with HWP

[PATCH v2 0/4] cpufreq: intel_pstate: Handle powersave governor correctly in the passive mode with HWP

[Rafael J. Wysocki]

Hi,

Even after the changes made very recently, the handling of the powersave
governor is not exactly as expected when intel_pstate operates in the
"passive" mode with HWP enabled.

Namely, in that case HWP is not limited to the policy min frequency, but it
can scale the frequency up to the policy max limit and it cannot be constrained
currently, because there are no provisions for that in the framework.

To address that, patches [1-3/4] add a new governor flag to indicate that this
governor wants the target frequency to be set to the exact value passed to the
driver, if possible, and change the powersave and performance governors to have
that flag set.

The last patch makes intel_pstate take that flag into account when programming
the HWP Request MSR.

Thanks!

[PATCH v2 1/4] cpufreq: Introduce governor flags

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

A new cpufreq governor flag will be added subsequently, so replace
the bool dynamic_switching fleid in struct cpufreq_governor with a
flags field and introduce CPUFREQ_GOV_FLAG_DYN_SWITCH to set for
the "dynamic switching" governors instead of it.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/cpufreq.c          |    2 +-
 drivers/cpufreq/cpufreq_governor.h |    2 +-
 include/linux/cpufreq.h            |    9 +++++++--
 kernel/sched/cpufreq_schedutil.c   |    2 +-
 4 files changed, 10 insertions(+), 5 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -2254,7 +2254,7 @@ static int cpufreq_init_governor(struct
 		return -EINVAL;
 
 	/* Platform doesn't want dynamic frequency switching ? */
-	if (policy->governor->dynamic_switching &&
+	if (policy->governor->flags & CPUFREQ_GOV_FLAG_DYN_SWITCH &&
 	    cpufreq_driver->flags & CPUFREQ_NO_AUTO_DYNAMIC_SWITCHING) {
 		struct cpufreq_governor *gov = cpufreq_fallback_governor();
 
Index: linux-pm/drivers/cpufreq/cpufreq_governor.h
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_governor.h
+++ linux-pm/drivers/cpufreq/cpufreq_governor.h
@@ -156,7 +156,7 @@ void cpufreq_dbs_governor_limits(struct
 #define CPUFREQ_DBS_GOVERNOR_INITIALIZER(_name_)			\
 	{								\
 		.name = _name_,						\
-		.dynamic_switching = true,				\
+		.flags = CPUFREQ_GOV_FLAG_DYN_SWITCH,			\
 		.owner = THIS_MODULE,					\
 		.init = cpufreq_dbs_governor_init,			\
 		.exit = cpufreq_dbs_governor_exit,			\
Index: linux-pm/include/linux/cpufreq.h
===================================================================
--- linux-pm.orig/include/linux/cpufreq.h
+++ linux-pm/include/linux/cpufreq.h
@@ -565,12 +565,17 @@ struct cpufreq_governor {
 					 char *buf);
 	int	(*store_setspeed)	(struct cpufreq_policy *policy,
 					 unsigned int freq);
-	/* For governors which change frequency dynamically by themselves */
-	bool			dynamic_switching;
 	struct list_head	governor_list;
 	struct module		*owner;
+	u8			flags;
 };
 
+/* Governor flags */
+
+/* For governors which change frequency dynamically by themselves */
+#define CPUFREQ_GOV_FLAG_DYN_SWITCH	BIT(0)
+
+
 /* Pass a target to the cpufreq driver */
 unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
 					unsigned int target_freq);
Index: linux-pm/kernel/sched/cpufreq_schedutil.c
===================================================================
--- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
+++ linux-pm/kernel/sched/cpufreq_schedutil.c
@@ -881,7 +881,7 @@ static void sugov_limits(struct cpufreq_
 struct cpufreq_governor schedutil_gov = {
 	.name			= "schedutil",
 	.owner			= THIS_MODULE,
-	.dynamic_switching	= true,
+	.flags			= CPUFREQ_GOV_FLAG_DYN_SWITCH,
 	.init			= sugov_init,
 	.exit			= sugov_exit,
 	.start			= sugov_start,

[PATCH v2 2/4] cpufreq: Introduce CPUFREQ_GOV_FLAG_STRICT_TARGET

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Introduce a new governor flag, CPUFREQ_GOV_FLAG_STRICT_TARGET, for
the govenors that want the target frequency to be set exactly to the
given value without leaving any room for adjustments on the hardware
side and set this flag for the powersave and performance governors.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/cpufreq_performance.c |    1 +
 drivers/cpufreq/cpufreq_powersave.c   |    1 +
 include/linux/cpufreq.h               |    3 +++
 3 files changed, 5 insertions(+)

Index: linux-pm/drivers/cpufreq/cpufreq_performance.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_performance.c
+++ linux-pm/drivers/cpufreq/cpufreq_performance.c
@@ -20,6 +20,7 @@ static void cpufreq_gov_performance_limi
 static struct cpufreq_governor cpufreq_gov_performance = {
 	.name		= "performance",
 	.owner		= THIS_MODULE,
+	.flags		= CPUFREQ_GOV_FLAG_STRICT_TARGET,
 	.limits		= cpufreq_gov_performance_limits,
 };
 
Index: linux-pm/drivers/cpufreq/cpufreq_powersave.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_powersave.c
+++ linux-pm/drivers/cpufreq/cpufreq_powersave.c
@@ -21,6 +21,7 @@ static struct cpufreq_governor cpufreq_g
 	.name		= "powersave",
 	.limits		= cpufreq_gov_powersave_limits,
 	.owner		= THIS_MODULE,
+	.flags		= CPUFREQ_GOV_FLAG_STRICT_TARGET,
 };
 
 MODULE_AUTHOR("Dominik Brodowski <linux@brodo.de>");
Index: linux-pm/include/linux/cpufreq.h
===================================================================
--- linux-pm.orig/include/linux/cpufreq.h
+++ linux-pm/include/linux/cpufreq.h
@@ -575,6 +575,9 @@ struct cpufreq_governor {
 /* For governors which change frequency dynamically by themselves */
 #define CPUFREQ_GOV_FLAG_DYN_SWITCH	BIT(0)
 
+/* For governors wanting the target frequency to be set exactly */
+#define CPUFREQ_GOV_FLAG_STRICT_TARGET	BIT(1)
+
 
 /* Pass a target to the cpufreq driver */
 unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,

[PATCH v2 3/4] cpufreq: Add strict_target to struct cpufreq_policy

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Add a new field to be set when the CPUFREQ_GOV_FLAG_STRICT_TARGET
flag is set for the current governor to struct cpufreq_policy, so
that the drivers needing to check CPUFREQ_GOV_FLAG_STRICT_TARGET do
not have to access the governor object during every frequency
transition.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/cpufreq.c |    2 ++
 include/linux/cpufreq.h   |    6 ++++++
 2 files changed, 8 insertions(+)

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -2280,6 +2280,8 @@ static int cpufreq_init_governor(struct
 		}
 	}
 
+	policy->strict_target = !!(policy->governor->flags & CPUFREQ_GOV_FLAG_STRICT_TARGET);
+
 	return 0;
 }
 
Index: linux-pm/include/linux/cpufreq.h
===================================================================
--- linux-pm.orig/include/linux/cpufreq.h
+++ linux-pm/include/linux/cpufreq.h
@@ -109,6 +109,12 @@ struct cpufreq_policy {
 	bool			fast_switch_enabled;
 
 	/*
+	 * Set if the CPUFREQ_GOV_FLAG_STRICT_TARGET flag is set for the
+	 * current governor.
+	 */
+	bool			strict_target;
+
+	/*
 	 * Preferred average time interval between consecutive invocations of
 	 * the driver to set the frequency for this policy.  To be set by the
 	 * scaling driver (0, which is the default, means no preference).

[PATCH v2 4/4] cpufreq: intel_pstate: Take CPUFREQ_GOV_FLAG_STRICT_TARGET into account

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Make intel_pstate take the new CPUFREQ_GOV_FLAG_STRICT_TARGET
governor flag into account when it operates in the passive mode with
HWP enabled, so as to fix the "powersave" governor behavior in that
case (currently, HWP is allowed to scale the performance all the way
up to the policy max limit when the "powersave" governor is used,
but it should be constrained to the policy min limit then).

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/intel_pstate.c |   16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

Index: linux-pm/drivers/cpufreq/intel_pstate.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/intel_pstate.c
+++ linux-pm/drivers/cpufreq/intel_pstate.c
@@ -2527,7 +2527,7 @@ static void intel_cpufreq_trace(struct c
 }
 
 static void intel_cpufreq_adjust_hwp(struct cpudata *cpu, u32 target_pstate,
-				     bool fast_switch)
+				     bool strict, bool fast_switch)
 {
 	u64 prev = READ_ONCE(cpu->hwp_req_cached), value = prev;
 
@@ -2539,7 +2539,7 @@ static void intel_cpufreq_adjust_hwp(str
 	 * field in it, so opportunistically update the max too if needed.
 	 */
 	value &= ~HWP_MAX_PERF(~0L);
-	value |= HWP_MAX_PERF(cpu->max_perf_ratio);
+	value |= HWP_MAX_PERF(strict ? target_pstate : cpu->max_perf_ratio);
 
 	if (value == prev)
 		return;
@@ -2562,14 +2562,16 @@ static void intel_cpufreq_adjust_perf_ct
 			      pstate_funcs.get_val(cpu, target_pstate));
 }
 
-static int intel_cpufreq_update_pstate(struct cpudata *cpu, int target_pstate,
-				       bool fast_switch)
+static int intel_cpufreq_update_pstate(struct cpufreq_policy *policy,
+				       int target_pstate, bool fast_switch)
 {
+	struct cpudata *cpu = all_cpu_data[policy->cpu];
 	int old_pstate = cpu->pstate.current_pstate;
 
 	target_pstate = intel_pstate_prepare_request(cpu, target_pstate);
 	if (hwp_active) {
-		intel_cpufreq_adjust_hwp(cpu, target_pstate, fast_switch);
+		intel_cpufreq_adjust_hwp(cpu, target_pstate,
+					 policy->strict_target, fast_switch);
 		cpu->pstate.current_pstate = target_pstate;
 	} else if (target_pstate != old_pstate) {
 		intel_cpufreq_adjust_perf_ctl(cpu, target_pstate, fast_switch);
@@ -2609,7 +2611,7 @@ static int intel_cpufreq_target(struct c
 		break;
 	}
 
-	target_pstate = intel_cpufreq_update_pstate(cpu, target_pstate, false);
+	target_pstate = intel_cpufreq_update_pstate(policy, target_pstate, false);
 
 	freqs.new = target_pstate * cpu->pstate.scaling;
 
@@ -2628,7 +2630,7 @@ static unsigned int intel_cpufreq_fast_s
 
 	target_pstate = DIV_ROUND_UP(target_freq, cpu->pstate.scaling);
 
-	target_pstate = intel_cpufreq_update_pstate(cpu, target_pstate, true);
+	target_pstate = intel_cpufreq_update_pstate(policy, target_pstate, true);
 
 	return target_pstate * cpu->pstate.scaling;
 }

Re: [PATCH v2 1/4] cpufreq: Introduce governor flags

[Viresh Kumar]

On 09-11-20, 17:51, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> A new cpufreq governor flag will be added subsequently, so replace
> the bool dynamic_switching fleid in struct cpufreq_governor with a
> flags field and introduce CPUFREQ_GOV_FLAG_DYN_SWITCH to set for
> the "dynamic switching" governors instead of it.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/cpufreq.c          |    2 +-
>  drivers/cpufreq/cpufreq_governor.h |    2 +-
>  include/linux/cpufreq.h            |    9 +++++++--
>  kernel/sched/cpufreq_schedutil.c   |    2 +-
>  4 files changed, 10 insertions(+), 5 deletions(-)
> 
> Index: linux-pm/drivers/cpufreq/cpufreq.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> +++ linux-pm/drivers/cpufreq/cpufreq.c
> @@ -2254,7 +2254,7 @@ static int cpufreq_init_governor(struct
>  		return -EINVAL;
>  
>  	/* Platform doesn't want dynamic frequency switching ? */
> -	if (policy->governor->dynamic_switching &&

I completely forgot that we had something like this :)

> +	if (policy->governor->flags & CPUFREQ_GOV_FLAG_DYN_SWITCH &&
>  	    cpufreq_driver->flags & CPUFREQ_NO_AUTO_DYNAMIC_SWITCHING) {
>  		struct cpufreq_governor *gov = cpufreq_fallback_governor();
>  
> Index: linux-pm/drivers/cpufreq/cpufreq_governor.h
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq_governor.h
> +++ linux-pm/drivers/cpufreq/cpufreq_governor.h
> @@ -156,7 +156,7 @@ void cpufreq_dbs_governor_limits(struct
>  #define CPUFREQ_DBS_GOVERNOR_INITIALIZER(_name_)			\
>  	{								\
>  		.name = _name_,						\
> -		.dynamic_switching = true,				\
> +		.flags = CPUFREQ_GOV_FLAG_DYN_SWITCH,			\
>  		.owner = THIS_MODULE,					\
>  		.init = cpufreq_dbs_governor_init,			\
>  		.exit = cpufreq_dbs_governor_exit,			\
> Index: linux-pm/include/linux/cpufreq.h
> ===================================================================
> --- linux-pm.orig/include/linux/cpufreq.h
> +++ linux-pm/include/linux/cpufreq.h
> @@ -565,12 +565,17 @@ struct cpufreq_governor {
>  					 char *buf);
>  	int	(*store_setspeed)	(struct cpufreq_policy *policy,
>  					 unsigned int freq);
> -	/* For governors which change frequency dynamically by themselves */
> -	bool			dynamic_switching;
>  	struct list_head	governor_list;
>  	struct module		*owner;
> +	u8			flags;
>  };
>  
> +/* Governor flags */
> +
> +/* For governors which change frequency dynamically by themselves */
> +#define CPUFREQ_GOV_FLAG_DYN_SWITCH	BIT(0)

Maybe just drop the FLAG_ part as we don't use it for other cpufreq related
flags as well. That will also give us space to write DYN as DYNAMIC (it may be
better as we use the full name in CPUFREQ_NO_AUTO_DYNAMIC_SWITCHING).

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH v2 2/4] cpufreq: Introduce CPUFREQ_GOV_FLAG_STRICT_TARGET

[Viresh Kumar]

On 09-11-20, 17:52, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Introduce a new governor flag, CPUFREQ_GOV_FLAG_STRICT_TARGET, for
> the govenors that want the target frequency to be set exactly to the
> given value without leaving any room for adjustments on the hardware
> side and set this flag for the powersave and performance governors.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/cpufreq_performance.c |    1 +
>  drivers/cpufreq/cpufreq_powersave.c   |    1 +
>  include/linux/cpufreq.h               |    3 +++
>  3 files changed, 5 insertions(+)
> 
> Index: linux-pm/drivers/cpufreq/cpufreq_performance.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq_performance.c
> +++ linux-pm/drivers/cpufreq/cpufreq_performance.c
> @@ -20,6 +20,7 @@ static void cpufreq_gov_performance_limi
>  static struct cpufreq_governor cpufreq_gov_performance = {
>  	.name		= "performance",
>  	.owner		= THIS_MODULE,
> +	.flags		= CPUFREQ_GOV_FLAG_STRICT_TARGET,
>  	.limits		= cpufreq_gov_performance_limits,
>  };
>  
> Index: linux-pm/drivers/cpufreq/cpufreq_powersave.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq_powersave.c
> +++ linux-pm/drivers/cpufreq/cpufreq_powersave.c
> @@ -21,6 +21,7 @@ static struct cpufreq_governor cpufreq_g
>  	.name		= "powersave",
>  	.limits		= cpufreq_gov_powersave_limits,
>  	.owner		= THIS_MODULE,
> +	.flags		= CPUFREQ_GOV_FLAG_STRICT_TARGET,
>  };
>  
>  MODULE_AUTHOR("Dominik Brodowski <linux@brodo.de>");
> Index: linux-pm/include/linux/cpufreq.h
> ===================================================================
> --- linux-pm.orig/include/linux/cpufreq.h
> +++ linux-pm/include/linux/cpufreq.h
> @@ -575,6 +575,9 @@ struct cpufreq_governor {
>  /* For governors which change frequency dynamically by themselves */
>  #define CPUFREQ_GOV_FLAG_DYN_SWITCH	BIT(0)
>  
> +/* For governors wanting the target frequency to be set exactly */
> +#define CPUFREQ_GOV_FLAG_STRICT_TARGET	BIT(1)
> +
>  
>  /* Pass a target to the cpufreq driver */
>  unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH v2 3/4] cpufreq: Add strict_target to struct cpufreq_policy

[Viresh Kumar]

On 09-11-20, 17:53, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Add a new field to be set when the CPUFREQ_GOV_FLAG_STRICT_TARGET
> flag is set for the current governor to struct cpufreq_policy, so
> that the drivers needing to check CPUFREQ_GOV_FLAG_STRICT_TARGET do
> not have to access the governor object during every frequency
> transition.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/cpufreq.c |    2 ++
>  include/linux/cpufreq.h   |    6 ++++++
>  2 files changed, 8 insertions(+)
> 
> Index: linux-pm/drivers/cpufreq/cpufreq.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> +++ linux-pm/drivers/cpufreq/cpufreq.c
> @@ -2280,6 +2280,8 @@ static int cpufreq_init_governor(struct
>  		}
>  	}
>  
> +	policy->strict_target = !!(policy->governor->flags & CPUFREQ_GOV_FLAG_STRICT_TARGET);
> +
>  	return 0;
>  }
>  
> Index: linux-pm/include/linux/cpufreq.h
> ===================================================================
> --- linux-pm.orig/include/linux/cpufreq.h
> +++ linux-pm/include/linux/cpufreq.h
> @@ -109,6 +109,12 @@ struct cpufreq_policy {
>  	bool			fast_switch_enabled;
>  
>  	/*
> +	 * Set if the CPUFREQ_GOV_FLAG_STRICT_TARGET flag is set for the
> +	 * current governor.
> +	 */
> +	bool			strict_target;
> +
> +	/*
>  	 * Preferred average time interval between consecutive invocations of
>  	 * the driver to set the frequency for this policy.  To be set by the
>  	 * scaling driver (0, which is the default, means no preference).

I was kind of hoping to avoid adding a field here when I proposed updating the
gov structure. I do understand the performance related penalty of accessing the
gov structure for fast switch case though and so wonder if we really need this,
then should we avoid changing the gov structure at all ? I mean there is only
one user of that field now, do we really need a flag for it ? We can just do the
string comparison here with powersave and performance to set strict_target.

Whatever you feel is better though.

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH v2 4/4] cpufreq: intel_pstate: Take CPUFREQ_GOV_FLAG_STRICT_TARGET into account

[Viresh Kumar]

On 09-11-20, 17:55, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Make intel_pstate take the new CPUFREQ_GOV_FLAG_STRICT_TARGET
> governor flag into account when it operates in the passive mode with
> HWP enabled, so as to fix the "powersave" governor behavior in that
> case (currently, HWP is allowed to scale the performance all the way
> up to the policy max limit when the "powersave" governor is used,
> but it should be constrained to the policy min limit then).
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/intel_pstate.c |   16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> Index: linux-pm/drivers/cpufreq/intel_pstate.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/intel_pstate.c
> +++ linux-pm/drivers/cpufreq/intel_pstate.c
> @@ -2527,7 +2527,7 @@ static void intel_cpufreq_trace(struct c
>  }
>  
>  static void intel_cpufreq_adjust_hwp(struct cpudata *cpu, u32 target_pstate,
> -				     bool fast_switch)
> +				     bool strict, bool fast_switch)
>  {
>  	u64 prev = READ_ONCE(cpu->hwp_req_cached), value = prev;
>  
> @@ -2539,7 +2539,7 @@ static void intel_cpufreq_adjust_hwp(str
>  	 * field in it, so opportunistically update the max too if needed.
>  	 */
>  	value &= ~HWP_MAX_PERF(~0L);
> -	value |= HWP_MAX_PERF(cpu->max_perf_ratio);
> +	value |= HWP_MAX_PERF(strict ? target_pstate : cpu->max_perf_ratio);
>  
>  	if (value == prev)
>  		return;
> @@ -2562,14 +2562,16 @@ static void intel_cpufreq_adjust_perf_ct
>  			      pstate_funcs.get_val(cpu, target_pstate));
>  }
>  
> -static int intel_cpufreq_update_pstate(struct cpudata *cpu, int target_pstate,
> -				       bool fast_switch)
> +static int intel_cpufreq_update_pstate(struct cpufreq_policy *policy,
> +				       int target_pstate, bool fast_switch)
>  {
> +	struct cpudata *cpu = all_cpu_data[policy->cpu];
>  	int old_pstate = cpu->pstate.current_pstate;
>  
>  	target_pstate = intel_pstate_prepare_request(cpu, target_pstate);
>  	if (hwp_active) {
> -		intel_cpufreq_adjust_hwp(cpu, target_pstate, fast_switch);
> +		intel_cpufreq_adjust_hwp(cpu, target_pstate,
> +					 policy->strict_target, fast_switch);
>  		cpu->pstate.current_pstate = target_pstate;
>  	} else if (target_pstate != old_pstate) {
>  		intel_cpufreq_adjust_perf_ctl(cpu, target_pstate, fast_switch);
> @@ -2609,7 +2611,7 @@ static int intel_cpufreq_target(struct c
>  		break;
>  	}
>  
> -	target_pstate = intel_cpufreq_update_pstate(cpu, target_pstate, false);
> +	target_pstate = intel_cpufreq_update_pstate(policy, target_pstate, false);
>  
>  	freqs.new = target_pstate * cpu->pstate.scaling;
>  
> @@ -2628,7 +2630,7 @@ static unsigned int intel_cpufreq_fast_s
>  
>  	target_pstate = DIV_ROUND_UP(target_freq, cpu->pstate.scaling);
>  
> -	target_pstate = intel_cpufreq_update_pstate(cpu, target_pstate, true);
> +	target_pstate = intel_cpufreq_update_pstate(policy, target_pstate, true);
>  
>  	return target_pstate * cpu->pstate.scaling;
>  }

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH v2 1/4] cpufreq: Introduce governor flags

[Rafael J. Wysocki]

On Tue, Nov 10, 2020 at 3:41 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 09-11-20, 17:51, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > A new cpufreq governor flag will be added subsequently, so replace
> > the bool dynamic_switching fleid in struct cpufreq_governor with a
> > flags field and introduce CPUFREQ_GOV_FLAG_DYN_SWITCH to set for
> > the "dynamic switching" governors instead of it.
> >
> > No intentional functional impact.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/cpufreq/cpufreq.c          |    2 +-
> >  drivers/cpufreq/cpufreq_governor.h |    2 +-
> >  include/linux/cpufreq.h            |    9 +++++++--
> >  kernel/sched/cpufreq_schedutil.c   |    2 +-
> >  4 files changed, 10 insertions(+), 5 deletions(-)
> >
> > Index: linux-pm/drivers/cpufreq/cpufreq.c
> > ===================================================================
> > --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> > +++ linux-pm/drivers/cpufreq/cpufreq.c
> > @@ -2254,7 +2254,7 @@ static int cpufreq_init_governor(struct
> >               return -EINVAL;
> >
> >       /* Platform doesn't want dynamic frequency switching ? */
> > -     if (policy->governor->dynamic_switching &&
>
> I completely forgot that we had something like this :)
>
> > +     if (policy->governor->flags & CPUFREQ_GOV_FLAG_DYN_SWITCH &&
> >           cpufreq_driver->flags & CPUFREQ_NO_AUTO_DYNAMIC_SWITCHING) {
> >               struct cpufreq_governor *gov = cpufreq_fallback_governor();
> >
> > Index: linux-pm/drivers/cpufreq/cpufreq_governor.h
> > ===================================================================
> > --- linux-pm.orig/drivers/cpufreq/cpufreq_governor.h
> > +++ linux-pm/drivers/cpufreq/cpufreq_governor.h
> > @@ -156,7 +156,7 @@ void cpufreq_dbs_governor_limits(struct
> >  #define CPUFREQ_DBS_GOVERNOR_INITIALIZER(_name_)                     \
> >       {                                                               \
> >               .name = _name_,                                         \
> > -             .dynamic_switching = true,                              \
> > +             .flags = CPUFREQ_GOV_FLAG_DYN_SWITCH,                   \
> >               .owner = THIS_MODULE,                                   \
> >               .init = cpufreq_dbs_governor_init,                      \
> >               .exit = cpufreq_dbs_governor_exit,                      \
> > Index: linux-pm/include/linux/cpufreq.h
> > ===================================================================
> > --- linux-pm.orig/include/linux/cpufreq.h
> > +++ linux-pm/include/linux/cpufreq.h
> > @@ -565,12 +565,17 @@ struct cpufreq_governor {
> >                                        char *buf);
> >       int     (*store_setspeed)       (struct cpufreq_policy *policy,
> >                                        unsigned int freq);
> > -     /* For governors which change frequency dynamically by themselves */
> > -     bool                    dynamic_switching;
> >       struct list_head        governor_list;
> >       struct module           *owner;
> > +     u8                      flags;
> >  };
> >
> > +/* Governor flags */
> > +
> > +/* For governors which change frequency dynamically by themselves */
> > +#define CPUFREQ_GOV_FLAG_DYN_SWITCH  BIT(0)
>
> Maybe just drop the FLAG_ part as we don't use it for other cpufreq related
> flags as well. That will also give us space to write DYN as DYNAMIC (it may be
> better as we use the full name in CPUFREQ_NO_AUTO_DYNAMIC_SWITCHING).

OK, I'll rename the flag (and the new one too).

> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Thanks!

Re: [PATCH v2 3/4] cpufreq: Add strict_target to struct cpufreq_policy

[Rafael J. Wysocki]

On Tue, Nov 10, 2020 at 3:47 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 09-11-20, 17:53, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Add a new field to be set when the CPUFREQ_GOV_FLAG_STRICT_TARGET
> > flag is set for the current governor to struct cpufreq_policy, so
> > that the drivers needing to check CPUFREQ_GOV_FLAG_STRICT_TARGET do
> > not have to access the governor object during every frequency
> > transition.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/cpufreq/cpufreq.c |    2 ++
> >  include/linux/cpufreq.h   |    6 ++++++
> >  2 files changed, 8 insertions(+)
> >
> > Index: linux-pm/drivers/cpufreq/cpufreq.c
> > ===================================================================
> > --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> > +++ linux-pm/drivers/cpufreq/cpufreq.c
> > @@ -2280,6 +2280,8 @@ static int cpufreq_init_governor(struct
> >               }
> >       }
> >
> > +     policy->strict_target = !!(policy->governor->flags & CPUFREQ_GOV_FLAG_STRICT_TARGET);
> > +
> >       return 0;
> >  }
> >
> > Index: linux-pm/include/linux/cpufreq.h
> > ===================================================================
> > --- linux-pm.orig/include/linux/cpufreq.h
> > +++ linux-pm/include/linux/cpufreq.h
> > @@ -109,6 +109,12 @@ struct cpufreq_policy {
> >       bool                    fast_switch_enabled;
> >
> >       /*
> > +      * Set if the CPUFREQ_GOV_FLAG_STRICT_TARGET flag is set for the
> > +      * current governor.
> > +      */
> > +     bool                    strict_target;
> > +
> > +     /*
> >        * Preferred average time interval between consecutive invocations of
> >        * the driver to set the frequency for this policy.  To be set by the
> >        * scaling driver (0, which is the default, means no preference).
>
> I was kind of hoping to avoid adding a field here when I proposed updating the
> gov structure. I do understand the performance related penalty of accessing the
> gov structure for fast switch case though and so wonder if we really need this,
> then should we avoid changing the gov structure at all ? I mean there is only
> one user of that field now, do we really need a flag for it ? We can just do the
> string comparison here with powersave and performance to set strict_target.
>
> Whatever you feel is better though.

The cost of having the flag is zero and it allows things to be
documented a bit better IMV.

> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Thanks!

[PATCH v3 0/4] cpufreq: intel_pstate: Handle powersave governor correctly in the passive mode with HWP

[Rafael J. Wysocki]

Hi,

On Monday, November 9, 2020 5:49:49 PM CET Rafael J. Wysocki wrote:
> 
> Even after the changes made very recently, the handling of the powersave
> governor is not exactly as expected when intel_pstate operates in the
> "passive" mode with HWP enabled.
> 
> Namely, in that case HWP is not limited to the policy min frequency, but it
> can scale the frequency up to the policy max limit and it cannot be constrained
> currently, because there are no provisions for that in the framework.
> 
> To address that, patches [1-3/4] add a new governor flag to indicate that this
> governor wants the target frequency to be set to the exact value passed to the
> driver, if possible, and change the powersave and performance governors to have
> that flag set.
> 
> The last patch makes intel_pstate take that flag into account when programming
> the HWP Request MSR.

The v3 simply uses different names for the new governor flags.

Thanks!

[PATCH v3 1/4] cpufreq: Introduce governor flags

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

A new cpufreq governor flag will be added subsequently, so replace
the bool dynamic_switching fleid in struct cpufreq_governor with a
flags field and introduce CPUFREQ_GOV_DYNAMIC_SWITCHING to set for
the "dynamic switching" governors instead of it.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c          |    2 +-
 drivers/cpufreq/cpufreq_governor.h |    2 +-
 include/linux/cpufreq.h            |    9 +++++++--
 kernel/sched/cpufreq_schedutil.c   |    2 +-
 4 files changed, 10 insertions(+), 5 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -2254,7 +2254,7 @@ static int cpufreq_init_governor(struct
 		return -EINVAL;
 
 	/* Platform doesn't want dynamic frequency switching ? */
-	if (policy->governor->dynamic_switching &&
+	if (policy->governor->flags & CPUFREQ_GOV_DYNAMIC_SWITCHING &&
 	    cpufreq_driver->flags & CPUFREQ_NO_AUTO_DYNAMIC_SWITCHING) {
 		struct cpufreq_governor *gov = cpufreq_fallback_governor();
 
Index: linux-pm/drivers/cpufreq/cpufreq_governor.h
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_governor.h
+++ linux-pm/drivers/cpufreq/cpufreq_governor.h
@@ -156,7 +156,7 @@ void cpufreq_dbs_governor_limits(struct
 #define CPUFREQ_DBS_GOVERNOR_INITIALIZER(_name_)			\
 	{								\
 		.name = _name_,						\
-		.dynamic_switching = true,				\
+		.flags = CPUFREQ_GOV_DYNAMIC_SWITCHING,			\
 		.owner = THIS_MODULE,					\
 		.init = cpufreq_dbs_governor_init,			\
 		.exit = cpufreq_dbs_governor_exit,			\
Index: linux-pm/include/linux/cpufreq.h
===================================================================
--- linux-pm.orig/include/linux/cpufreq.h
+++ linux-pm/include/linux/cpufreq.h
@@ -565,12 +565,17 @@ struct cpufreq_governor {
 					 char *buf);
 	int	(*store_setspeed)	(struct cpufreq_policy *policy,
 					 unsigned int freq);
-	/* For governors which change frequency dynamically by themselves */
-	bool			dynamic_switching;
 	struct list_head	governor_list;
 	struct module		*owner;
+	u8			flags;
 };
 
+/* Governor flags */
+
+/* For governors which change frequency dynamically by themselves */
+#define CPUFREQ_GOV_DYNAMIC_SWITCHING	BIT(0)
+
+
 /* Pass a target to the cpufreq driver */
 unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
 					unsigned int target_freq);
Index: linux-pm/kernel/sched/cpufreq_schedutil.c
===================================================================
--- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
+++ linux-pm/kernel/sched/cpufreq_schedutil.c
@@ -881,7 +881,7 @@ static void sugov_limits(struct cpufreq_
 struct cpufreq_governor schedutil_gov = {
 	.name			= "schedutil",
 	.owner			= THIS_MODULE,
-	.dynamic_switching	= true,
+	.flags			= CPUFREQ_GOV_DYNAMIC_SWITCHING,
 	.init			= sugov_init,
 	.exit			= sugov_exit,
 	.start			= sugov_start,

[PATCH v3 2/4] cpufreq: Introduce CPUFREQ_GOV_STRICT_TARGET

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Introduce a new governor flag, CPUFREQ_GOV_STRICT_TARGET, for the
governors that want the target frequency to be set exactly to the
given value without leaving any room for adjustments on the hardware
side and set this flag for the powersave and performance governors.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_performance.c |    1 +
 drivers/cpufreq/cpufreq_powersave.c   |    1 +
 include/linux/cpufreq.h               |    3 +++
 3 files changed, 5 insertions(+)

Index: linux-pm/drivers/cpufreq/cpufreq_performance.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_performance.c
+++ linux-pm/drivers/cpufreq/cpufreq_performance.c
@@ -20,6 +20,7 @@ static void cpufreq_gov_performance_limi
 static struct cpufreq_governor cpufreq_gov_performance = {
 	.name		= "performance",
 	.owner		= THIS_MODULE,
+	.flags		= CPUFREQ_GOV_STRICT_TARGET,
 	.limits		= cpufreq_gov_performance_limits,
 };
 
Index: linux-pm/drivers/cpufreq/cpufreq_powersave.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_powersave.c
+++ linux-pm/drivers/cpufreq/cpufreq_powersave.c
@@ -21,6 +21,7 @@ static struct cpufreq_governor cpufreq_g
 	.name		= "powersave",
 	.limits		= cpufreq_gov_powersave_limits,
 	.owner		= THIS_MODULE,
+	.flags		= CPUFREQ_GOV_STRICT_TARGET,
 };
 
 MODULE_AUTHOR("Dominik Brodowski <linux@brodo.de>");
Index: linux-pm/include/linux/cpufreq.h
===================================================================
--- linux-pm.orig/include/linux/cpufreq.h
+++ linux-pm/include/linux/cpufreq.h
@@ -575,6 +575,9 @@ struct cpufreq_governor {
 /* For governors which change frequency dynamically by themselves */
 #define CPUFREQ_GOV_DYNAMIC_SWITCHING	BIT(0)
 
+/* For governors wanting the target frequency to be set exactly */
+#define CPUFREQ_GOV_STRICT_TARGET	BIT(1)
+
 
 /* Pass a target to the cpufreq driver */
 unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,

[PATCH v3 3/4] cpufreq: Add strict_target to struct cpufreq_policy

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Add a new field to be set when the CPUFREQ_GOV_STRICT_TARGET flag is
set for the current governor to struct cpufreq_policy, so that the
drivers needing to check CPUFREQ_GOV_STRICT_TARGET do not have to
access the governor object during every frequency transition.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c |    2 ++
 include/linux/cpufreq.h   |    6 ++++++
 2 files changed, 8 insertions(+)

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -2280,6 +2280,8 @@ static int cpufreq_init_governor(struct
 		}
 	}
 
+	policy->strict_target = !!(policy->governor->flags & CPUFREQ_GOV_STRICT_TARGET);
+
 	return 0;
 }
 
Index: linux-pm/include/linux/cpufreq.h
===================================================================
--- linux-pm.orig/include/linux/cpufreq.h
+++ linux-pm/include/linux/cpufreq.h
@@ -109,6 +109,12 @@ struct cpufreq_policy {
 	bool			fast_switch_enabled;
 
 	/*
+	 * Set if the CPUFREQ_GOV_STRICT_TARGET flag is set for the current
+	 * governor.
+	 */
+	bool			strict_target;
+
+	/*
 	 * Preferred average time interval between consecutive invocations of
 	 * the driver to set the frequency for this policy.  To be set by the
 	 * scaling driver (0, which is the default, means no preference).

[PATCH v3 4/4] cpufreq: intel_pstate: Take CPUFREQ_GOV_STRICT_TARGET into account

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Make intel_pstate take the new CPUFREQ_GOV_STRICT_TARGET governor
flag into account when it operates in the passive mode with HWP
enabled, so as to fix the "powersave" governor behavior in that
case (currently, HWP is allowed to scale the performance all the
way up to the policy max limit when the "powersave" governor is
used, but it should be constrained to the policy min limit then).

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/intel_pstate.c |   16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

Index: linux-pm/drivers/cpufreq/intel_pstate.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/intel_pstate.c
+++ linux-pm/drivers/cpufreq/intel_pstate.c
@@ -2527,7 +2527,7 @@ static void intel_cpufreq_trace(struct c
 }
 
 static void intel_cpufreq_adjust_hwp(struct cpudata *cpu, u32 target_pstate,
-				     bool fast_switch)
+				     bool strict, bool fast_switch)
 {
 	u64 prev = READ_ONCE(cpu->hwp_req_cached), value = prev;
 
@@ -2539,7 +2539,7 @@ static void intel_cpufreq_adjust_hwp(str
 	 * field in it, so opportunistically update the max too if needed.
 	 */
 	value &= ~HWP_MAX_PERF(~0L);
-	value |= HWP_MAX_PERF(cpu->max_perf_ratio);
+	value |= HWP_MAX_PERF(strict ? target_pstate : cpu->max_perf_ratio);
 
 	if (value == prev)
 		return;
@@ -2562,14 +2562,16 @@ static void intel_cpufreq_adjust_perf_ct
 			      pstate_funcs.get_val(cpu, target_pstate));
 }
 
-static int intel_cpufreq_update_pstate(struct cpudata *cpu, int target_pstate,
-				       bool fast_switch)
+static int intel_cpufreq_update_pstate(struct cpufreq_policy *policy,
+				       int target_pstate, bool fast_switch)
 {
+	struct cpudata *cpu = all_cpu_data[policy->cpu];
 	int old_pstate = cpu->pstate.current_pstate;
 
 	target_pstate = intel_pstate_prepare_request(cpu, target_pstate);
 	if (hwp_active) {
-		intel_cpufreq_adjust_hwp(cpu, target_pstate, fast_switch);
+		intel_cpufreq_adjust_hwp(cpu, target_pstate,
+					 policy->strict_target, fast_switch);
 		cpu->pstate.current_pstate = target_pstate;
 	} else if (target_pstate != old_pstate) {
 		intel_cpufreq_adjust_perf_ctl(cpu, target_pstate, fast_switch);
@@ -2609,7 +2611,7 @@ static int intel_cpufreq_target(struct c
 		break;
 	}
 
-	target_pstate = intel_cpufreq_update_pstate(cpu, target_pstate, false);
+	target_pstate = intel_cpufreq_update_pstate(policy, target_pstate, false);
 
 	freqs.new = target_pstate * cpu->pstate.scaling;
 
@@ -2628,7 +2630,7 @@ static unsigned int intel_cpufreq_fast_s
 
 	target_pstate = DIV_ROUND_UP(target_freq, cpu->pstate.scaling);
 
-	target_pstate = intel_cpufreq_update_pstate(cpu, target_pstate, true);
+	target_pstate = intel_cpufreq_update_pstate(policy, target_pstate, true);
 
 	return target_pstate * cpu->pstate.scaling;
 }

RE: [PATCH v3 0/4] cpufreq: intel_pstate: Handle powersave governor correctly in the passive mode with HWP

[Doug Smythies]

On 2020.11.10 09:22 Rafael J. Wysocki wrote:
> On Monday, November 9, 2020 5:49:49 PM CET Rafael J. Wysocki wrote:
>>
>> Even after the changes made very recently, the handling of the powersave
>> governor is not exactly as expected when intel_pstate operates in the
>> "passive" mode with HWP enabled.
>>
>> Namely, in that case HWP is not limited to the policy min frequency, but it
>> can scale the frequency up to the policy max limit and it cannot be constrained
>> currently, because there are no provisions for that in the framework.
>>
>> To address that, patches [1-3/4] add a new governor flag to indicate that this
>> governor wants the target frequency to be set to the exact value passed to the
>> driver, if possible, and change the powersave and performance governors to have
>> that flag set.
>>
>> The last patch makes intel_pstate take that flag into account when programming
>> the HWP Request MSR.
> 
> The v3 simply uses different names for the new governor flags.

Thank you.

I tested v2, with positive results, as reported for v1. I do not have time to
re-test v3.

My input is to also default this flag to be set for the userspace and ondemand governors.

userspace: I tested with and without this flag set, and the flag is needed if
the user expects the scaling_setspeed to be enforced.
Disclaimer: I don't normally actually use the userspace governor.

ondemand: from my tests, the ondemand response more closely mimics acpi-ondemand with the flag set.
Power consumption has been better for the limited testing done.
However, it is also a function of work/sleep frequency for periodic workflows and a function of
INTEL_CPUFREQ_TRANSITION_DELAY_HWP. I am saying that my ability to support the suggestion to default
to setting the flag is a little weak.

... Doug