Re: [PATCH 2/2] perf/x86/rapl: Enable Baytrail/Braswell RAPL support

[Peter Zijlstra]

On Thu, Sep 08, 2016 at 05:08:58PM +0800, Harry Pan wrote:

> @@ -177,6 +187,16 @@ static inline u64 rapl_scale(u64 v, int cfg)
>  		pr_warn("Invalid domain %d, failed to scale data\n", cfg);
>  		return v;
>  	}
> +
> +	/*
> +	 * Some Atom series processors (BYT/BSW) use 2^ESU microjoules.
> +	 *
> +	 * TODO: this looks hacky, it's better to refactor scale-up mechanism
> +	 * to compromise the main stream processors and Atom ones.
> +	 */
> +	if (is_baytrail)
> +		return v << rapl_hw_unit[cfg - 1];
> +

Can't you simply set rapl_hw_unit[] such that 32 - rapl_hw_unit[] ends
up at the right number? Then you only get to much with values in
rapl_check_hw_unit without runtime overhead later.

> +static int rapl_check_hw_unit(enum rapl_quirk apply_quirk)
>  {
>  	u64 msr_rapl_power_unit_bits;
>  	int i;
> @@ -634,10 +674,20 @@ static int rapl_check_hw_unit(bool apply_quirk)
>  	 * "Intel Xeon Processor E5-1600 and E5-2600 v3 Product Families, V2
>  	 * of 2. Datasheet, September 2014, Reference Number: 330784-001 "
>  	 */
> -	if (apply_quirk)
> +	if (apply_quirk == RAPL_HSX_QUIRK)
>  		rapl_hw_unit[RAPL_IDX_RAM_NRG_STAT] = 16;
>  
>  	/*
> +	 * Some Atom processors (BYT/BSW) have 2^ESU microjoules increment,
> +	 * refer to Software Developers' Manual, Vol. 3C, Order No. 325384,
> +	 * Table 35-8 of MSR_RAPL_POWER_UNIT
> +	 */
> +	if (apply_quirk == RAPL_BYT_QUIRK)
> +		is_baytrail = true;
> +	else
> +		is_baytrail = false;

it was already false...

	/*
	 * comment explaining quirk goes here...
	 */
	if (apply_quirk = RAPL_BYT_QUIRK) {
		for (i = 0; i < NR_RAPL_DOMAINS; i++)
			rapl_hw_unit[i] = 32 - rapl_hw_unit[i];
	}

and then you get to verify what to do with rapl_timer_ms.



>  static const struct intel_rapl_init_fun snb_rapl_init __initconst = {
> -	.apply_quirk = false,
> +	.apply_quirk = 0,

Either leave it out (unmentioned members get initialized to 0) or add
RAPL_NO_QUIRK or so.

>  	.cntr_mask = RAPL_IDX_CLN,
>  	.attrs = rapl_events_cln_attr,
>  };
>