[PATCH] drm/i915: Fix forcewake active domain tracking

[PATCH] drm/i915: Fix forcewake active domain tracking

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

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.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Fixes: 003342a50021 ("drm/i915: Keep track of active forcewake domains in a bitmask")
Testcase: igt/drv_suspend/forcewake
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: "Paneri, Praveen" <praveen.paneri@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Cc: v4.10+ <stable@vger.kernel.org>
---
 drivers/gpu/drm/i915/intel_uncore.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 2a3f35c30501..efa040847d69 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -302,22 +302,21 @@ static void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv,
 	WARN_ON(active_domains);
 
 	fw = dev_priv->uncore.fw_domains_active;
-	if (fw)
-		dev_priv->uncore.funcs.force_wake_put(dev_priv, fw);
+	dev_priv->uncore.funcs.force_wake_put(dev_priv, fw);
+	dev_priv->uncore.fw_domains_active = 0;
 
 	fw_domains_reset(dev_priv, FORCEWAKE_ALL);
 
 	if (restore) { /* If reset with a user forcewake, try to restore */
-		if (fw)
-			dev_priv->uncore.funcs.force_wake_get(dev_priv, fw);
+		dev_priv->uncore.funcs.force_wake_get(dev_priv, fw);
+		dev_priv->uncore.fw_domains_active = fw;
 
 		if (IS_GEN6(dev_priv) || IS_GEN7(dev_priv))
 			dev_priv->uncore.fifo_count =
 				fifo_free_entries(dev_priv);
-	}
-
-	if (!restore)
+	} else {
 		assert_forcewakes_inactive(dev_priv);
+	}
 
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
-- 
2.9.3

Re: [PATCH] drm/i915: Fix forcewake active domain tracking

[Chris Wilson]

On Thu, Mar 09, 2017 at 03:52:50PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> 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.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Fixes: 003342a50021 ("drm/i915: Keep track of active forcewake domains in a bitmask")
> Testcase: igt/drv_suspend/forcewake
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: "Paneri, Praveen" <praveen.paneri@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Cc: v4.10+ <stable@vger.kernel.org>
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 2a3f35c30501..efa040847d69 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -302,22 +302,21 @@ static void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv,
>  	WARN_ON(active_domains);
>  
>  	fw = dev_priv->uncore.fw_domains_active;
> -	if (fw)
> -		dev_priv->uncore.funcs.force_wake_put(dev_priv, fw);

I see. On ilk-, funcs.force_wake_put/_get are NULL, so we need to keep
the if (fw) guard.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v2] drm/i915: Fix forcewake active domain tracking

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

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@intel.com>
Fixes: 003342a50021 ("drm/i915: Keep track of active forcewake domains in a bitmask")
Testcase: igt/drv_suspend/forcewake
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: "Paneri, Praveen" <praveen.paneri@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Cc: v4.10+ <stable@vger.kernel.org>
---
 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;
 
 	fw_domains_reset(dev_priv, FORCEWAKE_ALL);
 
 	if (restore) { /* If reset with a user forcewake, try to restore */
 		if (fw)
 			dev_priv->uncore.funcs.force_wake_get(dev_priv, fw);
+		dev_priv->uncore.fw_domains_active = fw;
 
 		if (IS_GEN6(dev_priv) || IS_GEN7(dev_priv))
 			dev_priv->uncore.fifo_count =
-- 
2.9.3

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

[Chris Wilson]

On Fri, Mar 10, 2017 at 07:32:51AM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> 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@intel.com>
> Fixes: 003342a50021 ("drm/i915: Keep track of active forcewake domains in a bitmask")
> Testcase: igt/drv_suspend/forcewake
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: "Paneri, Praveen" <praveen.paneri@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Cc: v4.10+ <stable@vger.kernel.org>
> ---
>  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?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Tvrtko Ursulin]

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@intel.com>
>>
>> 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@intel.com>
>> Fixes: 003342a50021 ("drm/i915: Keep track of active forcewake domains in a bitmask")
>> Testcase: igt/drv_suspend/forcewake
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: "Paneri, Praveen" <praveen.paneri@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> Cc: intel-gfx@lists.freedesktop.org
>> Cc: v4.10+ <stable@vger.kernel.org>
>> ---
>>  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

[PATCH v3] drm/i915: Fix forcewake active domain tracking

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

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.

v3: Move bitmask tracking to get/put vfunc for simplicity.
    (Chris Wilson)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Fixes: 003342a50021 ("drm/i915: Keep track of active forcewake domains in a bitmask")
Testcase: igt/drv_suspend/forcewake
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: "Paneri, Praveen" <praveen.paneri@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Cc: v4.10+ <stable@vger.kernel.org>
---
 drivers/gpu/drm/i915/intel_uncore.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 2a3f35c30501..71b9b387ad04 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -120,6 +120,8 @@ fw_domains_get(struct drm_i915_private *dev_priv, enum forcewake_domains fw_doma
 
 	for_each_fw_domain_masked(d, fw_domains, dev_priv)
 		fw_domain_wait_ack(d);
+
+	dev_priv->uncore.fw_domains_active |= fw_domains;
 }
 
 static void
@@ -131,6 +133,8 @@ fw_domains_put(struct drm_i915_private *dev_priv, enum forcewake_domains fw_doma
 		fw_domain_put(d);
 		fw_domain_posting_read(d);
 	}
+
+	dev_priv->uncore.fw_domains_active &= ~fw_domains;
 }
 
 static void
@@ -248,10 +252,8 @@ intel_uncore_fw_release_timer(struct hrtimer *timer)
 	if (WARN_ON(domain->wake_count == 0))
 		domain->wake_count++;
 
-	if (--domain->wake_count == 0) {
+	if (--domain->wake_count == 0)
 		dev_priv->uncore.funcs.force_wake_put(dev_priv, domain->mask);
-		dev_priv->uncore.fw_domains_active &= ~domain->mask;
-	}
 
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 
@@ -470,10 +472,8 @@ static void __intel_uncore_forcewake_get(struct drm_i915_private *dev_priv,
 			fw_domains &= ~domain->mask;
 	}
 
-	if (fw_domains) {
+	if (fw_domains)
 		dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_domains);
-		dev_priv->uncore.fw_domains_active |= fw_domains;
-	}
 }
 
 /**
@@ -940,7 +940,6 @@ static noinline void ___force_wake_auto(struct drm_i915_private *dev_priv,
 		fw_domain_arm_timer(domain);
 
 	dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_domains);
-	dev_priv->uncore.fw_domains_active |= fw_domains;
 }
 
 static inline void __force_wake_auto(struct drm_i915_private *dev_priv,
-- 
2.9.3

Re: [PATCH v3] drm/i915: Fix forcewake active domain tracking

[Chris Wilson]

On Fri, Mar 10, 2017 at 09:32:49AM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> 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.
> 
> v3: Move bitmask tracking to get/put vfunc for simplicity.
>     (Chris Wilson)
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Fixes: 003342a50021 ("drm/i915: Keep track of active forcewake domains in a bitmask")
> Testcase: igt/drv_suspend/forcewake
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: "Paneri, Praveen" <praveen.paneri@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Cc: v4.10+ <stable@vger.kernel.org>

After being a chicken and waiting for CI,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Tvrtko Ursulin]

On 10/03/2017 10:54, Chris Wilson wrote:
> On Fri, Mar 10, 2017 at 09:32:49AM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> 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.
>>
>> v3: Move bitmask tracking to get/put vfunc for simplicity.
>>     (Chris Wilson)
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Fixes: 003342a50021 ("drm/i915: Keep track of active forcewake domains in a bitmask")
>> Testcase: igt/drv_suspend/forcewake
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: "Paneri, Praveen" <praveen.paneri@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> Cc: intel-gfx@lists.freedesktop.org
>> Cc: v4.10+ <stable@vger.kernel.org>
>
> After being a chicken and waiting for CI,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

I know the feeling! Thanks, I also hope I haven't missed anything.

Pushed now.

Regards,

Tvrtko