On 03/07/2019 13:48, Chris Wilson wrote:
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.
Agreed, I wasn't trying to say we have to do it.
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?)
What is the connection between debug/non-debug kernels and
CONFIG_DRM_i915_RUNTIME_PM?
Regards,
Tvrtko
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
