* [PATCH v2] cpufreq: intel_pstate: Set EPP/EPB to 0 in performance mode
@ 2016-11-22 23:44 Srinivas Pandruvada
2016-11-23 0:42 ` Rafael J. Wysocki
0 siblings, 1 reply; 8+ messages in thread
From: Srinivas Pandruvada @ 2016-11-22 23:44 UTC (permalink / raw)
To: lenb, rjw; +Cc: linux-pm, Srinivas Pandruvada
When user has selected performance policy, then set the EPP (Energy
Performance Preference) or EPB (Energy Performance Bias) to maximum
performance mode.
Also when user switch back to powersave, then restore EPP/EPB to last
EPP/EPB value before entering performance mode. If user has not changed
EPP/EPB manually then it will be power on default value.
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
v2:
Save EPP/EPB when policy is switched to performance and restore
on entering powersave policy, when EPP/EPB is still 0.
drivers/cpufreq/intel_pstate.c | 82 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 82 insertions(+)
diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index 72e8bbc..8e7ceef 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -243,6 +243,8 @@ struct perf_limits {
* when per cpu controls are enforced
* @acpi_perf_data: Stores ACPI perf information read from _PSS
* @valid_pss_table: Set to true for valid ACPI _PSS entries found
+ * @epp_saved: Last saved HWP energy performance preference
+ * or energy performance bias
*
* This structure stores per CPU instance data for all CPUs.
*/
@@ -270,6 +272,7 @@ struct cpudata {
bool valid_pss_table;
#endif
unsigned int iowait_boost;
+ int epp_saved;
};
static struct cpudata **all_cpu_data;
@@ -568,6 +571,48 @@ static inline void update_turbo_state(void)
cpu->pstate.max_pstate == cpu->pstate.turbo_pstate);
}
+static int intel_pstate_get_epb(struct cpudata *cpu_data)
+{
+ u64 epb;
+ int ret;
+
+ if (!static_cpu_has(X86_FEATURE_EPB))
+ return -ENXIO;
+
+ ret = rdmsrl_on_cpu(cpu_data->cpu, MSR_IA32_ENERGY_PERF_BIAS, &epb);
+ if (ret)
+ return ret;
+
+ return (int)(epb & 0x0f);
+}
+
+static int intel_pstate_get_epp(struct cpudata *cpu_data, u64 hwp_req_data)
+{
+ int epp;
+
+ if (static_cpu_has(X86_FEATURE_HWP_EPP))
+ epp = (hwp_req_data >> 24) & 0xff;
+ else
+ /* When there is no EPP present, HWP uses EPB settings */
+ epp = intel_pstate_get_epb(cpu_data);
+
+ return epp;
+}
+
+static void intel_pstate_set_epb(int cpu, int pref)
+{
+ u64 epb;
+
+ if (!static_cpu_has(X86_FEATURE_EPB))
+ return;
+
+ if (rdmsrl_on_cpu(cpu, MSR_IA32_ENERGY_PERF_BIAS, &epb))
+ return;
+
+ epb = (epb & ~0x0f) | pref;
+ wrmsrl_on_cpu(cpu, MSR_IA32_ENERGY_PERF_BIAS, epb);
+}
+
static void intel_pstate_hwp_set(const struct cpumask *cpumask)
{
int min, hw_min, max, hw_max, cpu, range, adj_range;
@@ -576,6 +621,8 @@ static void intel_pstate_hwp_set(const struct cpumask *cpumask)
for_each_cpu(cpu, cpumask) {
int max_perf_pct, min_perf_pct;
+ struct cpudata *cpu_data = all_cpu_data[cpu];
+ int epp;
if (per_cpu_limits)
perf_limits = all_cpu_data[cpu]->perf_limits;
@@ -604,6 +651,41 @@ static void intel_pstate_hwp_set(const struct cpumask *cpumask)
value &= ~HWP_MAX_PERF(~0L);
value |= HWP_MAX_PERF(max);
+ if (cpu_data->policy == CPUFREQ_POLICY_PERFORMANCE) {
+ epp = intel_pstate_get_epp(cpu_data, value);
+ if (epp)
+ cpu_data->epp_saved = epp;
+
+ /* If EPP read was failed, then don't try to write */
+ if (epp < 0)
+ goto skip_epp;
+
+ epp = 0;
+ } else {
+ /* skip setting EPP, when saved value is invalid */
+ if (cpu_data->epp_saved < 0)
+ goto skip_epp;
+
+ /*
+ * No need to restore EPP when it is not zero. This
+ * means:
+ * - Policy is not changed
+ * - user has manually changed
+ * - Error reading EPB
+ */
+ epp = intel_pstate_get_epp(cpu_data, value);
+ if (epp)
+ goto skip_epp;
+ else
+ epp = cpu_data->epp_saved;
+ }
+ if (static_cpu_has(X86_FEATURE_HWP_EPP)) {
+ value &= ~GENMASK_ULL(31, 24);
+ value |= (u64)epp << 24;
+ } else {
+ intel_pstate_set_epb(cpu, epp);
+ }
+skip_epp:
wrmsrl_on_cpu(cpu, MSR_HWP_REQUEST, value);
}
}
--
2.7.4
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v2] cpufreq: intel_pstate: Set EPP/EPB to 0 in performance mode
2016-11-22 23:44 [PATCH v2] cpufreq: intel_pstate: Set EPP/EPB to 0 in performance mode Srinivas Pandruvada
@ 2016-11-23 0:42 ` Rafael J. Wysocki
2016-11-23 0:53 ` Rafael J. Wysocki
2016-11-23 1:16 ` Srinivas Pandruvada
0 siblings, 2 replies; 8+ messages in thread
From: Rafael J. Wysocki @ 2016-11-23 0:42 UTC (permalink / raw)
To: Srinivas Pandruvada; +Cc: Len Brown, Rafael J. Wysocki, Linux PM
On Wed, Nov 23, 2016 at 12:44 AM, Srinivas Pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
> When user has selected performance policy, then set the EPP (Energy
> Performance Preference) or EPB (Energy Performance Bias) to maximum
> performance mode.
> Also when user switch back to powersave, then restore EPP/EPB to last
> EPP/EPB value before entering performance mode. If user has not changed
> EPP/EPB manually then it will be power on default value.
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
> v2:
> Save EPP/EPB when policy is switched to performance and restore
> on entering powersave policy, when EPP/EPB is still 0.
>
> drivers/cpufreq/intel_pstate.c | 82 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 82 insertions(+)
>
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index 72e8bbc..8e7ceef 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -243,6 +243,8 @@ struct perf_limits {
> * when per cpu controls are enforced
> * @acpi_perf_data: Stores ACPI perf information read from _PSS
> * @valid_pss_table: Set to true for valid ACPI _PSS entries found
> + * @epp_saved: Last saved HWP energy performance preference
> + * or energy performance bias
> *
> * This structure stores per CPU instance data for all CPUs.
> */
> @@ -270,6 +272,7 @@ struct cpudata {
> bool valid_pss_table;
> #endif
> unsigned int iowait_boost;
> + int epp_saved;
> };
>
> static struct cpudata **all_cpu_data;
> @@ -568,6 +571,48 @@ static inline void update_turbo_state(void)
> cpu->pstate.max_pstate == cpu->pstate.turbo_pstate);
> }
>
> +static int intel_pstate_get_epb(struct cpudata *cpu_data)
> +{
> + u64 epb;
> + int ret;
> +
> + if (!static_cpu_has(X86_FEATURE_EPB))
> + return -ENXIO;
> +
> + ret = rdmsrl_on_cpu(cpu_data->cpu, MSR_IA32_ENERGY_PERF_BIAS, &epb);
> + if (ret)
> + return ret;
> +
> + return (int)(epb & 0x0f);
> +}
> +
> +static int intel_pstate_get_epp(struct cpudata *cpu_data, u64 hwp_req_data)
> +{
> + int epp;
> +
> + if (static_cpu_has(X86_FEATURE_HWP_EPP))
> + epp = (hwp_req_data >> 24) & 0xff;
> + else
> + /* When there is no EPP present, HWP uses EPB settings */
> + epp = intel_pstate_get_epb(cpu_data);
> +
> + return epp;
> +}
> +
> +static void intel_pstate_set_epb(int cpu, int pref)
> +{
> + u64 epb;
> +
> + if (!static_cpu_has(X86_FEATURE_EPB))
> + return;
> +
> + if (rdmsrl_on_cpu(cpu, MSR_IA32_ENERGY_PERF_BIAS, &epb))
> + return;
> +
> + epb = (epb & ~0x0f) | pref;
> + wrmsrl_on_cpu(cpu, MSR_IA32_ENERGY_PERF_BIAS, epb);
> +}
> +
> static void intel_pstate_hwp_set(const struct cpumask *cpumask)
> {
> int min, hw_min, max, hw_max, cpu, range, adj_range;
> @@ -576,6 +621,8 @@ static void intel_pstate_hwp_set(const struct cpumask *cpumask)
>
> for_each_cpu(cpu, cpumask) {
> int max_perf_pct, min_perf_pct;
> + struct cpudata *cpu_data = all_cpu_data[cpu];
> + int epp;
>
> if (per_cpu_limits)
> perf_limits = all_cpu_data[cpu]->perf_limits;
> @@ -604,6 +651,41 @@ static void intel_pstate_hwp_set(const struct cpumask *cpumask)
>
> value &= ~HWP_MAX_PERF(~0L);
> value |= HWP_MAX_PERF(max);
> + if (cpu_data->policy == CPUFREQ_POLICY_PERFORMANCE) {
> + epp = intel_pstate_get_epp(cpu_data, value);
> + if (epp)
> + cpu_data->epp_saved = epp;
I guess 0 is a valid value to save too, isn't it?
> +
> + /* If EPP read was failed, then don't try to write */
> + if (epp < 0)
> + goto skip_epp;
So I guess the above could be
epp = intel_pstate_get_epp(cpu_data, value);
/* If EPP read failed, then don't try to write */
if (epp < 0)
goto skip_epp;
cpu_data->epp_saved = epp;
> +
> + epp = 0;
> + } else {
> + /* skip setting EPP, when saved value is invalid */
> + if (cpu_data->epp_saved < 0)
> + goto skip_epp;
> +
> + /*
> + * No need to restore EPP when it is not zero. This
> + * means:
> + * - Policy is not changed
> + * - user has manually changed
> + * - Error reading EPB
> + */
> + epp = intel_pstate_get_epp(cpu_data, value);
> + if (epp)
> + goto skip_epp;
> + else
The "else" is not necessary, we won't reach the next statement if epp
is nonzero anyway.
> + epp = cpu_data->epp_saved;
> + }
> + if (static_cpu_has(X86_FEATURE_HWP_EPP)) {
> + value &= ~GENMASK_ULL(31, 24);
> + value |= (u64)epp << 24;
> + } else {
> + intel_pstate_set_epb(cpu, epp);
> + }
> +skip_epp:
> wrmsrl_on_cpu(cpu, MSR_HWP_REQUEST, value);
> }
> }
> --
Thanks,
Rafael
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2] cpufreq: intel_pstate: Set EPP/EPB to 0 in performance mode
2016-11-23 0:42 ` Rafael J. Wysocki
@ 2016-11-23 0:53 ` Rafael J. Wysocki
2016-11-23 1:20 ` Srinivas Pandruvada
2016-11-23 1:16 ` Srinivas Pandruvada
1 sibling, 1 reply; 8+ messages in thread
From: Rafael J. Wysocki @ 2016-11-23 0:53 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Srinivas Pandruvada, Len Brown, Rafael J. Wysocki, Linux PM
On Wed, Nov 23, 2016 at 1:42 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Wed, Nov 23, 2016 at 12:44 AM, Srinivas Pandruvada
> <srinivas.pandruvada@linux.intel.com> wrote:
>> When user has selected performance policy, then set the EPP (Energy
>> Performance Preference) or EPB (Energy Performance Bias) to maximum
>> performance mode.
>> Also when user switch back to powersave, then restore EPP/EPB to last
>> EPP/EPB value before entering performance mode. If user has not changed
>> EPP/EPB manually then it will be power on default value.
>>
>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>> ---
>> v2:
>> Save EPP/EPB when policy is switched to performance and restore
>> on entering powersave policy, when EPP/EPB is still 0.
>>
>> drivers/cpufreq/intel_pstate.c | 82 ++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 82 insertions(+)
>>
>> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
>> index 72e8bbc..8e7ceef 100644
>> --- a/drivers/cpufreq/intel_pstate.c
>> +++ b/drivers/cpufreq/intel_pstate.c
>> @@ -243,6 +243,8 @@ struct perf_limits {
>> * when per cpu controls are enforced
>> * @acpi_perf_data: Stores ACPI perf information read from _PSS
>> * @valid_pss_table: Set to true for valid ACPI _PSS entries found
>> + * @epp_saved: Last saved HWP energy performance preference
>> + * or energy performance bias
>> *
>> * This structure stores per CPU instance data for all CPUs.
>> */
>> @@ -270,6 +272,7 @@ struct cpudata {
>> bool valid_pss_table;
>> #endif
>> unsigned int iowait_boost;
>> + int epp_saved;
>> };
>>
>> static struct cpudata **all_cpu_data;
>> @@ -568,6 +571,48 @@ static inline void update_turbo_state(void)
>> cpu->pstate.max_pstate == cpu->pstate.turbo_pstate);
>> }
>>
>> +static int intel_pstate_get_epb(struct cpudata *cpu_data)
>> +{
>> + u64 epb;
>> + int ret;
>> +
>> + if (!static_cpu_has(X86_FEATURE_EPB))
>> + return -ENXIO;
>> +
>> + ret = rdmsrl_on_cpu(cpu_data->cpu, MSR_IA32_ENERGY_PERF_BIAS, &epb);
>> + if (ret)
>> + return ret;
>> +
>> + return (int)(epb & 0x0f);
>> +}
>> +
>> +static int intel_pstate_get_epp(struct cpudata *cpu_data, u64 hwp_req_data)
>> +{
>> + int epp;
>> +
>> + if (static_cpu_has(X86_FEATURE_HWP_EPP))
>> + epp = (hwp_req_data >> 24) & 0xff;
>> + else
>> + /* When there is no EPP present, HWP uses EPB settings */
>> + epp = intel_pstate_get_epb(cpu_data);
>> +
>> + return epp;
>> +}
>> +
>> +static void intel_pstate_set_epb(int cpu, int pref)
>> +{
>> + u64 epb;
>> +
>> + if (!static_cpu_has(X86_FEATURE_EPB))
>> + return;
>> +
>> + if (rdmsrl_on_cpu(cpu, MSR_IA32_ENERGY_PERF_BIAS, &epb))
>> + return;
>> +
>> + epb = (epb & ~0x0f) | pref;
>> + wrmsrl_on_cpu(cpu, MSR_IA32_ENERGY_PERF_BIAS, epb);
>> +}
>> +
>> static void intel_pstate_hwp_set(const struct cpumask *cpumask)
>> {
>> int min, hw_min, max, hw_max, cpu, range, adj_range;
>> @@ -576,6 +621,8 @@ static void intel_pstate_hwp_set(const struct cpumask *cpumask)
>>
>> for_each_cpu(cpu, cpumask) {
>> int max_perf_pct, min_perf_pct;
>> + struct cpudata *cpu_data = all_cpu_data[cpu];
>> + int epp;
>>
>> if (per_cpu_limits)
>> perf_limits = all_cpu_data[cpu]->perf_limits;
>> @@ -604,6 +651,41 @@ static void intel_pstate_hwp_set(const struct cpumask *cpumask)
>>
>> value &= ~HWP_MAX_PERF(~0L);
>> value |= HWP_MAX_PERF(max);
>> + if (cpu_data->policy == CPUFREQ_POLICY_PERFORMANCE) {
>> + epp = intel_pstate_get_epp(cpu_data, value);
>> + if (epp)
>> + cpu_data->epp_saved = epp;
>
> I guess 0 is a valid value to save too, isn't it?
>
>> +
>> + /* If EPP read was failed, then don't try to write */
>> + if (epp < 0)
>> + goto skip_epp;
>
> So I guess the above could be
>
> epp = intel_pstate_get_epp(cpu_data, value);
> /* If EPP read failed, then don't try to write */
> if (epp < 0)
> goto skip_epp;
>
> cpu_data->epp_saved = epp;
>
>> +
>> + epp = 0;
>> + } else {
>> + /* skip setting EPP, when saved value is invalid */
>> + if (cpu_data->epp_saved < 0)
>> + goto skip_epp;
>> +
This can be avoided too I suppose if epp_saved is initialized with the
initial value of the EPP.
Thanks,
Rafael
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2] cpufreq: intel_pstate: Set EPP/EPB to 0 in performance mode
2016-11-23 0:53 ` Rafael J. Wysocki
@ 2016-11-23 1:20 ` Srinivas Pandruvada
2016-11-23 1:41 ` Rafael J. Wysocki
0 siblings, 1 reply; 8+ messages in thread
From: Srinivas Pandruvada @ 2016-11-23 1:20 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Len Brown, Rafael J. Wysocki, Linux PM
On Wed, 2016-11-23 at 01:53 +0100, Rafael J. Wysocki wrote:
> On Wed, Nov 23, 2016 at 1:42 AM, Rafael J. Wysocki <rafael@kernel.org
> > wrote:
> >
> >
[...]
> > > invalid */
> > > + if (cpu_data->epp_saved < 0)
> > > + goto skip_epp;
> > > +
> This can be avoided too I suppose if epp_saved is initialized with
> the
> initial value of the EPP.
This is not only for initialization, but transient MSR read failures
later (which shouldn't happen, but for the completeness). So if you
weren't able to read for some unknown reason, then you shouldn't
restore.
Thanks,
Srinivas
>
> Thanks,
> Rafael
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm"
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] cpufreq: intel_pstate: Set EPP/EPB to 0 in performance mode
2016-11-23 1:20 ` Srinivas Pandruvada
@ 2016-11-23 1:41 ` Rafael J. Wysocki
0 siblings, 0 replies; 8+ messages in thread
From: Rafael J. Wysocki @ 2016-11-23 1:41 UTC (permalink / raw)
To: Srinivas Pandruvada; +Cc: Rafael J. Wysocki, Len Brown, Linux PM
On Tuesday, November 22, 2016 05:20:22 PM Srinivas Pandruvada wrote:
> On Wed, 2016-11-23 at 01:53 +0100, Rafael J. Wysocki wrote:
> > On Wed, Nov 23, 2016 at 1:42 AM, Rafael J. Wysocki <rafael@kernel.org
> > > wrote:
> > >
> > >
> [...]
>
> > > > invalid */
> > > > + if (cpu_data->epp_saved < 0)
> > > > + goto skip_epp;
> > > > +
> > This can be avoided too I suppose if epp_saved is initialized with
> > the
> > initial value of the EPP.
> This is not only for initialization, but transient MSR read failures
> later (which shouldn't happen, but for the completeness). So if you
> weren't able to read for some unknown reason, then you shouldn't
> restore.
OK, fair enough.
Thanks,
Rafael
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] cpufreq: intel_pstate: Set EPP/EPB to 0 in performance mode
2016-11-23 0:42 ` Rafael J. Wysocki
2016-11-23 0:53 ` Rafael J. Wysocki
@ 2016-11-23 1:16 ` Srinivas Pandruvada
2016-11-23 1:41 ` Rafael J. Wysocki
1 sibling, 1 reply; 8+ messages in thread
From: Srinivas Pandruvada @ 2016-11-23 1:16 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Len Brown, Rafael J. Wysocki, Linux PM
On Wed, 2016-11-23 at 01:42 +0100, Rafael J. Wysocki wrote:
> On Wed, Nov 23, 2016 at 12:44 AM, Srinivas Pandruvada
> <srinivas.pandruvada@linux.intel.com> wrote:
> >
> > When user has selected performance policy, then set the EPP (Energy
> > Performance Preference) or EPB (Energy Performance Bias) to maximum
> > performance mode.
> > Also when user switch back to powersave, then restore EPP/EPB to
> > last
> > EPP/EPB value before entering performance mode. If user has not
> > changed
> > EPP/EPB manually then it will be power on default value.
> >
> > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel
> > .com>
> > ---
> > v2:
> > Save EPP/EPB when policy is switched to performance and
> > restore
> > on entering powersave policy, when EPP/EPB is still 0.
> >
> > drivers/cpufreq/intel_pstate.c | 82
> > ++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 82 insertions(+)
> >
> > diff --git a/drivers/cpufreq/intel_pstate.c
> > b/drivers/cpufreq/intel_pstate.c
> > index 72e8bbc..8e7ceef 100644
> > --- a/drivers/cpufreq/intel_pstate.c
> > +++ b/drivers/cpufreq/intel_pstate.c
> > @@ -243,6 +243,8 @@ struct perf_limits {
> > * when per cpu controls are enforced
> > * @acpi_perf_data: Stores ACPI perf information read from _PSS
> > * @valid_pss_table: Set to true for valid ACPI _PSS entries
> > found
> > + * @epp_saved: Last saved HWP energy performance
> > preference
> > + * or energy performance bias
> > *
> > * This structure stores per CPU instance data for all CPUs.
> > */
> > @@ -270,6 +272,7 @@ struct cpudata {
> > bool valid_pss_table;
> > #endif
> > unsigned int iowait_boost;
> > + int epp_saved;
> > };
> >
> > static struct cpudata **all_cpu_data;
> > @@ -568,6 +571,48 @@ static inline void update_turbo_state(void)
> > cpu->pstate.max_pstate == cpu-
> > >pstate.turbo_pstate);
> > }
> >
> > +static int intel_pstate_get_epb(struct cpudata *cpu_data)
> > +{
> > + u64 epb;
> > + int ret;
> > +
> > + if (!static_cpu_has(X86_FEATURE_EPB))
> > + return -ENXIO;
> > +
> > + ret = rdmsrl_on_cpu(cpu_data->cpu,
> > MSR_IA32_ENERGY_PERF_BIAS, &epb);
> > + if (ret)
> > + return ret;
> > +
> > + return (int)(epb & 0x0f);
> > +}
> > +
> > +static int intel_pstate_get_epp(struct cpudata *cpu_data, u64
> > hwp_req_data)
> > +{
> > + int epp;
> > +
> > + if (static_cpu_has(X86_FEATURE_HWP_EPP))
> > + epp = (hwp_req_data >> 24) & 0xff;
> > + else
> > + /* When there is no EPP present, HWP uses EPB
> > settings */
> > + epp = intel_pstate_get_epb(cpu_data);
> > +
> > + return epp;
> > +}
> > +
> > +static void intel_pstate_set_epb(int cpu, int pref)
> > +{
> > + u64 epb;
> > +
> > + if (!static_cpu_has(X86_FEATURE_EPB))
> > + return;
> > +
> > + if (rdmsrl_on_cpu(cpu, MSR_IA32_ENERGY_PERF_BIAS, &epb))
> > + return;
> > +
> > + epb = (epb & ~0x0f) | pref;
> > + wrmsrl_on_cpu(cpu, MSR_IA32_ENERGY_PERF_BIAS, epb);
> > +}
> > +
> > static void intel_pstate_hwp_set(const struct cpumask *cpumask)
> > {
> > int min, hw_min, max, hw_max, cpu, range, adj_range;
> > @@ -576,6 +621,8 @@ static void intel_pstate_hwp_set(const struct
> > cpumask *cpumask)
> >
> > for_each_cpu(cpu, cpumask) {
> > int max_perf_pct, min_perf_pct;
> > + struct cpudata *cpu_data = all_cpu_data[cpu];
> > + int epp;
> >
> > if (per_cpu_limits)
> > perf_limits = all_cpu_data[cpu]-
> > >perf_limits;
> > @@ -604,6 +651,41 @@ static void intel_pstate_hwp_set(const struct
> > cpumask *cpumask)
> >
> > value &= ~HWP_MAX_PERF(~0L);
> > value |= HWP_MAX_PERF(max);
> > + if (cpu_data->policy == CPUFREQ_POLICY_PERFORMANCE)
> > {
> > + epp = intel_pstate_get_epp(cpu_data,
> > value);
> > + if (epp)
> > + cpu_data->epp_saved = epp;
> I guess 0 is a valid value to save too, isn't it?
Yes.
>
> >
> > +
> > + /* If EPP read was failed, then don't try
> > to write */
> > + if (epp < 0)
> > + goto skip_epp;
> So I guess the above could be
>
> epp = intel_pstate_get_epp(cpu_data, value);
> /* If EPP read failed, then don't try to write
> */
> if (epp < 0)
> goto skip_epp;
>
> cpu_data->epp_saved = epp;
>
No. We shouldn't store values to cpu_data->epp_saved when epp = 0,
since cpu_data->policy == CPUFREQ_POLICY_PERFORMANCE is called multiple
times one after other (not a one time call only after policy switch),
if you do that cpu_data->epp_saved is overwritten immediately with 0.
So when you actually switch to powersave policy you will restore 0.
> >
> > +
> > + epp = 0;
> > + } else {
> > + /* skip setting EPP, when saved value is
> > invalid */
> > + if (cpu_data->epp_saved < 0)
> > + goto skip_epp;
> > +
> > + /*
> > + * No need to restore EPP when it is not
> > zero. This
> > + * means:
> > + * - Policy is not changed
> > + * - user has manually changed
> > + * - Error reading EPB
> > + */
> > + epp = intel_pstate_get_epp(cpu_data,
> > value);
> > + if (epp)
> > + goto skip_epp;
> > + else
> The "else" is not necessary, we won't reach the next statement if epp
> is nonzero anyway.
That is correct.
>
> >
> > + epp = cpu_data->epp_saved;
> > + }
> > + if (static_cpu_has(X86_FEATURE_HWP_EPP)) {
> > + value &= ~GENMASK_ULL(31, 24);
> > + value |= (u64)epp << 24;
> > + } else {
> > + intel_pstate_set_epb(cpu, epp);
> > + }
> > +skip_epp:
> > wrmsrl_on_cpu(cpu, MSR_HWP_REQUEST, value);
> > }
> > }
Thanks,
Srinivas
> > --
> Thanks,
> Rafael
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm"
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2] cpufreq: intel_pstate: Set EPP/EPB to 0 in performance mode
2016-11-23 1:16 ` Srinivas Pandruvada
@ 2016-11-23 1:41 ` Rafael J. Wysocki
2016-11-23 1:37 ` Srinivas Pandruvada
0 siblings, 1 reply; 8+ messages in thread
From: Rafael J. Wysocki @ 2016-11-23 1:41 UTC (permalink / raw)
To: Srinivas Pandruvada; +Cc: Rafael J. Wysocki, Len Brown, Linux PM
On Tuesday, November 22, 2016 05:16:02 PM Srinivas Pandruvada wrote:
> On Wed, 2016-11-23 at 01:42 +0100, Rafael J. Wysocki wrote:
> > On Wed, Nov 23, 2016 at 12:44 AM, Srinivas Pandruvada
> > <srinivas.pandruvada@linux.intel.com> wrote:
> > >
> > > When user has selected performance policy, then set the EPP (Energy
> > > Performance Preference) or EPB (Energy Performance Bias) to maximum
> > > performance mode.
> > > Also when user switch back to powersave, then restore EPP/EPB to
> > > last
> > > EPP/EPB value before entering performance mode. If user has not
> > > changed
> > > EPP/EPB manually then it will be power on default value.
> > >
> > > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel
> > > .com>
> > > ---
> > > v2:
> > > Save EPP/EPB when policy is switched to performance and
> > > restore
> > > on entering powersave policy, when EPP/EPB is still 0.
> > >
> > > drivers/cpufreq/intel_pstate.c | 82
> > > ++++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 82 insertions(+)
> > >
> > > diff --git a/drivers/cpufreq/intel_pstate.c
> > > b/drivers/cpufreq/intel_pstate.c
> > > index 72e8bbc..8e7ceef 100644
> > > --- a/drivers/cpufreq/intel_pstate.c
> > > +++ b/drivers/cpufreq/intel_pstate.c
> > > @@ -243,6 +243,8 @@ struct perf_limits {
> > > * when per cpu controls are enforced
> > > * @acpi_perf_data: Stores ACPI perf information read from _PSS
> > > * @valid_pss_table: Set to true for valid ACPI _PSS entries
> > > found
> > > + * @epp_saved: Last saved HWP energy performance
> > > preference
> > > + * or energy performance bias
> > > *
> > > * This structure stores per CPU instance data for all CPUs.
> > > */
> > > @@ -270,6 +272,7 @@ struct cpudata {
> > > bool valid_pss_table;
> > > #endif
> > > unsigned int iowait_boost;
> > > + int epp_saved;
> > > };
> > >
> > > static struct cpudata **all_cpu_data;
> > > @@ -568,6 +571,48 @@ static inline void update_turbo_state(void)
> > > cpu->pstate.max_pstate == cpu-
> > > >pstate.turbo_pstate);
> > > }
> > >
> > > +static int intel_pstate_get_epb(struct cpudata *cpu_data)
> > > +{
> > > + u64 epb;
> > > + int ret;
> > > +
> > > + if (!static_cpu_has(X86_FEATURE_EPB))
> > > + return -ENXIO;
> > > +
> > > + ret = rdmsrl_on_cpu(cpu_data->cpu,
> > > MSR_IA32_ENERGY_PERF_BIAS, &epb);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + return (int)(epb & 0x0f);
> > > +}
> > > +
> > > +static int intel_pstate_get_epp(struct cpudata *cpu_data, u64
> > > hwp_req_data)
> > > +{
> > > + int epp;
> > > +
> > > + if (static_cpu_has(X86_FEATURE_HWP_EPP))
> > > + epp = (hwp_req_data >> 24) & 0xff;
> > > + else
> > > + /* When there is no EPP present, HWP uses EPB
> > > settings */
> > > + epp = intel_pstate_get_epb(cpu_data);
> > > +
> > > + return epp;
> > > +}
> > > +
> > > +static void intel_pstate_set_epb(int cpu, int pref)
> > > +{
> > > + u64 epb;
> > > +
> > > + if (!static_cpu_has(X86_FEATURE_EPB))
> > > + return;
> > > +
> > > + if (rdmsrl_on_cpu(cpu, MSR_IA32_ENERGY_PERF_BIAS, &epb))
> > > + return;
> > > +
> > > + epb = (epb & ~0x0f) | pref;
> > > + wrmsrl_on_cpu(cpu, MSR_IA32_ENERGY_PERF_BIAS, epb);
> > > +}
> > > +
> > > static void intel_pstate_hwp_set(const struct cpumask *cpumask)
> > > {
> > > int min, hw_min, max, hw_max, cpu, range, adj_range;
> > > @@ -576,6 +621,8 @@ static void intel_pstate_hwp_set(const struct
> > > cpumask *cpumask)
> > >
> > > for_each_cpu(cpu, cpumask) {
> > > int max_perf_pct, min_perf_pct;
> > > + struct cpudata *cpu_data = all_cpu_data[cpu];
> > > + int epp;
> > >
> > > if (per_cpu_limits)
> > > perf_limits = all_cpu_data[cpu]-
> > > >perf_limits;
> > > @@ -604,6 +651,41 @@ static void intel_pstate_hwp_set(const struct
> > > cpumask *cpumask)
> > >
> > > value &= ~HWP_MAX_PERF(~0L);
> > > value |= HWP_MAX_PERF(max);
> > > + if (cpu_data->policy == CPUFREQ_POLICY_PERFORMANCE)
> > > {
> > > + epp = intel_pstate_get_epp(cpu_data,
> > > value);
> > > + if (epp)
> > > + cpu_data->epp_saved = epp;
> > I guess 0 is a valid value to save too, isn't it?
> Yes.
>
> >
> > >
> > > +
> > > + /* If EPP read was failed, then don't try
> > > to write */
> > > + if (epp < 0)
> > > + goto skip_epp;
> > So I guess the above could be
> >
> > epp = intel_pstate_get_epp(cpu_data, value);
> > /* If EPP read failed, then don't try to write
> > */
> > if (epp < 0)
> > goto skip_epp;
> >
> > cpu_data->epp_saved = epp;
> >
> No. We shouldn't store values to cpu_data->epp_saved when epp = 0,
> since cpu_data->policy == CPUFREQ_POLICY_PERFORMANCE is called multiple
> times one after other (not a one time call only after policy switch),
> if you do that cpu_data->epp_saved is overwritten immediately with 0.
> So when you actually switch to powersave policy you will restore 0.
I see.
But I think then epp_saved should be reset to 0 in the "powersave" branch
or a stale value may be restored from it at one point.
Thanks,
Rafael
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2] cpufreq: intel_pstate: Set EPP/EPB to 0 in performance mode
2016-11-23 1:41 ` Rafael J. Wysocki
@ 2016-11-23 1:37 ` Srinivas Pandruvada
0 siblings, 0 replies; 8+ messages in thread
From: Srinivas Pandruvada @ 2016-11-23 1:37 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Rafael J. Wysocki, Len Brown, Linux PM
On Wed, 2016-11-23 at 02:41 +0100, Rafael J. Wysocki wrote:
> On Tuesday, November 22, 2016 05:16:02 PM Srinivas Pandruvada wrote:
> >
> > On Wed, 2016-11-23 at 01:42 +0100, Rafael J. Wysocki wrote:
> > >
> > > On Wed, Nov 23, 2016 at 12:44 AM, Srinivas Pandruvada
> > > <srinivas.pandruvada@linux.intel.com> wrote:
> > > >
> > > >
> > > > When user has selected performance policy, then set the EPP
> > > > (Energy
> > > > Performance Preference) or EPB (Energy Performance Bias) to
> > > > maximum
> > > > performance mode.
> > > > Also when user switch back to powersave, then restore EPP/EPB
> > > > to
> > > > last
> > > > EPP/EPB value before entering performance mode. If user has not
> > > > changed
> > > > EPP/EPB manually then it will be power on default value.
> > > >
> > > > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.i
> > > > ntel
> > > > .com>
> > > > ---
> > > > v2:
> > > > Save EPP/EPB when policy is switched to performance and
> > > > restore
> > > > on entering powersave policy, when EPP/EPB is still 0.
> > > >
> > > > drivers/cpufreq/intel_pstate.c | 82
> > > > ++++++++++++++++++++++++++++++++++++++++++
> > > > 1 file changed, 82 insertions(+)
> > > >
> > > > diff --git a/drivers/cpufreq/intel_pstate.c
> > > > b/drivers/cpufreq/intel_pstate.c
> > > > index 72e8bbc..8e7ceef 100644
> > > > --- a/drivers/cpufreq/intel_pstate.c
> > > > +++ b/drivers/cpufreq/intel_pstate.c
> > > > @@ -243,6 +243,8 @@ struct perf_limits {
> > > > * when per cpu controls are enforced
> > > > * @acpi_perf_data: Stores ACPI perf information read from
> > > > _PSS
> > > > * @valid_pss_table: Set to true for valid ACPI _PSS entries
> > > > found
> > > > + * @epp_saved: Last saved HWP energy performance
> > > > preference
> > > > + * or energy performance bias
> > > > *
> > > > * This structure stores per CPU instance data for all CPUs.
> > > > */
> > > > @@ -270,6 +272,7 @@ struct cpudata {
> > > > bool valid_pss_table;
> > > > #endif
> > > > unsigned int iowait_boost;
> > > > + int epp_saved;
> > > > };
> > > >
> > > > static struct cpudata **all_cpu_data;
> > > > @@ -568,6 +571,48 @@ static inline void
> > > > update_turbo_state(void)
> > > > cpu->pstate.max_pstate == cpu-
> > > > >
> > > > > pstate.turbo_pstate);
> > > > }
> > > >
> > > > +static int intel_pstate_get_epb(struct cpudata *cpu_data)
> > > > +{
> > > > + u64 epb;
> > > > + int ret;
> > > > +
> > > > + if (!static_cpu_has(X86_FEATURE_EPB))
> > > > + return -ENXIO;
> > > > +
> > > > + ret = rdmsrl_on_cpu(cpu_data->cpu,
> > > > MSR_IA32_ENERGY_PERF_BIAS, &epb);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + return (int)(epb & 0x0f);
> > > > +}
> > > > +
> > > > +static int intel_pstate_get_epp(struct cpudata *cpu_data, u64
> > > > hwp_req_data)
> > > > +{
> > > > + int epp;
> > > > +
> > > > + if (static_cpu_has(X86_FEATURE_HWP_EPP))
> > > > + epp = (hwp_req_data >> 24) & 0xff;
> > > > + else
> > > > + /* When there is no EPP present, HWP uses EPB
> > > > settings */
> > > > + epp = intel_pstate_get_epb(cpu_data);
> > > > +
> > > > + return epp;
> > > > +}
> > > > +
> > > > +static void intel_pstate_set_epb(int cpu, int pref)
> > > > +{
> > > > + u64 epb;
> > > > +
> > > > + if (!static_cpu_has(X86_FEATURE_EPB))
> > > > + return;
> > > > +
> > > > + if (rdmsrl_on_cpu(cpu, MSR_IA32_ENERGY_PERF_BIAS,
> > > > &epb))
> > > > + return;
> > > > +
> > > > + epb = (epb & ~0x0f) | pref;
> > > > + wrmsrl_on_cpu(cpu, MSR_IA32_ENERGY_PERF_BIAS, epb);
> > > > +}
> > > > +
> > > > static void intel_pstate_hwp_set(const struct cpumask
> > > > *cpumask)
> > > > {
> > > > int min, hw_min, max, hw_max, cpu, range, adj_range;
> > > > @@ -576,6 +621,8 @@ static void intel_pstate_hwp_set(const
> > > > struct
> > > > cpumask *cpumask)
> > > >
> > > > for_each_cpu(cpu, cpumask) {
> > > > int max_perf_pct, min_perf_pct;
> > > > + struct cpudata *cpu_data = all_cpu_data[cpu];
> > > > + int epp;
> > > >
> > > > if (per_cpu_limits)
> > > > perf_limits = all_cpu_data[cpu]-
> > > > >
> > > > > perf_limits;
> > > > @@ -604,6 +651,41 @@ static void intel_pstate_hwp_set(const
> > > > struct
> > > > cpumask *cpumask)
> > > >
> > > > value &= ~HWP_MAX_PERF(~0L);
> > > > value |= HWP_MAX_PERF(max);
> > > > + if (cpu_data->policy ==
> > > > CPUFREQ_POLICY_PERFORMANCE)
> > > > {
> > > > + epp = intel_pstate_get_epp(cpu_data,
> > > > value);
> > > > + if (epp)
> > > > + cpu_data->epp_saved = epp;
> > > I guess 0 is a valid value to save too, isn't it?
> > Yes.
> >
> > >
> > >
> > > >
> > > >
> > > > +
> > > > + /* If EPP read was failed, then don't
> > > > try
> > > > to write */
> > > > + if (epp < 0)
> > > > + goto skip_epp;
> > > So I guess the above could be
> > >
> > > epp = intel_pstate_get_epp(cpu_data,
> > > value);
> > > /* If EPP read failed, then don't try to
> > > write
> > > */
> > > if (epp < 0)
> > > goto skip_epp;
> > >
> > > cpu_data->epp_saved = epp;
> > >
> > No. We shouldn't store values to cpu_data->epp_saved when epp = 0,
> > since cpu_data->policy == CPUFREQ_POLICY_PERFORMANCE is called
> > multiple
> > times one after other (not a one time call only after policy
> > switch),
> > if you do that cpu_data->epp_saved is overwritten immediately with
> > 0.
> > So when you actually switch to powersave policy you will restore 0.
> I see.
>
> But I think then epp_saved should be reset to 0 in the "powersave"
> branch
> or a stale value may be restored from it at one point.
I see the point, changing to performance mode using EPP change. I will
submit a change.
Thanks,
Srinivas
>
> Thanks,
> Rafael
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm"
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-11-23 1:43 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-22 23:44 [PATCH v2] cpufreq: intel_pstate: Set EPP/EPB to 0 in performance mode Srinivas Pandruvada
2016-11-23 0:42 ` Rafael J. Wysocki
2016-11-23 0:53 ` Rafael J. Wysocki
2016-11-23 1:20 ` Srinivas Pandruvada
2016-11-23 1:41 ` Rafael J. Wysocki
2016-11-23 1:16 ` Srinivas Pandruvada
2016-11-23 1:41 ` Rafael J. Wysocki
2016-11-23 1:37 ` Srinivas Pandruvada
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.