[PATCH 1/2] drm/i915: Introduce intel_runtime_pm_disable to pair intel_runtime_pm_enable

[PATCH 1/2] drm/i915: Introduce intel_runtime_pm_disable to pair intel_runtime_pm_enable

[Imre Deak]

From: Chris Wilson <chris@chris-wilson.co.uk>

Currently, we cancel the extra wakeref we have for !runtime-pm devices
inside power_wells_fini_hw. However, this is not strictly paired with
the acquisition of that wakeref in runtime_pm_enable (as the fini_hw may
be called on errors paths before we even call runtime_pm_enable). Make
the symmetry more explicit and include a check that we do release all of
our rpm wakerefs.

v2: Fixup transfer of ownership back to core whilst keeping our wakeref
count balanced.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Imre Deak <imre.deak@intel.com>
Reviewed-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c         | 27 ++++++++-------------
 drivers/gpu/drm/i915/intel_drv.h        |  1 +
 drivers/gpu/drm/i915/intel_runtime_pm.c | 43 ++++++++++++++++++++++-----------
 3 files changed, 40 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 41111f2a9c39..021304e252eb 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1281,6 +1281,8 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
 	 */
 	if (INTEL_INFO(dev_priv)->num_pipes)
 		drm_kms_helper_poll_init(dev);
+
+	intel_runtime_pm_enable(dev_priv);
 }
 
 /**
@@ -1289,6 +1291,8 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
  */
 static void i915_driver_unregister(struct drm_i915_private *dev_priv)
 {
+	intel_runtime_pm_disable(dev_priv);
+
 	intel_fbdev_unregister(dev_priv);
 	intel_audio_deinit(dev_priv);
 
@@ -1366,16 +1370,6 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
 		goto out_fini;
 
 	pci_set_drvdata(pdev, &dev_priv->drm);
-	/*
-	 * Disable the system suspend direct complete optimization, which can
-	 * leave the device suspended skipping the driver's suspend handlers
-	 * if the device was already runtime suspended. This is needed due to
-	 * the difference in our runtime and system suspend sequence and
-	 * becaue the HDA driver may require us to enable the audio power
-	 * domain during system suspend.
-	 */
-	dev_pm_set_driver_flags(&pdev->dev, DPM_FLAG_NEVER_SKIP);
-
 	ret = i915_driver_init_early(dev_priv, ent);
 	if (ret < 0)
 		goto out_pci_disable;
@@ -1408,8 +1402,6 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	i915_driver_register(dev_priv);
 
-	intel_runtime_pm_enable(dev_priv);
-
 	intel_init_ipc(dev_priv);
 
 	intel_runtime_pm_put(dev_priv);
@@ -1441,13 +1433,13 @@ void i915_driver_unload(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct pci_dev *pdev = dev_priv->drm.pdev;
 
-	i915_driver_unregister(dev_priv);
-
-	if (i915_gem_suspend(dev_priv))
-		DRM_ERROR("failed to idle hardware; continuing to unload!\n");
-
 	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
 
+	i915_driver_unregister(dev_priv);
+
+	if (i915_gem_suspend(dev_priv))
+		DRM_ERROR("failed to idle hardware; continuing to unload!\n");
+
 	drm_atomic_helper_shutdown(dev);
 
 	intel_gvt_cleanup(dev_priv);
@@ -1474,6 +1466,7 @@ void i915_driver_unload(struct drm_device *dev)
 	i915_driver_cleanup_mmio(dev_priv);
 
 	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
+	WARN_ON(atomic_read(&dev_priv->runtime_pm.wakeref_count));
 }
 
 static void i915_driver_release(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 7b984aefce98..3f012716f2dc 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1959,6 +1959,7 @@ 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);
+void intel_runtime_pm_disable(struct drm_i915_private *dev_priv);
 const char *
 intel_display_power_domain_str(enum intel_display_power_domain domain);
 
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index e209edbc561d..c0983f0e46ac 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -3793,29 +3793,19 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv, bool resume)
  */
 void intel_power_domains_fini_hw(struct drm_i915_private *dev_priv)
 {
-	struct device *kdev = &dev_priv->drm.pdev->dev;
-
 	/*
 	 * The i915.ko module is still not prepared to be loaded when
 	 * the power well is not enabled, so just enable it in case
 	 * we're going to unload/reload.
-	 * The following also reacquires the RPM reference the core passed
-	 * to the driver during loading, which is dropped in
-	 * intel_runtime_pm_enable(). We have to hand back the control of the
-	 * device to the core with this reference held.
 	 */
-	intel_display_set_init_power(dev_priv, true);
+	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
+
+	/* Keep the power well enabled, but cancel its rpm wakeref. */
+	intel_runtime_pm_put(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);
-
-	/*
-	 * Remove the refcount we took in intel_runtime_pm_enable() in case
-	 * the platform doesn't support runtime PM.
-	 */
-	if (!HAS_RUNTIME_PM(dev_priv))
-		pm_runtime_put(kdev);
 }
 
 /**
@@ -4048,6 +4038,16 @@ void intel_runtime_pm_enable(struct drm_i915_private *dev_priv)
 	struct pci_dev *pdev = dev_priv->drm.pdev;
 	struct device *kdev = &pdev->dev;
 
+	/*
+	 * Disable the system suspend direct complete optimization, which can
+	 * leave the device suspended skipping the driver's suspend handlers
+	 * if the device was already runtime suspended. This is needed due to
+	 * the difference in our runtime and system suspend sequence and
+	 * becaue the HDA driver may require us to enable the audio power
+	 * domain during system suspend.
+	 */
+	dev_pm_set_driver_flags(kdev, DPM_FLAG_NEVER_SKIP);
+
 	pm_runtime_set_autosuspend_delay(kdev, 10000); /* 10s */
 	pm_runtime_mark_last_busy(kdev);
 
@@ -4074,3 +4074,18 @@ void intel_runtime_pm_enable(struct drm_i915_private *dev_priv)
 	 */
 	pm_runtime_put_autosuspend(kdev);
 }
+
+void intel_runtime_pm_disable(struct drm_i915_private *dev_priv)
+{
+	struct pci_dev *pdev = dev_priv->drm.pdev;
+	struct device *kdev = &pdev->dev;
+
+	/* Transfer rpm ownership back to core */
+	WARN(pm_runtime_get_sync(&dev_priv->drm.pdev->dev) < 0,
+	     "Failed to pass rpm ownership back to core\n");
+
+	pm_runtime_dont_use_autosuspend(kdev);
+
+	if (!HAS_RUNTIME_PM(dev_priv))
+		pm_runtime_put(kdev);
+}
-- 
2.13.2

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

[PATCH 2/2] drm/i915: Refactor intel_display_set_init_power() logic

[Imre Deak]

The device global init_power_on flag is somewhat arbitrary and makes
debugging power refcounting problems difficult. Instead arrange things
so that all display power domain get has a corresponding put call. After
this change we have the following sequences:

driver loading:
intel_power_domains_init_hw();
<other init steps>
intel_power_domains_enable();

driver unloading:
intel_power_domains_disable();
<other uninit steps>
intel_power_domains_fini_hw();

system suspend:
intel_power_domains_disable();
<other suspend steps>
intel_power_domains_suspend();

system resume:
intel_power_domains_resume();
<other resume steps>
intel_power_domains_enable();

at other times while the driver is loaded:
intel_display_power_get();
...
intel_display_power_put();

Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c         |  65 +++++++---------
 drivers/gpu/drm/i915/i915_drv.h         |   2 +-
 drivers/gpu/drm/i915/intel_display.c    |   5 +-
 drivers/gpu/drm/i915/intel_drv.h        |  15 +++-
 drivers/gpu/drm/i915/intel_runtime_pm.c | 133 +++++++++++++++++++++++---------
 5 files changed, 142 insertions(+), 78 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 021304e252eb..35a012ffc03b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1282,6 +1282,7 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
 	if (INTEL_INFO(dev_priv)->num_pipes)
 		drm_kms_helper_poll_init(dev);
 
+	intel_power_domains_enable(dev_priv);
 	intel_runtime_pm_enable(dev_priv);
 }
 
@@ -1292,6 +1293,7 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
 static void i915_driver_unregister(struct drm_i915_private *dev_priv)
 {
 	intel_runtime_pm_disable(dev_priv);
+	intel_power_domains_disable(dev_priv);
 
 	intel_fbdev_unregister(dev_priv);
 	intel_audio_deinit(dev_priv);
@@ -1374,7 +1376,7 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (ret < 0)
 		goto out_pci_disable;
 
-	intel_runtime_pm_get(dev_priv);
+	disable_rpm_wakeref_asserts(dev_priv);
 
 	ret = i915_driver_init_mmio(dev_priv);
 	if (ret < 0)
@@ -1404,7 +1406,7 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	intel_init_ipc(dev_priv);
 
-	intel_runtime_pm_put(dev_priv);
+	enable_rpm_wakeref_asserts(dev_priv);
 
 	i915_welcome_messages(dev_priv);
 
@@ -1415,7 +1417,7 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
 out_cleanup_mmio:
 	i915_driver_cleanup_mmio(dev_priv);
 out_runtime_pm_put:
-	intel_runtime_pm_put(dev_priv);
+	enable_rpm_wakeref_asserts(dev_priv);
 	i915_driver_cleanup_early(dev_priv);
 out_pci_disable:
 	pci_disable_device(pdev);
@@ -1433,7 +1435,7 @@ void i915_driver_unload(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct pci_dev *pdev = dev_priv->drm.pdev;
 
-	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
+	disable_rpm_wakeref_asserts(dev_priv);
 
 	i915_driver_unregister(dev_priv);
 
@@ -1465,7 +1467,8 @@ void i915_driver_unload(struct drm_device *dev)
 	i915_driver_cleanup_hw(dev_priv);
 	i915_driver_cleanup_mmio(dev_priv);
 
-	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
+	enable_rpm_wakeref_asserts(dev_priv);
+
 	WARN_ON(atomic_read(&dev_priv->runtime_pm.wakeref_count));
 }
 
@@ -1575,7 +1578,7 @@ static int i915_drm_suspend(struct drm_device *dev)
 
 	/* We do a lot of poking in a lot of registers, make sure they work
 	 * properly. */
-	intel_display_set_init_power(dev_priv, true);
+	intel_power_domains_disable(dev_priv);
 
 	drm_kms_helper_poll_disable(dev);
 
@@ -1612,6 +1615,18 @@ static int i915_drm_suspend(struct drm_device *dev)
 	return 0;
 }
 
+static enum i915_drm_suspend_mode
+get_suspend_mode(struct drm_i915_private *dev_priv, bool hibernate)
+{
+	if (hibernate)
+		return I915_DRM_SUSPEND_HIBERNATE;
+
+	if (suspend_to_idle(dev_priv))
+		return I915_DRM_SUSPEND_IDLE;
+
+	return I915_DRM_SUSPEND_MEM;
+}
+
 static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
@@ -1622,21 +1637,10 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
 
 	i915_gem_suspend_late(dev_priv);
 
-	intel_display_set_init_power(dev_priv, false);
 	intel_uncore_suspend(dev_priv);
 
-	/*
-	 * In case of firmware assisted context save/restore don't manually
-	 * deinit the power domains. This also means the CSR/DMC firmware will
-	 * stay active, it will power down any HW resources as required and
-	 * also enable deeper system power states that would be blocked if the
-	 * firmware was inactive.
-	 */
-	if (IS_GEN9_LP(dev_priv) || hibernation || !suspend_to_idle(dev_priv) ||
-	    dev_priv->csr.dmc_payload == NULL) {
-		intel_power_domains_suspend(dev_priv);
-		dev_priv->power_domains_suspended = true;
-	}
+	intel_power_domains_suspend(dev_priv,
+				    get_suspend_mode(dev_priv, hibernation));
 
 	ret = 0;
 	if (IS_GEN9_LP(dev_priv))
@@ -1648,10 +1652,7 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
 
 	if (ret) {
 		DRM_ERROR("Suspend complete failed: %d\n", ret);
-		if (dev_priv->power_domains_suspended) {
-			intel_power_domains_init_hw(dev_priv, true);
-			dev_priv->power_domains_suspended = false;
-		}
+		intel_power_domains_resume(dev_priv);
 
 		goto out;
 	}
@@ -1768,6 +1769,8 @@ static int i915_drm_resume(struct drm_device *dev)
 
 	intel_opregion_notify_adapter(dev_priv, PCI_D0);
 
+	intel_power_domains_enable(dev_priv);
+
 	enable_rpm_wakeref_asserts(dev_priv);
 
 	return 0;
@@ -1802,7 +1805,7 @@ static int i915_drm_resume_early(struct drm_device *dev)
 	ret = pci_set_power_state(pdev, PCI_D0);
 	if (ret) {
 		DRM_ERROR("failed to set PCI D0 power state (%d)\n", ret);
-		goto out;
+		return ret;
 	}
 
 	/*
@@ -1818,10 +1821,8 @@ static int i915_drm_resume_early(struct drm_device *dev)
 	 * depend on the device enable refcount we can't anyway depend on them
 	 * disabling/enabling the device.
 	 */
-	if (pci_enable_device(pdev)) {
-		ret = -EIO;
-		goto out;
-	}
+	if (pci_enable_device(pdev))
+		return -EIO;
 
 	pci_set_master(pdev);
 
@@ -1844,18 +1845,12 @@ static int i915_drm_resume_early(struct drm_device *dev)
 
 	intel_uncore_sanitize(dev_priv);
 
-	if (dev_priv->power_domains_suspended)
-		intel_power_domains_init_hw(dev_priv, true);
-	else
-		intel_display_set_init_power(dev_priv, true);
+	intel_power_domains_resume(dev_priv);
 
 	intel_engines_sanitize(dev_priv);
 
 	enable_rpm_wakeref_asserts(dev_priv);
 
-out:
-	dev_priv->power_domains_suspended = false;
-
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5fa13887b911..74482753a04e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -935,8 +935,8 @@ struct i915_power_domains {
 	 * Power wells needed for initialization at driver init and suspend
 	 * time are on. They are kept on until after the first modeset.
 	 */
-	bool init_power_on;
 	bool initializing;
+	bool display_core_suspended;
 	int power_well_count;
 
 	struct mutex lock;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 690e1e816e77..09d62b0c62cb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15848,6 +15848,8 @@ intel_modeset_setup_hw_state(struct drm_device *dev,
 	struct intel_encoder *encoder;
 	int i;
 
+	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
+
 	intel_early_display_was(dev_priv);
 	intel_modeset_readout_hw_state(dev);
 
@@ -15902,7 +15904,8 @@ intel_modeset_setup_hw_state(struct drm_device *dev,
 		if (WARN_ON(put_domains))
 			modeset_put_power_domains(dev_priv, put_domains);
 	}
-	intel_display_set_init_power(dev_priv, false);
+
+	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
 
 	intel_power_domains_verify_state(dev_priv);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3f012716f2dc..d0402051925b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1954,7 +1954,18 @@ int intel_power_domains_init(struct drm_i915_private *);
 void intel_power_domains_cleanup(struct drm_i915_private *dev_priv);
 void intel_power_domains_init_hw(struct drm_i915_private *dev_priv, bool resume);
 void intel_power_domains_fini_hw(struct drm_i915_private *dev_priv);
-void intel_power_domains_suspend(struct drm_i915_private *dev_priv);
+void intel_power_domains_enable(struct drm_i915_private *dev_priv);
+void intel_power_domains_disable(struct drm_i915_private *dev_priv);
+
+enum i915_drm_suspend_mode {
+	I915_DRM_SUSPEND_IDLE,
+	I915_DRM_SUSPEND_MEM,
+	I915_DRM_SUSPEND_HIBERNATE,
+};
+
+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);
@@ -2037,8 +2048,6 @@ bool intel_runtime_pm_get_if_in_use(struct drm_i915_private *dev_priv);
 void intel_runtime_pm_get_noresume(struct drm_i915_private *dev_priv);
 void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
 
-void intel_display_set_init_power(struct drm_i915_private *dev, bool enable);
-
 void chv_phy_powergate_lanes(struct intel_encoder *encoder,
 			     bool override, unsigned int mask);
 bool chv_phy_powergate_ch(struct drm_i915_private *dev_priv, enum dpio_phy phy,
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index c0983f0e46ac..6153d5be5cf6 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -257,30 +257,6 @@ bool intel_display_power_is_enabled(struct drm_i915_private *dev_priv,
 	return ret;
 }
 
-/**
- * intel_display_set_init_power - set the initial power domain state
- * @dev_priv: i915 device instance
- * @enable: whether to enable or disable the initial power domain state
- *
- * For simplicity our driver load/unload and system suspend/resume code assumes
- * that all power domains are always enabled. This functions controls the state
- * of this little hack. While the initial power domain state is enabled runtime
- * pm is effectively disabled.
- */
-void intel_display_set_init_power(struct drm_i915_private *dev_priv,
-				  bool enable)
-{
-	if (dev_priv->power_domains.init_power_on == enable)
-		return;
-
-	if (enable)
-		intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
-	else
-		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
-
-	dev_priv->power_domains.init_power_on = enable;
-}
-
 /*
  * Starting with Haswell, we have a "Power Down Well" that can be turned off
  * when not needed anymore. We have 4 registers that can request the power well
@@ -3750,6 +3726,10 @@ static void vlv_cmnlane_wa(struct drm_i915_private *dev_priv)
  * domains (and not in the INIT domain) are referenced or disabled during the
  * modeset state HW readout. After that the reference count of each power well
  * must match its HW enabled state, see intel_power_domains_verify_state().
+ *
+ * It will return with power domains disabled (to be enabled later by
+ * intel_power_domains_enable()) and must be paired with
+ * intel_power_domains_fini_hw().
  */
 void intel_power_domains_init_hw(struct drm_i915_private *dev_priv, bool resume)
 {
@@ -3775,8 +3755,13 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv, bool resume)
 		mutex_unlock(&power_domains->lock);
 	}
 
-	/* For now, we need the power well to be always enabled. */
-	intel_display_set_init_power(dev_priv, true);
+	/*
+	 * Keep all power wells enabled for any dependent HW access during
+	 * initialization and to make sure we keep BIOS enabled display HW
+	 * resources powered until display HW readout is complete. We drop
+	 * this reference in intel_power_domains_enable().
+	 */
+	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
 	/* Disable power support if the user asked so. */
 	if (!i915_modparams.disable_power_well)
 		intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
@@ -3790,16 +3775,13 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv, bool resume)
  *
  * De-initializes the display power domain HW state. It also ensures that the
  * device stays powered up so that the driver can be reloaded.
+ *
+ * It must be called with power domains already disabled (after a call to
+ * intel_power_domains_disable()) and must be paired with
+ * intel_power_domains_init_hw().
  */
 void intel_power_domains_fini_hw(struct drm_i915_private *dev_priv)
 {
-	/*
-	 * The i915.ko module is still not prepared to be loaded when
-	 * the power well is not enabled, so just enable it in case
-	 * we're going to unload/reload.
-	 */
-	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
-
 	/* Keep the power well enabled, but cancel its rpm wakeref. */
 	intel_runtime_pm_put(dev_priv);
 
@@ -3809,17 +3791,66 @@ void intel_power_domains_fini_hw(struct drm_i915_private *dev_priv)
 }
 
 /**
+ * intel_power_domains_enable - enable toggling of display power wells
+ * @dev_priv: i915 device instance
+ *
+ * Enable the ondemand enabling/disabling of the display power wells. Note that
+ * power wells not belonging to POWER_DOMAIN_INIT are allowed to be toggled
+ * only at specific points of the display modeset sequence, thus they are not
+ * affected by the intel_power_domains_enable()/disable() calls. The purpose
+ * of these function is to keep the rest of power wells enabled until the end
+ * of display HW readout (which will acquire the power references reflecting
+ * the current HW state).
+ */
+void intel_power_domains_enable(struct drm_i915_private *dev_priv)
+{
+	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
+}
+
+/**
+ * intel_power_domains_disable - disable toggling of display power wells
+ * @dev_priv: i915 device instance
+ *
+ * Disable the ondemand enabling/disabling of the display power wells. See
+ * intel_power_domains_enable() for which power wells this call controls.
+ */
+void intel_power_domains_disable(struct drm_i915_private *dev_priv)
+{
+	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
+}
+
+/**
  * intel_power_domains_suspend - suspend power domain state
  * @dev_priv: i915 device instance
+ * @suspend_mode: specifies the target suspend state (idle, mem, hibernation)
  *
  * This function prepares the hardware power domain state before entering
- * system suspend. It must be paired with intel_power_domains_init_hw().
+ * system suspend.
+ *
+ * It must be called with power domains already disabled (after a call to
+ * intel_power_domains_disable()) and paired with intel_power_domains_resume().
  */
-void intel_power_domains_suspend(struct drm_i915_private *dev_priv)
+void intel_power_domains_suspend(struct drm_i915_private *dev_priv,
+				 enum i915_drm_suspend_mode suspend_mode)
 {
+	struct i915_power_domains *power_domains = &dev_priv->power_domains;
+
+	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
+
+	/*
+	 * In case of firmware assisted context save/restore don't manually
+	 * deinit the power domains. This also means the CSR/DMC firmware will
+	 * stay active, it will power down any HW resources as required and
+	 * also enable deeper system power states that would be blocked if the
+	 * firmware was inactive.
+	 */
+	if (!IS_GEN9_LP(dev_priv) && suspend_mode == I915_DRM_SUSPEND_IDLE &&
+	    dev_priv->csr.dmc_payload != NULL)
+		return;
+
 	/*
 	 * Even if power well support was disabled we still want to disable
-	 * power wells while we are system suspended.
+	 * power wells if power domains must be deinitialized for suspend.
 	 */
 	if (!i915_modparams.disable_power_well)
 		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
@@ -3832,6 +3863,32 @@ void intel_power_domains_suspend(struct drm_i915_private *dev_priv)
 		skl_display_core_uninit(dev_priv);
 	else if (IS_GEN9_LP(dev_priv))
 		bxt_display_core_uninit(dev_priv);
+
+	power_domains->display_core_suspended = true;
+}
+
+/**
+ * intel_power_domains_resume - resume power domain state
+ * @dev_priv: i915 device instance
+ *
+ * This function resume the hardware power domain state during system resume.
+ *
+ * It will return with power domain support disabled (to be enabled later by
+ * intel_power_domains_enable()) and must be paired with
+ * intel_power_domains_suspend().
+ */
+void intel_power_domains_resume(struct drm_i915_private *dev_priv)
+{
+	struct i915_power_domains *power_domains = &dev_priv->power_domains;
+
+	if (power_domains->display_core_suspended) {
+		intel_power_domains_init_hw(dev_priv, true);
+		power_domains->display_core_suspended = false;
+
+		return;
+	}
+
+	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
 }
 
 static void intel_power_domains_dump_info(struct drm_i915_private *dev_priv)
@@ -4030,8 +4087,8 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv)
  * This function enables runtime pm at the end of the driver load sequence.
  *
  * Note that this function does currently not enable runtime pm for the
- * subordinate display power domains. That is only done on the first modeset
- * using intel_display_set_init_power().
+ * subordinate display power domains. That is done by
+ * intel_power_domains_enable().
  */
 void intel_runtime_pm_enable(struct drm_i915_private *dev_priv)
 {
-- 
2.13.2

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

Re: [PATCH 2/2] drm/i915: Refactor intel_display_set_init_power() logic

[Chris Wilson]

Quoting Imre Deak (2018-08-16 13:37:57)
> The device global init_power_on flag is somewhat arbitrary and makes
> debugging power refcounting problems difficult. Instead arrange things
> so that all display power domain get has a corresponding put call. After
> this change we have the following sequences:
> 
> driver loading:
> intel_power_domains_init_hw();
> <other init steps>
> intel_power_domains_enable();
> 
> driver unloading:
> intel_power_domains_disable();
> <other uninit steps>
> intel_power_domains_fini_hw();
> 
> system suspend:
> intel_power_domains_disable();
> <other suspend steps>
> intel_power_domains_suspend();
> 
> system resume:
> intel_power_domains_resume();
> <other resume steps>
> intel_power_domains_enable();
> 
> at other times while the driver is loaded:
> intel_display_power_get();
> ...
> intel_display_power_put();
> 
> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

I could follow the phases and was able to add the wakeref tracing
(though I resorted to keeping it stored in i915_power_domains), so

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

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/2] drm/i915: Introduce intel_runtime_pm_disable to pair intel_runtime_pm_enable

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Introduce intel_runtime_pm_disable to pair intel_runtime_pm_enable
URL   : https://patchwork.freedesktop.org/series/48315/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
62108989ff60 drm/i915: Introduce intel_runtime_pm_disable to pair intel_runtime_pm_enable
2e47b9b2aba3 drm/i915: Refactor intel_display_set_init_power() logic
-:436: CHECK:COMPARISON_TO_NULL: Comparison to NULL could be written "dev_priv->csr.dmc_payload"
#436: FILE: drivers/gpu/drm/i915/intel_runtime_pm.c:3848:
+	    dev_priv->csr.dmc_payload != NULL)

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

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

✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Introduce intel_runtime_pm_disable to pair intel_runtime_pm_enable

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Introduce intel_runtime_pm_disable to pair intel_runtime_pm_enable
URL   : https://patchwork.freedesktop.org/series/48315/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4678 -> Patchwork_9962 =

== Summary - SUCCESS ==

  No regressions found.

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

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

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

    igt@kms_setmode@basic-clone-single-crtc:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106745)

    
    ==== Possible fixes ====

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

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

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-bxt-dsi:         INCOMPLETE (fdo#103927) -> PASS

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

  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  fdo#106745 https://bugs.freedesktop.org/show_bug.cgi?id=106745
  fdo#107174 https://bugs.freedesktop.org/show_bug.cgi?id=107174
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362


== Participating hosts (54 -> 47) ==

  Missing    (7): fi-ilk-m540 fi-icl-u fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-kbl-7560u 


== Build changes ==

    * Linux: CI_DRM_4678 -> Patchwork_9962

  CI_DRM_4678: 433fa6ccf8553afcc0159a996297b82bf50d1fac @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4603: 1598fdb717546e25e8077935daa8e97768ad245d @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9962: 2e47b9b2aba3b18febcc6978f6c2b2d55f4d7ff1 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

2e47b9b2aba3 drm/i915: Refactor intel_display_set_init_power() logic
62108989ff60 drm/i915: Introduce intel_runtime_pm_disable to pair intel_runtime_pm_enable

== Logs ==

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

Re: ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Introduce intel_runtime_pm_disable to pair intel_runtime_pm_enable

[Imre Deak]

On Thu, Aug 16, 2018 at 01:47:08PM +0000, Patchwork wrote:
> == Series Details ==
> 
> Series: series starting with [1/2] drm/i915: Introduce intel_runtime_pm_disable to pair intel_runtime_pm_enable
> URL   : https://patchwork.freedesktop.org/series/48315/
> State : success

Pushed to -dinq, thanks for the review.

> 
> == Summary ==
> 
> = CI Bug Log - changes from CI_DRM_4678 -> Patchwork_9962 =
> 
> == Summary - SUCCESS ==
> 
>   No regressions found.
> 
>   External URL: https://patchwork.freedesktop.org/api/1.0/series/48315/revisions/1/mbox/
> 
> == Known issues ==
> 
>   Here are the changes found in Patchwork_9962 that come from known issues:
> 
>   === IGT changes ===
> 
>     ==== Issues hit ====
> 
>     igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
>       {fi-byt-clapper}:   PASS -> FAIL (fdo#107362, fdo#103191)
> 
>     igt@kms_setmode@basic-clone-single-crtc:
>       fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106745)
> 
>     
>     ==== Possible fixes ====
> 
>     igt@drv_selftest@live_hangcheck:
>       fi-skl-guc:         DMESG-FAIL (fdo#107174) -> PASS
>       fi-cfl-s3:          DMESG-FAIL (fdo#106560) -> PASS
> 
>     igt@kms_pipe_crc_basic@read-crc-pipe-b:
>       {fi-byt-clapper}:   FAIL (fdo#107362) -> PASS
> 
>     igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
>       fi-bxt-dsi:         INCOMPLETE (fdo#103927) -> PASS
> 
>     
>   {name}: This element is suppressed. This means it is ignored when computing
>           the status of the difference (SUCCESS, WARNING, or FAILURE).
> 
>   fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
>   fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
>   fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
>   fdo#106745 https://bugs.freedesktop.org/show_bug.cgi?id=106745
>   fdo#107174 https://bugs.freedesktop.org/show_bug.cgi?id=107174
>   fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
> 
> 
> == Participating hosts (54 -> 47) ==
> 
>   Missing    (7): fi-ilk-m540 fi-icl-u fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-kbl-7560u 
> 
> 
> == Build changes ==
> 
>     * Linux: CI_DRM_4678 -> Patchwork_9962
> 
>   CI_DRM_4678: 433fa6ccf8553afcc0159a996297b82bf50d1fac @ git://anongit.freedesktop.org/gfx-ci/linux
>   IGT_4603: 1598fdb717546e25e8077935daa8e97768ad245d @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
>   Patchwork_9962: 2e47b9b2aba3b18febcc6978f6c2b2d55f4d7ff1 @ git://anongit.freedesktop.org/gfx-ci/linux
> 
> 
> == Linux commits ==
> 
> 2e47b9b2aba3 drm/i915: Refactor intel_display_set_init_power() logic
> 62108989ff60 drm/i915: Introduce intel_runtime_pm_disable to pair intel_runtime_pm_enable
> 
> == Logs ==
> 
> For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9962/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with [1/2] drm/i915: Introduce intel_runtime_pm_disable to pair intel_runtime_pm_enable

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Introduce intel_runtime_pm_disable to pair intel_runtime_pm_enable
URL   : https://patchwork.freedesktop.org/series/48315/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4678_full -> Patchwork_9962_full =

== Summary - SUCCESS ==

  No regressions found.

  

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_cursor_legacy@2x-nonblocking-modeset-vs-cursor-atomic:
      shard-glk:          PASS -> FAIL (fdo#105454, fdo#106509)

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


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4678 -> Patchwork_9962

  CI_DRM_4678: 433fa6ccf8553afcc0159a996297b82bf50d1fac @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4603: 1598fdb717546e25e8077935daa8e97768ad245d @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9962: 2e47b9b2aba3b18febcc6978f6c2b2d55f4d7ff1 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

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