[PATCH 0/2] additional sysfs entries for CPPC

[PATCH 0/2] additional sysfs entries for CPPC

[Prashanth Prakash]

This patch-set adds few additional sysfs entries to expose the
performance capabilities of each CPU. The performance capabilities
include highest perf, lowest perf, nominal perf and lowest 
non-linear perf. See 8.4.7.1 for ACPI 6.1 spec for details on
these capabilities.

cppc_cpufreq driver operates in KHz scale whereas the delivered
performance computed in userspace will be in abstract CPPC scale, so
exposing perf capabilities should allow userspace to figure out the
conversion factor from CPPC scale to KHz.

Prashanth Prakash (2):
  ACPI / CPPC: read all perf caps in a single cppc read command
  ACPI / CPPC: add sysfs entries for CPPC perf capabilities

 drivers/acpi/cppc_acpi.c | 164 ++++++++++++++++++++++++++++++-----------------
 include/acpi/cppc_acpi.h |   3 +-
 2 files changed, 107 insertions(+), 60 deletions(-)

-- 
Qualcomm Datacenter Technologies on behalf of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 1/2] ACPI / CPPC: read all perf caps in a single cppc read command

[Prashanth Prakash]

CPPC performance capabilities do not change during runtime, so we can
read perf caps for all CPUs in a single CPPC read command and cache
them locally.

Signed-off-by: Prashanth Prakash <pprakash@codeaurora.org>
---
 drivers/acpi/cppc_acpi.c | 103 +++++++++++++++++++++++++++++++++--------------
 include/acpi/cppc_acpi.h |   1 +
 2 files changed, 73 insertions(+), 31 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 3ca0729..45f021a 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -92,6 +92,12 @@ struct cppc_pcc_data {
  */
 static DEFINE_PER_CPU(struct cpc_desc *, cpc_desc_ptr);
 
+/*
+ * The cppc_perf_caps structure contains the performance capabilities
+ * as described in section 8.4.7.1 of ACPI 6.1 spec
+ */
+static DEFINE_PER_CPU(struct cppc_perf_caps, cpc_perf_caps);
+
 /* pcc mapped address + header size + offset within PCC subspace */
 #define GET_PCC_VADDR(offs) (pcc_data.pcc_comm_addr + 0x8 + (offs))
 
@@ -971,49 +977,84 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val)
  */
 int cppc_get_perf_caps(int cpunum, struct cppc_perf_caps *perf_caps)
 {
-	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
-	struct cpc_register_resource *highest_reg, *lowest_reg, *ref_perf,
-								 *nom_perf;
-	u64 high, low, nom;
-	int ret = 0, regs_in_pcc = 0;
+	static bool initialized;
+	int ret = 0, i;
+	bool regs_in_pcc = false;
+	struct cppc_perf_caps *caps;
 
-	if (!cpc_desc) {
-		pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
-		return -ENODEV;
+	if (initialized) {
+		caps = &per_cpu(cpc_perf_caps, cpunum);
+
+		if (!caps->highest_perf || !caps->lowest_perf ||
+			!caps->nominal_perf || !caps->lowest_non_linear_perf)
+			return -EFAULT;
+
+		memcpy(perf_caps, caps, sizeof(struct cppc_perf_caps));
+		return 0;
 	}
 
-	highest_reg = &cpc_desc->cpc_regs[HIGHEST_PERF];
-	lowest_reg = &cpc_desc->cpc_regs[LOWEST_PERF];
-	ref_perf = &cpc_desc->cpc_regs[REFERENCE_PERF];
-	nom_perf = &cpc_desc->cpc_regs[NOMINAL_PERF];
+	/* Perf caps don't change over time, so read all of them at once */
+	for_each_possible_cpu(i) {
+		struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, i);
+		struct cpc_register_resource *highest_reg, *lowest_reg,
+			*nom_reg, *lowest_non_linear_reg;
+		u64 perf;
 
-	/* Are any of the regs PCC ?*/
-	if (CPC_IN_PCC(highest_reg) || CPC_IN_PCC(lowest_reg) ||
-		CPC_IN_PCC(ref_perf) || CPC_IN_PCC(nom_perf)) {
-		regs_in_pcc = 1;
-		down_write(&pcc_data.pcc_lock);
-		/* Ring doorbell once to update PCC subspace */
-		if (send_pcc_cmd(CMD_READ) < 0) {
-			ret = -EIO;
-			goto out_err;
+		if (!cpc_desc) {
+			pr_debug("No CPC descriptor for CPU:%d\n", i);
+			continue;
+		}
+
+		highest_reg = &cpc_desc->cpc_regs[HIGHEST_PERF];
+		lowest_reg = &cpc_desc->cpc_regs[LOWEST_PERF];
+		lowest_non_linear_reg = &cpc_desc->cpc_regs[LOW_NON_LINEAR_PERF];
+		nom_reg = &cpc_desc->cpc_regs[NOMINAL_PERF];
+
+		/* Are any of the regs in PCC ?*/
+		if (!regs_in_pcc) {
+			regs_in_pcc = CPC_IN_PCC(highest_reg) ||
+				CPC_IN_PCC(lowest_reg) ||
+				CPC_IN_PCC(lowest_non_linear_reg) ||
+				CPC_IN_PCC(nom_reg);
+			if (regs_in_pcc) {
+				down_write(&pcc_data.pcc_lock);
+				/* Skip if another CPU has completed init */
+				if (initialized)
+					goto out;
+
+				/* Ring doorbell once to update PCC subspace */
+				if (send_pcc_cmd(CMD_READ) < 0) {
+					ret = -EIO;
+					goto out;
+				}
+			}
 		}
-	}
 
-	cpc_read(cpunum, highest_reg, &high);
-	perf_caps->highest_perf = high;
+		caps = &per_cpu(cpc_perf_caps, i);
 
-	cpc_read(cpunum, lowest_reg, &low);
-	perf_caps->lowest_perf = low;
+		cpc_read(i, highest_reg, &perf);
+		caps->highest_perf = perf;
 
-	cpc_read(cpunum, nom_perf, &nom);
-	perf_caps->nominal_perf = nom;
+		cpc_read(i, lowest_reg, &perf);
+		caps->lowest_perf = perf;
 
-	if (!high || !low || !nom)
-		ret = -EFAULT;
+		cpc_read(i, nom_reg, &perf);
+		caps->nominal_perf = perf;
 
-out_err:
+		cpc_read(i, lowest_non_linear_reg, &perf);
+		caps->lowest_non_linear_perf = perf;
+	}
+
+	initialized = true;
+
+out:
 	if (regs_in_pcc)
 		up_write(&pcc_data.pcc_lock);
+
+	/* Copy over the per cpu data to perf_caps if init ws successful */
+	if (!ret)
+		ret = cppc_get_perf_caps(cpunum, perf_caps);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(cppc_get_perf_caps);
diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
index 427a7c3..3f64660 100644
--- a/include/acpi/cppc_acpi.h
+++ b/include/acpi/cppc_acpi.h
@@ -103,6 +103,7 @@ struct cppc_perf_caps {
 	u32 highest_perf;
 	u32 nominal_perf;
 	u32 lowest_perf;
+	u32 lowest_non_linear_perf;
 };
 
 struct cppc_perf_ctrls {
-- 
Qualcomm Datacenter Technologies on behalf of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 2/2] ACPI / CPPC: add sysfs entries for CPPC perf capabilities

[Prashanth Prakash]

Computed delivered performance using CPPC feedback counters will be
in the CPPC abstract scale, whereas cppc_cpufreq driver operates in
KHz scale. Exposing the CPPC performance capabilities (highest,
lowest, nominal, lowest non-linear) will allow userspace to figure
out the conversion factor from CPPC abstract scale to KHz.

Signed-off-by: Prashanth Prakash <pprakash@codeaurora.org>
---
 drivers/acpi/cppc_acpi.c | 61 ++++++++++++++++++++++++++----------------------
 include/acpi/cppc_acpi.h |  2 +-
 2 files changed, 34 insertions(+), 29 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 45f021a..3c6a035 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -138,49 +138,54 @@ struct cppc_attr {
 
 #define to_cpc_desc(a) container_of(a, struct cpc_desc, kobj)
 
+#define show_cppc_data(access_fn, struct_name, member_name)		\
+	static ssize_t show_##member_name(struct kobject *kobj,		\
+					struct attribute *attr,	char *buf) \
+	{								\
+		struct cpc_desc *cpc_ptr = to_cpc_desc(kobj);		\
+		struct struct_name st_name = {0};			\
+		int ret;						\
+									\
+		ret = access_fn(cpc_ptr->cpu_id, &st_name);		\
+		if (ret)						\
+			return ret;					\
+									\
+		return scnprintf(buf, PAGE_SIZE, "%llu\n",		\
+				(u64)st_name.member_name);		\
+	}								\
+	define_one_cppc_ro(member_name)
+
+show_cppc_data(cppc_get_perf_caps, cppc_perf_caps, highest_perf);
+show_cppc_data(cppc_get_perf_caps, cppc_perf_caps, lowest_perf);
+show_cppc_data(cppc_get_perf_caps, cppc_perf_caps, nominal_perf);
+show_cppc_data(cppc_get_perf_caps, cppc_perf_caps, lowest_non_linear_perf);
+show_cppc_data(cppc_get_perf_ctrs, cppc_perf_fb_ctrs, reference_perf);
+show_cppc_data(cppc_get_perf_ctrs, cppc_perf_fb_ctrs, wraparound_time);
+
 static ssize_t show_feedback_ctrs(struct kobject *kobj,
 		struct attribute *attr, char *buf)
 {
 	struct cpc_desc *cpc_ptr = to_cpc_desc(kobj);
 	struct cppc_perf_fb_ctrs fb_ctrs = {0};
+	int ret;
 
-	cppc_get_perf_ctrs(cpc_ptr->cpu_id, &fb_ctrs);
+	ret = cppc_get_perf_ctrs(cpc_ptr->cpu_id, &fb_ctrs);
+	if (ret)
+		return ret;
 
 	return scnprintf(buf, PAGE_SIZE, "ref:%llu del:%llu\n",
 			fb_ctrs.reference, fb_ctrs.delivered);
 }
 define_one_cppc_ro(feedback_ctrs);
 
-static ssize_t show_reference_perf(struct kobject *kobj,
-		struct attribute *attr, char *buf)
-{
-	struct cpc_desc *cpc_ptr = to_cpc_desc(kobj);
-	struct cppc_perf_fb_ctrs fb_ctrs = {0};
-
-	cppc_get_perf_ctrs(cpc_ptr->cpu_id, &fb_ctrs);
-
-	return scnprintf(buf, PAGE_SIZE, "%llu\n",
-			fb_ctrs.reference_perf);
-}
-define_one_cppc_ro(reference_perf);
-
-static ssize_t show_wraparound_time(struct kobject *kobj,
-				struct attribute *attr, char *buf)
-{
-	struct cpc_desc *cpc_ptr = to_cpc_desc(kobj);
-	struct cppc_perf_fb_ctrs fb_ctrs = {0};
-
-	cppc_get_perf_ctrs(cpc_ptr->cpu_id, &fb_ctrs);
-
-	return scnprintf(buf, PAGE_SIZE, "%llu\n", fb_ctrs.ctr_wrap_time);
-
-}
-define_one_cppc_ro(wraparound_time);
-
 static struct attribute *cppc_attrs[] = {
 	&feedback_ctrs.attr,
 	&reference_perf.attr,
 	&wraparound_time.attr,
+	&highest_perf.attr,
+	&lowest_perf.attr,
+	&lowest_non_linear_perf.attr,
+	&nominal_perf.attr,
 	NULL
 };
 
@@ -1124,7 +1129,7 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
 	perf_fb_ctrs->delivered = delivered;
 	perf_fb_ctrs->reference = reference;
 	perf_fb_ctrs->reference_perf = ref_perf;
-	perf_fb_ctrs->ctr_wrap_time = ctr_wrap_time;
+	perf_fb_ctrs->wraparound_time = ctr_wrap_time;
 out_err:
 	if (regs_in_pcc)
 		up_write(&pcc_data.pcc_lock);
diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
index 3f64660..08c2c78 100644
--- a/include/acpi/cppc_acpi.h
+++ b/include/acpi/cppc_acpi.h
@@ -116,7 +116,7 @@ struct cppc_perf_fb_ctrs {
 	u64 reference;
 	u64 delivered;
 	u64 reference_perf;
-	u64 ctr_wrap_time;
+	u64 wraparound_time;
 };
 
 /* Per CPU container for runtime CPPC management. */
-- 
Qualcomm Datacenter Technologies on behalf of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 0/2] additional sysfs entries for CPPC

[Al Stone]

On 12/14/2016 06:06 PM, Prashanth Prakash wrote:
> This patch-set adds few additional sysfs entries to expose the
> performance capabilities of each CPU. The performance capabilities
> include highest perf, lowest perf, nominal perf and lowest 
> non-linear perf. See 8.4.7.1 for ACPI 6.1 spec for details on
> these capabilities.
> 
> cppc_cpufreq driver operates in KHz scale whereas the delivered
> performance computed in userspace will be in abstract CPPC scale, so
> exposing perf capabilities should allow userspace to figure out the
> conversion factor from CPPC scale to KHz.
> 
> Prashanth Prakash (2):
>   ACPI / CPPC: read all perf caps in a single cppc read command
>   ACPI / CPPC: add sysfs entries for CPPC perf capabilities
> 
>  drivers/acpi/cppc_acpi.c | 164 ++++++++++++++++++++++++++++++-----------------
>  include/acpi/cppc_acpi.h |   3 +-
>  2 files changed, 107 insertions(+), 60 deletions(-)
> 

Nice addition, Prashanth.  I had thought about doing this, but got distracted.
Thanks for following through :).  I have not had a chance to test these yet, but
will do so as soon as I can; my initial review is pretty positive, though.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

Re: [PATCH 0/2] additional sysfs entries for CPPC

[Prakash, Prashanth]

On 1/3/2017 11:37 AM, Al Stone wrote:
> On 12/14/2016 06:06 PM, Prashanth Prakash wrote:
>> This patch-set adds few additional sysfs entries to expose the
>> performance capabilities of each CPU. The performance capabilities
>> include highest perf, lowest perf, nominal perf and lowest 
>> non-linear perf. See 8.4.7.1 for ACPI 6.1 spec for details on
>> these capabilities.
>>
>> cppc_cpufreq driver operates in KHz scale whereas the delivered
>> performance computed in userspace will be in abstract CPPC scale, so
>> exposing perf capabilities should allow userspace to figure out the
>> conversion factor from CPPC scale to KHz.
>>
>> Prashanth Prakash (2):
>>   ACPI / CPPC: read all perf caps in a single cppc read command
>>   ACPI / CPPC: add sysfs entries for CPPC perf capabilities
>>
>>  drivers/acpi/cppc_acpi.c | 164 ++++++++++++++++++++++++++++++-----------------
>>  include/acpi/cppc_acpi.h |   3 +-
>>  2 files changed, 107 insertions(+), 60 deletions(-)
>>
> Nice addition, Prashanth.  I had thought about doing this, but got distracted.
> Thanks for following through :).  I have not had a chance to test these yet, but
> will do so as soon as I can; my initial review is pretty positive, though.
>
Thanks Al! I look forward for your test results.

Thanks,
Prashanth

Re: [PATCH 0/2] additional sysfs entries for CPPC

[Al Stone]

On 01/03/2017 11:37 AM, Al Stone wrote:
> On 12/14/2016 06:06 PM, Prashanth Prakash wrote:
>> This patch-set adds few additional sysfs entries to expose the
>> performance capabilities of each CPU. The performance capabilities
>> include highest perf, lowest perf, nominal perf and lowest 
>> non-linear perf. See 8.4.7.1 for ACPI 6.1 spec for details on
>> these capabilities.
>>
>> cppc_cpufreq driver operates in KHz scale whereas the delivered
>> performance computed in userspace will be in abstract CPPC scale, so
>> exposing perf capabilities should allow userspace to figure out the
>> conversion factor from CPPC scale to KHz.
>>
>> Prashanth Prakash (2):
>>   ACPI / CPPC: read all perf caps in a single cppc read command
>>   ACPI / CPPC: add sysfs entries for CPPC perf capabilities
>>
>>  drivers/acpi/cppc_acpi.c | 164 ++++++++++++++++++++++++++++++-----------------
>>  include/acpi/cppc_acpi.h |   3 +-
>>  2 files changed, 107 insertions(+), 60 deletions(-)
>>
> 
> Nice addition, Prashanth.  I had thought about doing this, but got distracted.
> Thanks for following through :).  I have not had a chance to test these yet, but
> will do so as soon as I can; my initial review is pretty positive, though.
> 

Sorry for the delays :(.  These work for me, on an APM Mustang:

# ls
feedback_ctrs  lowest_non_linear_perf  nominal_perf    wraparound_time
highest_perf   lowest_perf	       reference_perf
# for ii in *; do echo $ii `cat $ii`; done
feedback_ctrs ref:195829033861120 del:1261303045816320
highest_perf 1000
lowest_non_linear_perf 250
lowest_perf 250
nominal_perf 1000
reference_perf 1000
wraparound_time 18446744073709551615


Tested-by: Al Stone <ahs3@redhat.com>


-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

Re: [PATCH 0/2] additional sysfs entries for CPPC

[Rafael J. Wysocki]

On Wed, Feb 8, 2017 at 11:10 PM, Al Stone <ahs3@redhat.com> wrote:
> On 01/03/2017 11:37 AM, Al Stone wrote:
>> On 12/14/2016 06:06 PM, Prashanth Prakash wrote:
>>> This patch-set adds few additional sysfs entries to expose the
>>> performance capabilities of each CPU. The performance capabilities
>>> include highest perf, lowest perf, nominal perf and lowest
>>> non-linear perf. See 8.4.7.1 for ACPI 6.1 spec for details on
>>> these capabilities.
>>>
>>> cppc_cpufreq driver operates in KHz scale whereas the delivered
>>> performance computed in userspace will be in abstract CPPC scale, so
>>> exposing perf capabilities should allow userspace to figure out the
>>> conversion factor from CPPC scale to KHz.
>>>
>>> Prashanth Prakash (2):
>>>   ACPI / CPPC: read all perf caps in a single cppc read command
>>>   ACPI / CPPC: add sysfs entries for CPPC perf capabilities
>>>
>>>  drivers/acpi/cppc_acpi.c | 164 ++++++++++++++++++++++++++++++-----------------
>>>  include/acpi/cppc_acpi.h |   3 +-
>>>  2 files changed, 107 insertions(+), 60 deletions(-)
>>>
>>
>> Nice addition, Prashanth.  I had thought about doing this, but got distracted.
>> Thanks for following through :).  I have not had a chance to test these yet, but
>> will do so as soon as I can; my initial review is pretty positive, though.
>>
>
> Sorry for the delays :(.  These work for me, on an APM Mustang:
>
> # ls
> feedback_ctrs  lowest_non_linear_perf  nominal_perf    wraparound_time
> highest_perf   lowest_perf             reference_perf
> # for ii in *; do echo $ii `cat $ii`; done
> feedback_ctrs ref:195829033861120 del:1261303045816320
> highest_perf 1000
> lowest_non_linear_perf 250
> lowest_perf 250
> nominal_perf 1000
> reference_perf 1000
> wraparound_time 18446744073709551615
>
>
> Tested-by: Al Stone <ahs3@redhat.com>

I'm not actually sure about the assumption this series is based on.

I don't see anything in the spec to guarantee that it will always be
safe to evaluate _CPC only once and cache its output.

Thanks,
Rafael

Re: [PATCH 0/2] additional sysfs entries for CPPC

[Prakash, Prashanth]

Hi Rafael,

On 2/8/2017 5:44 PM, Rafael J. Wysocki wrote:
> On Wed, Feb 8, 2017 at 11:10 PM, Al Stone <ahs3@redhat.com> wrote:
>> On 01/03/2017 11:37 AM, Al Stone wrote:
>>> On 12/14/2016 06:06 PM, Prashanth Prakash wrote:
>>>> This patch-set adds few additional sysfs entries to expose the
>>>> performance capabilities of each CPU. The performance capabilities
>>>> include highest perf, lowest perf, nominal perf and lowest
>>>> non-linear perf. See 8.4.7.1 for ACPI 6.1 spec for details on
>>>> these capabilities.
>>>>
>>>> cppc_cpufreq driver operates in KHz scale whereas the delivered
>>>> performance computed in userspace will be in abstract CPPC scale, so
>>>> exposing perf capabilities should allow userspace to figure out the
>>>> conversion factor from CPPC scale to KHz.
>>>>
>>>> Prashanth Prakash (2):
>>>>   ACPI / CPPC: read all perf caps in a single cppc read command
>>>>   ACPI / CPPC: add sysfs entries for CPPC perf capabilities
>>>>
>>>>  drivers/acpi/cppc_acpi.c | 164 ++++++++++++++++++++++++++++++-----------------
>>>>  include/acpi/cppc_acpi.h |   3 +-
>>>>  2 files changed, 107 insertions(+), 60 deletions(-)
>>>>
>>> Nice addition, Prashanth.  I had thought about doing this, but got distracted.
>>> Thanks for following through :).  I have not had a chance to test these yet, but
>>> will do so as soon as I can; my initial review is pretty positive, though.
>>>
>> Sorry for the delays :(.  These work for me, on an APM Mustang:
>>
>> # ls
>> feedback_ctrs  lowest_non_linear_perf  nominal_perf    wraparound_time
>> highest_perf   lowest_perf             reference_perf
>> # for ii in *; do echo $ii `cat $ii`; done
>> feedback_ctrs ref:195829033861120 del:1261303045816320
>> highest_perf 1000
>> lowest_non_linear_perf 250
>> lowest_perf 250
>> nominal_perf 1000
>> reference_perf 1000
>> wraparound_time 18446744073709551615
>>
>>
>> Tested-by: Al Stone <ahs3@redhat.com>
> I'm not actually sure about the assumption this series is based on.
>
> I don't see anything in the spec to guarantee that it will always be
> safe to evaluate _CPC only once and cache its output.
Among the Performance capabilities registers(section 8.4.7.1.1), the only
register that can change dynamically is Guaranteed performance register.
We are not supporting/using Guaranteed performance at the moment.

Guaranteed performance Register has an associated Notify event which will be
invoked when it changes. No such events are associated with other capabilities
register. Similar distinction is made in the beginning of section 8.4.7.1.1:
"Figure 8-47 outlines the static performance thresholds of the platform
 and the dynamic guaranteed performance threshold."

I agree spec isn't very clear about marking these registers as static except
that one sentence I quoted above, but there is enough in spec to guarantee
that the capabilities we are using will not change dynamically.

--
Thanks,
Prashanth

Re: [PATCH 0/2] additional sysfs entries for CPPC

[Prakash, Prashanth]

Hi Rafael,

On 2/13/2017 9:38 AM, Prakash, Prashanth wrote:
[...]
>>> Tested-by: Al Stone <ahs3@redhat.com>
>> I'm not actually sure about the assumption this series is based on.
>>
>> I don't see anything in the spec to guarantee that it will always be
>> safe to evaluate _CPC only once and cache its output.
> Among the Performance capabilities registers(section 8.4.7.1.1), the only
> register that can change dynamically is Guaranteed performance register.
> We are not supporting/using Guaranteed performance at the moment.
>
> Guaranteed performance Register has an associated Notify event which will be
> invoked when it changes. No such events are associated with other capabilities
> register. Similar distinction is made in the beginning of section 8.4.7.1.1:
> "Figure 8-47 outlines the static performance thresholds of the platform
>  and the dynamic guaranteed performance threshold."
>
> I agree spec isn't very clear about marking these registers as static except
> that one sentence I quoted above, but there is enough in spec to guarantee
> that the capabilities we are using will not change dynamically.

Does the above sound reasonable? Any other feedback on this patch set?

--
Thanks,
Prashanth

Re: [PATCH 0/2] additional sysfs entries for CPPC

[Prakash, Prashanth]

On 3/3/2017 11:32 AM, Prakash, Prashanth wrote:
> Hi Rafael,
>
> On 2/13/2017 9:38 AM, Prakash, Prashanth wrote:
> [...]
>>>> Tested-by: Al Stone <ahs3@redhat.com>
>>> I'm not actually sure about the assumption this series is based on.
>>>
>>> I don't see anything in the spec to guarantee that it will always be
>>> safe to evaluate _CPC only once and cache its output.
>> Among the Performance capabilities registers(section 8.4.7.1.1), the only
>> register that can change dynamically is Guaranteed performance register.
>> We are not supporting/using Guaranteed performance at the moment.
>>
>> Guaranteed performance Register has an associated Notify event which will be
>> invoked when it changes. No such events are associated with other capabilities
>> register. Similar distinction is made in the beginning of section 8.4.7.1.1:
>> "Figure 8-47 outlines the static performance thresholds of the platform
>>  and the dynamic guaranteed performance threshold."
>>
>> I agree spec isn't very clear about marking these registers as static except
>> that one sentence I quoted above, but there is enough in spec to guarantee
>> that the capabilities we are using will not change dynamically.
> Does the above sound reasonable? Any other feedback on this patch set?

Gentle Ping

--
Thanks,
Prashanth

Re: [PATCH 0/2] additional sysfs entries for CPPC

[Rafael J. Wysocki]

On Friday, March 24, 2017 10:34:42 AM Prakash, Prashanth wrote:
> 
> On 3/3/2017 11:32 AM, Prakash, Prashanth wrote:
> > Hi Rafael,
> >
> > On 2/13/2017 9:38 AM, Prakash, Prashanth wrote:
> > [...]
> >>>> Tested-by: Al Stone <ahs3@redhat.com>
> >>> I'm not actually sure about the assumption this series is based on.
> >>>
> >>> I don't see anything in the spec to guarantee that it will always be
> >>> safe to evaluate _CPC only once and cache its output.
> >> Among the Performance capabilities registers(section 8.4.7.1.1), the only
> >> register that can change dynamically is Guaranteed performance register.
> >> We are not supporting/using Guaranteed performance at the moment.
> >>
> >> Guaranteed performance Register has an associated Notify event which will be
> >> invoked when it changes. No such events are associated with other capabilities
> >> register. Similar distinction is made in the beginning of section 8.4.7.1.1:
> >> "Figure 8-47 outlines the static performance thresholds of the platform
> >>  and the dynamic guaranteed performance threshold."
> >>
> >> I agree spec isn't very clear about marking these registers as static except
> >> that one sentence I quoted above, but there is enough in spec to guarantee
> >> that the capabilities we are using will not change dynamically.
> > Does the above sound reasonable? Any other feedback on this patch set?
> 
> Gentle Ping

There are concerns that some CPPC parameters may change at run time on some
systems even though the spec doesn't mention that possibility, so the optimization
here may be premature.

Thanks,
Rafael

Re: [PATCH 0/2] additional sysfs entries for CPPC

[Prakash, Prashanth]

On 3/25/2017 7:32 AM, Rafael J. Wysocki wrote:
> On Friday, March 24, 2017 10:34:42 AM Prakash, Prashanth wrote:
>> On 3/3/2017 11:32 AM, Prakash, Prashanth wrote:
>>> Hi Rafael,
>>>
>>> On 2/13/2017 9:38 AM, Prakash, Prashanth wrote:
>>> [...]
>>>>>> Tested-by: Al Stone <ahs3@redhat.com>
>>>>> I'm not actually sure about the assumption this series is based on.
>>>>>
>>>>> I don't see anything in the spec to guarantee that it will always be
>>>>> safe to evaluate _CPC only once and cache its output.
>>>> Among the Performance capabilities registers(section 8.4.7.1.1), the only
>>>> register that can change dynamically is Guaranteed performance register.
>>>> We are not supporting/using Guaranteed performance at the moment.
>>>>
>>>> Guaranteed performance Register has an associated Notify event which will be
>>>> invoked when it changes. No such events are associated with other capabilities
>>>> register. Similar distinction is made in the beginning of section 8.4.7.1.1:
>>>> "Figure 8-47 outlines the static performance thresholds of the platform
>>>>  and the dynamic guaranteed performance threshold."
>>>>
>>>> I agree spec isn't very clear about marking these registers as static except
>>>> that one sentence I quoted above, but there is enough in spec to guarantee
>>>> that the capabilities we are using will not change dynamically.
>>> Does the above sound reasonable? Any other feedback on this patch set?
>> Gentle Ping
> There are concerns that some CPPC parameters may change at run time on some
> systems even though the spec doesn't mention that possibility, so the optimization
> here may be premature.
Thanks Rafael!  I will remove the caching of CPPC perf caps and submit a patch to
add the sysfs entries with existing interface.

--
Thanks,
Prashanth