SNB/BDW runtime PM

From: Paulo Zanoni <[email protected]>

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 <[email protected]>

---
 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(-)

drivers/gpu/drm/i915/i915_drv.h +1 -11

index 70feb61..493579d 100644
@@ -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 {

drivers/gpu/drm/i915/i915_irq.c +6 -37

index dee3a3b..2aefb94 100644
@@ -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);

drivers/gpu/drm/i915/i915_irq.c +3 -25

 }
 
 /* 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);
 }

drivers/gpu/drm/i915/intel_display.c +2 -2

index ed9233e..df69866 100644
@@ -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) {

drivers/gpu/drm/i915/intel_drv.h +0 -0

index 9a7db84..0dfb6e9 100644

Quoted 145 lines

From: Paulo Zanoni <[email protected]>

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 <[email protected]>

---
 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(-)

drivers/gpu/drm/i915/i915_drv.h +1 -11

index 70feb61..493579d 100644
@@ -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 {

drivers/gpu/drm/i915/i915_irq.c +6 -37

index dee3a3b..2aefb94 100644
@@ -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);

drivers/gpu/drm/i915/i915_irq.c void intel_hpd_init(struct drm_device *dev) · line 4114

Quoted 10 lines

drivers/gpu/drm/i915/i915_irq.c +1 -7

-
-     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 <[email protected]>

Quoted 75 lines

drivers/gpu/drm/i915/i915_irq.c +3 -25

 }
 
 /* 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);
 }

drivers/gpu/drm/i915/intel_display.c +2 -2

index ed9233e..df69866 100644
@@ -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) {

drivers/gpu/drm/i915/intel_drv.h +2 -2

index 9a7db84..0dfb6e9 100644
@@ -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 */

From: Paulo Zanoni <[email protected]>

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 <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

drivers/gpu/drm/i915/i915_drv.c +22 -6

index 74f7a91..f75d776 100644
@@ -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;

From: Paulo Zanoni <[email protected]>

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

Signed-off-by Paulo Zanoni <[email protected]>

---
 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(-)

drivers/gpu/drm/i915/i915_drv.c +0 -0

index f75d776..2c62e0c 100644

drivers/gpu/drm/i915/i915_drv.c +2 -0

+
 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)

drivers/gpu/drm/i915/i915_drv.c +7 -2

+
 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");

drivers/gpu/drm/i915/i915_drv.h +1 -1

index 493579d..309852d 100644
@@ -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

drivers/gpu/drm/i915/intel_display.c +5 -0

index df69866..35f65c1 100644
@@ -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);

Quoted 16 lines

From: Paulo Zanoni <[email protected]>

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

Signed-off-by Paulo Zanoni <[email protected]>

---
 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(-)

drivers/gpu/drm/i915/i915_drv.c +0 -0

index f75d776..2c62e0c 100644

drivers/gpu/drm/i915/i915_drv.c static int i915_pm_poweroff(struct device *dev) · line 840

Quoted 9 lines

drivers/gpu/drm/i915/i915_drv.c +6 -0

@@ -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);

Quoted 9 lines

drivers/gpu/drm/i915/i915_drv.c +1 -0

+
 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);
 }
 

drivers/gpu/drm/i915/i915_drv.c static void hsw_runtime_suspend(struct drm_i915_private *dev_priv) · line 860

Quoted 11 lines

drivers/gpu/drm/i915/i915_drv.c +11 -0

+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

Quoted 64 lines

drivers/gpu/drm/i915/i915_drv.c +7 -2

+
 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");

drivers/gpu/drm/i915/i915_drv.h +1 -1

index 493579d..309852d 100644
@@ -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

drivers/gpu/drm/i915/intel_display.c +7 -0

index df69866..35f65c1 100644
@@ -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;

From: Paulo Zanoni <[email protected]>

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

Signed-off-by Paulo Zanoni <[email protected]>

---
 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(-)

drivers/gpu/drm/i915/i915_drv.c +2 -8

index 2c62e0c..55f0181 100644
@@ -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)

drivers/gpu/drm/i915/i915_drv.h +0 -1

index 309852d..1debc412 100644
@@ -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

drivers/gpu/drm/i915/intel_display.c +0 -2

index 35f65c1..d6092be 100644
@@ -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) {

From: Paulo Zanoni <[email protected]>

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 <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

drivers/gpu/drm/i915/intel_display.c +17 -5

index d6092be..2be4129 100644
@@ -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;

From: Paulo Zanoni <[email protected]>

This sould be enough.

v2: BDW should also run hsw_runtime_resume (Ben).

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.c | 8 ++++++--
 drivers/gpu/drm/i915/i915_drv.h | 3 ++-
 2 files changed, 8 insertions(+), 3 deletions(-)

drivers/gpu/drm/i915/i915_drv.c +6 -2

index 55f0181..2dcbbc0 100644
@@ -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;

drivers/gpu/drm/i915/i915_drv.h +0 -0

index 1debc412..2f7246a 100644

Quoted 7 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

This sould be enough.

v2: BDW should also run hsw_runtime_resume (Ben).

Signed-off-by Paulo Zanoni <[email protected]>

Looks good,

Reviewed-by Imre Deak <[email protected]>

Quoted 39 lines

---
 drivers/gpu/drm/i915/i915_drv.c | 8 ++++++--
 drivers/gpu/drm/i915/i915_drv.h | 3 ++-
 2 files changed, 8 insertions(+), 3 deletions(-)

drivers/gpu/drm/i915/i915_drv.c +6 -2

index 55f0181..2dcbbc0 100644
@@ -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;

drivers/gpu/drm/i915/i915_drv.h +0 -0

index 1debc412..2f7246a 100644

drivers/gpu/drm/i915/i915_drv.h struct drm_i915_cmd_table { · line 2004

Quoted 9 lines

drivers/gpu/drm/i915/i915_drv.h +2 -1

@@ -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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

From: Paulo Zanoni <[email protected]>

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(-)