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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

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

On Fri, 30 Aug 2024, Tero Kristo wrote:

> On Thu, 2024-08-29 at 12:14 +0300, Ilpo Järvinen wrote:
> > On Wed, 28 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>
> > > ---
> > > v2:
> > >   * Converted a long sequence of if (...)'s to a switch
> > > 
> > >  .../uncore-frequency-common.h                 |   4 +
> > >  .../uncore-frequency/uncore-frequency-tpmi.c  | 158
> > > +++++++++++++++++-
> > >  2 files changed, 160 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..50b28b4b1fc0 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));
> > 
> > I wonder if DIV_ROUND_CLOSEST() would be more appropriate in these
> > two 
> > cases, rounding up isn't well justified as I think this wants to
> > round it 
> > back to the original number to deal with the minor divergences due to
> > precision loss during conversions?
> 
> Yes, this makes it sure that what is written to the file gets also read
> back as the same value. Using DIV_ROUND_CLOSEST() will not do the
> trick. Tried this out by using DIV_ROUND_CLOSEST() and got following:
> 
> # echo 94 > elc_high_threshold_percent 
> # cat elc_high_threshold_percent
> 94
> # echo 95 > elc_high_threshold_percent 
> # cat elc_high_threshold_percent 
> 94
> # echo 96 > elc_high_threshold_percent 
> # cat elc_high_threshold_percent 
> 95
> 
> However, using DIV_ROUND_UP() all values from 0-100 work just fine.

Okay, thanks for checking this out.

-- 
 i.