[PATCH 0/5] gen9+: Sanitize power well disabling during display uninit

[PATCH 0/5] gen9+: Sanitize power well disabling during display uninit

[Imre Deak]

This patchset aligns the display uninit sequence with Bspec, wrt. to
disabling power well 1 and the misc IO power well. It also tunes down a
timeout WARN to be a debug message when waiting for power wells to get
disabled while KVMR is active.

Imre Deak (5):
  drm/i915/gen9+: Add 10 us delay after power well 1/AUX IO pw disabling
  drm/i915/skl: Don't disable misc IO power well during display uninit
  drm/i915/bxt,glk: Fix assert on conditions for DC9 enabling
  drm/i915/gen9+: Don't remove secondary power well requests
  drm/i915/cnl: Fix comment about AUX IO power well enable/disable

 drivers/gpu/drm/i915/intel_runtime_pm.c | 137 ++++++++++++++++++++------------
 1 file changed, 85 insertions(+), 52 deletions(-)

-- 
2.7.4

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

[PATCH 1/5] drm/i915/gen9+: Add 10 us delay after power well 1/AUX IO pw disabling

[Imre Deak]

Bspec requires a 10 us delay after disabling power well 1 and - if not
toggled on-demand - the AUX IO power wells during display uninit.

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

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index efe80ed..fd59016 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -2701,6 +2701,8 @@ static void skl_display_core_uninit(struct drm_i915_private *dev_priv)
 	intel_power_well_disable(dev_priv, well);
 
 	mutex_unlock(&power_domains->lock);
+
+	usleep_range(10, 30);		/* 10 us delay per Bspec */
 }
 
 void bxt_display_core_init(struct drm_i915_private *dev_priv,
@@ -2758,6 +2760,8 @@ void bxt_display_core_uninit(struct drm_i915_private *dev_priv)
 	intel_power_well_disable(dev_priv, well);
 
 	mutex_unlock(&power_domains->lock);
+
+	usleep_range(10, 30);		/* 10 us delay per Bspec */
 }
 
 #define CNL_PROCMON_IDX(val) \
@@ -2859,6 +2863,8 @@ static void cnl_display_core_uninit(struct drm_i915_private *dev_priv)
 	intel_power_well_disable(dev_priv, well);
 	mutex_unlock(&power_domains->lock);
 
+	usleep_range(10, 30);		/* 10 us delay per Bspec */
+
 	/* 5. Disable Comp */
 	val = I915_READ(CHICKEN_MISC_2);
 	val |= COMP_PWR_DOWN;
-- 
2.7.4

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

[PATCH 2/5] drm/i915/skl: Don't disable misc IO power well during display uninit

[Imre Deak]

Bspec requires leaving the misc IO power well enabled during display
uninit, so align the code accordingly.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index fd59016..8418879 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -2694,9 +2694,10 @@ static void skl_display_core_uninit(struct drm_i915_private *dev_priv)
 
 	mutex_lock(&power_domains->lock);
 
-	well = lookup_power_well(dev_priv, SKL_DISP_PW_MISC_IO);
-	intel_power_well_disable(dev_priv, well);
-
+	/*
+	 * BSpec says to keep the MISC IO power well enabled here, only
+	 * remove our request for power well 1.
+	 */
 	well = lookup_power_well(dev_priv, SKL_DISP_PW_1);
 	intel_power_well_disable(dev_priv, well);
 
-- 
2.7.4

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

[PATCH 3/5] drm/i915/bxt, glk: Fix assert on conditions for DC9 enabling

[Imre Deak]

What we want to assert based on the conditions required by Bspec is that
power well 2 is disabled, so no need to check for other power wells.
In addition we can only check if the driver's request is removed, the
actual state depends on whether the other request bits are set or not
(BIOS, KVMR, DEBUG). So check only the driver's request bit.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 8418879..1fc75e6 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -549,7 +549,9 @@ static void assert_can_enable_dc9(struct drm_i915_private *dev_priv)
 		  "DC9 already programmed to be enabled.\n");
 	WARN_ONCE(I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5,
 		  "DC5 still not disabled to enable DC9.\n");
-	WARN_ONCE(I915_READ(HSW_PWR_WELL_DRIVER), "Power well on.\n");
+	WARN_ONCE(I915_READ(HSW_PWR_WELL_DRIVER) &
+		  SKL_POWER_WELL_REQ(SKL_DISP_PW_2),
+		  "Power well 2 on.\n");
 	WARN_ONCE(intel_irqs_enabled(dev_priv),
 		  "Interrupts not disabled yet.\n");
 
-- 
2.7.4

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

[PATCH 4/5] drm/i915/gen9+: Don't remove secondary power well requests

[Imre Deak]

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>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 109 ++++++++++++++++++--------------
 1 file changed, 63 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 1fc75e6..2fe715b 100644
--- 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);
-- 
2.7.4

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

[PATCH 5/5] drm/i915/cnl: Fix comment about AUX IO power well enable/disable

[Imre Deak]

The comments match an earlier version of the patch, fix them to match
the current state.

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

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 2fe715b..5eb9c5e 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -2845,7 +2845,10 @@ static void cnl_display_core_init(struct drm_i915_private *dev_priv, bool resume
 	val |= CL_POWER_DOWN_ENABLE;
 	I915_WRITE(CNL_PORT_CL1CM_DW5, val);
 
-	/* 4. Enable Power Well 1 (PG1) and Aux IO Power */
+	/*
+	 * 4. Enable Power Well 1 (PG1).
+	 *    The AUX IO power wells will be enabled on demand.
+	 */
 	mutex_lock(&power_domains->lock);
 	well = lookup_power_well(dev_priv, SKL_DISP_PW_1);
 	intel_power_well_enable(dev_priv, well);
@@ -2877,7 +2880,11 @@ static void cnl_display_core_uninit(struct drm_i915_private *dev_priv)
 	/* 3. Disable CD clock */
 	cnl_uninit_cdclk(dev_priv);
 
-	/* 4. Disable Power Well 1 (PG1) and Aux IO Power */
+	/*
+	 * 4. Disable Power Well 1 (PG1).
+	 *    The AUX IO power wells are toggled on demand, so they are already
+	 *    disabled at this point.
+	 */
 	mutex_lock(&power_domains->lock);
 	well = lookup_power_well(dev_priv, SKL_DISP_PW_1);
 	intel_power_well_disable(dev_priv, well);
-- 
2.7.4

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

✓ Fi.CI.BAT: success for gen9+: Sanitize power well disabling during display uninit

[Patchwork]

== Series Details ==

Series: gen9+: Sanitize power well disabling during display uninit
URL   : https://patchwork.freedesktop.org/series/26566/
State : success

== Summary ==

Series 26566v1 gen9+: Sanitize power well disabling during display uninit
https://patchwork.freedesktop.org/api/1.0/series/26566/revisions/1/mbox/

Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                pass       -> DMESG-WARN (fi-kbl-7560u) fdo#100125
Test kms_force_connector_basic:
        Subgroup force-connector-state:
                skip       -> PASS       (fi-snb-2520m) fdo#101048
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-c:
                pass       -> INCOMPLETE (fi-skl-6260u) fdo#100461

fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125
fdo#101048 https://bugs.freedesktop.org/show_bug.cgi?id=101048
fdo#100461 https://bugs.freedesktop.org/show_bug.cgi?id=100461

fi-bdw-5557u     total:279  pass:265  dwarn:3   dfail:0   fail:0   skip:11  time:440s
fi-bdw-gvtdvm    total:279  pass:257  dwarn:8   dfail:0   fail:0   skip:14  time:427s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:358s
fi-bsw-n3050     total:279  pass:243  dwarn:0   dfail:0   fail:0   skip:36  time:533s
fi-bxt-j4205     total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:507s
fi-byt-j1900     total:279  pass:254  dwarn:1   dfail:0   fail:0   skip:24  time:490s
fi-byt-n2820     total:279  pass:250  dwarn:1   dfail:0   fail:0   skip:28  time:483s
fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:597s
fi-hsw-4770      total:279  pass:259  dwarn:4   dfail:0   fail:0   skip:16  time:437s
fi-hsw-4770r     total:279  pass:259  dwarn:4   dfail:0   fail:0   skip:16  time:411s
fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:420s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:501s
fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:471s
fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:464s
fi-kbl-7560u     total:279  pass:268  dwarn:1   dfail:0   fail:0   skip:10  time:572s
fi-kbl-r         total:279  pass:260  dwarn:1   dfail:0   fail:0   skip:18  time:578s
fi-pnv-d510      total:279  pass:223  dwarn:1   dfail:0   fail:0   skip:55  time:554s
fi-skl-6260u     total:237  pass:229  dwarn:0   dfail:0   fail:0   skip:7  
fi-skl-6700hq    total:279  pass:223  dwarn:1   dfail:0   fail:30  skip:24  time:341s
fi-skl-6700k     total:279  pass:257  dwarn:4   dfail:0   fail:0   skip:18  time:468s
fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:481s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:434s
fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:545s
fi-snb-2600      total:279  pass:250  dwarn:0   dfail:0   fail:0   skip:29  time:400s

f7d0276ea92c21303ea253af21cfff0778ed5a39 drm-tip: 2017y-06m-29d-15h-35m-54s UTC integration manifest
8ccc622 drm/i915/cnl: Fix comment about AUX IO power well enable/disable
ad26976 drm/i915/gen9+: Don't remove secondary power well requests
007dcbf drm/i915/bxt, glk: Fix assert on conditions for DC9 enabling
9d07cb0 drm/i915/skl: Don't disable misc IO power well during display uninit
1db4f41 drm/i915/gen9+: Add 10 us delay after power well 1/AUX IO pw disabling

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_5073/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915/cnl: Fix comment about AUX IO power well enable/disable

[Rodrigo Vivi]

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>



On Thu, Jun 29, 2017 at 8:37 AM, Imre Deak <imre.deak@intel.com> wrote:
> The comments match an earlier version of the patch, fix them to match
> the current state.
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 2fe715b..5eb9c5e 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -2845,7 +2845,10 @@ static void cnl_display_core_init(struct drm_i915_private *dev_priv, bool resume
>         val |= CL_POWER_DOWN_ENABLE;
>         I915_WRITE(CNL_PORT_CL1CM_DW5, val);
>
> -       /* 4. Enable Power Well 1 (PG1) and Aux IO Power */
> +       /*
> +        * 4. Enable Power Well 1 (PG1).
> +        *    The AUX IO power wells will be enabled on demand.
> +        */
>         mutex_lock(&power_domains->lock);
>         well = lookup_power_well(dev_priv, SKL_DISP_PW_1);
>         intel_power_well_enable(dev_priv, well);
> @@ -2877,7 +2880,11 @@ static void cnl_display_core_uninit(struct drm_i915_private *dev_priv)
>         /* 3. Disable CD clock */
>         cnl_uninit_cdclk(dev_priv);
>
> -       /* 4. Disable Power Well 1 (PG1) and Aux IO Power */
> +       /*
> +        * 4. Disable Power Well 1 (PG1).
> +        *    The AUX IO power wells are toggled on demand, so they are already
> +        *    disabled at this point.
> +        */
>         mutex_lock(&power_domains->lock);
>         well = lookup_power_well(dev_priv, SKL_DISP_PW_1);
>         intel_power_well_disable(dev_priv, well);
> --
> 2.7.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915/bxt, glk: Fix assert on conditions for DC9 enabling

[Rodrigo Vivi]

"Disable all display engine functions using the full mode set disable
sequence on all pipes, transcoders, ports, planes, and power well 2
(PG2)."

I hope at this point we really already did all the rest besides the PG2.

Anyways I believe this patch itself makes total sense so:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>


On Thu, Jun 29, 2017 at 8:37 AM, Imre Deak <imre.deak@intel.com> wrote:
> What we want to assert based on the conditions required by Bspec is that
> power well 2 is disabled, so no need to check for other power wells.
> In addition we can only check if the driver's request is removed, the
> actual state depends on whether the other request bits are set or not
> (BIOS, KVMR, DEBUG). So check only the driver's request bit.
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 8418879..1fc75e6 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -549,7 +549,9 @@ static void assert_can_enable_dc9(struct drm_i915_private *dev_priv)
>                   "DC9 already programmed to be enabled.\n");
>         WARN_ONCE(I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5,
>                   "DC5 still not disabled to enable DC9.\n");
> -       WARN_ONCE(I915_READ(HSW_PWR_WELL_DRIVER), "Power well on.\n");
> +       WARN_ONCE(I915_READ(HSW_PWR_WELL_DRIVER) &
> +                 SKL_POWER_WELL_REQ(SKL_DISP_PW_2),
> +                 "Power well 2 on.\n");
>         WARN_ONCE(intel_irqs_enabled(dev_priv),
>                   "Interrupts not disabled yet.\n");
>
> --
> 2.7.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915/gen9+: Add 10 us delay after power well 1/AUX IO pw disabling

[Rodrigo Vivi]

Oh! indeed!

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>


On Thu, Jun 29, 2017 at 8:36 AM, Imre Deak <imre.deak@intel.com> wrote:
> Bspec requires a 10 us delay after disabling power well 1 and - if not
> toggled on-demand - the AUX IO power wells during display uninit.
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index efe80ed..fd59016 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -2701,6 +2701,8 @@ static void skl_display_core_uninit(struct drm_i915_private *dev_priv)
>         intel_power_well_disable(dev_priv, well);
>
>         mutex_unlock(&power_domains->lock);
> +
> +       usleep_range(10, 30);           /* 10 us delay per Bspec */
>  }
>
>  void bxt_display_core_init(struct drm_i915_private *dev_priv,
> @@ -2758,6 +2760,8 @@ void bxt_display_core_uninit(struct drm_i915_private *dev_priv)
>         intel_power_well_disable(dev_priv, well);
>
>         mutex_unlock(&power_domains->lock);
> +
> +       usleep_range(10, 30);           /* 10 us delay per Bspec */
>  }
>
>  #define CNL_PROCMON_IDX(val) \
> @@ -2859,6 +2863,8 @@ static void cnl_display_core_uninit(struct drm_i915_private *dev_priv)
>         intel_power_well_disable(dev_priv, well);
>         mutex_unlock(&power_domains->lock);
>
> +       usleep_range(10, 30);           /* 10 us delay per Bspec */
> +
>         /* 5. Disable Comp */
>         val = I915_READ(CHICKEN_MISC_2);
>         val |= COMP_PWR_DOWN;
> --
> 2.7.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915/skl: Don't disable misc IO power well during display uninit

[Rodrigo Vivi]

Indeed very clear there...
I don't know how did this never caused any big trouble...

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

On Thu, Jun 29, 2017 at 8:36 AM, Imre Deak <imre.deak@intel.com> wrote:
> Bspec requires leaving the misc IO power well enabled during display
> uninit, so align the code accordingly.
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index fd59016..8418879 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -2694,9 +2694,10 @@ static void skl_display_core_uninit(struct drm_i915_private *dev_priv)
>
>         mutex_lock(&power_domains->lock);
>
> -       well = lookup_power_well(dev_priv, SKL_DISP_PW_MISC_IO);
> -       intel_power_well_disable(dev_priv, well);
> -
> +       /*
> +        * BSpec says to keep the MISC IO power well enabled here, only
> +        * remove our request for power well 1.
> +        */
>         well = lookup_power_well(dev_priv, SKL_DISP_PW_1);
>         intel_power_well_disable(dev_priv, well);
>
> --
> 2.7.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915/gen9+: Don't remove secondary power well requests

[Rodrigo Vivi]

This patch makes sense and seems the right thing to do...

However removing the sanitize with I915_WRITE(*, val & ~mask);
doesn't give me very comfortable...

I've seem many power well timeouts on cnl due the lack of that sanitize...
I will try to save some time here and do some experiments with cnl.

On Thu, Jun 29, 2017 at 8:37 AM, Imre Deak <imre.deak@intel.com> wrote:
> 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>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 109 ++++++++++++++++++--------------
>  1 file changed, 63 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 1fc75e6..2fe715b 100644
> --- 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);
> --
> 2.7.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915/gen9+: Don't remove secondary power well requests

[Imre Deak]

On Thu, Jun 29, 2017 at 01:06:01PM -0700, Rodrigo Vivi wrote:
> This patch makes sense and seems the right thing to do...
> 
> However removing the sanitize with I915_WRITE(*, val & ~mask);
> doesn't give me very comfortable...
> 
> I've seem many power well timeouts on cnl due the lack of that sanitize...
> I will try to save some time here and do some experiments with cnl.

Thanks for the review.

You shouldn't get any timeouts due to this change. On CNL as on any other
GEN9+ platform previously you got a timeout when disabling power well 1
during the display uninit sequence, which was due to DMC's own request on
PW#1. After this change we'll detect this scenario and won't wait for
the power well to get disabled.

--Imre

> 
> On Thu, Jun 29, 2017 at 8:37 AM, Imre Deak <imre.deak@intel.com> wrote:
> > 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>
> > ---
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 109 ++++++++++++++++++--------------
> >  1 file changed, 63 insertions(+), 46 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 1fc75e6..2fe715b 100644
> > --- 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);
> > --
> > 2.7.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915/gen9+: Don't remove secondary power well requests

[Rodrigo Vivi]

yep, makes total sense and you were right... no timeout..

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

On Fri, Jun 30, 2017 at 5:29 AM, Imre Deak <imre.deak@intel.com> wrote:
> On Thu, Jun 29, 2017 at 01:06:01PM -0700, Rodrigo Vivi wrote:
>> This patch makes sense and seems the right thing to do...
>>
>> However removing the sanitize with I915_WRITE(*, val & ~mask);
>> doesn't give me very comfortable...
>>
>> I've seem many power well timeouts on cnl due the lack of that sanitize...
>> I will try to save some time here and do some experiments with cnl.
>
> Thanks for the review.
>
> You shouldn't get any timeouts due to this change. On CNL as on any other
> GEN9+ platform previously you got a timeout when disabling power well 1
> during the display uninit sequence, which was due to DMC's own request on
> PW#1. After this change we'll detect this scenario and won't wait for
> the power well to get disabled.
>
> --Imre
>
>>
>> On Thu, Jun 29, 2017 at 8:37 AM, Imre Deak <imre.deak@intel.com> wrote:
>> > 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>
>> > ---
>> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 109 ++++++++++++++++++--------------
>> >  1 file changed, 63 insertions(+), 46 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> > index 1fc75e6..2fe715b 100644
>> > --- 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);
>> > --
>> > 2.7.4
>> >
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>>
>>
>> --
>> Rodrigo Vivi
>> Blog: http://blog.vivi.eng.br



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✓ Fi.CI.BAT: success for gen9+: Sanitize power well disabling during display uninit

[Imre Deak]

On Thu, Jun 29, 2017 at 04:22:51PM +0000, Patchwork wrote:
> == Series Details ==
> 
> Series: gen9+: Sanitize power well disabling during display uninit
> URL   : https://patchwork.freedesktop.org/series/26566/
> State : success

Thanks for the review, I pushed the series to -dinq.

> 
> == Summary ==
> 
> Series 26566v1 gen9+: Sanitize power well disabling during display uninit
> https://patchwork.freedesktop.org/api/1.0/series/26566/revisions/1/mbox/
> 
> Test gem_exec_suspend:
>         Subgroup basic-s4-devices:
>                 pass       -> DMESG-WARN (fi-kbl-7560u) fdo#100125
> Test kms_force_connector_basic:
>         Subgroup force-connector-state:
>                 skip       -> PASS       (fi-snb-2520m) fdo#101048
> Test kms_pipe_crc_basic:
>         Subgroup suspend-read-crc-pipe-c:
>                 pass       -> INCOMPLETE (fi-skl-6260u) fdo#100461
> 
> fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125
> fdo#101048 https://bugs.freedesktop.org/show_bug.cgi?id=101048
> fdo#100461 https://bugs.freedesktop.org/show_bug.cgi?id=100461
> 
> fi-bdw-5557u     total:279  pass:265  dwarn:3   dfail:0   fail:0   skip:11  time:440s
> fi-bdw-gvtdvm    total:279  pass:257  dwarn:8   dfail:0   fail:0   skip:14  time:427s
> fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:358s
> fi-bsw-n3050     total:279  pass:243  dwarn:0   dfail:0   fail:0   skip:36  time:533s
> fi-bxt-j4205     total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:507s
> fi-byt-j1900     total:279  pass:254  dwarn:1   dfail:0   fail:0   skip:24  time:490s
> fi-byt-n2820     total:279  pass:250  dwarn:1   dfail:0   fail:0   skip:28  time:483s
> fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:597s
> fi-hsw-4770      total:279  pass:259  dwarn:4   dfail:0   fail:0   skip:16  time:437s
> fi-hsw-4770r     total:279  pass:259  dwarn:4   dfail:0   fail:0   skip:16  time:411s
> fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:420s
> fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:501s
> fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:471s
> fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:464s
> fi-kbl-7560u     total:279  pass:268  dwarn:1   dfail:0   fail:0   skip:10  time:572s
> fi-kbl-r         total:279  pass:260  dwarn:1   dfail:0   fail:0   skip:18  time:578s
> fi-pnv-d510      total:279  pass:223  dwarn:1   dfail:0   fail:0   skip:55  time:554s
> fi-skl-6260u     total:237  pass:229  dwarn:0   dfail:0   fail:0   skip:7  
> fi-skl-6700hq    total:279  pass:223  dwarn:1   dfail:0   fail:30  skip:24  time:341s
> fi-skl-6700k     total:279  pass:257  dwarn:4   dfail:0   fail:0   skip:18  time:468s
> fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:481s
> fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:434s
> fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:545s
> fi-snb-2600      total:279  pass:250  dwarn:0   dfail:0   fail:0   skip:29  time:400s
> 
> f7d0276ea92c21303ea253af21cfff0778ed5a39 drm-tip: 2017y-06m-29d-15h-35m-54s UTC integration manifest
> 8ccc622 drm/i915/cnl: Fix comment about AUX IO power well enable/disable
> ad26976 drm/i915/gen9+: Don't remove secondary power well requests
> 007dcbf drm/i915/bxt, glk: Fix assert on conditions for DC9 enabling
> 9d07cb0 drm/i915/skl: Don't disable misc IO power well during display uninit
> 1db4f41 drm/i915/gen9+: Add 10 us delay after power well 1/AUX IO pw disabling
> 
> == Logs ==
> 
> For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_5073/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx