[Intel-gfx] [PATCH] drm/i915: Check caller held wakerefs in assert_forcewakes_active

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-07-03 13:43:34)
> 
> On 03/07/2019 13:12, Chris Wilson wrote:
> > The intent of the assert is to document that the caller took the
> > appropriate wakerefs for the function. However, as Tvrtko pointed out,
> > we simply check whether the fw_domains are active and may be confused by
> > the auto wakeref which may be dropped between the check and use. Let's
> > be more careful in the assert and check that each fw_domain has an
> > explicit caller wakeref above and beyond the automatic wakeref.
> 
> Although we still don't know if it is our caller who took the fw or some 
> unrelated thread. Still, a more thorough check is better even if not 
> perfect.

Since it's not a mutex, we can't stuff an owner field in here, the only
way to properly track in that case would be to return cookies from
forcewake_get and verify the cookies are still active.

Not feeling the inclination to do that yet. Maybe if we get a few fw
leaks.

> > Reported-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/intel_uncore.c | 24 ++++++++++++++++++++++++
> >   1 file changed, 24 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > index 68d54e126d79..bc25a6e51463 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -738,6 +738,12 @@ void assert_forcewakes_inactive(struct intel_uncore *uncore)
> >   void assert_forcewakes_active(struct intel_uncore *uncore,
> >                             enum forcewake_domains fw_domains)
> >   {
> > +     struct intel_uncore_forcewake_domain *domain;
> > +     unsigned int tmp;
> > +
> > +     if (!IS_ENABLED(CONFIG_DRM_i915_RUNTIME_PM))
> > +             return;
> > +
> 
> If uncore->funcs.force_wake_get is set why wouldn't we still want to run 
> the asserts?

I'm just being worried by adding a loop under irq-off and didn't want to
add more trouble to non-debug kernels. (Closing the stable door much?)
> 
> >       if (!uncore->funcs.force_wake_get)
> >               return;
> >   
> > @@ -747,6 +753,24 @@ void assert_forcewakes_active(struct intel_uncore *uncore,
> >       WARN(fw_domains & ~uncore->fw_domains_active,
> >            "Expected %08x fw_domains to be active, but %08x are off\n",
> >            fw_domains, fw_domains & ~uncore->fw_domains_active);
> > +
> > +     /*
> > +      * Check that the caller has an explicit wakeref and we don't mistake
> > +      * it for the auto wakeref.
> > +      */
> > +     local_irq_disable();
> > +     for_each_fw_domain_masked(domain, fw_domains, uncore, tmp) {
> > +             unsigned int expect = 1;
> > +
> > +             if (hrtimer_active(&domain->timer))
> > +                     expect++;
> > +
> > +             if (WARN(domain->wake_count < expect,
> > +                      "Expected domain %d to be held awake by caller\n",
> > +                      domain->id))
> > +                     break;
> > +     }
> > +     local_irq_enable();
> 
> This part looks good. Let wait and see if CI calls me a fool.

Aye, that's what I'm waiting for as well. Personalized insults from CI :)
-Chris