diff mbox series

[v3] drm/i915: Verify power domains after enabling them

Message ID 20180817145837.26592-1-imre.deak@intel.com (mailing list archive)
State New, archived
Headers show
Series [v3] drm/i915: Verify power domains after enabling them | expand

Commit Message

Imre Deak Aug. 17, 2018, 2:58 p.m. UTC
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(-)

Comments

Chris Wilson Aug. 17, 2018, 3:28 p.m. UTC | #1
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
diff mbox series

Patch

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