[PATCH 00/12] drm/i915: add missing display power refs

[PATCH 00/12] drm/i915: add missing display power refs

[Imre Deak]

This patchset adds missing display power domain references during
modeset HW readout, which I noticed via wakeref warnings the
corresponding HW accesses triggered. The crt and ddi patches have a
concrete bugzilla reference they fix, I don't know of reports that would
be fixed by the rest of the patches in the set. On the way I also
noticed other places which are potentially racy (debugfs CRC, VGA
 disabling, pipe_assert) so I fixed those up too.

I left the IRQ setup and error capture code as-is: the former happens
during driver loading and resume, so the power is guaranteed to stay on
for the whole sequence. For error capture, we decided early on that we
won't add any locking around these accesses to minimize the chance for
locking inversions. Also for this we would need to grab a mutex which
isn't possible on the error capture path.

Mika came up with a very similar fix, since he suspects that it could
also be related to recent DMC problems. We agreed that I send mine since
it uses the more optimal rpm_get_if_in_use() helper (and looks more
polished).

Imre Deak (12):
  drm/i915: add helper to get a display power ref if it was already
    enabled
  drm/i915: ensure the HW is powered during display pipe HW readout
  drm/i915/ibx: ensure the HW is powered during PLL HW readout
  drm/i915: ensure the HW is powered when disabling VGA
  drm/i915: ensure the HW is powered during HW access in assert_pipe
  drm/i915/crt: ensure the HW is powered during HW state readout
  drm/i915/ddi: ensure the HW is powered during HW state readout
  drm/i915: ensure the HW is powered when accessing the CRC HW block
  drm/i915/dp: ensure the HW is powered during HW state readout
  drm/i915/dsi: ensure the HW is powered during HW state readout
  drm/i915/hdmi: ensure the HW is powered during HW state readout
  drm/i915/lvds: ensure the HW is powered during HW state readout

 drivers/gpu/drm/i915/i915_debugfs.c     |  28 ++++++--
 drivers/gpu/drm/i915/intel_crt.c        |  13 +++-
 drivers/gpu/drm/i915/intel_ddi.c        | 116 ++++++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_display.c    |  86 ++++++++++++++++-------
 drivers/gpu/drm/i915/intel_dp.c         |  18 +++--
 drivers/gpu/drm/i915/intel_drv.h        |   3 +
 drivers/gpu/drm/i915/intel_dsi.c        |  13 +++-
 drivers/gpu/drm/i915/intel_hdmi.c       |  14 +++-
 drivers/gpu/drm/i915/intel_lvds.c       |  14 +++-
 drivers/gpu/drm/i915/intel_runtime_pm.c | 111 +++++++++++++++++++++++++-----
 10 files changed, 315 insertions(+), 101 deletions(-)

-- 
2.5.0

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

[PATCH 01/12] drm/i915: add helper to get a display power ref if it was already enabled

[Imre Deak]

We have many places in the code where we check if a given display power
domain is enabled and if so access registers backed by this power
domain. We assumed that some modeset lock will prevent the power
reference from vanishing in the middle of the HW access, but this
assumption doesn't always hold. In such cases we get either the wakeref
not held, or an unclaimed register access error message. To fix this in
a future-proof way that's independent of other locks wrap any such
access with a get_ref_if_enabled()/put_ref() pair.

Kudos to Ville and Joonas for the ideas of this new interface.

CC: Mika Kuoppala <mika.kuoppala@intel.com>
CC: Chris Wilson <chris@chris-wilson.co.uk>
CC: Joonas Lahtinen <joonas.lahtinen@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_drv.h        |   3 +
 drivers/gpu/drm/i915/intel_runtime_pm.c | 111 ++++++++++++++++++++++++++------
 2 files changed, 96 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 878172a..9380ffe 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1436,6 +1436,8 @@ bool __intel_display_power_is_enabled(struct drm_i915_private *dev_priv,
 				      enum intel_display_power_domain domain);
 void intel_display_power_get(struct drm_i915_private *dev_priv,
 			     enum intel_display_power_domain domain);
+bool intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv,
+					enum intel_display_power_domain domain);
 void intel_display_power_put(struct drm_i915_private *dev_priv,
 			     enum intel_display_power_domain domain);
 
@@ -1522,6 +1524,7 @@ enable_rpm_wakeref_asserts(struct drm_i915_private *dev_priv)
 	enable_rpm_wakeref_asserts(dev_priv)
 
 void intel_runtime_pm_get(struct drm_i915_private *dev_priv);
+bool intel_runtime_pm_get_if_in_use(struct drm_i915_private *dev_priv);
 void intel_runtime_pm_get_noresume(struct drm_i915_private *dev_priv);
 void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
 
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index bbca527..a81f965 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -1435,39 +1435,90 @@ static void chv_pipe_power_well_disable(struct drm_i915_private *dev_priv,
 	chv_set_pipe_power_well(dev_priv, power_well, false);
 }
 
-/**
- * intel_display_power_get - grab a power domain reference
- * @dev_priv: i915 device instance
- * @domain: power domain to reference
- *
- * This function grabs a power domain reference for @domain and ensures that the
- * power domain and all its parents are powered up. Therefore users should only
- * grab a reference to the innermost power domain they need.
- *
- * Any power domain reference obtained by this function must have a symmetric
- * call to intel_display_power_put() to release the reference again.
- */
-void intel_display_power_get(struct drm_i915_private *dev_priv,
-			     enum intel_display_power_domain domain)
+static void
+__intel_display_power_get_domain(struct drm_i915_private *dev_priv,
+				 enum intel_display_power_domain domain)
 {
 	struct i915_power_domains *power_domains;
 	struct i915_power_well *power_well;
 	int i;
 
-	intel_runtime_pm_get(dev_priv);
-
 	power_domains = &dev_priv->power_domains;
 
-	mutex_lock(&power_domains->lock);
-
 	for_each_power_well(i, power_well, BIT(domain), power_domains) {
 		if (!power_well->count++)
 			intel_power_well_enable(dev_priv, power_well);
 	}
 
 	power_domains->domain_use_count[domain]++;
+}
+
+/**
+ * intel_display_power_get - grab a power domain reference
+ * @dev_priv: i915 device instance
+ * @domain: power domain to reference
+ *
+ * This function grabs a power domain reference for @domain and ensures that the
+ * power domain and all its parents are powered up. Therefore users should only
+ * grab a reference to the innermost power domain they need.
+ *
+ * Any power domain reference obtained by this function must have a symmetric
+ * call to intel_display_power_put() to release the reference again.
+ */
+void intel_display_power_get(struct drm_i915_private *dev_priv,
+			     enum intel_display_power_domain domain)
+{
+	struct i915_power_domains *power_domains;
+
+	intel_runtime_pm_get(dev_priv);
+
+	power_domains = &dev_priv->power_domains;
+
+	mutex_lock(&power_domains->lock);
+
+	__intel_display_power_get_domain(dev_priv, domain);
+
+	mutex_unlock(&power_domains->lock);
+}
+
+/**
+ * intel_display_power_get_if_enabled - grab a reference for an enabled display power domain
+ * @dev_priv: i915 device instance
+ * @domain: power domain to reference
+ *
+ * This function grabs a power domain reference for @domain and ensures that the
+ * power domain and all its parents are powered up. Therefore users should only
+ * grab a reference to the innermost power domain they need.
+ *
+ * Any power domain reference obtained by this function must have a symmetric
+ * call to intel_display_power_put() to release the reference again.
+ */
+bool intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv,
+					enum intel_display_power_domain domain)
+{
+	struct i915_power_domains *power_domains;
+	bool is_enabled;
+
+	if (!intel_runtime_pm_get_if_in_use(dev_priv))
+		return false;
+
+	power_domains = &dev_priv->power_domains;
+
+	mutex_lock(&power_domains->lock);
+
+	if (__intel_display_power_is_enabled(dev_priv, domain)) {
+		__intel_display_power_get_domain(dev_priv, domain);
+		is_enabled = true;
+	} else {
+		is_enabled = false;
+	}
 
 	mutex_unlock(&power_domains->lock);
+
+	if (!is_enabled)
+		intel_runtime_pm_put(dev_priv);
+
+	return is_enabled;
 }
 
 /**
@@ -2239,6 +2290,30 @@ void intel_runtime_pm_get(struct drm_i915_private *dev_priv)
 }
 
 /**
+ * intel_runtime_pm_get_if_in_use - grab a runtime pm reference if device in use
+ * @dev_priv: i915 device instance
+ *
+ * This function grabs a device-level runtime pm reference if the device is
+ * already in use and ensures that it is powered up.
+ *
+ * Any runtime pm reference obtained by this function must have a symmetric
+ * call to intel_runtime_pm_put() to release the reference again.
+ */
+bool intel_runtime_pm_get_if_in_use(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = dev_priv->dev;
+	struct device *device = &dev->pdev->dev;
+
+	if (IS_ENABLED(CONFIG_PM) && !pm_runtime_get_if_in_use(device))
+		return false;
+
+	atomic_inc(&dev_priv->pm.wakeref_count);
+	assert_rpm_wakelock_held(dev_priv);
+
+	return true;
+}
+
+/**
  * intel_runtime_pm_get_noresume - grab a runtime pm reference
  * @dev_priv: i915 device instance
  *
-- 
2.5.0

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

[PATCH 02/12] drm/i915: ensure the HW is powered during display pipe HW readout

[Imre Deak]

The assumption when adding the intel_display_power_is_enabled() checks
was that if it returns success the power can't be turned off afterwards
during the HW access, which is guaranteed by modeset locks. This isn't
always true, so make sure we hold a dedicated reference for the time of
the access.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 836bbdc..6abfc54 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8181,18 +8181,22 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	enum intel_display_power_domain power_domain;
 	uint32_t tmp;
+	bool ret;
 
-	if (!intel_display_power_is_enabled(dev_priv,
-					    POWER_DOMAIN_PIPE(crtc->pipe)))
+	power_domain = POWER_DOMAIN_PIPE(crtc->pipe);
+	if (!intel_display_power_get_if_enabled(dev_priv, power_domain))
 		return false;
 
 	pipe_config->cpu_transcoder = (enum transcoder) crtc->pipe;
 	pipe_config->shared_dpll = DPLL_ID_PRIVATE;
 
+	ret = false;
+
 	tmp = I915_READ(PIPECONF(crtc->pipe));
 	if (!(tmp & PIPECONF_ENABLE))
-		return false;
+		goto out;
 
 	if (IS_G4X(dev) || IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) {
 		switch (tmp & PIPECONF_BPC_MASK) {
@@ -8272,7 +8276,12 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
 	pipe_config->base.adjusted_mode.crtc_clock =
 		pipe_config->port_clock / pipe_config->pixel_multiplier;
 
-	return true;
+	ret = true;
+
+out:
+	intel_display_power_put(dev_priv, power_domain);
+
+	return ret;
 }
 
 static void ironlake_init_pch_refclk(struct drm_device *dev)
@@ -9376,18 +9385,21 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	enum intel_display_power_domain power_domain;
 	uint32_t tmp;
+	bool ret;
 
-	if (!intel_display_power_is_enabled(dev_priv,
-					    POWER_DOMAIN_PIPE(crtc->pipe)))
+	power_domain = POWER_DOMAIN_PIPE(crtc->pipe);
+	if (!intel_display_power_get_if_enabled(dev_priv, power_domain))
 		return false;
 
 	pipe_config->cpu_transcoder = (enum transcoder) crtc->pipe;
 	pipe_config->shared_dpll = DPLL_ID_PRIVATE;
 
+	ret = false;
 	tmp = I915_READ(PIPECONF(crtc->pipe));
 	if (!(tmp & PIPECONF_ENABLE))
-		return false;
+		goto out;
 
 	switch (tmp & PIPECONF_BPC_MASK) {
 	case PIPECONF_6BPC:
@@ -9450,7 +9462,12 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
 
 	ironlake_get_pfit_config(crtc, pipe_config);
 
-	return true;
+	ret = true;
+
+out:
+	intel_display_power_put(dev_priv, power_domain);
+
+	return ret;
 }
 
 static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv)
@@ -9982,12 +9999,17 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	enum intel_display_power_domain pfit_domain;
+	enum intel_display_power_domain power_domain;
+	unsigned long power_domain_mask;
 	uint32_t tmp;
+	bool ret;
 
-	if (!intel_display_power_is_enabled(dev_priv,
-					 POWER_DOMAIN_PIPE(crtc->pipe)))
+	power_domain = POWER_DOMAIN_PIPE(crtc->pipe);
+	if (!intel_display_power_get_if_enabled(dev_priv, power_domain))
 		return false;
+	power_domain_mask = BIT(power_domain);
+
+	ret = false;
 
 	pipe_config->cpu_transcoder = (enum transcoder) crtc->pipe;
 	pipe_config->shared_dpll = DPLL_ID_PRIVATE;
@@ -10014,13 +10036,14 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 			pipe_config->cpu_transcoder = TRANSCODER_EDP;
 	}
 
-	if (!intel_display_power_is_enabled(dev_priv,
-			POWER_DOMAIN_TRANSCODER(pipe_config->cpu_transcoder)))
-		return false;
+	power_domain = POWER_DOMAIN_TRANSCODER(pipe_config->cpu_transcoder);
+	if (!intel_display_power_get_if_enabled(dev_priv, power_domain))
+		goto out;
+	power_domain_mask |= BIT(power_domain);
 
 	tmp = I915_READ(PIPECONF(pipe_config->cpu_transcoder));
 	if (!(tmp & PIPECONF_ENABLE))
-		return false;
+		goto out;
 
 	haswell_get_ddi_port_state(crtc, pipe_config);
 
@@ -10030,14 +10053,14 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 		skl_init_scalers(dev, crtc, pipe_config);
 	}
 
-	pfit_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
-
 	if (INTEL_INFO(dev)->gen >= 9) {
 		pipe_config->scaler_state.scaler_id = -1;
 		pipe_config->scaler_state.scaler_users &= ~(1 << SKL_CRTC_INDEX);
 	}
 
-	if (intel_display_power_is_enabled(dev_priv, pfit_domain)) {
+	power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
+	if (intel_display_power_get_if_enabled(dev_priv, power_domain)) {
+		power_domain_mask |= BIT(power_domain);
 		if (INTEL_INFO(dev)->gen >= 9)
 			skylake_get_pfit_config(crtc, pipe_config);
 		else
@@ -10055,7 +10078,13 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 		pipe_config->pixel_multiplier = 1;
 	}
 
-	return true;
+	ret = true;
+
+out:
+	for_each_power_domain(power_domain, power_domain_mask)
+		intel_display_power_put(dev_priv, power_domain);
+
+	return ret;
 }
 
 static void i845_update_cursor(struct drm_crtc *crtc, u32 base,
-- 
2.5.0

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

[PATCH 03/12] drm/i915/ibx: ensure the HW is powered during PLL HW readout

[Imre Deak]

The assumption when adding the intel_display_power_is_enabled() checks
was that if it returns success the power can't be turned off afterwards
during the HW access, which is guaranteed by modeset locks. This isn't
always true, so make sure we hold a dedicated reference for the time of
the access.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6abfc54..fe249ff 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13699,7 +13699,7 @@ static bool ibx_pch_dpll_get_hw_state(struct drm_i915_private *dev_priv,
 {
 	uint32_t val;
 
-	if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_PLLS))
+	if (!intel_display_power_get_if_enabled(dev_priv, POWER_DOMAIN_PLLS))
 		return false;
 
 	val = I915_READ(PCH_DPLL(pll->id));
@@ -13707,6 +13707,8 @@ static bool ibx_pch_dpll_get_hw_state(struct drm_i915_private *dev_priv,
 	hw_state->fp0 = I915_READ(PCH_FP0(pll->id));
 	hw_state->fp1 = I915_READ(PCH_FP1(pll->id));
 
+	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
+
 	return val & DPLL_VCO_ENABLE;
 }
 
-- 
2.5.0

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

[PATCH 04/12] drm/i915: ensure the HW is powered when disabling VGA

[Imre Deak]

The assumption when adding the intel_display_power_is_enabled() checks
was that if it returns success the power can't be turned off afterwards
during the HW access, which is guaranteed by modeset locks. This isn't
always true, so make sure we hold a dedicated reference for the time of
the access.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fe249ff..ca6efa6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15705,10 +15705,12 @@ void i915_redisable_vga(struct drm_device *dev)
 	 * level, just check if the power well is enabled instead of trying to
 	 * follow the "don't touch the power well if we don't need it" policy
 	 * the rest of the driver uses. */
-	if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_VGA))
+	if (!intel_display_power_get_if_enabled(dev_priv, POWER_DOMAIN_VGA))
 		return;
 
 	i915_redisable_vga_power_on(dev);
+
+	intel_display_power_put(dev_priv, POWER_DOMAIN_VGA);
 }
 
 static bool primary_get_hw_state(struct intel_plane *plane)
-- 
2.5.0

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

[PATCH 05/12] drm/i915: ensure the HW is powered during HW access in assert_pipe

[Imre Deak]

The assumption when adding the intel_display_power_is_enabled() checks
was that if it returns success the power can't be turned off afterwards
during the HW access, which is guaranteed by modeset locks. This isn't
always true, so make sure we hold a dedicated reference for the time of
the access.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ca6efa6..35f46a3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1343,18 +1343,21 @@ void assert_pipe(struct drm_i915_private *dev_priv,
 	bool cur_state;
 	enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv,
 								      pipe);
+	enum intel_display_power_domain power_domain;
 
 	/* if we need the pipe quirk it must be always on */
 	if ((pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) ||
 	    (pipe == PIPE_B && dev_priv->quirks & QUIRK_PIPEB_FORCE))
 		state = true;
 
-	if (!intel_display_power_is_enabled(dev_priv,
-				POWER_DOMAIN_TRANSCODER(cpu_transcoder))) {
-		cur_state = false;
-	} else {
+	power_domain = POWER_DOMAIN_TRANSCODER(cpu_transcoder);
+	if (intel_display_power_get_if_enabled(dev_priv, power_domain)) {
 		u32 val = I915_READ(PIPECONF(cpu_transcoder));
 		cur_state = !!(val & PIPECONF_ENABLE);
+
+		intel_display_power_put(dev_priv, power_domain);
+	} else {
+		cur_state = false;
 	}
 
 	I915_STATE_WARN(cur_state != state,
-- 
2.5.0

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

[PATCH 06/12] drm/i915/crt: ensure the HW is powered during HW state readout

[Imre Deak]

The assumption when adding the intel_display_power_is_enabled() checks
was that if it returns success the power can't be turned off afterwards
during the HW access, which is guaranteed by modeset locks. This isn't
always true, so make sure we hold a dedicated reference for the time of
the access.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93439
CC: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_crt.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index ad5dfab..e686a91 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -71,22 +71,29 @@ static bool intel_crt_get_hw_state(struct intel_encoder *encoder,
 	struct intel_crt *crt = intel_encoder_to_crt(encoder);
 	enum intel_display_power_domain power_domain;
 	u32 tmp;
+	bool ret;
 
 	power_domain = intel_display_port_power_domain(encoder);
-	if (!intel_display_power_is_enabled(dev_priv, power_domain))
+	if (!intel_display_power_get_if_enabled(dev_priv, power_domain))
 		return false;
 
+	ret = false;
+
 	tmp = I915_READ(crt->adpa_reg);
 
 	if (!(tmp & ADPA_DAC_ENABLE))
-		return false;
+		goto out;
 
 	if (HAS_PCH_CPT(dev))
 		*pipe = PORT_TO_PIPE_CPT(tmp);
 	else
 		*pipe = PORT_TO_PIPE(tmp);
 
-	return true;
+	ret = true;
+out:
+	intel_display_power_put(dev_priv, power_domain);
+
+	return ret;
 }
 
 static unsigned int intel_crt_get_flags(struct intel_encoder *encoder)
-- 
2.5.0

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

[PATCH 07/12] drm/i915/ddi: ensure the HW is powered during HW state readout

[Imre Deak]

The assumption when adding the intel_display_power_is_enabled() checks
was that if it returns success the power can't be turned off afterwards
during the HW access, which is guaranteed by modeset locks. This isn't
always true, so make sure we hold a dedicated reference for the time of
the access.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93441
CC: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 116 +++++++++++++++++++++++++++------------
 1 file changed, 81 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index cdf2e14..21a9b83 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1913,13 +1913,16 @@ bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector)
 	enum transcoder cpu_transcoder;
 	enum intel_display_power_domain power_domain;
 	uint32_t tmp;
+	bool ret;
 
 	power_domain = intel_display_port_power_domain(intel_encoder);
-	if (!intel_display_power_is_enabled(dev_priv, power_domain))
+	if (!intel_display_power_get_if_enabled(dev_priv, power_domain))
 		return false;
 
-	if (!intel_encoder->get_hw_state(intel_encoder, &pipe))
-		return false;
+	if (!intel_encoder->get_hw_state(intel_encoder, &pipe)) {
+		ret = false;
+		goto out;
+	}
 
 	if (port == PORT_A)
 		cpu_transcoder = TRANSCODER_EDP;
@@ -1931,23 +1934,33 @@ bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector)
 	switch (tmp & TRANS_DDI_MODE_SELECT_MASK) {
 	case TRANS_DDI_MODE_SELECT_HDMI:
 	case TRANS_DDI_MODE_SELECT_DVI:
-		return (type == DRM_MODE_CONNECTOR_HDMIA);
+		ret = type == DRM_MODE_CONNECTOR_HDMIA;
+		break;
 
 	case TRANS_DDI_MODE_SELECT_DP_SST:
-		if (type == DRM_MODE_CONNECTOR_eDP)
-			return true;
-		return (type == DRM_MODE_CONNECTOR_DisplayPort);
+		ret = type == DRM_MODE_CONNECTOR_eDP ||
+		      type == DRM_MODE_CONNECTOR_DisplayPort;
+		break;
+
 	case TRANS_DDI_MODE_SELECT_DP_MST:
 		/* if the transcoder is in MST state then
 		 * connector isn't connected */
-		return false;
+		ret = false;
+		break;
 
 	case TRANS_DDI_MODE_SELECT_FDI:
-		return (type == DRM_MODE_CONNECTOR_VGA);
+		ret = type == DRM_MODE_CONNECTOR_VGA;
+		break;
 
 	default:
-		return false;
+		ret = false;
+		break;
 	}
+
+out:
+	intel_display_power_put(dev_priv, power_domain);
+
+	return ret;
 }
 
 bool intel_ddi_get_hw_state(struct intel_encoder *encoder,
@@ -1959,15 +1972,18 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder,
 	enum intel_display_power_domain power_domain;
 	u32 tmp;
 	int i;
+	bool ret;
 
 	power_domain = intel_display_port_power_domain(encoder);
-	if (!intel_display_power_is_enabled(dev_priv, power_domain))
+	if (!intel_display_power_get_if_enabled(dev_priv, power_domain))
 		return false;
 
+	ret = false;
+
 	tmp = I915_READ(DDI_BUF_CTL(port));
 
 	if (!(tmp & DDI_BUF_CTL_ENABLE))
-		return false;
+		goto out;
 
 	if (port == PORT_A) {
 		tmp = I915_READ(TRANS_DDI_FUNC_CTL(TRANSCODER_EDP));
@@ -1985,25 +2001,32 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder,
 			break;
 		}
 
-		return true;
-	} else {
-		for (i = TRANSCODER_A; i <= TRANSCODER_C; i++) {
-			tmp = I915_READ(TRANS_DDI_FUNC_CTL(i));
-
-			if ((tmp & TRANS_DDI_PORT_MASK)
-			    == TRANS_DDI_SELECT_PORT(port)) {
-				if ((tmp & TRANS_DDI_MODE_SELECT_MASK) == TRANS_DDI_MODE_SELECT_DP_MST)
-					return false;
-
-				*pipe = i;
-				return true;
-			}
+		ret = true;
+
+		goto out;
+	}
+
+	for (i = TRANSCODER_A; i <= TRANSCODER_C; i++) {
+		tmp = I915_READ(TRANS_DDI_FUNC_CTL(i));
+
+		if ((tmp & TRANS_DDI_PORT_MASK) == TRANS_DDI_SELECT_PORT(port)) {
+			if ((tmp & TRANS_DDI_MODE_SELECT_MASK) ==
+			    TRANS_DDI_MODE_SELECT_DP_MST)
+				goto out;
+
+			*pipe = i;
+			ret = true;
+
+			goto out;
 		}
 	}
 
 	DRM_DEBUG_KMS("No pipe for ddi port %c found\n", port_name(port));
 
-	return false;
+out:
+	intel_display_power_put(dev_priv, power_domain);
+
+	return ret;
 }
 
 void intel_ddi_enable_pipe_clock(struct intel_crtc *intel_crtc)
@@ -2449,12 +2472,14 @@ static bool hsw_ddi_wrpll_get_hw_state(struct drm_i915_private *dev_priv,
 {
 	uint32_t val;
 
-	if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_PLLS))
+	if (!intel_display_power_get_if_enabled(dev_priv, POWER_DOMAIN_PLLS))
 		return false;
 
 	val = I915_READ(WRPLL_CTL(pll->id));
 	hw_state->wrpll = val;
 
+	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
+
 	return val & WRPLL_PLL_ENABLE;
 }
 
@@ -2464,12 +2489,14 @@ static bool hsw_ddi_spll_get_hw_state(struct drm_i915_private *dev_priv,
 {
 	uint32_t val;
 
-	if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_PLLS))
+	if (!intel_display_power_get_if_enabled(dev_priv, POWER_DOMAIN_PLLS))
 		return false;
 
 	val = I915_READ(SPLL_CTL);
 	hw_state->spll = val;
 
+	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
+
 	return val & SPLL_PLL_ENABLE;
 }
 
@@ -2586,16 +2613,19 @@ static bool skl_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
 	uint32_t val;
 	unsigned int dpll;
 	const struct skl_dpll_regs *regs = skl_dpll_regs;
+	bool ret;
 
-	if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_PLLS))
+	if (!intel_display_power_get_if_enabled(dev_priv, POWER_DOMAIN_PLLS))
 		return false;
 
+	ret = false;
+
 	/* DPLL0 is not part of the shared DPLLs, so pll->id is 0 for DPLL1 */
 	dpll = pll->id + 1;
 
 	val = I915_READ(regs[pll->id].ctl);
 	if (!(val & LCPLL_PLL_ENABLE))
-		return false;
+		goto out;
 
 	val = I915_READ(DPLL_CTRL1);
 	hw_state->ctrl1 = (val >> (dpll * 6)) & 0x3f;
@@ -2605,8 +2635,12 @@ static bool skl_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
 		hw_state->cfgcr1 = I915_READ(regs[pll->id].cfgcr1);
 		hw_state->cfgcr2 = I915_READ(regs[pll->id].cfgcr2);
 	}
+	ret = true;
 
-	return true;
+out:
+	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
+
+	return ret;
 }
 
 static void skl_shared_dplls_init(struct drm_i915_private *dev_priv)
@@ -2873,13 +2907,16 @@ static bool bxt_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
 {
 	enum port port = (enum port)pll->id;	/* 1:1 port->PLL mapping */
 	uint32_t val;
+	bool ret;
 
-	if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_PLLS))
+	if (!intel_display_power_get_if_enabled(dev_priv, POWER_DOMAIN_PLLS))
 		return false;
 
+	ret = false;
+
 	val = I915_READ(BXT_PORT_PLL_ENABLE(port));
 	if (!(val & PORT_PLL_ENABLE))
-		return false;
+		goto out;
 
 	hw_state->ebb0 = I915_READ(BXT_PORT_PLL_EBB_0(port));
 	hw_state->ebb0 &= PORT_PLL_P1_MASK | PORT_PLL_P2_MASK;
@@ -2926,7 +2963,12 @@ static bool bxt_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
 				 I915_READ(BXT_PORT_PCS_DW12_LN23(port)));
 	hw_state->pcsdw12 &= LANE_STAGGER_MASK | LANESTAGGER_STRAP_OVRD;
 
-	return true;
+	ret = true;
+
+out:
+	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
+
+	return ret;
 }
 
 static void bxt_shared_dplls_init(struct drm_i915_private *dev_priv)
@@ -3061,11 +3103,15 @@ bool intel_ddi_is_audio_enabled(struct drm_i915_private *dev_priv,
 {
 	u32 temp;
 
-	if (intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_AUDIO)) {
+	if (intel_display_power_get_if_enabled(dev_priv, POWER_DOMAIN_AUDIO)) {
 		temp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
+
+		intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO);
+
 		if (temp & AUDIO_OUTPUT_ENABLE(intel_crtc->pipe))
 			return true;
 	}
+
 	return false;
 }
 
-- 
2.5.0

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

[PATCH 08/12] drm/i915: ensure the HW is powered when accessing the CRC HW block

[Imre Deak]

The assumption when adding the intel_display_power_is_enabled() checks
was that if it returns success the power can't be turned off afterwards
during the HW access, which is guaranteed by modeset locks. This isn't
always true, so make sure we hold a dedicated reference for the time of
the access.

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index ec0c2a05e..9e19cf0 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -825,8 +825,11 @@ static int i915_interrupt_info(struct seq_file *m, void *data)
 		}
 
 		for_each_pipe(dev_priv, pipe) {
-			if (!intel_display_power_is_enabled(dev_priv,
-						POWER_DOMAIN_PIPE(pipe))) {
+			enum intel_display_power_domain power_domain;
+
+			power_domain = POWER_DOMAIN_PIPE(pipe);
+			if (!intel_display_power_get_if_enabled(dev_priv,
+								power_domain)) {
 				seq_printf(m, "Pipe %c power disabled\n",
 					   pipe_name(pipe));
 				continue;
@@ -840,6 +843,8 @@ static int i915_interrupt_info(struct seq_file *m, void *data)
 			seq_printf(m, "Pipe %c IER:\t%08x\n",
 				   pipe_name(pipe),
 				   I915_READ(GEN8_DE_PIPE_IER(pipe)));
+
+			intel_display_power_put(dev_priv, power_domain);
 		}
 
 		seq_printf(m, "Display Engine port interrupt mask:\t%08x\n",
@@ -4004,6 +4009,7 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
 	struct intel_crtc *crtc = to_intel_crtc(intel_get_crtc_for_pipe(dev,
 									pipe));
+	enum intel_display_power_domain power_domain;
 	u32 val = 0; /* shut up gcc */
 	int ret;
 
@@ -4014,7 +4020,8 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 	if (pipe_crc->source && source)
 		return -EINVAL;
 
-	if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_PIPE(pipe))) {
+	power_domain = POWER_DOMAIN_PIPE(pipe);
+	if (!intel_display_power_get_if_enabled(dev_priv, power_domain)) {
 		DRM_DEBUG_KMS("Trying to capture CRC while pipe is off\n");
 		return -EIO;
 	}
@@ -4031,7 +4038,7 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 		ret = ivb_pipe_crc_ctl_reg(dev, pipe, &source, &val);
 
 	if (ret != 0)
-		return ret;
+		goto out;
 
 	/* none -> real source transition */
 	if (source) {
@@ -4043,8 +4050,10 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 		entries = kcalloc(INTEL_PIPE_CRC_ENTRIES_NR,
 				  sizeof(pipe_crc->entries[0]),
 				  GFP_KERNEL);
-		if (!entries)
-			return -ENOMEM;
+		if (!entries) {
+			ret = -ENOMEM;
+			goto out;
+		}
 
 		/*
 		 * When IPS gets enabled, the pipe CRC changes. Since IPS gets
@@ -4100,7 +4109,12 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 		hsw_enable_ips(crtc);
 	}
 
-	return 0;
+	ret = 0;
+
+out:
+	intel_display_power_put(dev_priv, power_domain);
+
+	return ret;
 }
 
 /*
-- 
2.5.0

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

[PATCH 09/12] drm/i915/dp: ensure the HW is powered during HW state readout

[Imre Deak]

The assumption when adding the intel_display_power_is_enabled() checks
was that if it returns success the power can't be turned off afterwards
during the HW access, which is guaranteed by modeset locks. This isn't
always true, so make sure we hold a dedicated reference for the time of
the access.

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 65d5084..34e644d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2360,15 +2360,18 @@ static bool intel_dp_get_hw_state(struct intel_encoder *encoder,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	enum intel_display_power_domain power_domain;
 	u32 tmp;
+	bool ret;
 
 	power_domain = intel_display_port_power_domain(encoder);
-	if (!intel_display_power_is_enabled(dev_priv, power_domain))
+	if (!intel_display_power_get_if_enabled(dev_priv, power_domain))
 		return false;
 
+	ret = false;
+
 	tmp = I915_READ(intel_dp->output_reg);
 
 	if (!(tmp & DP_PORT_EN))
-		return false;
+		goto out;
 
 	if (IS_GEN7(dev) && port == PORT_A) {
 		*pipe = PORT_TO_PIPE_CPT(tmp);
@@ -2379,7 +2382,9 @@ static bool intel_dp_get_hw_state(struct intel_encoder *encoder,
 			u32 trans_dp = I915_READ(TRANS_DP_CTL(p));
 			if (TRANS_DP_PIPE_TO_PORT(trans_dp) == port) {
 				*pipe = p;
-				return true;
+				ret = true;
+
+				goto out;
 			}
 		}
 
@@ -2391,7 +2396,12 @@ static bool intel_dp_get_hw_state(struct intel_encoder *encoder,
 		*pipe = PORT_TO_PIPE(tmp);
 	}
 
-	return true;
+	ret = true;
+
+out:
+	intel_display_power_put(dev_priv, power_domain);
+
+	return ret;
 }
 
 static void intel_dp_get_config(struct intel_encoder *encoder,
-- 
2.5.0

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

[PATCH 10/12] drm/i915/dsi: ensure the HW is powered during HW state readout

[Imre Deak]

The assumption when adding the intel_display_power_is_enabled() checks
was that if it returns success the power can't be turned off afterwards
during the HW access, which is guaranteed by modeset locks. This isn't
always true, so make sure we hold a dedicated reference for the time of
the access.

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

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 378f879..fcd746c 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -664,13 +664,16 @@ static bool intel_dsi_get_hw_state(struct intel_encoder *encoder,
 	struct drm_device *dev = encoder->base.dev;
 	enum intel_display_power_domain power_domain;
 	enum port port;
+	bool ret;
 
 	DRM_DEBUG_KMS("\n");
 
 	power_domain = intel_display_port_power_domain(encoder);
-	if (!intel_display_power_is_enabled(dev_priv, power_domain))
+	if (!intel_display_power_get_if_enabled(dev_priv, power_domain))
 		return false;
 
+	ret = false;
+
 	/* XXX: this only works for one DSI output */
 	for_each_dsi_port(port, intel_dsi->ports) {
 		i915_reg_t ctrl_reg = IS_BROXTON(dev) ?
@@ -691,12 +694,16 @@ static bool intel_dsi_get_hw_state(struct intel_encoder *encoder,
 		if (dpi_enabled || (func & CMD_MODE_DATA_WIDTH_MASK)) {
 			if (I915_READ(MIPI_DEVICE_READY(port)) & DEVICE_READY) {
 				*pipe = port == PORT_A ? PIPE_A : PIPE_B;
-				return true;
+				ret = true;
+
+				goto out;
 			}
 		}
 	}
+out:
+	intel_display_power_put(dev_priv, power_domain);
 
-	return false;
+	return ret;
 }
 
 static void intel_dsi_get_config(struct intel_encoder *encoder,
-- 
2.5.0

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

[PATCH 11/12] drm/i915/hdmi: ensure the HW is powered during HW state readout

[Imre Deak]

The assumption when adding the intel_display_power_is_enabled() checks
was that if it returns success the power can't be turned off afterwards
during the HW access, which is guaranteed by modeset locks. This isn't
always true, so make sure we hold a dedicated reference for the time of
the access.

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

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index edb7e90..80b44c0 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -880,15 +880,18 @@ static bool intel_hdmi_get_hw_state(struct intel_encoder *encoder,
 	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
 	enum intel_display_power_domain power_domain;
 	u32 tmp;
+	bool ret;
 
 	power_domain = intel_display_port_power_domain(encoder);
-	if (!intel_display_power_is_enabled(dev_priv, power_domain))
+	if (!intel_display_power_get_if_enabled(dev_priv, power_domain))
 		return false;
 
+	ret = false;
+
 	tmp = I915_READ(intel_hdmi->hdmi_reg);
 
 	if (!(tmp & SDVO_ENABLE))
-		return false;
+		goto out;
 
 	if (HAS_PCH_CPT(dev))
 		*pipe = PORT_TO_PIPE_CPT(tmp);
@@ -897,7 +900,12 @@ static bool intel_hdmi_get_hw_state(struct intel_encoder *encoder,
 	else
 		*pipe = PORT_TO_PIPE(tmp);
 
-	return true;
+	ret = true;
+
+out:
+	intel_display_power_put(dev_priv, power_domain);
+
+	return ret;
 }
 
 static void intel_hdmi_get_config(struct intel_encoder *encoder,
-- 
2.5.0

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

[PATCH 12/12] drm/i915/lvds: ensure the HW is powered during HW state readout

[Imre Deak]

The assumption when adding the intel_display_power_is_enabled() checks
was that if it returns success the power can't be turned off afterwards
during the HW access, which is guaranteed by modeset locks. This isn't
always true, so make sure we hold a dedicated reference for the time of
the access.

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

diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 811ddf7..30a8403 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -76,22 +76,30 @@ static bool intel_lvds_get_hw_state(struct intel_encoder *encoder,
 	struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(&encoder->base);
 	enum intel_display_power_domain power_domain;
 	u32 tmp;
+	bool ret;
 
 	power_domain = intel_display_port_power_domain(encoder);
-	if (!intel_display_power_is_enabled(dev_priv, power_domain))
+	if (!intel_display_power_get_if_enabled(dev_priv, power_domain))
 		return false;
 
+	ret = false;
+
 	tmp = I915_READ(lvds_encoder->reg);
 
 	if (!(tmp & LVDS_PORT_EN))
-		return false;
+		goto out;
 
 	if (HAS_PCH_CPT(dev))
 		*pipe = PORT_TO_PIPE_CPT(tmp);
 	else
 		*pipe = PORT_TO_PIPE(tmp);
 
-	return true;
+	ret = true;
+
+out:
+	intel_display_power_put(dev_priv, power_domain);
+
+	return ret;
 }
 
 static void intel_lvds_get_config(struct intel_encoder *encoder,
-- 
2.5.0

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

Re: [PATCH 01/12] drm/i915: add helper to get a display power ref if it was already enabled

[Joonas Lahtinen]

On pe, 2016-02-12 at 18:55 +0200, Imre Deak wrote:
> We have many places in the code where we check if a given display power
> domain is enabled and if so access registers backed by this power
> domain. We assumed that some modeset lock will prevent the power
> reference from vanishing in the middle of the HW access, but this
> assumption doesn't always hold. In such cases we get either the wakeref
> not held, or an unclaimed register access error message. To fix this in
> a future-proof way that's independent of other locks wrap any such
> access with a get_ref_if_enabled()/put_ref() pair.
> 
> Kudos to Ville and Joonas for the ideas of this new interface.
> 

A couple variables could be initialized at declaration, other than
that;

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

> CC: Mika Kuoppala <mika.kuoppala@intel.com>
> CC: Chris Wilson <chris@chris-wilson.co.uk>
> CC: Joonas Lahtinen <joonas.lahtinen@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_drv.h        |   3 +
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 111 ++++++++++++++++++++++++++------
>  2 files changed, 96 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 878172a..9380ffe 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1436,6 +1436,8 @@ bool __intel_display_power_is_enabled(struct drm_i915_private *dev_priv,
>  				      enum intel_display_power_domain domain);
>  void intel_display_power_get(struct drm_i915_private *dev_priv,
>  			     enum intel_display_power_domain domain);
> +bool intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv,
> +					enum intel_display_power_domain domain);
>  void intel_display_power_put(struct drm_i915_private *dev_priv,
>  			     enum intel_display_power_domain domain);
>  
> @@ -1522,6 +1524,7 @@ enable_rpm_wakeref_asserts(struct drm_i915_private *dev_priv)
>  	enable_rpm_wakeref_asserts(dev_priv)
>  
>  void intel_runtime_pm_get(struct drm_i915_private *dev_priv);
> +bool intel_runtime_pm_get_if_in_use(struct drm_i915_private *dev_priv);
>  void intel_runtime_pm_get_noresume(struct drm_i915_private *dev_priv);
>  void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
>  
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index bbca527..a81f965 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -1435,39 +1435,90 @@ static void chv_pipe_power_well_disable(struct drm_i915_private *dev_priv,
>  	chv_set_pipe_power_well(dev_priv, power_well, false);
>  }
>  
> -/**
> - * intel_display_power_get - grab a power domain reference
> - * @dev_priv: i915 device instance
> - * @domain: power domain to reference
> - *
> - * This function grabs a power domain reference for @domain and ensures that the
> - * power domain and all its parents are powered up. Therefore users should only
> - * grab a reference to the innermost power domain they need.
> - *
> - * Any power domain reference obtained by this function must have a symmetric
> - * call to intel_display_power_put() to release the reference again.
> - */
> -void intel_display_power_get(struct drm_i915_private *dev_priv,
> -			     enum intel_display_power_domain domain)
> +static void
> +__intel_display_power_get_domain(struct drm_i915_private *dev_priv,
> +				 enum intel_display_power_domain domain)
>  {
>  	struct i915_power_domains *power_domains;
>  	struct i915_power_well *power_well;
>  	int i;
>  
> -	intel_runtime_pm_get(dev_priv);
> -
>  	power_domains = &dev_priv->power_domains;
>  

You could squash this to the declaration line.

> -	mutex_lock(&power_domains->lock);
> -
>  	for_each_power_well(i, power_well, BIT(domain), power_domains) {
>  		if (!power_well->count++)
>  			intel_power_well_enable(dev_priv, power_well);
>  	}
>  
>  	power_domains->domain_use_count[domain]++;
> +}
> +
> +/**
> + * intel_display_power_get - grab a power domain reference
> + * @dev_priv: i915 device instance
> + * @domain: power domain to reference
> + *
> + * This function grabs a power domain reference for @domain and ensures that the
> + * power domain and all its parents are powered up. Therefore users should only
> + * grab a reference to the innermost power domain they need.
> + *
> + * Any power domain reference obtained by this function must have a symmetric
> + * call to intel_display_power_put() to release the reference again.
> + */
> +void intel_display_power_get(struct drm_i915_private *dev_priv,
> +			     enum intel_display_power_domain domain)
> +{
> +	struct i915_power_domains *power_domains;
> +
> +	intel_runtime_pm_get(dev_priv);
> +
> +	power_domains = &dev_priv->power_domains;

Same here.

> +
> +	mutex_lock(&power_domains->lock);
> +
> +	__intel_display_power_get_domain(dev_priv, domain);
> +
> +	mutex_unlock(&power_domains->lock);
> +}
> +
> +/**
> + * intel_display_power_get_if_enabled - grab a reference for an enabled display power domain
> + * @dev_priv: i915 device instance
> + * @domain: power domain to reference
> + *
> + * This function grabs a power domain reference for @domain and ensures that the
> + * power domain and all its parents are powered up. Therefore users should only
> + * grab a reference to the innermost power domain they need.
> + *
> + * Any power domain reference obtained by this function must have a symmetric
> + * call to intel_display_power_put() to release the reference again.
> + */
> +bool intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv,
> +					enum intel_display_power_domain domain)
> +{
> +	struct i915_power_domains *power_domains;
> +	bool is_enabled;
> +
> +	if (!intel_runtime_pm_get_if_in_use(dev_priv))
> +		return false;
> +
> +	power_domains = &dev_priv->power_domains;
> +

Aaand here.

> +	mutex_lock(&power_domains->lock);
> +
> +	if (__intel_display_power_is_enabled(dev_priv, domain)) {
> +		__intel_display_power_get_domain(dev_priv, domain);
> +		is_enabled = true;
> +	} else {
> +		is_enabled = false;
> +	}
>  
>  	mutex_unlock(&power_domains->lock);
> +
> +	if (!is_enabled)
> +		intel_runtime_pm_put(dev_priv);
> +
> +	return is_enabled;
>  }
>  
>  /**
> @@ -2239,6 +2290,30 @@ void intel_runtime_pm_get(struct drm_i915_private *dev_priv)
>  }
>  
>  /**
> + * intel_runtime_pm_get_if_in_use - grab a runtime pm reference if device in use
> + * @dev_priv: i915 device instance
> + *
> + * This function grabs a device-level runtime pm reference if the device is
> + * already in use and ensures that it is powered up.
> + *
> + * Any runtime pm reference obtained by this function must have a symmetric
> + * call to intel_runtime_pm_put() to release the reference again.
> + */
> +bool intel_runtime_pm_get_if_in_use(struct drm_i915_private *dev_priv)
> +{
> +	struct drm_device *dev = dev_priv->dev;
> +	struct device *device = &dev->pdev->dev;
> +
> +	if (IS_ENABLED(CONFIG_PM) && !pm_runtime_get_if_in_use(device))
> +		return false;
> +
> +	atomic_inc(&dev_priv->pm.wakeref_count);
> +	assert_rpm_wakelock_held(dev_priv);
> +
> +	return true;
> +}
> +
> +/**
>   * intel_runtime_pm_get_noresume - grab a runtime pm reference
>   * @dev_priv: i915 device instance
>   *
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/12] drm/i915: ensure the HW is powered during display pipe HW readout

[Mika Kuoppala]

Imre Deak <imre.deak@intel.com> writes:

> The assumption when adding the intel_display_power_is_enabled() checks
> was that if it returns success the power can't be turned off afterwards
> during the HW access, which is guaranteed by modeset locks. This isn't
> always true, so make sure we hold a dedicated reference for the time of
> the access.
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>


Was concerned on domain mask overflows but there was already
BUILD_BUG_ON for it.

Revieved-by: Mika Kuoppala <mika.kuoppala@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 67 ++++++++++++++++++++++++++----------
>  1 file changed, 48 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 836bbdc..6abfc54 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8181,18 +8181,22 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	enum intel_display_power_domain power_domain;
>  	uint32_t tmp;
> +	bool ret;
>  
> -	if (!intel_display_power_is_enabled(dev_priv,
> -					    POWER_DOMAIN_PIPE(crtc->pipe)))
> +	power_domain = POWER_DOMAIN_PIPE(crtc->pipe);
> +	if (!intel_display_power_get_if_enabled(dev_priv, power_domain))
>  		return false;
>  
>  	pipe_config->cpu_transcoder = (enum transcoder) crtc->pipe;
>  	pipe_config->shared_dpll = DPLL_ID_PRIVATE;
>  
> +	ret = false;
> +
>  	tmp = I915_READ(PIPECONF(crtc->pipe));
>  	if (!(tmp & PIPECONF_ENABLE))
> -		return false;
> +		goto out;
>  
>  	if (IS_G4X(dev) || IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) {
>  		switch (tmp & PIPECONF_BPC_MASK) {
> @@ -8272,7 +8276,12 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
>  	pipe_config->base.adjusted_mode.crtc_clock =
>  		pipe_config->port_clock / pipe_config->pixel_multiplier;
>  
> -	return true;
> +	ret = true;
> +
> +out:
> +	intel_display_power_put(dev_priv, power_domain);
> +
> +	return ret;
>  }
>  
>  static void ironlake_init_pch_refclk(struct drm_device *dev)
> @@ -9376,18 +9385,21 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	enum intel_display_power_domain power_domain;
>  	uint32_t tmp;
> +	bool ret;
>  
> -	if (!intel_display_power_is_enabled(dev_priv,
> -					    POWER_DOMAIN_PIPE(crtc->pipe)))
> +	power_domain = POWER_DOMAIN_PIPE(crtc->pipe);
> +	if (!intel_display_power_get_if_enabled(dev_priv, power_domain))
>  		return false;
>  
>  	pipe_config->cpu_transcoder = (enum transcoder) crtc->pipe;
>  	pipe_config->shared_dpll = DPLL_ID_PRIVATE;
>  
> +	ret = false;
>  	tmp = I915_READ(PIPECONF(crtc->pipe));
>  	if (!(tmp & PIPECONF_ENABLE))
> -		return false;
> +		goto out;
>  
>  	switch (tmp & PIPECONF_BPC_MASK) {
>  	case PIPECONF_6BPC:
> @@ -9450,7 +9462,12 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
>  
>  	ironlake_get_pfit_config(crtc, pipe_config);
>  
> -	return true;
> +	ret = true;
> +
> +out:
> +	intel_display_power_put(dev_priv, power_domain);
> +
> +	return ret;
>  }
>  
>  static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv)
> @@ -9982,12 +9999,17 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	enum intel_display_power_domain pfit_domain;
> +	enum intel_display_power_domain power_domain;
> +	unsigned long power_domain_mask;
>  	uint32_t tmp;
> +	bool ret;
>  
> -	if (!intel_display_power_is_enabled(dev_priv,
> -					 POWER_DOMAIN_PIPE(crtc->pipe)))
> +	power_domain = POWER_DOMAIN_PIPE(crtc->pipe);
> +	if (!intel_display_power_get_if_enabled(dev_priv, power_domain))
>  		return false;
> +	power_domain_mask = BIT(power_domain);
> +
> +	ret = false;
>  
>  	pipe_config->cpu_transcoder = (enum transcoder) crtc->pipe;
>  	pipe_config->shared_dpll = DPLL_ID_PRIVATE;
> @@ -10014,13 +10036,14 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  			pipe_config->cpu_transcoder = TRANSCODER_EDP;
>  	}
>  
> -	if (!intel_display_power_is_enabled(dev_priv,
> -			POWER_DOMAIN_TRANSCODER(pipe_config->cpu_transcoder)))
> -		return false;
> +	power_domain = POWER_DOMAIN_TRANSCODER(pipe_config->cpu_transcoder);
> +	if (!intel_display_power_get_if_enabled(dev_priv, power_domain))
> +		goto out;
> +	power_domain_mask |= BIT(power_domain);
>  
>  	tmp = I915_READ(PIPECONF(pipe_config->cpu_transcoder));
>  	if (!(tmp & PIPECONF_ENABLE))
> -		return false;
> +		goto out;
>  
>  	haswell_get_ddi_port_state(crtc, pipe_config);
>  
> @@ -10030,14 +10053,14 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  		skl_init_scalers(dev, crtc, pipe_config);
>  	}
>  
> -	pfit_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
> -
>  	if (INTEL_INFO(dev)->gen >= 9) {
>  		pipe_config->scaler_state.scaler_id = -1;
>  		pipe_config->scaler_state.scaler_users &= ~(1 << SKL_CRTC_INDEX);
>  	}
>  
> -	if (intel_display_power_is_enabled(dev_priv, pfit_domain)) {
> +	power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
> +	if (intel_display_power_get_if_enabled(dev_priv, power_domain)) {
> +		power_domain_mask |= BIT(power_domain);
>  		if (INTEL_INFO(dev)->gen >= 9)
>  			skylake_get_pfit_config(crtc, pipe_config);
>  		else
> @@ -10055,7 +10078,13 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  		pipe_config->pixel_multiplier = 1;
>  	}
>  
> -	return true;
> +	ret = true;
> +
> +out:
> +	for_each_power_domain(power_domain, power_domain_mask)
> +		intel_display_power_put(dev_priv, power_domain);
> +
> +	return ret;
>  }
>  
>  static void i845_update_cursor(struct drm_crtc *crtc, u32 base,
> -- 
> 2.5.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 03/12] drm/i915/ibx: ensure the HW is powered during PLL HW readout

[Mika Kuoppala]

Imre Deak <imre.deak@intel.com> writes:

> The assumption when adding the intel_display_power_is_enabled() checks
> was that if it returns success the power can't be turned off afterwards
> during the HW access, which is guaranteed by modeset locks. This isn't
> always true, so make sure we hold a dedicated reference for the time of
> the access.
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 6abfc54..fe249ff 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13699,7 +13699,7 @@ static bool ibx_pch_dpll_get_hw_state(struct drm_i915_private *dev_priv,
>  {
>  	uint32_t val;
>  
> -	if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_PLLS))
> +	if (!intel_display_power_get_if_enabled(dev_priv, POWER_DOMAIN_PLLS))
>  		return false;
>  
>  	val = I915_READ(PCH_DPLL(pll->id));
> @@ -13707,6 +13707,8 @@ static bool ibx_pch_dpll_get_hw_state(struct drm_i915_private *dev_priv,
>  	hw_state->fp0 = I915_READ(PCH_FP0(pll->id));
>  	hw_state->fp1 = I915_READ(PCH_FP1(pll->id));
>  
> +	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
> +
>  	return val & DPLL_VCO_ENABLE;
>  }
>  
> -- 
> 2.5.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 01/12] drm/i915: add helper to get a display power ref if it was already enabled

[Imre Deak]

We have many places in the code where we check if a given display power
domain is enabled and if so access registers backed by this power
domain. We assumed that some modeset lock will prevent the power
reference from vanishing in the middle of the HW access, but this
assumption doesn't always hold. In such cases we get either the wakeref
not held, or an unclaimed register access error message. To fix this in
a future-proof way that's independent of other locks wrap any such
access with a get_ref_if_enabled()/put_ref() pair.

Kudos to Ville and Joonas for the ideas of this new interface.

v2:
- init the power_domains ptr when declaring it everywhere (Joonas)

CC: Mika Kuoppala <mika.kuoppala@intel.com>
CC: Chris Wilson <chris@chris-wilson.co.uk>
CC: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h        |   3 +
 drivers/gpu/drm/i915/intel_runtime_pm.c | 111 ++++++++++++++++++++++++++------
 2 files changed, 93 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3cae376..f95f8b2 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1436,6 +1436,8 @@ bool __intel_display_power_is_enabled(struct drm_i915_private *dev_priv,
 				      enum intel_display_power_domain domain);
 void intel_display_power_get(struct drm_i915_private *dev_priv,
 			     enum intel_display_power_domain domain);
+bool intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv,
+					enum intel_display_power_domain domain);
 void intel_display_power_put(struct drm_i915_private *dev_priv,
 			     enum intel_display_power_domain domain);
 
@@ -1522,6 +1524,7 @@ enable_rpm_wakeref_asserts(struct drm_i915_private *dev_priv)
 	enable_rpm_wakeref_asserts(dev_priv)
 
 void intel_runtime_pm_get(struct drm_i915_private *dev_priv);
+bool intel_runtime_pm_get_if_in_use(struct drm_i915_private *dev_priv);
 void intel_runtime_pm_get_noresume(struct drm_i915_private *dev_priv);
 void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
 
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index bbca527..0e476f7 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -1435,39 +1435,84 @@ static void chv_pipe_power_well_disable(struct drm_i915_private *dev_priv,
 	chv_set_pipe_power_well(dev_priv, power_well, false);
 }
 
-/**
- * intel_display_power_get - grab a power domain reference
- * @dev_priv: i915 device instance
- * @domain: power domain to reference
- *
- * This function grabs a power domain reference for @domain and ensures that the
- * power domain and all its parents are powered up. Therefore users should only
- * grab a reference to the innermost power domain they need.
- *
- * Any power domain reference obtained by this function must have a symmetric
- * call to intel_display_power_put() to release the reference again.
- */
-void intel_display_power_get(struct drm_i915_private *dev_priv,
-			     enum intel_display_power_domain domain)
+static void
+__intel_display_power_get_domain(struct drm_i915_private *dev_priv,
+				 enum intel_display_power_domain domain)
 {
-	struct i915_power_domains *power_domains;
+	struct i915_power_domains *power_domains = &dev_priv->power_domains;
 	struct i915_power_well *power_well;
 	int i;
 
-	intel_runtime_pm_get(dev_priv);
-
-	power_domains = &dev_priv->power_domains;
-
-	mutex_lock(&power_domains->lock);
-
 	for_each_power_well(i, power_well, BIT(domain), power_domains) {
 		if (!power_well->count++)
 			intel_power_well_enable(dev_priv, power_well);
 	}
 
 	power_domains->domain_use_count[domain]++;
+}
+
+/**
+ * intel_display_power_get - grab a power domain reference
+ * @dev_priv: i915 device instance
+ * @domain: power domain to reference
+ *
+ * This function grabs a power domain reference for @domain and ensures that the
+ * power domain and all its parents are powered up. Therefore users should only
+ * grab a reference to the innermost power domain they need.
+ *
+ * Any power domain reference obtained by this function must have a symmetric
+ * call to intel_display_power_put() to release the reference again.
+ */
+void intel_display_power_get(struct drm_i915_private *dev_priv,
+			     enum intel_display_power_domain domain)
+{
+	struct i915_power_domains *power_domains = &dev_priv->power_domains;
+
+	intel_runtime_pm_get(dev_priv);
+
+	mutex_lock(&power_domains->lock);
+
+	__intel_display_power_get_domain(dev_priv, domain);
+
+	mutex_unlock(&power_domains->lock);
+}
+
+/**
+ * intel_display_power_get_if_enabled - grab a reference for an enabled display power domain
+ * @dev_priv: i915 device instance
+ * @domain: power domain to reference
+ *
+ * This function grabs a power domain reference for @domain and ensures that the
+ * power domain and all its parents are powered up. Therefore users should only
+ * grab a reference to the innermost power domain they need.
+ *
+ * Any power domain reference obtained by this function must have a symmetric
+ * call to intel_display_power_put() to release the reference again.
+ */
+bool intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv,
+					enum intel_display_power_domain domain)
+{
+	struct i915_power_domains *power_domains = &dev_priv->power_domains;
+	bool is_enabled;
+
+	if (!intel_runtime_pm_get_if_in_use(dev_priv))
+		return false;
+
+	mutex_lock(&power_domains->lock);
+
+	if (__intel_display_power_is_enabled(dev_priv, domain)) {
+		__intel_display_power_get_domain(dev_priv, domain);
+		is_enabled = true;
+	} else {
+		is_enabled = false;
+	}
 
 	mutex_unlock(&power_domains->lock);
+
+	if (!is_enabled)
+		intel_runtime_pm_put(dev_priv);
+
+	return is_enabled;
 }
 
 /**
@@ -2239,6 +2284,30 @@ void intel_runtime_pm_get(struct drm_i915_private *dev_priv)
 }
 
 /**
+ * intel_runtime_pm_get_if_in_use - grab a runtime pm reference if device in use
+ * @dev_priv: i915 device instance
+ *
+ * This function grabs a device-level runtime pm reference if the device is
+ * already in use and ensures that it is powered up.
+ *
+ * Any runtime pm reference obtained by this function must have a symmetric
+ * call to intel_runtime_pm_put() to release the reference again.
+ */
+bool intel_runtime_pm_get_if_in_use(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = dev_priv->dev;
+	struct device *device = &dev->pdev->dev;
+
+	if (IS_ENABLED(CONFIG_PM) && !pm_runtime_get_if_in_use(device))
+		return false;
+
+	atomic_inc(&dev_priv->pm.wakeref_count);
+	assert_rpm_wakelock_held(dev_priv);
+
+	return true;
+}
+
+/**
  * intel_runtime_pm_get_noresume - grab a runtime pm reference
  * @dev_priv: i915 device instance
  *
-- 
2.5.0

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

✗ Fi.CI.BAT: failure for drm/i915: add missing display power refs (rev2)

[Patchwork]

== Summary ==

Series 3372v2 drm/i915: add missing display power refs
http://patchwork.freedesktop.org/api/1.0/series/3372/revisions/2/mbox/

Test gem_ringfill:
        Subgroup basic-default-hang:
                incomplete -> PASS       (snb-dellxps)
Test gem_sync:
        Subgroup basic-bsd:
                dmesg-fail -> PASS       (ilk-hp8440p)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                dmesg-warn -> PASS       (ilk-hp8440p) UNSTABLE
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                pass       -> DMESG-WARN (bsw-nuc-2)
                pass       -> FAIL       (bdw-nuci7)
                dmesg-warn -> PASS       (byt-nuc)
        Subgroup basic-rte:
                dmesg-warn -> PASS       (bsw-nuc-2)
                pass       -> DMESG-WARN (byt-nuc) UNSTABLE

bdw-nuci7        total:162  pass:151  dwarn:0   dfail:0   fail:1   skip:10 
bdw-ultra        total:165  pass:152  dwarn:0   dfail:0   fail:0   skip:13 
bsw-nuc-2        total:165  pass:135  dwarn:1   dfail:0   fail:0   skip:29 
byt-nuc          total:165  pass:140  dwarn:1   dfail:0   fail:0   skip:24 
hsw-brixbox      total:165  pass:151  dwarn:0   dfail:0   fail:0   skip:14 
hsw-gt2          total:165  pass:154  dwarn:0   dfail:0   fail:1   skip:10 
ilk-hp8440p      total:165  pass:116  dwarn:0   dfail:0   fail:1   skip:48 
ivb-t430s        total:165  pass:150  dwarn:0   dfail:0   fail:1   skip:14 
skl-i5k-2        total:165  pass:150  dwarn:0   dfail:0   fail:0   skip:15 
snb-dellxps      total:165  pass:142  dwarn:0   dfail:0   fail:1   skip:22 
snb-x220t        total:165  pass:142  dwarn:0   dfail:0   fail:2   skip:21 

Results at /archive/results/CI_IGT_test/Patchwork_1409/

63cbdd1816fd78d404ed004b0f931c497625e0df drm-intel-nightly: 2016y-02m-16d-09h-41m-02s UTC integration manifest
9923732a7a5f183a295642371aa187600871c898 drm/i915/lvds: ensure the HW is powered during HW state readout
f7a8a5be29fc5fde49daf748fcbafe790223ca11 drm/i915/hdmi: ensure the HW is powered during HW state readout
753669366575204f86ea0db5aa2c99ff30ab44e2 drm/i915/dsi: ensure the HW is powered during HW state readout
23c646a338c69c70042ed2a127eb7e42b7e77a5c drm/i915/dp: ensure the HW is powered during HW state readout
8c1533bd7663d4326ec0642f4ef8b5a43bf6767e drm/i915: ensure the HW is powered when accessing the CRC HW block
bba254c2aaf4cddb4829af49741d39b47703510c drm/i915/ddi: ensure the HW is powered during HW state readout
c7989f51cb7a21cdff7eb7379bd32e4c47d41bfa drm/i915/crt: ensure the HW is powered during HW state readout
f9d1e8e1efbf2a931c87a290c98d0c79cbec0334 drm/i915: ensure the HW is powered during HW access in assert_pipe
d943a586787d57c69473552ff3cc85316992d64f drm/i915: ensure the HW is powered when disabling VGA
56c822adb459424867154e3fa84e6f11d6900c28 drm/i915/ibx: ensure the HW is powered during PLL HW readout
e72de3c6552b75c65344646fce44e1c0b02d1c0a drm/i915: ensure the HW is powered during display pipe HW readout
11f811f42d47b5fe6e56629791bf067425eecba0 drm/i915: add helper to get a display power ref if it was already enabled

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

Re: [PATCH 00/12] drm/i915: add missing display power refs

[Mika Kuoppala]

Imre Deak <imre.deak@intel.com> writes:

> This patchset adds missing display power domain references during
> modeset HW readout, which I noticed via wakeref warnings the
> corresponding HW accesses triggered. The crt and ddi patches have a
> concrete bugzilla reference they fix, I don't know of reports that would
> be fixed by the rest of the patches in the set. On the way I also
> noticed other places which are potentially racy (debugfs CRC, VGA
>  disabling, pipe_assert) so I fixed those up too.
>
> I left the IRQ setup and error capture code as-is: the former happens
> during driver loading and resume, so the power is guaranteed to stay on
> for the whole sequence. For error capture, we decided early on that we
> won't add any locking around these accesses to minimize the chance for
> locking inversions. Also for this we would need to grab a mutex which
> isn't possible on the error capture path.
>
> Mika came up with a very similar fix, since he suspects that it could
> also be related to recent DMC problems. We agreed that I send mine since
> it uses the more optimal rpm_get_if_in_use() helper (and looks more
> polished).

Yes it seems to help a quite alot, although some hardening
of dcstate writes is still needed on top of this patchset.

I didn't find fix for skl_ddb_get_hw_state() on the series
and it looks like it needs the ref also.

Thanks,
-Mika

>
> Imre Deak (12):
>   drm/i915: add helper to get a display power ref if it was already
>     enabled
>   drm/i915: ensure the HW is powered during display pipe HW readout
>   drm/i915/ibx: ensure the HW is powered during PLL HW readout
>   drm/i915: ensure the HW is powered when disabling VGA
>   drm/i915: ensure the HW is powered during HW access in assert_pipe
>   drm/i915/crt: ensure the HW is powered during HW state readout
>   drm/i915/ddi: ensure the HW is powered during HW state readout
>   drm/i915: ensure the HW is powered when accessing the CRC HW block
>   drm/i915/dp: ensure the HW is powered during HW state readout
>   drm/i915/dsi: ensure the HW is powered during HW state readout
>   drm/i915/hdmi: ensure the HW is powered during HW state readout
>   drm/i915/lvds: ensure the HW is powered during HW state readout
>
>  drivers/gpu/drm/i915/i915_debugfs.c     |  28 ++++++--
>  drivers/gpu/drm/i915/intel_crt.c        |  13 +++-
>  drivers/gpu/drm/i915/intel_ddi.c        | 116 ++++++++++++++++++++++----------
>  drivers/gpu/drm/i915/intel_display.c    |  86 ++++++++++++++++-------
>  drivers/gpu/drm/i915/intel_dp.c         |  18 +++--
>  drivers/gpu/drm/i915/intel_drv.h        |   3 +
>  drivers/gpu/drm/i915/intel_dsi.c        |  13 +++-
>  drivers/gpu/drm/i915/intel_hdmi.c       |  14 +++-
>  drivers/gpu/drm/i915/intel_lvds.c       |  14 +++-
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 111 +++++++++++++++++++++++++-----
>  10 files changed, 315 insertions(+), 101 deletions(-)
>
> -- 
> 2.5.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/12] drm/i915: ensure the HW is powered when accessing the CRC HW block

[Daniel Vetter]

On Fri, Feb 12, 2016 at 06:55:17PM +0200, Imre Deak wrote:
> The assumption when adding the intel_display_power_is_enabled() checks
> was that if it returns success the power can't be turned off afterwards
> during the HW access, which is guaranteed by modeset locks. This isn't
> always true, so make sure we hold a dedicated reference for the time of
> the access.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>

subject needs to be fixed to mention that this is for all the stuff in
i915_debugfs.c. With that fixed for patches 4-8:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>


> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 28 +++++++++++++++++++++-------
>  1 file changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index ec0c2a05e..9e19cf0 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -825,8 +825,11 @@ static int i915_interrupt_info(struct seq_file *m, void *data)
>  		}
>  
>  		for_each_pipe(dev_priv, pipe) {
> -			if (!intel_display_power_is_enabled(dev_priv,
> -						POWER_DOMAIN_PIPE(pipe))) {
> +			enum intel_display_power_domain power_domain;
> +
> +			power_domain = POWER_DOMAIN_PIPE(pipe);
> +			if (!intel_display_power_get_if_enabled(dev_priv,
> +								power_domain)) {
>  				seq_printf(m, "Pipe %c power disabled\n",
>  					   pipe_name(pipe));
>  				continue;
> @@ -840,6 +843,8 @@ static int i915_interrupt_info(struct seq_file *m, void *data)
>  			seq_printf(m, "Pipe %c IER:\t%08x\n",
>  				   pipe_name(pipe),
>  				   I915_READ(GEN8_DE_PIPE_IER(pipe)));
> +
> +			intel_display_power_put(dev_priv, power_domain);
>  		}
>  
>  		seq_printf(m, "Display Engine port interrupt mask:\t%08x\n",
> @@ -4004,6 +4009,7 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
>  	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
>  	struct intel_crtc *crtc = to_intel_crtc(intel_get_crtc_for_pipe(dev,
>  									pipe));
> +	enum intel_display_power_domain power_domain;
>  	u32 val = 0; /* shut up gcc */
>  	int ret;
>  
> @@ -4014,7 +4020,8 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
>  	if (pipe_crc->source && source)
>  		return -EINVAL;
>  
> -	if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_PIPE(pipe))) {
> +	power_domain = POWER_DOMAIN_PIPE(pipe);
> +	if (!intel_display_power_get_if_enabled(dev_priv, power_domain)) {
>  		DRM_DEBUG_KMS("Trying to capture CRC while pipe is off\n");
>  		return -EIO;
>  	}
> @@ -4031,7 +4038,7 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
>  		ret = ivb_pipe_crc_ctl_reg(dev, pipe, &source, &val);
>  
>  	if (ret != 0)
> -		return ret;
> +		goto out;
>  
>  	/* none -> real source transition */
>  	if (source) {
> @@ -4043,8 +4050,10 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
>  		entries = kcalloc(INTEL_PIPE_CRC_ENTRIES_NR,
>  				  sizeof(pipe_crc->entries[0]),
>  				  GFP_KERNEL);
> -		if (!entries)
> -			return -ENOMEM;
> +		if (!entries) {
> +			ret = -ENOMEM;
> +			goto out;
> +		}
>  
>  		/*
>  		 * When IPS gets enabled, the pipe CRC changes. Since IPS gets
> @@ -4100,7 +4109,12 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
>  		hsw_enable_ips(crtc);
>  	}
>  
> -	return 0;
> +	ret = 0;
> +
> +out:
> +	intel_display_power_put(dev_priv, power_domain);
> +
> +	return ret;
>  }
>  
>  /*
> -- 
> 2.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 12/12] drm/i915/lvds: ensure the HW is powered during HW state readout

[Daniel Vetter]

On Fri, Feb 12, 2016 at 06:55:21PM +0200, Imre Deak wrote:
> The assumption when adding the intel_display_power_is_enabled() checks
> was that if it returns success the power can't be turned off afterwards
> during the HW access, which is guaranteed by modeset locks. This isn't
> always true, so make sure we hold a dedicated reference for the time of
> the access.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>

For patches 9-12:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I'm assuming you're working on more patches to get rid of all the
powe_is_enabled() checks, so that we can remove this fundamentally unsafe
function evenutally?
-Daniel
> ---
>  drivers/gpu/drm/i915/intel_lvds.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 811ddf7..30a8403 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -76,22 +76,30 @@ static bool intel_lvds_get_hw_state(struct intel_encoder *encoder,
>  	struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(&encoder->base);
>  	enum intel_display_power_domain power_domain;
>  	u32 tmp;
> +	bool ret;
>  
>  	power_domain = intel_display_port_power_domain(encoder);
> -	if (!intel_display_power_is_enabled(dev_priv, power_domain))
> +	if (!intel_display_power_get_if_enabled(dev_priv, power_domain))
>  		return false;
>  
> +	ret = false;
> +
>  	tmp = I915_READ(lvds_encoder->reg);
>  
>  	if (!(tmp & LVDS_PORT_EN))
> -		return false;
> +		goto out;
>  
>  	if (HAS_PCH_CPT(dev))
>  		*pipe = PORT_TO_PIPE_CPT(tmp);
>  	else
>  		*pipe = PORT_TO_PIPE(tmp);
>  
> -	return true;
> +	ret = true;
> +
> +out:
> +	intel_display_power_put(dev_priv, power_domain);
> +
> +	return ret;
>  }
>  
>  static void intel_lvds_get_config(struct intel_encoder *encoder,
> -- 
> 2.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 12/12] drm/i915/lvds: ensure the HW is powered during HW state readout

[Imre Deak]

On ti, 2016-02-16 at 17:05 +0100, Daniel Vetter wrote:
> On Fri, Feb 12, 2016 at 06:55:21PM +0200, Imre Deak wrote:
> > The assumption when adding the intel_display_power_is_enabled()
> > checks
> > was that if it returns success the power can't be turned off
> > afterwards
> > during the HW access, which is guaranteed by modeset locks. This
> > isn't
> > always true, so make sure we hold a dedicated reference for the
> > time of
> > the access.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> For patches 9-12:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> I'm assuming you're working on more patches to get rid of all the
> powe_is_enabled() checks, so that we can remove this fundamentally
> unsafe
> function evenutally?

Mika noticed that I missed to convert the one in
skl_ddb_get_hw_state(), I'll send a patch for it.

For the other two remaining callers I gave an explanation in the cover
letter as to why I haven't converted them.

--Imre


> -Daniel
> > ---
> >  drivers/gpu/drm/i915/intel_lvds.c | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_lvds.c
> > b/drivers/gpu/drm/i915/intel_lvds.c
> > index 811ddf7..30a8403 100644
> > --- a/drivers/gpu/drm/i915/intel_lvds.c
> > +++ b/drivers/gpu/drm/i915/intel_lvds.c
> > @@ -76,22 +76,30 @@ static bool intel_lvds_get_hw_state(struct
> > intel_encoder *encoder,
> >  	struct intel_lvds_encoder *lvds_encoder =
> > to_lvds_encoder(&encoder->base);
> >  	enum intel_display_power_domain power_domain;
> >  	u32 tmp;
> > +	bool ret;
> >  
> >  	power_domain = intel_display_port_power_domain(encoder);
> > -	if (!intel_display_power_is_enabled(dev_priv,
> > power_domain))
> > +	if (!intel_display_power_get_if_enabled(dev_priv,
> > power_domain))
> >  		return false;
> >  
> > +	ret = false;
> > +
> >  	tmp = I915_READ(lvds_encoder->reg);
> >  
> >  	if (!(tmp & LVDS_PORT_EN))
> > -		return false;
> > +		goto out;
> >  
> >  	if (HAS_PCH_CPT(dev))
> >  		*pipe = PORT_TO_PIPE_CPT(tmp);
> >  	else
> >  		*pipe = PORT_TO_PIPE(tmp);
> >  
> > -	return true;
> > +	ret = true;
> > +
> > +out:
> > +	intel_display_power_put(dev_priv, power_domain);
> > +
> > +	return ret;
> >  }
> >  
> >  static void intel_lvds_get_config(struct intel_encoder *encoder,
> > -- 
> > 2.5.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 00/12] drm/i915: add missing display power refs

[Imre Deak]

On ti, 2016-02-16 at 16:04 +0200, Mika Kuoppala wrote:
> Imre Deak <imre.deak@intel.com> writes:
> 
> > This patchset adds missing display power domain references during
> > modeset HW readout, which I noticed via wakeref warnings the
> > corresponding HW accesses triggered. The crt and ddi patches have a
> > concrete bugzilla reference they fix, I don't know of reports that
> > would
> > be fixed by the rest of the patches in the set. On the way I also
> > noticed other places which are potentially racy (debugfs CRC, VGA
> >  disabling, pipe_assert) so I fixed those up too.
> > 
> > I left the IRQ setup and error capture code as-is: the former
> > happens
> > during driver loading and resume, so the power is guaranteed to
> > stay on
> > for the whole sequence. For error capture, we decided early on that
> > we
> > won't add any locking around these accesses to minimize the chance
> > for
> > locking inversions. Also for this we would need to grab a mutex
> > which
> > isn't possible on the error capture path.
> > 
> > Mika came up with a very similar fix, since he suspects that it
> > could
> > also be related to recent DMC problems. We agreed that I send mine
> > since
> > it uses the more optimal rpm_get_if_in_use() helper (and looks more
> > polished).
> 
> Yes it seems to help a quite alot, although some hardening
> of dcstate writes is still needed on top of this patchset.
> 
> I didn't find fix for skl_ddb_get_hw_state() on the series
> and it looks like it needs the ref also.

Thanks, I missed that. Will send a patch converting that one too.

--Imre

> 
> Thanks,
> -Mika
> 
> > 
> > Imre Deak (12):
> >   drm/i915: add helper to get a display power ref if it was already
> >     enabled
> >   drm/i915: ensure the HW is powered during display pipe HW readout
> >   drm/i915/ibx: ensure the HW is powered during PLL HW readout
> >   drm/i915: ensure the HW is powered when disabling VGA
> >   drm/i915: ensure the HW is powered during HW access in
> > assert_pipe
> >   drm/i915/crt: ensure the HW is powered during HW state readout
> >   drm/i915/ddi: ensure the HW is powered during HW state readout
> >   drm/i915: ensure the HW is powered when accessing the CRC HW
> > block
> >   drm/i915/dp: ensure the HW is powered during HW state readout
> >   drm/i915/dsi: ensure the HW is powered during HW state readout
> >   drm/i915/hdmi: ensure the HW is powered during HW state readout
> >   drm/i915/lvds: ensure the HW is powered during HW state readout
> > 
> >  drivers/gpu/drm/i915/i915_debugfs.c     |  28 ++++++--
> >  drivers/gpu/drm/i915/intel_crt.c        |  13 +++-
> >  drivers/gpu/drm/i915/intel_ddi.c        | 116
> > ++++++++++++++++++++++----------
> >  drivers/gpu/drm/i915/intel_display.c    |  86 ++++++++++++++++--
> > -----
> >  drivers/gpu/drm/i915/intel_dp.c         |  18 +++--
> >  drivers/gpu/drm/i915/intel_drv.h        |   3 +
> >  drivers/gpu/drm/i915/intel_dsi.c        |  13 +++-
> >  drivers/gpu/drm/i915/intel_hdmi.c       |  14 +++-
> >  drivers/gpu/drm/i915/intel_lvds.c       |  14 +++-
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 111
> > +++++++++++++++++++++++++-----
> >  10 files changed, 315 insertions(+), 101 deletions(-)
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 01/12] drm/i915: add helper to get a display power ref if it was already enabled

[Imre Deak]

We have many places in the code where we check if a given display power
domain is enabled and if so access registers backed by this power
domain. We assumed that some modeset lock will prevent the power
reference from vanishing in the middle of the HW access, but this
assumption doesn't always hold. In such cases we get either the wakeref
not held, or an unclaimed register access error message. To fix this in
a future-proof way that's independent of other locks wrap any such
access with a get_ref_if_enabled()/put_ref() pair.

Kudos to Ville and Joonas for the ideas of this new interface.

v2:
- init the power_domains ptr when declaring it everywhere (Joonas)
v3:
- don't report the device to be powered if runtime PM is disabled

CC: Mika Kuoppala <mika.kuoppala@intel.com>
CC: Chris Wilson <chris@chris-wilson.co.uk>
CC: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h        |   3 +
 drivers/gpu/drm/i915/intel_runtime_pm.c | 124 ++++++++++++++++++++++++++------
 2 files changed, 106 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3cae376..f95f8b2 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1436,6 +1436,8 @@ bool __intel_display_power_is_enabled(struct drm_i915_private *dev_priv,
 				      enum intel_display_power_domain domain);
 void intel_display_power_get(struct drm_i915_private *dev_priv,
 			     enum intel_display_power_domain domain);
+bool intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv,
+					enum intel_display_power_domain domain);
 void intel_display_power_put(struct drm_i915_private *dev_priv,
 			     enum intel_display_power_domain domain);
 
@@ -1522,6 +1524,7 @@ enable_rpm_wakeref_asserts(struct drm_i915_private *dev_priv)
 	enable_rpm_wakeref_asserts(dev_priv)
 
 void intel_runtime_pm_get(struct drm_i915_private *dev_priv);
+bool intel_runtime_pm_get_if_in_use(struct drm_i915_private *dev_priv);
 void intel_runtime_pm_get_noresume(struct drm_i915_private *dev_priv);
 void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
 
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index bbca527..a2e367c 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -1435,39 +1435,84 @@ static void chv_pipe_power_well_disable(struct drm_i915_private *dev_priv,
 	chv_set_pipe_power_well(dev_priv, power_well, false);
 }
 
-/**
- * intel_display_power_get - grab a power domain reference
- * @dev_priv: i915 device instance
- * @domain: power domain to reference
- *
- * This function grabs a power domain reference for @domain and ensures that the
- * power domain and all its parents are powered up. Therefore users should only
- * grab a reference to the innermost power domain they need.
- *
- * Any power domain reference obtained by this function must have a symmetric
- * call to intel_display_power_put() to release the reference again.
- */
-void intel_display_power_get(struct drm_i915_private *dev_priv,
-			     enum intel_display_power_domain domain)
+static void
+__intel_display_power_get_domain(struct drm_i915_private *dev_priv,
+				 enum intel_display_power_domain domain)
 {
-	struct i915_power_domains *power_domains;
+	struct i915_power_domains *power_domains = &dev_priv->power_domains;
 	struct i915_power_well *power_well;
 	int i;
 
-	intel_runtime_pm_get(dev_priv);
-
-	power_domains = &dev_priv->power_domains;
-
-	mutex_lock(&power_domains->lock);
-
 	for_each_power_well(i, power_well, BIT(domain), power_domains) {
 		if (!power_well->count++)
 			intel_power_well_enable(dev_priv, power_well);
 	}
 
 	power_domains->domain_use_count[domain]++;
+}
+
+/**
+ * intel_display_power_get - grab a power domain reference
+ * @dev_priv: i915 device instance
+ * @domain: power domain to reference
+ *
+ * This function grabs a power domain reference for @domain and ensures that the
+ * power domain and all its parents are powered up. Therefore users should only
+ * grab a reference to the innermost power domain they need.
+ *
+ * Any power domain reference obtained by this function must have a symmetric
+ * call to intel_display_power_put() to release the reference again.
+ */
+void intel_display_power_get(struct drm_i915_private *dev_priv,
+			     enum intel_display_power_domain domain)
+{
+	struct i915_power_domains *power_domains = &dev_priv->power_domains;
+
+	intel_runtime_pm_get(dev_priv);
+
+	mutex_lock(&power_domains->lock);
+
+	__intel_display_power_get_domain(dev_priv, domain);
+
+	mutex_unlock(&power_domains->lock);
+}
+
+/**
+ * intel_display_power_get_if_enabled - grab a reference for an enabled display power domain
+ * @dev_priv: i915 device instance
+ * @domain: power domain to reference
+ *
+ * This function grabs a power domain reference for @domain and ensures that the
+ * power domain and all its parents are powered up. Therefore users should only
+ * grab a reference to the innermost power domain they need.
+ *
+ * Any power domain reference obtained by this function must have a symmetric
+ * call to intel_display_power_put() to release the reference again.
+ */
+bool intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv,
+					enum intel_display_power_domain domain)
+{
+	struct i915_power_domains *power_domains = &dev_priv->power_domains;
+	bool is_enabled;
+
+	if (!intel_runtime_pm_get_if_in_use(dev_priv))
+		return false;
+
+	mutex_lock(&power_domains->lock);
+
+	if (__intel_display_power_is_enabled(dev_priv, domain)) {
+		__intel_display_power_get_domain(dev_priv, domain);
+		is_enabled = true;
+	} else {
+		is_enabled = false;
+	}
 
 	mutex_unlock(&power_domains->lock);
+
+	if (!is_enabled)
+		intel_runtime_pm_put(dev_priv);
+
+	return is_enabled;
 }
 
 /**
@@ -2239,6 +2284,43 @@ void intel_runtime_pm_get(struct drm_i915_private *dev_priv)
 }
 
 /**
+ * intel_runtime_pm_get_if_in_use - grab a runtime pm reference if device in use
+ * @dev_priv: i915 device instance
+ *
+ * This function grabs a device-level runtime pm reference if the device is
+ * already in use and ensures that it is powered up.
+ *
+ * Any runtime pm reference obtained by this function must have a symmetric
+ * call to intel_runtime_pm_put() to release the reference again.
+ */
+bool intel_runtime_pm_get_if_in_use(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = dev_priv->dev;
+	struct device *device = &dev->pdev->dev;
+	int ret;
+
+	if (!IS_ENABLED(CONFIG_PM))
+		return true;
+
+	ret = pm_runtime_get_if_in_use(device);
+
+	/*
+	 * In cases runtime PM is disabled by the RPM core and we get an
+	 * -EINVAL return value we are not supposed to call this function,
+	 * since the power state is undefined. This applies atm to the
+	 * late/early system suspend/resume handlers.
+	 */
+	WARN_ON_ONCE(ret < 0);
+	if (ret <= 0)
+		return false;
+
+	atomic_inc(&dev_priv->pm.wakeref_count);
+	assert_rpm_wakelock_held(dev_priv);
+
+	return true;
+}
+
+/**
  * intel_runtime_pm_get_noresume - grab a runtime pm reference
  * @dev_priv: i915 device instance
  *
-- 
2.5.0

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

Re: [PATCH 08/12] drm/i915: ensure the HW is powered when accessing the CRC HW block

[Imre Deak]

On ti, 2016-02-16 at 17:02 +0100, Daniel Vetter wrote:
> On Fri, Feb 12, 2016 at 06:55:17PM +0200, Imre Deak wrote:
> > The assumption when adding the intel_display_power_is_enabled()
> > checks
> > was that if it returns success the power can't be turned off
> > afterwards
> > during the HW access, which is guaranteed by modeset locks. This
> > isn't
> > always true, so make sure we hold a dedicated reference for the
> > time of
> > the access.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> subject needs to be fixed to mention that this is for all the stuff
> in
> i915_debugfs.c.

Ah, yea i915_interrupt_info() is also changed, will updated that in the
log.

> With that fixed for patches 4-8:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> 
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 28 +++++++++++++++++++++--
> > -----
> >  1 file changed, 21 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index ec0c2a05e..9e19cf0 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -825,8 +825,11 @@ static int i915_interrupt_info(struct seq_file
> > *m, void *data)
> >  		}
> >  
> >  		for_each_pipe(dev_priv, pipe) {
> > -			if
> > (!intel_display_power_is_enabled(dev_priv,
> > -						POWER_DOMAIN_PIPE(
> > pipe))) {
> > +			enum intel_display_power_domain
> > power_domain;
> > +
> > +			power_domain = POWER_DOMAIN_PIPE(pipe);
> > +			if
> > (!intel_display_power_get_if_enabled(dev_priv,
> > +								po
> > wer_domain)) {
> >  				seq_printf(m, "Pipe %c power
> > disabled\n",
> >  					   pipe_name(pipe));
> >  				continue;
> > @@ -840,6 +843,8 @@ static int i915_interrupt_info(struct seq_file
> > *m, void *data)
> >  			seq_printf(m, "Pipe %c IER:\t%08x\n",
> >  				   pipe_name(pipe),
> >  				   I915_READ(GEN8_DE_PIPE_IER(pipe
> > )));
> > +
> > +			intel_display_power_put(dev_priv,
> > power_domain);
> >  		}
> >  
> >  		seq_printf(m, "Display Engine port interrupt
> > mask:\t%08x\n",
> > @@ -4004,6 +4009,7 @@ static int pipe_crc_set_source(struct
> > drm_device *dev, enum pipe pipe,
> >  	struct intel_pipe_crc *pipe_crc = &dev_priv-
> > >pipe_crc[pipe];
> >  	struct intel_crtc *crtc =
> > to_intel_crtc(intel_get_crtc_for_pipe(dev,
> >  									
> > pipe));
> > +	enum intel_display_power_domain power_domain;
> >  	u32 val = 0; /* shut up gcc */
> >  	int ret;
> >  
> > @@ -4014,7 +4020,8 @@ static int pipe_crc_set_source(struct
> > drm_device *dev, enum pipe pipe,
> >  	if (pipe_crc->source && source)
> >  		return -EINVAL;
> >  
> > -	if (!intel_display_power_is_enabled(dev_priv,
> > POWER_DOMAIN_PIPE(pipe))) {
> > +	power_domain = POWER_DOMAIN_PIPE(pipe);
> > +	if (!intel_display_power_get_if_enabled(dev_priv,
> > power_domain)) {
> >  		DRM_DEBUG_KMS("Trying to capture CRC while pipe is
> > off\n");
> >  		return -EIO;
> >  	}
> > @@ -4031,7 +4038,7 @@ static int pipe_crc_set_source(struct
> > drm_device *dev, enum pipe pipe,
> >  		ret = ivb_pipe_crc_ctl_reg(dev, pipe, &source,
> > &val);
> >  
> >  	if (ret != 0)
> > -		return ret;
> > +		goto out;
> >  
> >  	/* none -> real source transition */
> >  	if (source) {
> > @@ -4043,8 +4050,10 @@ static int pipe_crc_set_source(struct
> > drm_device *dev, enum pipe pipe,
> >  		entries = kcalloc(INTEL_PIPE_CRC_ENTRIES_NR,
> >  				  sizeof(pipe_crc->entries[0]),
> >  				  GFP_KERNEL);
> > -		if (!entries)
> > -			return -ENOMEM;
> > +		if (!entries) {
> > +			ret = -ENOMEM;
> > +			goto out;
> > +		}
> >  
> >  		/*
> >  		 * When IPS gets enabled, the pipe CRC changes.
> > Since IPS gets
> > @@ -4100,7 +4109,12 @@ static int pipe_crc_set_source(struct
> > drm_device *dev, enum pipe pipe,
> >  		hsw_enable_ips(crtc);
> >  	}
> >  
> > -	return 0;
> > +	ret = 0;
> > +
> > +out:
> > +	intel_display_power_put(dev_priv, power_domain);
> > +
> > +	return ret;
> >  }
> >  
> >  /*
> > -- 
> > 2.5.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: add missing display power refs (rev3)

[Patchwork]

== Summary ==

Series 3372v3 drm/i915: add missing display power refs
http://patchwork.freedesktop.org/api/1.0/series/3372/revisions/3/mbox/

Test pm_rpm:
        Subgroup basic-pci-d3-state:
                dmesg-warn -> PASS       (bsw-nuc-2)

bsw-nuc-2        total:165  pass:136  dwarn:0   dfail:0   fail:0   skip:29 

Results at /archive/results/CI_IGT_test/Patchwork_1423/

e99a344c64936ee4a67459f22e5a72bc4ae470bf drm-intel-nightly: 2016y-02m-17d-11h-25m-24s UTC integration manifest
6bac804f8e89ac717f5344b2797775cb943ae920 drm/i915/lvds: ensure the HW is powered during HW state readout
9909cd7334eed8d7e889020313ddc2d22ff278ab drm/i915/hdmi: ensure the HW is powered during HW state readout
9aa76ff07c56503d61712402e0079d3fe56c79b9 drm/i915/dsi: ensure the HW is powered during HW state readout
08f821a5cbc83b3c1f70f575365d70c2d23bdce7 drm/i915/dp: ensure the HW is powered during HW state readout
d04bf77e309852d193e74b646e685c1bf20a2732 drm/i915: ensure the HW is powered when accessing the CRC HW block
4c00c62f648389fad9b892fd5079c4fe948e1125 drm/i915/ddi: ensure the HW is powered during HW state readout
efdfa9f8dd45fa1a69eb51319ae052886dc0c815 drm/i915/crt: ensure the HW is powered during HW state readout
e597521ee1a8ba7842db6cccf7cbc90bafcf0a90 drm/i915: ensure the HW is powered during HW access in assert_pipe
5210139ee20523d9aa76908c3196dd11a5ccfb2c drm/i915: ensure the HW is powered when disabling VGA
611b29445a5c346c2b1a3c65a257fe03e2fd881d drm/i915/ibx: ensure the HW is powered during PLL HW readout
2897bf36933467eb7cfddbd7e114a516187903d8 drm/i915: ensure the HW is powered during display pipe HW readout
f7ae78f860cf13df1bdc2b3f35d43fa900efe689 drm/i915: add helper to get a display power ref if it was already enabled

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

Re: ✓ Fi.CI.BAT: success for drm/i915: add missing display power refs (rev3)

[Imre Deak]

On ke, 2016-02-17 at 12:44 +0000, Patchwork wrote:
> == Summary ==
> 
> Series 3372v3 drm/i915: add missing display power refs
> http://patchwork.freedesktop.org/api/1.0/series/3372/revisions/3/mbox
> /
> 
> Test pm_rpm:
>         Subgroup basic-pci-d3-state:
>                 dmesg-warn -> PASS       (bsw-nuc-2)
> 
> bsw-nuc-
> 2        total:165  pass:136  dwarn:0   dfail:0   fail:0   skip:29 
> 
> Results at /archive/results/CI_IGT_test/Patchwork_1423/

I pushed the patchset to -dinq, thanks for the review.

--Imre

> e99a344c64936ee4a67459f22e5a72bc4ae470bf drm-intel-nightly: 2016y-
> 02m-17d-11h-25m-24s UTC integration manifest
> 6bac804f8e89ac717f5344b2797775cb943ae920 drm/i915/lvds: ensure the HW
> is powered during HW state readout
> 9909cd7334eed8d7e889020313ddc2d22ff278ab drm/i915/hdmi: ensure the HW
> is powered during HW state readout
> 9aa76ff07c56503d61712402e0079d3fe56c79b9 drm/i915/dsi: ensure the HW
> is powered during HW state readout
> 08f821a5cbc83b3c1f70f575365d70c2d23bdce7 drm/i915/dp: ensure the HW
> is powered during HW state readout
> d04bf77e309852d193e74b646e685c1bf20a2732 drm/i915: ensure the HW is
> powered when accessing the CRC HW block
> 4c00c62f648389fad9b892fd5079c4fe948e1125 drm/i915/ddi: ensure the HW
> is powered during HW state readout
> efdfa9f8dd45fa1a69eb51319ae052886dc0c815 drm/i915/crt: ensure the HW
> is powered during HW state readout
> e597521ee1a8ba7842db6cccf7cbc90bafcf0a90 drm/i915: ensure the HW is
> powered during HW access in assert_pipe
> 5210139ee20523d9aa76908c3196dd11a5ccfb2c drm/i915: ensure the HW is
> powered when disabling VGA
> 611b29445a5c346c2b1a3c65a257fe03e2fd881d drm/i915/ibx: ensure the HW
> is powered during PLL HW readout
> 2897bf36933467eb7cfddbd7e114a516187903d8 drm/i915: ensure the HW is
> powered during display pipe HW readout
> f7ae78f860cf13df1bdc2b3f35d43fa900efe689 drm/i915: add helper to get
> a display power ref if it was already enabled
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/12] drm/i915: ensure the HW is powered when accessing the CRC HW block

[Daniel Vetter]

On Wed, Feb 17, 2016 at 02:20:49PM +0200, Imre Deak wrote:
> On ti, 2016-02-16 at 17:02 +0100, Daniel Vetter wrote:
> > On Fri, Feb 12, 2016 at 06:55:17PM +0200, Imre Deak wrote:
> > > The assumption when adding the intel_display_power_is_enabled()
> > > checks
> > > was that if it returns success the power can't be turned off
> > > afterwards
> > > during the HW access, which is guaranteed by modeset locks. This
> > > isn't
> > > always true, so make sure we hold a dedicated reference for the
> > > time of
> > > the access.
> > > 
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > 
> > subject needs to be fixed to mention that this is for all the stuff
> > in
> > i915_debugfs.c.
> 
> Ah, yea i915_interrupt_info() is also changed, will updated that in the
> log.

Ah right makes sense. I thought I counted more to convert in
intel_display.c than what was in your patches, plus the error capture
stuff. And not converting error capture is indeed what we need to do ...
-Daniel

> 
> > With that fixed for patches 4-8:
> > 
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > 
> > > ---
> > >  drivers/gpu/drm/i915/i915_debugfs.c | 28 +++++++++++++++++++++--
> > > -----
> > >  1 file changed, 21 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > index ec0c2a05e..9e19cf0 100644
> > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > @@ -825,8 +825,11 @@ static int i915_interrupt_info(struct seq_file
> > > *m, void *data)
> > >  		}
> > >  
> > >  		for_each_pipe(dev_priv, pipe) {
> > > -			if
> > > (!intel_display_power_is_enabled(dev_priv,
> > > -						POWER_DOMAIN_PIPE(
> > > pipe))) {
> > > +			enum intel_display_power_domain
> > > power_domain;
> > > +
> > > +			power_domain = POWER_DOMAIN_PIPE(pipe);
> > > +			if
> > > (!intel_display_power_get_if_enabled(dev_priv,
> > > +								po
> > > wer_domain)) {
> > >  				seq_printf(m, "Pipe %c power
> > > disabled\n",
> > >  					   pipe_name(pipe));
> > >  				continue;
> > > @@ -840,6 +843,8 @@ static int i915_interrupt_info(struct seq_file
> > > *m, void *data)
> > >  			seq_printf(m, "Pipe %c IER:\t%08x\n",
> > >  				   pipe_name(pipe),
> > >  				   I915_READ(GEN8_DE_PIPE_IER(pipe
> > > )));
> > > +
> > > +			intel_display_power_put(dev_priv,
> > > power_domain);
> > >  		}
> > >  
> > >  		seq_printf(m, "Display Engine port interrupt
> > > mask:\t%08x\n",
> > > @@ -4004,6 +4009,7 @@ static int pipe_crc_set_source(struct
> > > drm_device *dev, enum pipe pipe,
> > >  	struct intel_pipe_crc *pipe_crc = &dev_priv-
> > > >pipe_crc[pipe];
> > >  	struct intel_crtc *crtc =
> > > to_intel_crtc(intel_get_crtc_for_pipe(dev,
> > >  									
> > > pipe));
> > > +	enum intel_display_power_domain power_domain;
> > >  	u32 val = 0; /* shut up gcc */
> > >  	int ret;
> > >  
> > > @@ -4014,7 +4020,8 @@ static int pipe_crc_set_source(struct
> > > drm_device *dev, enum pipe pipe,
> > >  	if (pipe_crc->source && source)
> > >  		return -EINVAL;
> > >  
> > > -	if (!intel_display_power_is_enabled(dev_priv,
> > > POWER_DOMAIN_PIPE(pipe))) {
> > > +	power_domain = POWER_DOMAIN_PIPE(pipe);
> > > +	if (!intel_display_power_get_if_enabled(dev_priv,
> > > power_domain)) {
> > >  		DRM_DEBUG_KMS("Trying to capture CRC while pipe is
> > > off\n");
> > >  		return -EIO;
> > >  	}
> > > @@ -4031,7 +4038,7 @@ static int pipe_crc_set_source(struct
> > > drm_device *dev, enum pipe pipe,
> > >  		ret = ivb_pipe_crc_ctl_reg(dev, pipe, &source,
> > > &val);
> > >  
> > >  	if (ret != 0)
> > > -		return ret;
> > > +		goto out;
> > >  
> > >  	/* none -> real source transition */
> > >  	if (source) {
> > > @@ -4043,8 +4050,10 @@ static int pipe_crc_set_source(struct
> > > drm_device *dev, enum pipe pipe,
> > >  		entries = kcalloc(INTEL_PIPE_CRC_ENTRIES_NR,
> > >  				  sizeof(pipe_crc->entries[0]),
> > >  				  GFP_KERNEL);
> > > -		if (!entries)
> > > -			return -ENOMEM;
> > > +		if (!entries) {
> > > +			ret = -ENOMEM;
> > > +			goto out;
> > > +		}
> > >  
> > >  		/*
> > >  		 * When IPS gets enabled, the pipe CRC changes.
> > > Since IPS gets
> > > @@ -4100,7 +4109,12 @@ static int pipe_crc_set_source(struct
> > > drm_device *dev, enum pipe pipe,
> > >  		hsw_enable_ips(crtc);
> > >  	}
> > >  
> > > -	return 0;
> > > +	ret = 0;
> > > +
> > > +out:
> > > +	intel_display_power_put(dev_priv, power_domain);
> > > +
> > > +	return ret;
> > >  }
> > >  
> > >  /*
> > > -- 
> > > 2.5.0
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx