Re: [PATCH] drm/i915/pmu: Measure sampler intervals

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 25/05/2018 18:45, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-05-25 18:31:35)
>>
>> On 25/05/2018 18:11, Chris Wilson wrote:
>>> hrtimer is not reliable enough to assume fixed intervals, and so even
>>> coarse accuracy (in the face of kasan and similar heavy debugging) we
>>> need to measure the actual interval between sample.
>>
>> It doesn't even average out to something acceptable under such Kconfigs?
>> Horror.. precise but inaccurate. /O\
>>
>>> While using a single timestamp to compute the interval does not allow
>>> very fine accuracy (consider the impact of a slow forcewake between
>>> different samples after the timestamp is read) is much better than
>>> assuming the interval.
>>>
>>> Testcase: igt/perf_pmu #ivb
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/i915_pmu.c | 20 +++++++++++++-------
>>>    drivers/gpu/drm/i915/i915_pmu.h |  4 ++++
>>>    2 files changed, 17 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
>>> index dc87797db500..f5087515eb43 100644
>>> --- a/drivers/gpu/drm/i915/i915_pmu.c
>>> +++ b/drivers/gpu/drm/i915/i915_pmu.c
>>> @@ -127,6 +127,7 @@ static void __i915_pmu_maybe_start_timer(struct drm_i915_private *i915)
>>>    {
>>>        if (!i915->pmu.timer_enabled && pmu_needs_timer(i915, true)) {
>>>                i915->pmu.timer_enabled = true;
>>> +             i915->pmu.timestamp = ktime_get();
>>>                hrtimer_start_range_ns(&i915->pmu.timer,
>>>                                       ns_to_ktime(PERIOD), 0,
>>>                                       HRTIMER_MODE_REL_PINNED);
>>> @@ -160,7 +161,7 @@ update_sample(struct i915_pmu_sample *sample, u32 unit, u32 val)
>>>        sample->cur += mul_u32_u32(val, unit);
>>>    }
>>>    
>>> -static void engines_sample(struct drm_i915_private *dev_priv)
>>> +static void engines_sample(struct drm_i915_private *dev_priv, u64 period)
>>>    {
>>>        struct intel_engine_cs *engine;
>>>        enum intel_engine_id id;
>>> @@ -183,7 +184,7 @@ static void engines_sample(struct drm_i915_private *dev_priv)
>>>                val = !i915_seqno_passed(current_seqno, last_seqno);
>>>    
>>>                update_sample(&engine->pmu.sample[I915_SAMPLE_BUSY],
>>> -                           PERIOD, val);
>>> +                           period, val);
>>>    
>>>                if (val && (engine->pmu.enable &
>>>                    (BIT(I915_SAMPLE_WAIT) | BIT(I915_SAMPLE_SEMA)))) {
>>> @@ -195,10 +196,10 @@ static void engines_sample(struct drm_i915_private *dev_priv)
>>>                }
>>>    
>>>                update_sample(&engine->pmu.sample[I915_SAMPLE_WAIT],
>>> -                           PERIOD, !!(val & RING_WAIT));
>>> +                           period, !!(val & RING_WAIT));
>>>    
>>>                update_sample(&engine->pmu.sample[I915_SAMPLE_SEMA],
>>> -                           PERIOD, !!(val & RING_WAIT_SEMAPHORE));
>>> +                           period, !!(val & RING_WAIT_SEMAPHORE));
>>>        }
>>>    
>>>        if (fw)
>>> @@ -207,7 +208,7 @@ static void engines_sample(struct drm_i915_private *dev_priv)
>>>        intel_runtime_pm_put(dev_priv);
>>>    }
>>>    
>>> -static void frequency_sample(struct drm_i915_private *dev_priv)
>>> +static void frequency_sample(struct drm_i915_private *dev_priv, u64 period)
>>
>> Period is unused in this function.
>>
>> But more importantly that leads to a problem. When reading the counter
>> the frequencies accumulator is divided by FREQUENCY define, which is
>> inverse of PERIOD. If the error is big enough to mess up the engines
>> sampling, is it big enough to affect the frequencies as well?
> 
> Yes, but fixing up frequencies I left for another patch, because it's
> going to be more involved (having to choose the divider more carefully)
> and would you believe it, but CI only complains about busy sampling ;)
> 
> I passed in the period as a reminder.

It might only remind someone to remove the unused variable so I am not 
sure it is worth it.

>> Improving that would need average frequency between two counter reads.
>> Which looks tricky to shoehorn into the pmu api. Maybe primitive running
>> average would do.
> 
> My plan was to expose cycles (Frequency x time) to the user, and then
> they calculate frequency by the comparing their own samples. (Since we
> give them (time, samples) for each pmu read).

Frequency times which time? On read? Don't exactly follow. :(

> 
>>>    {
>>>        if (dev_priv->pmu.enable &
>>>            config_enabled_mask(I915_PMU_ACTUAL_FREQUENCY)) {
>>> @@ -237,12 +238,17 @@ static enum hrtimer_restart i915_sample(struct hrtimer *hrtimer)
>>>    {
>>>        struct drm_i915_private *i915 =
>>>                container_of(hrtimer, struct drm_i915_private, pmu.timer);
>>> +     ktime_t now, period;
>>>    
>>>        if (!READ_ONCE(i915->pmu.timer_enabled))
>>>                return HRTIMER_NORESTART;
>>>    
>>> -     engines_sample(i915);
>>> -     frequency_sample(i915);
>>> +     now = ktime_get();
>>> +     period = ktime_sub(now, i915->pmu.timestamp);
>>> +     i915->pmu.timestamp = now;
>>> +
>>> +     engines_sample(i915, period);
>>> +     frequency_sample(i915, period);
>>>    
>>>        hrtimer_forward_now(hrtimer, ns_to_ktime(PERIOD));
>>>        return HRTIMER_RESTART;
>>> diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
>>> index 2ba735299f7c..0f1e4642077e 100644
>>> --- a/drivers/gpu/drm/i915/i915_pmu.h
>>> +++ b/drivers/gpu/drm/i915/i915_pmu.h
>>> @@ -52,6 +52,10 @@ struct i915_pmu {
>>>         * @timer: Timer for internal i915 PMU sampling.
>>>         */
>>>        struct hrtimer timer;
>>> +     /**
>>> +      * @timestamp: Timestamp of last internal i915 PMU sampling.
>>> +      */
>>> +     ktime_t timestamp;
>>>        /**
>>>         * @enable: Bitmask of all currently enabled events.
>>>         *
>>>
>>
>> Patch looks okay, just loses the some of the optimisation potential so I
>> am guessing we won't be thinking about replacing multiplies and divides
>> with shift any more. :)
>>
>> But the question of frequency counters is now bothering me.
>>
>> And if this problem is limited to Kasan then how much we want to
>> complicate things to make that work?
> 
> Not just kasan, but ivb really. That's the only to have never worked
> whatever the config. kasan affects more CI machines.

I am also thinking that with this approach we could start allowing timer 
slack, since we are measuring it anyway. It would release back some of 
the added cost of time queries. Well, not that they are significant. 
Digression anyway.

I want to understand why this is a problem on IVB and what is the 
solution for frequency counters.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx