[PATCH 0/3] platform/x86: Add support for Intel uncore ELC feature

[PATCH 0/3] platform/x86: Add support for Intel uncore ELC feature

[Tero Kristo]

Hi,

This series adds ELC (Efficiency Latency Control) support to Intel
uncore driver. This is a feature supported on newer SoCs, and it
basically acts as a way to fine tune efficiency vs. latency tradeoffs
wrt. uncore frequency scaling.

-Tero

[PATCH 1/3] Documentation: admin-guide: pm: Add efficiency vs. latency tradeoff to uncore documentation

[Tero Kristo]

Added documentation about the functionality of efficiency vs. latency tradeoff
control in intel Xeon processors, and how this is configured via sysfs.

Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
---
 .../pm/intel_uncore_frequency_scaling.rst     | 51 +++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/Documentation/admin-guide/pm/intel_uncore_frequency_scaling.rst b/Documentation/admin-guide/pm/intel_uncore_frequency_scaling.rst
index 5ab3440e6cee..fb83aa2b744e 100644
--- a/Documentation/admin-guide/pm/intel_uncore_frequency_scaling.rst
+++ b/Documentation/admin-guide/pm/intel_uncore_frequency_scaling.rst
@@ -113,3 +113,54 @@ to apply at each uncore* level.
 
 Support for "current_freq_khz" is available only at each fabric cluster
 level (i.e., in uncore* directory).
+
+Efficiency vs. Latency Tradeoff
+-------------------------------
+
+In the realm of high-performance computing, particularly with Xeon
+processors, managing uncore frequency is an important aspect of system
+optimization. Traditionally, the uncore frequency is ramped up rapidly
+in high load scenarios. While this strategy achieves low latency, which
+is crucial for time-sensitive computations, it does not necessarily yield
+the best performance per watt, —a key metric for energy efficiency and
+operational cost savings.
+
+The Efficiency vs. Latency Control (ELC) feature allows user to influence
+the uncore frequency scaling algorithm. Hardware monitors the average CPU
+utilization across all cores at regular intervals. If the average CPU
+utilization is below a user defined threshold (elc_low_threshold_percent),
+the user defined uncore frequency floor frequency will be used
+(elc_floor_freq_khz), minimizing latency. Similarly in high load scenario
+where the CPU utilization goes above the high threshold value
+(elc_high_threshold_percent) instead of jumping to maximum uncore
+frequency, uncore frequency is increased in 100MHz steps until the power
+limit is reached.
+
+Attributes for efficiency latency control:
+
+``elc_floor_freq_khz``
+	This attribute is used to get/set the efficiency latency floor frequency.
+	If this variable is lower than the 'min_freq_khz', it is ignored by
+	the firmware.
+
+``elc_low_threshold_percent``
+	This attribute is used to get/set the efficiency latency control low
+	threshold. This attribute is in percentages of CPU utilization.
+
+``elc_high_threshold_percent``
+	This attribute is used to get/set the efficiency latency control high
+	threshold. This attribute is in percentages of CPU utilization.
+
+``elc_high_threshold_enable``
+	This attribute is used to enable/disable the efficiency latency control
+	high threshold. Write '1' to enable, '0' to disable.
+
+Example system configuration below, which does following:
+  * when CPU utilization is less than 10%: sets uncore frequency to 800MHz
+  * when CPU utilization is higher than 95%: increases uncore frequency in
+    100MHz steps, until power limit is reached
+
+  elc_floor_freq_khz:800000
+  elc_high_threshold_percent:95
+  elc_high_threshold_enable:1
+  elc_low_threshold_percent:10
-- 
2.43.1

Re: [PATCH 1/3] Documentation: admin-guide: pm: Add efficiency vs. latency tradeoff to uncore documentation

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 3841 bytes --]

On Wed, 21 Aug 2024, Tero Kristo wrote:

> Added documentation about the functionality of efficiency vs. latency tradeoff
> control in intel Xeon processors, and how this is configured via sysfs.
> 
> Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
> ---
>  .../pm/intel_uncore_frequency_scaling.rst     | 51 +++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/Documentation/admin-guide/pm/intel_uncore_frequency_scaling.rst b/Documentation/admin-guide/pm/intel_uncore_frequency_scaling.rst
> index 5ab3440e6cee..fb83aa2b744e 100644
> --- a/Documentation/admin-guide/pm/intel_uncore_frequency_scaling.rst
> +++ b/Documentation/admin-guide/pm/intel_uncore_frequency_scaling.rst
> @@ -113,3 +113,54 @@ to apply at each uncore* level.
>  
>  Support for "current_freq_khz" is available only at each fabric cluster
>  level (i.e., in uncore* directory).
> +
> +Efficiency vs. Latency Tradeoff

Does this section even cover the "tradeoff" part in its body? Why not call 
it directly "Control" after ELC?

> +-------------------------------
> +
> +In the realm of high-performance computing, particularly with Xeon
> +processors, managing uncore frequency is an important aspect of system
> +optimization. Traditionally, the uncore frequency is ramped up rapidly
> +in high load scenarios. While this strategy achieves low latency, which
> +is crucial for time-sensitive computations, it does not necessarily yield
> +the best performance per watt, —a key metric for energy efficiency and
> +operational cost savings.

This entire paragraph feels more prose or history book than documentation 
text. I'd suggest using something that goes more directly into the point
about what ELC brings to the table (I suppose the goal is "performance 
per watt" optimization, even that goal is only implied by the current 
text, not explicitly stated as the goal here).

-- 
 i.

> +The Efficiency vs. Latency Control (ELC) feature allows user to influence
> +the uncore frequency scaling algorithm. Hardware monitors the average CPU
> +utilization across all cores at regular intervals. If the average CPU
> +utilization is below a user defined threshold (elc_low_threshold_percent),
> +the user defined uncore frequency floor frequency will be used
> +(elc_floor_freq_khz), minimizing latency. Similarly in high load scenario
> +where the CPU utilization goes above the high threshold value
> +(elc_high_threshold_percent) instead of jumping to maximum uncore
> +frequency, uncore frequency is increased in 100MHz steps until the power
> +limit is reached.
> +
> +Attributes for efficiency latency control:
> +
> +``elc_floor_freq_khz``
> +	This attribute is used to get/set the efficiency latency floor frequency.
> +	If this variable is lower than the 'min_freq_khz', it is ignored by
> +	the firmware.
> +
> +``elc_low_threshold_percent``
> +	This attribute is used to get/set the efficiency latency control low
> +	threshold. This attribute is in percentages of CPU utilization.
> +
> +``elc_high_threshold_percent``
> +	This attribute is used to get/set the efficiency latency control high
> +	threshold. This attribute is in percentages of CPU utilization.
> +
> +``elc_high_threshold_enable``
> +	This attribute is used to enable/disable the efficiency latency control
> +	high threshold. Write '1' to enable, '0' to disable.
> +
> +Example system configuration below, which does following:
> +  * when CPU utilization is less than 10%: sets uncore frequency to 800MHz
> +  * when CPU utilization is higher than 95%: increases uncore frequency in
> +    100MHz steps, until power limit is reached
> +
> +  elc_floor_freq_khz:800000
> +  elc_high_threshold_percent:95
> +  elc_high_threshold_enable:1
> +  elc_low_threshold_percent:10
> 

Re: [PATCH 1/3] Documentation: admin-guide: pm: Add efficiency vs. latency tradeoff to uncore documentation

[srinivas pandruvada]

On Fri, 2024-08-23 at 15:28 +0300, Ilpo Järvinen wrote:
> On Wed, 21 Aug 2024, Tero Kristo wrote:
> 
> > Added documentation about the functionality of efficiency vs.
> > latency tradeoff
> > control in intel Xeon processors, and how this is configured via
> > sysfs.
> > 
> > Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
> > ---
> >  .../pm/intel_uncore_frequency_scaling.rst     | 51
> > +++++++++++++++++++
> >  1 file changed, 51 insertions(+)
> > 
> > diff --git a/Documentation/admin-
> > guide/pm/intel_uncore_frequency_scaling.rst b/Documentation/admin-
> > guide/pm/intel_uncore_frequency_scaling.rst
> > index 5ab3440e6cee..fb83aa2b744e 100644
> > --- a/Documentation/admin-
> > guide/pm/intel_uncore_frequency_scaling.rst
> > +++ b/Documentation/admin-
> > guide/pm/intel_uncore_frequency_scaling.rst
> > @@ -113,3 +113,54 @@ to apply at each uncore* level.
> >  
> >  Support for "current_freq_khz" is available only at each fabric
> > cluster
> >  level (i.e., in uncore* directory).
> > +
> > +Efficiency vs. Latency Tradeoff
> 
> Does this section even cover the "tradeoff" part in its body? Why not
> call 
> it directly "Control" after ELC?
> 
> > +-------------------------------
> > +
> > +In the realm of high-performance computing, particularly with Xeon
> > +processors, managing uncore frequency is an important aspect of
> > system
> > +optimization. Traditionally, the uncore frequency is ramped up
> > rapidly
> > +in high load scenarios. While this strategy achieves low latency,
> > which
> > +is crucial for time-sensitive computations, it does not
> > necessarily yield
> > +the best performance per watt, —a key metric for energy efficiency
> > and
> > +operational cost savings.
> 
> This entire paragraph feels more prose or history book than
> documentation 
> text. I'd suggest using something that goes more directly into the
> point
> about what ELC brings to the table (I suppose the goal is
> "performance 
> per watt" optimization, even that goal is only implied by the current
> text, not explicitly stated as the goal here).
> 

What about this?

Traditionally, the uncore frequency is ramped up to reach the maximum 
possible level based on parameters like EPB (Energy perf Bias) and
other system power management settings programmed by BIOS.  While this
strategy achieves low latency for latency sensitive applications, it
does not necessarily yield the best performance per watt. 

The Efficiency Latency Control (ELC) feature is added to improve
performance per watt. With this feature hardware power management
algorithms optimize trade-off between latency and power consumption.
But for some latency sensitive workloads further tuning can be done
from OS to get desired performance.

The hardware monitors the average CPU utilization across all cores
in a power domain at regular intervals and decides a uncore frequency. 
While this may result in the best performance per watt, workload may be
expecting higher performance at the expense of power. Consider an
application that intermittently wakes up to perform memory reads on an
otherwise idle system. In such cases, if hardware lowers uncore
frequency, then there may be delay in ramp up of frequency to meet
target performance. 

The ELC control defines some parameters which can be changed from OS.
If the average CPU utilization is below a user defined threshold
(elc_low_threshold_percent attribute below), the user defined uncore
frequency floor frequency will be used (elc_floor_freq_khz attribute 
below) instead of hardware calculated minimum. 

Similarly in high load scenario where the CPU utilization goes above 
the high threshold value (elc_high_threshold_percent attribute below) 
instead of jumping to maximum uncore frequency, uncore frequency is 
increased in 100MHz steps until the power limit is reached.

Attributes for efficiency latency control: 
.. 
.. 

Thanks,
Srinivas

Re: [PATCH 1/3] Documentation: admin-guide: pm: Add efficiency vs. latency tradeoff to uncore documentation

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 5591 bytes --]

On Mon, 26 Aug 2024, srinivas pandruvada wrote:

> On Fri, 2024-08-23 at 15:28 +0300, Ilpo Järvinen wrote:
> > On Wed, 21 Aug 2024, Tero Kristo wrote:
> > 
> > > Added documentation about the functionality of efficiency vs.
> > > latency tradeoff
> > > control in intel Xeon processors, and how this is configured via
> > > sysfs.
> > > 
> > > Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
> > > ---
> > >  .../pm/intel_uncore_frequency_scaling.rst     | 51
> > > +++++++++++++++++++
> > >  1 file changed, 51 insertions(+)
> > > 
> > > diff --git a/Documentation/admin-
> > > guide/pm/intel_uncore_frequency_scaling.rst b/Documentation/admin-
> > > guide/pm/intel_uncore_frequency_scaling.rst
> > > index 5ab3440e6cee..fb83aa2b744e 100644
> > > --- a/Documentation/admin-
> > > guide/pm/intel_uncore_frequency_scaling.rst
> > > +++ b/Documentation/admin-
> > > guide/pm/intel_uncore_frequency_scaling.rst
> > > @@ -113,3 +113,54 @@ to apply at each uncore* level.
> > >  
> > >  Support for "current_freq_khz" is available only at each fabric
> > > cluster
> > >  level (i.e., in uncore* directory).
> > > +
> > > +Efficiency vs. Latency Tradeoff
> > 
> > Does this section even cover the "tradeoff" part in its body? Why not
> > call 
> > it directly "Control" after ELC?
> > 
> > > +-------------------------------
> > > +
> > > +In the realm of high-performance computing, particularly with Xeon
> > > +processors, managing uncore frequency is an important aspect of
> > > system
> > > +optimization. Traditionally, the uncore frequency is ramped up
> > > rapidly
> > > +in high load scenarios. While this strategy achieves low latency,
> > > which
> > > +is crucial for time-sensitive computations, it does not
> > > necessarily yield
> > > +the best performance per watt, —a key metric for energy efficiency
> > > and
> > > +operational cost savings.
> > 
> > This entire paragraph feels more prose or history book than
> > documentation 
> > text. I'd suggest using something that goes more directly into the
> > point
> > about what ELC brings to the table (I suppose the goal is
> > "performance 
> > per watt" optimization, even that goal is only implied by the current
> > text, not explicitly stated as the goal here).
> > 
> 
> What about this?
> 
> Traditionally, the uncore frequency is ramped up to reach the maximum 
> possible level based on parameters like EPB (Energy perf Bias) and
> other system power management settings programmed by BIOS.  While this
> strategy achieves low latency for latency sensitive applications, it
> does not necessarily yield the best performance per watt. 

This again starts with a wrong foot. Don't use words like "traditionally",
"in the past", "historically", "is added", etc. that refer to past time
in documentation text at all. The premise with documentation for feature x 
is that the feature x exists. After these patches have been accepted, the 
reality is that ELC exists and time before does not matter so we don't 
encumber documentation text with that era that has become irrelevant.

You might occasionally have to mention what is not possible without ELC 
in case it's still possible to run stuff without ELC but don't put time 
references to it. However, it's not something you should start with in
the documentation text.

> The Efficiency Latency Control (ELC) feature is added to improve

"is added to improve" -> "improves"

> performance per watt. With this feature hardware power management
> algorithms optimize trade-off between latency and power consumption.
> But for some latency sensitive workloads further tuning can be done
> from OS to get desired performance.

I'd just start with this paragraph. It goes straight into the point and 
is good in that it tries to summarize what ELC tries to achieve.

> The hardware monitors the average CPU utilization across all cores

hardware or ELC-capable HW?

> in a power domain at regular intervals and decides a uncore frequency. 

This kind of feels something that belongs to the first paragraph if it's 
about ELC. (I'm left slightly unsure if ELC refers only to those controls 
mentioned below, or if it is the automatic uncore freq control plus the 
manual controls. I assume it's the latter because of "with this feature 
hardware power management algorithms optimize" sentence.)

> While this may result in the best performance per watt, workload may be
> expecting higher performance at the expense of power. Consider an
> application that intermittently wakes up to perform memory reads on an
> otherwise idle system. In such cases, if hardware lowers uncore
> frequency, then there may be delay in ramp up of frequency to meet
> target performance. 
>
> The ELC control defines some parameters which can be changed from OS.
> If the average CPU utilization is below a user defined threshold
> (elc_low_threshold_percent attribute below), the user defined uncore
> frequency floor frequency will be used (elc_floor_freq_khz attribute 
> below) instead of hardware calculated minimum. 
>
> Similarly in high load scenario where the CPU utilization goes above 
> the high threshold value (elc_high_threshold_percent attribute below) 
> instead of jumping to maximum uncore frequency, uncore frequency is 
> increased in 100MHz steps until the power limit is reached.
>
> Attributes for efficiency latency control: 
> .. 
> .. 

There were a few spaces at the end if lines, those should be removed.

-- 
 i.

Re: [PATCH 1/3] Documentation: admin-guide: pm: Add efficiency vs. latency tradeoff to uncore documentation

[srinivas pandruvada]

On Tue, 2024-08-27 at 11:08 +0300, Ilpo Järvinen wrote:
> On Mon, 26 Aug 2024, srinivas pandruvada wrote:
> 
> > On Fri, 2024-08-23 at 15:28 +0300, Ilpo Järvinen wrote:
> > > On Wed, 21 Aug 2024, Tero Kristo wrote:
> > > 
> > > > Added documentation about the functionality of efficiency vs.
> > > > latency tradeoff
> > > > control in intel Xeon processors, and how this is configured
> > > > via
> > > > sysfs.
> > > > 
> > > > Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
> > > > ---
> > > >  .../pm/intel_uncore_frequency_scaling.rst     | 51
> > > > +++++++++++++++++++⁠Ayoub, Hatim This seems that when on AC
> > > > mode, Windows don't care about PC10. Is this correct? It seems
> > > > that with EPB=6 we can
> > > >  1 file changed, 51 insertions(+)
> > > > 
> > > > diff --git a/Documentation/admin-
> > > > guide/pm/intel_uncore_frequency_scaling.rst
> > > > b/Documentation/admin-
> > > > guide/pm/intel_uncore_frequency_scaling.rst
> > > > index 5ab3440e6cee..fb83aa2b744e 100644
> > > > --- a/Documentation/admin-
> > > > guide/pm/intel_uncore_frequency_scaling.rst
> > > > +++ b/Documentation/admin-
> > > > guide/pm/intel_uncore_frequency_scaling.rst
> > > > @@ -113,3 +113,54 @@ to apply at each uncore* level.
> > > >  
> > > >  Support for "current_freq_khz" is available only at each
> > > > fabric
> > > > cluster
> > > >  level (i.e., in uncore* directory).
> > > > +
> > > > +Efficiency vs. Latency Tradeoff
> > > 
> > > Does this section even cover the "tradeoff" part in its body? Why
> > > not
> > > call 
> > > it directly "Control" after ELC?
> > > 
> > > > +-------------------------------
> > > > +
> > > > +In the realm of high-performance computing, particularly with
> > > > Xeon
> > > > +processors, managing uncore frequency is an important aspect
> > > > of
> > > > system
> > > > +optimization. Traditionally, the uncore frequency is ramped up
> > > > rapidly
> > > > +in high load scenarios. While this strategy achieves low
> > > > latency,
> > > > which
> > > > +is crucial for time-sensitive computations, it does not
> > > > necessarily yield
> > > > +the best performance per watt, —a key metric for energy
> > > > efficiency
> > > > and
> > > > +operational cost savings.
> > > 
> > > This entire paragraph feels more prose or history book than
> > > documentation 
> > > text. I'd suggest using something that goes more directly into
> > > the
> > > point
> > > about what ELC brings to the table (I suppose the goal is
> > > "performance 
> > > per watt" optimization, even that goal is only implied by the
> > > current
> > > text, not explicitly stated as the goal here).
> > > 
> > 
> > What about this?
> > 
> > Traditionally, the uncore frequency is ramped up to reach the
> > maximum 
> > possible level based on parameters like EPB (Energy perf Bias) and
> > other system power management settings programmed by BIOS.  While
> > this
> > strategy achieves low latency for latency sensitive applications,
> > it
> > does not necessarily yield the best performance per watt. 
> 
> This again starts with a wrong foot. Don't use words like
> "traditionally",
> "in the past", "historically", "is added", etc. that refer to past
> time
> in documentation text at all. The premise with documentation for
> feature x 
> is that the feature x exists. After these patches have been accepted,
> the 
> reality is that ELC exists and time before does not matter so we
> don't 
> encumber documentation text with that era that has become irrelevant.
> 
While the choice of words are not correct, for me background is
important on why a feature is implemented.
Here even after ELC is implemented, majority of generations will still
not have this feature. Uncore is not just supported on TPMI systems.


> You might occasionally have to mention what is not possible without
> ELC 
> in case it's still possible to run stuff without ELC but don't put
> time 
> references to it. However, it's not something you should start with
> in
> the documentation text.
> 
> > The Efficiency Latency Control (ELC) feature is added to improve
> 
> "is added to improve" -> "improves"
Fine.

> 
> > performance per watt. With this feature hardware power management
> > algorithms optimize trade-off between latency and power
> > consumption.
> > But for some latency sensitive workloads further tuning can be done
> > from OS to get desired performance.
> 
> I'd just start with this paragraph. It goes straight into the point
> and 
> is good in that it tries to summarize what ELC tries to achieve.
There are so many features we have which improves perf/watt. Why ELC is
special needs some background.

> 
> > The hardware monitors the average CPU utilization across all cores
> 
> hardware or ELC-capable HW?
Hardware. hardware always does this.

> 
> > in a power domain at regular intervals and decides a uncore
> > frequency. 
> 
> This kind of feels something that belongs to the first paragraph if
> it's 
> about ELC. (I'm left slightly unsure if ELC refers only to those
> controls 
> mentioned below, or if it is the automatic uncore freq control plus
> the 
> manual controls. I assume it's the latter because of "with this
> feature 
> hardware power management algorithms optimize" sentence.)
It is later. Hardware doesn't do a PM feature depending only on OS.

> 
> > While this may result in the best performance per watt, workload
> > may be
> > expecting higher performance at the expense of power. Consider an
> > application that intermittently wakes up to perform memory reads on
> > an
> > otherwise idle system. In such cases, if hardware lowers uncore
> > frequency, then there may be delay in ramp up of frequency to meet
> > target performance. 
> > 
> > The ELC control defines some parameters which can be changed from
> > OS.
> > If the average CPU utilization is below a user defined threshold
> > (elc_low_threshold_percent attribute below), the user defined
> > uncore
> > frequency floor frequency will be used (elc_floor_freq_khz
> > attribute 
> > below) instead of hardware calculated minimum. 
> > 
> > Similarly in high load scenario where the CPU utilization goes
> > above 
> > the high threshold value (elc_high_threshold_percent attribute
> > below) 
> > instead of jumping to maximum uncore frequency, uncore frequency is
> > increased in 100MHz steps until the power limit is reached.
> > 
> > Attributes for efficiency latency control: 
> > .. 
> > .. 
> 
> There were a few spaces at the end if lines, those should be removed.
Yes in the patch.

Thanks,
Srinivas

> 

Re: [PATCH 1/3] Documentation: admin-guide: pm: Add efficiency vs. latency tradeoff to uncore documentation

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 8585 bytes --]

On Tue, 27 Aug 2024, srinivas pandruvada wrote:
> On Tue, 2024-08-27 at 11:08 +0300, Ilpo Järvinen wrote:
> > On Mon, 26 Aug 2024, srinivas pandruvada wrote:
> > 
> > > On Fri, 2024-08-23 at 15:28 +0300, Ilpo Järvinen wrote:
> > > > On Wed, 21 Aug 2024, Tero Kristo wrote:
> > > > 
> > > > > Added documentation about the functionality of efficiency vs.
> > > > > latency tradeoff
> > > > > control in intel Xeon processors, and how this is configured
> > > > > via
> > > > > sysfs.
> > > > > 
> > > > > Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
> > > > > ---
> > > > >  .../pm/intel_uncore_frequency_scaling.rst     | 51
> > > > > +++++++++++++++++++⁠Ayoub, Hatim This seems that when on AC
> > > > > mode, Windows don't care about PC10. Is this correct? It seems
> > > > > that with EPB=6 we can
> > > > >  1 file changed, 51 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/admin-
> > > > > guide/pm/intel_uncore_frequency_scaling.rst
> > > > > b/Documentation/admin-
> > > > > guide/pm/intel_uncore_frequency_scaling.rst
> > > > > index 5ab3440e6cee..fb83aa2b744e 100644
> > > > > --- a/Documentation/admin-
> > > > > guide/pm/intel_uncore_frequency_scaling.rst
> > > > > +++ b/Documentation/admin-
> > > > > guide/pm/intel_uncore_frequency_scaling.rst
> > > > > @@ -113,3 +113,54 @@ to apply at each uncore* level.
> > > > >  
> > > > >  Support for "current_freq_khz" is available only at each
> > > > > fabric
> > > > > cluster
> > > > >  level (i.e., in uncore* directory).
> > > > > +
> > > > > +Efficiency vs. Latency Tradeoff
> > > > 
> > > > Does this section even cover the "tradeoff" part in its body? Why
> > > > not
> > > > call 
> > > > it directly "Control" after ELC?
> > > > 
> > > > > +-------------------------------
> > > > > +
> > > > > +In the realm of high-performance computing, particularly with
> > > > > Xeon
> > > > > +processors, managing uncore frequency is an important aspect
> > > > > of
> > > > > system
> > > > > +optimization. Traditionally, the uncore frequency is ramped up
> > > > > rapidly
> > > > > +in high load scenarios. While this strategy achieves low
> > > > > latency,
> > > > > which
> > > > > +is crucial for time-sensitive computations, it does not
> > > > > necessarily yield
> > > > > +the best performance per watt, —a key metric for energy
> > > > > efficiency
> > > > > and
> > > > > +operational cost savings.
> > > > 
> > > > This entire paragraph feels more prose or history book than
> > > > documentation 
> > > > text. I'd suggest using something that goes more directly into
> > > > the
> > > > point
> > > > about what ELC brings to the table (I suppose the goal is
> > > > "performance 
> > > > per watt" optimization, even that goal is only implied by the
> > > > current
> > > > text, not explicitly stated as the goal here).
> > > > 
> > > 
> > > What about this?
> > > 
> > > Traditionally, the uncore frequency is ramped up to reach the
> > > maximum 
> > > possible level based on parameters like EPB (Energy perf Bias) and
> > > other system power management settings programmed by BIOS.  While
> > > this
> > > strategy achieves low latency for latency sensitive applications,
> > > it
> > > does not necessarily yield the best performance per watt. 
> > 
> > This again starts with a wrong foot. Don't use words like
> > "traditionally",
> > "in the past", "historically", "is added", etc. that refer to past
> > time
> > in documentation text at all. The premise with documentation for
> > feature x 
> > is that the feature x exists. After these patches have been accepted,
> > the 
> > reality is that ELC exists and time before does not matter so we
> > don't 
> > encumber documentation text with that era that has become irrelevant.
> > 
> While the choice of words are not correct, for me background is
> important on why a feature is implemented.
> Here even after ELC is implemented, majority of generations will still
> not have this feature. Uncore is not just supported on TPMI systems.

I don't doubt there are plenty of systems without it, but you're 
supposed to document ELC, not non-ELC systems under this section.

If the system does not have ELC, this section has zero relevance for
the admin. (And it's a job for marketting people, not for Linux 
documentation, to convince people they need this new and shiny
thing. :-))

Thus, the base assumption in this section is that ELC is supported and 
usable.

> > You might occasionally have to mention what is not possible without
> > ELC 
> > in case it's still possible to run stuff without ELC but don't put
> > time 
> > references to it. However, it's not something you should start with
> > in
> > the documentation text.
> > 
> > > The Efficiency Latency Control (ELC) feature is added to improve
> > 
> > "is added to improve" -> "improves"
> Fine.
> 
> > 
> > > performance per watt. With this feature hardware power management
> > > algorithms optimize trade-off between latency and power
> > > consumption.
> > > But for some latency sensitive workloads further tuning can be done
> > > from OS to get desired performance.
> > 
> > I'd just start with this paragraph. It goes straight into the point
> > and 
> > is good in that it tries to summarize what ELC tries to achieve.
>
> There are so many features we have which improves perf/watt.

I think you make an issue out of a non-issue if you think it's problem to 
state feature A improves performance if there are also features B and C 
that improve it.

> Why ELC is special needs some background.

"special"? I didn't get that impression at all. I'm far from convinced ELC 
is "special" or needs to be presented as such.

> > > The hardware monitors the average CPU utilization across all cores
> > 
> > hardware or ELC-capable HW?
> Hardware. hardware always does this.

So do I read you right, this is nothing ELC-specific? So all ELC does is 
adds those tunables (limits/overrides)?

> > > in a power domain at regular intervals and decides a uncore
> > > frequency. 
> > 
> > This kind of feels something that belongs to the first paragraph if
> > it's 
> > about ELC. (I'm left slightly unsure if ELC refers only to those
> > controls 
> > mentioned below, or if it is the automatic uncore freq control plus
> > the 
> > manual controls. I assume it's the latter because of "with this
> > feature 
> > hardware power management algorithms optimize" sentence.)
>
> It is later. Hardware doesn't do a PM feature depending only on OS.

I was specifically talking about what "ELC" is but you downgraded wording 
back to "hardware" in your reply. So I have to repeat myself, what ELC 
consists of? Are these non-OS dependent features (that "hardware always 
does") part of ELC or not (for the avoidance of potential 
misunderstandings, this is assuming no ELC tunables added by this series 
are touched by the admin)?

Obviously, when one of the tunables is touched, it impacts the allowed
operating region of the autonomous algorithm (ELC tunables acting as 
what look like "overrides").

> > > While this may result in the best performance per watt, workload
> > > may be
> > > expecting higher performance at the expense of power. Consider an
> > > application that intermittently wakes up to perform memory reads on
> > > an
> > > otherwise idle system. In such cases, if hardware lowers uncore
> > > frequency, then there may be delay in ramp up of frequency to meet
> > > target performance. 
> > > 
> > > The ELC control defines some parameters which can be changed from
> > > OS.
> > > If the average CPU utilization is below a user defined threshold
> > > (elc_low_threshold_percent attribute below), the user defined
> > > uncore
> > > frequency floor frequency will be used (elc_floor_freq_khz
> > > attribute 
> > > below) instead of hardware calculated minimum. 
> > > 
> > > Similarly in high load scenario where the CPU utilization goes
> > > above 
> > > the high threshold value (elc_high_threshold_percent attribute
> > > below) 
> > > instead of jumping to maximum uncore frequency, uncore frequency is
> > > increased in 100MHz steps until the power limit is reached.
> > > 
> > > Attributes for efficiency latency control: 
> > > .. 
> > > .. 
> > 
> > There were a few spaces at the end if lines, those should be removed.
> Yes in the patch.
> 
> Thanks,
> Srinivas
> 
> > 
> 

-- 
 i.

[PATCH 2/3] platform/x86/intel-uncore-freq: Add support for efficiency latency control

[Tero Kristo]

Add efficiency latency control support to the TPMI uncore driver. This
defines two new threshold values for controlling uncore frequency, low
threshold and high threshold. When CPU utilization is below low threshold,
the user configurable floor latency control frequency can be used by the
system. When CPU utilization is above high threshold, the uncore frequency
is increased in 100MHz steps until power limit is reached.

Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
---
 .../uncore-frequency-common.h                 |   4 +
 .../uncore-frequency/uncore-frequency-tpmi.c  | 153 +++++++++++++++++-
 2 files changed, 155 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.h b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.h
index 4c245b945e4e..b5c7311bfa05 100644
--- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.h
+++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.h
@@ -70,6 +70,10 @@ enum uncore_index {
 	UNCORE_INDEX_MIN_FREQ,
 	UNCORE_INDEX_MAX_FREQ,
 	UNCORE_INDEX_CURRENT_FREQ,
+	UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD,
+	UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD,
+	UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE,
+	UNCORE_INDEX_EFF_LAT_CTRL_FREQ,
 };
 
 int uncore_freq_common_init(int (*read)(struct uncore_data *data, unsigned int *value,
diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
index 9fa3037c03d1..3a83b6ce54a5 100644
--- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
+++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
@@ -30,6 +30,7 @@
 
 #define	UNCORE_MAJOR_VERSION		0
 #define	UNCORE_MINOR_VERSION		2
+#define UNCORE_ELC_SUPPORTED_VERSION	2
 #define UNCORE_HEADER_INDEX		0
 #define UNCORE_FABRIC_CLUSTER_OFFSET	8
 
@@ -46,6 +47,7 @@ struct tpmi_uncore_struct;
 /* Information for each cluster */
 struct tpmi_uncore_cluster_info {
 	bool root_domain;
+	bool elc_supported;
 	u8 __iomem *cluster_base;
 	struct uncore_data uncore_data;
 	struct tpmi_uncore_struct *uncore_root;
@@ -75,6 +77,10 @@ struct tpmi_uncore_struct {
 /* Bit definitions for CONTROL register */
 #define UNCORE_MAX_RATIO_MASK				GENMASK_ULL(14, 8)
 #define UNCORE_MIN_RATIO_MASK				GENMASK_ULL(21, 15)
+#define UNCORE_EFF_LAT_CTRL_RATIO_MASK			GENMASK_ULL(28, 22)
+#define UNCORE_EFF_LAT_CTRL_LOW_THRESHOLD_MASK		GENMASK_ULL(38, 32)
+#define UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE	BIT(39)
+#define UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_MASK		GENMASK_ULL(46, 40)
 
 /* Helper function to read MMIO offset for max/min control frequency */
 static void read_control_freq(struct tpmi_uncore_cluster_info *cluster_info,
@@ -89,6 +95,48 @@ static void read_control_freq(struct tpmi_uncore_cluster_info *cluster_info,
 		*value = FIELD_GET(UNCORE_MIN_RATIO_MASK, control) * UNCORE_FREQ_KHZ_MULTIPLIER;
 }
 
+/* Helper function to read efficiency latency control values over MMIO */
+static int read_eff_lat_ctrl(struct uncore_data *data, unsigned int *val, enum uncore_index index)
+{
+	struct tpmi_uncore_cluster_info *cluster_info;
+	u64 ctrl;
+
+	cluster_info = container_of(data, struct tpmi_uncore_cluster_info, uncore_data);
+	if (cluster_info->root_domain)
+		return -ENODATA;
+
+	if (!cluster_info->elc_supported)
+		return -EOPNOTSUPP;
+
+	ctrl = readq(cluster_info->cluster_base + UNCORE_CONTROL_INDEX);
+
+	switch (index) {
+	case UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD:
+		*val = FIELD_GET(UNCORE_EFF_LAT_CTRL_LOW_THRESHOLD_MASK, ctrl);
+		*val *= 100;
+		*val = DIV_ROUND_UP(*val, FIELD_MAX(UNCORE_EFF_LAT_CTRL_LOW_THRESHOLD_MASK));
+		break;
+
+	case UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD:
+		*val = FIELD_GET(UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_MASK, ctrl);
+		*val *= 100;
+		*val = DIV_ROUND_UP(*val, FIELD_MAX(UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_MASK));
+		break;
+
+	case UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE:
+		*val = FIELD_GET(UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE, ctrl);
+		break;
+	case UNCORE_INDEX_EFF_LAT_CTRL_FREQ:
+		*val = FIELD_GET(UNCORE_EFF_LAT_CTRL_RATIO_MASK, ctrl) * UNCORE_FREQ_KHZ_MULTIPLIER;
+		break;
+
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
 #define UNCORE_MAX_RATIO	FIELD_MAX(UNCORE_MAX_RATIO_MASK)
 
 /* Helper for sysfs read for max/min frequencies. Called under mutex locks */
@@ -137,6 +185,77 @@ static int uncore_read_control_freq(struct uncore_data *data, unsigned int *valu
 	return 0;
 }
 
+/* Helper function for writing efficiency latency control values over MMIO */
+static int write_eff_lat_ctrl(struct uncore_data *data, unsigned int val, enum uncore_index index)
+{
+	struct tpmi_uncore_cluster_info *cluster_info;
+	u64 control;
+
+	cluster_info = container_of(data, struct tpmi_uncore_cluster_info, uncore_data);
+
+	if (cluster_info->root_domain)
+		return -ENODATA;
+
+	if (!cluster_info->elc_supported)
+		return -EOPNOTSUPP;
+
+	switch (index) {
+	case UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD:
+		if (val > 100)
+			return -EINVAL;
+		break;
+
+	case UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD:
+		if (val > 100)
+			return -EINVAL;
+		break;
+
+	case UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE:
+		if (val > 1)
+			return -EINVAL;
+		break;
+
+	case UNCORE_INDEX_EFF_LAT_CTRL_FREQ:
+		val /= UNCORE_FREQ_KHZ_MULTIPLIER;
+		if (val > FIELD_MAX(UNCORE_EFF_LAT_CTRL_RATIO_MASK))
+			return -EINVAL;
+		break;
+
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	control = readq(cluster_info->cluster_base + UNCORE_CONTROL_INDEX);
+
+	if (index == UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD) {
+		val *= FIELD_MAX(UNCORE_EFF_LAT_CTRL_LOW_THRESHOLD_MASK);
+		val /= 100;
+		control &= ~UNCORE_EFF_LAT_CTRL_LOW_THRESHOLD_MASK;
+		control |= FIELD_PREP(UNCORE_EFF_LAT_CTRL_LOW_THRESHOLD_MASK, val);
+	}
+
+	if (index == UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD) {
+		val *= FIELD_MAX(UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_MASK);
+		val /= 100;
+		control &= ~UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_MASK;
+		control |= FIELD_PREP(UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_MASK, val);
+	}
+
+	if (index == UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE) {
+		control &= ~UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE;
+		control |= FIELD_PREP(UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE, val);
+	}
+
+	if (index == UNCORE_INDEX_EFF_LAT_CTRL_FREQ) {
+		control &= ~UNCORE_EFF_LAT_CTRL_RATIO_MASK;
+		control |= FIELD_PREP(UNCORE_EFF_LAT_CTRL_RATIO_MASK, val);
+	}
+
+	writeq(control, cluster_info->cluster_base + UNCORE_CONTROL_INDEX);
+
+	return 0;
+}
+
 /* Helper function to write MMIO offset for max/min control frequency */
 static void write_control_freq(struct tpmi_uncore_cluster_info *cluster_info, unsigned int input,
 			      unsigned int index)
@@ -156,7 +275,7 @@ static void write_control_freq(struct tpmi_uncore_cluster_info *cluster_info, un
 	writeq(control, (cluster_info->cluster_base + UNCORE_CONTROL_INDEX));
 }
 
-/* Callback for sysfs write for max/min frequencies. Called under mutex locks */
+/* Helper for sysfs write for max/min frequencies. Called under mutex locks */
 static int uncore_write_control_freq(struct uncore_data *data, unsigned int input,
 				     enum uncore_index index)
 {
@@ -234,6 +353,33 @@ static int uncore_read(struct uncore_data *data, unsigned int *value, enum uncor
 	case UNCORE_INDEX_CURRENT_FREQ:
 		return uncore_read_freq(data, value);
 
+	case UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD:
+	case UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD:
+	case UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE:
+	case UNCORE_INDEX_EFF_LAT_CTRL_FREQ:
+		return read_eff_lat_ctrl(data, value, index);
+
+	default:
+		break;
+	}
+
+	return -EOPNOTSUPP;
+}
+
+/* Callback for sysfs write for TPMI uncore data. Called under mutex locks. */
+static int uncore_write(struct uncore_data *data, unsigned int value, enum uncore_index index)
+{
+	switch (index) {
+	case UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD:
+	case UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD:
+	case UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE:
+	case UNCORE_INDEX_EFF_LAT_CTRL_FREQ:
+		return write_eff_lat_ctrl(data, value, index);
+
+	case UNCORE_INDEX_MIN_FREQ:
+	case UNCORE_INDEX_MAX_FREQ:
+		return uncore_write_control_freq(data, value, index);
+
 	default:
 		break;
 	}
@@ -291,7 +437,7 @@ static int uncore_probe(struct auxiliary_device *auxdev, const struct auxiliary_
 		return -EINVAL;
 
 	/* Register callbacks to uncore core */
-	ret = uncore_freq_common_init(uncore_read, uncore_write_control_freq);
+	ret = uncore_freq_common_init(uncore_read, uncore_write);
 	if (ret)
 		return ret;
 
@@ -409,6 +555,9 @@ static int uncore_probe(struct auxiliary_device *auxdev, const struct auxiliary_
 
 			cluster_info->uncore_root = tpmi_uncore;
 
+			if (TPMI_MINOR_VERSION(pd_info->ufs_header_ver) >= UNCORE_ELC_SUPPORTED_VERSION)
+				cluster_info->elc_supported = true;
+
 			ret = uncore_freq_add_entry(&cluster_info->uncore_data, 0);
 			if (ret) {
 				cluster_info->cluster_base = NULL;
-- 
2.43.1

Re: [PATCH 2/3] platform/x86/intel-uncore-freq: Add support for efficiency latency control

[Ilpo Järvinen]

On Wed, 21 Aug 2024, Tero Kristo wrote:

> Add efficiency latency control support to the TPMI uncore driver. This
> defines two new threshold values for controlling uncore frequency, low
> threshold and high threshold. When CPU utilization is below low threshold,
> the user configurable floor latency control frequency can be used by the
> system. When CPU utilization is above high threshold, the uncore frequency
> is increased in 100MHz steps until power limit is reached.
> 
> Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
> ---
>  .../uncore-frequency-common.h                 |   4 +
>  .../uncore-frequency/uncore-frequency-tpmi.c  | 153 +++++++++++++++++-
>  2 files changed, 155 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.h b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.h
> index 4c245b945e4e..b5c7311bfa05 100644
> --- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.h
> +++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.h
> @@ -70,6 +70,10 @@ enum uncore_index {
>  	UNCORE_INDEX_MIN_FREQ,
>  	UNCORE_INDEX_MAX_FREQ,
>  	UNCORE_INDEX_CURRENT_FREQ,
> +	UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD,
> +	UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD,
> +	UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE,
> +	UNCORE_INDEX_EFF_LAT_CTRL_FREQ,
>  };
>  
>  int uncore_freq_common_init(int (*read)(struct uncore_data *data, unsigned int *value,
> diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
> index 9fa3037c03d1..3a83b6ce54a5 100644
> --- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
> +++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
> @@ -30,6 +30,7 @@
>  
>  #define	UNCORE_MAJOR_VERSION		0
>  #define	UNCORE_MINOR_VERSION		2
> +#define UNCORE_ELC_SUPPORTED_VERSION	2
>  #define UNCORE_HEADER_INDEX		0
>  #define UNCORE_FABRIC_CLUSTER_OFFSET	8
>  
> @@ -46,6 +47,7 @@ struct tpmi_uncore_struct;
>  /* Information for each cluster */
>  struct tpmi_uncore_cluster_info {
>  	bool root_domain;
> +	bool elc_supported;
>  	u8 __iomem *cluster_base;
>  	struct uncore_data uncore_data;
>  	struct tpmi_uncore_struct *uncore_root;
> @@ -75,6 +77,10 @@ struct tpmi_uncore_struct {
>  /* Bit definitions for CONTROL register */
>  #define UNCORE_MAX_RATIO_MASK				GENMASK_ULL(14, 8)
>  #define UNCORE_MIN_RATIO_MASK				GENMASK_ULL(21, 15)
> +#define UNCORE_EFF_LAT_CTRL_RATIO_MASK			GENMASK_ULL(28, 22)
> +#define UNCORE_EFF_LAT_CTRL_LOW_THRESHOLD_MASK		GENMASK_ULL(38, 32)
> +#define UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE	BIT(39)
> +#define UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_MASK		GENMASK_ULL(46, 40)
>  
>  /* Helper function to read MMIO offset for max/min control frequency */
>  static void read_control_freq(struct tpmi_uncore_cluster_info *cluster_info,
> @@ -89,6 +95,48 @@ static void read_control_freq(struct tpmi_uncore_cluster_info *cluster_info,
>  		*value = FIELD_GET(UNCORE_MIN_RATIO_MASK, control) * UNCORE_FREQ_KHZ_MULTIPLIER;
>  }
>  
> +/* Helper function to read efficiency latency control values over MMIO */
> +static int read_eff_lat_ctrl(struct uncore_data *data, unsigned int *val, enum uncore_index index)
> +{
> +	struct tpmi_uncore_cluster_info *cluster_info;
> +	u64 ctrl;
> +
> +	cluster_info = container_of(data, struct tpmi_uncore_cluster_info, uncore_data);
> +	if (cluster_info->root_domain)
> +		return -ENODATA;
> +
> +	if (!cluster_info->elc_supported)
> +		return -EOPNOTSUPP;
> +
> +	ctrl = readq(cluster_info->cluster_base + UNCORE_CONTROL_INDEX);
> +
> +	switch (index) {
> +	case UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD:
> +		*val = FIELD_GET(UNCORE_EFF_LAT_CTRL_LOW_THRESHOLD_MASK, ctrl);
> +		*val *= 100;
> +		*val = DIV_ROUND_UP(*val, FIELD_MAX(UNCORE_EFF_LAT_CTRL_LOW_THRESHOLD_MASK));
> +		break;
> +
> +	case UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD:
> +		*val = FIELD_GET(UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_MASK, ctrl);
> +		*val *= 100;
> +		*val = DIV_ROUND_UP(*val, FIELD_MAX(UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_MASK));
> +		break;
> +
> +	case UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE:
> +		*val = FIELD_GET(UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE, ctrl);
> +		break;
> +	case UNCORE_INDEX_EFF_LAT_CTRL_FREQ:
> +		*val = FIELD_GET(UNCORE_EFF_LAT_CTRL_RATIO_MASK, ctrl) * UNCORE_FREQ_KHZ_MULTIPLIER;
> +		break;
> +
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +
>  #define UNCORE_MAX_RATIO	FIELD_MAX(UNCORE_MAX_RATIO_MASK)
>  
>  /* Helper for sysfs read for max/min frequencies. Called under mutex locks */
> @@ -137,6 +185,77 @@ static int uncore_read_control_freq(struct uncore_data *data, unsigned int *valu
>  	return 0;
>  }
>  
> +/* Helper function for writing efficiency latency control values over MMIO */
> +static int write_eff_lat_ctrl(struct uncore_data *data, unsigned int val, enum uncore_index index)
> +{
> +	struct tpmi_uncore_cluster_info *cluster_info;
> +	u64 control;
> +
> +	cluster_info = container_of(data, struct tpmi_uncore_cluster_info, uncore_data);
> +
> +	if (cluster_info->root_domain)
> +		return -ENODATA;
> +
> +	if (!cluster_info->elc_supported)
> +		return -EOPNOTSUPP;
> +
> +	switch (index) {
> +	case UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD:
> +		if (val > 100)
> +			return -EINVAL;
> +		break;
> +
> +	case UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD:
> +		if (val > 100)
> +			return -EINVAL;
> +		break;
> +
> +	case UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE:
> +		if (val > 1)
> +			return -EINVAL;
> +		break;
> +
> +	case UNCORE_INDEX_EFF_LAT_CTRL_FREQ:
> +		val /= UNCORE_FREQ_KHZ_MULTIPLIER;
> +		if (val > FIELD_MAX(UNCORE_EFF_LAT_CTRL_RATIO_MASK))
> +			return -EINVAL;
> +		break;
> +
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	control = readq(cluster_info->cluster_base + UNCORE_CONTROL_INDEX);
> +
> +	if (index == UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD) {
> +		val *= FIELD_MAX(UNCORE_EFF_LAT_CTRL_LOW_THRESHOLD_MASK);
> +		val /= 100;
> +		control &= ~UNCORE_EFF_LAT_CTRL_LOW_THRESHOLD_MASK;
> +		control |= FIELD_PREP(UNCORE_EFF_LAT_CTRL_LOW_THRESHOLD_MASK, val);
> +	}
> +
> +	if (index == UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD) {
> +		val *= FIELD_MAX(UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_MASK);
> +		val /= 100;
> +		control &= ~UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_MASK;
> +		control |= FIELD_PREP(UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_MASK, val);
> +	}
> +
> +	if (index == UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE) {
> +		control &= ~UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE;
> +		control |= FIELD_PREP(UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE, val);
> +	}
> +
> +	if (index == UNCORE_INDEX_EFF_LAT_CTRL_FREQ) {
> +		control &= ~UNCORE_EFF_LAT_CTRL_RATIO_MASK;
> +		control |= FIELD_PREP(UNCORE_EFF_LAT_CTRL_RATIO_MASK, val);
> +	}

Why are these not using switch/case?

-- 
 i.

> +
> +	writeq(control, cluster_info->cluster_base + UNCORE_CONTROL_INDEX);
> +
> +	return 0;
> +}

Re: [PATCH 2/3] platform/x86/intel-uncore-freq: Add support for efficiency latency control

[srinivas pandruvada]

[-- Attachment #1: Type: text/plain, Size: 7951 bytes --]

On Fri, 2024-08-23 at 15:48 +0300, Ilpo Järvinen wrote:
> On Wed, 21 Aug 2024, Tero Kristo wrote:
> 
> > Add efficiency latency control support to the TPMI uncore driver.
> > This
> > defines two new threshold values for controlling uncore frequency,
> > low
> > threshold and high threshold. When CPU utilization is below low
> > threshold,
> > the user configurable floor latency control frequency can be used
> > by the
> > system. When CPU utilization is above high threshold, the uncore
> > frequency
> > is increased in 100MHz steps until power limit is reached.
> > 
> > Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
> > ---
> >  .../uncore-frequency-common.h                 |   4 +
> >  .../uncore-frequency/uncore-frequency-tpmi.c  | 153
> > +++++++++++++++++-
> >  2 files changed, 155 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-
> > frequency-common.h b/drivers/platform/x86/intel/uncore-
> > frequency/uncore-frequency-common.h
> > index 4c245b945e4e..b5c7311bfa05 100644
> > --- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-
> > common.h
> > +++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-
> > common.h
> > @@ -70,6 +70,10 @@ enum uncore_index {
> >  	UNCORE_INDEX_MIN_FREQ,
> >  	UNCORE_INDEX_MAX_FREQ,
> >  	UNCORE_INDEX_CURRENT_FREQ,
> > +	UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD,
> > +	UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD,
> > +	UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE,
> > +	UNCORE_INDEX_EFF_LAT_CTRL_FREQ,
> >  };
> >  
> >  int uncore_freq_common_init(int (*read)(struct uncore_data *data,
> > unsigned int *value,
> > diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-
> > frequency-tpmi.c b/drivers/platform/x86/intel/uncore-
> > frequency/uncore-frequency-tpmi.c
> > index 9fa3037c03d1..3a83b6ce54a5 100644
> > --- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-
> > tpmi.c
> > +++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-
> > tpmi.c
> > @@ -30,6 +30,7 @@
> >  
> >  #define	UNCORE_MAJOR_VERSION		0
> >  #define	UNCORE_MINOR_VERSION		2
> > +#define UNCORE_ELC_SUPPORTED_VERSION	2
> >  #define UNCORE_HEADER_INDEX		0
> >  #define UNCORE_FABRIC_CLUSTER_OFFSET	8
> >  
> > @@ -46,6 +47,7 @@ struct tpmi_uncore_struct;
> >  /* Information for each cluster */
> >  struct tpmi_uncore_cluster_info {
> >  	bool root_domain;
> > +	bool elc_supported;
> >  	u8 __iomem *cluster_base;
> >  	struct uncore_data uncore_data;
> >  	struct tpmi_uncore_struct *uncore_root;
> > @@ -75,6 +77,10 @@ struct tpmi_uncore_struct {
> >  /* Bit definitions for CONTROL register */
> >  #define
> > UNCORE_MAX_RATIO_MASK				GENMASK_ULL(14, 8)
> >  #define
> > UNCORE_MIN_RATIO_MASK				GENMASK_ULL(21, 15)
> > +#define
> > UNCORE_EFF_LAT_CTRL_RATIO_MASK			GENMASK_ULL(28, 22)
> > +#define
> > UNCORE_EFF_LAT_CTRL_LOW_THRESHOLD_MASK		GENMASK_ULL(38, 32)
> > +#define UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE	BIT(39)
> > +#define
> > UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_MASK		GENMASK_ULL(46, 40)
> >  
> >  /* Helper function to read MMIO offset for max/min control
> > frequency */
> >  static void read_control_freq(struct tpmi_uncore_cluster_info
> > *cluster_info,
> > @@ -89,6 +95,48 @@ static void read_control_freq(struct
> > tpmi_uncore_cluster_info *cluster_info,
> >  		*value = FIELD_GET(UNCORE_MIN_RATIO_MASK, control)
> > * UNCORE_FREQ_KHZ_MULTIPLIER;
> >  }
> >  
> > +/* Helper function to read efficiency latency control values over
> > MMIO */
> > +static int read_eff_lat_ctrl(struct uncore_data *data, unsigned
> > int *val, enum uncore_index index)
> > +{
> > +	struct tpmi_uncore_cluster_info *cluster_info;
> > +	u64 ctrl;
> > +
> > +	cluster_info = container_of(data, struct
> > tpmi_uncore_cluster_info, uncore_data);
> > +	if (cluster_info->root_domain)
> > +		return -ENODATA;
> > +
> > +	if (!cluster_info->elc_supported)
> > +		return -EOPNOTSUPP;
> > +
> > +	ctrl = readq(cluster_info->cluster_base +
> > UNCORE_CONTROL_INDEX);
> > +
> > +	switch (index) {
> > +	case UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD:
> > +		*val =
> > FIELD_GET(UNCORE_EFF_LAT_CTRL_LOW_THRESHOLD_MASK, ctrl);
> > +		*val *= 100;
> > +		*val = DIV_ROUND_UP(*val,
> > FIELD_MAX(UNCORE_EFF_LAT_CTRL_LOW_THRESHOLD_MASK));
> > +		break;
> > +
> > +	case UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD:
> > +		*val =
> > FIELD_GET(UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_MASK, ctrl);
> > +		*val *= 100;
> > +		*val = DIV_ROUND_UP(*val,
> > FIELD_MAX(UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_MASK));
> > +		break;
> > +
> > +	case UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE:
> > +		*val =
> > FIELD_GET(UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE, ctrl);
> > +		break;
> > +	case UNCORE_INDEX_EFF_LAT_CTRL_FREQ:
> > +		*val = FIELD_GET(UNCORE_EFF_LAT_CTRL_RATIO_MASK,
> > ctrl) * UNCORE_FREQ_KHZ_MULTIPLIER;
> > +		break;
> > +
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  #define UNCORE_MAX_RATIO	FIELD_MAX(UNCORE_MAX_RATIO_MASK)
> >  
> >  /* Helper for sysfs read for max/min frequencies. Called under
> > mutex locks */
> > @@ -137,6 +185,77 @@ static int uncore_read_control_freq(struct
> > uncore_data *data, unsigned int *valu
> >  	return 0;
> >  }
> >  
> > +/* Helper function for writing efficiency latency control values
> > over MMIO */
> > +static int write_eff_lat_ctrl(struct uncore_data *data, unsigned
> > int val, enum uncore_index index)
> > +{
> > +	struct tpmi_uncore_cluster_info *cluster_info;
> > +	u64 control;
> > +
> > +	cluster_info = container_of(data, struct
> > tpmi_uncore_cluster_info, uncore_data);
> > +
> > +	if (cluster_info->root_domain)
> > +		return -ENODATA;
> > +
> > +	if (!cluster_info->elc_supported)
> > +		return -EOPNOTSUPP;
> > +
> > +	switch (index) {
> > +	case UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD:
> > +		if (val > 100)
> > +			return -EINVAL;
> > +		break;
> > +
> > +	case UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD:
> > +		if (val > 100)
> > +			return -EINVAL;
> > +		break;
> > +
> > +	case UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE:
> > +		if (val > 1)
> > +			return -EINVAL;
> > +		break;
> > +
> > +	case UNCORE_INDEX_EFF_LAT_CTRL_FREQ:
> > +		val /= UNCORE_FREQ_KHZ_MULTIPLIER;
> > +		if (val >
> > FIELD_MAX(UNCORE_EFF_LAT_CTRL_RATIO_MASK))
> > +			return -EINVAL;
> > +		break;
> > +
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	control = readq(cluster_info->cluster_base +
> > UNCORE_CONTROL_INDEX);
> > +
> > +	if (index == UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD) {
> > +		val *=
> > FIELD_MAX(UNCORE_EFF_LAT_CTRL_LOW_THRESHOLD_MASK);
> > +		val /= 100;
> > +		control &=
> > ~UNCORE_EFF_LAT_CTRL_LOW_THRESHOLD_MASK;
> > +		control |=
> > FIELD_PREP(UNCORE_EFF_LAT_CTRL_LOW_THRESHOLD_MASK, val);
> > +	}
> > +
> > +	if (index == UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD) {
> > +		val *=
> > FIELD_MAX(UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_MASK);
> > +		val /= 100;
> > +		control &=
> > ~UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_MASK;
> > +		control |=
> > FIELD_PREP(UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_MASK, val);
> > +	}
> > +
> > +	if (index ==
> > UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE) {
> > +		control &=
> > ~UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE;
> > +		control |=
> > FIELD_PREP(UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE, val);
> > +	}
> > +
> > +	if (index == UNCORE_INDEX_EFF_LAT_CTRL_FREQ) {
> > +		control &= ~UNCORE_EFF_LAT_CTRL_RATIO_MASK;
> > +		control |=
> > FIELD_PREP(UNCORE_EFF_LAT_CTRL_RATIO_MASK, val);
> > +	}
> 
> Why are these not using switch/case?
> 
I think they can reuse. Just need to repeat readq(). Something like
attached:


[-- Attachment #2: reuse_switch.diff --]
[-- Type: text/x-patch, Size: 2737 bytes --]

diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
index e6047fbbea76..2a338d6b8bbf 100644
--- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
+++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
@@ -191,50 +191,44 @@ static int write_eff_lat_ctrl(struct uncore_data *data, unsigned int val, enum u
 	case UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD:
 		if (val > FIELD_MAX(UNCORE_EFF_LAT_CTRL_LOW_THRESHOLD_MASK))
 			return -EINVAL;
+
+		control = readq(cluster_info->cluster_base + UNCORE_CONTROL_INDEX);
+		control &= ~UNCORE_EFF_LAT_CTRL_LOW_THRESHOLD_MASK;
+		control |= FIELD_PREP(UNCORE_EFF_LAT_CTRL_LOW_THRESHOLD_MASK, val);
 		break;
 
 	case UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD:
 		if (val > FIELD_MAX(UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_MASK))
 			return -EINVAL;
+
+		control = readq(cluster_info->cluster_base + UNCORE_CONTROL_INDEX);
+		control &= ~UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_MASK;
+		control |= FIELD_PREP(UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_MASK, val);
 		break;
 
 	case UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE:
 		if (val > 1)
 			return -EINVAL;
+
+		control = readq(cluster_info->cluster_base + UNCORE_CONTROL_INDEX);
+		control &= ~UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE;
+		control |= FIELD_PREP(UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE, val);
 		break;
 
 	case UNCORE_INDEX_EFF_LAT_CTRL_FREQ:
 		val /= UNCORE_FREQ_KHZ_MULTIPLIER;
 		if (val > FIELD_MAX(UNCORE_EFF_LAT_CTRL_RATIO_MASK))
 			return -EINVAL;
+
+		control = readq(cluster_info->cluster_base + UNCORE_CONTROL_INDEX);
+		control &= ~UNCORE_EFF_LAT_CTRL_RATIO_MASK;
+		control |= FIELD_PREP(UNCORE_EFF_LAT_CTRL_RATIO_MASK, val);
 		break;
 
 	default:
 		return -EOPNOTSUPP;
 	}
 
-	control = readq(cluster_info->cluster_base + UNCORE_CONTROL_INDEX);
-
-	if (index == UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD) {
-		control &= ~UNCORE_EFF_LAT_CTRL_LOW_THRESHOLD_MASK;
-		control |= FIELD_PREP(UNCORE_EFF_LAT_CTRL_LOW_THRESHOLD_MASK, val);
-	}
-
-	if (index == UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD) {
-		control &= ~UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_MASK;
-		control |= FIELD_PREP(UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_MASK, val);
-	}
-
-	if (index == UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE) {
-		control &= ~UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE;
-		control |= FIELD_PREP(UNCORE_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE, val);
-	}
-
-	if (index == UNCORE_INDEX_EFF_LAT_CTRL_FREQ) {
-		control &= ~UNCORE_EFF_LAT_CTRL_RATIO_MASK;
-		control |= FIELD_PREP(UNCORE_EFF_LAT_CTRL_RATIO_MASK, val);
-	}
-
 	writeq(control, cluster_info->cluster_base + UNCORE_CONTROL_INDEX);
 
 	return 0;

[PATCH 3/3] platform/x86/intel-uncore-freq: Add efficiency latency control to sysfs interface

[Tero Kristo]

Add the TPMI efficiency latency control fields to the sysfs interface.
The sysfs files are mapped to the TPMI uncore driver via the registered
uncore_read and uncore_write driver callbacks. These fields are not
populated on older non TPMI hardware.

Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
---
 .../uncore-frequency-common.c                 | 42 ++++++++++++++++---
 .../uncore-frequency-common.h                 | 13 +++++-
 2 files changed, 49 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c
index 4e880585cbe4..e22b683a7a43 100644
--- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c
+++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c
@@ -60,11 +60,16 @@ static ssize_t show_attr(struct uncore_data *data, char *buf, enum uncore_index
 static ssize_t store_attr(struct uncore_data *data, const char *buf, ssize_t count,
 			  enum uncore_index index)
 {
-	unsigned int input;
+	unsigned int input = 0;
 	int ret;
 
-	if (kstrtouint(buf, 10, &input))
-		return -EINVAL;
+	if (index == UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE) {
+		if (kstrtobool(buf, (bool *)&input))
+			return -EINVAL;
+	} else {
+		if (kstrtouint(buf, 10, &input))
+			return -EINVAL;
+	}
 
 	mutex_lock(&uncore_lock);
 	ret = uncore_write(data, input, index);
@@ -103,6 +108,18 @@ show_uncore_attr(max_freq_khz, UNCORE_INDEX_MAX_FREQ);
 
 show_uncore_attr(current_freq_khz, UNCORE_INDEX_CURRENT_FREQ);
 
+store_uncore_attr(elc_low_threshold_percent, UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD);
+store_uncore_attr(elc_high_threshold_percent, UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD);
+store_uncore_attr(elc_high_threshold_enable,
+		  UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE);
+store_uncore_attr(elc_floor_freq_khz, UNCORE_INDEX_EFF_LAT_CTRL_FREQ);
+
+show_uncore_attr(elc_low_threshold_percent, UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD);
+show_uncore_attr(elc_high_threshold_percent, UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD);
+show_uncore_attr(elc_high_threshold_enable,
+		 UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE);
+show_uncore_attr(elc_floor_freq_khz, UNCORE_INDEX_EFF_LAT_CTRL_FREQ);
+
 #define show_uncore_data(member_name)					\
 	static ssize_t show_##member_name(struct kobject *kobj,	\
 					   struct kobj_attribute *attr, char *buf)\
@@ -146,7 +163,8 @@ show_uncore_data(initial_max_freq_khz);
 
 static int create_attr_group(struct uncore_data *data, char *name)
 {
-	int ret, freq, index = 0;
+	int ret, index = 0;
+	unsigned int val;
 
 	init_attribute_rw(max_freq_khz);
 	init_attribute_rw(min_freq_khz);
@@ -168,10 +186,24 @@ static int create_attr_group(struct uncore_data *data, char *name)
 	data->uncore_attrs[index++] = &data->initial_min_freq_khz_kobj_attr.attr;
 	data->uncore_attrs[index++] = &data->initial_max_freq_khz_kobj_attr.attr;
 
-	ret = uncore_read(data, &freq, UNCORE_INDEX_CURRENT_FREQ);
+	ret = uncore_read(data, &val, UNCORE_INDEX_CURRENT_FREQ);
 	if (!ret)
 		data->uncore_attrs[index++] = &data->current_freq_khz_kobj_attr.attr;
 
+	ret = uncore_read(data, &val, UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD);
+	if (!ret) {
+		init_attribute_rw(elc_low_threshold_percent);
+		init_attribute_rw(elc_high_threshold_percent);
+		init_attribute_rw(elc_high_threshold_enable);
+		init_attribute_rw(elc_floor_freq_khz);
+
+		data->uncore_attrs[index++] = &data->elc_low_threshold_percent_kobj_attr.attr;
+		data->uncore_attrs[index++] = &data->elc_high_threshold_percent_kobj_attr.attr;
+		data->uncore_attrs[index++] =
+			&data->elc_high_threshold_enable_kobj_attr.attr;
+		data->uncore_attrs[index++] = &data->elc_floor_freq_khz_kobj_attr.attr;
+	}
+
 	data->uncore_attrs[index] = NULL;
 
 	data->uncore_attr_group.name = name;
diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.h b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.h
index b5c7311bfa05..26c854cd5d97 100644
--- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.h
+++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.h
@@ -34,6 +34,13 @@
  * @domain_id_kobj_attr: Storage for kobject attribute domain_id
  * @fabric_cluster_id_kobj_attr: Storage for kobject attribute fabric_cluster_id
  * @package_id_kobj_attr: Storage for kobject attribute package_id
+ * @elc_low_threshold_percent_kobj_attr:
+		Storage for kobject attribute elc_low_threshold_percent
+ * @elc_high_threshold_percent_kobj_attr:
+		Storage for kobject attribute elc_high_threshold_percent
+ * @elc_high_threshold_enable_kobj_attr:
+		Storage for kobject attribute elc_high_threshold_enable
+ * @elc_floor_freq_khz_kobj_attr: Storage for kobject attribute elc_floor_freq_khz
  * @uncore_attrs:	Attribute storage for group creation
  *
  * This structure is used to encapsulate all data related to uncore sysfs
@@ -61,7 +68,11 @@ struct uncore_data {
 	struct kobj_attribute domain_id_kobj_attr;
 	struct kobj_attribute fabric_cluster_id_kobj_attr;
 	struct kobj_attribute package_id_kobj_attr;
-	struct attribute *uncore_attrs[9];
+	struct kobj_attribute elc_low_threshold_percent_kobj_attr;
+	struct kobj_attribute elc_high_threshold_percent_kobj_attr;
+	struct kobj_attribute elc_high_threshold_enable_kobj_attr;
+	struct kobj_attribute elc_floor_freq_khz_kobj_attr;
+	struct attribute *uncore_attrs[13];
 };
 
 #define UNCORE_DOMAIN_ID_INVALID	-1
-- 
2.43.1

Re: [PATCH 3/3] platform/x86/intel-uncore-freq: Add efficiency latency control to sysfs interface

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 4563 bytes --]

On Wed, 21 Aug 2024, Tero Kristo wrote:

> Add the TPMI efficiency latency control fields to the sysfs interface.
> The sysfs files are mapped to the TPMI uncore driver via the registered
> uncore_read and uncore_write driver callbacks. These fields are not
> populated on older non TPMI hardware.
> 
> Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
> ---
>  .../uncore-frequency-common.c                 | 42 ++++++++++++++++---
>  .../uncore-frequency-common.h                 | 13 +++++-
>  2 files changed, 49 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c
> index 4e880585cbe4..e22b683a7a43 100644
> --- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c
> +++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c
> @@ -60,11 +60,16 @@ static ssize_t show_attr(struct uncore_data *data, char *buf, enum uncore_index
>  static ssize_t store_attr(struct uncore_data *data, const char *buf, ssize_t count,
>  			  enum uncore_index index)
>  {
> -	unsigned int input;
> +	unsigned int input = 0;
>  	int ret;
>  
> -	if (kstrtouint(buf, 10, &input))
> -		return -EINVAL;
> +	if (index == UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE) {
> +		if (kstrtobool(buf, (bool *)&input))
> +			return -EINVAL;
> +	} else {
> +		if (kstrtouint(buf, 10, &input))
> +			return -EINVAL;
> +	}
>  
>  	mutex_lock(&uncore_lock);
>  	ret = uncore_write(data, input, index);
> @@ -103,6 +108,18 @@ show_uncore_attr(max_freq_khz, UNCORE_INDEX_MAX_FREQ);
>  
>  show_uncore_attr(current_freq_khz, UNCORE_INDEX_CURRENT_FREQ);
>  
> +store_uncore_attr(elc_low_threshold_percent, UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD);
> +store_uncore_attr(elc_high_threshold_percent, UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD);
> +store_uncore_attr(elc_high_threshold_enable,
> +		  UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE);
> +store_uncore_attr(elc_floor_freq_khz, UNCORE_INDEX_EFF_LAT_CTRL_FREQ);
> +
> +show_uncore_attr(elc_low_threshold_percent, UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD);
> +show_uncore_attr(elc_high_threshold_percent, UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD);
> +show_uncore_attr(elc_high_threshold_enable,
> +		 UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE);
> +show_uncore_attr(elc_floor_freq_khz, UNCORE_INDEX_EFF_LAT_CTRL_FREQ);
> +
>  #define show_uncore_data(member_name)					\
>  	static ssize_t show_##member_name(struct kobject *kobj,	\
>  					   struct kobj_attribute *attr, char *buf)\
> @@ -146,7 +163,8 @@ show_uncore_data(initial_max_freq_khz);
>  
>  static int create_attr_group(struct uncore_data *data, char *name)
>  {
> -	int ret, freq, index = 0;
> +	int ret, index = 0;
> +	unsigned int val;
>  
>  	init_attribute_rw(max_freq_khz);
>  	init_attribute_rw(min_freq_khz);
> @@ -168,10 +186,24 @@ static int create_attr_group(struct uncore_data *data, char *name)
>  	data->uncore_attrs[index++] = &data->initial_min_freq_khz_kobj_attr.attr;
>  	data->uncore_attrs[index++] = &data->initial_max_freq_khz_kobj_attr.attr;
>  
> -	ret = uncore_read(data, &freq, UNCORE_INDEX_CURRENT_FREQ);
> +	ret = uncore_read(data, &val, UNCORE_INDEX_CURRENT_FREQ);
>  	if (!ret)
>  		data->uncore_attrs[index++] = &data->current_freq_khz_kobj_attr.attr;
>  
> +	ret = uncore_read(data, &val, UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD);
> +	if (!ret) {
> +		init_attribute_rw(elc_low_threshold_percent);
> +		init_attribute_rw(elc_high_threshold_percent);
> +		init_attribute_rw(elc_high_threshold_enable);
> +		init_attribute_rw(elc_floor_freq_khz);
> +
> +		data->uncore_attrs[index++] = &data->elc_low_threshold_percent_kobj_attr.attr;
> +		data->uncore_attrs[index++] = &data->elc_high_threshold_percent_kobj_attr.attr;
> +		data->uncore_attrs[index++] =
> +			&data->elc_high_threshold_enable_kobj_attr.attr;
> +		data->uncore_attrs[index++] = &data->elc_floor_freq_khz_kobj_attr.attr;
> +	}

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

But I have to say I'm not big fan of this function treating any error as 
an implicit indication of ELC not supported.

Is that even going to be true after this:

  https://patchwork.kernel.org/project/platform-driver-x86/patch/20240820204558.1296319-1-srinivas.pandruvada@linux.intel.com/

...as root_domain is eliminated for other reasons than ELC 
supported/not-supported (-ENODATA return path)?

-- 
 i.

Re: [PATCH 3/3] platform/x86/intel-uncore-freq: Add efficiency latency control to sysfs interface

[srinivas pandruvada]

On Fri, 2024-08-23 at 16:03 +0300, Ilpo Järvinen wrote:
> On Wed, 21 Aug 2024, Tero Kristo wrote:
> 
> > Add the TPMI efficiency latency control fields to the sysfs
> > interface.
> > The sysfs files are mapped to the TPMI uncore driver via the
> > registered
> > uncore_read and uncore_write driver callbacks. These fields are not
> > populated on older non TPMI hardware.
> > 
> > Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
> > ---
> >  .../uncore-frequency-common.c                 | 42
> > ++++++++++++++++---
> >  .../uncore-frequency-common.h                 | 13 +++++-
> >  2 files changed, 49 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-
> > frequency-common.c b/drivers/platform/x86/intel/uncore-
> > frequency/uncore-frequency-common.c
> > index 4e880585cbe4..e22b683a7a43 100644
> > --- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-
> > common.c
> > +++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-
> > common.c
> > @@ -60,11 +60,16 @@ static ssize_t show_attr(struct uncore_data
> > *data, char *buf, enum uncore_index
> >  static ssize_t store_attr(struct uncore_data *data, const char
> > *buf, ssize_t count,
> >                           enum uncore_index index)
> >  {
> > -       unsigned int input;
> > +       unsigned int input = 0;
> >         int ret;
> >  
> > -       if (kstrtouint(buf, 10, &input))
> > -               return -EINVAL;
> > +       if (index ==
> > UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE) {
> > +               if (kstrtobool(buf, (bool *)&input))
> > +                       return -EINVAL;
> > +       } else {
> > +               if (kstrtouint(buf, 10, &input))
> > +                       return -EINVAL;
> > +       }
> >  
> >         mutex_lock(&uncore_lock);
> >         ret = uncore_write(data, input, index);
> > @@ -103,6 +108,18 @@ show_uncore_attr(max_freq_khz,
> > UNCORE_INDEX_MAX_FREQ);
> >  
> >  show_uncore_attr(current_freq_khz, UNCORE_INDEX_CURRENT_FREQ);
> >  
> > +store_uncore_attr(elc_low_threshold_percent,
> > UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD);
> > +store_uncore_attr(elc_high_threshold_percent,
> > UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD);
> > +store_uncore_attr(elc_high_threshold_enable,
> > +                 UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE);
> > +store_uncore_attr(elc_floor_freq_khz,
> > UNCORE_INDEX_EFF_LAT_CTRL_FREQ);
> > +
> > +show_uncore_attr(elc_low_threshold_percent,
> > UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD);
> > +show_uncore_attr(elc_high_threshold_percent,
> > UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD);
> > +show_uncore_attr(elc_high_threshold_enable,
> > +                UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE);
> > +show_uncore_attr(elc_floor_freq_khz,
> > UNCORE_INDEX_EFF_LAT_CTRL_FREQ);
> > +
> >  #define
> > show_uncore_data(member_name)                                  \
> >         static ssize_t show_##member_name(struct kobject *kobj, \
> >                                            struct kobj_attribute
> > *attr, char *buf)\
> > @@ -146,7 +163,8 @@ show_uncore_data(initial_max_freq_khz);
> >  
> >  static int create_attr_group(struct uncore_data *data, char *name)
> >  {
> > -       int ret, freq, index = 0;
> > +       int ret, index = 0;
> > +       unsigned int val;
> >  
> >         init_attribute_rw(max_freq_khz);
> >         init_attribute_rw(min_freq_khz);
> > @@ -168,10 +186,24 @@ static int create_attr_group(struct
> > uncore_data *data, char *name)
> >         data->uncore_attrs[index++] = &data-
> > >initial_min_freq_khz_kobj_attr.attr;
> >         data->uncore_attrs[index++] = &data-
> > >initial_max_freq_khz_kobj_attr.attr;
> >  
> > -       ret = uncore_read(data, &freq, UNCORE_INDEX_CURRENT_FREQ);
> > +       ret = uncore_read(data, &val, UNCORE_INDEX_CURRENT_FREQ);
> >         if (!ret)
> >                 data->uncore_attrs[index++] = &data-
> > >current_freq_khz_kobj_attr.attr;
> >  
> > +       ret = uncore_read(data, &val,
> > UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD);
> > +       if (!ret) {
> > +               init_attribute_rw(elc_low_threshold_percent);
> > +               init_attribute_rw(elc_high_threshold_percent);
> > +               init_attribute_rw(elc_high_threshold_enable);
> > +               init_attribute_rw(elc_floor_freq_khz);
> > +
> > +               data->uncore_attrs[index++] = &data-
> > >elc_low_threshold_percent_kobj_attr.attr;
> > +               data->uncore_attrs[index++] = &data-
> > >elc_high_threshold_percent_kobj_attr.attr;
> > +               data->uncore_attrs[index++] =
> > +                       &data-
> > >elc_high_threshold_enable_kobj_attr.attr;
> > +               data->uncore_attrs[index++] = &data-
> > >elc_floor_freq_khz_kobj_attr.attr;
> > +       }
> 
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> 
> But I have to say I'm not big fan of this function treating any error
> as 
> an implicit indication of ELC not supported.
Also there is a check for version number, which supports ELC. So this
condition will never be true unless some IO read failure.

> 
> Is that even going to be true after this:
> 
>  
> https://patchwork.kernel.org/project/platform-driver-x86/patch/20240820204558.1296319-1-srinivas.pandruvada@linux.intel.com/
> 
> ...as root_domain is eliminated for other reasons than ELC 
> supported/not-supported (-ENODATA return path)?
Even if ELC is not supported, but all others fields will always be
supported from base version. The above change doesn't do anything with
root domain.

Thanks,
Srinivas
> 

Re: [PATCH 3/3] platform/x86/intel-uncore-freq: Add efficiency latency control to sysfs interface

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 7619 bytes --]

On Fri, 23 Aug 2024, srinivas pandruvada wrote:
> On Fri, 2024-08-23 at 16:03 +0300, Ilpo Järvinen wrote:
> > On Wed, 21 Aug 2024, Tero Kristo wrote:
> > 
> > > Add the TPMI efficiency latency control fields to the sysfs
> > > interface.
> > > The sysfs files are mapped to the TPMI uncore driver via the
> > > registered
> > > uncore_read and uncore_write driver callbacks. These fields are not
> > > populated on older non TPMI hardware.
> > > 
> > > Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
> > > ---
> > >  .../uncore-frequency-common.c                 | 42
> > > ++++++++++++++++---
> > >  .../uncore-frequency-common.h                 | 13 +++++-
> > >  2 files changed, 49 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-
> > > frequency-common.c b/drivers/platform/x86/intel/uncore-
> > > frequency/uncore-frequency-common.c
> > > index 4e880585cbe4..e22b683a7a43 100644
> > > --- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-
> > > common.c
> > > +++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-
> > > common.c
> > > @@ -60,11 +60,16 @@ static ssize_t show_attr(struct uncore_data
> > > *data, char *buf, enum uncore_index
> > >  static ssize_t store_attr(struct uncore_data *data, const char
> > > *buf, ssize_t count,
> > >                           enum uncore_index index)
> > >  {
> > > -       unsigned int input;
> > > +       unsigned int input = 0;
> > >         int ret;
> > >  
> > > -       if (kstrtouint(buf, 10, &input))
> > > -               return -EINVAL;
> > > +       if (index ==
> > > UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE) {
> > > +               if (kstrtobool(buf, (bool *)&input))
> > > +                       return -EINVAL;
> > > +       } else {
> > > +               if (kstrtouint(buf, 10, &input))
> > > +                       return -EINVAL;
> > > +       }
> > >  
> > >         mutex_lock(&uncore_lock);
> > >         ret = uncore_write(data, input, index);
> > > @@ -103,6 +108,18 @@ show_uncore_attr(max_freq_khz,
> > > UNCORE_INDEX_MAX_FREQ);
> > >  
> > >  show_uncore_attr(current_freq_khz, UNCORE_INDEX_CURRENT_FREQ);
> > >  
> > > +store_uncore_attr(elc_low_threshold_percent,
> > > UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD);
> > > +store_uncore_attr(elc_high_threshold_percent,
> > > UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD);
> > > +store_uncore_attr(elc_high_threshold_enable,
> > > +                 UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE);
> > > +store_uncore_attr(elc_floor_freq_khz,
> > > UNCORE_INDEX_EFF_LAT_CTRL_FREQ);
> > > +
> > > +show_uncore_attr(elc_low_threshold_percent,
> > > UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD);
> > > +show_uncore_attr(elc_high_threshold_percent,
> > > UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD);
> > > +show_uncore_attr(elc_high_threshold_enable,
> > > +                UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE);
> > > +show_uncore_attr(elc_floor_freq_khz,
> > > UNCORE_INDEX_EFF_LAT_CTRL_FREQ);
> > > +
> > >  #define
> > > show_uncore_data(member_name)                                  \
> > >         static ssize_t show_##member_name(struct kobject *kobj, \
> > >                                            struct kobj_attribute
> > > *attr, char *buf)\
> > > @@ -146,7 +163,8 @@ show_uncore_data(initial_max_freq_khz);
> > >  
> > >  static int create_attr_group(struct uncore_data *data, char *name)
> > >  {
> > > -       int ret, freq, index = 0;
> > > +       int ret, index = 0;
> > > +       unsigned int val;
> > >  
> > >         init_attribute_rw(max_freq_khz);
> > >         init_attribute_rw(min_freq_khz);
> > > @@ -168,10 +186,24 @@ static int create_attr_group(struct
> > > uncore_data *data, char *name)
> > >         data->uncore_attrs[index++] = &data-
> > > >initial_min_freq_khz_kobj_attr.attr;
> > >         data->uncore_attrs[index++] = &data-
> > > >initial_max_freq_khz_kobj_attr.attr;
> > >  
> > > -       ret = uncore_read(data, &freq, UNCORE_INDEX_CURRENT_FREQ);
> > > +       ret = uncore_read(data, &val, UNCORE_INDEX_CURRENT_FREQ);
> > >         if (!ret)
> > >                 data->uncore_attrs[index++] = &data-
> > > >current_freq_khz_kobj_attr.attr;
> > >  
> > > +       ret = uncore_read(data, &val,
> > > UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD);
> > > +       if (!ret) {
> > > +               init_attribute_rw(elc_low_threshold_percent);
> > > +               init_attribute_rw(elc_high_threshold_percent);
> > > +               init_attribute_rw(elc_high_threshold_enable);
> > > +               init_attribute_rw(elc_floor_freq_khz);
> > > +
> > > +               data->uncore_attrs[index++] = &data-
> > > >elc_low_threshold_percent_kobj_attr.attr;
> > > +               data->uncore_attrs[index++] = &data-
> > > >elc_high_threshold_percent_kobj_attr.attr;
> > > +               data->uncore_attrs[index++] =
> > > +                       &data-
> > > >elc_high_threshold_enable_kobj_attr.attr;
> > > +               data->uncore_attrs[index++] = &data-
> > > >elc_floor_freq_khz_kobj_attr.attr;
> > > +       }
> > 
> > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > 
> > But I have to say I'm not big fan of this function treating any error
> > as 
> > an implicit indication of ELC not supported.
>
> Also there is a check for version number, which supports ELC.

AFAICT, the version number check is not on the path that is called from 
create_attr_group().

The version number check is in uncore_probe() which then propagates this 
knowledge into read/write_eff_lat_ctrl() using ->elc_supported.

> So this
> condition will never be true unless some IO read failure.

So are you saying ->elc_supported check is not required (added by patch 
2)? It return -EOPNOTSUPP not because of an "IO read failure"??

> > Is that even going to be true after this:
> > 
> >  
> > https://patchwork.kernel.org/project/platform-driver-x86/patch/20240820204558.1296319-1-srinivas.pandruvada@linux.intel.com/
> > 
> > ...as root_domain is eliminated for other reasons than ELC 
> > supported/not-supported (-ENODATA return path)?
>
> Even if ELC is not supported, but all others fields will always be
> supported from base version. The above change doesn't do anything with
> root domain.

??

read/write_eff_lat_ctrl() check for ->root_domain and return -ENODATA
if it is true. If that patch from you I linked above is applied, this line 
won't execute on some systems:

	tpmi_uncore->root_cluster.root_domain = true;

Will that cause an issue (for read/write_eff_lat_ctrl())?

My concern here is that misusing error values like this to do 
supported/not-supported check leads to fragility that would not occur
if errors would be treated as hard errors and supported is checked by 
other means (which would be easy here using ->elc_supported, AFAICT).

-- 
 i.

Re: [PATCH 3/3] platform/x86/intel-uncore-freq: Add efficiency latency control to sysfs interface

[srinivas pandruvada]

On Fri, 2024-08-23 at 16:29 +0300, Ilpo Järvinen wrote:
> On Fri, 23 Aug 2024, srinivas pandruvada wrote:
> > On Fri, 2024-08-23 at 16:03 +0300, Ilpo Järvinen wrote:
> > > On Wed, 21 Aug 2024, Tero Kristo wrote:
> > > 
> > > > Add the TPMI efficiency latency control fields to the sysfs
> > > > interface.
> > > > The sysfs files are mapped to the TPMI uncore driver via the
> > > > registered
> > > > uncore_read and uncore_write driver callbacks. These fields are
> > > > not
> > > > populated on older non TPMI hardware.
> > > > 
> > > > Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
> > > > ---
> > > >  .../uncore-frequency-common.c                 | 42
> > > > ++++++++++++++++---
> > > >  .../uncore-frequency-common.h                 | 13 +++++-
> > > >  2 files changed, 49 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/platform/x86/intel/uncore-
> > > > frequency/uncore-
> > > > frequency-common.c b/drivers/platform/x86/intel/uncore-
> > > > frequency/uncore-frequency-common.c
> > > > index 4e880585cbe4..e22b683a7a43 100644
> > > > --- a/drivers/platform/x86/intel/uncore-frequency/uncore-
> > > > frequency-
> > > > common.c
> > > > +++ b/drivers/platform/x86/intel/uncore-frequency/uncore-
> > > > frequency-
> > > > common.c
> > > > @@ -60,11 +60,16 @@ static ssize_t show_attr(struct uncore_data
> > > > *data, char *buf, enum uncore_index
> > > >  static ssize_t store_attr(struct uncore_data *data, const char
> > > > *buf, ssize_t count,
> > > >                           enum uncore_index index)
> > > >  {
> > > > -       unsigned int input;
> > > > +       unsigned int input = 0;
> > > >         int ret;
> > > >  
> > > > -       if (kstrtouint(buf, 10, &input))
> > > > -               return -EINVAL;
> > > > +       if (index ==
> > > > UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE) {
> > > > +               if (kstrtobool(buf, (bool *)&input))
> > > > +                       return -EINVAL;
> > > > +       } else {
> > > > +               if (kstrtouint(buf, 10, &input))
> > > > +                       return -EINVAL;
> > > > +       }
> > > >  
> > > >         mutex_lock(&uncore_lock);
> > > >         ret = uncore_write(data, input, index);
> > > > @@ -103,6 +108,18 @@ show_uncore_attr(max_freq_khz,
> > > > UNCORE_INDEX_MAX_FREQ);
> > > >  
> > > >  show_uncore_attr(current_freq_khz, UNCORE_INDEX_CURRENT_FREQ);
> > > >  
> > > > +store_uncore_attr(elc_low_threshold_percent,
> > > > UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD);
> > > > +store_uncore_attr(elc_high_threshold_percent,
> > > > UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD);
> > > > +store_uncore_attr(elc_high_threshold_enable,
> > > > +                
> > > > UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE);
> > > > +store_uncore_attr(elc_floor_freq_khz,
> > > > UNCORE_INDEX_EFF_LAT_CTRL_FREQ);
> > > > +
> > > > +show_uncore_attr(elc_low_threshold_percent,
> > > > UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD);
> > > > +show_uncore_attr(elc_high_threshold_percent,
> > > > UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD);
> > > > +show_uncore_attr(elc_high_threshold_enable,
> > > > +               
> > > > UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE);
> > > > +show_uncore_attr(elc_floor_freq_khz,
> > > > UNCORE_INDEX_EFF_LAT_CTRL_FREQ);
> > > > +
> > > >  #define
> > > > show_uncore_data(member_name)                                  
> > > > \
> > > >         static ssize_t show_##member_name(struct kobject
> > > > *kobj, \
> > > >                                            struct
> > > > kobj_attribute
> > > > *attr, char *buf)\
> > > > @@ -146,7 +163,8 @@ show_uncore_data(initial_max_freq_khz);
> > > >  
> > > >  static int create_attr_group(struct uncore_data *data, char
> > > > *name)
> > > >  {
> > > > -       int ret, freq, index = 0;
> > > > +       int ret, index = 0;
> > > > +       unsigned int val;
> > > >  
> > > >         init_attribute_rw(max_freq_khz);
> > > >         init_attribute_rw(min_freq_khz);
> > > > @@ -168,10 +186,24 @@ static int create_attr_group(struct
> > > > uncore_data *data, char *name)
> > > >         data->uncore_attrs[index++] = &data-
> > > > > initial_min_freq_khz_kobj_attr.attr;
> > > >         data->uncore_attrs[index++] = &data-
> > > > > initial_max_freq_khz_kobj_attr.attr;
> > > >  
> > > > -       ret = uncore_read(data, &freq,
> > > > UNCORE_INDEX_CURRENT_FREQ);
> > > > +       ret = uncore_read(data, &val,
> > > > UNCORE_INDEX_CURRENT_FREQ);
> > > >         if (!ret)
> > > >                 data->uncore_attrs[index++] = &data-
> > > > > current_freq_khz_kobj_attr.attr;
> > > >  
> > > > +       ret = uncore_read(data, &val,
> > > > UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD);
> > > > +       if (!ret) {
> > > > +               init_attribute_rw(elc_low_threshold_percent);
> > > > +               init_attribute_rw(elc_high_threshold_percent);
> > > > +               init_attribute_rw(elc_high_threshold_enable);
> > > > +               init_attribute_rw(elc_floor_freq_khz);
> > > > +
> > > > +               data->uncore_attrs[index++] = &data-
> > > > > elc_low_threshold_percent_kobj_attr.attr;
> > > > +               data->uncore_attrs[index++] = &data-
> > > > > elc_high_threshold_percent_kobj_attr.attr;
> > > > +               data->uncore_attrs[index++] =
> > > > +                       &data-
> > > > > elc_high_threshold_enable_kobj_attr.attr;
> > > > +               data->uncore_attrs[index++] = &data-
> > > > > elc_floor_freq_khz_kobj_attr.attr;
> > > > +       }
> > > 
> > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > 
> > > But I have to say I'm not big fan of this function treating any
> > > error
> > > as 
> > > an implicit indication of ELC not supported.
> > 
> > Also there is a check for version number, which supports ELC.
> 
> AFAICT, the version number check is not on the path that is called
> from 
> create_attr_group().
> 
> The version number check is in uncore_probe() which then propagates
> this 
> knowledge into read/write_eff_lat_ctrl() using ->elc_supported.

I mean uncore_read() should fail if the current platform doesn't
support ELC.
Here that check is via a flag cluster_info->elc_supported.

> 
> > So this
> > condition will never be true unless some IO read failure.
> 
> So are you saying ->elc_supported check is not required (added by
> patch 
> 2)? It return -EOPNOTSUPP not because of an "IO read failure"??
> 
I take back IO read fail. readq() will never fail here. uncore_read()
can only fail on non TPMI platforms only for IO issues.

 We should check elc_supported.


> > > Is that even going to be true after this:
> > > 
> > >  
> > > https://patchwork.kernel.org/project/platform-driver-x86/patch/20240820204558.1296319-1-srinivas.pandruvada@linux.intel.com/
> > > 
> > > ...as root_domain is eliminated for other reasons than ELC 
> > > supported/not-supported (-ENODATA return path)?
> > 
> > Even if ELC is not supported, but all others fields will always be
> > supported from base version. The above change doesn't do anything
> > with
> > root domain.
> 
> ??
> 
> read/write_eff_lat_ctrl() check for ->root_domain and return -ENODATA
> if it is true. If that patch from you I linked above is applied, this
> line 
> won't execute on some systems:
> 
>         tpmi_uncore->root_cluster.root_domain = true;
> 
Yes and will return without calling any callbacks for any attribute for
root domain only on these systems. So read/write_eff_lat_ctrl() will
not be called for root domain. For other domains the callbacks are
called before this check. 

> Will that cause an issue (for read/write_eff_lat_ctrl())?
We don't present ELC fields on root_domain on any system.

Can you tell what kind of issues you are worried about, may be I am not
getting?

> 
> My concern here is that misusing error values like this to do 
> supported/not-supported check leads to fragility that would not occur
> if errors would be treated as hard errors and supported is checked by
> other means (which would be easy here using ->elc_supported, AFAICT).
> 

Attribute creation is in common part which includes non TPMI systems,
which we still support for all clients for several gens.

We can add a feature mask as part of struct uncore_data and avoid
calling uncore_read() and treat uncore_read() error as hard errors as a
separate change. elc_supported can be moved to this structure, but I
want to avoid as we will be adding some more features, which are again
TPMI specific, so more flags will be needed.

Thanks,
Srinivas

Re: [PATCH 3/3] platform/x86/intel-uncore-freq: Add efficiency latency control to sysfs interface

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 10059 bytes --]

On Fri, 23 Aug 2024, srinivas pandruvada wrote:

> On Fri, 2024-08-23 at 16:29 +0300, Ilpo Järvinen wrote:
> > On Fri, 23 Aug 2024, srinivas pandruvada wrote:
> > > On Fri, 2024-08-23 at 16:03 +0300, Ilpo Järvinen wrote:
> > > > On Wed, 21 Aug 2024, Tero Kristo wrote:
> > > > 
> > > > > Add the TPMI efficiency latency control fields to the sysfs
> > > > > interface.
> > > > > The sysfs files are mapped to the TPMI uncore driver via the
> > > > > registered
> > > > > uncore_read and uncore_write driver callbacks. These fields are
> > > > > not
> > > > > populated on older non TPMI hardware.
> > > > > 
> > > > > Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
> > > > > ---
> > > > >  .../uncore-frequency-common.c                 | 42
> > > > > ++++++++++++++++---
> > > > >  .../uncore-frequency-common.h                 | 13 +++++-
> > > > >  2 files changed, 49 insertions(+), 6 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/platform/x86/intel/uncore-
> > > > > frequency/uncore-
> > > > > frequency-common.c b/drivers/platform/x86/intel/uncore-
> > > > > frequency/uncore-frequency-common.c
> > > > > index 4e880585cbe4..e22b683a7a43 100644
> > > > > --- a/drivers/platform/x86/intel/uncore-frequency/uncore-
> > > > > frequency-
> > > > > common.c
> > > > > +++ b/drivers/platform/x86/intel/uncore-frequency/uncore-
> > > > > frequency-
> > > > > common.c
> > > > > @@ -60,11 +60,16 @@ static ssize_t show_attr(struct uncore_data
> > > > > *data, char *buf, enum uncore_index
> > > > >  static ssize_t store_attr(struct uncore_data *data, const char
> > > > > *buf, ssize_t count,
> > > > >                           enum uncore_index index)
> > > > >  {
> > > > > -       unsigned int input;
> > > > > +       unsigned int input = 0;
> > > > >         int ret;
> > > > >  
> > > > > -       if (kstrtouint(buf, 10, &input))
> > > > > -               return -EINVAL;
> > > > > +       if (index ==
> > > > > UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE) {
> > > > > +               if (kstrtobool(buf, (bool *)&input))
> > > > > +                       return -EINVAL;
> > > > > +       } else {
> > > > > +               if (kstrtouint(buf, 10, &input))
> > > > > +                       return -EINVAL;
> > > > > +       }
> > > > >  
> > > > >         mutex_lock(&uncore_lock);
> > > > >         ret = uncore_write(data, input, index);
> > > > > @@ -103,6 +108,18 @@ show_uncore_attr(max_freq_khz,
> > > > > UNCORE_INDEX_MAX_FREQ);
> > > > >  
> > > > >  show_uncore_attr(current_freq_khz, UNCORE_INDEX_CURRENT_FREQ);
> > > > >  
> > > > > +store_uncore_attr(elc_low_threshold_percent,
> > > > > UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD);
> > > > > +store_uncore_attr(elc_high_threshold_percent,
> > > > > UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD);
> > > > > +store_uncore_attr(elc_high_threshold_enable,
> > > > > +                
> > > > > UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE);
> > > > > +store_uncore_attr(elc_floor_freq_khz,
> > > > > UNCORE_INDEX_EFF_LAT_CTRL_FREQ);
> > > > > +
> > > > > +show_uncore_attr(elc_low_threshold_percent,
> > > > > UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD);
> > > > > +show_uncore_attr(elc_high_threshold_percent,
> > > > > UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD);
> > > > > +show_uncore_attr(elc_high_threshold_enable,
> > > > > +               
> > > > > UNCORE_INDEX_EFF_LAT_CTRL_HIGH_THRESHOLD_ENABLE);
> > > > > +show_uncore_attr(elc_floor_freq_khz,
> > > > > UNCORE_INDEX_EFF_LAT_CTRL_FREQ);
> > > > > +
> > > > >  #define
> > > > > show_uncore_data(member_name)                                  
> > > > > \
> > > > >         static ssize_t show_##member_name(struct kobject
> > > > > *kobj, \
> > > > >                                            struct
> > > > > kobj_attribute
> > > > > *attr, char *buf)\
> > > > > @@ -146,7 +163,8 @@ show_uncore_data(initial_max_freq_khz);
> > > > >  
> > > > >  static int create_attr_group(struct uncore_data *data, char
> > > > > *name)
> > > > >  {
> > > > > -       int ret, freq, index = 0;
> > > > > +       int ret, index = 0;
> > > > > +       unsigned int val;
> > > > >  
> > > > >         init_attribute_rw(max_freq_khz);
> > > > >         init_attribute_rw(min_freq_khz);
> > > > > @@ -168,10 +186,24 @@ static int create_attr_group(struct
> > > > > uncore_data *data, char *name)
> > > > >         data->uncore_attrs[index++] = &data-
> > > > > > initial_min_freq_khz_kobj_attr.attr;
> > > > >         data->uncore_attrs[index++] = &data-
> > > > > > initial_max_freq_khz_kobj_attr.attr;
> > > > >  
> > > > > -       ret = uncore_read(data, &freq,
> > > > > UNCORE_INDEX_CURRENT_FREQ);
> > > > > +       ret = uncore_read(data, &val,
> > > > > UNCORE_INDEX_CURRENT_FREQ);
> > > > >         if (!ret)
> > > > >                 data->uncore_attrs[index++] = &data-
> > > > > > current_freq_khz_kobj_attr.attr;
> > > > >  
> > > > > +       ret = uncore_read(data, &val,
> > > > > UNCORE_INDEX_EFF_LAT_CTRL_LOW_THRESHOLD);
> > > > > +       if (!ret) {
> > > > > +               init_attribute_rw(elc_low_threshold_percent);
> > > > > +               init_attribute_rw(elc_high_threshold_percent);
> > > > > +               init_attribute_rw(elc_high_threshold_enable);
> > > > > +               init_attribute_rw(elc_floor_freq_khz);
> > > > > +
> > > > > +               data->uncore_attrs[index++] = &data-
> > > > > > elc_low_threshold_percent_kobj_attr.attr;
> > > > > +               data->uncore_attrs[index++] = &data-
> > > > > > elc_high_threshold_percent_kobj_attr.attr;
> > > > > +               data->uncore_attrs[index++] =
> > > > > +                       &data-
> > > > > > elc_high_threshold_enable_kobj_attr.attr;
> > > > > +               data->uncore_attrs[index++] = &data-
> > > > > > elc_floor_freq_khz_kobj_attr.attr;
> > > > > +       }
> > > > 
> > > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > > 
> > > > But I have to say I'm not big fan of this function treating any
> > > > error
> > > > as 
> > > > an implicit indication of ELC not supported.
> > > 
> > > Also there is a check for version number, which supports ELC.
> > 
> > AFAICT, the version number check is not on the path that is called
> > from 
> > create_attr_group().
> > 
> > The version number check is in uncore_probe() which then propagates
> > this 
> > knowledge into read/write_eff_lat_ctrl() using ->elc_supported.
> 
> I mean uncore_read() should fail if the current platform doesn't
> support ELC.
> Here that check is via a flag cluster_info->elc_supported.
> 
> > 
> > > So this
> > > condition will never be true unless some IO read failure.
> > 
> > So are you saying ->elc_supported check is not required (added by
> > patch 
> > 2)? It return -EOPNOTSUPP not because of an "IO read failure"??
> > 
> I take back IO read fail. readq() will never fail here. uncore_read()
> can only fail on non TPMI platforms only for IO issues.
> 
>  We should check elc_supported.
> 
> 
> > > > Is that even going to be true after this:
> > > > 
> > > >  
> > > > https://patchwork.kernel.org/project/platform-driver-x86/patch/20240820204558.1296319-1-srinivas.pandruvada@linux.intel.com/
> > > > 
> > > > ...as root_domain is eliminated for other reasons than ELC 
> > > > supported/not-supported (-ENODATA return path)?
> > > 
> > > Even if ELC is not supported, but all others fields will always be
> > > supported from base version. The above change doesn't do anything
> > > with
> > > root domain.
> > 
> > ??
> > 
> > read/write_eff_lat_ctrl() check for ->root_domain and return -ENODATA
> > if it is true. If that patch from you I linked above is applied, this
> > line 
> > won't execute on some systems:
> > 
> >         tpmi_uncore->root_cluster.root_domain = true;
> > 
> Yes and will return without calling any callbacks for any attribute for
> root domain only on these systems. So read/write_eff_lat_ctrl() will
> not be called for root domain. For other domains the callbacks are
> called before this check. 

Okay, I see. I was missing this piece of understanding about the root 
domain.

> > Will that cause an issue (for read/write_eff_lat_ctrl())?
> We don't present ELC fields on root_domain on any system.
> 
> Can you tell what kind of issues you are worried about, may be I am not
> getting?

I don't think there will any issue with that other patch now that I 
understand the internals a bit better than before.

> > My concern here is that misusing error values like this to do 
> > supported/not-supported check leads to fragility that would not occur
> > if errors would be treated as hard errors and supported is checked by
> > other means (which would be easy here using ->elc_supported, AFAICT).
> > 
> 
> Attribute creation is in common part which includes non TPMI systems,
> which we still support for all clients for several gens.
> 
> We can add a feature mask as part of struct uncore_data and avoid
> calling uncore_read() and treat uncore_read() error as hard errors as a
> separate change. elc_supported can be moved to this structure, but I
> want to avoid as we will be adding some more features, which are again
> TPMI specific, so more flags will be needed.

Okay, lets just keep things as they are for now.

-- 
 i.