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

[Chris Wilson]

On Thu, Mar 24, 2016 at 02:19:38PM +0000, Chris Wilson wrote:
> On Thu, Mar 24, 2016 at 01:54:36PM +0000, Tvrtko Ursulin 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).
> > 
> > I don't see the bug, we got:
> > 
> > 1) __intel_uncore_forcewake_put - if refcount reaches zero it will
> > bump it and arm the timer to decrement and release. This is used
> > from explicit get/put paths.
> > 
> > 2) __force_wake_get - used from register reads only, so no explicit
> > put will happen. It just bumps the ref count and arms the timer.
> > 
> > I can't spot the bug, if there is one.
> 
> I mistook __force_wake_get for __intel_uncore_forcewake_get and jumped.
> 
> Having said that we do still have the issue of double-increment if the
> timer is already armed. Or do we? I'm pretty sure we do.

I'll answer that. That is also no, because of the spinlock guarding it.

Oh well, I really thought I had an explanation there.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre