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@xxxxxxxxx>
> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
> > ---
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
