> -----Original Message----- > From: Tauro, Riana <riana.tauro@xxxxxxxxx> > Sent: Wednesday, September 28, 2022 11:27 AM > To: Gupta, Anshuman <anshuman.gupta@xxxxxxxxx>; Belgaumkar, Vinay > <vinay.belgaumkar@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx > Cc: Dixit, Ashutosh <ashutosh.dixit@xxxxxxxxx>; Nilawar, Badal > <badal.nilawar@xxxxxxxxx> > 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.belgaumkar@xxxxxxxxx> > >> Sent: Monday, September 26, 2022 9:35 PM > >> To: Tauro, Riana <riana.tauro@xxxxxxxxx>; > >> intel-gfx@xxxxxxxxxxxxxxxxxxxxx > >> Cc: Gupta, Anshuman <anshuman.gupta@xxxxxxxxx>; Dixit, Ashutosh > >> <ashutosh.dixit@xxxxxxxxx> > >> 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@xxxxxxxxx> > >> > >> LGTM, > >> > >> Reviewed-by: Vinay Belgaumkar <vinay.belgaumkar@xxxxxxxxx> > >> > >>> --- > >>> 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;