[PATCH 0/6] SNB/BDW runtime PM

[PATCH 0/6] SNB/BDW runtime PM

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Hi

This series depends on:
 - [PATCH 0/6] More runtime PM fixes
 - [PATCH 00/16] Merge PC8 with runtime PM, v3
 - [PATCH 00/20] ILK+ interrupt improvements, v2

Patches 1 and 2 are the last pieces of my runtime PM rework, and patch 1 needs
all the previous patches to be merged before it can be merged.

Patch 3 adds runtime PM support to SNB. It's not widely tested and I haven't
tested it for a while, but at some point this was all that was needed to make my
SNB laptop work.

Patches 4-6 are the BDW bits, but I also haven't tested them since the last
rebase, and there's the possibility that something is still missing. I wouldn't
recommend applying this to production trees yet, but I would recommend you to
apply, test the patches and report the results to me!

More importantly, the basic reason why I sent this series to the list is that if
someone wants to add runtime PM support to some other platform (BYT anyone?),
they should really try to base their patches on top of these. And they should
really look at the SNB patch to have some idea on what they need to do first.

Q: Why don't we have IVB runtime PM support?
A: Just because it was easier for me to do development and test on a SNB
machine. I really expect that IVB runtime PM will just reuse the SNB bits.
The only thing missing for IVB is probably just a few "IS_IVYBRIDGE()" calls,
and a call to modeset_update_crtc_power_domains(). I can provide a patch for
this later.

Thanks,
Paulo

Paulo Zanoni (6):
  drm/i915: kill dev_priv->pm.regsave
  drm/i915: add gen-specific runtime suspend/resume functions
  drm/i915: add SNB runtime PM support
  drm/i915: remove HAS_PC8 check
  drm/i915: BDW needs D_COMP writes through MCHBAR
  drm/i915: add BDW runtime PM support

 drivers/gpu/drm/i915/i915_drv.c      | 53 ++++++++++++++++++++----
 drivers/gpu/drm/i915/i915_drv.h      | 16 ++------
 drivers/gpu/drm/i915/i915_irq.c      | 79 +++++-------------------------------
 drivers/gpu/drm/i915/intel_display.c | 43 ++++++++++++--------
 drivers/gpu/drm/i915/intel_drv.h     |  4 +-
 5 files changed, 87 insertions(+), 108 deletions(-)

-- 
1.8.5.3

[PATCH 1/6] drm/i915: kill dev_priv->pm.regsave

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Now that we don't keep the hotplug interrupts enabled anymore, we can
kill the regsave struct and just cal the normal IRQ preinstall,
postinstall and uninstall functions. This makes it easier to add
runtime PM support to non-HSW platforms.

The only downside is in case we get a request to update interrupts
while they are disabled, won't be able to update the regsave struct.
But this should never happen anyway, so we're not losing too much.

v2: - Rebase.
v3: - Rebase.
v4: - Rebase.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      | 12 +-----
 drivers/gpu/drm/i915/i915_irq.c      | 79 +++++-------------------------------
 drivers/gpu/drm/i915/intel_display.c |  4 +-
 drivers/gpu/drm/i915/intel_drv.h     |  4 +-
 4 files changed, 15 insertions(+), 84 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 70feb61..493579d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1351,23 +1351,13 @@ struct ilk_wm_values {
  * goes back to false exactly before we reenable the IRQs. We use this variable
  * to check if someone is trying to enable/disable IRQs while they're supposed
  * to be disabled. This shouldn't happen and we'll print some error messages in
- * case it happens, but if it actually happens we'll also update the variables
- * inside struct regsave so when we restore the IRQs they will contain the
- * latest expected values.
+ * case it happens.
  *
  * For more, read the Documentation/power/runtime_pm.txt.
  */
 struct i915_runtime_pm {
 	bool suspended;
 	bool irqs_disabled;
-
-	struct {
-		uint32_t deimr;
-		uint32_t sdeimr;
-		uint32_t gtimr;
-		uint32_t gtier;
-		uint32_t gen6_pmimr;
-	} regsave;
 };
 
 enum intel_pipe_crc_source {
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index dee3a3b..2aefb94 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -131,11 +131,8 @@ ironlake_enable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
 {
 	assert_spin_locked(&dev_priv->irq_lock);
 
-	if (dev_priv->pm.irqs_disabled) {
-		WARN(1, "IRQs disabled\n");
-		dev_priv->pm.regsave.deimr &= ~mask;
+	if (WARN_ON(dev_priv->pm.irqs_disabled))
 		return;
-	}
 
 	if ((dev_priv->irq_mask & mask) != 0) {
 		dev_priv->irq_mask &= ~mask;
@@ -149,11 +146,8 @@ ironlake_disable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
 {
 	assert_spin_locked(&dev_priv->irq_lock);
 
-	if (dev_priv->pm.irqs_disabled) {
-		WARN(1, "IRQs disabled\n");
-		dev_priv->pm.regsave.deimr |= mask;
+	if (WARN_ON(dev_priv->pm.irqs_disabled))
 		return;
-	}
 
 	if ((dev_priv->irq_mask & mask) != mask) {
 		dev_priv->irq_mask |= mask;
@@ -174,13 +168,8 @@ static void ilk_update_gt_irq(struct drm_i915_private *dev_priv,
 {
 	assert_spin_locked(&dev_priv->irq_lock);
 
-	if (dev_priv->pm.irqs_disabled) {
-		WARN(1, "IRQs disabled\n");
-		dev_priv->pm.regsave.gtimr &= ~interrupt_mask;
-		dev_priv->pm.regsave.gtimr |= (~enabled_irq_mask &
-						interrupt_mask);
+	if (WARN_ON(dev_priv->pm.irqs_disabled))
 		return;
-	}
 
 	dev_priv->gt_irq_mask &= ~interrupt_mask;
 	dev_priv->gt_irq_mask |= (~enabled_irq_mask & interrupt_mask);
@@ -212,13 +201,8 @@ static void snb_update_pm_irq(struct drm_i915_private *dev_priv,
 
 	assert_spin_locked(&dev_priv->irq_lock);
 
-	if (dev_priv->pm.irqs_disabled) {
-		WARN(1, "IRQs disabled\n");
-		dev_priv->pm.regsave.gen6_pmimr &= ~interrupt_mask;
-		dev_priv->pm.regsave.gen6_pmimr |= (~enabled_irq_mask &
-						     interrupt_mask);
+	if (WARN_ON(dev_priv->pm.irqs_disabled))
 		return;
-	}
 
 	new_val = dev_priv->pm_irq_mask;
 	new_val &= ~interrupt_mask;
@@ -358,14 +342,8 @@ static void ibx_display_interrupt_update(struct drm_i915_private *dev_priv,
 
 	assert_spin_locked(&dev_priv->irq_lock);
 
-	if (dev_priv->pm.irqs_disabled &&
-	    (interrupt_mask & SDE_HOTPLUG_MASK_CPT)) {
-		WARN(1, "IRQs disabled\n");
-		dev_priv->pm.regsave.sdeimr &= ~interrupt_mask;
-		dev_priv->pm.regsave.sdeimr |= (~enabled_irq_mask &
-						 interrupt_mask);
+	if (WARN_ON(dev_priv->pm.irqs_disabled))
 		return;
-	}
 
 	I915_WRITE(SDEIMR, sdeimr);
 	POSTING_READ(SDEIMR);
@@ -4091,57 +4069,20 @@ void intel_hpd_init(struct drm_device *dev)
 }
 
 /* Disable interrupts so we can allow runtime PM. */
-void hsw_runtime_pm_disable_interrupts(struct drm_device *dev)
+void intel_runtime_pm_disable_interrupts(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	unsigned long irqflags;
-
-	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
-
-	dev_priv->pm.regsave.deimr = I915_READ(DEIMR);
-	dev_priv->pm.regsave.sdeimr = I915_READ(SDEIMR);
-	dev_priv->pm.regsave.gtimr = I915_READ(GTIMR);
-	dev_priv->pm.regsave.gtier = I915_READ(GTIER);
-	dev_priv->pm.regsave.gen6_pmimr = I915_READ(GEN6_PMIMR);
-
-	ironlake_disable_display_irq(dev_priv, 0xffffffff);
-	ibx_disable_display_interrupt(dev_priv, 0xffffffff);
-	ilk_disable_gt_irq(dev_priv, 0xffffffff);
-	snb_disable_pm_irq(dev_priv, 0xffffffff);
 
+	dev->driver->irq_uninstall(dev);
 	dev_priv->pm.irqs_disabled = true;
-
-	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 }
 
 /* Restore interrupts so we can recover from runtime PM. */
-void hsw_runtime_pm_restore_interrupts(struct drm_device *dev)
+void intel_runtime_pm_restore_interrupts(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	unsigned long irqflags;
-	uint32_t val;
-
-	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
-
-	val = I915_READ(DEIMR);
-	WARN(val != 0xffffffff, "DEIMR is 0x%08x\n", val);
-
-	val = I915_READ(SDEIMR);
-	WARN(val != 0xffffffff, "SDEIMR is 0x%08x\n", val);
-
-	val = I915_READ(GTIMR);
-	WARN(val != 0xffffffff, "GTIMR is 0x%08x\n", val);
-
-	val = I915_READ(GEN6_PMIMR);
-	WARN(val != 0xffffffff, "GEN6_PMIMR is 0x%08x\n", val);
 
 	dev_priv->pm.irqs_disabled = false;
-
-	ironlake_enable_display_irq(dev_priv, ~dev_priv->pm.regsave.deimr);
-	ibx_enable_display_interrupt(dev_priv, ~dev_priv->pm.regsave.sdeimr);
-	ilk_enable_gt_irq(dev_priv, ~dev_priv->pm.regsave.gtimr);
-	snb_enable_pm_irq(dev_priv, ~dev_priv->pm.regsave.gen6_pmimr);
-	I915_WRITE(GTIER, dev_priv->pm.regsave.gtier);
-
-	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+	dev->driver->irq_preinstall(dev);
+	dev->driver->irq_postinstall(dev);
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ed9233e..df69866 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6829,7 +6829,7 @@ void hsw_enable_pc8(struct drm_i915_private *dev_priv)
 	}
 
 	lpt_disable_clkout_dp(dev);
-	hsw_runtime_pm_disable_interrupts(dev);
+	intel_runtime_pm_disable_interrupts(dev);
 	hsw_disable_lcpll(dev_priv, true, true);
 }
 
@@ -6843,7 +6843,7 @@ void hsw_disable_pc8(struct drm_i915_private *dev_priv)
 	DRM_DEBUG_KMS("Disabling package C8+\n");
 
 	hsw_restore_lcpll(dev_priv);
-	hsw_runtime_pm_restore_interrupts(dev);
+	intel_runtime_pm_restore_interrupts(dev);
 	lpt_init_pch_refclk(dev);
 
 	if (dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 9a7db84..0dfb6e9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -618,8 +618,8 @@ void ilk_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
 void ilk_disable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
 void snb_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
 void snb_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
-void hsw_runtime_pm_disable_interrupts(struct drm_device *dev);
-void hsw_runtime_pm_restore_interrupts(struct drm_device *dev);
+void intel_runtime_pm_disable_interrupts(struct drm_device *dev);
+void intel_runtime_pm_restore_interrupts(struct drm_device *dev);
 
 
 /* intel_crt.c */
-- 
1.8.5.3

[PATCH 2/6] drm/i915: add gen-specific runtime suspend/resume functions

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

We're adding runtime suspend support to more platforms, so organize
the code in a way that all a new platform needs to do is to add its
own gen-specific functions. Also rename the i915_ functions to intel_
to make it clear that it's the top level one, not something that just
runs on i915 platforms.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 74f7a91..f75d776 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -832,7 +832,23 @@ static int i915_pm_poweroff(struct device *dev)
 	return i915_drm_freeze(drm_dev);
 }
 
-static int i915_runtime_suspend(struct device *device)
+static void hsw_runtime_suspend(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = dev_priv->dev;
+
+	if (HAS_PC8(dev))
+		hsw_enable_pc8(dev_priv);
+}
+
+static void hsw_runtime_resume(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = dev_priv->dev;
+
+	if (HAS_PC8(dev))
+		hsw_disable_pc8(dev_priv);
+}
+
+static int intel_runtime_suspend(struct device *device)
 {
 	struct pci_dev *pdev = to_pci_dev(device);
 	struct drm_device *dev = pci_get_drvdata(pdev);
@@ -843,8 +859,8 @@ static int i915_runtime_suspend(struct device *device)
 
 	DRM_DEBUG_KMS("Suspending device\n");
 
-	if (HAS_PC8(dev))
-		hsw_enable_pc8(dev_priv);
+	if (IS_HASWELL(dev))
+		hsw_runtime_suspend(dev_priv);
 
 	i915_gem_release_all_mmaps(dev_priv);
 
@@ -864,7 +880,7 @@ static int i915_runtime_suspend(struct device *device)
 	return 0;
 }
 
-static int i915_runtime_resume(struct device *device)
+static int intel_runtime_resume(struct device *device)
 {
 	struct pci_dev *pdev = to_pci_dev(device);
 	struct drm_device *dev = pci_get_drvdata(pdev);
@@ -877,8 +893,8 @@ static int i915_runtime_resume(struct device *device)
 	intel_opregion_notify_adapter(dev, PCI_D0);
 	dev_priv->pm.suspended = false;
 
-	if (HAS_PC8(dev))
-		hsw_disable_pc8(dev_priv);
+	if (IS_HASWELL(dev))
+		hsw_runtime_resume(dev_priv);
 
 	DRM_DEBUG_KMS("Device resumed\n");
 	return 0;
@@ -891,8 +907,8 @@ static const struct dev_pm_ops i915_pm_ops = {
 	.thaw = i915_pm_thaw,
 	.poweroff = i915_pm_poweroff,
 	.restore = i915_pm_resume,
-	.runtime_suspend = i915_runtime_suspend,
-	.runtime_resume = i915_runtime_resume,
+	.runtime_suspend = intel_runtime_suspend,
+	.runtime_resume = intel_runtime_resume,
 };
 
 static const struct vm_operations_struct i915_gem_vm_ops = {
-- 
1.8.5.3

[PATCH 3/6] drm/i915: add SNB runtime PM support

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Just because I have a SNB machine and I can easily test it.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c      | 27 +++++++++++++++++++++++++--
 drivers/gpu/drm/i915/i915_drv.h      |  2 +-
 drivers/gpu/drm/i915/intel_display.c |  7 +++++++
 3 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index f75d776..2c62e0c 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -832,6 +832,13 @@ static int i915_pm_poweroff(struct device *dev)
 	return i915_drm_freeze(drm_dev);
 }
 
+static void snb_runtime_suspend(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = dev_priv->dev;
+
+	intel_runtime_pm_disable_interrupts(dev);
+}
+
 static void hsw_runtime_suspend(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
@@ -840,6 +847,18 @@ static void hsw_runtime_suspend(struct drm_i915_private *dev_priv)
 		hsw_enable_pc8(dev_priv);
 }
 
+static void snb_runtime_resume(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = dev_priv->dev;
+
+	intel_runtime_pm_restore_interrupts(dev);
+	intel_init_pch_refclk(dev);
+	i915_gem_init_swizzling(dev);
+	mutex_lock(&dev_priv->rps.hw_lock);
+	gen6_update_ring_freq(dev);
+	mutex_unlock(&dev_priv->rps.hw_lock);
+}
+
 static void hsw_runtime_resume(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
@@ -859,7 +878,9 @@ static int intel_runtime_suspend(struct device *device)
 
 	DRM_DEBUG_KMS("Suspending device\n");
 
-	if (IS_HASWELL(dev))
+	if (IS_GEN6(dev))
+		snb_runtime_suspend(dev_priv);
+	else if (IS_HASWELL(dev))
 		hsw_runtime_suspend(dev_priv);
 
 	i915_gem_release_all_mmaps(dev_priv);
@@ -893,7 +914,9 @@ static int intel_runtime_resume(struct device *device)
 	intel_opregion_notify_adapter(dev, PCI_D0);
 	dev_priv->pm.suspended = false;
 
-	if (IS_HASWELL(dev))
+	if (IS_GEN6(dev))
+		snb_runtime_resume(dev_priv);
+	else if (IS_HASWELL(dev))
 		hsw_runtime_resume(dev_priv);
 
 	DRM_DEBUG_KMS("Device resumed\n");
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 493579d..309852d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1998,7 +1998,7 @@ struct drm_i915_cmd_table {
 #define HAS_FPGA_DBG_UNCLAIMED(dev)	(INTEL_INFO(dev)->has_fpga_dbg)
 #define HAS_PSR(dev)		(IS_HASWELL(dev) || IS_BROADWELL(dev))
 #define HAS_PC8(dev)		(IS_HASWELL(dev)) /* XXX HSW:ULX */
-#define HAS_RUNTIME_PM(dev)	(IS_HASWELL(dev))
+#define HAS_RUNTIME_PM(dev)	(IS_GEN6(dev) || IS_HASWELL(dev))
 
 #define INTEL_PCH_DEVICE_ID_MASK		0xff00
 #define INTEL_PCH_IBX_DEVICE_ID_TYPE		0x3b00
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index df69866..35f65c1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6859,6 +6859,11 @@ void hsw_disable_pc8(struct drm_i915_private *dev_priv)
 	mutex_unlock(&dev_priv->rps.hw_lock);
 }
 
+static void snb_modeset_global_resources(struct drm_device *dev)
+{
+	modeset_update_crtc_power_domains(dev);
+}
+
 static void haswell_modeset_global_resources(struct drm_device *dev)
 {
 	modeset_update_crtc_power_domains(dev);
@@ -10758,6 +10763,8 @@ static void intel_init_display(struct drm_device *dev)
 		} else if (IS_GEN6(dev)) {
 			dev_priv->display.fdi_link_train = gen6_fdi_link_train;
 			dev_priv->display.write_eld = ironlake_write_eld;
+			dev_priv->display.modeset_global_resources =
+				snb_modeset_global_resources;
 		} else if (IS_IVYBRIDGE(dev)) {
 			/* FIXME: detect B0+ stepping and use auto training */
 			dev_priv->display.fdi_link_train = ivb_manual_fdi_link_train;
-- 
1.8.5.3

[PATCH 4/6] drm/i915: remove HAS_PC8 check

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Now that PC8 is part of runtime PM, the check is useless.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c      | 10 ++--------
 drivers/gpu/drm/i915/i915_drv.h      |  1 -
 drivers/gpu/drm/i915/intel_display.c |  4 ----
 3 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 2c62e0c..55f0181 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -841,10 +841,7 @@ static void snb_runtime_suspend(struct drm_i915_private *dev_priv)
 
 static void hsw_runtime_suspend(struct drm_i915_private *dev_priv)
 {
-	struct drm_device *dev = dev_priv->dev;
-
-	if (HAS_PC8(dev))
-		hsw_enable_pc8(dev_priv);
+	hsw_enable_pc8(dev_priv);
 }
 
 static void snb_runtime_resume(struct drm_i915_private *dev_priv)
@@ -861,10 +858,7 @@ static void snb_runtime_resume(struct drm_i915_private *dev_priv)
 
 static void hsw_runtime_resume(struct drm_i915_private *dev_priv)
 {
-	struct drm_device *dev = dev_priv->dev;
-
-	if (HAS_PC8(dev))
-		hsw_disable_pc8(dev_priv);
+	hsw_disable_pc8(dev_priv);
 }
 
 static int intel_runtime_suspend(struct device *device)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 309852d..1debc412 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1997,7 +1997,6 @@ struct drm_i915_cmd_table {
 #define HAS_DDI(dev)		(INTEL_INFO(dev)->has_ddi)
 #define HAS_FPGA_DBG_UNCLAIMED(dev)	(INTEL_INFO(dev)->has_fpga_dbg)
 #define HAS_PSR(dev)		(IS_HASWELL(dev) || IS_BROADWELL(dev))
-#define HAS_PC8(dev)		(IS_HASWELL(dev)) /* XXX HSW:ULX */
 #define HAS_RUNTIME_PM(dev)	(IS_GEN6(dev) || IS_HASWELL(dev))
 
 #define INTEL_PCH_DEVICE_ID_MASK		0xff00
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 35f65c1..d6092be 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6818,8 +6818,6 @@ void hsw_enable_pc8(struct drm_i915_private *dev_priv)
 	struct drm_device *dev = dev_priv->dev;
 	uint32_t val;
 
-	WARN_ON(!HAS_PC8(dev));
-
 	DRM_DEBUG_KMS("Enabling package C8+\n");
 
 	if (dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
@@ -6838,8 +6836,6 @@ void hsw_disable_pc8(struct drm_i915_private *dev_priv)
 	struct drm_device *dev = dev_priv->dev;
 	uint32_t val;
 
-	WARN_ON(!HAS_PC8(dev));
-
 	DRM_DEBUG_KMS("Disabling package C8+\n");
 
 	hsw_restore_lcpll(dev_priv);
-- 
1.8.5.3

[PATCH 5/6] drm/i915: BDW needs D_COMP writes through MCHBAR

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

That's what the spec said! And HSW needs it through pcode (you can
only read it through MCHBAR), so create hsw_write_dcomp to abstract
the weirdness.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d6092be..2be4129 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6664,6 +6664,22 @@ static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv)
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 }
 
+static void hsw_write_dcomp(struct drm_i915_private *dev_priv, uint32_t val)
+{
+	struct drm_device *dev = dev_priv->dev;
+
+	if (IS_HASWELL(dev)) {
+		mutex_lock(&dev_priv->rps.hw_lock);
+		if (sandybridge_pcode_write(dev_priv, GEN6_PCODE_WRITE_D_COMP,
+					    val))
+			DRM_ERROR("Failed to disable D_COMP\n");
+		mutex_unlock(&dev_priv->rps.hw_lock);
+	} else {
+		I915_WRITE(D_COMP, val);
+	}
+	POSTING_READ(D_COMP);
+}
+
 /*
  * This function implements pieces of two sequences from BSpec:
  * - Sequence for display software to disable LCPLL
@@ -6701,11 +6717,7 @@ static void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
 
 	val = I915_READ(D_COMP);
 	val |= D_COMP_COMP_DISABLE;
-	mutex_lock(&dev_priv->rps.hw_lock);
-	if (sandybridge_pcode_write(dev_priv, GEN6_PCODE_WRITE_D_COMP, val))
-		DRM_ERROR("Failed to disable D_COMP\n");
-	mutex_unlock(&dev_priv->rps.hw_lock);
-	POSTING_READ(D_COMP);
+	hsw_write_dcomp(dev_priv, val);
 	ndelay(100);
 
 	if (wait_for((I915_READ(D_COMP) & D_COMP_RCOMP_IN_PROGRESS) == 0, 1))
@@ -6760,11 +6772,7 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
 	val = I915_READ(D_COMP);
 	val |= D_COMP_COMP_FORCE;
 	val &= ~D_COMP_COMP_DISABLE;
-	mutex_lock(&dev_priv->rps.hw_lock);
-	if (sandybridge_pcode_write(dev_priv, GEN6_PCODE_WRITE_D_COMP, val))
-		DRM_ERROR("Failed to enable D_COMP\n");
-	mutex_unlock(&dev_priv->rps.hw_lock);
-	POSTING_READ(D_COMP);
+	hsw_write_dcomp(dev_priv, val);
 
 	val = I915_READ(LCPLL_CTL);
 	val &= ~LCPLL_PLL_DISABLE;
-- 
1.8.5.3

[PATCH 6/6] drm/i915: add BDW runtime PM support

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

This sould be enough.

v2: BDW should also run hsw_runtime_resume (Ben).

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 8 ++++++--
 drivers/gpu/drm/i915/i915_drv.h | 3 ++-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 55f0181..2dcbbc0 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -874,8 +874,10 @@ static int intel_runtime_suspend(struct device *device)
 
 	if (IS_GEN6(dev))
 		snb_runtime_suspend(dev_priv);
-	else if (IS_HASWELL(dev))
+	else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
 		hsw_runtime_suspend(dev_priv);
+	else
+		WARN_ON(1);
 
 	i915_gem_release_all_mmaps(dev_priv);
 
@@ -910,8 +912,10 @@ static int intel_runtime_resume(struct device *device)
 
 	if (IS_GEN6(dev))
 		snb_runtime_resume(dev_priv);
-	else if (IS_HASWELL(dev))
+	else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
 		hsw_runtime_resume(dev_priv);
+	else
+		WARN_ON(1);
 
 	DRM_DEBUG_KMS("Device resumed\n");
 	return 0;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1debc412..2f7246a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1997,7 +1997,8 @@ struct drm_i915_cmd_table {
 #define HAS_DDI(dev)		(INTEL_INFO(dev)->has_ddi)
 #define HAS_FPGA_DBG_UNCLAIMED(dev)	(INTEL_INFO(dev)->has_fpga_dbg)
 #define HAS_PSR(dev)		(IS_HASWELL(dev) || IS_BROADWELL(dev))
-#define HAS_RUNTIME_PM(dev)	(IS_GEN6(dev) || IS_HASWELL(dev))
+#define HAS_RUNTIME_PM(dev)	(IS_GEN6(dev) || IS_HASWELL(dev) || \
+				 IS_BROADWELL(dev))
 
 #define INTEL_PCH_DEVICE_ID_MASK		0xff00
 #define INTEL_PCH_IBX_DEVICE_ID_TYPE		0x3b00
-- 
1.8.5.3

Re: [PATCH 1/6] drm/i915: kill dev_priv->pm.regsave

[Imre Deak]

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

On Fri, 2014-03-07 at 20:12 -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Now that we don't keep the hotplug interrupts enabled anymore, we can
> kill the regsave struct and just cal the normal IRQ preinstall,
> postinstall and uninstall functions. This makes it easier to add
> runtime PM support to non-HSW platforms.
> 
> The only downside is in case we get a request to update interrupts
> while they are disabled, won't be able to update the regsave struct.
> But this should never happen anyway, so we're not losing too much.
> 
> v2: - Rebase.
> v3: - Rebase.
> v4: - Rebase.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      | 12 +-----
>  drivers/gpu/drm/i915/i915_irq.c      | 79 +++++-------------------------------
>  drivers/gpu/drm/i915/intel_display.c |  4 +-
>  drivers/gpu/drm/i915/intel_drv.h     |  4 +-
>  4 files changed, 15 insertions(+), 84 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 70feb61..493579d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1351,23 +1351,13 @@ struct ilk_wm_values {
>   * goes back to false exactly before we reenable the IRQs. We use this variable
>   * to check if someone is trying to enable/disable IRQs while they're supposed
>   * to be disabled. This shouldn't happen and we'll print some error messages in
> - * case it happens, but if it actually happens we'll also update the variables
> - * inside struct regsave so when we restore the IRQs they will contain the
> - * latest expected values.
> + * case it happens.
>   *
>   * For more, read the Documentation/power/runtime_pm.txt.
>   */
>  struct i915_runtime_pm {
>  	bool suspended;
>  	bool irqs_disabled;
> -
> -	struct {
> -		uint32_t deimr;
> -		uint32_t sdeimr;
> -		uint32_t gtimr;
> -		uint32_t gtier;
> -		uint32_t gen6_pmimr;
> -	} regsave;
>  };
>  
>  enum intel_pipe_crc_source {
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index dee3a3b..2aefb94 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -131,11 +131,8 @@ ironlake_enable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
>  {
>  	assert_spin_locked(&dev_priv->irq_lock);
>  
> -	if (dev_priv->pm.irqs_disabled) {
> -		WARN(1, "IRQs disabled\n");
> -		dev_priv->pm.regsave.deimr &= ~mask;
> +	if (WARN_ON(dev_priv->pm.irqs_disabled))
>  		return;
> -	}
>  
>  	if ((dev_priv->irq_mask & mask) != 0) {
>  		dev_priv->irq_mask &= ~mask;
> @@ -149,11 +146,8 @@ ironlake_disable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
>  {
>  	assert_spin_locked(&dev_priv->irq_lock);
>  
> -	if (dev_priv->pm.irqs_disabled) {
> -		WARN(1, "IRQs disabled\n");
> -		dev_priv->pm.regsave.deimr |= mask;
> +	if (WARN_ON(dev_priv->pm.irqs_disabled))
>  		return;
> -	}
>  
>  	if ((dev_priv->irq_mask & mask) != mask) {
>  		dev_priv->irq_mask |= mask;
> @@ -174,13 +168,8 @@ static void ilk_update_gt_irq(struct drm_i915_private *dev_priv,
>  {
>  	assert_spin_locked(&dev_priv->irq_lock);
>  
> -	if (dev_priv->pm.irqs_disabled) {
> -		WARN(1, "IRQs disabled\n");
> -		dev_priv->pm.regsave.gtimr &= ~interrupt_mask;
> -		dev_priv->pm.regsave.gtimr |= (~enabled_irq_mask &
> -						interrupt_mask);
> +	if (WARN_ON(dev_priv->pm.irqs_disabled))
>  		return;
> -	}
>  
>  	dev_priv->gt_irq_mask &= ~interrupt_mask;
>  	dev_priv->gt_irq_mask |= (~enabled_irq_mask & interrupt_mask);
> @@ -212,13 +201,8 @@ static void snb_update_pm_irq(struct drm_i915_private *dev_priv,
>  
>  	assert_spin_locked(&dev_priv->irq_lock);
>  
> -	if (dev_priv->pm.irqs_disabled) {
> -		WARN(1, "IRQs disabled\n");
> -		dev_priv->pm.regsave.gen6_pmimr &= ~interrupt_mask;
> -		dev_priv->pm.regsave.gen6_pmimr |= (~enabled_irq_mask &
> -						     interrupt_mask);
> +	if (WARN_ON(dev_priv->pm.irqs_disabled))
>  		return;
> -	}
>  
>  	new_val = dev_priv->pm_irq_mask;
>  	new_val &= ~interrupt_mask;
> @@ -358,14 +342,8 @@ static void ibx_display_interrupt_update(struct drm_i915_private *dev_priv,
>  
>  	assert_spin_locked(&dev_priv->irq_lock);
>  
> -	if (dev_priv->pm.irqs_disabled &&
> -	    (interrupt_mask & SDE_HOTPLUG_MASK_CPT)) {
> -		WARN(1, "IRQs disabled\n");
> -		dev_priv->pm.regsave.sdeimr &= ~interrupt_mask;
> -		dev_priv->pm.regsave.sdeimr |= (~enabled_irq_mask &
> -						 interrupt_mask);
> +	if (WARN_ON(dev_priv->pm.irqs_disabled))
>  		return;
> -	}
>  
>  	I915_WRITE(SDEIMR, sdeimr);
>  	POSTING_READ(SDEIMR);
> @@ -4091,57 +4069,20 @@ void intel_hpd_init(struct drm_device *dev)
>  }
>  
>  /* Disable interrupts so we can allow runtime PM. */
> -void hsw_runtime_pm_disable_interrupts(struct drm_device *dev)
> +void intel_runtime_pm_disable_interrupts(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	unsigned long irqflags;
> -
> -	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> -
> -	dev_priv->pm.regsave.deimr = I915_READ(DEIMR);
> -	dev_priv->pm.regsave.sdeimr = I915_READ(SDEIMR);
> -	dev_priv->pm.regsave.gtimr = I915_READ(GTIMR);
> -	dev_priv->pm.regsave.gtier = I915_READ(GTIER);
> -	dev_priv->pm.regsave.gen6_pmimr = I915_READ(GEN6_PMIMR);
> -
> -	ironlake_disable_display_irq(dev_priv, 0xffffffff);
> -	ibx_disable_display_interrupt(dev_priv, 0xffffffff);
> -	ilk_disable_gt_irq(dev_priv, 0xffffffff);
> -	snb_disable_pm_irq(dev_priv, 0xffffffff);
>  
> +	dev->driver->irq_uninstall(dev);
>  	dev_priv->pm.irqs_disabled = true;
> -
> -	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);

It made me think whether removing the locking here is ok. It seems to be
ok, as we get here in an idle state where no interrupts should happen. A
note about this change in the commit message would have been nice.
Otherwise the patch looks ok:

Reviewed-by: Imre Deak <imre.deak@intel.com>


>  }
>  
>  /* Restore interrupts so we can recover from runtime PM. */
> -void hsw_runtime_pm_restore_interrupts(struct drm_device *dev)
> +void intel_runtime_pm_restore_interrupts(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	unsigned long irqflags;
> -	uint32_t val;
> -
> -	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> -
> -	val = I915_READ(DEIMR);
> -	WARN(val != 0xffffffff, "DEIMR is 0x%08x\n", val);
> -
> -	val = I915_READ(SDEIMR);
> -	WARN(val != 0xffffffff, "SDEIMR is 0x%08x\n", val);
> -
> -	val = I915_READ(GTIMR);
> -	WARN(val != 0xffffffff, "GTIMR is 0x%08x\n", val);
> -
> -	val = I915_READ(GEN6_PMIMR);
> -	WARN(val != 0xffffffff, "GEN6_PMIMR is 0x%08x\n", val);
>  
>  	dev_priv->pm.irqs_disabled = false;
> -
> -	ironlake_enable_display_irq(dev_priv, ~dev_priv->pm.regsave.deimr);
> -	ibx_enable_display_interrupt(dev_priv, ~dev_priv->pm.regsave.sdeimr);
> -	ilk_enable_gt_irq(dev_priv, ~dev_priv->pm.regsave.gtimr);
> -	snb_enable_pm_irq(dev_priv, ~dev_priv->pm.regsave.gen6_pmimr);
> -	I915_WRITE(GTIER, dev_priv->pm.regsave.gtier);
> -
> -	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> +	dev->driver->irq_preinstall(dev);
> +	dev->driver->irq_postinstall(dev);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ed9233e..df69866 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6829,7 +6829,7 @@ void hsw_enable_pc8(struct drm_i915_private *dev_priv)
>  	}
>  
>  	lpt_disable_clkout_dp(dev);
> -	hsw_runtime_pm_disable_interrupts(dev);
> +	intel_runtime_pm_disable_interrupts(dev);
>  	hsw_disable_lcpll(dev_priv, true, true);
>  }
>  
> @@ -6843,7 +6843,7 @@ void hsw_disable_pc8(struct drm_i915_private *dev_priv)
>  	DRM_DEBUG_KMS("Disabling package C8+\n");
>  
>  	hsw_restore_lcpll(dev_priv);
> -	hsw_runtime_pm_restore_interrupts(dev);
> +	intel_runtime_pm_restore_interrupts(dev);
>  	lpt_init_pch_refclk(dev);
>  
>  	if (dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 9a7db84..0dfb6e9 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -618,8 +618,8 @@ void ilk_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
>  void ilk_disable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
>  void snb_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
>  void snb_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
> -void hsw_runtime_pm_disable_interrupts(struct drm_device *dev);
> -void hsw_runtime_pm_restore_interrupts(struct drm_device *dev);
> +void intel_runtime_pm_disable_interrupts(struct drm_device *dev);
> +void intel_runtime_pm_restore_interrupts(struct drm_device *dev);
>  
> 
>  /* intel_crt.c */


[-- 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 2/6] drm/i915: add gen-specific runtime suspend/resume functions

[Imre Deak]

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

On Fri, 2014-03-07 at 20:12 -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> We're adding runtime suspend support to more platforms, so organize
> the code in a way that all a new platform needs to do is to add its
> own gen-specific functions. Also rename the i915_ functions to intel_
> to make it clear that it's the top level one, not something that just
> runs on i915 platforms.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_drv.c | 32 ++++++++++++++++++++++++--------
>  1 file changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 74f7a91..f75d776 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -832,7 +832,23 @@ static int i915_pm_poweroff(struct device *dev)
>  	return i915_drm_freeze(drm_dev);
>  }
>  
> -static int i915_runtime_suspend(struct device *device)
> +static void hsw_runtime_suspend(struct drm_i915_private *dev_priv)
> +{
> +	struct drm_device *dev = dev_priv->dev;
> +
> +	if (HAS_PC8(dev))
> +		hsw_enable_pc8(dev_priv);
> +}
> +
> +static void hsw_runtime_resume(struct drm_i915_private *dev_priv)
> +{
> +	struct drm_device *dev = dev_priv->dev;
> +
> +	if (HAS_PC8(dev))
> +		hsw_disable_pc8(dev_priv);
> +}
> +
> +static int intel_runtime_suspend(struct device *device)
>  {
>  	struct pci_dev *pdev = to_pci_dev(device);
>  	struct drm_device *dev = pci_get_drvdata(pdev);
> @@ -843,8 +859,8 @@ static int i915_runtime_suspend(struct device *device)
>  
>  	DRM_DEBUG_KMS("Suspending device\n");
>  
> -	if (HAS_PC8(dev))
> -		hsw_enable_pc8(dev_priv);
> +	if (IS_HASWELL(dev))
> +		hsw_runtime_suspend(dev_priv);
>  
>  	i915_gem_release_all_mmaps(dev_priv);
>  
> @@ -864,7 +880,7 @@ static int i915_runtime_suspend(struct device *device)
>  	return 0;
>  }
>  
> -static int i915_runtime_resume(struct device *device)
> +static int intel_runtime_resume(struct device *device)
>  {
>  	struct pci_dev *pdev = to_pci_dev(device);
>  	struct drm_device *dev = pci_get_drvdata(pdev);
> @@ -877,8 +893,8 @@ static int i915_runtime_resume(struct device *device)
>  	intel_opregion_notify_adapter(dev, PCI_D0);
>  	dev_priv->pm.suspended = false;
>  
> -	if (HAS_PC8(dev))
> -		hsw_disable_pc8(dev_priv);
> +	if (IS_HASWELL(dev))
> +		hsw_runtime_resume(dev_priv);
>  
>  	DRM_DEBUG_KMS("Device resumed\n");
>  	return 0;
> @@ -891,8 +907,8 @@ static const struct dev_pm_ops i915_pm_ops = {
>  	.thaw = i915_pm_thaw,
>  	.poweroff = i915_pm_poweroff,
>  	.restore = i915_pm_resume,
> -	.runtime_suspend = i915_runtime_suspend,
> -	.runtime_resume = i915_runtime_resume,
> +	.runtime_suspend = intel_runtime_suspend,
> +	.runtime_resume = intel_runtime_resume,
>  };
>  
>  static const struct vm_operations_struct i915_gem_vm_ops = {


[-- 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/6] drm/i915: remove HAS_PC8 check

[Imre Deak]

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

On Fri, 2014-03-07 at 20:12 -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Now that PC8 is part of runtime PM, the check is useless.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Looks ok. It could be applied already after 2/6.
Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_drv.c      | 10 ++--------
>  drivers/gpu/drm/i915/i915_drv.h      |  1 -
>  drivers/gpu/drm/i915/intel_display.c |  4 ----
>  3 files changed, 2 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 2c62e0c..55f0181 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -841,10 +841,7 @@ static void snb_runtime_suspend(struct drm_i915_private *dev_priv)
>  
>  static void hsw_runtime_suspend(struct drm_i915_private *dev_priv)
>  {
> -	struct drm_device *dev = dev_priv->dev;
> -
> -	if (HAS_PC8(dev))
> -		hsw_enable_pc8(dev_priv);
> +	hsw_enable_pc8(dev_priv);
>  }
>  
>  static void snb_runtime_resume(struct drm_i915_private *dev_priv)
> @@ -861,10 +858,7 @@ static void snb_runtime_resume(struct drm_i915_private *dev_priv)
>  
>  static void hsw_runtime_resume(struct drm_i915_private *dev_priv)
>  {
> -	struct drm_device *dev = dev_priv->dev;
> -
> -	if (HAS_PC8(dev))
> -		hsw_disable_pc8(dev_priv);
> +	hsw_disable_pc8(dev_priv);
>  }
>  
>  static int intel_runtime_suspend(struct device *device)
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 309852d..1debc412 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1997,7 +1997,6 @@ struct drm_i915_cmd_table {
>  #define HAS_DDI(dev)		(INTEL_INFO(dev)->has_ddi)
>  #define HAS_FPGA_DBG_UNCLAIMED(dev)	(INTEL_INFO(dev)->has_fpga_dbg)
>  #define HAS_PSR(dev)		(IS_HASWELL(dev) || IS_BROADWELL(dev))
> -#define HAS_PC8(dev)		(IS_HASWELL(dev)) /* XXX HSW:ULX */
>  #define HAS_RUNTIME_PM(dev)	(IS_GEN6(dev) || IS_HASWELL(dev))
>  
>  #define INTEL_PCH_DEVICE_ID_MASK		0xff00
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 35f65c1..d6092be 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6818,8 +6818,6 @@ void hsw_enable_pc8(struct drm_i915_private *dev_priv)
>  	struct drm_device *dev = dev_priv->dev;
>  	uint32_t val;
>  
> -	WARN_ON(!HAS_PC8(dev));
> -
>  	DRM_DEBUG_KMS("Enabling package C8+\n");
>  
>  	if (dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
> @@ -6838,8 +6836,6 @@ void hsw_disable_pc8(struct drm_i915_private *dev_priv)
>  	struct drm_device *dev = dev_priv->dev;
>  	uint32_t val;
>  
> -	WARN_ON(!HAS_PC8(dev));
> -
>  	DRM_DEBUG_KMS("Disabling package C8+\n");
>  
>  	hsw_restore_lcpll(dev_priv);


[-- 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 3/6] drm/i915: add SNB runtime PM support

[Imre Deak]

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

On Fri, 2014-03-07 at 20:12 -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Just because I have a SNB machine and I can easily test it.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c      | 27 +++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/i915_drv.h      |  2 +-
>  drivers/gpu/drm/i915/intel_display.c |  7 +++++++
>  3 files changed, 33 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index f75d776..2c62e0c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -832,6 +832,13 @@ static int i915_pm_poweroff(struct device *dev)
>  	return i915_drm_freeze(drm_dev);
>  }
>  
> +static void snb_runtime_suspend(struct drm_i915_private *dev_priv)
> +{
> +	struct drm_device *dev = dev_priv->dev;
> +
> +	intel_runtime_pm_disable_interrupts(dev);
> +}

Afaics RPS is still enabled here, so if we need it enabled at this point
(so that GT stays at RC6 for example) we'd at least need here

cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work);
cancel_work_sync(&dev_priv->rps.work);

> +
>  static void hsw_runtime_suspend(struct drm_i915_private *dev_priv)
>  {
>  	struct drm_device *dev = dev_priv->dev;
> @@ -840,6 +847,18 @@ static void hsw_runtime_suspend(struct drm_i915_private *dev_priv)
>  		hsw_enable_pc8(dev_priv);
>  }
>  
> +static void snb_runtime_resume(struct drm_i915_private *dev_priv)
> +{
> +	struct drm_device *dev = dev_priv->dev;
> +
> +	intel_runtime_pm_restore_interrupts(dev);
> +	intel_init_pch_refclk(dev);
> +	i915_gem_init_swizzling(dev);
> +	mutex_lock(&dev_priv->rps.hw_lock);
> +	gen6_update_ring_freq(dev);
> +	mutex_unlock(&dev_priv->rps.hw_lock);
> +}

I haven't found any info on what exact state is lost in D3, but in any
case documenting this in the code would be great.

Here there are some bits restored for GTI in i915_gem_init_swizzling(),
but what about the clock gating registers and RPS registers. I'd be
easier if those would be also re-inited here, or a confirmation that
they are saved/restored by HW.

--Imre

> +
>  static void hsw_runtime_resume(struct drm_i915_private *dev_priv)
>  {
>  	struct drm_device *dev = dev_priv->dev;
> @@ -859,7 +878,9 @@ static int intel_runtime_suspend(struct device *device)
>  
>  	DRM_DEBUG_KMS("Suspending device\n");
>  
> -	if (IS_HASWELL(dev))
> +	if (IS_GEN6(dev))
> +		snb_runtime_suspend(dev_priv);
> +	else if (IS_HASWELL(dev))
>  		hsw_runtime_suspend(dev_priv);
>  
>  	i915_gem_release_all_mmaps(dev_priv);
> @@ -893,7 +914,9 @@ static int intel_runtime_resume(struct device *device)
>  	intel_opregion_notify_adapter(dev, PCI_D0);
>  	dev_priv->pm.suspended = false;
>  
> -	if (IS_HASWELL(dev))
> +	if (IS_GEN6(dev))
> +		snb_runtime_resume(dev_priv);
> +	else if (IS_HASWELL(dev))
>  		hsw_runtime_resume(dev_priv);
>  
>  	DRM_DEBUG_KMS("Device resumed\n");
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 493579d..309852d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1998,7 +1998,7 @@ struct drm_i915_cmd_table {
>  #define HAS_FPGA_DBG_UNCLAIMED(dev)	(INTEL_INFO(dev)->has_fpga_dbg)
>  #define HAS_PSR(dev)		(IS_HASWELL(dev) || IS_BROADWELL(dev))
>  #define HAS_PC8(dev)		(IS_HASWELL(dev)) /* XXX HSW:ULX */
> -#define HAS_RUNTIME_PM(dev)	(IS_HASWELL(dev))
> +#define HAS_RUNTIME_PM(dev)	(IS_GEN6(dev) || IS_HASWELL(dev))
>  
>  #define INTEL_PCH_DEVICE_ID_MASK		0xff00
>  #define INTEL_PCH_IBX_DEVICE_ID_TYPE		0x3b00
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index df69866..35f65c1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6859,6 +6859,11 @@ void hsw_disable_pc8(struct drm_i915_private *dev_priv)
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  }
>  
> +static void snb_modeset_global_resources(struct drm_device *dev)
> +{
> +	modeset_update_crtc_power_domains(dev);
> +}
> +
>  static void haswell_modeset_global_resources(struct drm_device *dev)
>  {
>  	modeset_update_crtc_power_domains(dev);
> @@ -10758,6 +10763,8 @@ static void intel_init_display(struct drm_device *dev)
>  		} else if (IS_GEN6(dev)) {
>  			dev_priv->display.fdi_link_train = gen6_fdi_link_train;
>  			dev_priv->display.write_eld = ironlake_write_eld;
> +			dev_priv->display.modeset_global_resources =
> +				snb_modeset_global_resources;
>  		} else if (IS_IVYBRIDGE(dev)) {
>  			/* FIXME: detect B0+ stepping and use auto training */
>  			dev_priv->display.fdi_link_train = ivb_manual_fdi_link_train;


[-- 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 5/6] drm/i915: BDW needs D_COMP writes through MCHBAR

[Imre Deak]

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

On Fri, 2014-03-07 at 20:12 -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> That's what the spec said! And HSW needs it through pcode (you can
> only read it through MCHBAR), so create hsw_write_dcomp to abstract
> the weirdness.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d6092be..2be4129 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6664,6 +6664,22 @@ static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv)
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  }
>  
> +static void hsw_write_dcomp(struct drm_i915_private *dev_priv, uint32_t val)
> +{
> +	struct drm_device *dev = dev_priv->dev;
> +
> +	if (IS_HASWELL(dev)) {
> +		mutex_lock(&dev_priv->rps.hw_lock);
> +		if (sandybridge_pcode_write(dev_priv, GEN6_PCODE_WRITE_D_COMP,
> +					    val))
> +			DRM_ERROR("Failed to disable D_COMP\n");
> +		mutex_unlock(&dev_priv->rps.hw_lock);
> +	} else {
> +		I915_WRITE(D_COMP, val);
> +	}
> +	POSTING_READ(D_COMP);
> +}
> +
>  /*
>   * This function implements pieces of two sequences from BSpec:
>   * - Sequence for display software to disable LCPLL
> @@ -6701,11 +6717,7 @@ static void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
>  
>  	val = I915_READ(D_COMP);
>  	val |= D_COMP_COMP_DISABLE;
> -	mutex_lock(&dev_priv->rps.hw_lock);
> -	if (sandybridge_pcode_write(dev_priv, GEN6_PCODE_WRITE_D_COMP, val))
> -		DRM_ERROR("Failed to disable D_COMP\n");
> -	mutex_unlock(&dev_priv->rps.hw_lock);
> -	POSTING_READ(D_COMP);
> +	hsw_write_dcomp(dev_priv, val);
>  	ndelay(100);
>  
>  	if (wait_for((I915_READ(D_COMP) & D_COMP_RCOMP_IN_PROGRESS) == 0, 1))
> @@ -6760,11 +6772,7 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
>  	val = I915_READ(D_COMP);
>  	val |= D_COMP_COMP_FORCE;
>  	val &= ~D_COMP_COMP_DISABLE;
> -	mutex_lock(&dev_priv->rps.hw_lock);
> -	if (sandybridge_pcode_write(dev_priv, GEN6_PCODE_WRITE_D_COMP, val))
> -		DRM_ERROR("Failed to enable D_COMP\n");
> -	mutex_unlock(&dev_priv->rps.hw_lock);
> -	POSTING_READ(D_COMP);
> +	hsw_write_dcomp(dev_priv, val);
>  
>  	val = I915_READ(LCPLL_CTL);
>  	val &= ~LCPLL_PLL_DISABLE;


[-- 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 6/6] drm/i915: add BDW runtime PM support

[Imre Deak]

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

On Fri, 2014-03-07 at 20:12 -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> This sould be enough.
> 
> v2: BDW should also run hsw_runtime_resume (Ben).
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Looks good,
Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_drv.c | 8 ++++++--
>  drivers/gpu/drm/i915/i915_drv.h | 3 ++-
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 55f0181..2dcbbc0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -874,8 +874,10 @@ static int intel_runtime_suspend(struct device *device)
>  
>  	if (IS_GEN6(dev))
>  		snb_runtime_suspend(dev_priv);
> -	else if (IS_HASWELL(dev))
> +	else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>  		hsw_runtime_suspend(dev_priv);
> +	else
> +		WARN_ON(1);
>  
>  	i915_gem_release_all_mmaps(dev_priv);
>  
> @@ -910,8 +912,10 @@ static int intel_runtime_resume(struct device *device)
>  
>  	if (IS_GEN6(dev))
>  		snb_runtime_resume(dev_priv);
> -	else if (IS_HASWELL(dev))
> +	else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>  		hsw_runtime_resume(dev_priv);
> +	else
> +		WARN_ON(1);
>  
>  	DRM_DEBUG_KMS("Device resumed\n");
>  	return 0;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1debc412..2f7246a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1997,7 +1997,8 @@ struct drm_i915_cmd_table {
>  #define HAS_DDI(dev)		(INTEL_INFO(dev)->has_ddi)
>  #define HAS_FPGA_DBG_UNCLAIMED(dev)	(INTEL_INFO(dev)->has_fpga_dbg)
>  #define HAS_PSR(dev)		(IS_HASWELL(dev) || IS_BROADWELL(dev))
> -#define HAS_RUNTIME_PM(dev)	(IS_GEN6(dev) || IS_HASWELL(dev))
> +#define HAS_RUNTIME_PM(dev)	(IS_GEN6(dev) || IS_HASWELL(dev) || \
> +				 IS_BROADWELL(dev))
>  
>  #define INTEL_PCH_DEVICE_ID_MASK		0xff00
>  #define INTEL_PCH_IBX_DEVICE_ID_TYPE		0x3b00


[-- 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 0/6] SNB/BDW runtime PM

[Imre Deak]

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

On Fri, 2014-03-07 at 20:12 -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Hi
> 
> This series depends on:
>  - [PATCH 0/6] More runtime PM fixes
>  - [PATCH 00/16] Merge PC8 with runtime PM, v3
>  - [PATCH 00/20] ILK+ interrupt improvements, v2
> 
> Patches 1 and 2 are the last pieces of my runtime PM rework, and patch 1 needs
> all the previous patches to be merged before it can be merged.
> 
> Patch 3 adds runtime PM support to SNB. It's not widely tested and I haven't
> tested it for a while, but at some point this was all that was needed to make my
> SNB laptop work.
> 
> Patches 4-6 are the BDW bits, but I also haven't tested them since the last
> rebase, and there's the possibility that something is still missing. I wouldn't
> recommend applying this to production trees yet, but I would recommend you to
> apply, test the patches and report the results to me!
> 
> More importantly, the basic reason why I sent this series to the list is that if
> someone wants to add runtime PM support to some other platform (BYT anyone?),

Yea, I'm trying right now to enable this on top of this patchset.

> they should really try to base their patches on top of these. And they should
> really look at the SNB patch to have some idea on what they need to do first.
> 
> Q: Why don't we have IVB runtime PM support?
> A: Just because it was easier for me to do development and test on a SNB
> machine. I really expect that IVB runtime PM will just reuse the SNB bits.
> The only thing missing for IVB is probably just a few "IS_IVYBRIDGE()" calls,
> and a call to modeset_update_crtc_power_domains(). I can provide a patch for
> this later.
> 
> Thanks,
> Paulo
> 
> Paulo Zanoni (6):
>   drm/i915: kill dev_priv->pm.regsave
>   drm/i915: add gen-specific runtime suspend/resume functions
>   drm/i915: add SNB runtime PM support
>   drm/i915: remove HAS_PC8 check
>   drm/i915: BDW needs D_COMP writes through MCHBAR
>   drm/i915: add BDW runtime PM support

These all have now a r-b from me, except the SNB one. That one looks ok
too, but I'd need some clarification on the context save/restore
semantics.

--Imre

>  drivers/gpu/drm/i915/i915_drv.c      | 53 ++++++++++++++++++++----
>  drivers/gpu/drm/i915/i915_drv.h      | 16 ++------
>  drivers/gpu/drm/i915/i915_irq.c      | 79 +++++-------------------------------
>  drivers/gpu/drm/i915/intel_display.c | 43 ++++++++++++--------
>  drivers/gpu/drm/i915/intel_drv.h     |  4 +-
>  5 files changed, 87 insertions(+), 108 deletions(-)
> 


[-- 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 1/6] drm/i915: kill dev_priv->pm.regsave

[Paulo Zanoni]

2014-03-20 9:58 GMT-03:00 Imre Deak <imre.deak@intel.com>:
> On Fri, 2014-03-07 at 20:12 -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> Now that we don't keep the hotplug interrupts enabled anymore, we can
>> kill the regsave struct and just cal the normal IRQ preinstall,
>> postinstall and uninstall functions. This makes it easier to add
>> runtime PM support to non-HSW platforms.
>>
>> The only downside is in case we get a request to update interrupts
>> while they are disabled, won't be able to update the regsave struct.
>> But this should never happen anyway, so we're not losing too much.
>>
>> v2: - Rebase.
>> v3: - Rebase.
>> v4: - Rebase.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h      | 12 +-----
>>  drivers/gpu/drm/i915/i915_irq.c      | 79 +++++-------------------------------
>>  drivers/gpu/drm/i915/intel_display.c |  4 +-
>>  drivers/gpu/drm/i915/intel_drv.h     |  4 +-
>>  4 files changed, 15 insertions(+), 84 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 70feb61..493579d 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1351,23 +1351,13 @@ struct ilk_wm_values {
>>   * goes back to false exactly before we reenable the IRQs. We use this variable
>>   * to check if someone is trying to enable/disable IRQs while they're supposed
>>   * to be disabled. This shouldn't happen and we'll print some error messages in
>> - * case it happens, but if it actually happens we'll also update the variables
>> - * inside struct regsave so when we restore the IRQs they will contain the
>> - * latest expected values.
>> + * case it happens.
>>   *
>>   * For more, read the Documentation/power/runtime_pm.txt.
>>   */
>>  struct i915_runtime_pm {
>>       bool suspended;
>>       bool irqs_disabled;
>> -
>> -     struct {
>> -             uint32_t deimr;
>> -             uint32_t sdeimr;
>> -             uint32_t gtimr;
>> -             uint32_t gtier;
>> -             uint32_t gen6_pmimr;
>> -     } regsave;
>>  };
>>
>>  enum intel_pipe_crc_source {
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index dee3a3b..2aefb94 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -131,11 +131,8 @@ ironlake_enable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
>>  {
>>       assert_spin_locked(&dev_priv->irq_lock);
>>
>> -     if (dev_priv->pm.irqs_disabled) {
>> -             WARN(1, "IRQs disabled\n");
>> -             dev_priv->pm.regsave.deimr &= ~mask;
>> +     if (WARN_ON(dev_priv->pm.irqs_disabled))
>>               return;
>> -     }
>>
>>       if ((dev_priv->irq_mask & mask) != 0) {
>>               dev_priv->irq_mask &= ~mask;
>> @@ -149,11 +146,8 @@ ironlake_disable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
>>  {
>>       assert_spin_locked(&dev_priv->irq_lock);
>>
>> -     if (dev_priv->pm.irqs_disabled) {
>> -             WARN(1, "IRQs disabled\n");
>> -             dev_priv->pm.regsave.deimr |= mask;
>> +     if (WARN_ON(dev_priv->pm.irqs_disabled))
>>               return;
>> -     }
>>
>>       if ((dev_priv->irq_mask & mask) != mask) {
>>               dev_priv->irq_mask |= mask;
>> @@ -174,13 +168,8 @@ static void ilk_update_gt_irq(struct drm_i915_private *dev_priv,
>>  {
>>       assert_spin_locked(&dev_priv->irq_lock);
>>
>> -     if (dev_priv->pm.irqs_disabled) {
>> -             WARN(1, "IRQs disabled\n");
>> -             dev_priv->pm.regsave.gtimr &= ~interrupt_mask;
>> -             dev_priv->pm.regsave.gtimr |= (~enabled_irq_mask &
>> -                                             interrupt_mask);
>> +     if (WARN_ON(dev_priv->pm.irqs_disabled))
>>               return;
>> -     }
>>
>>       dev_priv->gt_irq_mask &= ~interrupt_mask;
>>       dev_priv->gt_irq_mask |= (~enabled_irq_mask & interrupt_mask);
>> @@ -212,13 +201,8 @@ static void snb_update_pm_irq(struct drm_i915_private *dev_priv,
>>
>>       assert_spin_locked(&dev_priv->irq_lock);
>>
>> -     if (dev_priv->pm.irqs_disabled) {
>> -             WARN(1, "IRQs disabled\n");
>> -             dev_priv->pm.regsave.gen6_pmimr &= ~interrupt_mask;
>> -             dev_priv->pm.regsave.gen6_pmimr |= (~enabled_irq_mask &
>> -                                                  interrupt_mask);
>> +     if (WARN_ON(dev_priv->pm.irqs_disabled))
>>               return;
>> -     }
>>
>>       new_val = dev_priv->pm_irq_mask;
>>       new_val &= ~interrupt_mask;
>> @@ -358,14 +342,8 @@ static void ibx_display_interrupt_update(struct drm_i915_private *dev_priv,
>>
>>       assert_spin_locked(&dev_priv->irq_lock);
>>
>> -     if (dev_priv->pm.irqs_disabled &&
>> -         (interrupt_mask & SDE_HOTPLUG_MASK_CPT)) {
>> -             WARN(1, "IRQs disabled\n");
>> -             dev_priv->pm.regsave.sdeimr &= ~interrupt_mask;
>> -             dev_priv->pm.regsave.sdeimr |= (~enabled_irq_mask &
>> -                                              interrupt_mask);
>> +     if (WARN_ON(dev_priv->pm.irqs_disabled))
>>               return;
>> -     }
>>
>>       I915_WRITE(SDEIMR, sdeimr);
>>       POSTING_READ(SDEIMR);
>> @@ -4091,57 +4069,20 @@ void intel_hpd_init(struct drm_device *dev)
>>  }
>>
>>  /* Disable interrupts so we can allow runtime PM. */
>> -void hsw_runtime_pm_disable_interrupts(struct drm_device *dev)
>> +void intel_runtime_pm_disable_interrupts(struct drm_device *dev)
>>  {
>>       struct drm_i915_private *dev_priv = dev->dev_private;
>> -     unsigned long irqflags;
>> -
>> -     spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>> -
>> -     dev_priv->pm.regsave.deimr = I915_READ(DEIMR);
>> -     dev_priv->pm.regsave.sdeimr = I915_READ(SDEIMR);
>> -     dev_priv->pm.regsave.gtimr = I915_READ(GTIMR);
>> -     dev_priv->pm.regsave.gtier = I915_READ(GTIER);
>> -     dev_priv->pm.regsave.gen6_pmimr = I915_READ(GEN6_PMIMR);
>> -
>> -     ironlake_disable_display_irq(dev_priv, 0xffffffff);
>> -     ibx_disable_display_interrupt(dev_priv, 0xffffffff);
>> -     ilk_disable_gt_irq(dev_priv, 0xffffffff);
>> -     snb_disable_pm_irq(dev_priv, 0xffffffff);
>>
>> +     dev->driver->irq_uninstall(dev);
>>       dev_priv->pm.irqs_disabled = true;
>> -
>> -     spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>
> It made me think whether removing the locking here is ok. It seems to be
> ok, as we get here in an idle state where no interrupts should happen. A
> note about this change in the commit message would have been nice.
> Otherwise the patch looks ok:
>
> Reviewed-by: Imre Deak <imre.deak@intel.com>

I wrote this patch many months ago, something in my head was telling
me there was a reason behind the removal, but I can't remember, and I
added it back and it seems to work just fine... I guess I'll keep it
there and continue testing.

>
>
>>  }
>>
>>  /* Restore interrupts so we can recover from runtime PM. */
>> -void hsw_runtime_pm_restore_interrupts(struct drm_device *dev)
>> +void intel_runtime_pm_restore_interrupts(struct drm_device *dev)
>>  {
>>       struct drm_i915_private *dev_priv = dev->dev_private;
>> -     unsigned long irqflags;
>> -     uint32_t val;
>> -
>> -     spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>> -
>> -     val = I915_READ(DEIMR);
>> -     WARN(val != 0xffffffff, "DEIMR is 0x%08x\n", val);
>> -
>> -     val = I915_READ(SDEIMR);
>> -     WARN(val != 0xffffffff, "SDEIMR is 0x%08x\n", val);
>> -
>> -     val = I915_READ(GTIMR);
>> -     WARN(val != 0xffffffff, "GTIMR is 0x%08x\n", val);
>> -
>> -     val = I915_READ(GEN6_PMIMR);
>> -     WARN(val != 0xffffffff, "GEN6_PMIMR is 0x%08x\n", val);
>>
>>       dev_priv->pm.irqs_disabled = false;
>> -
>> -     ironlake_enable_display_irq(dev_priv, ~dev_priv->pm.regsave.deimr);
>> -     ibx_enable_display_interrupt(dev_priv, ~dev_priv->pm.regsave.sdeimr);
>> -     ilk_enable_gt_irq(dev_priv, ~dev_priv->pm.regsave.gtimr);
>> -     snb_enable_pm_irq(dev_priv, ~dev_priv->pm.regsave.gen6_pmimr);
>> -     I915_WRITE(GTIER, dev_priv->pm.regsave.gtier);
>> -
>> -     spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>> +     dev->driver->irq_preinstall(dev);
>> +     dev->driver->irq_postinstall(dev);
>>  }
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index ed9233e..df69866 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -6829,7 +6829,7 @@ void hsw_enable_pc8(struct drm_i915_private *dev_priv)
>>       }
>>
>>       lpt_disable_clkout_dp(dev);
>> -     hsw_runtime_pm_disable_interrupts(dev);
>> +     intel_runtime_pm_disable_interrupts(dev);
>>       hsw_disable_lcpll(dev_priv, true, true);
>>  }
>>
>> @@ -6843,7 +6843,7 @@ void hsw_disable_pc8(struct drm_i915_private *dev_priv)
>>       DRM_DEBUG_KMS("Disabling package C8+\n");
>>
>>       hsw_restore_lcpll(dev_priv);
>> -     hsw_runtime_pm_restore_interrupts(dev);
>> +     intel_runtime_pm_restore_interrupts(dev);
>>       lpt_init_pch_refclk(dev);
>>
>>       if (dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 9a7db84..0dfb6e9 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -618,8 +618,8 @@ void ilk_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
>>  void ilk_disable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
>>  void snb_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
>>  void snb_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
>> -void hsw_runtime_pm_disable_interrupts(struct drm_device *dev);
>> -void hsw_runtime_pm_restore_interrupts(struct drm_device *dev);
>> +void intel_runtime_pm_disable_interrupts(struct drm_device *dev);
>> +void intel_runtime_pm_restore_interrupts(struct drm_device *dev);
>>
>>
>>  /* intel_crt.c */
>



-- 
Paulo Zanoni

Re: [PATCH 3/6] drm/i915: add SNB runtime PM support

[Paulo Zanoni]

2014-04-01 12:34 GMT-03:00 Imre Deak <imre.deak@intel.com>:
> On Fri, 2014-03-07 at 20:12 -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> Just because I have a SNB machine and I can easily test it.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.c      | 27 +++++++++++++++++++++++++--
>>  drivers/gpu/drm/i915/i915_drv.h      |  2 +-
>>  drivers/gpu/drm/i915/intel_display.c |  7 +++++++
>>  3 files changed, 33 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index f75d776..2c62e0c 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -832,6 +832,13 @@ static int i915_pm_poweroff(struct device *dev)
>>       return i915_drm_freeze(drm_dev);
>>  }
>>
>> +static void snb_runtime_suspend(struct drm_i915_private *dev_priv)
>> +{
>> +     struct drm_device *dev = dev_priv->dev;
>> +
>> +     intel_runtime_pm_disable_interrupts(dev);
>> +}
>
> Afaics RPS is still enabled here, so if we need it enabled at this point
> (so that GT stays at RC6 for example) we'd at least need here
>
> cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work);
> cancel_work_sync(&dev_priv->rps.work);

Congratulations, you've found the only runtime PM bug I can still
reproduce on my local tree. But this problem also happens for HSW, not
just SNB, so IMHO its fix should go on a separate patch. And it should
go to -fixes.

>
>> +
>>  static void hsw_runtime_suspend(struct drm_i915_private *dev_priv)
>>  {
>>       struct drm_device *dev = dev_priv->dev;
>> @@ -840,6 +847,18 @@ static void hsw_runtime_suspend(struct drm_i915_private *dev_priv)
>>               hsw_enable_pc8(dev_priv);
>>  }
>>
>> +static void snb_runtime_resume(struct drm_i915_private *dev_priv)
>> +{
>> +     struct drm_device *dev = dev_priv->dev;
>> +
>> +     intel_runtime_pm_restore_interrupts(dev);
>> +     intel_init_pch_refclk(dev);
>> +     i915_gem_init_swizzling(dev);
>> +     mutex_lock(&dev_priv->rps.hw_lock);
>> +     gen6_update_ring_freq(dev);
>> +     mutex_unlock(&dev_priv->rps.hw_lock);
>> +}
>
> I haven't found any info on what exact state is lost in D3, but in any
> case documenting this in the code would be great.

Unfortunately we don't have this documentation, and that's why we have
so many different tests on the pm_pc8 test suite. This function is
just based on the HSW code, and I am kinda trusting the test suite and
my manual tests.

>
> Here there are some bits restored for GTI in i915_gem_init_swizzling(),
> but what about the clock gating registers and RPS registers. I'd be
> easier if those would be also re-inited here, or a confirmation that
> they are saved/restored by HW.

The clock gating registers were manually checked a looong time ago on
HSW, and I didn't really remember to check them on SNB. We even
discussed on the mailing list how to do this check, and I proposed an
implementation a long time ago:
http://lists.freedesktop.org/archives/intel-gfx/2013-November/036351.html
, but I never wrote this code.

Also, if you have ideas of IGT tests that could catch these possible
problems you mentioned, please feel free to contribute! Either by
writing code or by giving me a description of the code to be written
:)

I'll move this SNB patch to the end of the series.

Thanks,
Paulo

>
> --Imre
>
>> +
>>  static void hsw_runtime_resume(struct drm_i915_private *dev_priv)
>>  {
>>       struct drm_device *dev = dev_priv->dev;
>> @@ -859,7 +878,9 @@ static int intel_runtime_suspend(struct device *device)
>>
>>       DRM_DEBUG_KMS("Suspending device\n");
>>
>> -     if (IS_HASWELL(dev))
>> +     if (IS_GEN6(dev))
>> +             snb_runtime_suspend(dev_priv);
>> +     else if (IS_HASWELL(dev))
>>               hsw_runtime_suspend(dev_priv);
>>
>>       i915_gem_release_all_mmaps(dev_priv);
>> @@ -893,7 +914,9 @@ static int intel_runtime_resume(struct device *device)
>>       intel_opregion_notify_adapter(dev, PCI_D0);
>>       dev_priv->pm.suspended = false;
>>
>> -     if (IS_HASWELL(dev))
>> +     if (IS_GEN6(dev))
>> +             snb_runtime_resume(dev_priv);
>> +     else if (IS_HASWELL(dev))
>>               hsw_runtime_resume(dev_priv);
>>
>>       DRM_DEBUG_KMS("Device resumed\n");
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 493579d..309852d 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1998,7 +1998,7 @@ struct drm_i915_cmd_table {
>>  #define HAS_FPGA_DBG_UNCLAIMED(dev)  (INTEL_INFO(dev)->has_fpga_dbg)
>>  #define HAS_PSR(dev)         (IS_HASWELL(dev) || IS_BROADWELL(dev))
>>  #define HAS_PC8(dev)         (IS_HASWELL(dev)) /* XXX HSW:ULX */
>> -#define HAS_RUNTIME_PM(dev)  (IS_HASWELL(dev))
>> +#define HAS_RUNTIME_PM(dev)  (IS_GEN6(dev) || IS_HASWELL(dev))
>>
>>  #define INTEL_PCH_DEVICE_ID_MASK             0xff00
>>  #define INTEL_PCH_IBX_DEVICE_ID_TYPE         0x3b00
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index df69866..35f65c1 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -6859,6 +6859,11 @@ void hsw_disable_pc8(struct drm_i915_private *dev_priv)
>>       mutex_unlock(&dev_priv->rps.hw_lock);
>>  }
>>
>> +static void snb_modeset_global_resources(struct drm_device *dev)
>> +{
>> +     modeset_update_crtc_power_domains(dev);
>> +}
>> +
>>  static void haswell_modeset_global_resources(struct drm_device *dev)
>>  {
>>       modeset_update_crtc_power_domains(dev);
>> @@ -10758,6 +10763,8 @@ static void intel_init_display(struct drm_device *dev)
>>               } else if (IS_GEN6(dev)) {
>>                       dev_priv->display.fdi_link_train = gen6_fdi_link_train;
>>                       dev_priv->display.write_eld = ironlake_write_eld;
>> +                     dev_priv->display.modeset_global_resources =
>> +                             snb_modeset_global_resources;
>>               } else if (IS_IVYBRIDGE(dev)) {
>>                       /* FIXME: detect B0+ stepping and use auto training */
>>                       dev_priv->display.fdi_link_train = ivb_manual_fdi_link_train;
>



-- 
Paulo Zanoni

Re: [PATCH 6/6] drm/i915: add BDW runtime PM support

[Daniel Vetter]

On Tue, Apr 01, 2014 at 06:51:39PM +0300, Imre Deak wrote:
> On Fri, 2014-03-07 at 20:12 -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > This sould be enough.
> > 
> > v2: BDW should also run hsw_runtime_resume (Ben).
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Looks good,
> Reviewed-by: Imre Deak <imre.deak@intel.com>

Pulled in entire series. I'm not really happy about the split in what we
restore between snb and hsw/bdw and the next guy who adds a platform hook
here gets to refactor it all.

It /should/ all be driven by common code like the generic resume/driver
load code. If we have subtile differences here it'll be a lot of fun with
random oopses.
-Daniel

> 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 8 ++++++--
> >  drivers/gpu/drm/i915/i915_drv.h | 3 ++-
> >  2 files changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 55f0181..2dcbbc0 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -874,8 +874,10 @@ static int intel_runtime_suspend(struct device *device)
> >  
> >  	if (IS_GEN6(dev))
> >  		snb_runtime_suspend(dev_priv);
> > -	else if (IS_HASWELL(dev))
> > +	else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> >  		hsw_runtime_suspend(dev_priv);
> > +	else
> > +		WARN_ON(1);
> >  
> >  	i915_gem_release_all_mmaps(dev_priv);
> >  
> > @@ -910,8 +912,10 @@ static int intel_runtime_resume(struct device *device)
> >  
> >  	if (IS_GEN6(dev))
> >  		snb_runtime_resume(dev_priv);
> > -	else if (IS_HASWELL(dev))
> > +	else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> >  		hsw_runtime_resume(dev_priv);
> > +	else
> > +		WARN_ON(1);
> >  
> >  	DRM_DEBUG_KMS("Device resumed\n");
> >  	return 0;
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 1debc412..2f7246a 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1997,7 +1997,8 @@ struct drm_i915_cmd_table {
> >  #define HAS_DDI(dev)		(INTEL_INFO(dev)->has_ddi)
> >  #define HAS_FPGA_DBG_UNCLAIMED(dev)	(INTEL_INFO(dev)->has_fpga_dbg)
> >  #define HAS_PSR(dev)		(IS_HASWELL(dev) || IS_BROADWELL(dev))
> > -#define HAS_RUNTIME_PM(dev)	(IS_GEN6(dev) || IS_HASWELL(dev))
> > +#define HAS_RUNTIME_PM(dev)	(IS_GEN6(dev) || IS_HASWELL(dev) || \
> > +				 IS_BROADWELL(dev))
> >  
> >  #define INTEL_PCH_DEVICE_ID_MASK		0xff00
> >  #define INTEL_PCH_IBX_DEVICE_ID_TYPE		0x3b00
> 



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


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