> -----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.
> > +
> > + 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;
