[Intel-gfx] [PATCH 1/1] drm/i915/guc/slpc: Add SLPC selftest live_slpc_power

[Belgaumkar, Vinay]

On 9/22/2022 7:32 AM, Riana Tauro wrote:
> A fundamental assumption is that at lower frequencies,
> not only do we run slower, but we save power compared to
> higher frequencies.
> live_slpc_power checks if running at low frequency saves power
>
> Signed-off-by: Riana Tauro <riana.tauro at intel.com>
> ---
>   drivers/gpu/drm/i915/gt/selftest_slpc.c | 116 ++++++++++++++++++++++--
>   1 file changed, 107 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/selftest_slpc.c b/drivers/gpu/drm/i915/gt/selftest_slpc.c
> index f8a1d27df272..f22f091d2844 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_slpc.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_slpc.c
> @@ -11,7 +11,8 @@
>   enum test_type {
>   	VARY_MIN,
>   	VARY_MAX,
> -	MAX_GRANTED
> +	MAX_GRANTED,
> +	SLPC_POWER,
>   };
>   
>   static int slpc_set_min_freq(struct intel_guc_slpc *slpc, u32 freq)
> @@ -41,6 +42,42 @@ static int slpc_set_max_freq(struct intel_guc_slpc *slpc, u32 freq)
>   	return ret;
>   }
>   
> +static int slpc_set_freq(struct intel_gt *gt, u32 freq)
> +{
> +	int err;
> +	struct intel_guc_slpc *slpc = &gt->uc.guc.slpc;
> +
> +	err = slpc_set_max_freq(slpc, freq);
> +	if (err) {
> +		pr_err("Unable to update max freq");
> +		return err;
> +	}
> +
> +	err = slpc_set_min_freq(slpc, freq);
> +	if (err) {
> +		pr_err("Unable to update min freq");
> +		return err;
> +	}
> +
> +	return intel_rps_read_actual_frequency(&gt->rps);
The return value here is overloaded (either -ERR or frequency). Can we 
just return the error status here and query the act_freq in the caller 
instead?
> +}
> +
> +static u64 measure_slpc_power_at(struct intel_gt *gt, int *freq)
Name is a little misleading, maybe slpc_measure_power_at() ?
> +{
> +	u64 x[5];
> +	int i;
> +
> +	*freq = slpc_set_freq(gt, *freq);
Here, we can check for return code and then query for act_freq.
> +	for (i = 0; i < 5; i++)
> +		x[i] = __measure_power(5);
> +	*freq = (*freq + intel_rps_read_actual_frequency(&gt->rps)) / 2;
> +
> +	/* A simple triangle filter for better result stability */
> +	sort(x, 5, sizeof(*x), cmp_u64, NULL);
> +
> +	return div_u64(x[1] + 2 * x[2] + x[3], 4);
we are duplicating code from selftest_rps here, is it possible to add a 
helper instead (like __measure_power())?
> +}
> +
>   static int vary_max_freq(struct intel_guc_slpc *slpc, struct intel_rps *rps,
>   			 u32 *max_act_freq)
>   {
> @@ -113,6 +150,52 @@ static int vary_min_freq(struct intel_guc_slpc *slpc, struct intel_rps *rps,
>   	return err;
>   }
>   
> +static int slpc_power(struct intel_gt *gt, struct intel_engine_cs *engine)
> +{
> +	struct intel_guc_slpc *slpc = &gt->uc.guc.slpc;
> +	struct {
> +		u64 power;
> +		int freq;
> +	} min, max;
> +	int err = 0;
> +
> +	/*
> +	 * Our fundamental assumption is that running at lower frequency
> +	 * actually saves power. Let's see if our RAPL measurement support
supports*
> +	 * that theory.
> +	 */
> +	if (!librapl_supported(gt->i915))
> +		return 0;
> +
> +	min.freq = slpc->min_freq;
> +	min.power =  measure_slpc_power_at(gt, &min.freq);
> +
> +	max.freq = slpc->rp0_freq;
> +	max.power = measure_slpc_power_at(gt, &max.freq);
> +
> +	pr_info("%s: min:%llumW @ %uMHz, max:%llumW @ %uMHz\n",
> +		engine->name,
> +		min.power, min.freq,
> +		max.power, max.freq);
> +
> +	if (10 * min.freq >= 9 * max.freq) {
> +		pr_notice("Could not control frequency, ran at [%uMHz, %uMhz]\n",
> +			  min.freq, max.freq);
> +	}
> +
> +	if (11 * min.power > 10 * max.power) {
> +		pr_err("%s: did not conserve power when setting lower frequency!\n",
> +		       engine->name);
> +		err = -EINVAL;
> +	}
> +
> +	/* Restore min/max frequencies */
> +	slpc_set_max_freq(slpc, slpc->rp0_freq);
> +	slpc_set_min_freq(slpc, slpc->min_freq);
> +
> +	return err;
> +}
> +
>   static int max_granted_freq(struct intel_guc_slpc *slpc, struct intel_rps *rps, u32 *max_act_freq)
>   {
>   	struct intel_gt *gt = rps_to_gt(rps);
> @@ -233,17 +316,23 @@ static int run_test(struct intel_gt *gt, int test_type)
>   
>   			err = max_granted_freq(slpc, rps, &max_act_freq);
>   			break;
> +
> +		case SLPC_POWER:
> +			err = slpc_power(gt, engine);
> +			break;
>   		}
>   
> -		pr_info("Max actual frequency for %s was %d\n",
> -			engine->name, max_act_freq);
> +		if (test_type != SLPC_POWER) {
> +			pr_info("Max actual frequency for %s was %d\n",
> +				engine->name, max_act_freq);
>   
> -		/* Actual frequency should rise above min */
> -		if (max_act_freq <= slpc_min_freq) {
> -			pr_err("Actual freq did not rise above min\n");
> -			pr_err("Perf Limit Reasons: 0x%x\n",
> -			       intel_uncore_read(gt->uncore, GT0_PERF_LIMIT_REASONS));
> -			err = -EINVAL;
> +			/* Actual frequency should rise above min */
> +			if (max_act_freq <= slpc_min_freq) {
> +				pr_err("Actual freq did not rise above min\n");
> +				pr_err("Perf Limit Reasons: 0x%x\n",
> +				       intel_uncore_read(gt->uncore, GT0_PERF_LIMIT_REASONS));
> +				err = -EINVAL;
> +			}
>   		}
>   
>   		igt_spinner_end(&spin);
> @@ -292,12 +381,21 @@ static int live_slpc_max_granted(void *arg)
>   	return run_test(gt, MAX_GRANTED);
>   }
>   
> +static int live_slpc_power(void *arg)
> +{
> +	struct drm_i915_private *i915 = arg;
> +	struct intel_gt *gt = to_gt(i915);
> +
> +	return run_test(gt, SLPC_POWER);
> +}
> +
>   int intel_slpc_live_selftests(struct drm_i915_private *i915)
>   {
>   	static const struct i915_subtest tests[] = {
>   		SUBTEST(live_slpc_vary_max),
>   		SUBTEST(live_slpc_vary_min),
>   		SUBTEST(live_slpc_max_granted),
> +		SUBTEST(live_slpc_power),

Thanks,

Vinay.

>   	};
>   
>   	if (intel_gt_is_wedged(to_gt(i915)))