[Intel-gfx] [PATCH] drm/i915/pmu: Fix sleep under atomic in RC6 readout

[Tvrtko Ursulin]

On 06/02/2018 16:10, Imre Deak wrote:
> On Tue, Feb 06, 2018 at 04:04:10PM +0000, Chris Wilson wrote:
>> Quoting Tvrtko Ursulin (2018-02-06 14:31:07)
>>> +static u64 read_rc6_residency(struct drm_i915_private *i915)
>>> +{
>>> +       u64 val;
>>> +
>>> +       val = intel_rc6_residency_ns(i915, IS_VALLEYVIEW(i915) ?
>>> +                                          VLV_GT_RENDER_RC6 : GEN6_GT_GFX_RC6);
>>> +       if (HAS_RC6p(i915))
>>> +               val += intel_rc6_residency_ns(i915, GEN6_GT_GFX_RC6p);
>>> +       if (HAS_RC6pp(i915))
>>> +               val += intel_rc6_residency_ns(i915, GEN6_GT_GFX_RC6pp);
>>
>> We really should mention that these may produce interesting results
>> every 53 minutes. Switching to a timer will allow us to notice the
>> wraparound in each counter.
>>
>>> +
>>> +       return val;
>>> +}
>>> +
>>> +static void
>>> +update_rc6_sample(struct drm_i915_private *i915, u64 val, bool locked)
>>> +{
>>> +       unsigned long flags;
>>> +
>>> +       if (!locked)
>>> +               spin_lock_irqsave(&i915->pmu.lock, flags);
>>> +
>>> +       /*
>>> +        * Update stored RC6 counter only if it is greater than the current
>>> +        * value. This deals with periods of runtime suspend during which we are
>>> +        * estimating the RC6 residency, so do not want to overshoot the real
>>> +        * value read once the device is woken up.
>>> +        */
>>> +       if (val > i915->pmu.sample[__I915_SAMPLE_RC6].cur)
>>> +               i915->pmu.sample[__I915_SAMPLE_RC6].cur = val;
>>
>> 64b wraparound? Maybe not today, maybe not tomorrow... ;)

But in 585 years we're in trouble? :))

>>
>>> +
>>> +       /* We don't need to sample RC6 from the timer any more. */
>>> +       i915->pmu.timer_enabled =
>>> +               __pmu_needs_timer(i915,
>>> +                                 i915->pmu.enable & ~config_enabled_mask(I915_PMU_RC6_RESIDENCY),
>>> +                                 READ_ONCE(i915->gt.awake));
>>
>> But we do... :)
>> One thing I had in mind was to hook into runtime suspend/resume to read
>> the counters there and compensate, but the more I think about it, we may
>> as well solve the lack of resolution in the rc6 counters whilst we are
>> here. https://bugs.freedesktop.org/show_bug.cgi?id=94852.
> 
> How about using dev->power.suspended_jiffies?

This sounds promising and I hope it ends up with a much nicer fix so 
I'll give it a go.

RC6 wraparound issue I hope we can leave for a different patch. I hope 
it will be possible without a timer, as long as someone is not taking 
samples every 54 minutes only. (Don't mention VLV.) :)

Regards,

Tvrtko