Quoting Mika Kuoppala (2019-07-08 16:40:30)
> Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxx>
And hopefully this is last fix required to this simple loop. I should
never have listened to Tvrtko... Who needs helpful debug anyway? :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
