drm/i915/gen9+: Don't remove secondary power well requests

So far in an attempt to make sure all power wells get disabled during
display uninitialization the driver removed any secondary request bits
(BIOS, KVMR, DEBUG) that were set for a given power well. The known
source for these requests was DMC's request on power well 1 and the misc
IO power well. Since DMC is inactive (DC states are disabled) at the
point we disable these power wells, there shouldn't be any reason to
leave them on. However there are two problems with the above
assumption: Bspec requires that the misc IO power well stays enabled
(without providing a reason) and there can be KVMR requests that we
can't remove anyway (the KVMR request register is R/O). Atm, a KVMR
request can trigger a timeout WARN when trying to disable power wells.

To make the code aligned to Bspec and to get rid of the KVMR WARN, don't
try to remove the secondary requests, only detect them and stop polling
for the power well disabled state when any one is set.

Also add a comment about the timeout values required by Bspec when
enabling power wells and the fact that waiting for them to get disabled
is not required by Bspec.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98564
Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/1498750622-14023-5-git-send-email-imre.deak@intel.com

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 1fc75e6769bc5ffe71f0b849a3e32698903f8a9d..2fe715b25f9e03f36bdffc433b35cfc339fa0107 100644 (file)
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -341,6 +341,59 @@ static void skl_power_well_pre_disable(struct drm_i915_private *dev_priv,
                                                1 << PIPE_C | 1 << PIPE_B);
 }
 
+static void gen9_wait_for_power_well_enable(struct drm_i915_private *dev_priv,
+                                           struct i915_power_well *power_well)
+{
+       int id = power_well->id;
+
+       /* Timeout for PW1:10 us, AUX:not specified, other PWs:20 us. */
+       WARN_ON(intel_wait_for_register(dev_priv,
+                                       HSW_PWR_WELL_DRIVER,
+                                       SKL_POWER_WELL_STATE(id),
+                                       SKL_POWER_WELL_STATE(id),
+                                       1));
+}
+
+static u32 gen9_power_well_requesters(struct drm_i915_private *dev_priv, int id)
+{
+       u32 req_mask = SKL_POWER_WELL_REQ(id);
+       u32 ret;
+
+       ret = I915_READ(HSW_PWR_WELL_BIOS) & req_mask ? 1 : 0;
+       ret |= I915_READ(HSW_PWR_WELL_DRIVER) & req_mask ? 2 : 0;
+       ret |= I915_READ(HSW_PWR_WELL_KVMR) & req_mask ? 4 : 0;
+       ret |= I915_READ(HSW_PWR_WELL_DEBUG) & req_mask ? 8 : 0;
+
+       return ret;
+}
+
+static void gen9_wait_for_power_well_disable(struct drm_i915_private *dev_priv,
+                                            struct i915_power_well *power_well)
+{
+       int id = power_well->id;
+       bool disabled;
+       u32 reqs;
+
+       /*
+        * Bspec doesn't require waiting for PWs to get disabled, but still do
+        * this for paranoia. The known cases where a PW will be forced on:
+        * - a KVMR request on any power well via the KVMR request register
+        * - a DMC request on PW1 and MISC_IO power wells via the BIOS and
+        *   DEBUG request registers
+        * Skip the wait in case any of the request bits are set and print a
+        * diagnostic message.
+        */
+       wait_for((disabled = !(I915_READ(HSW_PWR_WELL_DRIVER) &
+                              SKL_POWER_WELL_STATE(id))) ||
+                (reqs = gen9_power_well_requesters(dev_priv, id)), 1);
+       if (disabled)
+               return;
+
+       DRM_DEBUG_KMS("%s forced on (bios:%d driver:%d kvmr:%d debug:%d)\n",
+                     power_well->name,
+                     !!(reqs & 1), !!(reqs & 2), !!(reqs & 4), !!(reqs & 8));
+}
+
 static void hsw_set_power_well(struct drm_i915_private *dev_priv,
                               struct i915_power_well *power_well, bool enable)
 {
@@ -746,45 +799,6 @@ void skl_disable_dc6(struct drm_i915_private *dev_priv)
        gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
 }
 
-static void
-gen9_sanitize_power_well_requests(struct drm_i915_private *dev_priv,
-                                 struct i915_power_well *power_well)
-{
-       enum skl_disp_power_wells power_well_id = power_well->id;
-       u32 val;
-       u32 mask;
-
-       mask = SKL_POWER_WELL_REQ(power_well_id);
-
-       val = I915_READ(HSW_PWR_WELL_KVMR);
-       if (WARN_ONCE(val & mask, "Clearing unexpected KVMR request for %s\n",
-                     power_well->name))
-               I915_WRITE(HSW_PWR_WELL_KVMR, val & ~mask);
-
-       val = I915_READ(HSW_PWR_WELL_BIOS);
-       val |= I915_READ(HSW_PWR_WELL_DEBUG);
-
-       if (!(val & mask))
-               return;
-
-       /*
-        * DMC is known to force on the request bits for power well 1 on SKL
-        * and BXT and the misc IO power well on SKL but we don't expect any
-        * other request bits to be set, so WARN for those.
-        */
-       if (power_well_id == SKL_DISP_PW_1 ||
-           (IS_GEN9_BC(dev_priv) &&
-            power_well_id == SKL_DISP_PW_MISC_IO))
-               DRM_DEBUG_DRIVER("Clearing auxiliary requests for %s forced on "
-                                "by DMC\n", power_well->name);
-       else
-               WARN_ONCE(1, "Clearing unexpected auxiliary requests for %s\n",
-                         power_well->name);
-
-       I915_WRITE(HSW_PWR_WELL_BIOS, val & ~mask);
-       I915_WRITE(HSW_PWR_WELL_DEBUG, val & ~mask);
-}
-
 static void skl_set_power_well(struct drm_i915_private *dev_priv,
                               struct i915_power_well *power_well, bool enable)
 {
@@ -848,6 +862,8 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
                        DRM_DEBUG_KMS("Enabling %s\n", power_well->name);
                        check_fuse_status = true;
                }
+
+               gen9_wait_for_power_well_enable(dev_priv, power_well);
        } else {
                if (enable_requested) {
                        I915_WRITE(HSW_PWR_WELL_DRIVER, tmp & ~req_mask);
@@ -855,14 +871,9 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
                        DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
                }
 
-               gen9_sanitize_power_well_requests(dev_priv, power_well);
+               gen9_wait_for_power_well_disable(dev_priv, power_well);
        }
 
-       if (wait_for(!!(I915_READ(HSW_PWR_WELL_DRIVER) & state_mask) == enable,
-                    1))
-               DRM_ERROR("%s %s timeout\n",
-                         power_well->name, enable ? "enable" : "disable");
-
        if (check_fuse_status) {
                if (power_well->id == SKL_DISP_PW_1) {
                        if (intel_wait_for_register(dev_priv,
@@ -2699,6 +2710,8 @@ static void skl_display_core_uninit(struct drm_i915_private *dev_priv)
        /*
         * BSpec says to keep the MISC IO power well enabled here, only
         * remove our request for power well 1.
+        * Note that even though the driver's request is removed power well 1
+        * may stay enabled after this due to DMC's own request on it.
         */
        well = lookup_power_well(dev_priv, SKL_DISP_PW_1);
        intel_power_well_disable(dev_priv, well);
@@ -2756,7 +2769,11 @@ void bxt_display_core_uninit(struct drm_i915_private *dev_priv)
 
        /* The spec doesn't call for removing the reset handshake flag */
 
-       /* Disable PG1 */
+       /*
+        * Disable PW1 (PG1).
+        * Note that even though the driver's request is removed power well 1
+        * may stay enabled after this due to DMC's own request on it.
+        */
        mutex_lock(&power_domains->lock);
 
        well = lookup_power_well(dev_priv, SKL_DISP_PW_1);