[RFC] Runtime display PM for VLV/BYT

[RFC] Runtime display PM for VLV/BYT

[Jesse Barnes]

This set adds bits needed for runtime power support, currently only
lightly tested on VLV/BYT:
  1) suspend/resume callbacks for different platforms
  2) save/restore of display state across a power well toggle
  3) get/put of display power well in critical places

The TODO list still has a few items on it, and I'm looking for feedback:
  1) sprinkle around some power well WARNs so we can catch things easily
  2) add some tests using DPMS and NULL mode sets and comparing power
     well state
  3) better debugfs support for multiple wells
  4) refcount of power well in debugfs (with ref holders?)
  5) more testing - I think the load time ref is still busted here and
     on HSW
  6) convert HSW as well so DPMS will shut things down, not just mode
     sets

Thoughts or comments?

Thanks,
Jesse

[PATCH 1/5] drm/i915/vlv: power well support for VLV/BYT

[Jesse Barnes]

Had to conditionalize some HSW bits and add virtual functions for
get/set on the power wells.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_drv.h     |  10 +-
 drivers/gpu/drm/i915/intel_drv.h    |   2 +
 drivers/gpu/drm/i915/intel_pm.c     | 128 +++-------------------
 drivers/gpu/drm/i915/intel_uncore.c | 211 ++++++++++++++++++++++++++++++++++++
 4 files changed, 238 insertions(+), 113 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ca74ef2..4e97840 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -401,6 +401,14 @@ struct drm_i915_display_funcs {
 struct intel_uncore_funcs {
 	void (*force_wake_get)(struct drm_i915_private *dev_priv);
 	void (*force_wake_put)(struct drm_i915_private *dev_priv);
+	void (*set_display_power)(struct drm_i915_private *dev_priv,
+				  bool enable);
+	bool (*display_power_enabled)(struct drm_i915_private *dev_priv,
+				      enum intel_display_power_domain domain);
+	void (*display_power_get)(struct drm_i915_private *dev_priv,
+				  enum intel_display_power_domain domain);
+	void (*display_power_put)(struct drm_i915_private *dev_priv,
+				  enum intel_display_power_domain domain);
 };
 
 struct intel_uncore {
@@ -1702,7 +1710,7 @@ struct drm_i915_file_private {
 #define HAS_IPS(dev)		(IS_ULT(dev))
 
 #define HAS_DDI(dev)		(INTEL_INFO(dev)->has_ddi)
-#define HAS_POWER_WELL(dev)	(IS_HASWELL(dev))
+#define HAS_POWER_WELL(dev)	(IS_HASWELL(dev) || IS_VALLEYVIEW(dev))
 #define HAS_FPGA_DBG_UNCLAIMED(dev)	(INTEL_INFO(dev)->has_fpga_dbg)
 #define HAS_PSR(dev)		(IS_HASWELL(dev))
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 476d98b..9317383 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -798,6 +798,8 @@ void intel_display_power_get(struct drm_device *dev,
 			     enum intel_display_power_domain domain);
 void intel_display_power_put(struct drm_device *dev,
 			     enum intel_display_power_domain domain);
+void __intel_power_well_get(struct i915_power_well *power_well);
+void __intel_power_well_put(struct i915_power_well *power_well);
 void intel_init_power_well(struct drm_device *dev);
 void intel_set_power_well(struct drm_device *dev, bool enable);
 void intel_enable_gt_powersave(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 0b4de57..d1abc42 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5367,146 +5367,48 @@ bool intel_display_power_enabled(struct drm_device *dev,
 	if (!HAS_POWER_WELL(dev))
 		return true;
 
-	switch (domain) {
-	case POWER_DOMAIN_PIPE_A:
-	case POWER_DOMAIN_TRANSCODER_EDP:
-		return true;
-	case POWER_DOMAIN_VGA:
-	case POWER_DOMAIN_PIPE_B:
-	case POWER_DOMAIN_PIPE_C:
-	case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
-	case POWER_DOMAIN_PIPE_B_PANEL_FITTER:
-	case POWER_DOMAIN_PIPE_C_PANEL_FITTER:
-	case POWER_DOMAIN_TRANSCODER_A:
-	case POWER_DOMAIN_TRANSCODER_B:
-	case POWER_DOMAIN_TRANSCODER_C:
-		return I915_READ(HSW_PWR_WELL_DRIVER) ==
-		     (HSW_PWR_WELL_ENABLE_REQUEST | HSW_PWR_WELL_STATE_ENABLED);
-	default:
-		BUG();
-	}
+	return dev_priv->uncore.funcs.display_power_enabled(dev_priv, domain);
 }
 
-static void __intel_set_power_well(struct drm_device *dev, bool enable)
+void __intel_power_well_get(struct i915_power_well *power_well)
 {
+	struct drm_device *dev = power_well->device;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	bool is_enabled, enable_requested;
-	uint32_t tmp;
 
-	tmp = I915_READ(HSW_PWR_WELL_DRIVER);
-	is_enabled = tmp & HSW_PWR_WELL_STATE_ENABLED;
-	enable_requested = tmp & HSW_PWR_WELL_ENABLE_REQUEST;
-
-	if (enable) {
-		if (!enable_requested)
-			I915_WRITE(HSW_PWR_WELL_DRIVER,
-				   HSW_PWR_WELL_ENABLE_REQUEST);
-
-		if (!is_enabled) {
-			DRM_DEBUG_KMS("Enabling power well\n");
-			if (wait_for((I915_READ(HSW_PWR_WELL_DRIVER) &
-				      HSW_PWR_WELL_STATE_ENABLED), 20))
-				DRM_ERROR("Timeout enabling power well\n");
-		}
-	} else {
-		if (enable_requested) {
-			unsigned long irqflags;
-			enum pipe p;
-
-			I915_WRITE(HSW_PWR_WELL_DRIVER, 0);
-			POSTING_READ(HSW_PWR_WELL_DRIVER);
-			DRM_DEBUG_KMS("Requesting to disable the power well\n");
-
-			/*
-			 * After this, the registers on the pipes that are part
-			 * of the power well will become zero, so we have to
-			 * adjust our counters according to that.
-			 *
-			 * FIXME: Should we do this in general in
-			 * drm_vblank_post_modeset?
-			 */
-			spin_lock_irqsave(&dev->vbl_lock, irqflags);
-			for_each_pipe(p)
-				if (p != PIPE_A)
-					dev->last_vblank[p] = 0;
-			spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
-		}
-	}
-}
-
-static void __intel_power_well_get(struct i915_power_well *power_well)
-{
 	if (!power_well->count++)
-		__intel_set_power_well(power_well->device, true);
+		dev_priv->uncore.funcs.set_display_power(dev_priv, true);
 }
 
-static void __intel_power_well_put(struct i915_power_well *power_well)
+void __intel_power_well_put(struct i915_power_well *power_well)
 {
+	struct drm_device *dev = power_well->device;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
 	WARN_ON(!power_well->count);
 	if (!--power_well->count)
-		__intel_set_power_well(power_well->device, false);
+		dev_priv->uncore.funcs.set_display_power(dev_priv, false);
 }
 
 void intel_display_power_get(struct drm_device *dev,
 			     enum intel_display_power_domain domain)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct i915_power_well *power_well = &dev_priv->power_well;
 
 	if (!HAS_POWER_WELL(dev))
 		return;
 
-	switch (domain) {
-	case POWER_DOMAIN_PIPE_A:
-	case POWER_DOMAIN_TRANSCODER_EDP:
-		return;
-	case POWER_DOMAIN_VGA:
-	case POWER_DOMAIN_PIPE_B:
-	case POWER_DOMAIN_PIPE_C:
-	case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
-	case POWER_DOMAIN_PIPE_B_PANEL_FITTER:
-	case POWER_DOMAIN_PIPE_C_PANEL_FITTER:
-	case POWER_DOMAIN_TRANSCODER_A:
-	case POWER_DOMAIN_TRANSCODER_B:
-	case POWER_DOMAIN_TRANSCODER_C:
-		spin_lock_irq(&power_well->lock);
-		__intel_power_well_get(power_well);
-		spin_unlock_irq(&power_well->lock);
-		return;
-	default:
-		BUG();
-	}
+	dev_priv->uncore.funcs.display_power_get(dev_priv, domain);
 }
 
 void intel_display_power_put(struct drm_device *dev,
 			     enum intel_display_power_domain domain)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct i915_power_well *power_well = &dev_priv->power_well;
 
 	if (!HAS_POWER_WELL(dev))
 		return;
 
-	switch (domain) {
-	case POWER_DOMAIN_PIPE_A:
-	case POWER_DOMAIN_TRANSCODER_EDP:
-		return;
-	case POWER_DOMAIN_VGA:
-	case POWER_DOMAIN_PIPE_B:
-	case POWER_DOMAIN_PIPE_C:
-	case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
-	case POWER_DOMAIN_PIPE_B_PANEL_FITTER:
-	case POWER_DOMAIN_PIPE_C_PANEL_FITTER:
-	case POWER_DOMAIN_TRANSCODER_A:
-	case POWER_DOMAIN_TRANSCODER_B:
-	case POWER_DOMAIN_TRANSCODER_C:
-		spin_lock_irq(&power_well->lock);
-		__intel_power_well_put(power_well);
-		spin_unlock_irq(&power_well->lock);
-		return;
-	default:
-		BUG();
-	}
+	dev_priv->uncore.funcs.display_power_put(dev_priv, domain);
 }
 
 static struct i915_power_well *hsw_pwr;
@@ -5595,7 +5497,8 @@ static void intel_resume_power_well(struct drm_device *dev)
 		return;
 
 	spin_lock_irq(&power_well->lock);
-	__intel_set_power_well(dev, power_well->count > 0);
+	dev_priv->uncore.funcs.set_display_power(dev_priv,
+						 power_well->count > 0);
 	spin_unlock_irq(&power_well->lock);
 }
 
@@ -5618,8 +5521,9 @@ void intel_init_power_well(struct drm_device *dev)
 
 	/* We're taking over the BIOS, so clear any requests made by it since
 	 * the driver is in charge now. */
-	if (I915_READ(HSW_PWR_WELL_BIOS) & HSW_PWR_WELL_ENABLE_REQUEST)
-		I915_WRITE(HSW_PWR_WELL_BIOS, 0);
+	if (IS_HASWELL(dev))
+		if (I915_READ(HSW_PWR_WELL_BIOS) & HSW_PWR_WELL_ENABLE_REQUEST)
+			I915_WRITE(HSW_PWR_WELL_BIOS, 0);
 }
 
 /* Disables PC8 so we can use the GMBUS and DP AUX interrupts. */
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 288a3a6..ef5d7fd 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -216,6 +216,209 @@ static void gen6_force_wake_work(struct work_struct *work)
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
 
+static void hsw_display_power_get(struct drm_i915_private *dev_priv,
+				  enum intel_display_power_domain domain)
+{
+	struct i915_power_well *power_well = &dev_priv->power_well;
+
+	switch (domain) {
+	case POWER_DOMAIN_PIPE_A:
+	case POWER_DOMAIN_TRANSCODER_EDP:
+		return;
+	case POWER_DOMAIN_VGA:
+	case POWER_DOMAIN_PIPE_B:
+	case POWER_DOMAIN_PIPE_C:
+	case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
+	case POWER_DOMAIN_PIPE_B_PANEL_FITTER:
+	case POWER_DOMAIN_PIPE_C_PANEL_FITTER:
+	case POWER_DOMAIN_TRANSCODER_A:
+	case POWER_DOMAIN_TRANSCODER_B:
+	case POWER_DOMAIN_TRANSCODER_C:
+		spin_lock_irq(&power_well->lock);
+		__intel_power_well_get(power_well);
+		spin_unlock_irq(&power_well->lock);
+		return;
+	default:
+		BUG();
+	}
+}
+
+static void hsw_display_power_put(struct drm_i915_private *dev_priv,
+				  enum intel_display_power_domain domain)
+{
+	struct i915_power_well *power_well = &dev_priv->power_well;
+	switch (domain) {
+	case POWER_DOMAIN_PIPE_A:
+	case POWER_DOMAIN_TRANSCODER_EDP:
+		return;
+	case POWER_DOMAIN_VGA:
+	case POWER_DOMAIN_PIPE_B:
+	case POWER_DOMAIN_PIPE_C:
+	case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
+	case POWER_DOMAIN_PIPE_B_PANEL_FITTER:
+	case POWER_DOMAIN_PIPE_C_PANEL_FITTER:
+	case POWER_DOMAIN_TRANSCODER_A:
+	case POWER_DOMAIN_TRANSCODER_B:
+	case POWER_DOMAIN_TRANSCODER_C:
+		spin_lock_irq(&power_well->lock);
+		__intel_power_well_put(power_well);
+		spin_unlock_irq(&power_well->lock);
+		return;
+	default:
+		BUG();
+	}
+}
+
+static void vlv_display_power_get(struct drm_i915_private *dev_priv,
+				  enum intel_display_power_domain domain)
+{
+	struct i915_power_well *power_well = &dev_priv->power_well;
+
+	spin_lock_irq(&power_well->lock);
+	__intel_power_well_get(power_well);
+	spin_unlock_irq(&power_well->lock);
+}
+
+static void vlv_display_power_put(struct drm_i915_private *dev_priv,
+				  enum intel_display_power_domain domain)
+{
+	struct i915_power_well *power_well = &dev_priv->power_well;
+
+	spin_lock_irq(&power_well->lock);
+	__intel_power_well_put(power_well);
+	spin_unlock_irq(&power_well->lock);
+}
+
+static bool hsw_display_power_enabled(struct drm_i915_private *dev_priv,
+				      enum intel_display_power_domain domain)
+{
+	switch (domain) {
+	case POWER_DOMAIN_PIPE_A:
+	case POWER_DOMAIN_TRANSCODER_EDP:
+		return true;
+	case POWER_DOMAIN_VGA:
+	case POWER_DOMAIN_PIPE_B:
+	case POWER_DOMAIN_PIPE_C:
+	case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
+	case POWER_DOMAIN_PIPE_B_PANEL_FITTER:
+	case POWER_DOMAIN_PIPE_C_PANEL_FITTER:
+	case POWER_DOMAIN_TRANSCODER_A:
+	case POWER_DOMAIN_TRANSCODER_B:
+	case POWER_DOMAIN_TRANSCODER_C:
+		return I915_READ(HSW_PWR_WELL_DRIVER) ==
+			(HSW_PWR_WELL_ENABLE_REQUEST |
+			 HSW_PWR_WELL_STATE_ENABLED);
+	default:
+		BUG();
+	}
+}
+
+static bool __vlv_get_power_well(struct drm_i915_private *dev_priv,
+				 u32 pwrgt_mask)
+{
+	u32 reg_val;
+
+	mutex_lock(&dev_priv->rps.hw_lock);
+	reg_val = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_STATUS);
+	mutex_unlock(&dev_priv->rps.hw_lock);
+
+	return !(reg_val & pwrgt_mask);
+}
+
+static bool vlv_display_power_enabled(struct drm_i915_private *dev_priv,
+				      enum intel_display_power_domain domain)
+{
+	return __vlv_get_power_well(dev_priv, DISP2D_PWRGT);
+}
+
+static void hsw_set_display_power(struct drm_i915_private *dev_priv,
+				  bool enable)
+{
+	struct drm_device *dev = dev_priv->dev;
+	bool is_enabled, enable_requested;
+	uint32_t tmp;
+
+	tmp = I915_READ(HSW_PWR_WELL_DRIVER);
+	is_enabled = tmp & HSW_PWR_WELL_STATE_ENABLED;
+	enable_requested = tmp & HSW_PWR_WELL_ENABLE_REQUEST;
+
+	if (enable) {
+		if (!enable_requested)
+			I915_WRITE(HSW_PWR_WELL_DRIVER,
+				   HSW_PWR_WELL_ENABLE_REQUEST);
+
+		if (!is_enabled) {
+			DRM_DEBUG_KMS("Enabling power well\n");
+			if (wait_for((I915_READ(HSW_PWR_WELL_DRIVER) &
+				      HSW_PWR_WELL_STATE_ENABLED), 20))
+				DRM_ERROR("Timeout enabling power well\n");
+		}
+	} else {
+		if (enable_requested) {
+			unsigned long irqflags;
+			enum pipe p;
+
+			I915_WRITE(HSW_PWR_WELL_DRIVER, 0);
+			POSTING_READ(HSW_PWR_WELL_DRIVER);
+			DRM_DEBUG_KMS("Requesting to disable the power well\n");
+
+			/*
+			 * After this, the registers on the pipes that are part
+			 * of the power well will become zero, so we have to
+			 * adjust our counters according to that.
+			 *
+			 * FIXME: Should we do this in general in
+			 * drm_vblank_post_modeset?
+			 */
+			spin_lock_irqsave(&dev->vbl_lock, irqflags);
+			for_each_pipe(p)
+				if (p != PIPE_A)
+					dev->last_vblank[p] = 0;
+			spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
+		}
+	}
+}
+
+static void __vlv_set_power_well(struct drm_i915_private *dev_priv,
+				 u32 pwrgt_mask, bool enable)
+{
+	u32 reg_val;
+
+	mutex_lock(&dev_priv->rps.hw_lock);
+	reg_val = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_STATUS);
+	if (enable) {
+		if (reg_val & pwrgt_mask) {
+			reg_val &= ~pwrgt_mask;
+			vlv_punit_write(dev_priv, PUNIT_REG_PWRGT_CTRL,
+					reg_val);
+			if (wait_for(!(vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_STATUS) & pwrgt_mask), 100))
+				DRM_ERROR("timed out waiting for power well enable\n");
+		}
+	} else {
+		reg_val |= pwrgt_mask;
+		vlv_punit_write(dev_priv, PUNIT_REG_PWRGT_CTRL, reg_val);
+		if (wait_for(vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_STATUS) & pwrgt_mask, 100))
+			DRM_ERROR("timed out waiting for power well disable\n");	}
+	reg_val = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_STATUS);
+	mutex_unlock(&dev_priv->rps.hw_lock);
+}
+
+static void vlv_set_display_power(struct drm_i915_private *dev_priv,
+				  bool enable)
+{
+	__vlv_set_power_well(dev_priv, DISP2D_PWRGT, enable);
+}
+
+static void vlv_set_render_power(struct drm_i915_private *dev_priv, bool enable)
+{
+	__vlv_set_power_well(dev_priv, RENDER_PWRGT, enable);
+}
+
+static void vlv_set_media_power(struct drm_i915_private *dev_priv, bool enable)
+{
+	__vlv_set_power_well(dev_priv, MEDIA_PWRGT, enable);
+}
+
 void intel_uncore_early_sanitize(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -234,9 +437,17 @@ void intel_uncore_init(struct drm_device *dev)
 	if (IS_VALLEYVIEW(dev)) {
 		dev_priv->uncore.funcs.force_wake_get = vlv_force_wake_get;
 		dev_priv->uncore.funcs.force_wake_put = vlv_force_wake_put;
+		dev_priv->uncore.funcs.set_display_power = vlv_set_display_power;
+		dev_priv->uncore.funcs.display_power_enabled = vlv_display_power_enabled;
+		dev_priv->uncore.funcs.display_power_get = vlv_display_power_get;
+		dev_priv->uncore.funcs.display_power_put = vlv_display_power_put;
 	} else if (IS_HASWELL(dev)) {
 		dev_priv->uncore.funcs.force_wake_get = __gen6_gt_force_wake_mt_get;
 		dev_priv->uncore.funcs.force_wake_put = __gen6_gt_force_wake_mt_put;
+		dev_priv->uncore.funcs.set_display_power = hsw_set_display_power;
+		dev_priv->uncore.funcs.display_power_enabled = hsw_display_power_enabled;
+		dev_priv->uncore.funcs.display_power_get = hsw_display_power_get;
+		dev_priv->uncore.funcs.display_power_put = hsw_display_power_put;
 	} else if (IS_IVYBRIDGE(dev)) {
 		u32 ecobus;
 
-- 
1.8.3.1

[PATCH 2/5] drm/i915: add display power well report out to debugfs

[Jesse Barnes]

For tracking current state.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 5fd6a5d..9f81e80 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1727,6 +1727,18 @@ static int i915_pc8_status(struct seq_file *m, void *unused)
 	return 0;
 }
 
+static int i915_display_power_status(struct seq_file *m, void *unused)
+{
+	struct drm_info_node *node = (struct drm_info_node *) m->private;
+	struct drm_device *dev = node->minor->dev;
+	bool enabled;
+
+	enabled = intel_display_power_enabled(dev, POWER_DOMAIN_VGA);
+	seq_printf(m, "Display power on: %s\n", yesno(enabled));
+
+	return 0;
+}
+
 static int
 i915_wedged_get(void *data, u64 *val)
 {
@@ -2234,6 +2246,7 @@ static struct drm_info_list i915_debugfs_list[] = {
 	{"i915_edp_psr_status", i915_edp_psr_status, 0},
 	{"i915_energy_uJ", i915_energy_uJ, 0},
 	{"i915_pc8_status", i915_pc8_status, 0},
+	{"i915_display_power_status", i915_display_power_status, 0},
 };
 #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
 
-- 
1.8.3.1

[PATCH 3/5] drm/i915/vlv: suspend/resume fixes for VLV/BYT

[Jesse Barnes]

We were missing a few bits around power well handling and Gunit
save/restore.  The code added should be sufficient for runtime D3 as
well (though that requires additional changes to how we handle
save/restore of state).

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_drv.c     |  31 +++++++++
 drivers/gpu/drm/i915/i915_drv.h     |  11 +++
 drivers/gpu/drm/i915/i915_gem.c     |   5 +-
 drivers/gpu/drm/i915/i915_reg.h     |   8 +++
 drivers/gpu/drm/i915/intel_uncore.c | 133 ++++++++++++++++++++++++++++++++----
 5 files changed, 174 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 82a1d53..01ff272 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -459,6 +459,21 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
 	return 1;
 }
 
+/*
+ * On VLV/BYT, transitioning from D0 to a Dx state requires the following
+ * steps:
+ *   1) force enable gfx clocks with GTLC survivability reg
+ *   2) take force wake ref for register saving (really necessary?)
+ *   3) save Gunit regs
+ *   4) drop gfx freq to minimum
+ *   5) drop force wake ref (not needed if we don't need #2)
+ *   6) clear allow wake bit (prevents further forcewake requests)
+ *   7) power gate render, media, and display power wells
+ *   8) release gfx clocks
+ * Along with the usual steps of idling the GPU, dealing with display state,
+ * etc.
+ */
+
 static int i915_drm_freeze(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -471,6 +486,7 @@ static int i915_drm_freeze(struct drm_device *dev)
 
 	/* We do a lot of poking in a lot of registers, make sure they work
 	 * properly. */
+	intel_uncore_prepare_suspend(dev);
 	hsw_disable_package_c8(dev_priv);
 	intel_set_power_well(dev, true);
 
@@ -515,6 +531,8 @@ static int i915_drm_freeze(struct drm_device *dev)
 	intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED);
 	console_unlock();
 
+	intel_uncore_suspend(dev);
+
 	return 0;
 }
 
@@ -578,6 +596,17 @@ static void intel_resume_hotplug(struct drm_device *dev)
 	drm_helper_hpd_irq_event(dev);
 }
 
+/*
+ * On VLV/BYT, the resume steps from a Dx state are as follows:
+ *  1) force gfx clocks on
+ *  2) ungate render, media, display power wells
+ *  3) restore gunit regs
+ *  4) set allow wake bit in wake control
+ *  5) take force wake ref on render & media
+ *  6) re-enable RC6
+ *  7) drop force wake ref
+ *  8) release gfx clocks
+ */
 static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -587,6 +616,8 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
 
 	intel_uncore_sanitize(dev);
 
+	intel_uncore_resume(dev);
+
 	if (drm_core_check_feature(dev, DRIVER_MODESET) &&
 	    restore_gtt_mappings) {
 		mutex_lock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4e97840..85cd5be 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -409,6 +409,9 @@ struct intel_uncore_funcs {
 				  enum intel_display_power_domain domain);
 	void (*display_power_put)(struct drm_i915_private *dev_priv,
 				  enum intel_display_power_domain domain);
+	void (*prepare_suspend)(struct drm_i915_private *dev_priv);
+	void (*suspend)(struct drm_i915_private *dev_priv);
+	void (*resume)(struct drm_i915_private *dev_priv);
 };
 
 struct intel_uncore {
@@ -842,6 +845,11 @@ struct i915_suspend_saved_registers {
 	u32 savePIPEB_LINK_N1;
 	u32 saveMCHBAR_RENDER_STANDBY;
 	u32 savePCH_PORT_HOTPLUG;
+	u32 saveGUNIT_Control;
+	u32 saveGUNIT_Control2;
+	u32 saveGUNIT_CZClockGatingDisable1;
+	u32 saveGUNIT_CZClockGatingDisable2;
+	u32 saveDPIO_CFG_DATA;
 };
 
 struct intel_gen6_power_mgmt {
@@ -1824,6 +1832,9 @@ extern void intel_pm_init(struct drm_device *dev);
 extern void intel_hpd_init(struct drm_device *dev);
 extern void intel_pm_init(struct drm_device *dev);
 
+extern void intel_uncore_prepare_suspend(struct drm_device *dev);
+extern void intel_uncore_suspend(struct drm_device *dev);
+extern void intel_uncore_resume(struct drm_device *dev);
 extern void intel_uncore_sanitize(struct drm_device *dev);
 extern void intel_uncore_early_sanitize(struct drm_device *dev);
 extern void intel_uncore_init(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index eb29a23..bd189d6 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4470,8 +4470,9 @@ int i915_gem_init(struct drm_device *dev)
 
 	if (IS_VALLEYVIEW(dev)) {
 		/* VLVA0 (potential hack), BIOS isn't actually waking us */
-		I915_WRITE(VLV_GTLC_WAKE_CTRL, 1);
-		if (wait_for((I915_READ(VLV_GTLC_PW_STATUS) & 1) == 1, 10))
+		I915_WRITE(VLV_GTLC_WAKE_CTRL, VLV_ALLOW_WAKE_REQ);
+		if (wait_for((I915_READ(VLV_GTLC_PW_STATUS) &
+			      VLV_ALLOW_WAKE_REQ), 10))
 			DRM_DEBUG_DRIVER("allow wake ack timed out\n");
 	}
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index b64b1a6..cbf3e48 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -791,8 +791,11 @@
 #define IIR		0x020a4
 #define IMR		0x020a8
 #define ISR		0x020ac
+#define VLV_GUNIT_CONTROL	(VLV_DISPLAY_BASE + 0x2030)
+#define VLV_GUNIT_CONTROL2	(VLV_DISPLAY_BASE + 0x2034)
 #define VLV_GUNIT_CLOCK_GATE	(VLV_DISPLAY_BASE + 0x2060)
 #define   GCFG_DIS		(1<<8)
+#define VLV_GUNIT_CLOCK_GATE2	(VLV_DISPLAY_BASE + 0x2064)
 #define VLV_IIR_RW	(VLV_DISPLAY_BASE + 0x2084)
 #define VLV_IER		(VLV_DISPLAY_BASE + 0x20a0)
 #define VLV_IIR		(VLV_DISPLAY_BASE + 0x20a4)
@@ -4644,7 +4647,12 @@
 #define  FORCEWAKE_ACK_HSW			0x130044
 #define  FORCEWAKE_ACK				0x130090
 #define  VLV_GTLC_WAKE_CTRL			0x130090
+#define   VLV_ALLOW_WAKE_REQ			(1<<0)
 #define  VLV_GTLC_PW_STATUS			0x130094
+#define   VLV_ALLOW_WAKE_ACK			(1<<0)
+#define VLV_GTLC_SURVIVABILITY_REG		0x130098
+#define   VLV_GFX_CLK_STATUS			(1<<3)
+#define   VLV_GFX_CLK_FORCE_ON			(1<<2)
 #define  FORCEWAKE_MT				0xa188 /* multi-threaded */
 #define   FORCEWAKE_KERNEL			0x1
 #define   FORCEWAKE_USER			0x2
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index ef5d7fd..b126f5a 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -419,10 +419,110 @@ static void vlv_set_media_power(struct drm_i915_private *dev_priv, bool enable)
 	__vlv_set_power_well(dev_priv, MEDIA_PWRGT, enable);
 }
 
+static void vlv_set_gfx_clock(struct drm_i915_private *dev_priv, bool enable)
+{
+	u32 reg;
+
+	if (!IS_VALLEYVIEW(dev_priv->dev))
+		return;
+
+	reg = I915_READ(VLV_GTLC_SURVIVABILITY_REG);
+	if (enable)
+		reg |= VLV_GFX_CLK_FORCE_ON;
+	else
+		reg &= ~VLV_GFX_CLK_FORCE_ON;
+	I915_WRITE(VLV_GTLC_SURVIVABILITY_REG, reg);
+	if (wait_for_atomic(((VLV_GFX_CLK_STATUS &
+			      I915_READ(VLV_GTLC_SURVIVABILITY_REG)) != 0),
+			    100)) {
+		dev_err(&dev_priv->dev->pdev->dev,
+			"GFX_CLK_ON timed out, suspend might fail\n");
+	}
+}
+
+static void vlv_gunit_save(struct drm_i915_private *dev_priv)
+{
+	dev_priv->regfile.saveGUNIT_Control = I915_READ(VLV_GUNIT_CONTROL);
+	dev_priv->regfile.saveGUNIT_Control2 = I915_READ(VLV_GUNIT_CONTROL2);
+	dev_priv->regfile.saveGUNIT_CZClockGatingDisable1 =
+		I915_READ(VLV_GUNIT_CLOCK_GATE);
+	dev_priv->regfile.saveGUNIT_CZClockGatingDisable2 =
+		I915_READ(VLV_GUNIT_CLOCK_GATE2);
+	dev_priv->regfile.saveDPIO_CFG_DATA = I915_READ(DPIO_CTL);
+}
+
+static void vlv_gunit_restore(struct drm_i915_private *dev_priv)
+{
+	I915_WRITE(VLV_GUNIT_CONTROL, dev_priv->regfile.saveGUNIT_Control);
+	I915_WRITE(VLV_GUNIT_CONTROL2, dev_priv->regfile.saveGUNIT_Control2);
+	I915_WRITE(VLV_GUNIT_CLOCK_GATE,
+		   dev_priv->regfile.saveGUNIT_CZClockGatingDisable1);
+	I915_WRITE(VLV_GUNIT_CLOCK_GATE2,
+		   dev_priv->regfile.saveGUNIT_CZClockGatingDisable2);
+	I915_WRITE(DPIO_CTL, dev_priv->regfile.saveDPIO_CFG_DATA);
+}
+
+static void vlv_uncore_prepare_suspend(struct drm_i915_private *dev_priv)
+{
+	vlv_set_gfx_clock(dev_priv, true);
+	vlv_gunit_save(dev_priv);
+}
+
+static void vlv_uncore_suspend(struct drm_i915_private *dev_priv)
+{
+	u32 reg;
+
+	mutex_lock(&dev_priv->rps.hw_lock);
+	valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_delay);
+	mutex_unlock(&dev_priv->rps.hw_lock);
+
+	reg = I915_READ(VLV_GTLC_WAKE_CTRL);
+	reg &= ~VLV_ALLOW_WAKE_REQ;
+	I915_WRITE(VLV_GTLC_WAKE_CTRL, reg);
+	if (wait_for_atomic(!(I915_READ(VLV_GTLC_PW_STATUS) &
+			      VLV_ALLOW_WAKE_ACK), 100)) {
+		dev_err(&dev_priv->dev->pdev->dev,
+			"ALLOW_WAKE_SET timed out, suspend might fail\n");
+	}
+
+	intel_set_power_well(dev_priv->dev, false);
+
+	vlv_set_display_power(dev_priv, false);
+	vlv_set_render_power(dev_priv, false);
+	vlv_set_media_power(dev_priv, false);
+
+	vlv_set_gfx_clock(dev_priv, false);
+}
+
+static void vlv_uncore_resume(struct drm_i915_private *dev_priv)
+{
+	u32 reg;
+
+	vlv_gunit_restore(dev_priv);
+
+	reg = I915_READ(VLV_GTLC_WAKE_CTRL);
+	reg |= VLV_ALLOW_WAKE_REQ;
+	I915_WRITE(VLV_GTLC_WAKE_CTRL, reg);
+	if (wait_for_atomic((0 != (I915_READ(VLV_GTLC_PW_STATUS) &
+				   VLV_ALLOW_WAKE_ACK)), 100)) {
+		dev_err(&dev_priv->dev->pdev->dev,
+			"ALLOW_WAKE_SET timed out, resume might fail\n");
+	}
+
+	vlv_set_gfx_clock(dev_priv, false);
+}
+
 void intel_uncore_early_sanitize(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	if (IS_VALLEYVIEW(dev)) {
+		vlv_set_gfx_clock(dev_priv, true);
+		vlv_set_display_power(dev_priv, true);
+		vlv_set_render_power(dev_priv, true);
+		vlv_set_media_power(dev_priv, true);
+	}
+
 	if (HAS_FPGA_DBG_UNCLAIMED(dev))
 		__raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
 }
@@ -441,6 +541,9 @@ void intel_uncore_init(struct drm_device *dev)
 		dev_priv->uncore.funcs.display_power_enabled = vlv_display_power_enabled;
 		dev_priv->uncore.funcs.display_power_get = vlv_display_power_get;
 		dev_priv->uncore.funcs.display_power_put = vlv_display_power_put;
+		dev_priv->uncore.funcs.prepare_suspend = vlv_uncore_prepare_suspend;
+		dev_priv->uncore.funcs.suspend = vlv_uncore_suspend;
+		dev_priv->uncore.funcs.resume = vlv_uncore_resume;
 	} else if (IS_HASWELL(dev)) {
 		dev_priv->uncore.funcs.force_wake_get = __gen6_gt_force_wake_mt_get;
 		dev_priv->uncore.funcs.force_wake_put = __gen6_gt_force_wake_mt_put;
@@ -512,26 +615,32 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev)
 
 void intel_uncore_sanitize(struct drm_device *dev)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 reg_val;
-
 	intel_uncore_forcewake_reset(dev);
 
 	/* BIOS often leaves RC6 enabled, but disable it for hw init */
 	intel_disable_gt_powersave(dev);
+}
 
-	/* Turn off power gate, require especially for the BIOS less system */
-	if (IS_VALLEYVIEW(dev)) {
-
-		mutex_lock(&dev_priv->rps.hw_lock);
-		reg_val = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_STATUS);
 
-		if (reg_val & (RENDER_PWRGT | MEDIA_PWRGT | DISP2D_PWRGT))
-			vlv_punit_write(dev_priv, PUNIT_REG_PWRGT_CTRL, 0x0);
+void intel_uncore_prepare_suspend(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	if (dev_priv->uncore.funcs.prepare_suspend)
+		dev_priv->uncore.funcs.prepare_suspend(dev_priv);
+}
 
-		mutex_unlock(&dev_priv->rps.hw_lock);
+void intel_uncore_suspend(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	if (dev_priv->uncore.funcs.suspend)
+		dev_priv->uncore.funcs.suspend(dev_priv);
+}
 
-	}
+void intel_uncore_resume(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	if (dev_priv->uncore.funcs.resume)
+		dev_priv->uncore.funcs.resume(dev_priv);
 }
 
 /*
-- 
1.8.3.1

[PATCH 4/5] drm/i915: take power well refs when needed

[Jesse Barnes]

When accessing the display regs for hw state readout or cross check, we
need to make sure the power well is enabled so we can read valid
register state.

Likewise, in an actual mode set, we need to take a ref on the
appropriate power well so that the mode set succeeds.  From then on, the
power well ref will be tracked by the CRTC enable/disable code.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_dma.c      |  2 ++
 drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 313a8c9..91c3e6c 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1367,6 +1367,8 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	i915_disable_vga_mem(dev);
 	intel_display_power_put(dev, POWER_DOMAIN_VGA);
 
+	intel_display_power_put(dev, POWER_DOMAIN_PIPE(0));
+
 	/* Only enable hotplug handling once the fbdev is fully set up. */
 	dev_priv->enable_hotplug_processing = true;
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6e4729b..62ee110 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3841,6 +3841,8 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 	if (intel_crtc->active)
 		return;
 
+	intel_display_power_get(dev, POWER_DOMAIN_PIPE(pipe));
+
 	intel_crtc->active = true;
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
@@ -3975,6 +3977,9 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	intel_update_watermarks(crtc);
 
 	intel_update_fbc(dev);
+
+	if (IS_VALLEYVIEW(dev))
+		intel_display_power_put(dev, POWER_DOMAIN_PIPE(pipe));
 }
 
 static void i9xx_crtc_off(struct drm_crtc *crtc)
@@ -4134,6 +4139,11 @@ static void intel_connector_check_state(struct intel_connector *connector)
  * consider. */
 void intel_connector_dpms(struct drm_connector *connector, int mode)
 {
+	struct intel_crtc *intel_crtc = to_intel_crtc(connector->encoder->crtc);
+	enum intel_display_power_domain domain;
+
+	domain = POWER_DOMAIN_PIPE(intel_crtc->pipe);
+
 	/* All the simple cases only support two dpms states. */
 	if (mode != DRM_MODE_DPMS_ON)
 		mode = DRM_MODE_DPMS_OFF;
@@ -4141,6 +4151,7 @@ void intel_connector_dpms(struct drm_connector *connector, int mode)
 	if (mode == connector->dpms)
 		return;
 
+	intel_display_power_get(connector->dev, domain);
 	connector->dpms = mode;
 
 	/* Only need to change hw state when actually enabled */
@@ -4148,6 +4159,7 @@ void intel_connector_dpms(struct drm_connector *connector, int mode)
 		intel_encoder_dpms(to_intel_encoder(connector->encoder), mode);
 
 	intel_modeset_check_state(connector->dev);
+	intel_display_power_put(connector->dev, domain);
 }
 
 /* Simple connector->get_hw_state implementation for encoders that support only
@@ -9192,6 +9204,15 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 	for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc)
 		intel_crtc_disable(&intel_crtc->base);
 
+	/*
+	 * We take a ref here so the mode set will hit live hw.  Once
+	 * we call the CRTC enable, we can drop our ref since it'll get
+	 * tracked there from then on.
+	 */
+	for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc)
+		intel_display_power_get(dev,
+					POWER_DOMAIN_PIPE(intel_crtc->pipe));
+
 	for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) {
 		if (intel_crtc->base.enabled)
 			dev_priv->display.crtc_disable(&intel_crtc->base);
@@ -9247,6 +9268,10 @@ done:
 	}
 
 out:
+	for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc)
+		intel_display_power_put(dev,
+					POWER_DOMAIN_PIPE(intel_crtc->pipe));
+
 	kfree(pipe_config);
 	kfree(saved_mode);
 	return ret;
@@ -10692,6 +10717,11 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 
 		crtc->base.enabled = crtc->active;
 
+		if (crtc->active)
+			intel_display_power_get(dev,
+						POWER_DOMAIN_PIPE(crtc->pipe));
+
+
 		DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
 			      crtc->base.base.id,
 			      crtc->active ? "enabled" : "disabled");
-- 
1.8.3.1

Re: [PATCH 4/5] drm/i915: take power well refs when needed

[Paulo Zanoni]

2013/10/14 Jesse Barnes <jbarnes@virtuousgeek.org>:
> When accessing the display regs for hw state readout or cross check, we
> need to make sure the power well is enabled so we can read valid
> register state.

On the current code (HSW) we already check for the power wells in the
HW state readout code: if the power well is disabled, then transcoders
A/B/C are disabled. The current code takes care to not touch registers
on a disabled power well.

>
> Likewise, in an actual mode set, we need to take a ref on the
> appropriate power well so that the mode set succeeds.  From then on, the
> power well ref will be tracked by the CRTC enable/disable code.
>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/i915_dma.c      |  2 ++
>  drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 32 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 313a8c9..91c3e6c 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1367,6 +1367,8 @@ static int i915_load_modeset_init(struct drm_device *dev)
>         i915_disable_vga_mem(dev);
>         intel_display_power_put(dev, POWER_DOMAIN_VGA);
>
> +       intel_display_power_put(dev, POWER_DOMAIN_PIPE(0));
> +

Why is this here? It certainly deserves a comment in the code.


>         /* Only enable hotplug handling once the fbdev is fully set up. */
>         dev_priv->enable_hotplug_processing = true;
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 6e4729b..62ee110 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3841,6 +3841,8 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>         if (intel_crtc->active)
>                 return;
>
> +       intel_display_power_get(dev, POWER_DOMAIN_PIPE(pipe));
> +
>         intel_crtc->active = true;
>
>         for_each_encoder_on_crtc(dev, crtc, encoder)
> @@ -3975,6 +3977,9 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>         intel_update_watermarks(crtc);
>
>         intel_update_fbc(dev);
> +
> +       if (IS_VALLEYVIEW(dev))
> +               intel_display_power_put(dev, POWER_DOMAIN_PIPE(pipe));

So now HSW uses global_resources while VLV uses crtc enable/disable. I
really think both platforms should try to do the same thing.

By the way, at least on Haswell, if we do an equivalent change we'll
have problems, because when you disable the power well at
crtc_disable, then everything you did at crtc_mode_set will be undone,
and it won't be redone at crtc_enable. When you reenable the power
well, the registers go back to their default values, not the values
that were previously there. Did you check if VLV behaves the same?

>  }
>
>  static void i9xx_crtc_off(struct drm_crtc *crtc)
> @@ -4134,6 +4139,11 @@ static void intel_connector_check_state(struct intel_connector *connector)
>   * consider. */
>  void intel_connector_dpms(struct drm_connector *connector, int mode)
>  {
> +       struct intel_crtc *intel_crtc = to_intel_crtc(connector->encoder->crtc);
> +       enum intel_display_power_domain domain;
> +
> +       domain = POWER_DOMAIN_PIPE(intel_crtc->pipe);
> +
>         /* All the simple cases only support two dpms states. */
>         if (mode != DRM_MODE_DPMS_ON)
>                 mode = DRM_MODE_DPMS_OFF;
> @@ -4141,6 +4151,7 @@ void intel_connector_dpms(struct drm_connector *connector, int mode)
>         if (mode == connector->dpms)
>                 return;
>
> +       intel_display_power_get(connector->dev, domain);
>         connector->dpms = mode;
>
>         /* Only need to change hw state when actually enabled */
> @@ -4148,6 +4159,7 @@ void intel_connector_dpms(struct drm_connector *connector, int mode)
>                 intel_encoder_dpms(to_intel_encoder(connector->encoder), mode);
>
>         intel_modeset_check_state(connector->dev);
> +       intel_display_power_put(connector->dev, domain);
>  }
>
>  /* Simple connector->get_hw_state implementation for encoders that support only
> @@ -9192,6 +9204,15 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>         for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc)
>                 intel_crtc_disable(&intel_crtc->base);
>
> +       /*
> +        * We take a ref here so the mode set will hit live hw.  Once
> +        * we call the CRTC enable, we can drop our ref since it'll get
> +        * tracked there from then on.
> +        */
> +       for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc)
> +               intel_display_power_get(dev,
> +                                       POWER_DOMAIN_PIPE(intel_crtc->pipe));
> +
>         for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) {
>                 if (intel_crtc->base.enabled)
>                         dev_priv->display.crtc_disable(&intel_crtc->base);
> @@ -9247,6 +9268,10 @@ done:
>         }
>
>  out:
> +       for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc)
> +               intel_display_power_put(dev,
> +                                       POWER_DOMAIN_PIPE(intel_crtc->pipe));
> +
>         kfree(pipe_config);
>         kfree(saved_mode);
>         return ret;
> @@ -10692,6 +10717,11 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>
>                 crtc->base.enabled = crtc->active;
>
> +               if (crtc->active)
> +                       intel_display_power_get(dev,
> +                                               POWER_DOMAIN_PIPE(crtc->pipe));
> +

What about the panel fitter power domains? Sometimes the panel fitter
is the thing that makes you require a power well, even though you're
on a pipe that doesn't need it.

And on Haswell you also have to take into account
TRANSCODER_EDP+PIPE_A versus TRANSCODER_A+PIPE_A, where the first
doesn't need the power well but the second needs it.

> +
>                 DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
>                               crtc->base.base.id,
>                               crtc->active ? "enabled" : "disabled");
> --
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 4/5] drm/i915: take power well refs when needed

[Jesse Barnes]

On Tue, 15 Oct 2013 16:54:00 -0300
Paulo Zanoni <przanoni@gmail.com> wrote:

> 2013/10/14 Jesse Barnes <jbarnes@virtuousgeek.org>:
> > When accessing the display regs for hw state readout or cross check, we
> > need to make sure the power well is enabled so we can read valid
> > register state.
> 
> On the current code (HSW) we already check for the power wells in the
> HW state readout code: if the power well is disabled, then transcoders
> A/B/C are disabled. The current code takes care to not touch registers
> on a disabled power well.

Yeah and we should probably preserve that behavior, for the readout
code at least.

> > +       intel_display_power_put(dev, POWER_DOMAIN_PIPE(0));
> > +
> 
> Why is this here? It certainly deserves a comment in the code.

It probably needs to be removed.  The refcounting for the initial load
is still screwy due to the unconditional enable later on.

> So now HSW uses global_resources while VLV uses crtc enable/disable. I
> really think both platforms should try to do the same thing.

Definitely agreed.

> By the way, at least on Haswell, if we do an equivalent change we'll
> have problems, because when you disable the power well at
> crtc_disable, then everything you did at crtc_mode_set will be undone,
> and it won't be redone at crtc_enable. When you reenable the power
> well, the registers go back to their default values, not the values
> that were previously there. Did you check if VLV behaves the same?

No that's taken into account here.  In __intel_set_mode we take a
private ref on the appropriate power well so that we'll preserve state
until we do the first crtc_enable.  From then on, the ref is tracked
there and we drop the private one in __intel_set_mode

> > +               if (crtc->active)
> > +                       intel_display_power_get(dev,
> > +                                               POWER_DOMAIN_PIPE(crtc->pipe));
> > +
> 
> What about the panel fitter power domains? Sometimes the panel fitter
> is the thing that makes you require a power well, even though you're
> on a pipe that doesn't need it.
> 
> And on Haswell you also have to take into account
> TRANSCODER_EDP+PIPE_A versus TRANSCODER_A+PIPE_A, where the first
> doesn't need the power well but the second needs it.

Yeah I'm still not sure how to handle this in generic code.  Maybe the
power well mapping function Imre added will be enough, but it
definitely gets tricky when we look at all the different platforms we
have to (and will have to) handle.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 4/5] drm/i915: take power well refs when needed

[Paulo Zanoni]

2013/10/15 Jesse Barnes <jbarnes@virtuousgeek.org>:
> On Tue, 15 Oct 2013 16:54:00 -0300
> Paulo Zanoni <przanoni@gmail.com> wrote:
>
>> 2013/10/14 Jesse Barnes <jbarnes@virtuousgeek.org>:
>> > When accessing the display regs for hw state readout or cross check, we
>> > need to make sure the power well is enabled so we can read valid
>> > register state.
>>
>> On the current code (HSW) we already check for the power wells in the
>> HW state readout code: if the power well is disabled, then transcoders
>> A/B/C are disabled. The current code takes care to not touch registers
>> on a disabled power well.
>
> Yeah and we should probably preserve that behavior, for the readout
> code at least.
>
>> > +       intel_display_power_put(dev, POWER_DOMAIN_PIPE(0));
>> > +
>>
>> Why is this here? It certainly deserves a comment in the code.
>
> It probably needs to be removed.  The refcounting for the initial load
> is still screwy due to the unconditional enable later on.
>
>> So now HSW uses global_resources while VLV uses crtc enable/disable. I
>> really think both platforms should try to do the same thing.
>
> Definitely agreed.
>
>> By the way, at least on Haswell, if we do an equivalent change we'll
>> have problems, because when you disable the power well at
>> crtc_disable, then everything you did at crtc_mode_set will be undone,
>> and it won't be redone at crtc_enable. When you reenable the power
>> well, the registers go back to their default values, not the values
>> that were previously there. Did you check if VLV behaves the same?
>
> No that's taken into account here.  In __intel_set_mode we take a
> private ref on the appropriate power well so that we'll preserve state
> until we do the first crtc_enable.  From then on, the ref is tracked
> there and we drop the private one in __intel_set_mode

Yeah, but then after all this is done, at some point we'll call
crtc_disable, then we'll put the last ref back and then the power well
will be disabled. Then the user moves the mouse and we call
crtc_enable, so we enable the power well, all its registers to back to
their reset state, then crtc_enable sets some of the registers, but
that won't really be enough since no one called crtc_mode_set again,
and all the state set by crtc_mode_set (not touched by crtc_enable) is
back to the default values. No? What am I missing?

>
>> > +               if (crtc->active)
>> > +                       intel_display_power_get(dev,
>> > +                                               POWER_DOMAIN_PIPE(crtc->pipe));
>> > +
>>
>> What about the panel fitter power domains? Sometimes the panel fitter
>> is the thing that makes you require a power well, even though you're
>> on a pipe that doesn't need it.
>>
>> And on Haswell you also have to take into account
>> TRANSCODER_EDP+PIPE_A versus TRANSCODER_A+PIPE_A, where the first
>> doesn't need the power well but the second needs it.
>
> Yeah I'm still not sure how to handle this in generic code.  Maybe the
> power well mapping function Imre added will be enough, but it
> definitely gets tricky when we look at all the different platforms we
> have to (and will have to) handle.
>
> Thanks,
> --
> Jesse Barnes, Intel Open Source Technology Center



-- 
Paulo Zanoni

Re: [PATCH 4/5] drm/i915: take power well refs when needed

[Jesse Barnes]

On Tue, 15 Oct 2013 17:47:20 -0300
Paulo Zanoni <przanoni@gmail.com> wrote:

> 2013/10/15 Jesse Barnes <jbarnes@virtuousgeek.org>:
> > On Tue, 15 Oct 2013 16:54:00 -0300
> > Paulo Zanoni <przanoni@gmail.com> wrote:
> >
> >> 2013/10/14 Jesse Barnes <jbarnes@virtuousgeek.org>:
> >> > When accessing the display regs for hw state readout or cross check, we
> >> > need to make sure the power well is enabled so we can read valid
> >> > register state.
> >>
> >> On the current code (HSW) we already check for the power wells in the
> >> HW state readout code: if the power well is disabled, then transcoders
> >> A/B/C are disabled. The current code takes care to not touch registers
> >> on a disabled power well.
> >
> > Yeah and we should probably preserve that behavior, for the readout
> > code at least.
> >
> >> > +       intel_display_power_put(dev, POWER_DOMAIN_PIPE(0));
> >> > +
> >>
> >> Why is this here? It certainly deserves a comment in the code.
> >
> > It probably needs to be removed.  The refcounting for the initial load
> > is still screwy due to the unconditional enable later on.
> >
> >> So now HSW uses global_resources while VLV uses crtc enable/disable. I
> >> really think both platforms should try to do the same thing.
> >
> > Definitely agreed.
> >
> >> By the way, at least on Haswell, if we do an equivalent change we'll
> >> have problems, because when you disable the power well at
> >> crtc_disable, then everything you did at crtc_mode_set will be undone,
> >> and it won't be redone at crtc_enable. When you reenable the power
> >> well, the registers go back to their default values, not the values
> >> that were previously there. Did you check if VLV behaves the same?
> >
> > No that's taken into account here.  In __intel_set_mode we take a
> > private ref on the appropriate power well so that we'll preserve state
> > until we do the first crtc_enable.  From then on, the ref is tracked
> > there and we drop the private one in __intel_set_mode
> 
> Yeah, but then after all this is done, at some point we'll call
> crtc_disable, then we'll put the last ref back and then the power well
> will be disabled. Then the user moves the mouse and we call
> crtc_enable, so we enable the power well, all its registers to back to
> their reset state, then crtc_enable sets some of the registers, but
> that won't really be enough since no one called crtc_mode_set again,
> and all the state set by crtc_mode_set (not touched by crtc_enable) is
> back to the default values. No? What am I missing?

That's where the save/restore state of the later patch comes in.  We
need that if we're going to support DPMS and runtime suspend w/o a mode
set.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 4/5] drm/i915: take power well refs when needed

[Paulo Zanoni]

2013/10/15 Jesse Barnes <jbarnes@virtuousgeek.org>:
> On Tue, 15 Oct 2013 17:47:20 -0300
> Paulo Zanoni <przanoni@gmail.com> wrote:
>
>> 2013/10/15 Jesse Barnes <jbarnes@virtuousgeek.org>:
>> > On Tue, 15 Oct 2013 16:54:00 -0300
>> > Paulo Zanoni <przanoni@gmail.com> wrote:
>> >
>> >> 2013/10/14 Jesse Barnes <jbarnes@virtuousgeek.org>:
>> >> > When accessing the display regs for hw state readout or cross check, we
>> >> > need to make sure the power well is enabled so we can read valid
>> >> > register state.
>> >>
>> >> On the current code (HSW) we already check for the power wells in the
>> >> HW state readout code: if the power well is disabled, then transcoders
>> >> A/B/C are disabled. The current code takes care to not touch registers
>> >> on a disabled power well.
>> >
>> > Yeah and we should probably preserve that behavior, for the readout
>> > code at least.
>> >
>> >> > +       intel_display_power_put(dev, POWER_DOMAIN_PIPE(0));
>> >> > +
>> >>
>> >> Why is this here? It certainly deserves a comment in the code.
>> >
>> > It probably needs to be removed.  The refcounting for the initial load
>> > is still screwy due to the unconditional enable later on.
>> >
>> >> So now HSW uses global_resources while VLV uses crtc enable/disable. I
>> >> really think both platforms should try to do the same thing.
>> >
>> > Definitely agreed.
>> >
>> >> By the way, at least on Haswell, if we do an equivalent change we'll
>> >> have problems, because when you disable the power well at
>> >> crtc_disable, then everything you did at crtc_mode_set will be undone,
>> >> and it won't be redone at crtc_enable. When you reenable the power
>> >> well, the registers go back to their default values, not the values
>> >> that were previously there. Did you check if VLV behaves the same?
>> >
>> > No that's taken into account here.  In __intel_set_mode we take a
>> > private ref on the appropriate power well so that we'll preserve state
>> > until we do the first crtc_enable.  From then on, the ref is tracked
>> > there and we drop the private one in __intel_set_mode
>>
>> Yeah, but then after all this is done, at some point we'll call
>> crtc_disable, then we'll put the last ref back and then the power well
>> will be disabled. Then the user moves the mouse and we call
>> crtc_enable, so we enable the power well, all its registers to back to
>> their reset state, then crtc_enable sets some of the registers, but
>> that won't really be enough since no one called crtc_mode_set again,
>> and all the state set by crtc_mode_set (not touched by crtc_enable) is
>> back to the default values. No? What am I missing?
>
> That's where the save/restore state of the later patch comes in.  We
> need that if we're going to support DPMS and runtime suspend w/o a mode
> set.

Oh... I wasn't even thinking about it since it's on a later patch. But
makes sense.

But instead of the save/restore thing, shouldn't we just move all the
code that touches registers from mode_set to crtc_enable? IMHO it's
the shiny solution here. And anything else that we may need to
save/restore should be moved to crtc enable/disable and be saved in
"struct intel_crtc" every time it is touched. I really think that
removing stuff from the save/restore code is the way to go. Also, with
my suggestion, crtc_enable will fully implement the "mode set
sequence" from BSpec, so people like the Audio guys will have an
easier time understanding our code :)

>
> --
> Jesse Barnes, Intel Open Source Technology Center



-- 
Paulo Zanoni

Re: [PATCH 4/5] drm/i915: take power well refs when needed

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 1820 bytes --]

On Tue, 2013-10-15 at 13:40 -0700, Jesse Barnes wrote:
> On Tue, 15 Oct 2013 16:54:00 -0300
> Paulo Zanoni <przanoni@gmail.com> wrote:
> [...]
> No that's taken into account here.  In __intel_set_mode we take a
> private ref on the appropriate power well so that we'll preserve state
> until we do the first crtc_enable.  From then on, the ref is tracked
> there and we drop the private one in __intel_set_mode
> 
> > > +               if (crtc->active)
> > > +                       intel_display_power_get(dev,
> > > +                                               POWER_DOMAIN_PIPE(crtc->pipe));
> > > +
> > 
> > What about the panel fitter power domains? Sometimes the panel fitter
> > is the thing that makes you require a power well, even though you're
> > on a pipe that doesn't need it.
> > 
> > And on Haswell you also have to take into account
> > TRANSCODER_EDP+PIPE_A versus TRANSCODER_A+PIPE_A, where the first
> > doesn't need the power well but the second needs it.
> 
> Yeah I'm still not sure how to handle this in generic code.  Maybe the
> power well mapping function Imre added will be enough, but it
> definitely gets tricky when we look at all the different platforms we
> have to (and will have to) handle.

Isn't the power domain abstraction a neat idea exactly for the above
case? Generic code just asks for the domain it needs and doesn't care
how it maps to power wells on the given platform. So for transcoder_edp
+pipe_a it'd end up asking for POWER_DOMAIN_PIPE_A and
POWER_DOMAIN_TRANSCODER_EDP, both of which is a nop on HSW, and for the
other case POWER_DOMAIN_PIPE_A and POWER_DOMAIN_TRANSCODER_A which would
enable the power well. You also have the POWER_DOMAIN_PIPE,
POWER_DOMAIN_TRANSCODER, POWER_DOMAIN_PIPE_PANEL_FITTER helpers already.

--Imre

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 4/5] drm/i915: take power well refs when needed

[Jesse Barnes]

On Wed, 16 Oct 2013 14:10:13 +0300
Imre Deak <imre.deak@intel.com> wrote:

> On Tue, 2013-10-15 at 13:40 -0700, Jesse Barnes wrote:
> > On Tue, 15 Oct 2013 16:54:00 -0300
> > Paulo Zanoni <przanoni@gmail.com> wrote:
> > [...]
> > No that's taken into account here.  In __intel_set_mode we take a
> > private ref on the appropriate power well so that we'll preserve state
> > until we do the first crtc_enable.  From then on, the ref is tracked
> > there and we drop the private one in __intel_set_mode
> > 
> > > > +               if (crtc->active)
> > > > +                       intel_display_power_get(dev,
> > > > +                                               POWER_DOMAIN_PIPE(crtc->pipe));
> > > > +
> > > 
> > > What about the panel fitter power domains? Sometimes the panel fitter
> > > is the thing that makes you require a power well, even though you're
> > > on a pipe that doesn't need it.
> > > 
> > > And on Haswell you also have to take into account
> > > TRANSCODER_EDP+PIPE_A versus TRANSCODER_A+PIPE_A, where the first
> > > doesn't need the power well but the second needs it.
> > 
> > Yeah I'm still not sure how to handle this in generic code.  Maybe the
> > power well mapping function Imre added will be enough, but it
> > definitely gets tricky when we look at all the different platforms we
> > have to (and will have to) handle.
> 
> Isn't the power domain abstraction a neat idea exactly for the above
> case? Generic code just asks for the domain it needs and doesn't care
> how it maps to power wells on the given platform. So for transcoder_edp
> +pipe_a it'd end up asking for POWER_DOMAIN_PIPE_A and
> POWER_DOMAIN_TRANSCODER_EDP, both of which is a nop on HSW, and for the
> other case POWER_DOMAIN_PIPE_A and POWER_DOMAIN_TRANSCODER_A which would
> enable the power well. You also have the POWER_DOMAIN_PIPE,
> POWER_DOMAIN_TRANSCODER, POWER_DOMAIN_PIPE_PANEL_FITTER helpers already.

Yeah I think it can work.  I missed your function that takes a crtc
though as well, so we don't end up polluting the generic functions with
TRANSCODER references that don't exist on the Atom platforms for
example.  That's the main thing I'm worried about, since as we get more
and more wells I think it'll get easier to get it wrong in the generic
code, if we have to use all the required domains for all platforms
there.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 4/5] drm/i915: take power well refs when needed

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 3038 bytes --]

On Wed, 2013-10-16 at 08:08 -0700, Jesse Barnes wrote:
> On Wed, 16 Oct 2013 14:10:13 +0300
> Imre Deak <imre.deak@intel.com> wrote:
> 
> > On Tue, 2013-10-15 at 13:40 -0700, Jesse Barnes wrote:
> > > On Tue, 15 Oct 2013 16:54:00 -0300
> > > Paulo Zanoni <przanoni@gmail.com> wrote:
> > > [...]
> > > No that's taken into account here.  In __intel_set_mode we take a
> > > private ref on the appropriate power well so that we'll preserve state
> > > until we do the first crtc_enable.  From then on, the ref is tracked
> > > there and we drop the private one in __intel_set_mode
> > > 
> > > > > +               if (crtc->active)
> > > > > +                       intel_display_power_get(dev,
> > > > > +                                               POWER_DOMAIN_PIPE(crtc->pipe));
> > > > > +
> > > > 
> > > > What about the panel fitter power domains? Sometimes the panel fitter
> > > > is the thing that makes you require a power well, even though you're
> > > > on a pipe that doesn't need it.
> > > > 
> > > > And on Haswell you also have to take into account
> > > > TRANSCODER_EDP+PIPE_A versus TRANSCODER_A+PIPE_A, where the first
> > > > doesn't need the power well but the second needs it.
> > > 
> > > Yeah I'm still not sure how to handle this in generic code.  Maybe the
> > > power well mapping function Imre added will be enough, but it
> > > definitely gets tricky when we look at all the different platforms we
> > > have to (and will have to) handle.
> > 
> > Isn't the power domain abstraction a neat idea exactly for the above
> > case? Generic code just asks for the domain it needs and doesn't care
> > how it maps to power wells on the given platform. So for transcoder_edp
> > +pipe_a it'd end up asking for POWER_DOMAIN_PIPE_A and
> > POWER_DOMAIN_TRANSCODER_EDP, both of which is a nop on HSW, and for the
> > other case POWER_DOMAIN_PIPE_A and POWER_DOMAIN_TRANSCODER_A which would
> > enable the power well. You also have the POWER_DOMAIN_PIPE,
> > POWER_DOMAIN_TRANSCODER, POWER_DOMAIN_PIPE_PANEL_FITTER helpers already.
> 
> Yeah I think it can work.  I missed your function that takes a crtc
> though as well, so we don't end up polluting the generic functions with
> TRANSCODER references that don't exist on the Atom platforms for
> example.  That's the main thing I'm worried about, since as we get more
> and more wells I think it'll get easier to get it wrong in the generic
> code, if we have to use all the required domains for all platforms
> there.

Afaics, on VLV for example we'd ask for pipe A/B and transcoder A/B
power domains, which is still correct. It's true that there the
pipe-transcoder connection is fixed, and so we'll always ask for the
same pipe/transcoder power domain pair, but I think it's still ok
conceptually.

So atm the power domains as defined are platform independent, which is
great. If we can't avoid adding a platform specific ones in the future,
we could still handle those in platform specific code.

--Imre

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH 5/5] drm/i915/vlv: support save/restore of display state around power well toggle

[Jesse Barnes]

If we disable the power well at runtime, we need to save enough display
state so we can restore it when the power well comes back again.  Add
support for that on VLV by reusing some of the _freeze and _thaw code.

Note we need to drop the power well lock in this path around the restore,
since we'll end up in mode set functions that take more refs on the
power well.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_uncore.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index b126f5a..070ff00 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -406,7 +406,26 @@ static void __vlv_set_power_well(struct drm_i915_private *dev_priv,
 static void vlv_set_display_power(struct drm_i915_private *dev_priv,
 				  bool enable)
 {
-	__vlv_set_power_well(dev_priv, DISP2D_PWRGT, enable);
+	struct drm_device *dev = dev_priv->dev;
+	struct i915_power_well *power_well = &dev_priv->power_well;
+
+	if (enable) {
+		/* Lost all the display state, restore it */
+		if (vlv_display_power_enabled(dev_priv))
+			return; /* already on, skip the fireworks */
+		__vlv_set_power_well(dev_priv, DISP2D_PWRGT, true);
+		spin_unlock_irq(&power_well->lock);
+		i915_restore_state(dev);
+		intel_modeset_init_hw(dev);
+		intel_modeset_setup_hw_state(dev, true);
+		spin_lock_irq(&power_well->lock);
+	} else {
+		if (!vlv_display_power_enabled(dev_priv))
+			return; /* already off, skip the fireworks */
+		/* Make sure we save the state we need */
+		i915_save_state(dev);
+		__vlv_set_power_well(dev_priv, DISP2D_PWRGT, enable);
+	}
 }
 
 static void vlv_set_render_power(struct drm_i915_private *dev_priv, bool enable)
-- 
1.8.3.1

Re: [PATCH 5/5] drm/i915/vlv: support save/restore of display state around power well toggle

[Paulo Zanoni]

2013/10/14 Jesse Barnes <jbarnes@virtuousgeek.org>:
> If we disable the power well at runtime, we need to save enough display
> state so we can restore it when the power well comes back again.  Add
> support for that on VLV by reusing some of the _freeze and _thaw code.
>
> Note we need to drop the power well lock in this path around the restore,
> since we'll end up in mode set functions that take more refs on the
> power well.
>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index b126f5a..070ff00 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -406,7 +406,26 @@ static void __vlv_set_power_well(struct drm_i915_private *dev_priv,
>  static void vlv_set_display_power(struct drm_i915_private *dev_priv,
>                                   bool enable)
>  {
> -       __vlv_set_power_well(dev_priv, DISP2D_PWRGT, enable);
> +       struct drm_device *dev = dev_priv->dev;
> +       struct i915_power_well *power_well = &dev_priv->power_well;
> +
> +       if (enable) {
> +               /* Lost all the display state, restore it */
> +               if (vlv_display_power_enabled(dev_priv))
> +                       return; /* already on, skip the fireworks */
> +               __vlv_set_power_well(dev_priv, DISP2D_PWRGT, true);
> +               spin_unlock_irq(&power_well->lock);
> +               i915_restore_state(dev);

At least on Haswell, i915_restore_state restores a lot more than just
what was lost on the power well. Do you really need all this? Besides,
I kinda fear you can break things by restoring stuff that is already
running. While we're doing this restoring, interrputs are happening,
everything is running, yet we call stuff like intel_i2c_reset,
i915_gem_restore_fences and others. Another example: we have code to
restore the backlight registers, but backlight is usually on the
output that works even with the power well disabled. So if we disable
the power well, then change backlight on the only output that is
working, then reenable the power well we'll "restore" a bogus
backlight state on a case where we shouldn't be touching backlight at
all.

> +               intel_modeset_init_hw(dev);
> +               intel_modeset_setup_hw_state(dev, true);
> +               spin_lock_irq(&power_well->lock);
> +       } else {
> +               if (!vlv_display_power_enabled(dev_priv))
> +                       return; /* already off, skip the fireworks */
> +               /* Make sure we save the state we need */
> +               i915_save_state(dev);
> +               __vlv_set_power_well(dev_priv, DISP2D_PWRGT, enable);
> +       }
>  }
>
>  static void vlv_set_render_power(struct drm_i915_private *dev_priv, bool enable)
> --
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 5/5] drm/i915/vlv: support save/restore of display state around power well toggle

[Jesse Barnes]

On Tue, 15 Oct 2013 17:09:19 -0300
Paulo Zanoni <przanoni@gmail.com> wrote:

> 2013/10/14 Jesse Barnes <jbarnes@virtuousgeek.org>:
> > If we disable the power well at runtime, we need to save enough display
> > state so we can restore it when the power well comes back again.  Add
> > support for that on VLV by reusing some of the _freeze and _thaw code.
> >
> > Note we need to drop the power well lock in this path around the restore,
> > since we'll end up in mode set functions that take more refs on the
> > power well.
> >
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/intel_uncore.c | 21 ++++++++++++++++++++-
> >  1 file changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > index b126f5a..070ff00 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -406,7 +406,26 @@ static void __vlv_set_power_well(struct drm_i915_private *dev_priv,
> >  static void vlv_set_display_power(struct drm_i915_private *dev_priv,
> >                                   bool enable)
> >  {
> > -       __vlv_set_power_well(dev_priv, DISP2D_PWRGT, enable);
> > +       struct drm_device *dev = dev_priv->dev;
> > +       struct i915_power_well *power_well = &dev_priv->power_well;
> > +
> > +       if (enable) {
> > +               /* Lost all the display state, restore it */
> > +               if (vlv_display_power_enabled(dev_priv))
> > +                       return; /* already on, skip the fireworks */
> > +               __vlv_set_power_well(dev_priv, DISP2D_PWRGT, true);
> > +               spin_unlock_irq(&power_well->lock);
> > +               i915_restore_state(dev);
> 
> At least on Haswell, i915_restore_state restores a lot more than just
> what was lost on the power well. Do you really need all this? Besides,
> I kinda fear you can break things by restoring stuff that is already
> running. While we're doing this restoring, interrputs are happening,
> everything is running, yet we call stuff like intel_i2c_reset,
> i915_gem_restore_fences and others. Another example: we have code to
> restore the backlight registers, but backlight is usually on the
> output that works even with the power well disabled. So if we disable
> the power well, then change backlight on the only output that is
> working, then reenable the power well we'll "restore" a bogus
> backlight state on a case where we shouldn't be touching backlight at
> all.

Yeah this is definitely a WIP.  We'll probably need power well specific
save/restore functions.  Depending on which well was toggled, the state
that needs to be saved & restored will be different.  It may be best to
try to push all this into modeset_setup_hw_state, since it should know
which power wells are needed for different ops and thus restore the
right state.  But that means ripping apart save/restore_state and
putting bits in the right places, potentially with specific hooks like
the uncore save/restore I added for other stuff.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 5/5] drm/i915/vlv: support save/restore of display state around power well toggle

[Daniel Vetter]

On Tue, Oct 15, 2013 at 01:42:53PM -0700, Jesse Barnes wrote:
> On Tue, 15 Oct 2013 17:09:19 -0300
> Paulo Zanoni <przanoni@gmail.com> wrote:
> 
> > 2013/10/14 Jesse Barnes <jbarnes@virtuousgeek.org>:
> > > If we disable the power well at runtime, we need to save enough display
> > > state so we can restore it when the power well comes back again.  Add
> > > support for that on VLV by reusing some of the _freeze and _thaw code.
> > >
> > > Note we need to drop the power well lock in this path around the restore,
> > > since we'll end up in mode set functions that take more refs on the
> > > power well.
> > >
> > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > ---
> > >  drivers/gpu/drm/i915/intel_uncore.c | 21 ++++++++++++++++++++-
> > >  1 file changed, 20 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > > index b126f5a..070ff00 100644
> > > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > > @@ -406,7 +406,26 @@ static void __vlv_set_power_well(struct drm_i915_private *dev_priv,
> > >  static void vlv_set_display_power(struct drm_i915_private *dev_priv,
> > >                                   bool enable)
> > >  {
> > > -       __vlv_set_power_well(dev_priv, DISP2D_PWRGT, enable);
> > > +       struct drm_device *dev = dev_priv->dev;
> > > +       struct i915_power_well *power_well = &dev_priv->power_well;
> > > +
> > > +       if (enable) {
> > > +               /* Lost all the display state, restore it */
> > > +               if (vlv_display_power_enabled(dev_priv))
> > > +                       return; /* already on, skip the fireworks */
> > > +               __vlv_set_power_well(dev_priv, DISP2D_PWRGT, true);
> > > +               spin_unlock_irq(&power_well->lock);
> > > +               i915_restore_state(dev);
> > 
> > At least on Haswell, i915_restore_state restores a lot more than just
> > what was lost on the power well. Do you really need all this? Besides,
> > I kinda fear you can break things by restoring stuff that is already
> > running. While we're doing this restoring, interrputs are happening,
> > everything is running, yet we call stuff like intel_i2c_reset,
> > i915_gem_restore_fences and others. Another example: we have code to
> > restore the backlight registers, but backlight is usually on the
> > output that works even with the power well disabled. So if we disable
> > the power well, then change backlight on the only output that is
> > working, then reenable the power well we'll "restore" a bogus
> > backlight state on a case where we shouldn't be touching backlight at
> > all.
> 
> Yeah this is definitely a WIP.  We'll probably need power well specific
> save/restore functions.  Depending on which well was toggled, the state
> that needs to be saved & restored will be different.  It may be best to
> try to push all this into modeset_setup_hw_state, since it should know
> which power wells are needed for different ops and thus restore the
> right state.  But that means ripping apart save/restore_state and
> putting bits in the right places, potentially with specific hooks like
> the uncore save/restore I added for other stuff.

setup_hw_state only deals with things which should be active (and restores
them when they're not active). But what we want here is to re-run the
->mode_set callback code when enabling a pipe again through dpms. So for
me it feels much saner to just add that callback there instead of
magically restoring register state in the get/put functions.

The reason for that is that at least long-term I expect the ->mode_set
callbacks to die and that we'll move everything which is in there either
into ->enable callbacks or into the compute config stage. With that we'll
have the guarantee that the hw touching code sequence is always the same,
no matter which exact upper level function caused a given state change
(modeset, dpms, resume, ...).

I don't really have an opinion about how we should handle this stuff on
the gem side of things. For that magically restoring the gem hw state in
the get/put functions sounds like the right approach. For gem we probably
also need some delayed actual put to have a bit of hysteresis. That's
kinda why I think we should have separate interfaces for display power
well management and gem/irq/aux power well management.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC] Runtime display PM for VLV/BYT

[Ville Syrjälä]

On Mon, Oct 14, 2013 at 04:07:44PM -0700, Jesse Barnes wrote:
> This set adds bits needed for runtime power support, currently only
> lightly tested on VLV/BYT:
>   1) suspend/resume callbacks for different platforms
>   2) save/restore of display state across a power well toggle
>   3) get/put of display power well in critical places
> 
> The TODO list still has a few items on it, and I'm looking for feedback:
>   1) sprinkle around some power well WARNs so we can catch things easily
>   2) add some tests using DPMS and NULL mode sets and comparing power
>      well state
>   3) better debugfs support for multiple wells
>   4) refcount of power well in debugfs (with ref holders?)
>   5) more testing - I think the load time ref is still busted here and
>      on HSW
>   6) convert HSW as well so DPMS will shut things down, not just mode
>      sets
> 
> Thoughts or comments?

I'd also like to see what Imre cooked up, and then come up with some
grand unified design. Based on our discussions I think his power well
abstraction sounded somewhat nicer and more general.

Also your locking seems to be fubar in places (frobbing with sideband
while holding a spinlock). I think Imre converted the power wells to
use a mutex everywhere.

Or perhaps we just start with your stuff and Imre rebases his stuff on
top?

-- 
Ville Syrjälä
Intel OTC

Re: [RFC] Runtime display PM for VLV/BYT

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 2030 bytes --]

On Tue, 2013-10-15 at 11:06 +0300, Ville Syrjälä wrote:
> On Mon, Oct 14, 2013 at 04:07:44PM -0700, Jesse Barnes wrote:
> > This set adds bits needed for runtime power support, currently only
> > lightly tested on VLV/BYT:
> >   1) suspend/resume callbacks for different platforms
> >   2) save/restore of display state across a power well toggle
> >   3) get/put of display power well in critical places
> > 
> > The TODO list still has a few items on it, and I'm looking for feedback:
> >   1) sprinkle around some power well WARNs so we can catch things easily
> >   2) add some tests using DPMS and NULL mode sets and comparing power
> >      well state
> >   3) better debugfs support for multiple wells
> >   4) refcount of power well in debugfs (with ref holders?)
> >   5) more testing - I think the load time ref is still busted here and
> >      on HSW
> >   6) convert HSW as well so DPMS will shut things down, not just mode
> >      sets
> > 
> > Thoughts or comments?
> 
> I'd also like to see what Imre cooked up, and then come up with some
> grand unified design. Based on our discussions I think his power well
> abstraction sounded somewhat nicer and more general.

I've pushed what I have so far to:
https://github.com/ideak/linux/commits/powerwells

I've tested this on VLV with VGA output so far and somewhat on HSW. I'd
still have to check the need to do any HW state save/restore and the GFX
clock forcing, afaics Jesse has already code for these in his patchset.

> Also your locking seems to be fubar in places (frobbing with sideband
> while holding a spinlock). I think Imre converted the power wells to
> use a mutex everywhere.

Yea, I solved that by changing power_well->lock to be a mutex.

> Or perhaps we just start with your stuff and Imre rebases his stuff on
> top?

That works for me too. In any case would be nice to get some feedback
especially from Paulo as my changes are mostly about the current power
domain / well handling parts.

--Imre

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [RFC] Runtime display PM for VLV/BYT

[Jesse Barnes]

On Tue, 15 Oct 2013 15:16:11 +0300
Imre Deak <imre.deak@intel.com> wrote:

> On Tue, 2013-10-15 at 11:06 +0300, Ville Syrjälä wrote:
> > On Mon, Oct 14, 2013 at 04:07:44PM -0700, Jesse Barnes wrote:
> > > This set adds bits needed for runtime power support, currently only
> > > lightly tested on VLV/BYT:
> > >   1) suspend/resume callbacks for different platforms
> > >   2) save/restore of display state across a power well toggle
> > >   3) get/put of display power well in critical places
> > > 
> > > The TODO list still has a few items on it, and I'm looking for feedback:
> > >   1) sprinkle around some power well WARNs so we can catch things easily
> > >   2) add some tests using DPMS and NULL mode sets and comparing power
> > >      well state
> > >   3) better debugfs support for multiple wells
> > >   4) refcount of power well in debugfs (with ref holders?)
> > >   5) more testing - I think the load time ref is still busted here and
> > >      on HSW
> > >   6) convert HSW as well so DPMS will shut things down, not just mode
> > >      sets
> > > 
> > > Thoughts or comments?
> > 
> > I'd also like to see what Imre cooked up, and then come up with some
> > grand unified design. Based on our discussions I think his power well
> > abstraction sounded somewhat nicer and more general.
> 
> I've pushed what I have so far to:
> https://github.com/ideak/linux/commits/powerwells
> 
> I've tested this on VLV with VGA output so far and somewhat on HSW. I'd
> still have to check the need to do any HW state save/restore and the GFX
> clock forcing, afaics Jesse has already code for these in his patchset.

I think a few can be pushed upstream now:

drm/i915: change power_well->lock to be mutex
drm/i915: factor out is_always_on_domain 
drm/i915: factor out reset_vblank_counter
drm/i915: fix HAS_POWER_WELL->IS_HASWELL - pushed this but it doesn't
  look like Daniel has picked it up yet
drm/i915: check powerwell in get_pipe_config

drm/i915: enable only the needed power domains during modeset
Looks good too, and I guess there's no harm in pushing the earlier
patch to move the get of the power wells out of the HSW global res
function, even if we do move to get/put later.

For drm/i915: support for multiple power wells, I think I prefer a
single uncore function taking a power well, rather than an array of
power_well structs.  If we went that direction, we could probably
rename the power_well variable to audio_power_wells or something and
just track the ones we need to expose to the audio driver there, rather
than everything.  That might be a little clearer than what we have
today, but I guess I'd have to see it coded to be sure.

For drm/i915: add intel_encoder_get_hw_state, do you think it would be
clearer to take refs in the caller of the encoder->get_hw_state instead?

I like drm/i915: get port power domains for connector detect and 
drm/i915: add output power domains too, we'll need those if we want to
do fine grained output control on BYT and future chips.  But they'll
probably need to be rebased on top of the above once it goes upstream.

Overall I don't think there's a ton of conflict between our patchsets,
they seem mostly complimentary and let me remove hacks in mine.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC] Runtime display PM for VLV/BYT

[Imre Deak]

On Tue, 2013-10-15 at 09:23 -0700, Jesse Barnes wrote:
> On Tue, 15 Oct 2013 15:16:11 +0300
> Imre Deak <imre.deak@intel.com> wrote:
> 
> > On Tue, 2013-10-15 at 11:06 +0300, Ville Syrjälä wrote:
> > > On Mon, Oct 14, 2013 at 04:07:44PM -0700, Jesse Barnes wrote:
> > > > This set adds bits needed for runtime power support, currently only
> > > > lightly tested on VLV/BYT:
> > > >   1) suspend/resume callbacks for different platforms
> > > >   2) save/restore of display state across a power well toggle
> > > >   3) get/put of display power well in critical places
> > > > 
> > > > The TODO list still has a few items on it, and I'm looking for feedback:
> > > >   1) sprinkle around some power well WARNs so we can catch things easily
> > > >   2) add some tests using DPMS and NULL mode sets and comparing power
> > > >      well state
> > > >   3) better debugfs support for multiple wells
> > > >   4) refcount of power well in debugfs (with ref holders?)
> > > >   5) more testing - I think the load time ref is still busted here and
> > > >      on HSW
> > > >   6) convert HSW as well so DPMS will shut things down, not just mode
> > > >      sets
> > > > 
> > > > Thoughts or comments?
> > > 
> > > I'd also like to see what Imre cooked up, and then come up with some
> > > grand unified design. Based on our discussions I think his power well
> > > abstraction sounded somewhat nicer and more general.
> > 
> > I've pushed what I have so far to:
> > https://github.com/ideak/linux/commits/powerwells
> > 
> > I've tested this on VLV with VGA output so far and somewhat on HSW. I'd
> > still have to check the need to do any HW state save/restore and the GFX
> > clock forcing, afaics Jesse has already code for these in his patchset.
> 
> I think a few can be pushed upstream now:
> 
> drm/i915: change power_well->lock to be mutex
> drm/i915: factor out is_always_on_domain 
> drm/i915: factor out reset_vblank_counter
> drm/i915: fix HAS_POWER_WELL->IS_HASWELL - pushed this but it doesn't
>   look like Daniel has picked it up yet
> drm/i915: check powerwell in get_pipe_config

Ok, I can send the missing ones to the list.

> drm/i915: enable only the needed power domains during modeset
> Looks good too, and I guess there's no harm in pushing the earlier
> patch to move the get of the power wells out of the HSW global res
> function, even if we do move to get/put later.

Agreed, with the above we'd preserve the current behavior and later
things can be fine grained.

> For drm/i915: support for multiple power wells, I think I prefer a
> single uncore function taking a power well, rather than an array of
> power_well structs.  If we went that direction, we could probably
> rename the power_well variable to audio_power_wells or something and
> just track the ones we need to expose to the audio driver there, rather
> than everything.  That might be a little clearer than what we have
> today, but I guess I'd have to see it coded to be sure.

I added the array mainly so that we can ref count each power well
separately. Currently this isn't a problem as we only need to refcount a
single power well, but on future HW with more of those we need to keep
the ref counters somewhere. I also store the mask of domains for which
we need the given power well there and having a vtable for Gen dependent
HW accessors looked like a nice way.

I agree the interface for requesting power wells for audio could be
better. We could add a new POWER_DOMAIN_AUDIO for that and map it to the
required power wells. But yea, this could be done later.

> For drm/i915: add intel_encoder_get_hw_state, do you think it would be
> clearer to take refs in the caller of the encoder->get_hw_state instead?

What would be the point, if all the callers would take a ref anyway? Or
do you want to take the reference once and do additional things besides
get_hw_state()? 

Related to this: I made intel_encoder_get_hw_state() only check if the
power well is on and return false if it's not to indicate that the
encoder is off. I also thought of doing the same as you and take a ref
instead, not sure what's the right way. Maybe doing the readout only if
the power is on, but also making sure we have a reference in this case?
So with a new helper we'd have in intel_encoder_get_hw_state():

{
  if (!intel_display_power_get_if_enabled(...))
         return false;
  ... do the readout
  intel_display_power_put(...)
}

> I like drm/i915: get port power domains for connector detect and 
> drm/i915: add output power domains too, we'll need those if we want to
> do fine grained output control on BYT and future chips.  But they'll
> probably need to be rebased on top of the above once it goes upstream.

Yea, we can reconsider them once we agree about the above.

> Overall I don't think there's a ton of conflict between our patchsets,
> they seem mostly complimentary and let me remove hacks in mine.

Ok, thanks a lot for the review,
Imre


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

Re: [RFC] Runtime display PM for VLV/BYT

[Daniel Vetter]

On Tue, Oct 15, 2013 at 8:15 PM, Imre Deak <imre.deak@intel.com> wrote:
> Related to this: I made intel_encoder_get_hw_state() only check if the
> power well is on and return false if it's not to indicate that the
> encoder is off. I also thought of doing the same as you and take a ref
> instead, not sure what's the right way. Maybe doing the readout only if
> the power is on, but also making sure we have a reference in this case?
> So with a new helper we'd have in intel_encoder_get_hw_state():

I think the approach we've quickly discussed in today's call is
probably simplest: We grab a temporary reference to all the display
power wells around all the dpms/modeset functions and ignore any power
well checks on top of that. The hw will (well, should) be in the power
on default state, so nothing should magically turn on if we don't want
that.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC] Runtime display PM for VLV/BYT

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 1208 bytes --]

On Wed, 2013-10-16 at 00:09 +0200, Daniel Vetter wrote:
> On Tue, Oct 15, 2013 at 8:15 PM, Imre Deak <imre.deak@intel.com> wrote:
> > Related to this: I made intel_encoder_get_hw_state() only check if the
> > power well is on and return false if it's not to indicate that the
> > encoder is off. I also thought of doing the same as you and take a ref
> > instead, not sure what's the right way. Maybe doing the readout only if
> > the power is on, but also making sure we have a reference in this case?
> > So with a new helper we'd have in intel_encoder_get_hw_state():
> 
> I think the approach we've quickly discussed in today's call is
> probably simplest: We grab a temporary reference to all the display
> power wells around all the dpms/modeset functions and ignore any power
> well checks on top of that. The hw will (well, should) be in the power
> on default state, so nothing should magically turn on if we don't want
> that.

Ok, I'm fine with this too for now, later it can be improved if needed. 
I added a new POWER_DOMAIN_INIT for keeping all power wells on through
driver init->first modeset and suspend->first modeset, that could be
used for this purpose too.

--Imre

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [RFC] Runtime display PM for VLV/BYT

[Daniel Vetter]

On Mon, Oct 14, 2013 at 04:07:44PM -0700, Jesse Barnes wrote:
>   5) more testing - I think the load time ref is still busted here and
>      on HSW

I've chatted quite a bit yesterday with Paulo about how we can test
runtime pm better, since he wanted to get started with testing D3 while
vpg is cooking up the kernel support for this. For general tests that
stuff works I've suggested to pimp the existing pc8.c testcase and
duplicate all the subtests we have there over all the fancy runtime pm
features we'll get. So

for (pm_method in pc8, vlv_power_well, D3, ..)
	for (subtest)
		do_subtest(pm_method)

The pm_method would encapsulate stuff like getting in/out of that state
and checking that we indeed have residency. All the functional tests could
then be shared (i2c, gpu workloads, ...). Even more important once we
stumble over bugs and need new test scenarios.

The other thing is static register setup. Atm we set up registers after
boot, on resume and (in parts at least) after gpu reset, and we already
have bug reports to prove that this is too complicated for us to get
right. Adding D3 and tons other things will make it even more fun.

The problem isn't really the static set of w/as in the ->init_clock_gating
vfuncs, but all the bits and pieces splattered all over the driver:
- w/a which are someplace else due to ordering constraints (e.g. the ring
  specific w/a can't be done in init_clock_gating since the ring enabling
  will clear them again)
- static register setup which is someplace else for better code structure
  (e.g. the ddi buffer translation table setup)

Paulo's idea is to convert the w/a list in init_clock_gating into a table
and also add all the other registers in there but marked with a special
bit saying that those workarounds/settings shouldn't be applied in in the
init_clock_gating code. Then we could scan this table at the usual places
and check the hw state (e.g. after each modeset with all the other modeset
state). The upshot compared to doing this in userspace (Eric started such
a tool in i-g-t/tools/intel_reg_checker) is that we won't have a
synchronization problem between two codebases.

Imo the more dynamic state is already sufficiently locked down with all
our asserts and state cross-checks plus the functional checks from pc8.c

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch