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

["Tauro, Riana" <riana.tauro@intel.com>]

On 9/27/2022 4:42 PM, Gupta, Anshuman wrote:
> 
> 
>> -----Original Message-----
>> From: Belgaumkar, Vinay <vinay.belgaumkar@intel.com>
>> Sent: Monday, September 26, 2022 9:35 PM
>> To: Tauro, Riana <riana.tauro@intel.com>; intel-gfx@lists.freedesktop.org
>> Cc: Gupta, Anshuman <anshuman.gupta@intel.com>; Dixit, Ashutosh
>> <ashutosh.dixit@intel.com>
>> Subject: Re: [PATCH 3/3] drm/i915/guc/slpc: Add SLPC selftest live_slpc_power
>>
>>
>> On 9/23/2022 4:00 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
>>>
>>> v2: re-use code to measure power
>>>       fixed cosmetic review comments (Vinay)
>>>
>>> Signed-off-by: Riana Tauro <riana.tauro@intel.com>
>>
>> LGTM,
>>
>> Reviewed-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
>>
>>> ---
>>>    drivers/gpu/drm/i915/gt/selftest_slpc.c | 127 ++++++++++++++++++++++--
>>>    1 file changed, 118 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/selftest_slpc.c
>>> b/drivers/gpu/drm/i915/gt/selftest_slpc.c
>>> index 928f74718881..4c6e9257e593 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,39 @@ 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 err;
>>> +}
>>> +
>>> +static u64 measure_power_at_freq(struct intel_gt *gt, int *freq, u64
>>> +*power) {
>>> +	int err = 0;
>>> +
>>> +	err = slpc_set_freq(gt, *freq);
>>> +	if (err)
>>> +		return err;
>>> +	*freq = intel_rps_read_actual_frequency(&gt->rps);
>>> +	*power = measure_power(&gt->rps, freq);
>>> +
>>> +	return err;
>>> +}
>>> +
>>>    static int vary_max_freq(struct intel_guc_slpc *slpc, struct intel_rps *rps,
>>>    			 u32 *max_act_freq)
>>>    {
>>> @@ -113,6 +147,58 @@ 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 supports
>>> +	 * that theory.
>>> +	 */
>>> +	if (!librapl_supported(gt->i915))
>>> +		return 0;
> 	This seems a wrong abstraction, this should a generic call should check both hwmon registration for dgfx and rapl for igfx.
> 	Br,
> 	Anshuman Gupta.
The current librapl_supported has only rapl related changes. The hwmon 
energy is yet to be added.

Will change the name with the hwmon patch

Thanks
Riana Tauro

>>> +
>>> +	min.freq = slpc->min_freq;
>>> +	err = measure_power_at_freq(gt, &min.freq, &min.power);
>>> +
>>> +	if (err)
>>> +		return err;
>>> +
>>> +	max.freq = slpc->rp0_freq;
>>> +	err = measure_power_at_freq(gt, &max.freq, &max.power);
>>> +
>>> +	if (err)
>>> +		return err;
>>> +
>>> +	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 +319,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);
>>> @@ -316,12 +408,29 @@ static int live_slpc_max_granted(void *arg)
>>>    	return ret;
>>>    }
>>>
>>> +static int live_slpc_power(void *arg) {
>>> +	struct drm_i915_private *i915 = arg;
>>> +	struct intel_gt *gt;
>>> +	unsigned int i;
>>> +	int ret;
>>> +
>>> +	for_each_gt(gt, i915, i) {
>>> +		ret = run_test(gt, SLPC_POWER);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +
>>> +	return ret;
>>> +}
>>> +
>>>    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),
>>>    	};
>>>
>>>    	struct intel_gt *gt;