* [PATCH 1/4] drm/i915: Remove redundant toggling from the power well sync_hw hooks
2017-02-15 11:59 [PATCH 0/4] drm/i915: Fix clearing of BIOS power well requests Imre Deak
@ 2017-02-15 11:59 ` Imre Deak
2017-02-16 9:13 ` Ander Conselvan De Oliveira
2017-02-15 11:59 ` [PATCH 2/4] drm/i915: Call the sync_hw hook for power wells without a domain Imre Deak
` (3 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Imre Deak @ 2017-02-15 11:59 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 1/4] drm/i915: Remove redundant toggling from the power well sync_hw hooks
2017-02-15 11:59 ` [PATCH 1/4] drm/i915: Remove redundant toggling from the power well sync_hw hooks Imre Deak
@ 2017-02-16 9:13 ` Ander Conselvan De Oliveira
0 siblings, 0 replies; 13+ messages in thread
From: Ander Conselvan De Oliveira @ 2017-02-16 9:13 UTC (permalink / raw)
To: Imre Deak, intel-gfx
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/4] drm/i915: Call the sync_hw hook for power wells without a domain
2017-02-15 11:59 [PATCH 0/4] drm/i915: Fix clearing of BIOS power well requests Imre Deak
2017-02-15 11:59 ` [PATCH 1/4] drm/i915: Remove redundant toggling from the power well sync_hw hooks Imre Deak
@ 2017-02-15 11:59 ` Imre Deak
2017-02-16 9:31 ` Ander Conselvan De Oliveira
2017-02-15 11:59 ` [PATCH 3/4] drm/i915/gen9: Fix clearing of the BIOS power well request register Imre Deak
` (2 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Imre Deak @ 2017-02-15 11:59 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 2/4] drm/i915: Call the sync_hw hook for power wells without a domain
2017-02-15 11:59 ` [PATCH 2/4] drm/i915: Call the sync_hw hook for power wells without a domain Imre Deak
@ 2017-02-16 9:31 ` Ander Conselvan De Oliveira
2017-02-16 14:55 ` Imre Deak
0 siblings, 1 reply; 13+ messages in thread
From: Ander Conselvan De Oliveira @ 2017-02-16 9:31 UTC (permalink / raw)
To: Imre Deak, intel-gfx
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 2/4] drm/i915: Call the sync_hw hook for power wells without a domain
2017-02-16 9:31 ` Ander Conselvan De Oliveira
@ 2017-02-16 14:55 ` Imre Deak
0 siblings, 0 replies; 13+ messages in thread
From: Imre Deak @ 2017-02-16 14:55 UTC (permalink / raw)
To: Ander Conselvan De Oliveira; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/4] drm/i915/gen9: Fix clearing of the BIOS power well request register
2017-02-15 11:59 [PATCH 0/4] drm/i915: Fix clearing of BIOS power well requests Imre Deak
2017-02-15 11:59 ` [PATCH 1/4] drm/i915: Remove redundant toggling from the power well sync_hw hooks Imre Deak
2017-02-15 11:59 ` [PATCH 2/4] drm/i915: Call the sync_hw hook for power wells without a domain Imre Deak
@ 2017-02-15 11:59 ` Imre Deak
2017-02-16 9:43 ` Ander Conselvan De Oliveira
2017-02-15 11:59 ` [PATCH 4/4] drm/i915: Preserve the state of power wells not explicitly enabled Imre Deak
2017-02-15 14:52 ` ✓ Fi.CI.BAT: success for drm/i915: Fix clearing of BIOS power well requests Patchwork
4 siblings, 1 reply; 13+ messages in thread
From: Imre Deak @ 2017-02-15 11:59 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 3/4] drm/i915/gen9: Fix clearing of the BIOS power well request register
2017-02-15 11:59 ` [PATCH 3/4] drm/i915/gen9: Fix clearing of the BIOS power well request register Imre Deak
@ 2017-02-16 9:43 ` Ander Conselvan De Oliveira
2017-02-16 15:13 ` Imre Deak
0 siblings, 1 reply; 13+ messages in thread
From: Ander Conselvan De Oliveira @ 2017-02-16 9:43 UTC (permalink / raw)
To: Imre Deak, intel-gfx
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 3/4] drm/i915/gen9: Fix clearing of the BIOS power well request register
2017-02-16 9:43 ` Ander Conselvan De Oliveira
@ 2017-02-16 15:13 ` Imre Deak
0 siblings, 0 replies; 13+ messages in thread
From: Imre Deak @ 2017-02-16 15:13 UTC (permalink / raw)
To: Ander Conselvan De Oliveira; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 4/4] drm/i915: Preserve the state of power wells not explicitly enabled
2017-02-15 11:59 [PATCH 0/4] drm/i915: Fix clearing of BIOS power well requests Imre Deak
` (2 preceding siblings ...)
2017-02-15 11:59 ` [PATCH 3/4] drm/i915/gen9: Fix clearing of the BIOS power well request register Imre Deak
@ 2017-02-15 11:59 ` Imre Deak
2017-02-16 12:18 ` Ander Conselvan De Oliveira
2017-02-15 14:52 ` ✓ Fi.CI.BAT: success for drm/i915: Fix clearing of BIOS power well requests Patchwork
4 siblings, 1 reply; 13+ messages in thread
From: Imre Deak @ 2017-02-15 11:59 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 4/4] drm/i915: Preserve the state of power wells not explicitly enabled
2017-02-15 11:59 ` [PATCH 4/4] drm/i915: Preserve the state of power wells not explicitly enabled Imre Deak
@ 2017-02-16 12:18 ` Ander Conselvan De Oliveira
2017-02-16 12:41 ` Imre Deak
0 siblings, 1 reply; 13+ messages in thread
From: Ander Conselvan De Oliveira @ 2017-02-16 12:18 UTC (permalink / raw)
To: Imre Deak, intel-gfx
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 4/4] drm/i915: Preserve the state of power wells not explicitly enabled
2017-02-16 12:18 ` Ander Conselvan De Oliveira
@ 2017-02-16 12:41 ` Imre Deak
0 siblings, 0 replies; 13+ messages in thread
From: Imre Deak @ 2017-02-16 12:41 UTC (permalink / raw)
To: Ander Conselvan De Oliveira; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Fix clearing of BIOS power well requests
2017-02-15 11:59 [PATCH 0/4] drm/i915: Fix clearing of BIOS power well requests Imre Deak
` (3 preceding siblings ...)
2017-02-15 11:59 ` [PATCH 4/4] drm/i915: Preserve the state of power wells not explicitly enabled Imre Deak
@ 2017-02-15 14:52 ` Patchwork
4 siblings, 0 replies; 13+ messages in thread
From: Patchwork @ 2017-02-15 14:52 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx
== 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
^ permalink raw reply [flat|nested] 13+ messages in thread