On 11/22/2022 8:10 AM, Dixit, Ashutosh wrote:
On Sun, 20 Nov 2022 23:29:46 -0800, Riana Tauro wrote:diff --git a/drivers/gpu/drm/i915/gt/selftest_rc6.c b/drivers/gpu/drm/i915/gt/selftest_rc6.c index 15b84c428f66..845058ed83ed 100644 --- a/drivers/gpu/drm/i915/gt/selftest_rc6.c +++ b/drivers/gpu/drm/i915/gt/selftest_rc6.c @@ -61,9 +61,9 @@ int live_rc6_manual(void *arg) res[0] = rc6_residency(rc6); dt = ktime_get(); - rc0_power = libpower_get_energy_uJ(); + rc0_power = libpower_get_energy_uJ(gt->i915); msleep(250); - rc0_power = libpower_get_energy_uJ() - rc0_power; + rc0_power = libpower_get_energy_uJ(gt->i915) - rc0_power; dt = ktime_sub(ktime_get(), dt); res[1] = rc6_residency(rc6); if ((res[1] - res[0]) >> 10) { @@ -89,9 +89,9 @@ int live_rc6_manual(void *arg) res[0] = rc6_residency(rc6); intel_uncore_forcewake_flush(rc6_to_uncore(rc6), FORCEWAKE_ALL); dt = ktime_get(); - rc6_power = libpower_get_energy_uJ(); + rc6_power = libpower_get_energy_uJ(gt->i915); msleep(100); - rc6_power = libpower_get_energy_uJ() - rc6_power; + rc6_power = libpower_get_energy_uJ(gt->i915) - rc6_power;So: * arg for live_rps_power and live_rc6_manual is gt * freq's are per gt * rc6 residency is per gt But the power/energy we are using is per device (gt->i915)? And we expect device level power to be low when only one gt might be in rc6? Shouldn't all these functions be per gt? Specially when we might have multiple gt's soon. Or if per gt functions don't make in all cases have both device and gt level functions? i915 hwmon provides both gt and device/package/card level energy but iGFX till now reads a MSR which does not need gt or i915. So per gt I think should work for now.
Will modify to return per-gt values if present else the default device level energy.
dt = ktime_sub(ktime_get(), dt); res[1] = rc6_residency(rc6); if (res[1] == res[0]) { diff --git a/drivers/gpu/drm/i915/gt/selftest_rps.c b/drivers/gpu/drm/i915/gt/selftest_rps.c index b8b0b0c7617e..6732aa7d4792 100644 --- a/drivers/gpu/drm/i915/gt/selftest_rps.c +++ b/drivers/gpu/drm/i915/gt/selftest_rps.c @@ -1090,38 +1090,38 @@ int live_rps_interrupt(void *arg) return err; } -static u64 __measure_power(int duration_ms) +static u64 __measure_power(struct intel_gt *gt, int duration_ms) { u64 dE, dt; dt = ktime_get(); - dE = libpower_get_energy_uJ(); + dE = libpower_get_energy_uJ(gt->i915); usleep_range(1000 * duration_ms, 2000 * duration_ms); - dE = libpower_get_energy_uJ() - dE; + dE = libpower_get_energy_uJ(gt->i915) - dE; dt = ktime_get() - dt; return div64_u64(1000 * 1000 * dE, dt); } -static u64 measure_power(struct intel_rps *rps, int *freq) +static u64 measure_power(struct intel_gt *gt, int *freq) { u64 x[5]; int i; for (i = 0; i < 5; i++) - x[i] = __measure_power(5); + x[i] = __measure_power(gt, 5); - *freq = (*freq + intel_rps_read_actual_frequency(rps)) / 2; + *freq = (*freq + intel_rps_read_actual_frequency(>->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); } -static u64 measure_power_at(struct intel_rps *rps, int *freq) +static u64 measure_power_at(struct intel_gt *gt, int *freq) { - *freq = rps_set_check(rps, *freq); - return measure_power(rps, freq); + *freq = rps_set_check(>->rps, *freq);Hmm looks like this whole live_rps_power stuff is only for host turbo, not slpc. Anyway that's a future patch.
live_slpc_power is present in selftest_slpc.c.
+ return measure_power(gt, freq); } int live_rps_power(void *arg) @@ -1187,10 +1187,10 @@ int live_rps_power(void *arg) } max.freq = rps->max_freq; - max.power = measure_power_at(rps, &max.freq); + max.power = measure_power_at(gt, &max.freq); min.freq = rps->min_freq; - min.power = measure_power_at(rps, &min.freq); + min.power = measure_power_at(gt, &min.freq); igt_spinner_end(&spin); st_engine_heartbeat_enable(engine); diff --git a/drivers/gpu/drm/i915/gt/selftest_slpc.c b/drivers/gpu/drm/i915/gt/selftest_slpc.c index fc1cdda82ec6..c4b14f2b268c 100644 --- a/drivers/gpu/drm/i915/gt/selftest_slpc.c +++ b/drivers/gpu/drm/i915/gt/selftest_slpc.c @@ -78,7 +78,7 @@ static u64 measure_power_at_freq(struct intel_gt *gt, int *freq, u64 *power) if (err) return err; *freq = intel_rps_read_actual_frequency(>->rps); - *power = measure_power(>->rps, freq); + *power = measure_power(gt, freq); return err; }
Changes for slpc present here.
diff --git a/drivers/gpu/drm/i915/selftests/libpower.c b/drivers/gpu/drm/i915/selftests/libpower.c index c66e993c5f85..df37cba30353 100644 --- a/drivers/gpu/drm/i915/selftests/libpower.c +++ b/drivers/gpu/drm/i915/selftests/libpower.c @@ -6,29 +6,30 @@ #include <asm/msr.h> #include "i915_drv.h" +#include "i915_hwmon.h" #include "libpower.h" -bool libpower_supported(const struct drm_i915_private *i915) -{ - /* Discrete cards require hwmon integration */ - if (IS_DGFX(i915)) - return false; - - return libpower_get_energy_uJ(); -} - -u64 libpower_get_energy_uJ(void) +u64 libpower_get_energy_uJ(struct drm_i915_private *i915) { unsigned long long power; u32 units; + long energy_uJ = 0; - if (rdmsrl_safe(MSR_RAPL_POWER_UNIT, &power)) - return 0; + if (IS_DGFX(i915)) { +#if IS_REACHABLE(CONFIG_HWMON) + if (i915_hwmon_get_energy(i915, &energy_uJ)) +#endifNo IS_REACHABLE stuff here. Declare i915_hwmon_get_energy similar to other functions in i915_hwmon.h. Thanks. -- Ashutosh
Thanks for the review comments. Will address in next rev. Thanks Riana
+ return 0; + } else { + if (rdmsrl_safe(MSR_RAPL_POWER_UNIT, &power)) + return 0; - units = (power & 0x1f00) >> 8; + units = (power & 0x1f00) >> 8; - if (rdmsrl_safe(MSR_PP1_ENERGY_STATUS, &power)) - return 0; + if (rdmsrl_safe(MSR_PP1_ENERGY_STATUS, &power)) + return 0; - return (1000000 * power) >> units; /* convert to uJ */ + energy_uJ = (1000000 * power) >> units; /* convert to uJ */ + } + return energy_uJ; } diff --git a/drivers/gpu/drm/i915/selftests/libpower.h b/drivers/gpu/drm/i915/selftests/libpower.h index 5352981eb946..03a44611f9e9 100644 --- a/drivers/gpu/drm/i915/selftests/libpower.h +++ b/drivers/gpu/drm/i915/selftests/libpower.h @@ -10,8 +10,10 @@ struct drm_i915_private; -bool libpower_supported(const struct drm_i915_private *i915); - -u64 libpower_get_energy_uJ(void); +u64 libpower_get_energy_uJ(struct drm_i915_private *i915); +static inline bool libpower_supported(struct drm_i915_private *i915) +{ + return libpower_get_energy_uJ(i915); +} #endif /* SELFTEST_LIBPOWER_H */ -- 2.25.1
