Re: [PATCH 1/2] powercap/rapl: handle missing msrs

[Rafael J. Wysocki]

On Monday, June 13, 2016 11:00:26 PM Rafael J. Wysocki wrote:
> On Monday, June 13, 2016 11:53:10 AM jacob wrote:
> > Hi Rafael,
> > 
> > Any feedback? It that is OK, can you take this patch independent of the
> > second patch (which is going into tip tree)?
> 
> I'll do that.

Applied now, thanks!

Re: [PATCH 1/2] powercap/rapl: handle missing msrs

[Rafael J. Wysocki]

On Monday, June 13, 2016 11:00:26 PM Rafael J. Wysocki wrote:
> On Monday, June 13, 2016 11:53:10 AM jacob wrote:
> > Hi Rafael,
> > 
> > Any feedback? It that is OK, can you take this patch independent of the
> > second patch (which is going into tip tree)?
> 
> I'll do that.

Applied now, thanks!

Re: [PATCH 1/2] powercap/rapl: handle missing msrs

[Rafael J. Wysocki]

On Monday, June 13, 2016 11:53:10 AM jacob wrote:
> Hi Rafael,
> 
> Any feedback? It that is OK, can you take this patch independent of the
> second patch (which is going into tip tree)?

I'll do that.

Thanks,
Rafael

Re: [PATCH 1/2] powercap/rapl: handle missing msrs

[Rafael J. Wysocki]

On Monday, June 13, 2016 11:53:10 AM jacob wrote:
> Hi Rafael,
> 
> Any feedback? It that is OK, can you take this patch independent of the
> second patch (which is going into tip tree)?

I'll do that.

Thanks,
Rafael

Re: [PATCH 1/2] powercap/rapl: handle missing msrs

[jacob]

Hi Rafael,

Any feedback? It that is OK, can you take this patch independent of the
second patch (which is going into tip tree)?
[PATCH 2/2] powercap/rapl: add support for denverton

This patch affects some Broxton/Apollo Lake where the missing MSR will
cause the driver fail to load.

On Tue, 31 May 2016 13:41:29 -0700
Jacob Pan  wrote:

> Some RAPL MSRs may not exist on some CPUs, we need to continue
> the topology detection and enumerate what is available.
> 
> This patch handles the missing MSRs then report to powercap
> layer only the features available.
> 
> Signed-off-by: Jacob Pan 
> ---
>  drivers/powercap/intel_rapl.c | 103
> -- 1 file changed, 79
> insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/powercap/intel_rapl.c
> b/drivers/powercap/intel_rapl.c index b0a2dc4..d51a8d4 100644
> --- a/drivers/powercap/intel_rapl.c
> +++ b/drivers/powercap/intel_rapl.c
> @@ -329,14 +329,14 @@ static int release_zone(struct powercap_zone
> *power_zone) 
>  static int find_nr_power_limit(struct rapl_domain *rd)
>  {
> - int i;
> + int i, nr_pl = 0;
>  
>   for (i = 0; i < NR_POWER_LIMITS; i++) {
> - if (rd->rpl[i].name == NULL)
> - break;
> + if (rd->rpl[i].name)
> + nr_pl++;
>   }
>  
> - return i;
> + return nr_pl;
>  }
>  
>  static int set_domain_enable(struct powercap_zone *power_zone, bool
> mode) @@ -411,15 +411,38 @@ static const struct powercap_zone_ops
> zone_ops[] = { },
>  };
>  
> -static int set_power_limit(struct powercap_zone *power_zone, int id,
> +
> +/*
> + * Constraint index used by powercap can be different than power
> limit (PL)
> + * index in that some  PLs maybe missing due to non-existant MSRs.
> So we
> + * need to convert here by finding the valid PLs only (name
> populated).
> + */
> +static int contraint_to_pl(struct rapl_domain *rd, int cid)
> +{
> + int i, j;
> +
> + for (i = 0, j = 0; i < NR_POWER_LIMITS; i++) {
> + if ((rd->rpl[i].name) && j++ == cid) {
> + pr_debug("%s: index %d\n", __func__, i);
> + return i;
> + }
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int set_power_limit(struct powercap_zone *power_zone, int cid,
>   u64 power_limit)
>  {
>   struct rapl_domain *rd;
>   struct rapl_package *rp;
>   int ret = 0;
> + int id;
>  
>   get_online_cpus();
>   rd = power_zone_to_rapl_domain(power_zone);
> + id = contraint_to_pl(rd, cid);
> +
>   rp = rd->rp;
>  
>   if (rd->state & DOMAIN_STATE_BIOS_LOCKED) {
> @@ -446,16 +469,18 @@ set_exit:
>   return ret;
>  }
>  
> -static int get_current_power_limit(struct powercap_zone *power_zone,
> int id, +static int get_current_power_limit(struct powercap_zone
> *power_zone, int cid, u64 *data)
>  {
>   struct rapl_domain *rd;
>   u64 val;
>   int prim;
>   int ret = 0;
> + int id;
>  
>   get_online_cpus();
>   rd = power_zone_to_rapl_domain(power_zone);
> + id = contraint_to_pl(rd, cid);
>   switch (rd->rpl[id].prim_id) {
>   case PL1_ENABLE:
>   prim = POWER_LIMIT1;
> @@ -477,14 +502,17 @@ static int get_current_power_limit(struct
> powercap_zone *power_zone, int id, return ret;
>  }
>  
> -static int set_time_window(struct powercap_zone *power_zone, int id,
> +static int set_time_window(struct powercap_zone *power_zone, int cid,
>   u64
> window) {
>   struct rapl_domain *rd;
>   int ret = 0;
> + int id;
>  
>   get_online_cpus();
>   rd = power_zone_to_rapl_domain(power_zone);
> + id = contraint_to_pl(rd, cid);
> +
>   switch (rd->rpl[id].prim_id) {
>   case PL1_ENABLE:
>   rapl_write_data_raw(rd, TIME_WINDOW1, window);
> @@ -499,14 +527,17 @@ static int set_time_window(struct powercap_zone
> *power_zone, int id, return ret;
>  }
>  
> -static int get_time_window(struct powercap_zone *power_zone, int id,
> u64 *data) +static int get_time_window(struct powercap_zone
> *power_zone, int cid, u64 *data) {
>   struct rapl_domain *rd;
>   u64 val;
>   int ret = 0;
> + int id;
>  
>   get_online_cpus();
>   rd = power_zone_to_rapl_domain(power_zone);
> + id = contraint_to_pl(rd, cid);
> +
>   switch (rd->rpl[id].prim_id) {
>   case PL1_ENABLE:
>   ret = rapl_read_data_raw(rd, TIME_WINDOW1, true,
> ); @@ -525,15 +556,17 @@ static int get_time_window(struct
> powercap_zone *power_zone, int id, u64 *data) return ret;
>  }
>  
> -static const char *get_constraint_name(struct powercap_zone
> *power_zone, int id) +static const char *get_constraint_name(struct
> powercap_zone *power_zone, int cid) {
> - struct rapl_power_limit *rpl;
>   struct rapl_domain *rd;
> + int id;
>  
>   rd = 

Re: [PATCH 1/2] powercap/rapl: handle missing msrs

[jacob]

Hi Rafael,

Any feedback? It that is OK, can you take this patch independent of the
second patch (which is going into tip tree)?
[PATCH 2/2] powercap/rapl: add support for denverton

This patch affects some Broxton/Apollo Lake where the missing MSR will
cause the driver fail to load.

On Tue, 31 May 2016 13:41:29 -0700
Jacob Pan  wrote:

> Some RAPL MSRs may not exist on some CPUs, we need to continue
> the topology detection and enumerate what is available.
> 
> This patch handles the missing MSRs then report to powercap
> layer only the features available.
> 
> Signed-off-by: Jacob Pan 
> ---
>  drivers/powercap/intel_rapl.c | 103
> -- 1 file changed, 79
> insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/powercap/intel_rapl.c
> b/drivers/powercap/intel_rapl.c index b0a2dc4..d51a8d4 100644
> --- a/drivers/powercap/intel_rapl.c
> +++ b/drivers/powercap/intel_rapl.c
> @@ -329,14 +329,14 @@ static int release_zone(struct powercap_zone
> *power_zone) 
>  static int find_nr_power_limit(struct rapl_domain *rd)
>  {
> - int i;
> + int i, nr_pl = 0;
>  
>   for (i = 0; i < NR_POWER_LIMITS; i++) {
> - if (rd->rpl[i].name == NULL)
> - break;
> + if (rd->rpl[i].name)
> + nr_pl++;
>   }
>  
> - return i;
> + return nr_pl;
>  }
>  
>  static int set_domain_enable(struct powercap_zone *power_zone, bool
> mode) @@ -411,15 +411,38 @@ static const struct powercap_zone_ops
> zone_ops[] = { },
>  };
>  
> -static int set_power_limit(struct powercap_zone *power_zone, int id,
> +
> +/*
> + * Constraint index used by powercap can be different than power
> limit (PL)
> + * index in that some  PLs maybe missing due to non-existant MSRs.
> So we
> + * need to convert here by finding the valid PLs only (name
> populated).
> + */
> +static int contraint_to_pl(struct rapl_domain *rd, int cid)
> +{
> + int i, j;
> +
> + for (i = 0, j = 0; i < NR_POWER_LIMITS; i++) {
> + if ((rd->rpl[i].name) && j++ == cid) {
> + pr_debug("%s: index %d\n", __func__, i);
> + return i;
> + }
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int set_power_limit(struct powercap_zone *power_zone, int cid,
>   u64 power_limit)
>  {
>   struct rapl_domain *rd;
>   struct rapl_package *rp;
>   int ret = 0;
> + int id;
>  
>   get_online_cpus();
>   rd = power_zone_to_rapl_domain(power_zone);
> + id = contraint_to_pl(rd, cid);
> +
>   rp = rd->rp;
>  
>   if (rd->state & DOMAIN_STATE_BIOS_LOCKED) {
> @@ -446,16 +469,18 @@ set_exit:
>   return ret;
>  }
>  
> -static int get_current_power_limit(struct powercap_zone *power_zone,
> int id, +static int get_current_power_limit(struct powercap_zone
> *power_zone, int cid, u64 *data)
>  {
>   struct rapl_domain *rd;
>   u64 val;
>   int prim;
>   int ret = 0;
> + int id;
>  
>   get_online_cpus();
>   rd = power_zone_to_rapl_domain(power_zone);
> + id = contraint_to_pl(rd, cid);
>   switch (rd->rpl[id].prim_id) {
>   case PL1_ENABLE:
>   prim = POWER_LIMIT1;
> @@ -477,14 +502,17 @@ static int get_current_power_limit(struct
> powercap_zone *power_zone, int id, return ret;
>  }
>  
> -static int set_time_window(struct powercap_zone *power_zone, int id,
> +static int set_time_window(struct powercap_zone *power_zone, int cid,
>   u64
> window) {
>   struct rapl_domain *rd;
>   int ret = 0;
> + int id;
>  
>   get_online_cpus();
>   rd = power_zone_to_rapl_domain(power_zone);
> + id = contraint_to_pl(rd, cid);
> +
>   switch (rd->rpl[id].prim_id) {
>   case PL1_ENABLE:
>   rapl_write_data_raw(rd, TIME_WINDOW1, window);
> @@ -499,14 +527,17 @@ static int set_time_window(struct powercap_zone
> *power_zone, int id, return ret;
>  }
>  
> -static int get_time_window(struct powercap_zone *power_zone, int id,
> u64 *data) +static int get_time_window(struct powercap_zone
> *power_zone, int cid, u64 *data) {
>   struct rapl_domain *rd;
>   u64 val;
>   int ret = 0;
> + int id;
>  
>   get_online_cpus();
>   rd = power_zone_to_rapl_domain(power_zone);
> + id = contraint_to_pl(rd, cid);
> +
>   switch (rd->rpl[id].prim_id) {
>   case PL1_ENABLE:
>   ret = rapl_read_data_raw(rd, TIME_WINDOW1, true,
> ); @@ -525,15 +556,17 @@ static int get_time_window(struct
> powercap_zone *power_zone, int id, u64 *data) return ret;
>  }
>  
> -static const char *get_constraint_name(struct powercap_zone
> *power_zone, int id) +static const char *get_constraint_name(struct
> powercap_zone *power_zone, int cid) {
> - struct rapl_power_limit *rpl;
>   struct rapl_domain *rd;
> + int id;
>  
>   rd = power_zone_to_rapl_domain(power_zone);
> - rpl = (struct