> -----Original Message-----
> From: Tauro, Riana <riana.ta...@intel.com>
> Sent: Wednesday, September 28, 2022 11:27 AM
> To: Gupta, Anshuman <anshuman.gu...@intel.com>; Belgaumkar, Vinay
> <vinay.belgaum...@intel.com>; intel-gfx@lists.freedesktop.org
> Cc: Dixit, Ashutosh <ashutosh.di...@intel.com>; Nilawar, Badal
> <badal.nila...@intel.com>
> Subject: Re: [PATCH 3/3] drm/i915/guc/slpc: Add SLPC selftest
> live_slpc_power
>
>
>
> On 9/27/2022 4:42 PM, Gupta, Anshuman wrote:
> >
> >
> >> -----Original Message-----
> >> From: Belgaumkar, Vinay <vinay.belgaum...@intel.com>
> >> Sent: Monday, September 26, 2022 9:35 PM
> >> To: Tauro, Riana <riana.ta...@intel.com>;
> >> intel-gfx@lists.freedesktop.org
> >> Cc: Gupta, Anshuman <anshuman.gu...@intel.com>; Dixit, Ashutosh
> >> <ashutosh.di...@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.ta...@intel.com>
> >>
> >> LGTM,
> >>
> >> Reviewed-by: Vinay Belgaumkar <vinay.belgaum...@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 = >->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(>->rps);
> >>> + *power = measure_power(>->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 = >->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
HWMON series is already reviewed and ready to merge just waiting for CI results.
I think we can merge this after hwmom.
Br,
Anshuman Gupta.
>
> 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;
