[PATCH 0/4] drm/i915: Fix clearing of BIOS power well requests

[PATCH 0/4] drm/i915: Fix clearing of BIOS power well requests

[Imre Deak]

While reading Ander's GLK DDI power well patches [1] I noticed a few
issues in the existing code related to clearing/taking over the power
well request bits from BIOS. These didn't cause a problem so far, but
would be exposed by his patchset, so after discussing with him I
decided to fix them.

Cc: Ander Conselvan de Oliveira <conselvan2@gmail.com>
Cc: David Weinehall <david.weinehall@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>

[1]
https://lists.freedesktop.org/archives/intel-gfx/2017-February/119532.html

Imre Deak (4):
  drm/i915: Remove redundant toggling from the power well sync_hw hooks
  drm/i915: Call the sync_hw hook for power wells without a domain
  drm/i915/gen9: Fix clearing of the BIOS power well request register
  drm/i915: Preserve the state of power wells not explicitly enabled

 drivers/gpu/drm/i915/intel_runtime_pm.c | 109 ++++++++++++++------------------
 1 file changed, 49 insertions(+), 60 deletions(-)

-- 
2.5.0

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

[PATCH 1/4] drm/i915: Remove redundant toggling from the power well sync_hw hooks

[Imre Deak]

Doing an explicit enable/disable in the power well sync_hw hook based on
the power well's reference count is redundant, since by the time these
hooks are called all the power wells are enabled and have a reference.
So remove the redundant toggling.

This is needed by a follow-up patchset that adds power wells which we
can't enable/disable during power domain initialization and so want to
preserve their state until modeset init time.

Cc: Ander Conselvan de Oliveira <conselvan2@gmail.com>
Cc: David Weinehall <david.weinehall@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 52 +++++++--------------------------
 1 file changed, 10 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 8795679..0f64bc1 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -847,8 +847,6 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
 static void hsw_power_well_sync_hw(struct drm_i915_private *dev_priv,
 				   struct i915_power_well *power_well)
 {
-	hsw_set_power_well(dev_priv, power_well, power_well->count > 0);
-
 	/*
 	 * We're taking over the BIOS, so clear any requests made by it since
 	 * the driver is in charge now.
@@ -881,8 +879,6 @@ static bool skl_power_well_enabled(struct drm_i915_private *dev_priv,
 static void skl_power_well_sync_hw(struct drm_i915_private *dev_priv,
 				struct i915_power_well *power_well)
 {
-	skl_set_power_well(dev_priv, power_well, power_well->count > 0);
-
 	/* Clear any request made by BIOS as driver is taking over */
 	I915_WRITE(HSW_PWR_WELL_BIOS, 0);
 }
@@ -917,16 +913,6 @@ static bool bxt_dpio_cmn_power_well_enabled(struct drm_i915_private *dev_priv,
 	return bxt_ddi_phy_is_enabled(dev_priv, power_well->data);
 }
 
-static void bxt_dpio_cmn_power_well_sync_hw(struct drm_i915_private *dev_priv,
-					    struct i915_power_well *power_well)
-{
-	if (power_well->count > 0)
-		bxt_dpio_cmn_power_well_enable(dev_priv, power_well);
-	else
-		bxt_dpio_cmn_power_well_disable(dev_priv, power_well);
-}
-
-
 static void bxt_verify_ddi_phy_power_wells(struct drm_i915_private *dev_priv)
 {
 	struct i915_power_well *power_well;
@@ -989,13 +975,9 @@ static void gen9_dc_off_power_well_disable(struct drm_i915_private *dev_priv,
 		gen9_enable_dc5(dev_priv);
 }
 
-static void gen9_dc_off_power_well_sync_hw(struct drm_i915_private *dev_priv,
-					   struct i915_power_well *power_well)
+static void i9xx_power_well_sync_hw_noop(struct drm_i915_private *dev_priv,
+					 struct i915_power_well *power_well)
 {
-	if (power_well->count > 0)
-		gen9_dc_off_power_well_enable(dev_priv, power_well);
-	else
-		gen9_dc_off_power_well_disable(dev_priv, power_well);
 }
 
 static void i9xx_always_on_power_well_noop(struct drm_i915_private *dev_priv,
@@ -1045,12 +1027,6 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
 	mutex_unlock(&dev_priv->rps.hw_lock);
 }
 
-static void vlv_power_well_sync_hw(struct drm_i915_private *dev_priv,
-				   struct i915_power_well *power_well)
-{
-	vlv_set_power_well(dev_priv, power_well, power_well->count > 0);
-}
-
 static void vlv_power_well_enable(struct drm_i915_private *dev_priv,
 				  struct i915_power_well *power_well)
 {
@@ -1661,14 +1637,6 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
 	mutex_unlock(&dev_priv->rps.hw_lock);
 }
 
-static void chv_pipe_power_well_sync_hw(struct drm_i915_private *dev_priv,
-					struct i915_power_well *power_well)
-{
-	WARN_ON_ONCE(power_well->id != PIPE_A);
-
-	chv_set_pipe_power_well(dev_priv, power_well, power_well->count > 0);
-}
-
 static void chv_pipe_power_well_enable(struct drm_i915_private *dev_priv,
 				       struct i915_power_well *power_well)
 {
@@ -1914,21 +1882,21 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
 	BIT_ULL(POWER_DOMAIN_INIT))
 
 static const struct i915_power_well_ops i9xx_always_on_power_well_ops = {
-	.sync_hw = i9xx_always_on_power_well_noop,
+	.sync_hw = i9xx_power_well_sync_hw_noop,
 	.enable = i9xx_always_on_power_well_noop,
 	.disable = i9xx_always_on_power_well_noop,
 	.is_enabled = i9xx_always_on_power_well_enabled,
 };
 
 static const struct i915_power_well_ops chv_pipe_power_well_ops = {
-	.sync_hw = chv_pipe_power_well_sync_hw,
+	.sync_hw = i9xx_power_well_sync_hw_noop,
 	.enable = chv_pipe_power_well_enable,
 	.disable = chv_pipe_power_well_disable,
 	.is_enabled = chv_pipe_power_well_enabled,
 };
 
 static const struct i915_power_well_ops chv_dpio_cmn_power_well_ops = {
-	.sync_hw = vlv_power_well_sync_hw,
+	.sync_hw = i9xx_power_well_sync_hw_noop,
 	.enable = chv_dpio_cmn_power_well_enable,
 	.disable = chv_dpio_cmn_power_well_disable,
 	.is_enabled = vlv_power_well_enabled,
@@ -1958,14 +1926,14 @@ static const struct i915_power_well_ops skl_power_well_ops = {
 };
 
 static const struct i915_power_well_ops gen9_dc_off_power_well_ops = {
-	.sync_hw = gen9_dc_off_power_well_sync_hw,
+	.sync_hw = i9xx_power_well_sync_hw_noop,
 	.enable = gen9_dc_off_power_well_enable,
 	.disable = gen9_dc_off_power_well_disable,
 	.is_enabled = gen9_dc_off_power_well_enabled,
 };
 
 static const struct i915_power_well_ops bxt_dpio_cmn_power_well_ops = {
-	.sync_hw = bxt_dpio_cmn_power_well_sync_hw,
+	.sync_hw = i9xx_power_well_sync_hw_noop,
 	.enable = bxt_dpio_cmn_power_well_enable,
 	.disable = bxt_dpio_cmn_power_well_disable,
 	.is_enabled = bxt_dpio_cmn_power_well_enabled,
@@ -2000,21 +1968,21 @@ static struct i915_power_well bdw_power_wells[] = {
 };
 
 static const struct i915_power_well_ops vlv_display_power_well_ops = {
-	.sync_hw = vlv_power_well_sync_hw,
+	.sync_hw = i9xx_power_well_sync_hw_noop,
 	.enable = vlv_display_power_well_enable,
 	.disable = vlv_display_power_well_disable,
 	.is_enabled = vlv_power_well_enabled,
 };
 
 static const struct i915_power_well_ops vlv_dpio_cmn_power_well_ops = {
-	.sync_hw = vlv_power_well_sync_hw,
+	.sync_hw = i9xx_power_well_sync_hw_noop,
 	.enable = vlv_dpio_cmn_power_well_enable,
 	.disable = vlv_dpio_cmn_power_well_disable,
 	.is_enabled = vlv_power_well_enabled,
 };
 
 static const struct i915_power_well_ops vlv_dpio_power_well_ops = {
-	.sync_hw = vlv_power_well_sync_hw,
+	.sync_hw = i9xx_power_well_sync_hw_noop,
 	.enable = vlv_power_well_enable,
 	.disable = vlv_power_well_disable,
 	.is_enabled = vlv_power_well_enabled,
-- 
2.5.0

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

[PATCH 2/4] drm/i915: Call the sync_hw hook for power wells without a domain

[Imre Deak]

So far the sync_hw hook wasn't called for power wells not belonging to
any power domain, that is the GEN9 PW1 and MISC_IO power wells. This
wasn't a problem so far since the goal of the sync_hw hook - to clear
the corresponding BIOS request bit - was guaranteed by clearing the
whole BIOS request register elsewhere. This will change with the next
patch, so fix up the inconsistency.

Cc: Ander Conselvan de Oliveira <conselvan2@gmail.com>
Cc: David Weinehall <david.weinehall@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 34 +++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 0f64bc1..f9aff26 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -49,17 +49,24 @@
  * present for a given platform.
  */
 
-#define for_each_power_well(i, power_well, domain_mask, power_domains)	\
-	for (i = 0;							\
-	     i < (power_domains)->power_well_count &&			\
+#define for_each_power_well(i, power_well)				\
+	for ((i) = 0;							\
+	     (i) < (power_domains)->power_well_count &&			\
 		 ((power_well) = &(power_domains)->power_wells[i]);	\
-	     i++)							\
+	     (i)++)
+
+#define for_each_power_well_rev(i, power_well)				 \
+	for ((i) = (power_domains)->power_well_count - 1;                  \
+	     (i) >= 0 && ((power_well) = &(power_domains)->power_wells[i]);\
+	     (i)--)
+
+#define for_each_power_domain_well(i, power_well, domain_mask, power_domains) \
+	for_each_power_well(i, power_well)				\
 		for_each_if ((power_well)->domains & (domain_mask))
 
-#define for_each_power_well_rev(i, power_well, domain_mask, power_domains) \
-	for (i = (power_domains)->power_well_count - 1;			 \
-	     i >= 0 && ((power_well) = &(power_domains)->power_wells[i]);\
-	     i--)							 \
+#define for_each_power_domain_well_rev(i, power_well, domain_mask,	\
+				       power_domains)			\
+	for_each_power_well_rev(i, power_well)				\
 		for_each_if ((power_well)->domains & (domain_mask))
 
 bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv,
@@ -210,7 +217,8 @@ bool __intel_display_power_is_enabled(struct drm_i915_private *dev_priv,
 
 	is_enabled = true;
 
-	for_each_power_well_rev(i, power_well, BIT_ULL(domain), power_domains) {
+	for_each_power_domain_well_rev(i, power_well, BIT_ULL(domain),
+				       power_domains) {
 		if (power_well->always_on)
 			continue;
 
@@ -1665,7 +1673,8 @@ __intel_display_power_get_domain(struct drm_i915_private *dev_priv,
 	struct i915_power_well *power_well;
 	int i;
 
-	for_each_power_well(i, power_well, BIT_ULL(domain), power_domains)
+	for_each_power_domain_well(i, power_well, BIT_ULL(domain),
+				   power_domains)
 		intel_power_well_get(dev_priv, power_well);
 
 	power_domains->domain_use_count[domain]++;
@@ -1760,7 +1769,8 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
 	     intel_display_power_domain_str(domain));
 	power_domains->domain_use_count[domain]--;
 
-	for_each_power_well_rev(i, power_well, BIT_ULL(domain), power_domains)
+	for_each_power_domain_well_rev(i, power_well, BIT_ULL(domain),
+				       power_domains)
 		intel_power_well_put(dev_priv, power_well);
 
 	mutex_unlock(&power_domains->lock);
@@ -2427,7 +2437,7 @@ static void intel_power_domains_sync_hw(struct drm_i915_private *dev_priv)
 	int i;
 
 	mutex_lock(&power_domains->lock);
-	for_each_power_well(i, power_well, POWER_DOMAIN_MASK, power_domains) {
+	for_each_power_well(i, power_well) {
 		power_well->ops->sync_hw(dev_priv, power_well);
 		power_well->hw_enabled = power_well->ops->is_enabled(dev_priv,
 								     power_well);
-- 
2.5.0

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

[PATCH 3/4] drm/i915/gen9: Fix clearing of the BIOS power well request register

[Imre Deak]

Atm, in the power well sync_hw hook we are clearing all BIOS request
bits, not just the one corresponding to the given power well. This could
turn off an unrelated power well inadvertently if it didn't have a
request bit set in the driver request register.

This didn't cause a problem so far, since we enabled all power wells
explicitly before clearing the BIOS request register. A follow-up
patchset will add power wells that won't get enabled this way, so fix up
the inconsistency.

Cc: Ander Conselvan de Oliveira <conselvan2@gmail.com>
Cc: David Weinehall <david.weinehall@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index f9aff26..b7b0e0f 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -887,8 +887,13 @@ static bool skl_power_well_enabled(struct drm_i915_private *dev_priv,
 static void skl_power_well_sync_hw(struct drm_i915_private *dev_priv,
 				struct i915_power_well *power_well)
 {
+	uint32_t mask = SKL_POWER_WELL_REQ(power_well->id);
+	uint32_t bios_req = I915_READ(HSW_PWR_WELL_BIOS);
+
 	/* Clear any request made by BIOS as driver is taking over */
-	I915_WRITE(HSW_PWR_WELL_BIOS, 0);
+	if (bios_req & mask) {
+		I915_WRITE(HSW_PWR_WELL_BIOS, bios_req & ~mask);
+	}
 }
 
 static void skl_power_well_enable(struct drm_i915_private *dev_priv,
-- 
2.5.0

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

[PATCH 4/4] drm/i915: Preserve the state of power wells not explicitly enabled

[Imre Deak]

Atm, power wells that BIOS has enabled, but which we don't explicitly
enable during power domain initialization would get disabled as we clear
the BIOS request bit in the given power well sync_hw hook. To prevent
this copy over any set request bits in the BIOS request register to the
driver request register and clear the BIOS request bit only afterwards.

This doesn't make a difference now, since we enable all power wells
during power domain initialization. A follow-up patchset will add power
wells for which this isn't true, so fix up the inconsistency.

Cc: Ander Conselvan de Oliveira <conselvan2@gmail.com>
Cc: David Weinehall <david.weinehall@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index b7b0e0f..e747215 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -855,12 +855,14 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
 static void hsw_power_well_sync_hw(struct drm_i915_private *dev_priv,
 				   struct i915_power_well *power_well)
 {
-	/*
-	 * We're taking over the BIOS, so clear any requests made by it since
-	 * the driver is in charge now.
-	 */
-	if (I915_READ(HSW_PWR_WELL_BIOS) & HSW_PWR_WELL_ENABLE_REQUEST)
+	/* Take over the request bit if set by BIOS. */
+	if (I915_READ(HSW_PWR_WELL_BIOS) & HSW_PWR_WELL_ENABLE_REQUEST) {
+		if (!(I915_READ(HSW_PWR_WELL_DRIVER) &
+		      HSW_PWR_WELL_ENABLE_REQUEST))
+			I915_WRITE(HSW_PWR_WELL_DRIVER,
+				   HSW_PWR_WELL_ENABLE_REQUEST);
 		I915_WRITE(HSW_PWR_WELL_BIOS, 0);
+	}
 }
 
 static void hsw_power_well_enable(struct drm_i915_private *dev_priv,
@@ -890,8 +892,12 @@ static void skl_power_well_sync_hw(struct drm_i915_private *dev_priv,
 	uint32_t mask = SKL_POWER_WELL_REQ(power_well->id);
 	uint32_t bios_req = I915_READ(HSW_PWR_WELL_BIOS);
 
-	/* Clear any request made by BIOS as driver is taking over */
+	/* Take over the request bit if set by BIOS. */
 	if (bios_req & mask) {
+		uint32_t drv_req = I915_READ(HSW_PWR_WELL_DRIVER);
+
+		if (!(drv_req & mask))
+			I915_WRITE(HSW_PWR_WELL_DRIVER, drv_req | mask);
 		I915_WRITE(HSW_PWR_WELL_BIOS, bios_req & ~mask);
 	}
 }
-- 
2.5.0

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

✓ Fi.CI.BAT: success for drm/i915: Fix clearing of BIOS power well requests

[Patchwork]

== Series Details ==

Series: drm/i915: Fix clearing of BIOS power well requests
URL   : https://patchwork.freedesktop.org/series/19699/
State : success

== Summary ==

Series 19699v1 drm/i915: Fix clearing of BIOS power well requests
https://patchwork.freedesktop.org/api/1.0/series/19699/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-a:
                dmesg-fail -> PASS       (fi-snb-2520m)
        Subgroup nonblocking-crc-pipe-a-frame-sequence:
                dmesg-fail -> PASS       (fi-snb-2520m)
        Subgroup nonblocking-crc-pipe-b:
                dmesg-fail -> PASS       (fi-snb-2520m)
        Subgroup nonblocking-crc-pipe-b-frame-sequence:
                dmesg-fail -> PASS       (fi-snb-2520m)
        Subgroup read-crc-pipe-a:
                incomplete -> PASS       (fi-snb-2520m)

fi-bdw-5557u     total:252  pass:238  dwarn:3   dfail:0   fail:0   skip:11 
fi-bsw-n3050     total:252  pass:210  dwarn:3   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:252  pass:230  dwarn:3   dfail:0   fail:0   skip:19 
fi-bxt-t5700     total:83   pass:70   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:252  pass:222  dwarn:3   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:252  pass:218  dwarn:3   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:252  pass:233  dwarn:3   dfail:0   fail:0   skip:16 
fi-hsw-4770r     total:252  pass:233  dwarn:3   dfail:0   fail:0   skip:16 
fi-ilk-650       total:252  pass:199  dwarn:3   dfail:0   fail:0   skip:50 
fi-ivb-3520m     total:252  pass:231  dwarn:3   dfail:0   fail:0   skip:18 
fi-ivb-3770      total:252  pass:231  dwarn:3   dfail:0   fail:0   skip:18 
fi-kbl-7500u     total:252  pass:231  dwarn:3   dfail:0   fail:0   skip:18 
fi-skl-6260u     total:252  pass:239  dwarn:3   dfail:0   fail:0   skip:10 
fi-skl-6700hq    total:252  pass:232  dwarn:3   dfail:0   fail:0   skip:17 
fi-skl-6700k     total:252  pass:230  dwarn:4   dfail:0   fail:0   skip:18 
fi-skl-6770hq    total:252  pass:239  dwarn:3   dfail:0   fail:0   skip:10 
fi-snb-2520m     total:252  pass:221  dwarn:3   dfail:0   fail:0   skip:28 
fi-snb-2600      total:252  pass:220  dwarn:3   dfail:0   fail:0   skip:29 

cc11223a7f11b4e2d15f1c645326ac6f34568d88 drm-tip: 2017y-02m-15d-13h-44m-31s UTC integration manifest
480c3cf drm/i915: Preserve the state of power wells not explicitly enabled
0384629 drm/i915/gen9: Fix clearing of the BIOS power well request register
b379a27 drm/i915: Call the sync_hw hook for power wells without a domain
b78547c drm/i915: Remove redundant toggling from the power well sync_hw hooks

== Logs ==

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

Re: [PATCH 1/4] drm/i915: Remove redundant toggling from the power well sync_hw hooks

[Ander Conselvan De Oliveira]

On Wed, 2017-02-15 at 13:59 +0200, Imre Deak wrote:
> Doing an explicit enable/disable in the power well sync_hw hook based on
> the power well's reference count is redundant, since by the time these
> hooks are called all the power wells are enabled and have a reference.
> So remove the redundant toggling.
> 
> This is needed by a follow-up patchset that adds power wells which we
> can't enable/disable during power domain initialization and so want to
> preserve their state until modeset init time.
> 
> Cc: Ander Conselvan de Oliveira <conselvan2@gmail.com>
> Cc: David Weinehall <david.weinehall@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 52 +++++++-------------------------
> -
>  1 file changed, 10 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 8795679..0f64bc1 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -847,8 +847,6 @@ static void skl_set_power_well(struct drm_i915_private
> *dev_priv,
>  static void hsw_power_well_sync_hw(struct drm_i915_private *dev_priv,
>  				   struct i915_power_well *power_well)
>  {
> -	hsw_set_power_well(dev_priv, power_well, power_well->count > 0);
> -
>  	/*
>  	 * We're taking over the BIOS, so clear any requests made by it since
>  	 * the driver is in charge now.
> @@ -881,8 +879,6 @@ static bool skl_power_well_enabled(struct drm_i915_private
> *dev_priv,
>  static void skl_power_well_sync_hw(struct drm_i915_private *dev_priv,
>  				struct i915_power_well *power_well)
>  {
> -	skl_set_power_well(dev_priv, power_well, power_well->count > 0);
> -
>  	/* Clear any request made by BIOS as driver is taking over */
>  	I915_WRITE(HSW_PWR_WELL_BIOS, 0);
>  }
> @@ -917,16 +913,6 @@ static bool bxt_dpio_cmn_power_well_enabled(struct
> drm_i915_private *dev_priv,
>  	return bxt_ddi_phy_is_enabled(dev_priv, power_well->data);
>  }
>  
> -static void bxt_dpio_cmn_power_well_sync_hw(struct drm_i915_private
> *dev_priv,
> -					    struct i915_power_well
> *power_well)
> -{
> -	if (power_well->count > 0)
> -		bxt_dpio_cmn_power_well_enable(dev_priv, power_well);
> -	else
> -		bxt_dpio_cmn_power_well_disable(dev_priv, power_well);
> -}
> -
> -
>  static void bxt_verify_ddi_phy_power_wells(struct drm_i915_private *dev_priv)
>  {
>  	struct i915_power_well *power_well;
> @@ -989,13 +975,9 @@ static void gen9_dc_off_power_well_disable(struct
> drm_i915_private *dev_priv,
>  		gen9_enable_dc5(dev_priv);
>  }
>  
> -static void gen9_dc_off_power_well_sync_hw(struct drm_i915_private *dev_priv,
> -					   struct i915_power_well
> *power_well)
> +static void i9xx_power_well_sync_hw_noop(struct drm_i915_private *dev_priv,
> +					 struct i915_power_well *power_well)
>  {
> -	if (power_well->count > 0)
> -		gen9_dc_off_power_well_enable(dev_priv, power_well);
> -	else
> -		gen9_dc_off_power_well_disable(dev_priv, power_well);
>  }
>  
>  static void i9xx_always_on_power_well_noop(struct drm_i915_private *dev_priv,
> @@ -1045,12 +1027,6 @@ static void vlv_set_power_well(struct drm_i915_private
> *dev_priv,
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  }
>  
> -static void vlv_power_well_sync_hw(struct drm_i915_private *dev_priv,
> -				   struct i915_power_well *power_well)
> -{
> -	vlv_set_power_well(dev_priv, power_well, power_well->count > 0);
> -}
> -
>  static void vlv_power_well_enable(struct drm_i915_private *dev_priv,
>  				  struct i915_power_well *power_well)
>  {
> @@ -1661,14 +1637,6 @@ static void chv_set_pipe_power_well(struct
> drm_i915_private *dev_priv,
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  }
>  
> -static void chv_pipe_power_well_sync_hw(struct drm_i915_private *dev_priv,
> -					struct i915_power_well *power_well)
> -{
> -	WARN_ON_ONCE(power_well->id != PIPE_A);
> -
> -	chv_set_pipe_power_well(dev_priv, power_well, power_well->count > 0);
> -}
> -
>  static void chv_pipe_power_well_enable(struct drm_i915_private *dev_priv,
>  				       struct i915_power_well *power_well)
>  {
> @@ -1914,21 +1882,21 @@ void intel_display_power_put(struct drm_i915_private
> *dev_priv,
>  	BIT_ULL(POWER_DOMAIN_INIT))
>  
>  static const struct i915_power_well_ops i9xx_always_on_power_well_ops = {
> -	.sync_hw = i9xx_always_on_power_well_noop,
> +	.sync_hw = i9xx_power_well_sync_hw_noop,
>  	.enable = i9xx_always_on_power_well_noop,
>  	.disable = i9xx_always_on_power_well_noop,
>  	.is_enabled = i9xx_always_on_power_well_enabled,
>  };
>  
>  static const struct i915_power_well_ops chv_pipe_power_well_ops = {
> -	.sync_hw = chv_pipe_power_well_sync_hw,
> +	.sync_hw = i9xx_power_well_sync_hw_noop,
>  	.enable = chv_pipe_power_well_enable,
>  	.disable = chv_pipe_power_well_disable,
>  	.is_enabled = chv_pipe_power_well_enabled,
>  };
>  
>  static const struct i915_power_well_ops chv_dpio_cmn_power_well_ops = {
> -	.sync_hw = vlv_power_well_sync_hw,
> +	.sync_hw = i9xx_power_well_sync_hw_noop,
>  	.enable = chv_dpio_cmn_power_well_enable,
>  	.disable = chv_dpio_cmn_power_well_disable,
>  	.is_enabled = vlv_power_well_enabled,
> @@ -1958,14 +1926,14 @@ static const struct i915_power_well_ops
> skl_power_well_ops = {
>  };
>  
>  static const struct i915_power_well_ops gen9_dc_off_power_well_ops = {
> -	.sync_hw = gen9_dc_off_power_well_sync_hw,
> +	.sync_hw = i9xx_power_well_sync_hw_noop,
>  	.enable = gen9_dc_off_power_well_enable,
>  	.disable = gen9_dc_off_power_well_disable,
>  	.is_enabled = gen9_dc_off_power_well_enabled,
>  };
>  
>  static const struct i915_power_well_ops bxt_dpio_cmn_power_well_ops = {
> -	.sync_hw = bxt_dpio_cmn_power_well_sync_hw,
> +	.sync_hw = i9xx_power_well_sync_hw_noop,
>  	.enable = bxt_dpio_cmn_power_well_enable,
>  	.disable = bxt_dpio_cmn_power_well_disable,
>  	.is_enabled = bxt_dpio_cmn_power_well_enabled,
> @@ -2000,21 +1968,21 @@ static struct i915_power_well bdw_power_wells[] = {
>  };
>  
>  static const struct i915_power_well_ops vlv_display_power_well_ops = {
> -	.sync_hw = vlv_power_well_sync_hw,
> +	.sync_hw = i9xx_power_well_sync_hw_noop,
>  	.enable = vlv_display_power_well_enable,
>  	.disable = vlv_display_power_well_disable,
>  	.is_enabled = vlv_power_well_enabled,
>  };
>  
>  static const struct i915_power_well_ops vlv_dpio_cmn_power_well_ops = {
> -	.sync_hw = vlv_power_well_sync_hw,
> +	.sync_hw = i9xx_power_well_sync_hw_noop,
>  	.enable = vlv_dpio_cmn_power_well_enable,
>  	.disable = vlv_dpio_cmn_power_well_disable,
>  	.is_enabled = vlv_power_well_enabled,
>  };
>  
>  static const struct i915_power_well_ops vlv_dpio_power_well_ops = {
> -	.sync_hw = vlv_power_well_sync_hw,
> +	.sync_hw = i9xx_power_well_sync_hw_noop,
>  	.enable = vlv_power_well_enable,
>  	.disable = vlv_power_well_disable,
>  	.is_enabled = vlv_power_well_enabled,
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915: Call the sync_hw hook for power wells without a domain

[Ander Conselvan De Oliveira]

On Wed, 2017-02-15 at 13:59 +0200, Imre Deak wrote:
> So far the sync_hw hook wasn't called for power wells not belonging to
> any power domain, that is the GEN9 PW1 and MISC_IO power wells. This
> wasn't a problem so far since the goal of the sync_hw hook - to clear
> the corresponding BIOS request bit - was guaranteed by clearing the
> whole BIOS request register elsewhere. This will change with the next
> patch, so fix up the inconsistency.
> 
> Cc: Ander Conselvan de Oliveira <conselvan2@gmail.com>
> Cc: David Weinehall <david.weinehall@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 34 +++++++++++++++++++++------------
>  1 file changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 0f64bc1..f9aff26 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -49,17 +49,24 @@
>   * present for a given platform.
>   */
>  
> -#define for_each_power_well(i, power_well, domain_mask, power_domains)	\
> -	for (i = 0;							\
> -	     i < (power_domains)->power_well_count &&			\
> +#define for_each_power_well(i, power_well)				\
> +	for ((i) = 0;							\
> +	     (i) < (power_domains)->power_well_count &&			\

This now requires that power_domains is in scope to work properly. Would it make
sense to still pass that as argument to the macro or, alternatively, pass
dev_priv? Could probably nuke the i parameter too, since no caller uses it?

But either way,

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

>  		 ((power_well) = &(power_domains)->power_wells[i]);	\
> -	     i++)							\
> +	     (i)++)
> +
> +#define for_each_power_well_rev(i, power_well)				 \
> +	for ((i) = (power_domains)->power_well_count - 1;                  \
> +	     (i) >= 0 && ((power_well) = &(power_domains)->power_wells[i]);\
> +	     (i)--)
> +
> +#define for_each_power_domain_well(i, power_well, domain_mask, power_domains) \
> +	for_each_power_well(i, power_well)				\
>  		for_each_if ((power_well)->domains & (domain_mask))
>  
> -#define for_each_power_well_rev(i, power_well, domain_mask, power_domains) \
> -	for (i = (power_domains)->power_well_count - 1;			 \
> -	     i >= 0 && ((power_well) = &(power_domains)->power_wells[i]);\
> -	     i--)							 \
> +#define for_each_power_domain_well_rev(i, power_well, domain_mask,	\
> +				       power_domains)			\
> +	for_each_power_well_rev(i, power_well)				\
>  		for_each_if ((power_well)->domains & (domain_mask))
>  
>  bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv,
> @@ -210,7 +217,8 @@ bool __intel_display_power_is_enabled(struct drm_i915_private *dev_priv,
>  
>  	is_enabled = true;
>  
> -	for_each_power_well_rev(i, power_well, BIT_ULL(domain), power_domains) {
> +	for_each_power_domain_well_rev(i, power_well, BIT_ULL(domain),
> +				       power_domains) {
>  		if (power_well->always_on)
>  			continue;
>  
> @@ -1665,7 +1673,8 @@ __intel_display_power_get_domain(struct drm_i915_private *dev_priv,
>  	struct i915_power_well *power_well;
>  	int i;
>  
> -	for_each_power_well(i, power_well, BIT_ULL(domain), power_domains)
> +	for_each_power_domain_well(i, power_well, BIT_ULL(domain),
> +				   power_domains)
>  		intel_power_well_get(dev_priv, power_well);
>  
>  	power_domains->domain_use_count[domain]++;
> @@ -1760,7 +1769,8 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>  	     intel_display_power_domain_str(domain));
>  	power_domains->domain_use_count[domain]--;
>  
> -	for_each_power_well_rev(i, power_well, BIT_ULL(domain), power_domains)
> +	for_each_power_domain_well_rev(i, power_well, BIT_ULL(domain),
> +				       power_domains)
>  		intel_power_well_put(dev_priv, power_well);
>  
>  	mutex_unlock(&power_domains->lock);
> @@ -2427,7 +2437,7 @@ static void intel_power_domains_sync_hw(struct drm_i915_private *dev_priv)
>  	int i;
>  
>  	mutex_lock(&power_domains->lock);
> -	for_each_power_well(i, power_well, POWER_DOMAIN_MASK, power_domains) {
> +	for_each_power_well(i, power_well) {
>  		power_well->ops->sync_hw(dev_priv, power_well);
>  		power_well->hw_enabled = power_well->ops->is_enabled(dev_priv,
>  								     power_well);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915/gen9: Fix clearing of the BIOS power well request register

[Ander Conselvan De Oliveira]

On Wed, 2017-02-15 at 13:59 +0200, Imre Deak wrote:
> Atm, in the power well sync_hw hook we are clearing all BIOS request
> bits, not just the one corresponding to the given power well. This could
> turn off an unrelated power well inadvertently if it didn't have a
> request bit set in the driver request register.
> 
> This didn't cause a problem so far, since we enabled all power wells
> explicitly before clearing the BIOS request register. A follow-up
> patchset will add power wells that won't get enabled this way, so fix up
> the inconsistency.
> 
> Cc: Ander Conselvan de Oliveira <conselvan2@gmail.com>
> Cc: David Weinehall <david.weinehall@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index f9aff26..b7b0e0f 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -887,8 +887,13 @@ static bool skl_power_well_enabled(struct drm_i915_private *dev_priv,
>  static void skl_power_well_sync_hw(struct drm_i915_private *dev_priv,
>  				struct i915_power_well *power_well)
>  {
> +	uint32_t mask = SKL_POWER_WELL_REQ(power_well->id);
> +	uint32_t bios_req = I915_READ(HSW_PWR_WELL_BIOS);
> +
>  	/* Clear any request made by BIOS as driver is taking over */
> -	I915_WRITE(HSW_PWR_WELL_BIOS, 0);
> +	if (bios_req & mask) {
> +		I915_WRITE(HSW_PWR_WELL_BIOS, bios_req & ~mask);
> +	}
>  }
>  
>  static void skl_power_well_enable(struct drm_i915_private *dev_priv,

With this change we still end up disabling every power well from the BIOS
register because of intel_power_domains_sync_hw() iterating over all the power
wells. Maybe that's worth mentioning in the commit message? Dunno.

Anyway,

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

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

Re: [PATCH 4/4] drm/i915: Preserve the state of power wells not explicitly enabled

[Ander Conselvan De Oliveira]

On Wed, 2017-02-15 at 13:59 +0200, Imre Deak wrote:
> Atm, power wells that BIOS has enabled, but which we don't explicitly
> enable during power domain initialization would get disabled as we clear
> the BIOS request bit in the given power well sync_hw hook. To prevent
> this copy over any set request bits in the BIOS request register to the
> driver request register and clear the BIOS request bit only afterwards.
> 
> This doesn't make a difference now, since we enable all power wells
> during power domain initialization. A follow-up patchset will add power
> wells for which this isn't true, so fix up the inconsistency.
> 
> Cc: Ander Conselvan de Oliveira <conselvan2@gmail.com>
> Cc: David Weinehall <david.weinehall@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index b7b0e0f..e747215 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -855,12 +855,14 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>  static void hsw_power_well_sync_hw(struct drm_i915_private *dev_priv,
>  				   struct i915_power_well *power_well)
>  {
> -	/*
> -	 * We're taking over the BIOS, so clear any requests made by it since
> -	 * the driver is in charge now.
> -	 */
> -	if (I915_READ(HSW_PWR_WELL_BIOS) & HSW_PWR_WELL_ENABLE_REQUEST)
> +	/* Take over the request bit if set by BIOS. */
> +	if (I915_READ(HSW_PWR_WELL_BIOS) & HSW_PWR_WELL_ENABLE_REQUEST) {
> +		if (!(I915_READ(HSW_PWR_WELL_DRIVER) &
> +		      HSW_PWR_WELL_ENABLE_REQUEST))
> +			I915_WRITE(HSW_PWR_WELL_DRIVER,
> +				   HSW_PWR_WELL_ENABLE_REQUEST);
>  		I915_WRITE(HSW_PWR_WELL_BIOS, 0);
> +	}
>  }
>  
>  static void hsw_power_well_enable(struct drm_i915_private *dev_priv,
> @@ -890,8 +892,12 @@ static void skl_power_well_sync_hw(struct drm_i915_private *dev_priv,
>  	uint32_t mask = SKL_POWER_WELL_REQ(power_well->id);
>  	uint32_t bios_req = I915_READ(HSW_PWR_WELL_BIOS);
>  
> -	/* Clear any request made by BIOS as driver is taking over */
> +	/* Take over the request bit if set by BIOS. */
>  	if (bios_req & mask) {
> +		uint32_t drv_req = I915_READ(HSW_PWR_WELL_DRIVER);
> +
> +		if (!(drv_req & mask))
> +			I915_WRITE(HSW_PWR_WELL_DRIVER, drv_req | mask);
>  		I915_WRITE(HSW_PWR_WELL_BIOS, bios_req & ~mask);
>  	}
>  }

With this change it will be possible for the hardware state readout code to
inherit the state of the DDI IO power wells in Geminilake, which is what I
wanted. I'm just thinking if there is a risk for inconsistency if the hardware
state readout doesn't fix the reference count to a domain that is left enabled.
Would it make sense to add a WARN or at least a debug message somewhere if this
happens, so it doesn't go unnoticed?

Ander

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

Re: [PATCH 4/4] drm/i915: Preserve the state of power wells not explicitly enabled

[Imre Deak]

On Thu, Feb 16, 2017 at 02:18:53PM +0200, Ander Conselvan De Oliveira wrote:
> On Wed, 2017-02-15 at 13:59 +0200, Imre Deak wrote:
> > Atm, power wells that BIOS has enabled, but which we don't explicitly
> > enable during power domain initialization would get disabled as we clear
> > the BIOS request bit in the given power well sync_hw hook. To prevent
> > this copy over any set request bits in the BIOS request register to the
> > driver request register and clear the BIOS request bit only afterwards.
> > 
> > This doesn't make a difference now, since we enable all power wells
> > during power domain initialization. A follow-up patchset will add power
> > wells for which this isn't true, so fix up the inconsistency.
> > 
> > Cc: Ander Conselvan de Oliveira <conselvan2@gmail.com>
> > Cc: David Weinehall <david.weinehall@linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index b7b0e0f..e747215 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -855,12 +855,14 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> >  static void hsw_power_well_sync_hw(struct drm_i915_private *dev_priv,
> >  				   struct i915_power_well *power_well)
> >  {
> > -	/*
> > -	 * We're taking over the BIOS, so clear any requests made by it since
> > -	 * the driver is in charge now.
> > -	 */
> > -	if (I915_READ(HSW_PWR_WELL_BIOS) & HSW_PWR_WELL_ENABLE_REQUEST)
> > +	/* Take over the request bit if set by BIOS. */
> > +	if (I915_READ(HSW_PWR_WELL_BIOS) & HSW_PWR_WELL_ENABLE_REQUEST) {
> > +		if (!(I915_READ(HSW_PWR_WELL_DRIVER) &
> > +		      HSW_PWR_WELL_ENABLE_REQUEST))
> > +			I915_WRITE(HSW_PWR_WELL_DRIVER,
> > +				   HSW_PWR_WELL_ENABLE_REQUEST);
> >  		I915_WRITE(HSW_PWR_WELL_BIOS, 0);
> > +	}
> >  }
> >  
> >  static void hsw_power_well_enable(struct drm_i915_private *dev_priv,
> > @@ -890,8 +892,12 @@ static void skl_power_well_sync_hw(struct drm_i915_private *dev_priv,
> >  	uint32_t mask = SKL_POWER_WELL_REQ(power_well->id);
> >  	uint32_t bios_req = I915_READ(HSW_PWR_WELL_BIOS);
> >  
> > -	/* Clear any request made by BIOS as driver is taking over */
> > +	/* Take over the request bit if set by BIOS. */
> >  	if (bios_req & mask) {
> > +		uint32_t drv_req = I915_READ(HSW_PWR_WELL_DRIVER);
> > +
> > +		if (!(drv_req & mask))
> > +			I915_WRITE(HSW_PWR_WELL_DRIVER, drv_req | mask);
> >  		I915_WRITE(HSW_PWR_WELL_BIOS, bios_req & ~mask);
> >  	}
> >  }
> 
> With this change it will be possible for the hardware state readout code to
> inherit the state of the DDI IO power wells in Geminilake, which is what I
> wanted. I'm just thinking if there is a risk for inconsistency if the hardware
> state readout doesn't fix the reference count to a domain that is left enabled.
> Would it make sense to add a WARN or at least a debug message somewhere if this
> happens, so it doesn't go unnoticed?

Yes, good idea. This requires then that power_well->count == power_well->hw_enabled
for all power wells at the end of modeset readout. I'll add that.

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

Re: [PATCH 2/4] drm/i915: Call the sync_hw hook for power wells without a domain

[Imre Deak]

On Thu, Feb 16, 2017 at 11:31:01AM +0200, Ander Conselvan De Oliveira wrote:
> On Wed, 2017-02-15 at 13:59 +0200, Imre Deak wrote:
> > So far the sync_hw hook wasn't called for power wells not belonging to
> > any power domain, that is the GEN9 PW1 and MISC_IO power wells. This
> > wasn't a problem so far since the goal of the sync_hw hook - to clear
> > the corresponding BIOS request bit - was guaranteed by clearing the
> > whole BIOS request register elsewhere. This will change with the next
> > patch, so fix up the inconsistency.
> > 
> > Cc: Ander Conselvan de Oliveira <conselvan2@gmail.com>
> > Cc: David Weinehall <david.weinehall@linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 34 +++++++++++++++++++++------------
> >  1 file changed, 22 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 0f64bc1..f9aff26 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -49,17 +49,24 @@
> >   * present for a given platform.
> >   */
> >  
> > -#define for_each_power_well(i, power_well, domain_mask, power_domains)	\
> > -	for (i = 0;							\
> > -	     i < (power_domains)->power_well_count &&			\
> > +#define for_each_power_well(i, power_well)				\
> > +	for ((i) = 0;							\
> > +	     (i) < (power_domains)->power_well_count &&			\
> 
> This now requires that power_domains is in scope to work properly. Would it make
> sense to still pass that as argument to the macro or, alternatively, pass
> dev_priv?

Yes, that was a copy/paste fail. Yep, dev_priv simplifies things, will
change to that.

> Could probably nuke the i parameter too, since no caller uses it?

Ok. I also moved these to i915_dev.h. I'll resend the patchset after a
trybot round.

> 
> But either way,
> 
> Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

Thanks,
Imre

> 
> >  		 ((power_well) = &(power_domains)->power_wells[i]);	\
> > -	     i++)							\
> > +	     (i)++)
> > +
> > +#define for_each_power_well_rev(i, power_well)				 \
> > +	for ((i) = (power_domains)->power_well_count - 1;                  \
> > +	     (i) >= 0 && ((power_well) = &(power_domains)->power_wells[i]);\
> > +	     (i)--)
> > +
> > +#define for_each_power_domain_well(i, power_well, domain_mask, power_domains) \
> > +	for_each_power_well(i, power_well)				\
> >  		for_each_if ((power_well)->domains & (domain_mask))
> >  
> > -#define for_each_power_well_rev(i, power_well, domain_mask, power_domains) \
> > -	for (i = (power_domains)->power_well_count - 1;			 \
> > -	     i >= 0 && ((power_well) = &(power_domains)->power_wells[i]);\
> > -	     i--)							 \
> > +#define for_each_power_domain_well_rev(i, power_well, domain_mask,	\
> > +				       power_domains)			\
> > +	for_each_power_well_rev(i, power_well)				\
> >  		for_each_if ((power_well)->domains & (domain_mask))
> >  
> >  bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv,
> > @@ -210,7 +217,8 @@ bool __intel_display_power_is_enabled(struct drm_i915_private *dev_priv,
> >  
> >  	is_enabled = true;
> >  
> > -	for_each_power_well_rev(i, power_well, BIT_ULL(domain), power_domains) {
> > +	for_each_power_domain_well_rev(i, power_well, BIT_ULL(domain),
> > +				       power_domains) {
> >  		if (power_well->always_on)
> >  			continue;
> >  
> > @@ -1665,7 +1673,8 @@ __intel_display_power_get_domain(struct drm_i915_private *dev_priv,
> >  	struct i915_power_well *power_well;
> >  	int i;
> >  
> > -	for_each_power_well(i, power_well, BIT_ULL(domain), power_domains)
> > +	for_each_power_domain_well(i, power_well, BIT_ULL(domain),
> > +				   power_domains)
> >  		intel_power_well_get(dev_priv, power_well);
> >  
> >  	power_domains->domain_use_count[domain]++;
> > @@ -1760,7 +1769,8 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> >  	     intel_display_power_domain_str(domain));
> >  	power_domains->domain_use_count[domain]--;
> >  
> > -	for_each_power_well_rev(i, power_well, BIT_ULL(domain), power_domains)
> > +	for_each_power_domain_well_rev(i, power_well, BIT_ULL(domain),
> > +				       power_domains)
> >  		intel_power_well_put(dev_priv, power_well);
> >  
> >  	mutex_unlock(&power_domains->lock);
> > @@ -2427,7 +2437,7 @@ static void intel_power_domains_sync_hw(struct drm_i915_private *dev_priv)
> >  	int i;
> >  
> >  	mutex_lock(&power_domains->lock);
> > -	for_each_power_well(i, power_well, POWER_DOMAIN_MASK, power_domains) {
> > +	for_each_power_well(i, power_well) {
> >  		power_well->ops->sync_hw(dev_priv, power_well);
> >  		power_well->hw_enabled = power_well->ops->is_enabled(dev_priv,
> >  								     power_well);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915/gen9: Fix clearing of the BIOS power well request register

[Imre Deak]

On Thu, Feb 16, 2017 at 11:43:06AM +0200, Ander Conselvan De Oliveira wrote:
> On Wed, 2017-02-15 at 13:59 +0200, Imre Deak wrote:
> > Atm, in the power well sync_hw hook we are clearing all BIOS request
> > bits, not just the one corresponding to the given power well. This could
> > turn off an unrelated power well inadvertently if it didn't have a
> > request bit set in the driver request register.
> > 
> > This didn't cause a problem so far, since we enabled all power wells
> > explicitly before clearing the BIOS request register. A follow-up
> > patchset will add power wells that won't get enabled this way, so fix up
> > the inconsistency.
> > 
> > Cc: Ander Conselvan de Oliveira <conselvan2@gmail.com>
> > Cc: David Weinehall <david.weinehall@linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index f9aff26..b7b0e0f 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -887,8 +887,13 @@ static bool skl_power_well_enabled(struct drm_i915_private *dev_priv,
> >  static void skl_power_well_sync_hw(struct drm_i915_private *dev_priv,
> >  				struct i915_power_well *power_well)
> >  {
> > +	uint32_t mask = SKL_POWER_WELL_REQ(power_well->id);
> > +	uint32_t bios_req = I915_READ(HSW_PWR_WELL_BIOS);
> > +
> >  	/* Clear any request made by BIOS as driver is taking over */
> > -	I915_WRITE(HSW_PWR_WELL_BIOS, 0);
> > +	if (bios_req & mask) {
> > +		I915_WRITE(HSW_PWR_WELL_BIOS, bios_req & ~mask);
> > +	}
> >  }
> >  
> >  static void skl_power_well_enable(struct drm_i915_private *dev_priv,
> 
> With this change we still end up disabling every power well from the BIOS
> register because of intel_power_domains_sync_hw() iterating over all the power
> wells. 

Yes, that's what is expected. This patch just changes how this clearing
is done, the next one makes sure we preserve the state for any power
well without a reference.

> Maybe that's worth mentioning in the commit message? Dunno.

Ok, will add something.

> Anyway,
> 
> Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx