Re: [Intel-gfx] [PATCH v2] drm/i915: Fix forcewake active domain tracking

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 10/03/2017 08:50, Chris Wilson wrote:
On Fri, Mar 10, 2017 at 07:32:51AM +0000, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>

In commit 003342a50021 ("drm/i915: Keep track of active
forcewake domains in a bitmask") I forgot to adjust the
newly introduce fw_domains_active state across reset.

This caused the assert_forcewakes_inactive to trigger
during suspend and resume if there were user held
forcewakes.

v2: Bitmask checks are required since vfuncs are not
    always present.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Fixes: 003342a50021 ("drm/i915: Keep track of active forcewake domains in a bitmask")
Testcase: igt/drv_suspend/forcewake
Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Cc: "Paneri, Praveen" <praveen.paneri@xxxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx>
Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
Cc: v4.10+ <stable@xxxxxxxxxxxxxxx>
---
 drivers/gpu/drm/i915/intel_uncore.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 2a3f35c30501..7efae77faca0 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -304,12 +304,14 @@ static void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv,
 	fw = dev_priv->uncore.fw_domains_active;
 	if (fw)
 		dev_priv->uncore.funcs.force_wake_put(dev_priv, fw);
+	dev_priv->uncore.fw_domains_active = 0;

Hmm, I think we would be happier with (think of a good name)

static void __intel_uncore_force_wake_get(i915, fw_domains)
{
	i915->uncore.funcs.force_wake_get(i915, fw_domains);
	i915->uncore.funcs.fw_domains_active |= fw_domain;
}

static void __intel_uncore_force_wake_put(i915, fw_domains)
{
	i915->uncore.funcs.force_wake_put(i915, fw_domains);
	i915->uncore.funcs.fw_domains_active &= ~fw_domain;
}

Another alternative would be to move the bitops down to the callbacks.
gcc might be happier, at the expense of some duplication and risk of
forgetting.

Anyway worth the effort?

Yes I agree, was considering the second option myself. Think I still prefer that one to keep the inseparable together.

Regards,

Tvrtko




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]