[Intel-gfx] [PATCH] drm/i915: Explicitly track active fw_domain timers

[Chris Wilson]

Quoting Mika Kuoppala (2019-07-08 16:40:30)
> Chris Wilson <chris at chris-wilson.co.uk> writes:
> 
> > Stop guessing over whether we have an extra wakeref held by the delayed
> > fw put, and track it explicitly for the sake of debug.
> >
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_uncore.c | 13 ++++++-------
> >  drivers/gpu/drm/i915/intel_uncore.h |  1 +
> >  2 files changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > index 5f0367fd3200..768fee3c59aa 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -78,6 +78,8 @@ fw_domain_reset(const struct intel_uncore_forcewake_domain *d)
> >  static inline void
> >  fw_domain_arm_timer(struct intel_uncore_forcewake_domain *d)
> >  {
> > +     GEM_BUG_ON(d->uncore->fw_domains_timer & d->mask);
> > +     d->uncore->fw_domains_timer |= d->mask;
> >       d->wake_count++;
> >       hrtimer_start_range_ns(&d->timer,
> >                              NSEC_PER_MSEC,
> > @@ -353,9 +355,8 @@ intel_uncore_fw_release_timer(struct hrtimer *timer)
> >               return HRTIMER_RESTART;
> >  
> >       spin_lock_irqsave(&uncore->lock, irqflags);
> > -     if (WARN_ON(domain->wake_count == 0))
> > -             domain->wake_count++;
> >  
> > +     uncore->fw_domains_timer &= ~domain->mask;
> >       if (--domain->wake_count == 0)
> >               uncore->funcs.force_wake_put(uncore, domain->mask);
> >  
> > @@ -673,8 +674,7 @@ static void __intel_uncore_forcewake_put(struct intel_uncore *uncore,
> >       fw_domains &= uncore->fw_domains;
> >  
> >       for_each_fw_domain_masked(domain, fw_domains, uncore, tmp) {
> > -             if (WARN_ON(domain->wake_count == 0))
> > -                     continue;
> > +             GEM_BUG_ON(!domain->wake_count);
> 
> First impression was harsh. But on second thought, if we can't get
> our counts right, then what hope is there. None.

And we've had the warning there for yonks without a hit, so indeed I
feel justified in making any regression harsh.

> >               if (--domain->wake_count) {
> >                       domain->active = true;
> > @@ -764,7 +764,7 @@ void assert_forcewakes_active(struct intel_uncore *uncore,
> >               unsigned int actual = READ_ONCE(domain->wake_count);
> >               unsigned int expect = 1;
> >  
> > -             if (hrtimer_active(&domain->timer) && READ_ONCE(domain->active))
> > +             if (uncore->fw_domains_timer & domain->mask)
> >                       expect++; /* pending automatic release */
> 
> assetion of timer active on release path seems also superfluous
> as the count is our guard.
> 
> 
> 
> Reviewed-by: Mika Kuoppala <mika.kuoppala at linux.intel.com>

And hopefully this is last fix required to this simple loop. I should
never have listened to Tvrtko... Who needs helpful debug anyway? :)
-Chris