[Intel-gfx] [PATCH v3 3/3] drm/i915/selftests: Add hwmon support in libpower for dgfx
Tauro, Riana
riana.tauro at intel.com
Mon Nov 28 11:25:46 UTC 2022
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))
>> +#endif
>
> No 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
>>
More information about the Intel-gfx
mailing list