All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: Verify power domains after enabling them
@ 2018-08-17 12:26 Imre Deak
  2018-08-17 12:32 ` Chris Wilson
                   ` (7 more replies)
  0 siblings, 8 replies; 17+ messages in thread
From: Imre Deak @ 2018-08-17 12:26 UTC (permalink / raw)
  To: intel-gfx

After
commit 2cd9a689e97b ("Refactor intel_display_set_init_power() logic")
it makes more sense to check the power domain/well refcounts after
enabling the power domains functionality. Before that it's guaranteed
that most power wells (in the INIT domain) will have a reference held,
so not an interesting state.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c    | 2 --
 drivers/gpu/drm/i915/intel_runtime_pm.c | 2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 09d62b0c62cb..ad0f0e5389d9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15907,8 +15907,6 @@ intel_modeset_setup_hw_state(struct drm_device *dev,
 
 	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
 
-	intel_power_domains_verify_state(dev_priv);
-
 	intel_fbc_init_pipe_state(dev_priv);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 6153d5be5cf6..fad39239ef9c 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -3805,6 +3805,8 @@ void intel_power_domains_fini_hw(struct drm_i915_private *dev_priv)
 void intel_power_domains_enable(struct drm_i915_private *dev_priv)
 {
 	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
+
+	intel_power_domains_verify_state(dev_priv);
 }
 
 /**
-- 
2.13.2

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

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

* Re: [PATCH] drm/i915: Verify power domains after enabling them
  2018-08-17 12:26 [PATCH] drm/i915: Verify power domains after enabling them Imre Deak
@ 2018-08-17 12:32 ` Chris Wilson
  2018-08-17 12:53   ` Imre Deak
  2018-08-17 12:51 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 17+ messages in thread
From: Chris Wilson @ 2018-08-17 12:32 UTC (permalink / raw)
  To: Imre Deak, intel-gfx

Quoting Imre Deak (2018-08-17 13:26:13)
> After
> commit 2cd9a689e97b ("Refactor intel_display_set_init_power() logic")
> it makes more sense to check the power domain/well refcounts after
> enabling the power domains functionality. Before that it's guaranteed
> that most power wells (in the INIT domain) will have a reference held,
> so not an interesting state.

Indeed, that is true. But it is also used to check that we do acquire
the powerwells on init. I think it would sensible to include a verify
state at the end of power_domains_init_hw, or do you think the
sync_hw makes that superfluous?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Verify power domains after enabling them
  2018-08-17 12:26 [PATCH] drm/i915: Verify power domains after enabling them Imre Deak
  2018-08-17 12:32 ` Chris Wilson
@ 2018-08-17 12:51 ` Patchwork
  2018-08-17 13:09 ` ✓ Fi.CI.BAT: success " Patchwork
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2018-08-17 12:51 UTC (permalink / raw)
  To: Imre Deak; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Verify power domains after enabling them
URL   : https://patchwork.freedesktop.org/series/48394/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
89a97df5f335 drm/i915: Verify power domains after enabling them
-:7: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 2cd9a689e97b ("drm/i915: Refactor intel_display_set_init_power() logic")'
#7: 
commit 2cd9a689e97b ("Refactor intel_display_set_init_power() logic")

total: 1 errors, 0 warnings, 0 checks, 16 lines checked

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

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

* Re: [PATCH] drm/i915: Verify power domains after enabling them
  2018-08-17 12:32 ` Chris Wilson
@ 2018-08-17 12:53   ` Imre Deak
  2018-08-17 12:58     ` Chris Wilson
  0 siblings, 1 reply; 17+ messages in thread
From: Imre Deak @ 2018-08-17 12:53 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On Fri, Aug 17, 2018 at 01:32:24PM +0100, Chris Wilson wrote:
> Quoting Imre Deak (2018-08-17 13:26:13)
> > After
> > commit 2cd9a689e97b ("Refactor intel_display_set_init_power() logic")
> > it makes more sense to check the power domain/well refcounts after
> > enabling the power domains functionality. Before that it's guaranteed
> > that most power wells (in the INIT domain) will have a reference held,
> > so not an interesting state.
> 
> Indeed, that is true. But it is also used to check that we do acquire
> the powerwells on init.

Yes, intel_power_domains_enable() is called both during init and system
resume, after HW readout and acquiring the needed power wells.

> I think it would sensible to include a verify state at the end of
> power_domains_init_hw, or do you think the sync_hw makes that
> superfluous?

We have all power wells in the INIT domain enabled there, so I thought
it's less interesting, but yes I can also add the check to init_hw,
fini_hw, enable/disable and suspend/resume steps.

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

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

* Re: [PATCH] drm/i915: Verify power domains after enabling them
  2018-08-17 12:53   ` Imre Deak
@ 2018-08-17 12:58     ` Chris Wilson
  0 siblings, 0 replies; 17+ messages in thread
From: Chris Wilson @ 2018-08-17 12:58 UTC (permalink / raw)
  To: Imre Deak; +Cc: intel-gfx

Quoting Imre Deak (2018-08-17 13:53:06)
> On Fri, Aug 17, 2018 at 01:32:24PM +0100, Chris Wilson wrote:
> > Quoting Imre Deak (2018-08-17 13:26:13)
> > > After
> > > commit 2cd9a689e97b ("Refactor intel_display_set_init_power() logic")
> > > it makes more sense to check the power domain/well refcounts after
> > > enabling the power domains functionality. Before that it's guaranteed
> > > that most power wells (in the INIT domain) will have a reference held,
> > > so not an interesting state.
> > 
> > Indeed, that is true. But it is also used to check that we do acquire
> > the powerwells on init.
> 
> Yes, intel_power_domains_enable() is called both during init and system
> resume, after HW readout and acquiring the needed power wells.
> 
> > I think it would sensible to include a verify state at the end of
> > power_domains_init_hw, or do you think the sync_hw makes that
> > superfluous?
> 
> We have all power wells in the INIT domain enabled there, so I thought
> it's less interesting, but yes I can also add the check to init_hw,
> fini_hw, enable/disable and suspend/resume steps.

Yup, agree on the less interesting, but still useful to nip errors in
the bud where we require a powerwell but failed to bring the HW up. And
the pm state change is so infrequent (albeit suspend/resume are latency
sensitive for some environments) that a bit of sanity checking is
worthwhile. On the other hand, if it does take over 1ms to verify the
state, perhaps not for resume. ;)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* ✓ Fi.CI.BAT: success for drm/i915: Verify power domains after enabling them
  2018-08-17 12:26 [PATCH] drm/i915: Verify power domains after enabling them Imre Deak
  2018-08-17 12:32 ` Chris Wilson
  2018-08-17 12:51 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
@ 2018-08-17 13:09 ` Patchwork
  2018-08-17 13:18 ` [PATCH v2] " Imre Deak
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2018-08-17 13:09 UTC (permalink / raw)
  To: Imre Deak; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Verify power domains after enabling them
URL   : https://patchwork.freedesktop.org/series/48394/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4685 -> Patchwork_9970 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/48394/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9970:

  === IGT changes ===

    ==== Possible regressions ====

    igt@drv_selftest@live_evict:
      {fi-bsw-kefka}:     PASS -> DMESG-WARN

    
    ==== Warnings ====

    {igt@pm_rpm@module-reload}:
      {fi-icl-u}:         WARN (fdo#107602) -> DMESG-FAIL

    
== Known issues ==

  Here are the changes found in Patchwork_9970 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_coherency:
      fi-gdg-551:         PASS -> DMESG-FAIL (fdo#107164)

    igt@drv_selftest@live_guc:
      {fi-icl-u}:         PASS -> DMESG-WARN (fdo#107591) +14

    igt@drv_selftest@live_hangcheck:
      fi-cfl-s3:          PASS -> DMESG-FAIL (fdo#106560)

    igt@gem_exec_suspend@basic-s4-devices:
      fi-kbl-7500u:       PASS -> DMESG-WARN (fdo#107139, fdo#105128)

    igt@kms_pipe_crc_basic@read-crc-pipe-a:
      {fi-byt-clapper}:   PASS -> FAIL (fdo#107362)

    {igt@pm_rpm@module-reload}:
      fi-byt-j1900:       NOTRUN -> WARN (fdo#107602)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_hangcheck:
      fi-skl-guc:         DMESG-FAIL (fdo#107174) -> PASS

    igt@gem_exec_basic@readonly-blt:
      fi-byt-n2820:       FAIL (fdo#105900) -> PASS

    igt@kms_frontbuffer_tracking@basic:
      fi-hsw-peppy:       DMESG-FAIL (fdo#102614) -> PASS

    igt@prime_vgem@basic-fence-flip:
      fi-ivb-3770:        FAIL (fdo#104008) -> PASS

    
    ==== Warnings ====

    {igt@kms_psr@primary_page_flip}:
      fi-cnl-psr:         DMESG-FAIL (fdo#107372) -> DMESG-WARN (fdo#107372)

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
  fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
  fdo#105128 https://bugs.freedesktop.org/show_bug.cgi?id=105128
  fdo#105900 https://bugs.freedesktop.org/show_bug.cgi?id=105900
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  fdo#107139 https://bugs.freedesktop.org/show_bug.cgi?id=107139
  fdo#107164 https://bugs.freedesktop.org/show_bug.cgi?id=107164
  fdo#107174 https://bugs.freedesktop.org/show_bug.cgi?id=107174
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107372 https://bugs.freedesktop.org/show_bug.cgi?id=107372
  fdo#107591 https://bugs.freedesktop.org/show_bug.cgi?id=107591
  fdo#107602 https://bugs.freedesktop.org/show_bug.cgi?id=107602


== Participating hosts (52 -> 49) ==

  Additional (1): fi-byt-j1900 
  Missing    (4): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4685 -> Patchwork_9970

  CI_DRM_4685: df7e8eddc3830216d3fec15e2c7d0b6ec97e7bae @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4606: 38a44003774e35c587c67c8766b35e75dbb993b8 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9970: 89a97df5f3355480c510785fac831ed292e605f3 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

89a97df5f335 drm/i915: Verify power domains after enabling them

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9970/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* [PATCH v2] drm/i915: Verify power domains after enabling them
  2018-08-17 12:26 [PATCH] drm/i915: Verify power domains after enabling them Imre Deak
                   ` (2 preceding siblings ...)
  2018-08-17 13:09 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2018-08-17 13:18 ` Imre Deak
  2018-08-17 13:25   ` Chris Wilson
  2018-08-17 14:58   ` [PATCH v3] " Imre Deak
  2018-08-17 13:27 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Verify power domains after enabling them (rev2) Patchwork
                   ` (3 subsequent siblings)
  7 siblings, 2 replies; 17+ messages in thread
From: Imre Deak @ 2018-08-17 13:18 UTC (permalink / raw)
  To: intel-gfx

After
commit 2cd9a689e97b ("Refactor intel_display_set_init_power() logic")
it makes more sense to check the power domain/well refcounts after
enabling the power domains functionality. Before that it's guaranteed
that most power wells (in the INIT domain) will have a reference held,
so not an interesting state.

While at it also add the check after the init_hw/fini_hw, disable
and suspend/resume steps. The check is fast since the power well HW
state is cached.

v2:
- Add the state check to more spots. (Chris)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c    |  2 --
 drivers/gpu/drm/i915/intel_drv.h        |  1 -
 drivers/gpu/drm/i915/intel_runtime_pm.c | 24 +++++++++++++++++++-----
 3 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 09d62b0c62cb..ad0f0e5389d9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15907,8 +15907,6 @@ intel_modeset_setup_hw_state(struct drm_device *dev,
 
 	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
 
-	intel_power_domains_verify_state(dev_priv);
-
 	intel_fbc_init_pipe_state(dev_priv);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d0402051925b..529e72bcc5ef 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1966,7 +1966,6 @@ enum i915_drm_suspend_mode {
 void intel_power_domains_suspend(struct drm_i915_private *dev_priv,
 				 enum i915_drm_suspend_mode);
 void intel_power_domains_resume(struct drm_i915_private *dev_priv);
-void intel_power_domains_verify_state(struct drm_i915_private *dev_priv);
 void bxt_display_core_init(struct drm_i915_private *dev_priv, bool resume);
 void bxt_display_core_uninit(struct drm_i915_private *dev_priv);
 void intel_runtime_pm_enable(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 6153d5be5cf6..813dc7684634 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -3716,6 +3716,8 @@ static void vlv_cmnlane_wa(struct drm_i915_private *dev_priv)
 	cmn->desc->ops->disable(dev_priv, cmn);
 }
 
+static void intel_power_domains_verify_state(struct drm_i915_private *dev_priv);
+
 /**
  * intel_power_domains_init_hw - initialize hardware power domain state
  * @dev_priv: i915 device instance
@@ -3767,6 +3769,8 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv, bool resume)
 		intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
 	intel_power_domains_sync_hw(dev_priv);
 	power_domains->initializing = false;
+
+	intel_power_domains_verify_state(dev_priv);
 }
 
 /**
@@ -3788,6 +3792,8 @@ void intel_power_domains_fini_hw(struct drm_i915_private *dev_priv)
 	/* Remove the refcount we took to keep power well support disabled. */
 	if (!i915_modparams.disable_power_well)
 		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
+
+	intel_power_domains_verify_state(dev_priv);
 }
 
 /**
@@ -3805,6 +3811,8 @@ void intel_power_domains_fini_hw(struct drm_i915_private *dev_priv)
 void intel_power_domains_enable(struct drm_i915_private *dev_priv)
 {
 	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
+
+	intel_power_domains_verify_state(dev_priv);
 }
 
 /**
@@ -3817,6 +3825,8 @@ void intel_power_domains_enable(struct drm_i915_private *dev_priv)
 void intel_power_domains_disable(struct drm_i915_private *dev_priv)
 {
 	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
+
+	intel_power_domains_verify_state(dev_priv);
 }
 
 /**
@@ -3846,7 +3856,7 @@ void intel_power_domains_suspend(struct drm_i915_private *dev_priv,
 	 */
 	if (!IS_GEN9_LP(dev_priv) && suspend_mode == I915_DRM_SUSPEND_IDLE &&
 	    dev_priv->csr.dmc_payload != NULL)
-		return;
+		goto verify_state;
 
 	/*
 	 * Even if power well support was disabled we still want to disable
@@ -3865,6 +3875,10 @@ void intel_power_domains_suspend(struct drm_i915_private *dev_priv,
 		bxt_display_core_uninit(dev_priv);
 
 	power_domains->display_core_suspended = true;
+
+verify_state:
+	intel_power_domains_verify_state(dev_priv);
+
 }
 
 /**
@@ -3884,11 +3898,11 @@ void intel_power_domains_resume(struct drm_i915_private *dev_priv)
 	if (power_domains->display_core_suspended) {
 		intel_power_domains_init_hw(dev_priv, true);
 		power_domains->display_core_suspended = false;
-
-		return;
+	} else {
+		intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
 	}
 
-	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
+	intel_power_domains_verify_state(dev_priv);
 }
 
 static void intel_power_domains_dump_info(struct drm_i915_private *dev_priv)
@@ -3919,7 +3933,7 @@ static void intel_power_domains_dump_info(struct drm_i915_private *dev_priv)
  * acquiring reference counts for any power wells in use and disabling the
  * ones left on by BIOS but not required by any active output.
  */
-void intel_power_domains_verify_state(struct drm_i915_private *dev_priv)
+static void intel_power_domains_verify_state(struct drm_i915_private *dev_priv)
 {
 	struct i915_power_domains *power_domains = &dev_priv->power_domains;
 	struct i915_power_well *power_well;
-- 
2.13.2

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

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

* Re: [PATCH v2] drm/i915: Verify power domains after enabling them
  2018-08-17 13:18 ` [PATCH v2] " Imre Deak
@ 2018-08-17 13:25   ` Chris Wilson
  2018-08-17 14:58   ` [PATCH v3] " Imre Deak
  1 sibling, 0 replies; 17+ messages in thread
From: Chris Wilson @ 2018-08-17 13:25 UTC (permalink / raw)
  To: Imre Deak, intel-gfx

Quoting Imre Deak (2018-08-17 14:18:02)
> After
> commit 2cd9a689e97b ("Refactor intel_display_set_init_power() logic")
> it makes more sense to check the power domain/well refcounts after
> enabling the power domains functionality. Before that it's guaranteed
> that most power wells (in the INIT domain) will have a reference held,
> so not an interesting state.
> 
> While at it also add the check after the init_hw/fini_hw, disable
> and suspend/resume steps. The check is fast since the power well HW
> state is cached.
> 
> v2:
> - Add the state check to more spots. (Chris)
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
> @@ -3865,6 +3875,10 @@ void intel_power_domains_suspend(struct drm_i915_private *dev_priv,
>                 bxt_display_core_uninit(dev_priv);
>  
>         power_domains->display_core_suspended = true;
> +
> +verify_state:
> +       intel_power_domains_verify_state(dev_priv);
> +
>  }

Bonus '\n'.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Verify power domains after enabling them (rev2)
  2018-08-17 12:26 [PATCH] drm/i915: Verify power domains after enabling them Imre Deak
                   ` (3 preceding siblings ...)
  2018-08-17 13:18 ` [PATCH v2] " Imre Deak
@ 2018-08-17 13:27 ` Patchwork
  2018-08-17 13:44 ` ✗ Fi.CI.BAT: failure " Patchwork
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2018-08-17 13:27 UTC (permalink / raw)
  To: Imre Deak; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Verify power domains after enabling them (rev2)
URL   : https://patchwork.freedesktop.org/series/48394/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
3f95a53c8ade drm/i915: Verify power domains after enabling them
-:7: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 2cd9a689e97b ("drm/i915: Refactor intel_display_set_init_power() logic")'
#7: 
commit 2cd9a689e97b ("Refactor intel_display_set_init_power() logic")

total: 1 errors, 0 warnings, 0 checks, 95 lines checked

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

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

* ✗ Fi.CI.BAT: failure for drm/i915: Verify power domains after enabling them (rev2)
  2018-08-17 12:26 [PATCH] drm/i915: Verify power domains after enabling them Imre Deak
                   ` (4 preceding siblings ...)
  2018-08-17 13:27 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Verify power domains after enabling them (rev2) Patchwork
@ 2018-08-17 13:44 ` Patchwork
  2018-08-17 13:56   ` Imre Deak
  2018-08-17 13:57   ` Chris Wilson
  2018-08-17 15:07 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Verify power domains after enabling them (rev3) Patchwork
  2018-08-17 15:24 ` ✓ Fi.CI.BAT: success " Patchwork
  7 siblings, 2 replies; 17+ messages in thread
From: Patchwork @ 2018-08-17 13:44 UTC (permalink / raw)
  To: Imre Deak; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Verify power domains after enabling them (rev2)
URL   : https://patchwork.freedesktop.org/series/48394/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4685 -> Patchwork_9971 =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_9971 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9971, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/48394/revisions/2/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9971:

  === IGT changes ===

    ==== Possible regressions ====

    igt@gem_exec_suspend@basic-s3:
      fi-skl-6770hq:      PASS -> DMESG-WARN +3
      {fi-cfl-8109u}:     PASS -> DMESG-WARN +3
      fi-cfl-s3:          PASS -> DMESG-WARN +3
      fi-skl-6260u:       PASS -> DMESG-WARN +3
      fi-kbl-7567u:       PASS -> DMESG-WARN +3
      fi-kbl-guc:         PASS -> DMESG-WARN
      {fi-kbl-8809g}:     PASS -> DMESG-WARN

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
      fi-whl-u:           PASS -> DMESG-WARN +3
      fi-cfl-guc:         PASS -> DMESG-WARN +3
      fi-glk-j4005:       PASS -> DMESG-WARN +3
      fi-kbl-7500u:       PASS -> DMESG-WARN +3
      fi-kbl-7560u:       PASS -> DMESG-WARN +3

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      {fi-skl-iommu}:     PASS -> DMESG-WARN +3
      fi-skl-6700k2:      PASS -> DMESG-WARN +4
      fi-skl-6700hq:      PASS -> DMESG-WARN +3

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-glk-dsi:         PASS -> DMESG-WARN +3
      fi-cfl-8700k:       PASS -> DMESG-WARN +3
      fi-skl-guc:         PASS -> DMESG-WARN +3
      fi-bxt-j4205:       PASS -> DMESG-WARN +3
      fi-bxt-dsi:         PASS -> DMESG-WARN +3
      fi-cnl-psr:         PASS -> DMESG-WARN +3
      fi-skl-gvtdvm:      PASS -> DMESG-WARN +3
      fi-skl-6600u:       PASS -> DMESG-WARN +3

    
    ==== Warnings ====

    {igt@pm_rpm@module-reload}:
      {fi-icl-u}:         WARN (fdo#107602) -> DMESG-FAIL

    
== Known issues ==

  Here are the changes found in Patchwork_9971 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_guc:
      {fi-icl-u}:         PASS -> DMESG-WARN (fdo#107591) +14

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      {fi-icl-u}:         PASS -> DMESG-WARN (fdo#107382) +3

    {igt@pm_rpm@module-reload}:
      fi-byt-j1900:       NOTRUN -> WARN (fdo#107602)

    
    ==== Possible fixes ====

    igt@gem_exec_basic@readonly-blt:
      fi-byt-n2820:       FAIL (fdo#105900) -> PASS

    igt@kms_frontbuffer_tracking@basic:
      fi-hsw-peppy:       DMESG-FAIL (fdo#102614) -> PASS

    igt@prime_vgem@basic-fence-flip:
      fi-ivb-3770:        FAIL (fdo#104008) -> PASS

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
  fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
  fdo#105900 https://bugs.freedesktop.org/show_bug.cgi?id=105900
  fdo#107382 https://bugs.freedesktop.org/show_bug.cgi?id=107382
  fdo#107591 https://bugs.freedesktop.org/show_bug.cgi?id=107591
  fdo#107602 https://bugs.freedesktop.org/show_bug.cgi?id=107602


== Participating hosts (52 -> 49) ==

  Additional (1): fi-byt-j1900 
  Missing    (4): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4685 -> Patchwork_9971

  CI_DRM_4685: df7e8eddc3830216d3fec15e2c7d0b6ec97e7bae @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4606: 38a44003774e35c587c67c8766b35e75dbb993b8 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9971: 3f95a53c8ade0421b75908a68a0dad7a3e6d106c @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

3f95a53c8ade drm/i915: Verify power domains after enabling them

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9971/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: ✗ Fi.CI.BAT: failure for drm/i915: Verify power domains after enabling them (rev2)
  2018-08-17 13:44 ` ✗ Fi.CI.BAT: failure " Patchwork
@ 2018-08-17 13:56   ` Imre Deak
  2018-08-17 13:57   ` Chris Wilson
  1 sibling, 0 replies; 17+ messages in thread
From: Imre Deak @ 2018-08-17 13:56 UTC (permalink / raw)
  To: intel-gfx

On Fri, Aug 17, 2018 at 01:44:57PM +0000, Patchwork wrote:
> == Series Details ==
> 
> Series: drm/i915: Verify power domains after enabling them (rev2)
> URL   : https://patchwork.freedesktop.org/series/48394/
> State : failure
> 
> == Summary ==
> 
> = CI Bug Log - changes from CI_DRM_4685 -> Patchwork_9971 =
> 
> == Summary - FAILURE ==
> 
>   Serious unknown changes coming with Patchwork_9971 absolutely need to be
>   verified manually.
>   
>   If you think the reported changes have nothing to do with the changes
>   introduced in Patchwork_9971, please notify your bug team to allow them
>   to document this new failure mode, which will reduce false positives in CI.
> 
>   External URL: https://patchwork.freedesktop.org/api/1.0/series/48394/revisions/2/mbox/
> 
> == Possible new issues ==
> 
>   Here are the unknown changes that may have been introduced in Patchwork_9971:
> 
>   === IGT changes ===
> 
>     ==== Possible regressions ====
> 
>     igt@gem_exec_suspend@basic-s3:
>       fi-skl-6770hq:      PASS -> DMESG-WARN +3
>       {fi-cfl-8109u}:     PASS -> DMESG-WARN +3
>       fi-cfl-s3:          PASS -> DMESG-WARN +3
>       fi-skl-6260u:       PASS -> DMESG-WARN +3
>       fi-kbl-7567u:       PASS -> DMESG-WARN +3
>       fi-kbl-guc:         PASS -> DMESG-WARN
>       {fi-kbl-8809g}:     PASS -> DMESG-WARN

Err,
[  304.564287] [drm:intel_power_domains_verify_state [i915]] *ERROR* power well DC off state mismatch (refcount 0/enabled 1)

After deiniting display core/DMC firmware DC states will be disabled,
even though we don't hold a reference on the dc_off power well. Will fix
the check for that case in v3.

> 
>     igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
>       fi-whl-u:           PASS -> DMESG-WARN +3
>       fi-cfl-guc:         PASS -> DMESG-WARN +3
>       fi-glk-j4005:       PASS -> DMESG-WARN +3
>       fi-kbl-7500u:       PASS -> DMESG-WARN +3
>       fi-kbl-7560u:       PASS -> DMESG-WARN +3
> 
>     igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
>       {fi-skl-iommu}:     PASS -> DMESG-WARN +3
>       fi-skl-6700k2:      PASS -> DMESG-WARN +4
>       fi-skl-6700hq:      PASS -> DMESG-WARN +3
> 
>     igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
>       fi-glk-dsi:         PASS -> DMESG-WARN +3
>       fi-cfl-8700k:       PASS -> DMESG-WARN +3
>       fi-skl-guc:         PASS -> DMESG-WARN +3
>       fi-bxt-j4205:       PASS -> DMESG-WARN +3
>       fi-bxt-dsi:         PASS -> DMESG-WARN +3
>       fi-cnl-psr:         PASS -> DMESG-WARN +3
>       fi-skl-gvtdvm:      PASS -> DMESG-WARN +3
>       fi-skl-6600u:       PASS -> DMESG-WARN +3
> 
>     
>     ==== Warnings ====
> 
>     {igt@pm_rpm@module-reload}:
>       {fi-icl-u}:         WARN (fdo#107602) -> DMESG-FAIL
> 
>     
> == Known issues ==
> 
>   Here are the changes found in Patchwork_9971 that come from known issues:
> 
>   === IGT changes ===
> 
>     ==== Issues hit ====
> 
>     igt@drv_selftest@live_guc:
>       {fi-icl-u}:         PASS -> DMESG-WARN (fdo#107591) +14
> 
>     igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
>       {fi-icl-u}:         PASS -> DMESG-WARN (fdo#107382) +3
> 
>     {igt@pm_rpm@module-reload}:
>       fi-byt-j1900:       NOTRUN -> WARN (fdo#107602)
> 
>     
>     ==== Possible fixes ====
> 
>     igt@gem_exec_basic@readonly-blt:
>       fi-byt-n2820:       FAIL (fdo#105900) -> PASS
> 
>     igt@kms_frontbuffer_tracking@basic:
>       fi-hsw-peppy:       DMESG-FAIL (fdo#102614) -> PASS
> 
>     igt@prime_vgem@basic-fence-flip:
>       fi-ivb-3770:        FAIL (fdo#104008) -> PASS
> 
>     
>   {name}: This element is suppressed. This means it is ignored when computing
>           the status of the difference (SUCCESS, WARNING, or FAILURE).
> 
>   fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
>   fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
>   fdo#105900 https://bugs.freedesktop.org/show_bug.cgi?id=105900
>   fdo#107382 https://bugs.freedesktop.org/show_bug.cgi?id=107382
>   fdo#107591 https://bugs.freedesktop.org/show_bug.cgi?id=107591
>   fdo#107602 https://bugs.freedesktop.org/show_bug.cgi?id=107602
> 
> 
> == Participating hosts (52 -> 49) ==
> 
>   Additional (1): fi-byt-j1900 
>   Missing    (4): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-hsw-4200u 
> 
> 
> == Build changes ==
> 
>     * Linux: CI_DRM_4685 -> Patchwork_9971
> 
>   CI_DRM_4685: df7e8eddc3830216d3fec15e2c7d0b6ec97e7bae @ git://anongit.freedesktop.org/gfx-ci/linux
>   IGT_4606: 38a44003774e35c587c67c8766b35e75dbb993b8 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
>   Patchwork_9971: 3f95a53c8ade0421b75908a68a0dad7a3e6d106c @ git://anongit.freedesktop.org/gfx-ci/linux
> 
> 
> == Linux commits ==
> 
> 3f95a53c8ade drm/i915: Verify power domains after enabling them
> 
> == Logs ==
> 
> For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9971/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: ✗ Fi.CI.BAT: failure for drm/i915: Verify power domains after enabling them (rev2)
  2018-08-17 13:44 ` ✗ Fi.CI.BAT: failure " Patchwork
  2018-08-17 13:56   ` Imre Deak
@ 2018-08-17 13:57   ` Chris Wilson
  1 sibling, 0 replies; 17+ messages in thread
From: Chris Wilson @ 2018-08-17 13:57 UTC (permalink / raw)
  To: Imre Deak, Patchwork; +Cc: intel-gfx

Quoting Patchwork (2018-08-17 14:44:57)
> == Series Details ==
> 
> Series: drm/i915: Verify power domains after enabling them (rev2)
> URL   : https://patchwork.freedesktop.org/series/48394/
> State : failure
> 
> == Summary ==
> 
> = CI Bug Log - changes from CI_DRM_4685 -> Patchwork_9971 =
> 
> == Summary - FAILURE ==
> 
>   Serious unknown changes coming with Patchwork_9971 absolutely need to be
>   verified manually.
>   
>   If you think the reported changes have nothing to do with the changes
>   introduced in Patchwork_9971, please notify your bug team to allow them
>   to document this new failure mode, which will reduce false positives in CI.
> 
>   External URL: https://patchwork.freedesktop.org/api/1.0/series/48394/revisions/2/mbox/
> 
> == Possible new issues ==
> 
>   Here are the unknown changes that may have been introduced in Patchwork_9971:
> 
>   === IGT changes ===
> 
>     ==== Possible regressions ====
> 
>     igt@gem_exec_suspend@basic-s3:
>       fi-skl-6770hq:      PASS -> DMESG-WARN +3
>       {fi-cfl-8109u}:     PASS -> DMESG-WARN +3
>       fi-cfl-s3:          PASS -> DMESG-WARN +3
>       fi-skl-6260u:       PASS -> DMESG-WARN +3
>       fi-kbl-7567u:       PASS -> DMESG-WARN +3
>       fi-kbl-guc:         PASS -> DMESG-WARN
>       {fi-kbl-8809g}:     PASS -> DMESG-WARN

All CSR platforms. There's some floating state across suspend?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* [PATCH v3] drm/i915: Verify power domains after enabling them
  2018-08-17 13:18 ` [PATCH v2] " Imre Deak
  2018-08-17 13:25   ` Chris Wilson
@ 2018-08-17 14:58   ` Imre Deak
  2018-08-17 15:28     ` Chris Wilson
  1 sibling, 1 reply; 17+ messages in thread
From: Imre Deak @ 2018-08-17 14:58 UTC (permalink / raw)
  To: intel-gfx

After
commit 2cd9a689e97b ("drm/i915: Refactor intel_display_set_init_power() logic")
it makes more sense to check the power domain/well refcounts after
enabling the power domains functionality. Before that it's guaranteed
that most power wells (in the INIT domain) will have a reference held,
so not an interesting state.

While at it also add the check after the init_hw/fini_hw, disable and
suspend/resume steps. Make the test optional on a Kconfig option since
it may add substantial overhead: on VLV/CHV the corresponding PUNIT reg
access for each power well may take up to 20ms.

v2:
- Add the state check to more spots. (Chris)

v3:
- During suspend check the state before deiniting display core.
  Afterwards DC states are disabled (and so the dc_off power well is
  enabled) even though we don't hold a reference on it.
- Do the test conditionally based on a new Kconfig option. (Chris)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v2)
---
 drivers/gpu/drm/i915/Kconfig.debug      | 12 +++++++++++
 drivers/gpu/drm/i915/intel_display.c    |  2 --
 drivers/gpu/drm/i915/intel_drv.h        |  1 -
 drivers/gpu/drm/i915/intel_runtime_pm.c | 36 +++++++++++++++++++++++++++------
 4 files changed, 42 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug
index 459f8f88a34c..9e36ffb5eb7c 100644
--- a/drivers/gpu/drm/i915/Kconfig.debug
+++ b/drivers/gpu/drm/i915/Kconfig.debug
@@ -30,6 +30,7 @@ config DRM_I915_DEBUG
 	select SW_SYNC # signaling validation framework (igt/syncobj*)
 	select DRM_I915_SW_FENCE_DEBUG_OBJECTS
 	select DRM_I915_SELFTEST
+	select DRM_I915_DEBUG_RUNTIME_PM
         default n
         help
           Choose this option to turn on extra driver debugging that may affect
@@ -167,3 +168,14 @@ config DRM_I915_DEBUG_VBLANK_EVADE
 	  the vblank.
 
 	  If in doubt, say "N".
+
+config DRM_I915_DEBUG_RUNTIME_PM
+	bool "Enable extra state checking for runtime PM"
+	depends on DRM_I915
+	default n
+	help
+	  Choose this option to turn on extra state checking for the
+	  runtime PM functionality. This may introduce overhead during
+	  driver loading, suspend and resume operations.
+
+	  If in doubt, say "N"
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 09d62b0c62cb..ad0f0e5389d9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15907,8 +15907,6 @@ intel_modeset_setup_hw_state(struct drm_device *dev,
 
 	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
 
-	intel_power_domains_verify_state(dev_priv);
-
 	intel_fbc_init_pipe_state(dev_priv);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d0402051925b..529e72bcc5ef 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1966,7 +1966,6 @@ enum i915_drm_suspend_mode {
 void intel_power_domains_suspend(struct drm_i915_private *dev_priv,
 				 enum i915_drm_suspend_mode);
 void intel_power_domains_resume(struct drm_i915_private *dev_priv);
-void intel_power_domains_verify_state(struct drm_i915_private *dev_priv);
 void bxt_display_core_init(struct drm_i915_private *dev_priv, bool resume);
 void bxt_display_core_uninit(struct drm_i915_private *dev_priv);
 void intel_runtime_pm_enable(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 6153d5be5cf6..f1263f8f40ac 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -3716,6 +3716,8 @@ static void vlv_cmnlane_wa(struct drm_i915_private *dev_priv)
 	cmn->desc->ops->disable(dev_priv, cmn);
 }
 
+static void intel_power_domains_verify_state(struct drm_i915_private *dev_priv);
+
 /**
  * intel_power_domains_init_hw - initialize hardware power domain state
  * @dev_priv: i915 device instance
@@ -3767,6 +3769,8 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv, bool resume)
 		intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
 	intel_power_domains_sync_hw(dev_priv);
 	power_domains->initializing = false;
+
+	intel_power_domains_verify_state(dev_priv);
 }
 
 /**
@@ -3788,6 +3792,8 @@ void intel_power_domains_fini_hw(struct drm_i915_private *dev_priv)
 	/* Remove the refcount we took to keep power well support disabled. */
 	if (!i915_modparams.disable_power_well)
 		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
+
+	intel_power_domains_verify_state(dev_priv);
 }
 
 /**
@@ -3805,6 +3811,8 @@ void intel_power_domains_fini_hw(struct drm_i915_private *dev_priv)
 void intel_power_domains_enable(struct drm_i915_private *dev_priv)
 {
 	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
+
+	intel_power_domains_verify_state(dev_priv);
 }
 
 /**
@@ -3817,6 +3825,8 @@ void intel_power_domains_enable(struct drm_i915_private *dev_priv)
 void intel_power_domains_disable(struct drm_i915_private *dev_priv)
 {
 	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
+
+	intel_power_domains_verify_state(dev_priv);
 }
 
 /**
@@ -3845,15 +3855,19 @@ void intel_power_domains_suspend(struct drm_i915_private *dev_priv,
 	 * firmware was inactive.
 	 */
 	if (!IS_GEN9_LP(dev_priv) && suspend_mode == I915_DRM_SUSPEND_IDLE &&
-	    dev_priv->csr.dmc_payload != NULL)
+	    dev_priv->csr.dmc_payload != NULL) {
+		intel_power_domains_verify_state(dev_priv);
 		return;
+	}
 
 	/*
 	 * Even if power well support was disabled we still want to disable
 	 * power wells if power domains must be deinitialized for suspend.
 	 */
-	if (!i915_modparams.disable_power_well)
+	if (!i915_modparams.disable_power_well) {
 		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
+		intel_power_domains_verify_state(dev_priv);
+	}
 
 	if (IS_ICELAKE(dev_priv))
 		icl_display_core_uninit(dev_priv);
@@ -3884,11 +3898,11 @@ void intel_power_domains_resume(struct drm_i915_private *dev_priv)
 	if (power_domains->display_core_suspended) {
 		intel_power_domains_init_hw(dev_priv, true);
 		power_domains->display_core_suspended = false;
-
-		return;
+	} else {
+		intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
 	}
 
-	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
+	intel_power_domains_verify_state(dev_priv);
 }
 
 static void intel_power_domains_dump_info(struct drm_i915_private *dev_priv)
@@ -3919,7 +3933,9 @@ static void intel_power_domains_dump_info(struct drm_i915_private *dev_priv)
  * acquiring reference counts for any power wells in use and disabling the
  * ones left on by BIOS but not required by any active output.
  */
-void intel_power_domains_verify_state(struct drm_i915_private *dev_priv)
+#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
+
+static void intel_power_domains_verify_state(struct drm_i915_private *dev_priv)
 {
 	struct i915_power_domains *power_domains = &dev_priv->power_domains;
 	struct i915_power_well *power_well;
@@ -3974,6 +3990,14 @@ void intel_power_domains_verify_state(struct drm_i915_private *dev_priv)
 	mutex_unlock(&power_domains->lock);
 }
 
+#else
+
+static void intel_power_domains_verify_state(struct drm_i915_private *dev_priv)
+{
+}
+
+#endif
+
 /**
  * intel_runtime_pm_get - grab a runtime pm reference
  * @dev_priv: i915 device instance
-- 
2.13.2

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

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

* ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Verify power domains after enabling them (rev3)
  2018-08-17 12:26 [PATCH] drm/i915: Verify power domains after enabling them Imre Deak
                   ` (5 preceding siblings ...)
  2018-08-17 13:44 ` ✗ Fi.CI.BAT: failure " Patchwork
@ 2018-08-17 15:07 ` Patchwork
  2018-08-17 15:24 ` ✓ Fi.CI.BAT: success " Patchwork
  7 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2018-08-17 15:07 UTC (permalink / raw)
  To: Imre Deak; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Verify power domains after enabling them (rev3)
URL   : https://patchwork.freedesktop.org/series/48394/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
c7bd0b63b579 drm/i915: Verify power domains after enabling them
-:7: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#7: 
commit 2cd9a689e97b ("drm/i915: Refactor intel_display_set_init_power() logic")

-:137: CHECK:COMPARISON_TO_NULL: Comparison to NULL could be written "dev_priv->csr.dmc_payload"
#137: FILE: drivers/gpu/drm/i915/intel_runtime_pm.c:3858:
+	    dev_priv->csr.dmc_payload != NULL) {

total: 0 errors, 1 warnings, 1 checks, 135 lines checked

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

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

* ✓ Fi.CI.BAT: success for drm/i915: Verify power domains after enabling them (rev3)
  2018-08-17 12:26 [PATCH] drm/i915: Verify power domains after enabling them Imre Deak
                   ` (6 preceding siblings ...)
  2018-08-17 15:07 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Verify power domains after enabling them (rev3) Patchwork
@ 2018-08-17 15:24 ` Patchwork
  2018-08-20  9:20   ` Imre Deak
  7 siblings, 1 reply; 17+ messages in thread
From: Patchwork @ 2018-08-17 15:24 UTC (permalink / raw)
  To: Imre Deak; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Verify power domains after enabling them (rev3)
URL   : https://patchwork.freedesktop.org/series/48394/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4685 -> Patchwork_9973 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/48394/revisions/3/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9973:

  === IGT changes ===

    ==== Warnings ====

    {igt@pm_rpm@module-reload}:
      {fi-icl-u}:         WARN (fdo#107602) -> DMESG-FAIL

    
== Known issues ==

  Here are the changes found in Patchwork_9973 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_guc:
      {fi-icl-u}:         PASS -> DMESG-WARN (fdo#107591) +14

    igt@drv_selftest@live_hangcheck:
      fi-skl-6600u:       PASS -> DMESG-FAIL (fdo#106560, fdo#107174)

    igt@gem_exec_suspend@basic-s4-devices:
      fi-kbl-7500u:       PASS -> DMESG-WARN (fdo#107139, fdo#105128)

    igt@kms_frontbuffer_tracking@basic:
      {fi-byt-clapper}:   PASS -> FAIL (fdo#103167)

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a:
      {fi-byt-clapper}:   PASS -> FAIL (fdo#107362)

    {igt@pm_rpm@module-reload}:
      fi-byt-j1900:       NOTRUN -> WARN (fdo#107602)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_hangcheck:
      fi-skl-guc:         DMESG-FAIL (fdo#107174) -> PASS

    igt@gem_exec_basic@readonly-blt:
      fi-byt-n2820:       FAIL (fdo#105900) -> PASS

    igt@kms_frontbuffer_tracking@basic:
      fi-hsw-peppy:       DMESG-FAIL (fdo#102614) -> PASS

    igt@prime_vgem@basic-fence-flip:
      fi-ivb-3770:        FAIL (fdo#104008) -> PASS

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
  fdo#105128 https://bugs.freedesktop.org/show_bug.cgi?id=105128
  fdo#105900 https://bugs.freedesktop.org/show_bug.cgi?id=105900
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  fdo#107139 https://bugs.freedesktop.org/show_bug.cgi?id=107139
  fdo#107174 https://bugs.freedesktop.org/show_bug.cgi?id=107174
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107591 https://bugs.freedesktop.org/show_bug.cgi?id=107591
  fdo#107602 https://bugs.freedesktop.org/show_bug.cgi?id=107602


== Participating hosts (52 -> 49) ==

  Additional (1): fi-byt-j1900 
  Missing    (4): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4685 -> Patchwork_9973

  CI_DRM_4685: df7e8eddc3830216d3fec15e2c7d0b6ec97e7bae @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4606: 38a44003774e35c587c67c8766b35e75dbb993b8 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9973: c7bd0b63b5794846f480ad271ce92866cb41c5eb @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

c7bd0b63b579 drm/i915: Verify power domains after enabling them

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9973/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH v3] drm/i915: Verify power domains after enabling them
  2018-08-17 14:58   ` [PATCH v3] " Imre Deak
@ 2018-08-17 15:28     ` Chris Wilson
  0 siblings, 0 replies; 17+ messages in thread
From: Chris Wilson @ 2018-08-17 15:28 UTC (permalink / raw)
  To: Imre Deak, intel-gfx

Quoting Imre Deak (2018-08-17 15:58:37)
> After
> commit 2cd9a689e97b ("drm/i915: Refactor intel_display_set_init_power() logic")
> it makes more sense to check the power domain/well refcounts after
> enabling the power domains functionality. Before that it's guaranteed
> that most power wells (in the INIT domain) will have a reference held,
> so not an interesting state.
> 
> While at it also add the check after the init_hw/fini_hw, disable and
> suspend/resume steps. Make the test optional on a Kconfig option since
> it may add substantial overhead: on VLV/CHV the corresponding PUNIT reg
> access for each power well may take up to 20ms.
> 
> v2:
> - Add the state check to more spots. (Chris)
> 
> v3:
> - During suspend check the state before deiniting display core.
>   Afterwards DC states are disabled (and so the dc_off power well is
>   enabled) even though we don't hold a reference on it.
> - Do the test conditionally based on a new Kconfig option. (Chris)
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v2)

After cowardly waiting for CI to confirm the suspend state test was in
the right spot,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Could you add

@@ -1318,6 +1319,8 @@  static void i915_welcome_messages(struct drm_i915_private *dev_priv)
 		DRM_INFO("DRM_I915_DEBUG enabled\n");
 	if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
 		DRM_INFO("DRM_I915_DEBUG_GEM enabled\n");
+	if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM))
+		DRM_INFO("DRM_I915_DEBUG_RUNTIME_PM enabled\n");
 }

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

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

* Re: ✓ Fi.CI.BAT: success for drm/i915: Verify power domains after enabling them (rev3)
  2018-08-17 15:24 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2018-08-20  9:20   ` Imre Deak
  0 siblings, 0 replies; 17+ messages in thread
From: Imre Deak @ 2018-08-20  9:20 UTC (permalink / raw)
  To: intel-gfx, Chris Wilson

On Fri, Aug 17, 2018 at 03:24:40PM +0000, Patchwork wrote:
> == Series Details ==
> 
> Series: drm/i915: Verify power domains after enabling them (rev3)
> URL   : https://patchwork.freedesktop.org/series/48394/
> State : success

Pushed to -dinq with i915_welcome_messages() updated, thanks for the
review.

> 
> == Summary ==
> 
> = CI Bug Log - changes from CI_DRM_4685 -> Patchwork_9973 =
> 
> == Summary - SUCCESS ==
> 
>   No regressions found.
> 
>   External URL: https://patchwork.freedesktop.org/api/1.0/series/48394/revisions/3/mbox/
> 
> == Possible new issues ==
> 
>   Here are the unknown changes that may have been introduced in Patchwork_9973:
> 
>   === IGT changes ===
> 
>     ==== Warnings ====
> 
>     {igt@pm_rpm@module-reload}:
>       {fi-icl-u}:         WARN (fdo#107602) -> DMESG-FAIL
> 
>     
> == Known issues ==
> 
>   Here are the changes found in Patchwork_9973 that come from known issues:
> 
>   === IGT changes ===
> 
>     ==== Issues hit ====
> 
>     igt@drv_selftest@live_guc:
>       {fi-icl-u}:         PASS -> DMESG-WARN (fdo#107591) +14
> 
>     igt@drv_selftest@live_hangcheck:
>       fi-skl-6600u:       PASS -> DMESG-FAIL (fdo#106560, fdo#107174)
> 
>     igt@gem_exec_suspend@basic-s4-devices:
>       fi-kbl-7500u:       PASS -> DMESG-WARN (fdo#107139, fdo#105128)
> 
>     igt@kms_frontbuffer_tracking@basic:
>       {fi-byt-clapper}:   PASS -> FAIL (fdo#103167)
> 
>     igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a:
>       {fi-byt-clapper}:   PASS -> FAIL (fdo#107362)
> 
>     {igt@pm_rpm@module-reload}:
>       fi-byt-j1900:       NOTRUN -> WARN (fdo#107602)
> 
>     
>     ==== Possible fixes ====
> 
>     igt@drv_selftest@live_hangcheck:
>       fi-skl-guc:         DMESG-FAIL (fdo#107174) -> PASS
> 
>     igt@gem_exec_basic@readonly-blt:
>       fi-byt-n2820:       FAIL (fdo#105900) -> PASS
> 
>     igt@kms_frontbuffer_tracking@basic:
>       fi-hsw-peppy:       DMESG-FAIL (fdo#102614) -> PASS
> 
>     igt@prime_vgem@basic-fence-flip:
>       fi-ivb-3770:        FAIL (fdo#104008) -> PASS
> 
>     
>   {name}: This element is suppressed. This means it is ignored when computing
>           the status of the difference (SUCCESS, WARNING, or FAILURE).
> 
>   fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
>   fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
>   fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
>   fdo#105128 https://bugs.freedesktop.org/show_bug.cgi?id=105128
>   fdo#105900 https://bugs.freedesktop.org/show_bug.cgi?id=105900
>   fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
>   fdo#107139 https://bugs.freedesktop.org/show_bug.cgi?id=107139
>   fdo#107174 https://bugs.freedesktop.org/show_bug.cgi?id=107174
>   fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
>   fdo#107591 https://bugs.freedesktop.org/show_bug.cgi?id=107591
>   fdo#107602 https://bugs.freedesktop.org/show_bug.cgi?id=107602
> 
> 
> == Participating hosts (52 -> 49) ==
> 
>   Additional (1): fi-byt-j1900 
>   Missing    (4): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-hsw-4200u 
> 
> 
> == Build changes ==
> 
>     * Linux: CI_DRM_4685 -> Patchwork_9973
> 
>   CI_DRM_4685: df7e8eddc3830216d3fec15e2c7d0b6ec97e7bae @ git://anongit.freedesktop.org/gfx-ci/linux
>   IGT_4606: 38a44003774e35c587c67c8766b35e75dbb993b8 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
>   Patchwork_9973: c7bd0b63b5794846f480ad271ce92866cb41c5eb @ git://anongit.freedesktop.org/gfx-ci/linux
> 
> 
> == Linux commits ==
> 
> c7bd0b63b579 drm/i915: Verify power domains after enabling them
> 
> == Logs ==
> 
> For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9973/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

end of thread, other threads:[~2018-08-20  9:21 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-17 12:26 [PATCH] drm/i915: Verify power domains after enabling them Imre Deak
2018-08-17 12:32 ` Chris Wilson
2018-08-17 12:53   ` Imre Deak
2018-08-17 12:58     ` Chris Wilson
2018-08-17 12:51 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
2018-08-17 13:09 ` ✓ Fi.CI.BAT: success " Patchwork
2018-08-17 13:18 ` [PATCH v2] " Imre Deak
2018-08-17 13:25   ` Chris Wilson
2018-08-17 14:58   ` [PATCH v3] " Imre Deak
2018-08-17 15:28     ` Chris Wilson
2018-08-17 13:27 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Verify power domains after enabling them (rev2) Patchwork
2018-08-17 13:44 ` ✗ Fi.CI.BAT: failure " Patchwork
2018-08-17 13:56   ` Imre Deak
2018-08-17 13:57   ` Chris Wilson
2018-08-17 15:07 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Verify power domains after enabling them (rev3) Patchwork
2018-08-17 15:24 ` ✓ Fi.CI.BAT: success " Patchwork
2018-08-20  9:20   ` Imre Deak

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.