Intel-GFX Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: Fix forcewake active domain tracking
@ 2017-03-09 15:52 Tvrtko Ursulin
  2017-03-09 19:23 ` ✗ Fi.CI.BAT: failure for " Patchwork
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Tvrtko Ursulin @ 2017-03-09 15:52 UTC (permalink / raw)
  To: Intel-gfx; +Cc: intel-gfx, Paneri, Praveen, Daniel Vetter, v4 . 10+

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* ✗ Fi.CI.BAT: failure for drm/i915: Fix forcewake active domain tracking
  2017-03-09 15:52 [PATCH] drm/i915: Fix forcewake active domain tracking Tvrtko Ursulin
@ 2017-03-09 19:23 ` Patchwork
  2017-03-09 20:48   ` Chris Wilson
  2017-03-09 21:12 ` [PATCH] " Chris Wilson
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 12+ messages in thread
From: Patchwork @ 2017-03-09 19:23 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Fix forcewake active domain tracking
URL   : https://patchwork.freedesktop.org/series/21004/
State : failure

== Summary ==

Series 21004v1 drm/i915: Fix forcewake active domain tracking
https://patchwork.freedesktop.org/api/1.0/series/21004/revisions/1/mbox/

Test core_auth:
        Subgroup basic-auth:
                pass       -> SKIP       (fi-ilk-650)
Test core_prop_blob:
        Subgroup basic:
                pass       -> SKIP       (fi-ilk-650)
Test drv_getparams_basic:
        Subgroup basic-eu-total:
                pass       -> SKIP       (fi-ilk-650)
        Subgroup basic-subslice-total:
                pass       -> SKIP       (fi-ilk-650)
Test drv_hangman:
        Subgroup error-state-basic:
                pass       -> SKIP       (fi-ilk-650)
Test drv_module_reload:
        Subgroup basic-no-display:
                pass       -> FAIL       (fi-ilk-650)
        Subgroup basic-reload:
                pass       -> FAIL       (fi-ilk-650) fdo#100045
        Subgroup basic-reload-final:
                pass       -> FAIL       (fi-ilk-650) fdo#100045
Test gem_basic:
        Subgroup bad-close:
                pass       -> SKIP       (fi-ilk-650)
        Subgroup create-close:
                pass       -> SKIP       (fi-ilk-650)
        Subgroup create-fd-close:
                pass       -> SKIP       (fi-ilk-650)
Test gem_busy:
        Subgroup basic-busy-default:
                pass       -> SKIP       (fi-ilk-650)
        Subgroup basic-hang-default:
                pass       -> SKIP       (fi-ilk-650)
Test gem_close_race:
        Subgroup basic-process:
                pass       -> SKIP       (fi-ilk-650)
        Subgroup basic-threads:
                pass       -> SKIP       (fi-ilk-650)
Test gem_cpu_reloc:
        Subgroup basic:
                pass       -> SKIP       (fi-ilk-650)
Test gem_cs_tlb:
        Subgroup basic-default:
                pass       -> SKIP       (fi-ilk-650)
Test gem_exec_basic:
        Subgroup basic-bsd:
                pass       -> SKIP       (fi-ilk-650)
        Subgroup basic-default:
                pass       -> SKIP       (fi-ilk-650)
        Subgroup basic-render:
                pass       -> SKIP       (fi-ilk-650)
        Subgroup gtt-bsd:
                pass       -> SKIP       (fi-ilk-650)
        Subgroup gtt-default:
                pass       -> SKIP       (fi-ilk-650)
        Subgroup gtt-render:
                pass       -> SKIP       (fi-ilk-650)
        Subgroup readonly-bsd:
                pass       -> SKIP       (fi-ilk-650)
        Subgroup readonly-default:
                pass       -> SKIP       (fi-ilk-650)
        Subgroup readonly-render:
                pass       -> SKIP       (fi-ilk-650)
Test gem_exec_create:
        Subgroup basic:
                pass       -> SKIP       (fi-ilk-650)
Test gem_exec_fence:
        Subgroup await-hang-default:
                pass       -> SKIP       (fi-ilk-650)
        Subgroup basic-await-default:
                pass       -> SKIP       (fi-ilk-650)
        Subgroup basic-busy-default:
                pass       -> SKIP       (fi-ilk-650)
        Subgroup basic-wait-default:
                pass       -> SKIP       (fi-ilk-650)
        Subgroup nb-await-default:
                pass       -> SKIP       (fi-ilk-650)
Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                pass       -> SKIP       (fi-ilk-650)
                incomplete -> PASS       (fi-skl-6700k) fdo#100130
        Subgroup basic-batch-kernel-default-wb:
                pass       -> SKIP       (fi-ilk-650)
        Subgroup basic-uc-pro-default:
                pass       -> SKIP       (fi-ilk-650)
        Subgroup basic-uc-prw-default:
                pass       -> SKIP       (fi-ilk-650)
        Subgroup basic-uc-ro-default:
                pass       -> SKIP       (fi-ilk-650)
        Subgroup basic-uc-rw-default:
                pass       -> SKIP       (fi-ilk-650)
        Subgroup basic-uc-set-default:
                pass       -> SKIP       (fi-ilk-650)
        Subgroup basic-wb-pro-default:
                pass       -> SKIP       (fi-ilk-650)
                pass       -> INCOMPLETE (fi-skl-6700hq)
        Subgroup basic-wb-prw-default:
WARNING: Long output truncated

510c200742ced5a91d07e48220b669a3c9b30c0c drm-tip: 2017y-03m-09d-15h-21m-14s UTC integration manifest
b1962c6 drm/i915: Fix forcewake active domain tracking

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4122/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: ✗ Fi.CI.BAT:  failure for drm/i915: Fix forcewake active domain tracking
  2017-03-09 19:23 ` ✗ Fi.CI.BAT: failure for " Patchwork
@ 2017-03-09 20:48   ` Chris Wilson
  0 siblings, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2017-03-09 20:48 UTC (permalink / raw)
  To: intel-gfx

On Thu, Mar 09, 2017 at 07:23:14PM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: drm/i915: Fix forcewake active domain tracking
> URL   : https://patchwork.freedesktop.org/series/21004/
> State : failure
> 
> == Summary ==
> 
> Series 21004v1 drm/i915: Fix forcewake active domain tracking
> https://patchwork.freedesktop.org/api/1.0/series/21004/revisions/1/mbox/
> 
> Test core_auth:
>         Subgroup basic-auth:
>                 pass       -> SKIP       (fi-ilk-650)

[    3.470450] BUG: unable to handle kernel NULL pointer dereference at           (null)
[    3.485921] IP:           (null)
[    3.489198] PGD 0

[    3.492793] Oops: 0010 [#1] PREEMPT SMP
[    3.496675] Modules linked in: i915(+) e1000e ptp pps_core prime_numbers
[    3.503426] CPU: 1 PID: 194 Comm: systemd-udevd Not tainted 4.11.0-rc1-CI-Patchwork_4122+ #1
[    3.511941] Hardware name: Hewlett-Packard HP Compaq 8100 Elite SFF PC/304Ah, BIOS 786H1 v01.13 07/14/2011
[    3.521671] task: ffff88020961cbc0 task.stack: ffffc90000470000
[    3.527634] RIP: 0010:          (null)
[    3.531427] RSP: 0018:ffffc90000473a60 EFLAGS: 00010046
[    3.536696] RAX: 0000000000000282 RBX: ffff880205b70da8 RCX: ffff88020961d410
[    3.543875] RDX: 0000000000000001 RSI: 0000000000000000 RDI: ffff880205b70000
[    3.551057] RBP: ffffc90000473ab0 R08: 00000000d9242164 R09: 59311d7500000000
[    3.558235] R10: ffffffff827ede60 R11: ffff88020961cbc0 R12: 0000000000000000
[    3.565412] R13: ffff880205b70c58 R14: ffff880205b70000 R15: 0000000000000282
[    3.572590] FS:  00007f34796fa8c0(0000) GS:ffff88021bc40000(0000) knlGS:0000000000000000
[    3.580757] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    3.586546] CR2: 0000000000000000 CR3: 0000000209753000 CR4: 00000000000006e0
[    3.593723] Call Trace:
[    3.596246]  ? intel_uncore_forcewake_reset+0x110/0x260 [i915]
[    3.602153]  __intel_uncore_early_sanitize+0x55/0xb0 [i915]
[    3.607801]  intel_uncore_init+0x162/0x5e0 [i915]
[    3.612577]  i915_driver_load+0x5c5/0x1570 [i915]
[    3.617330]  ? trace_hardirqs_on_caller+0xe7/0x200
[    3.622167]  ? trace_hardirqs_on+0xd/0x10
[    3.626247]  i915_pci_probe+0x34/0xa0 [i915]
[    3.630563]  pci_device_probe+0x91/0x110
[    3.634533]  driver_probe_device+0x2ba/0x450
[    3.638848]  __driver_attach+0xde/0xe0
[    3.642642]  ? driver_probe_device+0x450/0x450
[    3.647952]  bus_for_each_dev+0x61/0xa0
[    3.651836]  driver_attach+0x19/0x20
[    3.655457]  bus_add_driver+0x1fb/0x270
[    3.659337]  ? 0xffffffffa01fc000
[    3.662697]  driver_register+0x5b/0xd0
[    3.666490]  ? 0xffffffffa01fc000
[    3.669852]  __pci_register_driver+0x5b/0x60
[    3.674191]  i915_init+0x6b/0x6e [i915]
[    3.678076]  do_one_initcall+0x3f/0x170
[    3.681958]  ? rcu_read_lock_sched_held+0x75/0x80
[    3.686710]  ? kmem_cache_alloc_trace+0x274/0x2e0
[    3.691460]  ? do_init_module+0x22/0x1f3
[    3.695427]  do_init_module+0x5a/0x1f3
[    3.699221]  load_module+0x2091/0x2410
[    3.703015]  ? symbol_put_addr+0x60/0x60
[    3.706984]  ? kernel_read_file+0x105/0x190
[    3.711212]  SyS_finit_module+0xbc/0xf0
[    3.715094]  entry_SYSCALL_64_fastpath+0x1c/0xb1

ilk being the only !fw machine in the BAT set.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] drm/i915: Fix forcewake active domain tracking
  2017-03-09 15:52 [PATCH] drm/i915: Fix forcewake active domain tracking Tvrtko Ursulin
  2017-03-09 19:23 ` ✗ Fi.CI.BAT: failure for " Patchwork
@ 2017-03-09 21:12 ` Chris Wilson
  2017-03-10  7:32   ` [PATCH v2] " Tvrtko Ursulin
  2017-03-10  9:22 ` ✓ Fi.CI.BAT: success for drm/i915: Fix forcewake active domain tracking (rev2) Patchwork
  2017-03-10 10:47 ` ✓ Fi.CI.BAT: success for drm/i915: Fix forcewake active domain tracking (rev4) Patchwork
  3 siblings, 1 reply; 12+ messages in thread
From: Chris Wilson @ 2017-03-09 21:12 UTC (permalink / raw)
  To: Tvrtko Ursulin
  Cc: Intel-gfx, Tvrtko Ursulin, Paneri, Praveen, Daniel Vetter,
	Jani Nikula, v4 . 10+

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2] drm/i915: Fix forcewake active domain tracking
  2017-03-09 21:12 ` [PATCH] " Chris Wilson
@ 2017-03-10  7:32   ` Tvrtko Ursulin
  2017-03-10  8:50     ` Chris Wilson
  0 siblings, 1 reply; 12+ messages in thread
From: Tvrtko Ursulin @ 2017-03-10  7:32 UTC (permalink / raw)
  To: Intel-gfx; +Cc: intel-gfx, Paneri, Praveen, Daniel Vetter, v4 . 10+

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] drm/i915: Fix forcewake active domain tracking
  2017-03-10  7:32   ` [PATCH v2] " Tvrtko Ursulin
@ 2017-03-10  8:50     ` Chris Wilson
  2017-03-10  9:09       ` Tvrtko Ursulin
  0 siblings, 1 reply; 12+ messages in thread
From: Chris Wilson @ 2017-03-10  8:50 UTC (permalink / raw)
  To: Tvrtko Ursulin
  Cc: Intel-gfx, Tvrtko Ursulin, Paneri, Praveen, Daniel Vetter,
	Jani Nikula, v4 . 10+

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] drm/i915: Fix forcewake active domain tracking
  2017-03-10  8:50     ` Chris Wilson
@ 2017-03-10  9:09       ` Tvrtko Ursulin
  2017-03-10  9:32         ` [PATCH v3] " Tvrtko Ursulin
  0 siblings, 1 reply; 12+ messages in thread
From: Tvrtko Ursulin @ 2017-03-10  9:09 UTC (permalink / raw)
  To: Chris Wilson, Tvrtko Ursulin, Intel-gfx, Tvrtko Ursulin,
	Paneri, Praveen, Daniel Vetter, Jani Nikula, v4 . 10+


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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

* ✓ Fi.CI.BAT: success for drm/i915: Fix forcewake active domain tracking (rev2)
  2017-03-09 15:52 [PATCH] drm/i915: Fix forcewake active domain tracking Tvrtko Ursulin
  2017-03-09 19:23 ` ✗ Fi.CI.BAT: failure for " Patchwork
  2017-03-09 21:12 ` [PATCH] " Chris Wilson
@ 2017-03-10  9:22 ` Patchwork
  2017-03-10 10:47 ` ✓ Fi.CI.BAT: success for drm/i915: Fix forcewake active domain tracking (rev4) Patchwork
  3 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2017-03-10  9:22 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Fix forcewake active domain tracking (rev2)
URL   : https://patchwork.freedesktop.org/series/21004/
State : success

== Summary ==

Series 21004v2 drm/i915: Fix forcewake active domain tracking
https://patchwork.freedesktop.org/api/1.0/series/21004/revisions/2/mbox/

Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                pass       -> DMESG-WARN (fi-bxt-t5700) fdo#100125

fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 461s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 605s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 538s
fi-bxt-t5700     total:278  pass:257  dwarn:1   dfail:0   fail:0   skip:20  time: 595s
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27  time: 503s
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31  time: 510s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 449s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 435s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 438s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 500s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 498s
fi-kbl-7500u     total:278  pass:259  dwarn:1   dfail:0   fail:0   skip:18  time: 477s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 514s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 606s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 495s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 563s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 553s
fi-snb-2600      total:278  pass:248  dwarn:0   dfail:0   fail:1   skip:29  time: 421s

48275556138fab369735c85ce5e3d6d6d6238f87 drm-tip: 2017y-03m-10d-07h-47m-54s UTC integration manifest
0039829 drm/i915: Fix forcewake active domain tracking

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4130/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v3] drm/i915: Fix forcewake active domain tracking
  2017-03-10  9:09       ` Tvrtko Ursulin
@ 2017-03-10  9:32         ` Tvrtko Ursulin
  2017-03-10 10:54           ` Chris Wilson
  0 siblings, 1 reply; 12+ messages in thread
From: Tvrtko Ursulin @ 2017-03-10  9:32 UTC (permalink / raw)
  To: Intel-gfx; +Cc: intel-gfx, Paneri, Praveen, Daniel Vetter, v4 . 10+

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* ✓ Fi.CI.BAT: success for drm/i915: Fix forcewake active domain tracking (rev4)
  2017-03-09 15:52 [PATCH] drm/i915: Fix forcewake active domain tracking Tvrtko Ursulin
                   ` (2 preceding siblings ...)
  2017-03-10  9:22 ` ✓ Fi.CI.BAT: success for drm/i915: Fix forcewake active domain tracking (rev2) Patchwork
@ 2017-03-10 10:47 ` Patchwork
  3 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2017-03-10 10:47 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Fix forcewake active domain tracking (rev4)
URL   : https://patchwork.freedesktop.org/series/21004/
State : success

== Summary ==

Series 21004v4 drm/i915: Fix forcewake active domain tracking
https://patchwork.freedesktop.org/api/1.0/series/21004/revisions/4/mbox/

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 466s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 611s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 539s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 598s
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27  time: 506s
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31  time: 497s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 438s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 432s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 445s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 505s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 493s
fi-kbl-7500u     total:278  pass:259  dwarn:1   dfail:0   fail:0   skip:18  time: 477s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 509s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 594s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 503s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 554s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 555s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time: 434s

ac8ee658bd289a75ffb5d8aad3878a0394358a5b drm-tip: 2017y-03m-10d-09h-07m-40s UTC integration manifest
341ac06 drm/i915: Fix forcewake active domain tracking

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4132/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] drm/i915: Fix forcewake active domain tracking
  2017-03-10  9:32         ` [PATCH v3] " Tvrtko Ursulin
@ 2017-03-10 10:54           ` Chris Wilson
  2017-03-10 11:51             ` Tvrtko Ursulin
  0 siblings, 1 reply; 12+ messages in thread
From: Chris Wilson @ 2017-03-10 10:54 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: Intel-gfx, v4 . 10+, Paneri, Praveen, Daniel Vetter

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] drm/i915: Fix forcewake active domain tracking
  2017-03-10 10:54           ` Chris Wilson
@ 2017-03-10 11:51             ` Tvrtko Ursulin
  0 siblings, 0 replies; 12+ messages in thread
From: Tvrtko Ursulin @ 2017-03-10 11:51 UTC (permalink / raw)
  To: Chris Wilson, Tvrtko Ursulin, Intel-gfx, Tvrtko Ursulin,
	Paneri, Praveen, Daniel Vetter, Jani Nikula, v4 . 10+


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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2017-03-10 11:51 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-09 15:52 [PATCH] drm/i915: Fix forcewake active domain tracking Tvrtko Ursulin
2017-03-09 19:23 ` ✗ Fi.CI.BAT: failure for " Patchwork
2017-03-09 20:48   ` Chris Wilson
2017-03-09 21:12 ` [PATCH] " Chris Wilson
2017-03-10  7:32   ` [PATCH v2] " Tvrtko Ursulin
2017-03-10  8:50     ` Chris Wilson
2017-03-10  9:09       ` Tvrtko Ursulin
2017-03-10  9:32         ` [PATCH v3] " Tvrtko Ursulin
2017-03-10 10:54           ` Chris Wilson
2017-03-10 11:51             ` Tvrtko Ursulin
2017-03-10  9:22 ` ✓ Fi.CI.BAT: success for drm/i915: Fix forcewake active domain tracking (rev2) Patchwork
2017-03-10 10:47 ` ✓ Fi.CI.BAT: success for drm/i915: Fix forcewake active domain tracking (rev4) Patchwork

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox