[Intel-gfx] [PATCH] drm/i915: Only arm the forcewake release timer on the final put

[Chris Wilson]

On Thu, Mar 24, 2016 at 02:33:32PM +0000, Dave Gordon wrote:
> On 24/03/16 13:42, Chris Wilson wrote:
> >On Thu, Mar 24, 2016 at 01:32:53PM +0000, Chris Wilson wrote:
> >>If we arm the release timer on acquiring the forcewake, we will release
> >>the forcewake on the jiffie afterwards. If we only arm the release timer
> >>on the final put, we will release the forcewake slightly later instead.
> >>
> >>Much, much worse, we did not acquire a refcount for the armed timing
> >>during the get(), and so unbalanced our forcewake counting.
> >>
> >>Reported-by: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> >>Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> >>Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> >>Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> >>---
> >>  drivers/gpu/drm/i915/intel_uncore.c | 3 +--
> >>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> >>index 96799392c2c7..d857168c6c9b 100644
> >>--- a/drivers/gpu/drm/i915/intel_uncore.c
> >>+++ b/drivers/gpu/drm/i915/intel_uncore.c
> >>@@ -60,6 +60,7 @@ fw_domain_reset(const struct intel_uncore_forcewake_domain *d)
> >>  static inline void
> >>  fw_domain_arm_timer(struct intel_uncore_forcewake_domain *d)
> >>  {
> >>+	d->wake_count++;
> >>  	mod_timer_pinned(&d->timer, jiffies + 1);
> >
> >Which raise the obvious issue that we double increment the counter if
> >the timer was pending (where we would only then release it once).
> >-Chris
> 
> 'jiffies + 1' might be only a nanosecond away; would it be better to
> use 'jiffies + 2'? OTOH that might be quite a long time and
> therefore increase power consumption :( So is there a somewhat
> higher-resolution cyclic timer that we could use?

Right, we could switch to using hrtimers. Or msecs_to_jiffies(). jiffies
+ 1 was a number plucked out of the air. I believe Mika did some
experimentation but never arrived at a conclusion for a better value.

An adaptive algo might look at the running average time between
forcewakes and set the timer for some relative value. That might be an
interesting approach.
 
> Also, why mod_timer_pinned() ? I'd think that if this actually
> happens a whole jiffie later, there'd be little correlation between
> the current CPU activity and what's happening when the timer fires,
> so no real point in pinning the timer to current CPU.

The idea behind using timer_pinned was to try and keep the activity on
the same cpu, in the vain hope that the timer actually expires
relatively quickly before the CPU went to sleep again.
 
> Using mod_timer() instead would allow it to apply slack and align
> the callback to other timer activity, maybe reducing CPU overhead at
> the possible cost of a slight increase in GPU power.

We trade off CPU power vs GPU power, and I know how power hungry our GPU
is even at idle frequencies. And we should have system tests that detect
if we hold off rc6 for too long (or rather if we never hit rc6 :().
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre