On 23/11/17 16:07, Chris Wilson wrote:
> Quoting Chris Wilson (2017-11-23 16:02:20)
>> Quoting Tvrtko Ursulin (2017-11-23 13:37:30)
>>>
>>> On 23/11/2017 08:22, Chris Wilson wrote:
>>>> Make sure the HW is idle before we start sampling the GPU for busyness.
>>>> If we do not rest for long enough between tests, we may carry the
>>>> sampling over.
>>>
>>> I'd rather not have this since as I said yesterday each opened PMU event
>>> is supposed to record the initial counter value as reference. If that is
>>> failing or not good enough on some tests/platforms I would rather first
>>> understand why and how.
>>
>> All legacy sampling fails... :| I'm putting it back!
>
> So presumably it is coupling with the parking.
Or more precisely averaging with the previous sample. Every time one PMU
client signals the timer to self-disarm, but the new PMU client enables
the timer again in this window, the previous sample value does not get
cleared.
I tried this for instance:
diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index 650089b30d46..a5ca27eeef40 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -147,9 +147,26 @@ void i915_pmu_gt_parked(struct drm_i915_private *i915)
spin_unlock_irq(&i915->pmu.lock);
}
+static void pmu_init_previous_samples(struct drm_i915_private *i915)
+{
+ struct intel_engine_cs *engine;
+ enum intel_engine_id id;
+ unsigned int i;
+
+ for_each_engine(engine, i915, id) {
+ for (i = 0; i < ARRAY_SIZE(engine->pmu.sample); i++)
+ engine->pmu.sample[i].prev = 0;
+ }
+
+ for (i = 0; i < ARRAY_SIZE(i915->pmu.sample); i++)
+ i915->pmu.sample[i].prev = i915->gt_pm.rps.idle_freq;
+}
+
static void __i915_pmu_maybe_start_timer(struct drm_i915_private *i915)
{
if (!i915->pmu.timer_enabled && pmu_needs_timer(i915, true)) {
+ hrtimer_cancel(&i915->pmu.timer);
+ pmu_init_previous_samples(i915);
i915->pmu.timer_enabled = true;
hrtimer_start_range_ns(&i915->pmu.timer,
ns_to_ktime(PERIOD), 0,
@@ -262,31 +279,13 @@ static void frequency_sample(struct drm_i915_private *dev_priv)
}
}
-static void pmu_init_previous_samples(struct drm_i915_private *i915)
-{
- struct intel_engine_cs *engine;
- enum intel_engine_id id;
- unsigned int i;
-
- for_each_engine(engine, i915, id) {
- for (i = 0; i < ARRAY_SIZE(engine->pmu.sample); i++)
- engine->pmu.sample[i].prev = 0;
- }
-
- for (i = 0; i < ARRAY_SIZE(i915->pmu.sample); i++)
- i915->pmu.sample[i].prev = i915->gt_pm.rps.idle_freq;
-}
-
static enum hrtimer_restart i915_sample(struct hrtimer *hrtimer)
{
struct drm_i915_private *i915 =
container_of(hrtimer, struct drm_i915_private, pmu.timer);
- if (!READ_ONCE(i915->pmu.timer_enabled)) {
- pmu_init_previous_samples(i915);
-
+ if (!READ_ONCE(i915->pmu.timer_enabled))
return HRTIMER_NORESTART;
- }
engines_sample(i915);
frequency_sample(i915);
@@ -847,8 +846,6 @@ void i915_pmu_register(struct drm_i915_private *i915)
hrtimer_init(&i915->pmu.timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
i915->pmu.timer.function = i915_sample;
- pmu_init_previous_samples(i915);
-
for_each_engine(engine, i915, id)
INIT_DELAYED_WORK(&engine->pmu.disable_busy_stats,
__disable_busy_stats);
---
But it is too evil to wait for the sampling timer in case of MMIO in the
irq disabled section. Even though it would happen only on disable-enable
transitions quicker than sampling period. It is not an use case, but still..
Or we could drop sample averaging.. ?
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
