LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Huang Rui <ray.huang@amd.com>
To: Shuah Khan <skhan@linuxfoundation.org>
Cc: "Rafael J . Wysocki" <rafael.j.wysocki@intel.com>,
	Viresh Kumar <viresh.kumar@linaro.org>,
	Borislav Petkov <bp@suse.de>, Ingo Molnar <mingo@kernel.org>,
	"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
	"Sharma, Deepak" <Deepak.Sharma@amd.com>,
	"Deucher, Alexander" <Alexander.Deucher@amd.com>,
	"Limonciello, Mario" <Mario.Limonciello@amd.com>,
	"Fontenot, Nathan" <Nathan.Fontenot@amd.com>,
	"Su, Jinzhou (Joe)" <Jinzhou.Su@amd.com>,
	"Du, Xiaojian" <Xiaojian.Du@amd.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"x86@kernel.org" <x86@kernel.org>
Subject: Re: [PATCH 16/19] cpupower: enable boost state support for amd-pstate module
Date: Thu, 16 Sep 2021 17:27:35 +0800	[thread overview]
Message-ID: <20210916092735.GB3755511@hr-amd> (raw)
In-Reply-To: <ed2906aa-d898-9d3a-ed04-7ad0ecc51f96@linuxfoundation.org>

On Fri, Sep 10, 2021 at 06:42:42AM +0800, Shuah Khan wrote:
> On 9/8/21 8:59 AM, Huang Rui wrote:
> > The AMD P-state boost API is different from ACPI hardware P-states, so
> > implement the support for amd-pstate kernel module.
> > 
> 
> This commit log doesn't make sense. If these sysfs entries are used
> for amd-pstate kernel module, why are they defined here.
> 
> Describe how these are used and the relationship between these defines
> and the amd-pstate kernel module

Will refine the commit log in V2.

> 
> > Signed-off-by: Huang Rui <ray.huang@amd.com>
> > ---
> >   tools/power/cpupower/lib/cpufreq.c        | 20 ++++++++++++++++++++
> >   tools/power/cpupower/lib/cpufreq.h        |  3 +++
> >   tools/power/cpupower/utils/helpers/misc.c |  7 +++++++
> >   3 files changed, 30 insertions(+)
> > 
> > diff --git a/tools/power/cpupower/lib/cpufreq.c b/tools/power/cpupower/lib/cpufreq.c
> > index 3f92ddadaad2..37da87bdcfb1 100644
> > --- a/tools/power/cpupower/lib/cpufreq.c
> > +++ b/tools/power/cpupower/lib/cpufreq.c
> > @@ -790,3 +790,23 @@ unsigned long cpufreq_get_transitions(unsigned int cpu)
> >   {
> >   	return sysfs_cpufreq_get_one_value(cpu, STATS_NUM_TRANSITIONS);
> >   }
> > +
> > +int amd_pstate_boost_support(unsigned int cpu)
> > +{
> > +	unsigned int highest_perf, nominal_perf;
> > +
> > +	highest_perf = sysfs_cpufreq_get_one_value(cpu, AMD_PSTATE_HIGHEST_PERF);
> > +	nominal_perf = sysfs_cpufreq_get_one_value(cpu, AMD_PSTATE_NOMINAL_PERF);
> > +
> > +	return highest_perf > nominal_perf ? 1 : 0;
> > +}
> > +
> > +int amd_pstate_boost_enabled(unsigned int cpu)
> > +{
> > +	unsigned int cpuinfo_max, amd_pstate_max;
> > +
> > +	cpuinfo_max = sysfs_cpufreq_get_one_value(cpu, CPUINFO_MAX_FREQ);
> > +	amd_pstate_max = sysfs_cpufreq_get_one_value(cpu, AMD_PSTATE_MAX_FREQ);
> > +
> > +	return cpuinfo_max == amd_pstate_max ? 1 : 0;
> > +}
> 
> Why are these amd specific routines added to common file.
> Why not add them to tools/power/cpupower/utils/helpers/amd.c?

You're right. As mentioned at last mail, I move all the amd_pstate* from
lib/cpufreq.c to utils/helpers/amd.c

> 
> > diff --git a/tools/power/cpupower/lib/cpufreq.h b/tools/power/cpupower/lib/cpufreq.h
> > index 95f4fd9e2656..d54d02a7a4f4 100644
> > --- a/tools/power/cpupower/lib/cpufreq.h
> > +++ b/tools/power/cpupower/lib/cpufreq.h
> > @@ -203,6 +203,9 @@ int cpufreq_modify_policy_governor(unsigned int cpu, char *governor);
> >   int cpufreq_set_frequency(unsigned int cpu,
> >   				unsigned long target_frequency);
> >   
> > +int amd_pstate_boost_support(unsigned int cpu);
> > +int amd_pstate_boost_enabled(unsigned int cpu);
> > +
> >   #ifdef __cplusplus
> >   }
> >   #endif
> > diff --git a/tools/power/cpupower/utils/helpers/misc.c b/tools/power/cpupower/utils/helpers/misc.c
> > index 07d80775fb68..aba979320760 100644
> > --- a/tools/power/cpupower/utils/helpers/misc.c
> > +++ b/tools/power/cpupower/utils/helpers/misc.c
> > @@ -10,6 +10,7 @@
> >   #if defined(__i386__) || defined(__x86_64__)
> >   
> >   #include "cpupower_intern.h"
> > +#include "cpufreq.h"
> >   
> >   #define MSR_AMD_HWCR	0xc0010015
> >   
> > @@ -39,6 +40,12 @@ int cpufreq_has_boost_support(unsigned int cpu, int *support, int *active,
> >   	
> 
> This logic here calls amd_pci_get_num_boost_states() ---
> There is another routine called decode_pstates() in
> tools/power/cpupower/utils/helpers/amd.c
> 

The decode_pstates() is for legacy ACPI hardware Pstates (AMD only has 3),
but new amd_pstate function supports the finer grain frequency range. It's
the different hardware design. So we don't have the pstate number anymore.

> 		if (ret)
> >   				return ret;
> >   		}
> > +	} if ((cpupower_cpu_info.caps & CPUPOWER_CAP_AMD_PSTATE) &&
> > +	      amd_pstate_boost_support(cpu)) {
> 
> Coupled with the above routines, the naming amd_pstate_boost_support()
> is rather confusing.
> 
> Also why is this amd_pstate_boost_support() added to
> > +		*support = 1;
> > +
> > +		if (amd_pstate_boost_enabled(cpu))
> > +			*active = 1;

OK, yes, it can be merged in one function here. Will update this in V2.

If the boost is not enabled, the maximum perf will be the normal perf
(similiar with P0 before).

Thanks,
Ray

  reply	other threads:[~2021-09-16  9:27 UTC|newest]

Thread overview: 59+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-08 14:59 [PATCH 00/19] cpufreq: introduce a new AMD CPU frequency control mechanism Huang Rui
2021-09-08 14:59 ` [PATCH 01/19] x86/cpufreatures: add AMD CPPC extension feature flag Huang Rui
2021-09-08 20:00   ` Shuah Khan
2021-09-09  9:45     ` Huang Rui
2021-09-09 17:58   ` Borislav Petkov
2021-09-13  9:48     ` Huang Rui
2021-09-13 13:04       ` Borislav Petkov
2021-09-16  9:30         ` Huang Rui
2021-09-08 14:59 ` [PATCH 02/19] x86/msr: add AMD CPPC MSR definitions Huang Rui
2021-09-08 14:59 ` [PATCH 03/19] ACPI: CPPC: add cppc enable register function Huang Rui
2021-09-08 19:14   ` Fontenot, Nathan
2021-09-09  9:50     ` Huang Rui
2021-09-09  0:21   ` Shuah Khan
2021-09-09  9:58     ` Huang Rui
2021-09-08 14:59 ` [PATCH 04/19] cpufreq: amd: introduce a new amd pstate driver to support future processors Huang Rui
2021-09-09 15:01   ` Peter Zijlstra
2021-09-13  8:11     ` Huang Rui
2021-09-13  8:56       ` Peter Zijlstra
2021-09-13 10:54         ` Huang Rui
2021-09-13 11:56           ` Peter Zijlstra
2021-09-16 10:09             ` Huang Rui
2021-09-16 11:19               ` Rafael J. Wysocki
2021-09-17  3:41                 ` Huang Rui
2021-09-09 15:03   ` Peter Zijlstra
2021-09-13 11:55     ` Huang Rui
2021-09-09 19:31   ` Fontenot, Nathan
2021-09-13 11:18     ` Huang Rui
2021-09-08 14:59 ` [PATCH 05/19] cpufreq: amd: add fast switch function for amd-pstate module Huang Rui
2021-09-08 14:59 ` [PATCH 06/19] cpufreq: amd: add acpi cppc function as the backend for legacy processors Huang Rui
2021-09-08 14:59 ` [PATCH 07/19] cpufreq: amd: add trace for amd-pstate module Huang Rui
2021-09-08 14:59 ` [PATCH 08/19] cpufreq: amd: add boost mode support for amd-pstate Huang Rui
2021-09-08 18:24   ` Fontenot, Nathan
2021-09-09 10:12     ` Huang Rui
2021-09-08 14:59 ` [PATCH 09/19] cpufreq: amd: add amd-pstate checking support check attribute Huang Rui
2021-09-08 14:59 ` [PATCH 10/19] cpufreq: amd: add amd-pstate frequencies attributes Huang Rui
2021-09-08 18:13   ` Fontenot, Nathan
2021-09-08 14:59 ` [PATCH 11/19] cpufreq: amd: add amd-pstate performance attributes Huang Rui
2021-09-08 18:20   ` Fontenot, Nathan
2021-09-08 14:59 ` [PATCH 12/19] cpupower: add AMD P-state capability flag Huang Rui
2021-09-08 14:59 ` [PATCH 13/19] cpupower: add the function to check amd-pstate enabled Huang Rui
2021-09-09 22:16   ` Shuah Khan
2021-09-13 11:29     ` Huang Rui
2021-09-08 14:59 ` [PATCH 14/19] cpupower: initial AMD P-state capability Huang Rui
2021-09-09 22:16   ` Shuah Khan
2021-09-13 12:58     ` Huang Rui
2021-09-08 14:59 ` [PATCH 15/19] cpupower: add amd-pstate sysfs entries into libcpufreq Huang Rui
2021-09-09 22:26   ` Shuah Khan
2021-09-16  8:47     ` Huang Rui
2021-09-08 14:59 ` [PATCH 16/19] cpupower: enable boost state support for amd-pstate module Huang Rui
2021-09-08 17:32   ` Fontenot, Nathan
2021-09-09  9:59     ` Huang Rui
2021-09-09 22:42   ` Shuah Khan
2021-09-16  9:27     ` Huang Rui [this message]
2021-09-08 14:59 ` [PATCH 17/19] cpupower: add amd-pstate get data function to query the info Huang Rui
2021-09-09 22:45   ` Shuah Khan
2021-09-08 15:00 ` [PATCH 18/19] cpupower: print amd-pstate information on cpupower Huang Rui
2021-09-09 22:46   ` Shuah Khan
2021-09-16  9:29     ` Huang Rui
2021-09-08 15:00 ` [PATCH 19/19] Documentation: amd-pstate: add amd-pstate driver introduction Huang Rui

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210916092735.GB3755511@hr-amd \
    --to=ray.huang@amd.com \
    --cc=Alexander.Deucher@amd.com \
    --cc=Deepak.Sharma@amd.com \
    --cc=Jinzhou.Su@amd.com \
    --cc=Mario.Limonciello@amd.com \
    --cc=Nathan.Fontenot@amd.com \
    --cc=Xiaojian.Du@amd.com \
    --cc=bp@suse.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=mingo@kernel.org \
    --cc=rafael.j.wysocki@intel.com \
    --cc=skhan@linuxfoundation.org \
    --cc=viresh.kumar@linaro.org \
    --cc=x86@kernel.org \
    --subject='Re: [PATCH 16/19] cpupower: enable boost state support for amd-pstate module' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).