drm/i915: Refactor intel_display_set_init_power() logic
authorImre Deak <imre.deak@intel.com>
Thu, 16 Aug 2018 12:37:57 +0000 (15:37 +0300)
committerImre Deak <imre.deak@intel.com>
Thu, 16 Aug 2018 14:12:15 +0000 (17:12 +0300)
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>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Link: https://patchwork.freedesktop.org/patch/msgid/20180816123757.3286-2-imre.deak@intel.com
drivers/gpu/drm/i915/i915_drv.c
drivers/gpu/drm/i915/i915_drv.h
drivers/gpu/drm/i915/intel_display.c
drivers/gpu/drm/i915/intel_drv.h
drivers/gpu/drm/i915/intel_runtime_pm.c

index 021304e252eb3ed36d18cf4af3fd9bc2365124da..35a012ffc03b920db2cc0b530c8d0aa2ea23cc29 100644 (file)
@@ -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 @@ out_cleanup_hw:
 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;
 }
 
index 5fa13887b911913c174b3597295d7048af5e28d7..74482753a04e0804db0e1a00c9b59eb27ae35941 100644 (file)
@@ -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;
index 3b41a247943a8bd3b5a239d921ce808d424dc83b..592b847db88e5b03c52efb94a25621f686728612 100644 (file)
@@ -15846,6 +15846,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);
 
@@ -15900,7 +15902,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);
 
index 364fc2504fa445501e76860c922a51f42bd05491..b2ce343b6027f77372e79d657388f5c5f3c22d9b 100644 (file)
@@ -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,
index c0983f0e46ac6fc1ee590b967ef10a1d1dd62ded..6153d5be5cf6b56fd9032a1ee81d5971b186d7cb 100644 (file)
@@ -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);
 
@@ -3808,18 +3790,67 @@ void intel_power_domains_fini_hw(struct drm_i915_private *dev_priv)
                intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
 }
 
+/**
+ * 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)
 {