Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756470AbbGQHK6 (ORCPT ); Fri, 17 Jul 2015 03:10:58 -0400 Received: from mail-la0-f52.google.com ([209.85.215.52]:36589 "EHLO mail-la0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752076AbbGQHK5 convert rfc822-to-8bit (ORCPT ); Fri, 17 Jul 2015 03:10:57 -0400 MIME-Version: 1.0 In-Reply-To: <55A899E4.4030708@oracle.com> References: <20150716181706.6500.64386.stgit@buzz> <55A899E4.4030708@oracle.com> Date: Fri, 17 Jul 2015 10:10:54 +0300 Message-ID: Subject: Re: [PATCH RFC] intel_pstate: play well with frequency limits set by acpi From: Konstantin Khlebnikov To: ethan zhao Cc: Konstantin Khlebnikov , linux-pm@vger.kernel.org, "Rafael J. Wysocki" , Linux Kernel Mailing List , linux-acpi@vger.kernel.org, Kristen Carlson Accardi , Len Brown Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7111 Lines: 186 On Fri, Jul 17, 2015 at 9:00 AM, ethan zhao wrote: > > On 2015/7/17 2:17, Konstantin Khlebnikov wrote: >> >> IPMI can control CPU P-states remotely: configuration is reported via >> common ACPI interface (_PPC/_PSS/etc). This patch adds required minimal >> support in intel_pstate to receive and use these P-state limits. >> >> * ignore limit of top state in _PPC: it lower than turbo boost frequency >> * register intel_pstate in acpi-processor to get states from _PSS >> * link acpi_processor_get_bios_limit: this adds attribute "bios_limit" >> >> Signed-off-by: Konstantin Khlebnikov >> --- >> drivers/acpi/processor_perflib.c | 3 +- >> drivers/cpufreq/intel_pstate.c | 57 >> ++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 59 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/acpi/processor_perflib.c >> b/drivers/acpi/processor_perflib.c >> index cfc8aba72f86..781e328c9d5f 100644 >> --- a/drivers/acpi/processor_perflib.c >> +++ b/drivers/acpi/processor_perflib.c >> @@ -98,7 +98,8 @@ static int acpi_processor_ppc_notifier(struct >> notifier_block *nb, >> ppc = (unsigned int)pr->performance_platform_limit; >> - if (ppc >= pr->performance->state_count) >> + /* Ignore limit of top state: it lower than turbo boost frequency >> */ >> + if (!ppc || ppc >= pr->performance->state_count) > > Perhaps the !ppc is wrong if we check it against ACPI spec. > Zero value of ppc means: > > "0 – States 0 through Nth state are available (all states available)" Yep, also states are ordered by power - state0 must must have highest performance. This code below clamps range of available frequences to 0.._PSS[_PPC].frequency. So if _PPC = 0 then there should be no limit but then turbo is enabled frequency of state0 is actually could be lower than effective turbo frequency: for hardware I have it's _PSS[0] = 2601 Mhz, PSS[1] = 2600 Mhz while turbo state is 3400 Mhz. > > >> goto out; >> cpufreq_verify_within_limits(policy, 0, >> diff --git a/drivers/cpufreq/intel_pstate.c >> b/drivers/cpufreq/intel_pstate.c >> index 15ada47bb720..4a34ddf4fa73 100644 >> --- a/drivers/cpufreq/intel_pstate.c >> +++ b/drivers/cpufreq/intel_pstate.c >> @@ -26,6 +26,7 @@ >> #include >> #include >> #include >> +#include >> #include >> #include >> @@ -113,6 +114,9 @@ struct cpudata { >> u64 prev_mperf; >> u64 prev_tsc; >> struct sample sample; >> +#ifdef CONFIG_ACPI_PROCESSOR >> + struct acpi_processor_performance acpi_data; >> +#endif >> }; >> static struct cpudata **all_cpu_data; >> @@ -145,6 +149,7 @@ static int hwp_active; >> struct perf_limits { >> int no_turbo; >> + int no_acpi; >> int turbo_disabled; >> int max_perf_pct; >> int min_perf_pct; >> @@ -158,6 +163,7 @@ struct perf_limits { >> static struct perf_limits limits = { >> .no_turbo = 0, >> + .no_acpi = !IS_ENABLED(CONFIG_ACPI_PROCESSOR), >> .turbo_disabled = 0, >> .max_perf_pct = 100, >> .max_perf = int_tofp(1), >> @@ -449,6 +455,18 @@ static ssize_t store_min_perf_pct(struct kobject *a, >> struct attribute *b, >> return count; >> } >> +static ssize_t store_no_acpi(struct kobject *a, struct attribute *b, >> + const char *buf, size_t count) >> +{ >> +#ifdef CONFIG_ACPI_PROCESSOR >> + return kstrtouint(buf, 0, &limits.no_acpi) ?: count; >> +#else >> + return -ENODEV; >> +#endif >> +} >> +show_one(no_acpi, no_acpi); >> +define_one_global_rw(no_acpi); >> + >> show_one(max_perf_pct, max_perf_pct); >> show_one(min_perf_pct, min_perf_pct); >> @@ -460,6 +478,7 @@ define_one_global_ro(num_pstates); >> static struct attribute *intel_pstate_attributes[] = { >> &no_turbo.attr, >> + &no_acpi.attr, >> &max_perf_pct.attr, >> &min_perf_pct.attr, >> &turbo_pct.attr, >> @@ -1049,6 +1068,38 @@ static int intel_pstate_cpu_init(struct >> cpufreq_policy *policy) >> policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL; >> cpumask_set_cpu(policy->cpu, policy->cpus); >> +#ifdef CONFIG_ACPI_PROCESSOR >> + if (!limits.no_acpi) { >> + /* >> + * Minimum necessary to get acpi_processor_ppc_notifier() >> and >> + * acpi_processor_get_bios_limit() working. >> + */ >> + if (!zalloc_cpumask_var(&cpu->acpi_data.shared_cpu_map, >> + GFP_KERNEL)) >> + rc = -ENOMEM; >> + else >> + rc = acpi_processor_register_performance( >> + &cpu->acpi_data, policy->cpu); >> + if (rc) { >> + pr_err("intel_pstate: acpi init failed: %d\n", >> rc); >> + free_cpumask_var(cpu->acpi_data.shared_cpu_map); >> + limits.no_acpi = 1; >> + } >> + } >> +#endif >> + return 0; >> +} >> + >> +static int intel_pstate_cpu_exit(struct cpufreq_policy *policy) >> +{ >> +#ifdef CONFIG_ACPI_PROCESSOR >> + struct cpudata *cpu = all_cpu_data[policy->cpu]; >> + >> + if (cpu->acpi_data.state_count) >> + acpi_processor_unregister_performance(&cpu->acpi_data, >> + policy->cpu); >> + free_cpumask_var(cpu->acpi_data.shared_cpu_map); >> +#endif >> return 0; >> } >> @@ -1057,7 +1108,11 @@ static struct cpufreq_driver intel_pstate_driver >> = { >> .verify = intel_pstate_verify_policy, >> .setpolicy = intel_pstate_set_policy, >> .get = intel_pstate_get, >> +#ifdef CONFIG_ACPI_PROCESSOR >> + .bios_limit = acpi_processor_get_bios_limit, >> +#endif >> .init = intel_pstate_cpu_init, >> + .exit = intel_pstate_cpu_exit, >> .stop_cpu = intel_pstate_stop_cpu, >> .name = "intel_pstate", >> }; >> @@ -1286,6 +1341,8 @@ static int __init intel_pstate_setup(char *str) >> force_load = 1; >> if (!strcmp(str, "hwp_only")) >> hwp_only = 1; >> + if (!strcmp(str, "no_acpi")) >> + limits.no_acpi = 1; >> return 0; >> } >> early_param("intel_pstate", intel_pstate_setup); >> > > > Thanks, > Ethan > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/