[PATCH 0/6] Final PC8+ patches

[PATCH 0/6] Final PC8+ patches

[Paulo Zanoni]

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

Hi

>From my previous series, patches 1 to 6.3 already got Reviewed-by stamps, and
patches 7+ got some comments. So this series replaces patches 7+ of the previous
series: it's on top of -nightly + patches 1 to 6.3 from the previous series.

There's nothing much new besides some bug fixing and an additional module option
to allow some tuning on how often we reach PC8+.

If you want to test everything, please check the c8-wip-20130819 branch of my
Kernel tree: http://cgit.freedesktop.org/~pzanoni/linux/?h=c8-wip-20130819 .

Thanks,
Paulo

Paulo Zanoni (6):
  drm/i915: grab force_wake when restoring LCPLL
  drm/i915: fix SDEIMR assertion when disabling LCPLL
  drm/i915: allow package C8+ states on Haswell (disabled)
  drm/i915: add i915_pc8_status debugfs file
  drm/i915: add i915.pc8_timeout function
  drm/i915: enable Package C8+ by default

 drivers/gpu/drm/i915/i915_debugfs.c  |  25 +++++
 drivers/gpu/drm/i915/i915_dma.c      |  10 ++
 drivers/gpu/drm/i915/i915_drv.c      |  15 +++
 drivers/gpu/drm/i915/i915_drv.h      |  73 ++++++++++++++
 drivers/gpu/drm/i915/i915_gem.c      |   2 +
 drivers/gpu/drm/i915/i915_irq.c      | 101 +++++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c | 184 +++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_dp.c      |   3 +
 drivers/gpu/drm/i915/intel_drv.h     |   8 ++
 drivers/gpu/drm/i915/intel_i2c.c     |   2 +
 drivers/gpu/drm/i915/intel_pm.c      |  13 ++-
 11 files changed, 428 insertions(+), 8 deletions(-)

-- 
1.8.1.2

[PATCH 1/6] drm/i915: grab force_wake when restoring LCPLL

[Paulo Zanoni]

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

If LCPLL is disabled, there's a chance we might be in package C8 state
or deeper, and we'll get a hard hang when restoring LCPLL (also, a red
led lights up on my motherboard). So grab the force_wake, which will
get us out of RC6 and, as a consequence, out of PC8+ (since we need
RC6 to get into PC8+).

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 00114a5..8c63d8e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6031,6 +6031,10 @@ void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
 		    LCPLL_POWER_DOWN_ALLOW)) == LCPLL_PLL_LOCK)
 		return;
 
+	/* Make sure we're not on PC8 state before disabling PC8, otherwise
+	 * we'll hang the machine! */
+	dev_priv->uncore.funcs.force_wake_get(dev_priv);
+
 	if (val & LCPLL_POWER_DOWN_ALLOW) {
 		val &= ~LCPLL_POWER_DOWN_ALLOW;
 		I915_WRITE(LCPLL_CTL, val);
@@ -6058,6 +6062,8 @@ void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
 					LCPLL_CD_SOURCE_FCLK_DONE) == 0, 1))
 			DRM_ERROR("Switching back to LCPLL failed\n");
 	}
+
+	dev_priv->uncore.funcs.force_wake_put(dev_priv);
 }
 
 static void haswell_modeset_global_resources(struct drm_device *dev)
-- 
1.8.1.2

[PATCH 2/6] drm/i915: fix SDEIMR assertion when disabling LCPLL

[Paulo Zanoni]

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

This was causing WARNs in one machine, so instead of trying to guess
exactly which hotplug bits should exist, just do the test on the
non-HPD bits. We don't care about the state of the hotplug bits, we
just care about the others, that need to be 1.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8c63d8e..b9a4d8b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5930,11 +5930,7 @@ static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv)
 	struct intel_ddi_plls *plls = &dev_priv->ddi_plls;
 	struct intel_crtc *crtc;
 	unsigned long irqflags;
-	uint32_t val, pch_hpd_mask;
-
-	pch_hpd_mask = SDE_PORTB_HOTPLUG_CPT | SDE_PORTC_HOTPLUG_CPT;
-	if (!(dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE))
-		pch_hpd_mask |= SDE_PORTD_HOTPLUG_CPT | SDE_CRT_HOTPLUG_CPT;
+	uint32_t val;
 
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head)
 		WARN(crtc->base.enabled, "CRTC for pipe %c enabled\n",
@@ -5960,7 +5956,7 @@ static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv)
 	WARN((val & ~DE_PCH_EVENT_IVB) != val,
 	     "Unexpected DEIMR bits enabled: 0x%x\n", val);
 	val = I915_READ(SDEIMR);
-	WARN((val & ~pch_hpd_mask) != val,
+	WARN((val | SDE_HOTPLUG_MASK_CPT) != 0xffffffff,
 	     "Unexpected SDEIMR bits enabled: 0x%x\n", val);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 }
-- 
1.8.1.2

[PATCH 3/6] drm/i915: allow package C8+ states on Haswell (disabled)

[Paulo Zanoni]

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

This patch allows PC8+ states on Haswell. These states can only be
reached when all the display outputs are disabled, and they allow some
more power savings.

The fact that the graphics device is allowing PC8+ doesn't mean that
the machine will actually enter PC8+: all the other devices also need
to allow PC8+.

For now this option is disabled by default. You need i915.allow_pc8=1
if you want it.

This patch adds a big comment inside i915_drv.h explaining how it
works and how it tracks things. Read it.

v2: (this is not really v2, many previous versions were already sent,
     but they had different names)
    - Use the new functions to enable/disable GTIMR and GEN6_PMIMR
    - Rename almost all variables and functions to names suggested by
      Chris
    - More WARNs on the IRQ handling code
    - Also disable PC8 when there's GPU work to do (thanks to Ben for
      the help on this), so apps can run caster
    - Enable PC8 on a delayed work function that is delayed for 5
      seconds. This makes sure we only enable PC8+ if we're really
      idle
    - Make sure we're not in PC8+ when suspending
v3: - WARN if IRQs are disabled on __wait_seqno
    - Replace some DRM_ERRORs with WARNs
    - Fix calls to restore GT and PM interrupts
    - Use intel_mark_busy instead of intel_ring_advance to disable PC8
v4: - Use the force_wake, Luke!
v5: - Remove the "IIR is not zero" WARNs
    - Move the force_wake chunk to its own patch
    - Only restore what's missing from RC6, not everything

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c      |  10 +++
 drivers/gpu/drm/i915/i915_drv.c      |  11 +++
 drivers/gpu/drm/i915/i915_drv.h      |  72 +++++++++++++++
 drivers/gpu/drm/i915/i915_gem.c      |   2 +
 drivers/gpu/drm/i915/i915_irq.c      | 101 +++++++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c | 170 ++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_dp.c      |   3 +
 drivers/gpu/drm/i915/intel_drv.h     |   8 ++
 drivers/gpu/drm/i915/intel_i2c.c     |   2 +
 drivers/gpu/drm/i915/intel_pm.c      |  13 ++-
 10 files changed, 390 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index d2dc02b..4c7669f 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1486,6 +1486,14 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	mutex_init(&dev_priv->rps.hw_lock);
 	mutex_init(&dev_priv->modeset_restore_lock);
 
+	mutex_init(&dev_priv->pc8.lock);
+	dev_priv->pc8.requirements_met = false;
+	dev_priv->pc8.gpu_idle = false;
+	dev_priv->pc8.irqs_disabled = false;
+	dev_priv->pc8.enabled = false;
+	dev_priv->pc8.disable_count = 2; /* requirements_met + gpu_idle */
+	INIT_DELAYED_WORK(&dev_priv->pc8.enable_work, hsw_enable_pc8_work);
+
 	i915_dump_device_info(dev_priv);
 
 	/* Not all pre-production machines fall into this category, only the
@@ -1740,6 +1748,8 @@ int i915_driver_unload(struct drm_device *dev)
 	cancel_work_sync(&dev_priv->gpu_error.work);
 	i915_destroy_error_state(dev);
 
+	cancel_delayed_work_sync(&dev_priv->pc8.enable_work);
+
 	if (dev->pdev->msi_enabled)
 		pci_disable_msi(dev->pdev);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index a9c8f18..f197362 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -141,6 +141,10 @@ module_param_named(fastboot, i915_fastboot, bool, 0600);
 MODULE_PARM_DESC(fastboot, "Try to skip unnecessary mode sets at boot time "
 		 "(default: false)");
 
+int i915_enable_pc8 __read_mostly = 0;
+module_param_named(enable_pc8, i915_enable_pc8, int, 0600);
+MODULE_PARM_DESC(enable_pc8, "Enable support for low power package C states (PC8+) (default: false)");
+
 bool i915_prefault_disable __read_mostly;
 module_param_named(prefault_disable, i915_prefault_disable, bool, 0600);
 MODULE_PARM_DESC(prefault_disable,
@@ -557,6 +561,9 @@ static int i915_drm_freeze(struct drm_device *dev)
 	dev_priv->modeset_restore = MODESET_SUSPENDED;
 	mutex_unlock(&dev_priv->modeset_restore_lock);
 
+	/* We do a lot of poking in a lot of registers, make sure they work
+	 * properly. */
+	hsw_disable_package_c8(dev_priv);
 	intel_set_power_well(dev, true);
 
 	drm_kms_helper_poll_disable(dev);
@@ -713,6 +720,10 @@ static int __i915_drm_thaw(struct drm_device *dev)
 		schedule_work(&dev_priv->console_resume_work);
 	}
 
+	/* Undo what we did at i915_drm_freeze so the refcount goes back to the
+	 * expected level. */
+	hsw_enable_package_c8(dev_priv);
+
 	mutex_lock(&dev_priv->modeset_restore_lock);
 	dev_priv->modeset_restore = MODESET_DONE;
 	mutex_unlock(&dev_priv->modeset_restore_lock);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b9a4499..34c5af5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1083,6 +1083,75 @@ struct intel_wm_level {
 	uint32_t fbc_val;
 };
 
+/*
+ * This struct tracks the state needed for the Package C8+ feature.
+ *
+ * Package states C8 and deeper are really deep PC states that can only be
+ * reached when all the devices on the system allow it, so even if the graphics
+ * device allows PC8+, it doesn't mean the system will actually get to these
+ * states.
+ *
+ * Our driver only allows PC8+ when all the outputs are disabled, the power well
+ * is disabled and the GPU is idle. When these conditions are met, we manually
+ * do the other conditions: disable the interrupts, clocks and switch LCPLL
+ * refclk to Fclk.
+ *
+ * When we really reach PC8 or deeper states (not just when we allow it) we lose
+ * the state of some registers, so when we come back from PC8+ we need to
+ * restore this state. We don't get into PC8+ if we're not in RC6, so we don't
+ * need to take care of the registers kept by RC6.
+ *
+ * The interrupt disabling is part of the requirements. We can only leave the
+ * PCH HPD interrupts enabled. If we're in PC8+ and we get another interrupt we
+ * can lock the machine.
+ *
+ * Ideally every piece of our code that needs PC8+ disabled would call
+ * hsw_disable_package_c8, which would increment disable_count and prevent the
+ * system from reaching PC8+. But we don't have a symmetric way to do this for
+ * everything, so we have the requirements_met and gpu_idle variables. When we
+ * switch requirements_met or gpu_idle to true we decrease disable_count, and
+ * increase it in the opposite case. The requirements_met variable is true when
+ * all the CRTCs, encoders and the power well are disabled. The gpu_idle
+ * variable is true when the GPU is idle.
+ *
+ * In addition to everything, we only actually enable PC8+ if disable_count
+ * stays at zero for at least some seconds. This is implemented with the
+ * enable_work variable. We do this so we don't enable/disable PC8 dozens of
+ * consecutive times when all screens are disabled and some background app
+ * queries the state of our connectors, or we have some application constantly
+ * waking up to use the GPU. Only after the enable_work function actually
+ * enables PC8+ the "enable" variable will become true, which means that it can
+ * be false even if disable_count is 0.
+ *
+ * The irqs_disabled variable becomes true exactly after we disable the IRQs and
+ * 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.
+ *
+ * For more, read "Display Sequences for Package C8" on our documentation.
+ */
+struct i915_package_c8 {
+	bool requirements_met;
+	bool gpu_idle;
+	bool irqs_disabled;
+	/* Only true after the delayed work task actually enables it. */
+	bool enabled;
+	int disable_count;
+	struct mutex lock;
+	struct delayed_work enable_work;
+
+	struct {
+		uint32_t deimr;
+		uint32_t sdeimr;
+		uint32_t gtimr;
+		uint32_t gtier;
+		uint32_t gen6_pmimr;
+	} regsave;
+};
+
 typedef struct drm_i915_private {
 	struct drm_device *dev;
 	struct kmem_cache *slab;
@@ -1267,6 +1336,8 @@ typedef struct drm_i915_private {
 		uint16_t cur_latency[5];
 	} wm;
 
+	struct i915_package_c8 pc8;
+
 	/* Old dri1 support infrastructure, beware the dragons ya fools entering
 	 * here! */
 	struct i915_dri1_state dri1;
@@ -1638,6 +1709,7 @@ extern unsigned int i915_preliminary_hw_support __read_mostly;
 extern int i915_disable_power_well __read_mostly;
 extern int i915_enable_ips __read_mostly;
 extern bool i915_fastboot __read_mostly;
+extern int i915_enable_pc8 __read_mostly;
 extern bool i915_prefault_disable __read_mostly;
 
 extern int i915_suspend(struct drm_device *dev, pm_message_t state);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c7efb60..f0e0962 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -999,6 +999,8 @@ static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno,
 	bool wait_forever = true;
 	int ret;
 
+	WARN(dev_priv->pc8.irqs_disabled, "IRQs disabled\n");
+
 	if (i915_seqno_passed(ring->get_seqno(ring, true), seqno))
 		return 0;
 
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index caf83da..a03b445 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -85,6 +85,12 @@ ironlake_enable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
 {
 	assert_spin_locked(&dev_priv->irq_lock);
 
+	if (dev_priv->pc8.irqs_disabled) {
+		WARN(1, "IRQs disabled\n");
+		dev_priv->pc8.regsave.deimr &= ~mask;
+		return;
+	}
+
 	if ((dev_priv->irq_mask & mask) != 0) {
 		dev_priv->irq_mask &= ~mask;
 		I915_WRITE(DEIMR, dev_priv->irq_mask);
@@ -97,6 +103,12 @@ ironlake_disable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
 {
 	assert_spin_locked(&dev_priv->irq_lock);
 
+	if (dev_priv->pc8.irqs_disabled) {
+		WARN(1, "IRQs disabled\n");
+		dev_priv->pc8.regsave.deimr |= mask;
+		return;
+	}
+
 	if ((dev_priv->irq_mask & mask) != mask) {
 		dev_priv->irq_mask |= mask;
 		I915_WRITE(DEIMR, dev_priv->irq_mask);
@@ -116,6 +128,14 @@ static void ilk_update_gt_irq(struct drm_i915_private *dev_priv,
 {
 	assert_spin_locked(&dev_priv->irq_lock);
 
+	if (dev_priv->pc8.irqs_disabled) {
+		WARN(1, "IRQs disabled\n");
+		dev_priv->pc8.regsave.gtimr &= ~interrupt_mask;
+		dev_priv->pc8.regsave.gtimr |= (~enabled_irq_mask &
+						interrupt_mask);
+		return;
+	}
+
 	dev_priv->gt_irq_mask &= ~interrupt_mask;
 	dev_priv->gt_irq_mask |= (~enabled_irq_mask & interrupt_mask);
 	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
@@ -146,6 +166,14 @@ static void snb_update_pm_irq(struct drm_i915_private *dev_priv,
 
 	assert_spin_locked(&dev_priv->irq_lock);
 
+	if (dev_priv->pc8.irqs_disabled) {
+		WARN(1, "IRQs disabled\n");
+		dev_priv->pc8.regsave.gen6_pmimr &= ~interrupt_mask;
+		dev_priv->pc8.regsave.gen6_pmimr |= (~enabled_irq_mask &
+						     interrupt_mask);
+		return;
+	}
+
 	new_val = dev_priv->pm_irq_mask;
 	new_val &= ~interrupt_mask;
 	new_val |= (~enabled_irq_mask & interrupt_mask);
@@ -257,6 +285,15 @@ static void ibx_display_interrupt_update(struct drm_i915_private *dev_priv,
 
 	assert_spin_locked(&dev_priv->irq_lock);
 
+	if (dev_priv->pc8.irqs_disabled &&
+	    (interrupt_mask & SDE_HOTPLUG_MASK_CPT)) {
+		WARN(1, "IRQs disabled\n");
+		dev_priv->pc8.regsave.sdeimr &= ~interrupt_mask;
+		dev_priv->pc8.regsave.sdeimr |= (~enabled_irq_mask &
+						 interrupt_mask);
+		return;
+	}
+
 	I915_WRITE(SDEIMR, sdeimr);
 	POSTING_READ(SDEIMR);
 }
@@ -3113,3 +3150,67 @@ void intel_hpd_init(struct drm_device *dev)
 		dev_priv->display.hpd_irq_setup(dev);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 }
+
+/* Disable interrupts so we can allow Package C8+. */
+void hsw_pc8_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->pc8.regsave.deimr = I915_READ(DEIMR);
+	dev_priv->pc8.regsave.sdeimr = I915_READ(SDEIMR);
+	dev_priv->pc8.regsave.gtimr = I915_READ(GTIMR);
+	dev_priv->pc8.regsave.gtier = I915_READ(GTIER);
+	dev_priv->pc8.regsave.gen6_pmimr = I915_READ(GEN6_PMIMR);
+
+	ironlake_disable_display_irq(dev_priv, ~DE_PCH_EVENT_IVB);
+	ibx_disable_display_interrupt(dev_priv, ~SDE_HOTPLUG_MASK_CPT);
+	ilk_disable_gt_irq(dev_priv, 0xffffffff);
+	snb_disable_pm_irq(dev_priv, 0xffffffff);
+
+	dev_priv->pc8.irqs_disabled = true;
+
+	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+}
+
+/* Restore interrupts so we can recover from Package C8+. */
+void hsw_pc8_restore_interrupts(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	unsigned long irqflags;
+	uint32_t val, expected;
+
+	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
+
+	val = I915_READ(DEIMR);
+	expected = ~DE_PCH_EVENT_IVB;
+	WARN(val != expected, "DEIMR is 0x%08x, not 0x%08x\n", val, expected);
+
+	val = I915_READ(SDEIMR) & ~SDE_HOTPLUG_MASK_CPT;
+	expected = ~SDE_HOTPLUG_MASK_CPT;
+	WARN(val != expected, "SDEIMR non-HPD bits are 0x%08x, not 0x%08x\n",
+	     val, expected);
+
+	val = I915_READ(GTIMR);
+	expected = 0xffffffff;
+	WARN(val != expected, "GTIMR is 0x%08x, not 0x%08x\n", val, expected);
+
+	val = I915_READ(GEN6_PMIMR);
+	expected = 0xffffffff;
+	WARN(val != expected, "GEN6_PMIMR is 0x%08x, not 0x%08x\n", val,
+	     expected);
+
+	dev_priv->pc8.irqs_disabled = false;
+
+	ironlake_enable_display_irq(dev_priv, ~dev_priv->pc8.regsave.deimr);
+	ibx_enable_display_interrupt(dev_priv,
+				     ~dev_priv->pc8.regsave.sdeimr &
+				     ~SDE_HOTPLUG_MASK_CPT);
+	ilk_enable_gt_irq(dev_priv, ~dev_priv->pc8.regsave.gtimr);
+	snb_enable_pm_irq(dev_priv, ~dev_priv->pc8.regsave.gen6_pmimr);
+	I915_WRITE(GTIER, dev_priv->pc8.regsave.gtier);
+
+	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b9a4d8b..7d5b01a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6062,6 +6062,166 @@ void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
 	dev_priv->uncore.funcs.force_wake_put(dev_priv);
 }
 
+void hsw_enable_pc8_work(struct work_struct *__work)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(to_delayed_work(__work), struct drm_i915_private,
+			     pc8.enable_work);
+	struct drm_device *dev = dev_priv->dev;
+	uint32_t val;
+
+	if (dev_priv->pc8.enabled)
+		return;
+
+	DRM_DEBUG_KMS("Enabling package C8+\n");
+
+	dev_priv->pc8.enabled = true;
+
+	if (dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
+		val = I915_READ(SOUTH_DSPCLK_GATE_D);
+		val &= ~PCH_LP_PARTITION_LEVEL_DISABLE;
+		I915_WRITE(SOUTH_DSPCLK_GATE_D, val);
+	}
+
+	lpt_disable_clkout_dp(dev);
+	hsw_pc8_disable_interrupts(dev);
+	hsw_disable_lcpll(dev_priv, true, true);
+}
+
+static void __hsw_enable_package_c8(struct drm_i915_private *dev_priv)
+{
+	WARN_ON(!mutex_is_locked(&dev_priv->pc8.lock));
+	WARN(dev_priv->pc8.disable_count < 1,
+	     "pc8.disable_count: %d\n", dev_priv->pc8.disable_count);
+
+	dev_priv->pc8.disable_count--;
+	if (dev_priv->pc8.disable_count != 0)
+		return;
+
+	schedule_delayed_work(&dev_priv->pc8.enable_work,
+			      msecs_to_jiffies(5 * 1000));
+}
+
+static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = dev_priv->dev;
+	uint32_t val;
+
+	WARN_ON(!mutex_is_locked(&dev_priv->pc8.lock));
+	WARN(dev_priv->pc8.disable_count < 0,
+	     "pc8.disable_count: %d\n", dev_priv->pc8.disable_count);
+
+	dev_priv->pc8.disable_count++;
+	if (dev_priv->pc8.disable_count != 1)
+		return;
+
+	cancel_delayed_work_sync(&dev_priv->pc8.enable_work);
+	if (!dev_priv->pc8.enabled)
+		return;
+
+	DRM_DEBUG_KMS("Disabling package C8+\n");
+
+	hsw_restore_lcpll(dev_priv);
+	hsw_pc8_restore_interrupts(dev);
+	lpt_init_pch_refclk(dev);
+
+	if (dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
+		val = I915_READ(SOUTH_DSPCLK_GATE_D);
+		val |= PCH_LP_PARTITION_LEVEL_DISABLE;
+		I915_WRITE(SOUTH_DSPCLK_GATE_D, val);
+	}
+
+	intel_prepare_ddi(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);
+	dev_priv->pc8.enabled = false;
+}
+
+void hsw_enable_package_c8(struct drm_i915_private *dev_priv)
+{
+	mutex_lock(&dev_priv->pc8.lock);
+	__hsw_enable_package_c8(dev_priv);
+	mutex_unlock(&dev_priv->pc8.lock);
+}
+
+void hsw_disable_package_c8(struct drm_i915_private *dev_priv)
+{
+	mutex_lock(&dev_priv->pc8.lock);
+	__hsw_disable_package_c8(dev_priv);
+	mutex_unlock(&dev_priv->pc8.lock);
+}
+
+static bool hsw_can_enable_package_c8(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = dev_priv->dev;
+	struct intel_crtc *crtc;
+	uint32_t val;
+
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head)
+		if (crtc->base.enabled)
+			return false;
+
+	/* This case is still possible since we have the i915.disable_power_well
+	 * parameter and also the KVMr or something else might be requesting the
+	 * power well. */
+	val = I915_READ(HSW_PWR_WELL_DRIVER);
+	if (val != 0) {
+		DRM_DEBUG_KMS("Not enabling PC8: power well on\n");
+		return false;
+	}
+
+	return true;
+}
+
+/* Since we're called from modeset_global_resources there's no way to
+ * symmetrically increase and decrease the refcount, so we use
+ * dev_priv->pc8.requirements_met to track whether we already have the refcount
+ * or not.
+ */
+static void hsw_update_package_c8(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	bool allow;
+
+	if (!i915_enable_pc8)
+		return;
+
+	mutex_lock(&dev_priv->pc8.lock);
+
+	allow = hsw_can_enable_package_c8(dev_priv);
+
+	if (allow == dev_priv->pc8.requirements_met)
+		goto done;
+
+	dev_priv->pc8.requirements_met = allow;
+
+	if (allow)
+		__hsw_enable_package_c8(dev_priv);
+	else
+		__hsw_disable_package_c8(dev_priv);
+
+done:
+	mutex_unlock(&dev_priv->pc8.lock);
+}
+
+static void hsw_package_c8_gpu_idle(struct drm_i915_private *dev_priv)
+{
+	if (!dev_priv->pc8.gpu_idle) {
+		dev_priv->pc8.gpu_idle = true;
+		hsw_enable_package_c8(dev_priv);
+	}
+}
+
+static void hsw_package_c8_gpu_busy(struct drm_i915_private *dev_priv)
+{
+	if (dev_priv->pc8.gpu_idle) {
+		dev_priv->pc8.gpu_idle = false;
+		hsw_disable_package_c8(dev_priv);
+	}
+}
+
 static void haswell_modeset_global_resources(struct drm_device *dev)
 {
 	bool enable = false;
@@ -6077,6 +6237,8 @@ static void haswell_modeset_global_resources(struct drm_device *dev)
 	}
 
 	intel_set_power_well(dev, enable);
+
+	hsw_update_package_c8(dev);
 }
 
 static int haswell_crtc_mode_set(struct drm_crtc *crtc,
@@ -7308,13 +7470,19 @@ static void intel_decrease_pllclock(struct drm_crtc *crtc)
 
 void intel_mark_busy(struct drm_device *dev)
 {
-	i915_update_gfx_val(dev->dev_private);
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	hsw_package_c8_gpu_busy(dev_priv);
+	i915_update_gfx_val(dev_priv);
 }
 
 void intel_mark_idle(struct drm_device *dev)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc;
 
+	hsw_package_c8_gpu_idle(dev_priv);
+
 	if (!i915_powersave)
 		return;
 
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 2726d4d..2151d13 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -344,6 +344,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 	else
 		precharge = 5;
 
+	intel_aux_display_runtime_get(dev_priv);
+
 	/* Try to wait for any previous AUX channel activity */
 	for (try = 0; try < 3; try++) {
 		status = I915_READ_NOTRACE(ch_ctl);
@@ -434,6 +436,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 	ret = recv_bytes;
 out:
 	pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
+	intel_aux_display_runtime_put(dev_priv);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 8222f24..1760808 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -745,6 +745,7 @@ extern void intel_set_power_well(struct drm_device *dev, bool enable);
 extern void intel_enable_gt_powersave(struct drm_device *dev);
 extern void intel_disable_gt_powersave(struct drm_device *dev);
 extern void ironlake_teardown_rc6(struct drm_device *dev);
+void gen6_update_ring_freq(struct drm_device *dev);
 
 extern bool intel_ddi_get_hw_state(struct intel_encoder *encoder,
 				   enum pipe *pipe);
@@ -784,5 +785,12 @@ extern void ilk_disable_gt_irq(struct drm_i915_private *dev_priv,
 extern void snb_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
 extern void snb_disable_pm_irq(struct drm_i915_private *dev_priv,
 			       uint32_t mask);
+extern void hsw_enable_pc8_work(struct work_struct *__work);
+extern void hsw_enable_package_c8(struct drm_i915_private *dev_priv);
+extern void hsw_disable_package_c8(struct drm_i915_private *dev_priv);
+extern void hsw_pc8_disable_interrupts(struct drm_device *dev);
+extern void hsw_pc8_restore_interrupts(struct drm_device *dev);
+extern void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv);
+extern void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv);
 
 #endif /* __INTEL_DRV_H__ */
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 639fe19..d1c1e0f7 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -398,6 +398,7 @@ gmbus_xfer(struct i2c_adapter *adapter,
 	int i, reg_offset;
 	int ret = 0;
 
+	intel_aux_display_runtime_get(dev_priv);
 	mutex_lock(&dev_priv->gmbus_mutex);
 
 	if (bus->force_bit) {
@@ -497,6 +498,7 @@ timeout:
 
 out:
 	mutex_unlock(&dev_priv->gmbus_mutex);
+	intel_aux_display_runtime_put(dev_priv);
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 8dca253..18f1068 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3607,7 +3607,7 @@ static void gen6_enable_rps(struct drm_device *dev)
 	gen6_gt_force_wake_put(dev_priv);
 }
 
-static void gen6_update_ring_freq(struct drm_device *dev)
+void gen6_update_ring_freq(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int min_freq = 15;
@@ -5433,6 +5433,17 @@ void intel_init_power_well(struct drm_device *dev)
 		I915_WRITE(HSW_PWR_WELL_BIOS, 0);
 }
 
+/* Disables PC8 so we can use the GMBUS and DP AUX interrupts. */
+void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv)
+{
+	hsw_disable_package_c8(dev_priv);
+}
+
+void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv)
+{
+	hsw_enable_package_c8(dev_priv);
+}
+
 /* Set up chip specific power management-related functions */
 void intel_init_pm(struct drm_device *dev)
 {
-- 
1.8.1.2

[PATCH 4/6] drm/i915: add i915_pc8_status debugfs file

[Paulo Zanoni]

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

Make it print the value of the variables on the PC8 struct.

v2: Update to recent renames and add the new fields.

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 4785d8c..4d836f1 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1769,6 +1769,30 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 	return 0;
 }
 
+static int i915_pc8_status(struct seq_file *m, void *unused)
+{
+	struct drm_info_node *node = (struct drm_info_node *) m->private;
+	struct drm_device *dev = node->minor->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (!IS_HASWELL(dev)) {
+		seq_puts(m, "not supported\n");
+		return 0;
+	}
+
+	mutex_lock(&dev_priv->pc8.lock);
+	seq_printf(m, "Requirements met: %s\n",
+		   yesno(dev_priv->pc8.requirements_met));
+	seq_printf(m, "GPU idle: %s\n", yesno(dev_priv->pc8.gpu_idle));
+	seq_printf(m, "Disable count: %d\n", dev_priv->pc8.disable_count);
+	seq_printf(m, "IRQs disabled: %s\n",
+		   yesno(dev_priv->pc8.irqs_disabled));
+	seq_printf(m, "Enabled: %s\n", yesno(dev_priv->pc8.enabled));
+	mutex_unlock(&dev_priv->pc8.lock);
+
+	return 0;
+}
+
 static int
 i915_wedged_get(void *data, u64 *val)
 {
@@ -2208,6 +2232,7 @@ static struct drm_info_list i915_debugfs_list[] = {
 	{"i915_dpio", i915_dpio_info, 0},
 	{"i915_llc", i915_llc, 0},
 	{"i915_edp_psr_status", i915_edp_psr_status, 0},
+	{"i915_pc8_status", i915_pc8_status, 0},
 };
 #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
 
-- 
1.8.1.2

[PATCH 5/6] drm/i915: add i915.pc8_timeout function

[Paulo Zanoni]

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

We currently only enter PC8+ after all its required conditions are
met, there's no rendering, and we stay like that for at least 5
seconds.

I chose "5 seconds" because this value is conservative and won't make
us enter/leave PC8+ thousands of times after the screen is off: some
desktop environments have applications that wake up and do rendering
every 1-3 seconds, even when the screen is off and the machine is
completely idle.

But when I was testing my PC8+ patches I set the default value to
100ms so I could use the bad-behaving desktop environments to
stress-test my patches. I also thought it would be a good idea to ask
our power management team to test different values, but I'm pretty
sure they would ask me for an easy way to change the timeout. So to
help these 2 cases I decided to create an option that would make it
easier to change the default value. I also expect people making
specific products that use our driver could try to find the perfect
timeout for them.

Anyway, fixing the bad-behaving applications will always lead to
better power savings than just changing the timeout value: you need to
stop waking the Kernel, not quickly put it back to sleep again after
you wake it for nothing. Bad sleep leads to bad mood!

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

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index f197362..ad28a72 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -145,6 +145,10 @@ int i915_enable_pc8 __read_mostly = 0;
 module_param_named(enable_pc8, i915_enable_pc8, int, 0600);
 MODULE_PARM_DESC(enable_pc8, "Enable support for low power package C states (PC8+) (default: false)");
 
+int i915_pc8_timeout __read_mostly = 5000;
+module_param_named(pc8_timeout, i915_pc8_timeout, int, 0600);
+MODULE_PARM_DESC(pc8_timeout, "Number of msecs of idleness required to enter PC8+ (default: 5000)");
+
 bool i915_prefault_disable __read_mostly;
 module_param_named(prefault_disable, i915_prefault_disable, bool, 0600);
 MODULE_PARM_DESC(prefault_disable,
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 34c5af5..3138083 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1710,6 +1710,7 @@ extern int i915_disable_power_well __read_mostly;
 extern int i915_enable_ips __read_mostly;
 extern bool i915_fastboot __read_mostly;
 extern int i915_enable_pc8 __read_mostly;
+extern int i915_pc8_timeout __read_mostly;
 extern bool i915_prefault_disable __read_mostly;
 
 extern int i915_suspend(struct drm_device *dev, pm_message_t state);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7d5b01a..ba3bdd5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6099,7 +6099,7 @@ static void __hsw_enable_package_c8(struct drm_i915_private *dev_priv)
 		return;
 
 	schedule_delayed_work(&dev_priv->pc8.enable_work,
-			      msecs_to_jiffies(5 * 1000));
+			      msecs_to_jiffies(i915_pc8_timeout));
 }
 
 static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv)
-- 
1.8.1.2

[PATCH 6/6] drm/i915: enable Package C8+ by default

[Paulo Zanoni]

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

This should be working, so enable it by default. Also easy to revert.

v2: Rebase, s/allow/enable/.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index ad28a72..735dd56 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -141,9 +141,9 @@ module_param_named(fastboot, i915_fastboot, bool, 0600);
 MODULE_PARM_DESC(fastboot, "Try to skip unnecessary mode sets at boot time "
 		 "(default: false)");
 
-int i915_enable_pc8 __read_mostly = 0;
+int i915_enable_pc8 __read_mostly = 1;
 module_param_named(enable_pc8, i915_enable_pc8, int, 0600);
-MODULE_PARM_DESC(enable_pc8, "Enable support for low power package C states (PC8+) (default: false)");
+MODULE_PARM_DESC(enable_pc8, "Enable support for low power package C states (PC8+) (default: true)");
 
 int i915_pc8_timeout __read_mostly = 5000;
 module_param_named(pc8_timeout, i915_pc8_timeout, int, 0600);
-- 
1.8.1.2

[PATCH igt] tests: add pc8

[Paulo Zanoni]

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

This test chekcs our code that enables Package C8+. The environment
requirements for this test are quite complicated:
  - The machine needs to be properly configured to reach PC8+ when
    possible, which means all the power management policies and device
    drivers must be properly configured.
  - You need at least one output connected.
  - You need the /dev/cpu/0/msr file available.
  - You need the /dev/i2c-* files available.

v2: - Many many changes due to the latest review comments and rebase.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 tests/.gitignore  |   1 +
 tests/Makefile.am |   1 +
 tests/pc8.c       | 804 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 806 insertions(+)
 create mode 100644 tests/pc8.c

diff --git a/tests/.gitignore b/tests/.gitignore
index b5eb7a5..19f827a 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -93,6 +93,7 @@ getversion
 kms_flip
 kms_render
 kms_setmode
+pc8
 prime_nv_api
 prime_nv_pcopy
 prime_nv_test
diff --git a/tests/Makefile.am b/tests/Makefile.am
index c724829..370a700 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -45,6 +45,7 @@ TESTS_progs_M = \
 	kms_render \
 	kms_setmode \
 	$(NOUVEAU_TESTS_M) \
+	pc8 \
 	prime_self_import \
 	$(NULL)
 
diff --git a/tests/pc8.c b/tests/pc8.c
new file mode 100644
index 0000000..06c54b7
--- /dev/null
+++ b/tests/pc8.c
@@ -0,0 +1,804 @@
+/*
+ * Copyright © 2013 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Paulo Zanoni <paulo.r.zanoni@intel.com>
+ *
+ */
+
+#include <stdio.h>
+#include <stdint.h>
+#include <stdbool.h>
+#include <string.h>
+
+#include <unistd.h>
+#include <fcntl.h>
+#include <dirent.h>
+#include <sys/ioctl.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <linux/i2c.h>
+#include <linux/i2c-dev.h>
+
+#include "drm.h"
+#include "drmtest.h"
+#include "intel_batchbuffer.h"
+#include "intel_gpu_tools.h"
+#include "i915_drm.h"
+
+#define MSR_PC8_RES	0x630
+#define MSR_PC9_RES	0x631
+#define MSR_PC10_RES	0x632
+
+#define MAX_CONNECTORS	32
+#define MAX_ENCODERS	32
+#define MAX_CRTCS	16
+
+int drm_fd, msr_fd;
+struct mode_set_data ms_data;
+
+/* Stuff used when creating FBs and mode setting. */
+struct mode_set_data {
+	drmModeResPtr res;
+	drmModeConnectorPtr connectors[MAX_CONNECTORS];
+	drmModePropertyBlobPtr edids[MAX_CONNECTORS];
+
+	drm_intel_bufmgr *bufmgr;
+	uint32_t devid;
+};
+
+/* Stuff we query at different times so we can compare. */
+struct compare_data {
+	drmModeResPtr res;
+	drmModeEncoderPtr encoders[MAX_ENCODERS];
+	drmModeConnectorPtr connectors[MAX_CONNECTORS];
+	drmModeCrtcPtr crtcs[MAX_CRTCS];
+	drmModePropertyBlobPtr edids[MAX_CONNECTORS];
+};
+
+struct compare_registers {
+	/* We know these are lost */
+	uint32_t arb_mode;
+	uint32_t tilectl;
+
+	/* Stuff touched at init_clock_gating, so we can make sure we
+	 * don't need to call it when reiniting. */
+	uint32_t gen6_ucgctl2;
+	uint32_t gen7_l3cntlreg1;
+	uint32_t transa_chicken1;
+
+	uint32_t deier;
+	uint32_t gtier;
+
+	uint32_t ddi_buf_trans_a_1;
+	uint32_t ddi_buf_trans_b_5;
+	uint32_t ddi_buf_trans_c_10;
+	uint32_t ddi_buf_trans_d_15;
+	uint32_t ddi_buf_trans_e_20;
+};
+
+/* If the read fails, then the machine doesn't support PC8+ residencies. */
+static bool supports_pc8_plus_residencies(void)
+{
+	int rc;
+	uint64_t val;
+
+	rc = pread(msr_fd, &val, sizeof(uint64_t), MSR_PC8_RES);
+	if (!rc == sizeof(val))
+		return false;
+	rc = pread(msr_fd, &val, sizeof(uint64_t), MSR_PC9_RES);
+	if (!rc == sizeof(val))
+		return false;
+	rc = pread(msr_fd, &val, sizeof(uint64_t), MSR_PC10_RES);
+	if (!rc == sizeof(val))
+		return false;
+
+	return true;
+}
+
+static uint64_t get_residency(uint32_t type)
+{
+	int rc;
+	uint64_t ret;
+
+	rc = pread(msr_fd, &ret, sizeof(uint64_t), type);
+	igt_assert(rc == sizeof(ret));
+
+	return ret;
+}
+
+static bool pc8_plus_residency_changed(unsigned int timeout_sec)
+{
+	unsigned int i;
+	uint64_t res_pc8, res_pc9, res_pc10;
+	int to_sleep = 100 * 1000;
+
+	res_pc8 = get_residency(MSR_PC8_RES);
+	res_pc9 = get_residency(MSR_PC9_RES);
+	res_pc10 = get_residency(MSR_PC10_RES);
+
+	for (i = 0; i < timeout_sec * 1000 * 1000; i += to_sleep) {
+		if (res_pc8 != get_residency(MSR_PC8_RES) ||
+		    res_pc9 != get_residency(MSR_PC9_RES) ||
+		    res_pc10 != get_residency(MSR_PC10_RES)) {
+			return true;
+		}
+		usleep(to_sleep);
+	}
+
+	return false;
+}
+
+/* Checks not only if PC8+ is allowed, but also if we're reaching it.
+ * We call this when we expect this function to return quickly since PC8 is
+ * actually enabled, so the 30s timeout we use shouldn't matter. */
+static bool pc8_plus_enabled(void)
+{
+	return pc8_plus_residency_changed(30);
+}
+
+/* We call this when we expect PC8+ to be actually disabled, so we should not
+ * return until the 5s timeout expires. In other words: in the "happy case",
+ * every time we call this function the program will take 5s more to finish. */
+static bool pc8_plus_disabled(void)
+{
+	return !pc8_plus_residency_changed(5);
+}
+
+static void disable_all_screens(struct mode_set_data *data)
+{
+	int i, rc;
+
+	for (i = 0; i < data->res->count_crtcs; i++) {
+		rc = drmModeSetCrtc(drm_fd, data->res->crtcs[i], -1, 0, 0,
+				    NULL, 0, NULL);
+		igt_assert(rc == 0);
+	}
+}
+
+static uint32_t create_fb(struct mode_set_data *data, int width, int height)
+{
+	struct kmstest_fb fb;
+	cairo_t *cr;
+	uint32_t buffer_id;
+
+	buffer_id = kmstest_create_fb(drm_fd, width, height, 32, 24, false,
+				      &fb);
+	cr = kmstest_get_cairo_ctx(drm_fd, &fb);
+	kmstest_paint_test_pattern(cr, width, height);
+	return buffer_id;
+}
+
+static void enable_one_screen(struct mode_set_data *data)
+{
+	uint32_t crtc_id = 0, buffer_id = 0, connector_id = 0;
+	drmModeModeInfoPtr mode = NULL;
+	int i, rc;
+
+	for (i = 0; i < data->res->count_connectors; i++) {
+		drmModeConnectorPtr c = data->connectors[i];
+
+		if (c->connection == DRM_MODE_CONNECTED && c->count_modes) {
+			connector_id = c->connector_id;
+			mode = &c->modes[0];
+			break;
+		}
+	}
+
+	crtc_id = data->res->crtcs[0];
+	buffer_id = create_fb(data, mode->hdisplay, mode->vdisplay);
+
+	igt_assert(crtc_id);
+	igt_assert(buffer_id);
+	igt_assert(connector_id);
+	igt_assert(mode);
+
+	rc = drmModeSetCrtc(drm_fd, crtc_id, buffer_id, 0, 0, &connector_id,
+			    1, mode);
+	igt_assert(rc == 0);
+}
+
+static drmModePropertyBlobPtr get_connector_edid(drmModeConnectorPtr connector,
+						 int index)
+{
+	unsigned int i;
+	drmModeObjectPropertiesPtr props;
+	drmModePropertyBlobPtr ret = NULL;
+
+	props = drmModeObjectGetProperties(drm_fd, connector->connector_id,
+					   DRM_MODE_OBJECT_CONNECTOR);
+
+	for (i = 0; i < props->count_props; i++) {
+		drmModePropertyPtr prop = drmModeGetProperty(drm_fd,
+							     props->props[i]);
+
+		if (strcmp(prop->name, "EDID") == 0) {
+			igt_assert(prop->flags & DRM_MODE_PROP_BLOB);
+			igt_assert(prop->count_blobs == 0);
+			ret = drmModeGetPropertyBlob(drm_fd,
+						     props->prop_values[i]);
+		}
+
+		drmModeFreeProperty(prop);
+	}
+
+	drmModeFreeObjectProperties(props);
+	return ret;
+}
+
+static void init_mode_set_data(struct mode_set_data *data)
+{
+	int i;
+
+	data->res = drmModeGetResources(drm_fd);
+	igt_assert(data->res);
+	igt_assert(data->res->count_connectors <= MAX_CONNECTORS);
+
+	for (i = 0; i < data->res->count_connectors; i++) {
+		data->connectors[i] = drmModeGetConnector(drm_fd,
+						data->res->connectors[i]);
+		data->edids[i] = get_connector_edid(data->connectors[i], i);
+	}
+
+	data->bufmgr = drm_intel_bufmgr_gem_init(drm_fd, 4096);
+	data->devid = intel_get_drm_devid(drm_fd);
+
+	do_or_die(igt_set_vt_graphics_mode());
+	drm_intel_bufmgr_gem_enable_reuse(data->bufmgr);
+}
+
+static void fini_mode_set_data(struct mode_set_data *data)
+{
+	int i;
+
+	drm_intel_bufmgr_destroy(data->bufmgr);
+
+	for (i = 0; i < data->res->count_connectors; i++) {
+		drmModeFreeConnector(data->connectors[i]);
+		drmModeFreePropertyBlob(data->edids[i]);
+	}
+	drmModeFreeResources(data->res);
+}
+
+static void get_drm_info(struct compare_data *data)
+{
+	int i;
+
+	data->res = drmModeGetResources(drm_fd);
+	igt_assert(data->res);
+
+	igt_assert(data->res->count_connectors <= MAX_CONNECTORS);
+	igt_assert(data->res->count_encoders <= MAX_ENCODERS);
+	igt_assert(data->res->count_crtcs <= MAX_CRTCS);
+
+	for (i = 0; i < data->res->count_connectors; i++) {
+		data->connectors[i] = drmModeGetConnector(drm_fd,
+						data->res->connectors[i]);
+		data->edids[i] = get_connector_edid(data->connectors[i], i);
+	}
+	for (i = 0; i < data->res->count_encoders; i++)
+		data->encoders[i] = drmModeGetEncoder(drm_fd,
+						data->res->encoders[i]);
+	for (i = 0; i < data->res->count_crtcs; i++)
+		data->crtcs[i] = drmModeGetCrtc(drm_fd, data->res->crtcs[i]);
+}
+
+static void get_registers(struct compare_registers *data)
+{
+	intel_register_access_init(intel_get_pci_device(), 0);
+	data->arb_mode = INREG(0x4030);
+	data->tilectl = INREG(0x101000);
+	data->gen6_ucgctl2 = INREG(0x9404);
+	data->gen7_l3cntlreg1 = INREG(0xB0C1);
+	data->transa_chicken1 = INREG(0xF0060);
+	data->deier = INREG(0x4400C);
+	data->gtier = INREG(0x4401C);
+	data->ddi_buf_trans_a_1 = INREG(0x64E00);
+	data->ddi_buf_trans_b_5 = INREG(0x64E70);
+	data->ddi_buf_trans_c_10 = INREG(0x64EE0);
+	data->ddi_buf_trans_d_15 = INREG(0x64F58);
+	data->ddi_buf_trans_e_20 = INREG(0x64FCC);
+	intel_register_access_fini();
+}
+
+static void free_drm_info(struct compare_data *data)
+{
+	int i;
+
+	for (i = 0; i < data->res->count_connectors; i++) {
+		drmModeFreeConnector(data->connectors[i]);
+		drmModeFreePropertyBlob(data->edids[i]);
+	}
+	for (i = 0; i < data->res->count_encoders; i++)
+		drmModeFreeEncoder(data->encoders[i]);
+	for (i = 0; i < data->res->count_crtcs; i++)
+		drmModeFreeCrtc(data->crtcs[i]);
+
+	drmModeFreeResources(data->res);
+}
+
+#define COMPARE(d1, d2, data) igt_assert(d1->data == d2->data)
+#define COMPARE_ARRAY(d1, d2, size, data) do { \
+	for (i = 0; i < size; i++) \
+		igt_assert(d1->data[i] == d2->data[i]); \
+} while (0)
+
+static void assert_drm_resources_equal(struct compare_data *d1,
+				       struct compare_data *d2)
+{
+	COMPARE(d1, d2, res->count_connectors);
+	COMPARE(d1, d2, res->count_encoders);
+	COMPARE(d1, d2, res->count_crtcs);
+	COMPARE(d1, d2, res->min_width);
+	COMPARE(d1, d2, res->max_width);
+	COMPARE(d1, d2, res->min_height);
+	COMPARE(d1, d2, res->max_height);
+}
+
+static void assert_modes_equal(drmModeModeInfoPtr m1, drmModeModeInfoPtr m2)
+{
+	COMPARE(m1, m2, clock);
+	COMPARE(m1, m2, hdisplay);
+	COMPARE(m1, m2, hsync_start);
+	COMPARE(m1, m2, hsync_end);
+	COMPARE(m1, m2, htotal);
+	COMPARE(m1, m2, hskew);
+	COMPARE(m1, m2, vdisplay);
+	COMPARE(m1, m2, vsync_start);
+	COMPARE(m1, m2, vsync_end);
+	COMPARE(m1, m2, vtotal);
+	COMPARE(m1, m2, vscan);
+	COMPARE(m1, m2, vrefresh);
+	COMPARE(m1, m2, flags);
+	COMPARE(m1, m2, type);
+	igt_assert(strcmp(m1->name, m2->name) == 0);
+}
+
+static void assert_drm_connectors_equal(drmModeConnectorPtr c1,
+					drmModeConnectorPtr c2)
+{
+	int i;
+
+	COMPARE(c1, c2, connector_id);
+	COMPARE(c1, c2, connector_type);
+	COMPARE(c1, c2, connector_type_id);
+	COMPARE(c1, c2, mmWidth);
+	COMPARE(c1, c2, mmHeight);
+	COMPARE(c1, c2, count_modes);
+	COMPARE(c1, c2, count_props);
+	COMPARE(c1, c2, count_encoders);
+	COMPARE_ARRAY(c1, c2, c1->count_props, props);
+	COMPARE_ARRAY(c1, c2, c1->count_encoders, encoders);
+
+	for (i = 0; i < c1->count_modes; i++)
+		assert_modes_equal(&c1->modes[0], &c2->modes[0]);
+}
+
+static void assert_drm_encoders_equal(drmModeEncoderPtr e1,
+				      drmModeEncoderPtr e2)
+{
+	COMPARE(e1, e2, encoder_id);
+	COMPARE(e1, e2, encoder_type);
+	COMPARE(e1, e2, possible_crtcs);
+	COMPARE(e1, e2, possible_clones);
+}
+
+static void assert_drm_crtcs_equal(drmModeCrtcPtr c1, drmModeCrtcPtr c2)
+{
+	COMPARE(c1, c2, crtc_id);
+}
+
+static void assert_drm_edids_equal(drmModePropertyBlobPtr e1,
+				   drmModePropertyBlobPtr e2)
+{
+	if (!e1 && !e2)
+		return;
+	igt_assert(e1 && e2);
+
+	COMPARE(e1, e2, id);
+	COMPARE(e1, e2, length);
+
+	igt_assert(memcmp(e1->data, e2->data, e1->length) == 0);
+}
+
+static void compare_registers(struct compare_registers *d1,
+			      struct compare_registers *d2)
+{
+	COMPARE(d1, d2, gen6_ucgctl2);
+	COMPARE(d1, d2, gen7_l3cntlreg1);
+	COMPARE(d1, d2, transa_chicken1);
+	COMPARE(d1, d2, arb_mode);
+	COMPARE(d1, d2, tilectl);
+	COMPARE(d1, d2, arb_mode);
+	COMPARE(d1, d2, tilectl);
+	COMPARE(d1, d2, gtier);
+	COMPARE(d1, d2, ddi_buf_trans_a_1);
+	COMPARE(d1, d2, ddi_buf_trans_b_5);
+	COMPARE(d1, d2, ddi_buf_trans_c_10);
+	COMPARE(d1, d2, ddi_buf_trans_d_15);
+	COMPARE(d1, d2, ddi_buf_trans_e_20);
+}
+
+static void assert_drm_infos_equal(struct compare_data *d1,
+				   struct compare_data *d2)
+{
+	int i;
+
+	assert_drm_resources_equal(d1, d2);
+
+	for (i = 0; i < d1->res->count_connectors; i++) {
+		assert_drm_connectors_equal(d1->connectors[i],
+					    d2->connectors[i]);
+		assert_drm_edids_equal(d1->edids[i], d2->edids[i]);
+	}
+
+	for (i = 0; i < d1->res->count_encoders; i++)
+		assert_drm_encoders_equal(d1->encoders[i], d2->encoders[i]);
+
+	for (i = 0; i < d1->res->count_crtcs; i++)
+		assert_drm_crtcs_equal(d1->crtcs[i], d2->crtcs[i]);
+}
+
+static void blt_color_fill(struct intel_batchbuffer *batch,
+			   drm_intel_bo *buf,
+			   const unsigned int pages)
+{
+	const unsigned short height = pages/4;
+	const unsigned short width = 4096;
+
+	BEGIN_BATCH(5);
+	OUT_BATCH(COLOR_BLT_CMD | COLOR_BLT_WRITE_ALPHA | COLOR_BLT_WRITE_RGB);
+	OUT_BATCH((3 << 24) | /* 32 Bit Color */
+		  0xF0 | /* Raster OP copy background register */
+		  0); /* Dest pitch is 0 */
+	OUT_BATCH(width << 16 | height);
+	OUT_RELOC(buf, I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER, 0);
+	OUT_BATCH(rand()); /* random pattern */
+	ADVANCE_BATCH();
+}
+
+static void test_batch(struct mode_set_data *data)
+{
+	struct intel_batchbuffer *batch;
+	int64_t timeout_ns = 1000 * 1000 * 1000 * 2;
+	drm_intel_bo *dst;
+	int i, rc;
+
+	dst = drm_intel_bo_alloc(data->bufmgr, "dst", (8 << 20), 4096);
+
+	batch = intel_batchbuffer_alloc(data->bufmgr,
+					intel_get_drm_devid(drm_fd));
+
+	for (i = 0; i < 1000; i++)
+		blt_color_fill(batch, dst, ((8 << 20) >> 12));
+
+	rc = drm_intel_gem_bo_wait(dst, timeout_ns);
+	igt_assert(!rc);
+
+	intel_batchbuffer_free(batch);
+}
+
+/* We could check the checksum too, but just the header is probably enough. */
+static bool edid_is_valid(const unsigned char *edid)
+{
+	char edid_header[] = {
+		0x0, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0x0,
+	};
+
+	return (memcmp(edid, edid_header, sizeof(edid_header)) == 0);
+}
+
+static int count_drm_valid_edids(struct mode_set_data *data)
+{
+	int i, ret = 0;
+
+	for (i = 0; i < data->res->count_connectors; i++)
+		if (data->edids[i] && edid_is_valid(data->edids[i]->data))
+			ret++;
+	return ret;
+}
+
+static int count_drm_edp_outputs(struct mode_set_data *data)
+{
+	int i, ret = 0;
+
+	for (i = 0; i < data->res->count_connectors; i++)
+		if (data->connectors[i]->connector_type ==
+		    DRM_MODE_CONNECTOR_eDP)
+			ret++;
+	return ret;
+}
+
+static bool i2c_edid_is_valid(int fd)
+{
+	int rc;
+	unsigned char edid[128] = {};
+	struct i2c_msg msgs[] = {
+		{ /* Start at 0. */
+			.addr = 0x50,
+			.flags = 0,
+			.len = 1,
+			.buf = edid,
+		}, { /* Now read the EDID. */
+			.addr = 0x50,
+			.flags = I2C_M_RD,
+			.len = 128,
+			.buf = edid,
+		}
+	};
+	struct i2c_rdwr_ioctl_data msgset = {
+		.msgs = msgs,
+		.nmsgs = 2,
+	};
+
+	rc = ioctl(fd, I2C_RDWR, &msgset);
+	return (rc >= 0) ? edid_is_valid(edid) : false;
+}
+
+static int count_i2c_valid_edids(void)
+{
+	int fd, ret = 0;
+	DIR *dir;
+
+	struct dirent *dirent;
+	char full_name[32];
+
+	dir = opendir("/dev/");
+	igt_assert(dir);
+
+	while ((dirent = readdir(dir))) {
+		if (strncmp(dirent->d_name, "i2c-", 4) == 0) {
+			snprintf(full_name, 32, "/dev/%s", dirent->d_name);
+			fd = open(full_name, O_RDWR);
+			igt_assert(fd != -1);
+			if (i2c_edid_is_valid(fd))
+				ret++;
+			close(fd);
+		}
+	}
+
+	closedir(dir);
+
+	return 0;
+}
+
+/* We run this test when all outputs are turned off. When eDP panels are turned
+ * off, I2C doesn't work on them and we get a nice dmesg backtrace. So count how
+ * many eDP panels we have and subtract them. */
+static bool test_i2c(struct mode_set_data *data)
+{
+	int i2c_edids = count_i2c_valid_edids();
+	int drm_edids = count_drm_valid_edids(data);
+	int edps = count_drm_edp_outputs(data);
+
+	return i2c_edids + edps == drm_edids;
+}
+
+static bool setup_environment(void)
+{
+	int i2c_dev_files;
+	DIR *dev_dir;
+	struct dirent *dirent;
+
+	drm_fd = drm_open_any();
+	if (drm_fd <= 0)
+		return false;
+
+	init_mode_set_data(&ms_data);
+
+	/* Only Haswell supports the PC8 feature. */
+	if (!IS_HASWELL(ms_data.devid)) {
+		printf("PC8+ feature only supported on Haswell.\n");
+		return false;
+	}
+
+	/* Make sure our Kernel supports MSR and the module is loaded. */
+	msr_fd = open("/dev/cpu/0/msr", O_RDONLY);
+	if (msr_fd == -1) {
+		printf("Can't open /dev/cpu/0/msr.\n");
+		return false;
+	}
+
+	/* Make sure the /dev/i2c-* files exist. */
+	dev_dir = opendir("/dev");
+	if (!dev_dir) {
+		printf("Can't open /dev.\n");
+		return false;
+	}
+	i2c_dev_files = 0;
+	while ((dirent = readdir(dev_dir))) {
+		if (strncmp(dirent->d_name, "i2c-", 4) == 0)
+			i2c_dev_files++;
+	}
+	closedir(dev_dir);
+	if (!i2c_dev_files) {
+		printf("No /dev/i2c-* files.\n");
+		return false;
+	}
+
+	/* Non-ULT machines don't support PC8+. */
+	if (!supports_pc8_plus_residencies()) {
+		printf("Machine doesn't support PC8+ residencies.\n");
+		return false;
+	}
+
+	/* Make sure PC8+ residencies move! */
+	disable_all_screens(&ms_data);
+	if (!pc8_plus_enabled()) {
+		printf("Machine is not reaching PC8+ states, please check its "
+		       "configuration.\n");
+		return false;
+	}
+
+	/* Make sure PC8+ residencies stop! */
+	enable_one_screen(&ms_data);
+	if (!pc8_plus_disabled()) {
+		printf("PC8+ residency didn't stop with screen enabled.\n");
+		return false;
+	}
+
+	return true;
+}
+
+static void teardown_environment(void)
+{
+	fini_mode_set_data(&ms_data);
+	drmClose(drm_fd);
+	close(msr_fd);
+}
+
+/* Test of the DRM resources reported by the IOCTLs are still the same. This
+ * ensures we still see the monitors with the same eyes. We get the EDIDs and
+ * compare them, which ensures we use DP AUX or GMBUS depending on what's
+ * connected. */
+static void drm_resources_equal_subtest(void)
+{
+	struct compare_data pre_pc8, during_pc8, post_pc8;
+
+	printf("Checking the if the DRM resources match.\n");
+
+	enable_one_screen(&ms_data);
+	igt_assert(pc8_plus_disabled());
+	get_drm_info(&pre_pc8);
+	igt_assert(pc8_plus_disabled());
+
+	disable_all_screens(&ms_data);
+	igt_assert(pc8_plus_enabled());
+	get_drm_info(&during_pc8);
+	igt_assert(pc8_plus_enabled());
+
+	enable_one_screen(&ms_data);
+	igt_assert(pc8_plus_disabled());
+	get_drm_info(&post_pc8);
+	igt_assert(pc8_plus_disabled());
+
+	assert_drm_infos_equal(&pre_pc8, &during_pc8);
+	assert_drm_infos_equal(&pre_pc8, &post_pc8);
+
+	free_drm_info(&pre_pc8);
+	free_drm_info(&during_pc8);
+	free_drm_info(&post_pc8);
+}
+
+/* Make sure interrupts are working. */
+static void batch_subtest(void)
+{
+	printf("Testing batchbuffers.\n");
+
+	enable_one_screen(&ms_data);
+	igt_assert(pc8_plus_disabled());
+
+	disable_all_screens(&ms_data);
+	igt_assert(pc8_plus_enabled());
+	test_batch(&ms_data);
+	igt_assert(pc8_plus_enabled());
+}
+
+/* Try to use raw I2C, which also needs interrupts. */
+static void i2c_subtest(void)
+{
+	printf("Testing raw i2c protocol calls.\n");
+
+	enable_one_screen(&ms_data);
+	igt_assert(pc8_plus_disabled());
+
+	disable_all_screens(&ms_data);
+	igt_assert(pc8_plus_enabled());
+	igt_assert(test_i2c(&ms_data));
+	igt_assert(pc8_plus_enabled());
+
+	enable_one_screen(&ms_data);
+}
+
+/* Make us enter/leave PC8+ many times. */
+static void stress_test(void)
+{
+	int i;
+
+	printf("Stress testing.\n");
+
+	for (i = 0; i < 100; i++) {
+		disable_all_screens(&ms_data);
+		igt_assert(pc8_plus_enabled());
+		test_batch(&ms_data);
+		igt_assert(pc8_plus_enabled());
+	}
+}
+
+/* Just reading/writing registers from outside the Kernel is not really a safe
+ * thing to do on Haswell, so don't do this test on the default case. */
+static void register_compare_subtest(void)
+{
+	struct compare_registers pre_pc8, post_pc8;
+
+	printf("Testing register compare.\n");
+
+	enable_one_screen(&ms_data);
+	igt_assert(pc8_plus_disabled());
+	get_registers(&pre_pc8);
+	igt_assert(pc8_plus_disabled());
+
+	disable_all_screens(&ms_data);
+	igt_assert(pc8_plus_enabled());
+	enable_one_screen(&ms_data);
+	igt_assert(pc8_plus_disabled());
+	/* Wait for the registers to be restored. */
+	sleep(1);
+	get_registers(&post_pc8);
+	igt_assert(pc8_plus_disabled());
+
+	compare_registers(&pre_pc8, &post_pc8);
+}
+
+int main(int argc, char *argv[])
+{
+	bool do_register_compare = false;
+
+	if (argc > 1 && strcmp(argv[1], "--do-register-compare") == 0)
+		do_register_compare = true;
+
+	igt_subtest_init(argc, argv);
+
+	/* Skip instead of failing in case the machine is not prepared to reach
+	 * PC8+. We don't want bug reports from cases where the machine is just
+	 * not properly configured. */
+	printf("Checking if the environment is properly configured.\n");
+	if (!setup_environment())
+		return 77;
+
+	igt_subtest("drm-resources-equal")
+		drm_resources_equal_subtest();
+	igt_subtest("batch")
+		batch_subtest();
+	igt_subtest("i2c")
+		i2c_subtest();
+	igt_subtest("stress-test")
+		stress_test();
+	if (do_register_compare)
+		igt_subtest("register-compare")
+			register_compare_subtest();
+
+	teardown_environment();
+
+	printf("Done!\n");
+	return 0;
+}
-- 
1.8.1.2

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

Re: [PATCH 1/6] drm/i915: grab force_wake when restoring LCPLL

[Chris Wilson]

On Mon, Aug 19, 2013 at 01:18:07PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> If LCPLL is disabled, there's a chance we might be in package C8 state
> or deeper, and we'll get a hard hang when restoring LCPLL (also, a red
> led lights up on my motherboard). So grab the force_wake, which will
> get us out of RC6 and, as a consequence, out of PC8+ (since we need
> RC6 to get into PC8+).
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 00114a5..8c63d8e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6031,6 +6031,10 @@ void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
>  		    LCPLL_POWER_DOWN_ALLOW)) == LCPLL_PLL_LOCK)
>  		return;
>  
> +	/* Make sure we're not on PC8 state before disabling PC8, otherwise
> +	 * we'll hang the machine! */
> +	dev_priv->uncore.funcs.force_wake_get(dev_priv);

Calling the bare function pointer should be a warning that what you are
doing is quite perilous and needs a phat comment beforehand.

Or it is a mistake and you just meant gen6_gt_force_wake_get().
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/6] drm/i915: allow package C8+ states on Haswell (disabled)

[Chris Wilson]

On Mon, Aug 19, 2013 at 01:18:09PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> This patch allows PC8+ states on Haswell. These states can only be
> reached when all the display outputs are disabled, and they allow some
> more power savings.
> 
> The fact that the graphics device is allowing PC8+ doesn't mean that
> the machine will actually enter PC8+: all the other devices also need
> to allow PC8+.
> 
> For now this option is disabled by default. You need i915.allow_pc8=1
> if you want it.
> 
> This patch adds a big comment inside i915_drv.h explaining how it
> works and how it tracks things. Read it.
> 
> v2: (this is not really v2, many previous versions were already sent,
>      but they had different names)
>     - Use the new functions to enable/disable GTIMR and GEN6_PMIMR
>     - Rename almost all variables and functions to names suggested by
>       Chris
>     - More WARNs on the IRQ handling code
>     - Also disable PC8 when there's GPU work to do (thanks to Ben for
>       the help on this), so apps can run caster
>     - Enable PC8 on a delayed work function that is delayed for 5
>       seconds. This makes sure we only enable PC8+ if we're really
>       idle
>     - Make sure we're not in PC8+ when suspending
> v3: - WARN if IRQs are disabled on __wait_seqno
>     - Replace some DRM_ERRORs with WARNs
>     - Fix calls to restore GT and PM interrupts
>     - Use intel_mark_busy instead of intel_ring_advance to disable PC8
> v4: - Use the force_wake, Luke!
> v5: - Remove the "IIR is not zero" WARNs
>     - Move the force_wake chunk to its own patch
>     - Only restore what's missing from RC6, not everything
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

I think this is a nice looking patch now, and I think I understand what
you need from this patch alone, so
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 1/6] drm/i915: grab force_wake when restoring LCPLL

[Paulo Zanoni]

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

If LCPLL is disabled, there's a chance we might be in package C8 state
or deeper, and we'll get a hard hang when restoring LCPLL (also, a red
led lights up on my motherboard). So grab the force_wake, which will
get us out of RC6 and, as a consequence, out of PC8+ (since we need
RC6 to get into PC8+).

v2: Call the correct force_wake interface.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 00114a5..ef22161 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6031,6 +6031,10 @@ void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
 		    LCPLL_POWER_DOWN_ALLOW)) == LCPLL_PLL_LOCK)
 		return;
 
+	/* Make sure we're not on PC8 state before disabling PC8, otherwise
+	 * we'll hang the machine! */
+	gen6_gt_force_wake_get(dev_priv);
+
 	if (val & LCPLL_POWER_DOWN_ALLOW) {
 		val &= ~LCPLL_POWER_DOWN_ALLOW;
 		I915_WRITE(LCPLL_CTL, val);
@@ -6058,6 +6062,8 @@ void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
 					LCPLL_CD_SOURCE_FCLK_DONE) == 0, 1))
 			DRM_ERROR("Switching back to LCPLL failed\n");
 	}
+
+	gen6_gt_force_wake_put(dev_priv);
 }
 
 static void haswell_modeset_global_resources(struct drm_device *dev)
-- 
1.8.1.2

Re: [PATCH 1/6] drm/i915: grab force_wake when restoring LCPLL

[Ben Widawsky]

On Mon, Aug 19, 2013 at 04:01:13PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> If LCPLL is disabled, there's a chance we might be in package C8 state
> or deeper, and we'll get a hard hang when restoring LCPLL (also, a red
> led lights up on my motherboard). So grab the force_wake, which will
> get us out of RC6 and, as a consequence, out of PC8+ (since we need
> RC6 to get into PC8+).
> 
> v2: Call the correct force_wake interface.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Hi Paulo. As I recall, the steps in the bspec are laid out exactly so we
don't have to call forcewake. I think the explanation sounds a little
fishy.

Specifically, I thought D_COMP COMP_FORCE + waiting for PLL lock was
sufficient. If it's not, I worry you're papering over another bug, or
incorrect bspec sequence.

Not opposed to the patch in the meanwhile, but it just feels a little
unsafe to me.

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 1/6] drm/i915: grab force_wake when restoring LCPLL

[Paulo Zanoni]

2013/8/19 Ben Widawsky <ben@bwidawsk.net>:
> On Mon, Aug 19, 2013 at 04:01:13PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> If LCPLL is disabled, there's a chance we might be in package C8 state
>> or deeper, and we'll get a hard hang when restoring LCPLL (also, a red
>> led lights up on my motherboard). So grab the force_wake, which will
>> get us out of RC6 and, as a consequence, out of PC8+ (since we need
>> RC6 to get into PC8+).
>>
>> v2: Call the correct force_wake interface.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Hi Paulo. As I recall, the steps in the bspec are laid out exactly so we
> don't have to call forcewake. I think the explanation sounds a little
> fishy.

Well, we seem to do exactly what BSpec says we need to do and we still
get the hard machine hang if we're in PC8 while restoring from LCPLL.


>
> Specifically, I thought D_COMP COMP_FORCE + waiting for PLL lock was
> sufficient.

Why? I don't see D_COMP documentation anywhere or any text that leads
me to believe that. The spec just says "enable these bits, disable
these others" without any explanations on what we're really doing.


> If it's not, I worry you're papering over another bug, or
> incorrect bspec sequence.
>
> Not opposed to the patch in the meanwhile, but it just feels a little
> unsafe to me.

I'll send some emails requesting for clarification. We certainly can't
merge PC8 without this patch or an equivalent fix.


>
> --
> Ben Widawsky, Intel Open Source Technology Center



-- 
Paulo Zanoni

Re: [PATCH igt] tests: add pc8

[Daniel Vetter]

On Mon, Aug 19, 2013 at 01:20:02PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> This test chekcs our code that enables Package C8+. The environment
> requirements for this test are quite complicated:
>   - The machine needs to be properly configured to reach PC8+ when
>     possible, which means all the power management policies and device
>     drivers must be properly configured.
>   - You need at least one output connected.
>   - You need the /dev/cpu/0/msr file available.
>   - You need the /dev/i2c-* files available.
> 
> v2: - Many many changes due to the latest review comments and rebase.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Applied and pushed with a small fixup patch on top to convert your new
testcase to the latest igt infrastructure changes.

Thanks, Daniel

> ---
>  tests/.gitignore  |   1 +
>  tests/Makefile.am |   1 +
>  tests/pc8.c       | 804 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 806 insertions(+)
>  create mode 100644 tests/pc8.c
> 
> diff --git a/tests/.gitignore b/tests/.gitignore
> index b5eb7a5..19f827a 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -93,6 +93,7 @@ getversion
>  kms_flip
>  kms_render
>  kms_setmode
> +pc8
>  prime_nv_api
>  prime_nv_pcopy
>  prime_nv_test
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index c724829..370a700 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -45,6 +45,7 @@ TESTS_progs_M = \
>  	kms_render \
>  	kms_setmode \
>  	$(NOUVEAU_TESTS_M) \
> +	pc8 \
>  	prime_self_import \
>  	$(NULL)
>  
> diff --git a/tests/pc8.c b/tests/pc8.c
> new file mode 100644
> index 0000000..06c54b7
> --- /dev/null
> +++ b/tests/pc8.c
> @@ -0,0 +1,804 @@
> +/*
> + * Copyright © 2013 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + *    Paulo Zanoni <paulo.r.zanoni@intel.com>
> + *
> + */
> +
> +#include <stdio.h>
> +#include <stdint.h>
> +#include <stdbool.h>
> +#include <string.h>
> +
> +#include <unistd.h>
> +#include <fcntl.h>
> +#include <dirent.h>
> +#include <sys/ioctl.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <linux/i2c.h>
> +#include <linux/i2c-dev.h>
> +
> +#include "drm.h"
> +#include "drmtest.h"
> +#include "intel_batchbuffer.h"
> +#include "intel_gpu_tools.h"
> +#include "i915_drm.h"
> +
> +#define MSR_PC8_RES	0x630
> +#define MSR_PC9_RES	0x631
> +#define MSR_PC10_RES	0x632
> +
> +#define MAX_CONNECTORS	32
> +#define MAX_ENCODERS	32
> +#define MAX_CRTCS	16
> +
> +int drm_fd, msr_fd;
> +struct mode_set_data ms_data;
> +
> +/* Stuff used when creating FBs and mode setting. */
> +struct mode_set_data {
> +	drmModeResPtr res;
> +	drmModeConnectorPtr connectors[MAX_CONNECTORS];
> +	drmModePropertyBlobPtr edids[MAX_CONNECTORS];
> +
> +	drm_intel_bufmgr *bufmgr;
> +	uint32_t devid;
> +};
> +
> +/* Stuff we query at different times so we can compare. */
> +struct compare_data {
> +	drmModeResPtr res;
> +	drmModeEncoderPtr encoders[MAX_ENCODERS];
> +	drmModeConnectorPtr connectors[MAX_CONNECTORS];
> +	drmModeCrtcPtr crtcs[MAX_CRTCS];
> +	drmModePropertyBlobPtr edids[MAX_CONNECTORS];
> +};
> +
> +struct compare_registers {
> +	/* We know these are lost */
> +	uint32_t arb_mode;
> +	uint32_t tilectl;
> +
> +	/* Stuff touched at init_clock_gating, so we can make sure we
> +	 * don't need to call it when reiniting. */
> +	uint32_t gen6_ucgctl2;
> +	uint32_t gen7_l3cntlreg1;
> +	uint32_t transa_chicken1;
> +
> +	uint32_t deier;
> +	uint32_t gtier;
> +
> +	uint32_t ddi_buf_trans_a_1;
> +	uint32_t ddi_buf_trans_b_5;
> +	uint32_t ddi_buf_trans_c_10;
> +	uint32_t ddi_buf_trans_d_15;
> +	uint32_t ddi_buf_trans_e_20;
> +};
> +
> +/* If the read fails, then the machine doesn't support PC8+ residencies. */
> +static bool supports_pc8_plus_residencies(void)
> +{
> +	int rc;
> +	uint64_t val;
> +
> +	rc = pread(msr_fd, &val, sizeof(uint64_t), MSR_PC8_RES);
> +	if (!rc == sizeof(val))
> +		return false;
> +	rc = pread(msr_fd, &val, sizeof(uint64_t), MSR_PC9_RES);
> +	if (!rc == sizeof(val))
> +		return false;
> +	rc = pread(msr_fd, &val, sizeof(uint64_t), MSR_PC10_RES);
> +	if (!rc == sizeof(val))
> +		return false;
> +
> +	return true;
> +}
> +
> +static uint64_t get_residency(uint32_t type)
> +{
> +	int rc;
> +	uint64_t ret;
> +
> +	rc = pread(msr_fd, &ret, sizeof(uint64_t), type);
> +	igt_assert(rc == sizeof(ret));
> +
> +	return ret;
> +}
> +
> +static bool pc8_plus_residency_changed(unsigned int timeout_sec)
> +{
> +	unsigned int i;
> +	uint64_t res_pc8, res_pc9, res_pc10;
> +	int to_sleep = 100 * 1000;
> +
> +	res_pc8 = get_residency(MSR_PC8_RES);
> +	res_pc9 = get_residency(MSR_PC9_RES);
> +	res_pc10 = get_residency(MSR_PC10_RES);
> +
> +	for (i = 0; i < timeout_sec * 1000 * 1000; i += to_sleep) {
> +		if (res_pc8 != get_residency(MSR_PC8_RES) ||
> +		    res_pc9 != get_residency(MSR_PC9_RES) ||
> +		    res_pc10 != get_residency(MSR_PC10_RES)) {
> +			return true;
> +		}
> +		usleep(to_sleep);
> +	}
> +
> +	return false;
> +}
> +
> +/* Checks not only if PC8+ is allowed, but also if we're reaching it.
> + * We call this when we expect this function to return quickly since PC8 is
> + * actually enabled, so the 30s timeout we use shouldn't matter. */
> +static bool pc8_plus_enabled(void)
> +{
> +	return pc8_plus_residency_changed(30);
> +}
> +
> +/* We call this when we expect PC8+ to be actually disabled, so we should not
> + * return until the 5s timeout expires. In other words: in the "happy case",
> + * every time we call this function the program will take 5s more to finish. */
> +static bool pc8_plus_disabled(void)
> +{
> +	return !pc8_plus_residency_changed(5);
> +}
> +
> +static void disable_all_screens(struct mode_set_data *data)
> +{
> +	int i, rc;
> +
> +	for (i = 0; i < data->res->count_crtcs; i++) {
> +		rc = drmModeSetCrtc(drm_fd, data->res->crtcs[i], -1, 0, 0,
> +				    NULL, 0, NULL);
> +		igt_assert(rc == 0);
> +	}
> +}
> +
> +static uint32_t create_fb(struct mode_set_data *data, int width, int height)
> +{
> +	struct kmstest_fb fb;
> +	cairo_t *cr;
> +	uint32_t buffer_id;
> +
> +	buffer_id = kmstest_create_fb(drm_fd, width, height, 32, 24, false,
> +				      &fb);
> +	cr = kmstest_get_cairo_ctx(drm_fd, &fb);
> +	kmstest_paint_test_pattern(cr, width, height);
> +	return buffer_id;
> +}
> +
> +static void enable_one_screen(struct mode_set_data *data)
> +{
> +	uint32_t crtc_id = 0, buffer_id = 0, connector_id = 0;
> +	drmModeModeInfoPtr mode = NULL;
> +	int i, rc;
> +
> +	for (i = 0; i < data->res->count_connectors; i++) {
> +		drmModeConnectorPtr c = data->connectors[i];
> +
> +		if (c->connection == DRM_MODE_CONNECTED && c->count_modes) {
> +			connector_id = c->connector_id;
> +			mode = &c->modes[0];
> +			break;
> +		}
> +	}
> +
> +	crtc_id = data->res->crtcs[0];
> +	buffer_id = create_fb(data, mode->hdisplay, mode->vdisplay);
> +
> +	igt_assert(crtc_id);
> +	igt_assert(buffer_id);
> +	igt_assert(connector_id);
> +	igt_assert(mode);
> +
> +	rc = drmModeSetCrtc(drm_fd, crtc_id, buffer_id, 0, 0, &connector_id,
> +			    1, mode);
> +	igt_assert(rc == 0);
> +}
> +
> +static drmModePropertyBlobPtr get_connector_edid(drmModeConnectorPtr connector,
> +						 int index)
> +{
> +	unsigned int i;
> +	drmModeObjectPropertiesPtr props;
> +	drmModePropertyBlobPtr ret = NULL;
> +
> +	props = drmModeObjectGetProperties(drm_fd, connector->connector_id,
> +					   DRM_MODE_OBJECT_CONNECTOR);
> +
> +	for (i = 0; i < props->count_props; i++) {
> +		drmModePropertyPtr prop = drmModeGetProperty(drm_fd,
> +							     props->props[i]);
> +
> +		if (strcmp(prop->name, "EDID") == 0) {
> +			igt_assert(prop->flags & DRM_MODE_PROP_BLOB);
> +			igt_assert(prop->count_blobs == 0);
> +			ret = drmModeGetPropertyBlob(drm_fd,
> +						     props->prop_values[i]);
> +		}
> +
> +		drmModeFreeProperty(prop);
> +	}
> +
> +	drmModeFreeObjectProperties(props);
> +	return ret;
> +}
> +
> +static void init_mode_set_data(struct mode_set_data *data)
> +{
> +	int i;
> +
> +	data->res = drmModeGetResources(drm_fd);
> +	igt_assert(data->res);
> +	igt_assert(data->res->count_connectors <= MAX_CONNECTORS);
> +
> +	for (i = 0; i < data->res->count_connectors; i++) {
> +		data->connectors[i] = drmModeGetConnector(drm_fd,
> +						data->res->connectors[i]);
> +		data->edids[i] = get_connector_edid(data->connectors[i], i);
> +	}
> +
> +	data->bufmgr = drm_intel_bufmgr_gem_init(drm_fd, 4096);
> +	data->devid = intel_get_drm_devid(drm_fd);
> +
> +	do_or_die(igt_set_vt_graphics_mode());
> +	drm_intel_bufmgr_gem_enable_reuse(data->bufmgr);
> +}
> +
> +static void fini_mode_set_data(struct mode_set_data *data)
> +{
> +	int i;
> +
> +	drm_intel_bufmgr_destroy(data->bufmgr);
> +
> +	for (i = 0; i < data->res->count_connectors; i++) {
> +		drmModeFreeConnector(data->connectors[i]);
> +		drmModeFreePropertyBlob(data->edids[i]);
> +	}
> +	drmModeFreeResources(data->res);
> +}
> +
> +static void get_drm_info(struct compare_data *data)
> +{
> +	int i;
> +
> +	data->res = drmModeGetResources(drm_fd);
> +	igt_assert(data->res);
> +
> +	igt_assert(data->res->count_connectors <= MAX_CONNECTORS);
> +	igt_assert(data->res->count_encoders <= MAX_ENCODERS);
> +	igt_assert(data->res->count_crtcs <= MAX_CRTCS);
> +
> +	for (i = 0; i < data->res->count_connectors; i++) {
> +		data->connectors[i] = drmModeGetConnector(drm_fd,
> +						data->res->connectors[i]);
> +		data->edids[i] = get_connector_edid(data->connectors[i], i);
> +	}
> +	for (i = 0; i < data->res->count_encoders; i++)
> +		data->encoders[i] = drmModeGetEncoder(drm_fd,
> +						data->res->encoders[i]);
> +	for (i = 0; i < data->res->count_crtcs; i++)
> +		data->crtcs[i] = drmModeGetCrtc(drm_fd, data->res->crtcs[i]);
> +}
> +
> +static void get_registers(struct compare_registers *data)
> +{
> +	intel_register_access_init(intel_get_pci_device(), 0);
> +	data->arb_mode = INREG(0x4030);
> +	data->tilectl = INREG(0x101000);
> +	data->gen6_ucgctl2 = INREG(0x9404);
> +	data->gen7_l3cntlreg1 = INREG(0xB0C1);
> +	data->transa_chicken1 = INREG(0xF0060);
> +	data->deier = INREG(0x4400C);
> +	data->gtier = INREG(0x4401C);
> +	data->ddi_buf_trans_a_1 = INREG(0x64E00);
> +	data->ddi_buf_trans_b_5 = INREG(0x64E70);
> +	data->ddi_buf_trans_c_10 = INREG(0x64EE0);
> +	data->ddi_buf_trans_d_15 = INREG(0x64F58);
> +	data->ddi_buf_trans_e_20 = INREG(0x64FCC);
> +	intel_register_access_fini();
> +}
> +
> +static void free_drm_info(struct compare_data *data)
> +{
> +	int i;
> +
> +	for (i = 0; i < data->res->count_connectors; i++) {
> +		drmModeFreeConnector(data->connectors[i]);
> +		drmModeFreePropertyBlob(data->edids[i]);
> +	}
> +	for (i = 0; i < data->res->count_encoders; i++)
> +		drmModeFreeEncoder(data->encoders[i]);
> +	for (i = 0; i < data->res->count_crtcs; i++)
> +		drmModeFreeCrtc(data->crtcs[i]);
> +
> +	drmModeFreeResources(data->res);
> +}
> +
> +#define COMPARE(d1, d2, data) igt_assert(d1->data == d2->data)
> +#define COMPARE_ARRAY(d1, d2, size, data) do { \
> +	for (i = 0; i < size; i++) \
> +		igt_assert(d1->data[i] == d2->data[i]); \
> +} while (0)
> +
> +static void assert_drm_resources_equal(struct compare_data *d1,
> +				       struct compare_data *d2)
> +{
> +	COMPARE(d1, d2, res->count_connectors);
> +	COMPARE(d1, d2, res->count_encoders);
> +	COMPARE(d1, d2, res->count_crtcs);
> +	COMPARE(d1, d2, res->min_width);
> +	COMPARE(d1, d2, res->max_width);
> +	COMPARE(d1, d2, res->min_height);
> +	COMPARE(d1, d2, res->max_height);
> +}
> +
> +static void assert_modes_equal(drmModeModeInfoPtr m1, drmModeModeInfoPtr m2)
> +{
> +	COMPARE(m1, m2, clock);
> +	COMPARE(m1, m2, hdisplay);
> +	COMPARE(m1, m2, hsync_start);
> +	COMPARE(m1, m2, hsync_end);
> +	COMPARE(m1, m2, htotal);
> +	COMPARE(m1, m2, hskew);
> +	COMPARE(m1, m2, vdisplay);
> +	COMPARE(m1, m2, vsync_start);
> +	COMPARE(m1, m2, vsync_end);
> +	COMPARE(m1, m2, vtotal);
> +	COMPARE(m1, m2, vscan);
> +	COMPARE(m1, m2, vrefresh);
> +	COMPARE(m1, m2, flags);
> +	COMPARE(m1, m2, type);
> +	igt_assert(strcmp(m1->name, m2->name) == 0);
> +}
> +
> +static void assert_drm_connectors_equal(drmModeConnectorPtr c1,
> +					drmModeConnectorPtr c2)
> +{
> +	int i;
> +
> +	COMPARE(c1, c2, connector_id);
> +	COMPARE(c1, c2, connector_type);
> +	COMPARE(c1, c2, connector_type_id);
> +	COMPARE(c1, c2, mmWidth);
> +	COMPARE(c1, c2, mmHeight);
> +	COMPARE(c1, c2, count_modes);
> +	COMPARE(c1, c2, count_props);
> +	COMPARE(c1, c2, count_encoders);
> +	COMPARE_ARRAY(c1, c2, c1->count_props, props);
> +	COMPARE_ARRAY(c1, c2, c1->count_encoders, encoders);
> +
> +	for (i = 0; i < c1->count_modes; i++)
> +		assert_modes_equal(&c1->modes[0], &c2->modes[0]);
> +}
> +
> +static void assert_drm_encoders_equal(drmModeEncoderPtr e1,
> +				      drmModeEncoderPtr e2)
> +{
> +	COMPARE(e1, e2, encoder_id);
> +	COMPARE(e1, e2, encoder_type);
> +	COMPARE(e1, e2, possible_crtcs);
> +	COMPARE(e1, e2, possible_clones);
> +}
> +
> +static void assert_drm_crtcs_equal(drmModeCrtcPtr c1, drmModeCrtcPtr c2)
> +{
> +	COMPARE(c1, c2, crtc_id);
> +}
> +
> +static void assert_drm_edids_equal(drmModePropertyBlobPtr e1,
> +				   drmModePropertyBlobPtr e2)
> +{
> +	if (!e1 && !e2)
> +		return;
> +	igt_assert(e1 && e2);
> +
> +	COMPARE(e1, e2, id);
> +	COMPARE(e1, e2, length);
> +
> +	igt_assert(memcmp(e1->data, e2->data, e1->length) == 0);
> +}
> +
> +static void compare_registers(struct compare_registers *d1,
> +			      struct compare_registers *d2)
> +{
> +	COMPARE(d1, d2, gen6_ucgctl2);
> +	COMPARE(d1, d2, gen7_l3cntlreg1);
> +	COMPARE(d1, d2, transa_chicken1);
> +	COMPARE(d1, d2, arb_mode);
> +	COMPARE(d1, d2, tilectl);
> +	COMPARE(d1, d2, arb_mode);
> +	COMPARE(d1, d2, tilectl);
> +	COMPARE(d1, d2, gtier);
> +	COMPARE(d1, d2, ddi_buf_trans_a_1);
> +	COMPARE(d1, d2, ddi_buf_trans_b_5);
> +	COMPARE(d1, d2, ddi_buf_trans_c_10);
> +	COMPARE(d1, d2, ddi_buf_trans_d_15);
> +	COMPARE(d1, d2, ddi_buf_trans_e_20);
> +}
> +
> +static void assert_drm_infos_equal(struct compare_data *d1,
> +				   struct compare_data *d2)
> +{
> +	int i;
> +
> +	assert_drm_resources_equal(d1, d2);
> +
> +	for (i = 0; i < d1->res->count_connectors; i++) {
> +		assert_drm_connectors_equal(d1->connectors[i],
> +					    d2->connectors[i]);
> +		assert_drm_edids_equal(d1->edids[i], d2->edids[i]);
> +	}
> +
> +	for (i = 0; i < d1->res->count_encoders; i++)
> +		assert_drm_encoders_equal(d1->encoders[i], d2->encoders[i]);
> +
> +	for (i = 0; i < d1->res->count_crtcs; i++)
> +		assert_drm_crtcs_equal(d1->crtcs[i], d2->crtcs[i]);
> +}
> +
> +static void blt_color_fill(struct intel_batchbuffer *batch,
> +			   drm_intel_bo *buf,
> +			   const unsigned int pages)
> +{
> +	const unsigned short height = pages/4;
> +	const unsigned short width = 4096;
> +
> +	BEGIN_BATCH(5);
> +	OUT_BATCH(COLOR_BLT_CMD | COLOR_BLT_WRITE_ALPHA | COLOR_BLT_WRITE_RGB);
> +	OUT_BATCH((3 << 24) | /* 32 Bit Color */
> +		  0xF0 | /* Raster OP copy background register */
> +		  0); /* Dest pitch is 0 */
> +	OUT_BATCH(width << 16 | height);
> +	OUT_RELOC(buf, I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER, 0);
> +	OUT_BATCH(rand()); /* random pattern */
> +	ADVANCE_BATCH();
> +}
> +
> +static void test_batch(struct mode_set_data *data)
> +{
> +	struct intel_batchbuffer *batch;
> +	int64_t timeout_ns = 1000 * 1000 * 1000 * 2;
> +	drm_intel_bo *dst;
> +	int i, rc;
> +
> +	dst = drm_intel_bo_alloc(data->bufmgr, "dst", (8 << 20), 4096);
> +
> +	batch = intel_batchbuffer_alloc(data->bufmgr,
> +					intel_get_drm_devid(drm_fd));
> +
> +	for (i = 0; i < 1000; i++)
> +		blt_color_fill(batch, dst, ((8 << 20) >> 12));
> +
> +	rc = drm_intel_gem_bo_wait(dst, timeout_ns);
> +	igt_assert(!rc);
> +
> +	intel_batchbuffer_free(batch);
> +}
> +
> +/* We could check the checksum too, but just the header is probably enough. */
> +static bool edid_is_valid(const unsigned char *edid)
> +{
> +	char edid_header[] = {
> +		0x0, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0x0,
> +	};
> +
> +	return (memcmp(edid, edid_header, sizeof(edid_header)) == 0);
> +}
> +
> +static int count_drm_valid_edids(struct mode_set_data *data)
> +{
> +	int i, ret = 0;
> +
> +	for (i = 0; i < data->res->count_connectors; i++)
> +		if (data->edids[i] && edid_is_valid(data->edids[i]->data))
> +			ret++;
> +	return ret;
> +}
> +
> +static int count_drm_edp_outputs(struct mode_set_data *data)
> +{
> +	int i, ret = 0;
> +
> +	for (i = 0; i < data->res->count_connectors; i++)
> +		if (data->connectors[i]->connector_type ==
> +		    DRM_MODE_CONNECTOR_eDP)
> +			ret++;
> +	return ret;
> +}
> +
> +static bool i2c_edid_is_valid(int fd)
> +{
> +	int rc;
> +	unsigned char edid[128] = {};
> +	struct i2c_msg msgs[] = {
> +		{ /* Start at 0. */
> +			.addr = 0x50,
> +			.flags = 0,
> +			.len = 1,
> +			.buf = edid,
> +		}, { /* Now read the EDID. */
> +			.addr = 0x50,
> +			.flags = I2C_M_RD,
> +			.len = 128,
> +			.buf = edid,
> +		}
> +	};
> +	struct i2c_rdwr_ioctl_data msgset = {
> +		.msgs = msgs,
> +		.nmsgs = 2,
> +	};
> +
> +	rc = ioctl(fd, I2C_RDWR, &msgset);
> +	return (rc >= 0) ? edid_is_valid(edid) : false;
> +}
> +
> +static int count_i2c_valid_edids(void)
> +{
> +	int fd, ret = 0;
> +	DIR *dir;
> +
> +	struct dirent *dirent;
> +	char full_name[32];
> +
> +	dir = opendir("/dev/");
> +	igt_assert(dir);
> +
> +	while ((dirent = readdir(dir))) {
> +		if (strncmp(dirent->d_name, "i2c-", 4) == 0) {
> +			snprintf(full_name, 32, "/dev/%s", dirent->d_name);
> +			fd = open(full_name, O_RDWR);
> +			igt_assert(fd != -1);
> +			if (i2c_edid_is_valid(fd))
> +				ret++;
> +			close(fd);
> +		}
> +	}
> +
> +	closedir(dir);
> +
> +	return 0;
> +}
> +
> +/* We run this test when all outputs are turned off. When eDP panels are turned
> + * off, I2C doesn't work on them and we get a nice dmesg backtrace. So count how
> + * many eDP panels we have and subtract them. */
> +static bool test_i2c(struct mode_set_data *data)
> +{
> +	int i2c_edids = count_i2c_valid_edids();
> +	int drm_edids = count_drm_valid_edids(data);
> +	int edps = count_drm_edp_outputs(data);
> +
> +	return i2c_edids + edps == drm_edids;
> +}
> +
> +static bool setup_environment(void)
> +{
> +	int i2c_dev_files;
> +	DIR *dev_dir;
> +	struct dirent *dirent;
> +
> +	drm_fd = drm_open_any();
> +	if (drm_fd <= 0)
> +		return false;
> +
> +	init_mode_set_data(&ms_data);
> +
> +	/* Only Haswell supports the PC8 feature. */
> +	if (!IS_HASWELL(ms_data.devid)) {
> +		printf("PC8+ feature only supported on Haswell.\n");
> +		return false;
> +	}
> +
> +	/* Make sure our Kernel supports MSR and the module is loaded. */
> +	msr_fd = open("/dev/cpu/0/msr", O_RDONLY);
> +	if (msr_fd == -1) {
> +		printf("Can't open /dev/cpu/0/msr.\n");
> +		return false;
> +	}
> +
> +	/* Make sure the /dev/i2c-* files exist. */
> +	dev_dir = opendir("/dev");
> +	if (!dev_dir) {
> +		printf("Can't open /dev.\n");
> +		return false;
> +	}
> +	i2c_dev_files = 0;
> +	while ((dirent = readdir(dev_dir))) {
> +		if (strncmp(dirent->d_name, "i2c-", 4) == 0)
> +			i2c_dev_files++;
> +	}
> +	closedir(dev_dir);
> +	if (!i2c_dev_files) {
> +		printf("No /dev/i2c-* files.\n");
> +		return false;
> +	}
> +
> +	/* Non-ULT machines don't support PC8+. */
> +	if (!supports_pc8_plus_residencies()) {
> +		printf("Machine doesn't support PC8+ residencies.\n");
> +		return false;
> +	}
> +
> +	/* Make sure PC8+ residencies move! */
> +	disable_all_screens(&ms_data);
> +	if (!pc8_plus_enabled()) {
> +		printf("Machine is not reaching PC8+ states, please check its "
> +		       "configuration.\n");
> +		return false;
> +	}
> +
> +	/* Make sure PC8+ residencies stop! */
> +	enable_one_screen(&ms_data);
> +	if (!pc8_plus_disabled()) {
> +		printf("PC8+ residency didn't stop with screen enabled.\n");
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +static void teardown_environment(void)
> +{
> +	fini_mode_set_data(&ms_data);
> +	drmClose(drm_fd);
> +	close(msr_fd);
> +}
> +
> +/* Test of the DRM resources reported by the IOCTLs are still the same. This
> + * ensures we still see the monitors with the same eyes. We get the EDIDs and
> + * compare them, which ensures we use DP AUX or GMBUS depending on what's
> + * connected. */
> +static void drm_resources_equal_subtest(void)
> +{
> +	struct compare_data pre_pc8, during_pc8, post_pc8;
> +
> +	printf("Checking the if the DRM resources match.\n");
> +
> +	enable_one_screen(&ms_data);
> +	igt_assert(pc8_plus_disabled());
> +	get_drm_info(&pre_pc8);
> +	igt_assert(pc8_plus_disabled());
> +
> +	disable_all_screens(&ms_data);
> +	igt_assert(pc8_plus_enabled());
> +	get_drm_info(&during_pc8);
> +	igt_assert(pc8_plus_enabled());
> +
> +	enable_one_screen(&ms_data);
> +	igt_assert(pc8_plus_disabled());
> +	get_drm_info(&post_pc8);
> +	igt_assert(pc8_plus_disabled());
> +
> +	assert_drm_infos_equal(&pre_pc8, &during_pc8);
> +	assert_drm_infos_equal(&pre_pc8, &post_pc8);
> +
> +	free_drm_info(&pre_pc8);
> +	free_drm_info(&during_pc8);
> +	free_drm_info(&post_pc8);
> +}
> +
> +/* Make sure interrupts are working. */
> +static void batch_subtest(void)
> +{
> +	printf("Testing batchbuffers.\n");
> +
> +	enable_one_screen(&ms_data);
> +	igt_assert(pc8_plus_disabled());
> +
> +	disable_all_screens(&ms_data);
> +	igt_assert(pc8_plus_enabled());
> +	test_batch(&ms_data);
> +	igt_assert(pc8_plus_enabled());
> +}
> +
> +/* Try to use raw I2C, which also needs interrupts. */
> +static void i2c_subtest(void)
> +{
> +	printf("Testing raw i2c protocol calls.\n");
> +
> +	enable_one_screen(&ms_data);
> +	igt_assert(pc8_plus_disabled());
> +
> +	disable_all_screens(&ms_data);
> +	igt_assert(pc8_plus_enabled());
> +	igt_assert(test_i2c(&ms_data));
> +	igt_assert(pc8_plus_enabled());
> +
> +	enable_one_screen(&ms_data);
> +}
> +
> +/* Make us enter/leave PC8+ many times. */
> +static void stress_test(void)
> +{
> +	int i;
> +
> +	printf("Stress testing.\n");
> +
> +	for (i = 0; i < 100; i++) {
> +		disable_all_screens(&ms_data);
> +		igt_assert(pc8_plus_enabled());
> +		test_batch(&ms_data);
> +		igt_assert(pc8_plus_enabled());
> +	}
> +}
> +
> +/* Just reading/writing registers from outside the Kernel is not really a safe
> + * thing to do on Haswell, so don't do this test on the default case. */
> +static void register_compare_subtest(void)
> +{
> +	struct compare_registers pre_pc8, post_pc8;
> +
> +	printf("Testing register compare.\n");
> +
> +	enable_one_screen(&ms_data);
> +	igt_assert(pc8_plus_disabled());
> +	get_registers(&pre_pc8);
> +	igt_assert(pc8_plus_disabled());
> +
> +	disable_all_screens(&ms_data);
> +	igt_assert(pc8_plus_enabled());
> +	enable_one_screen(&ms_data);
> +	igt_assert(pc8_plus_disabled());
> +	/* Wait for the registers to be restored. */
> +	sleep(1);
> +	get_registers(&post_pc8);
> +	igt_assert(pc8_plus_disabled());
> +
> +	compare_registers(&pre_pc8, &post_pc8);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	bool do_register_compare = false;
> +
> +	if (argc > 1 && strcmp(argv[1], "--do-register-compare") == 0)
> +		do_register_compare = true;
> +
> +	igt_subtest_init(argc, argv);
> +
> +	/* Skip instead of failing in case the machine is not prepared to reach
> +	 * PC8+. We don't want bug reports from cases where the machine is just
> +	 * not properly configured. */
> +	printf("Checking if the environment is properly configured.\n");
> +	if (!setup_environment())
> +		return 77;
> +
> +	igt_subtest("drm-resources-equal")
> +		drm_resources_equal_subtest();
> +	igt_subtest("batch")
> +		batch_subtest();
> +	igt_subtest("i2c")
> +		i2c_subtest();
> +	igt_subtest("stress-test")
> +		stress_test();
> +	if (do_register_compare)
> +		igt_subtest("register-compare")
> +			register_compare_subtest();
> +
> +	teardown_environment();
> +
> +	printf("Done!\n");
> +	return 0;
> +}
> -- 
> 1.8.1.2
> 
> _______________________________________________
> 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

Re: [PATCH 2/6] drm/i915: fix SDEIMR assertion when disabling LCPLL

[Rodrigo Vivi]

ok, if we don't care about this hotplug bits state I liked the simplification.

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

On Mon, Aug 19, 2013 at 1:18 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> This was causing WARNs in one machine, so instead of trying to guess
> exactly which hotplug bits should exist, just do the test on the
> non-HPD bits. We don't care about the state of the hotplug bits, we
> just care about the others, that need to be 1.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8c63d8e..b9a4d8b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5930,11 +5930,7 @@ static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv)
>         struct intel_ddi_plls *plls = &dev_priv->ddi_plls;
>         struct intel_crtc *crtc;
>         unsigned long irqflags;
> -       uint32_t val, pch_hpd_mask;
> -
> -       pch_hpd_mask = SDE_PORTB_HOTPLUG_CPT | SDE_PORTC_HOTPLUG_CPT;
> -       if (!(dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE))
> -               pch_hpd_mask |= SDE_PORTD_HOTPLUG_CPT | SDE_CRT_HOTPLUG_CPT;
> +       uint32_t val;
>
>         list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head)
>                 WARN(crtc->base.enabled, "CRTC for pipe %c enabled\n",
> @@ -5960,7 +5956,7 @@ static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv)
>         WARN((val & ~DE_PCH_EVENT_IVB) != val,
>              "Unexpected DEIMR bits enabled: 0x%x\n", val);
>         val = I915_READ(SDEIMR);
> -       WARN((val & ~pch_hpd_mask) != val,
> +       WARN((val | SDE_HOTPLUG_MASK_CPT) != 0xffffffff,
>              "Unexpected SDEIMR bits enabled: 0x%x\n", val);
>         spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  }
> --
> 1.8.1.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

Re: [PATCH 4/6] drm/i915: add i915_pc8_status debugfs file

[Rodrigo Vivi]

I haven't reviewed the patches that add pc8 itself, but I can assume
it is correct since it was already rv-b Chris.

So:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

On Mon, Aug 19, 2013 at 1:18 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Make it print the value of the variables on the PC8 struct.
>
> v2: Update to recent renames and add the new fields.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 4785d8c..4d836f1 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1769,6 +1769,30 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>         return 0;
>  }
>
> +static int i915_pc8_status(struct seq_file *m, void *unused)
> +{
> +       struct drm_info_node *node = (struct drm_info_node *) m->private;
> +       struct drm_device *dev = node->minor->dev;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +       if (!IS_HASWELL(dev)) {
> +               seq_puts(m, "not supported\n");
> +               return 0;
> +       }
> +
> +       mutex_lock(&dev_priv->pc8.lock);
> +       seq_printf(m, "Requirements met: %s\n",
> +                  yesno(dev_priv->pc8.requirements_met));
> +       seq_printf(m, "GPU idle: %s\n", yesno(dev_priv->pc8.gpu_idle));
> +       seq_printf(m, "Disable count: %d\n", dev_priv->pc8.disable_count);
> +       seq_printf(m, "IRQs disabled: %s\n",
> +                  yesno(dev_priv->pc8.irqs_disabled));
> +       seq_printf(m, "Enabled: %s\n", yesno(dev_priv->pc8.enabled));
> +       mutex_unlock(&dev_priv->pc8.lock);
> +
> +       return 0;
> +}
> +
>  static int
>  i915_wedged_get(void *data, u64 *val)
>  {
> @@ -2208,6 +2232,7 @@ static struct drm_info_list i915_debugfs_list[] = {
>         {"i915_dpio", i915_dpio_info, 0},
>         {"i915_llc", i915_llc, 0},
>         {"i915_edp_psr_status", i915_edp_psr_status, 0},
> +       {"i915_pc8_status", i915_pc8_status, 0},
>  };
>  #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
>
> --
> 1.8.1.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

Re: [PATCH 5/6] drm/i915: add i915.pc8_timeout function

[Rodrigo Vivi]

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

On Mon, Aug 19, 2013 at 1:18 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> We currently only enter PC8+ after all its required conditions are
> met, there's no rendering, and we stay like that for at least 5
> seconds.
>
> I chose "5 seconds" because this value is conservative and won't make
> us enter/leave PC8+ thousands of times after the screen is off: some
> desktop environments have applications that wake up and do rendering
> every 1-3 seconds, even when the screen is off and the machine is
> completely idle.
>
> But when I was testing my PC8+ patches I set the default value to
> 100ms so I could use the bad-behaving desktop environments to
> stress-test my patches. I also thought it would be a good idea to ask
> our power management team to test different values, but I'm pretty
> sure they would ask me for an easy way to change the timeout. So to
> help these 2 cases I decided to create an option that would make it
> easier to change the default value. I also expect people making
> specific products that use our driver could try to find the perfect
> timeout for them.
>
> Anyway, fixing the bad-behaving applications will always lead to
> better power savings than just changing the timeout value: you need to
> stop waking the Kernel, not quickly put it back to sleep again after
> you wake it for nothing. Bad sleep leads to bad mood!
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c      | 4 ++++
>  drivers/gpu/drm/i915/i915_drv.h      | 1 +
>  drivers/gpu/drm/i915/intel_display.c | 2 +-
>  3 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index f197362..ad28a72 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -145,6 +145,10 @@ int i915_enable_pc8 __read_mostly = 0;
>  module_param_named(enable_pc8, i915_enable_pc8, int, 0600);
>  MODULE_PARM_DESC(enable_pc8, "Enable support for low power package C states (PC8+) (default: false)");
>
> +int i915_pc8_timeout __read_mostly = 5000;
> +module_param_named(pc8_timeout, i915_pc8_timeout, int, 0600);
> +MODULE_PARM_DESC(pc8_timeout, "Number of msecs of idleness required to enter PC8+ (default: 5000)");
> +
>  bool i915_prefault_disable __read_mostly;
>  module_param_named(prefault_disable, i915_prefault_disable, bool, 0600);
>  MODULE_PARM_DESC(prefault_disable,
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 34c5af5..3138083 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1710,6 +1710,7 @@ extern int i915_disable_power_well __read_mostly;
>  extern int i915_enable_ips __read_mostly;
>  extern bool i915_fastboot __read_mostly;
>  extern int i915_enable_pc8 __read_mostly;
> +extern int i915_pc8_timeout __read_mostly;
>  extern bool i915_prefault_disable __read_mostly;
>
>  extern int i915_suspend(struct drm_device *dev, pm_message_t state);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7d5b01a..ba3bdd5 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6099,7 +6099,7 @@ static void __hsw_enable_package_c8(struct drm_i915_private *dev_priv)
>                 return;
>
>         schedule_delayed_work(&dev_priv->pc8.enable_work,
> -                             msecs_to_jiffies(5 * 1000));
> +                             msecs_to_jiffies(i915_pc8_timeout));
>  }
>
>  static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv)
> --
> 1.8.1.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

Re: [PATCH 6/6] drm/i915: enable Package C8+ by default

[Rodrigo Vivi]

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

On Mon, Aug 19, 2013 at 1:18 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> This should be working, so enable it by default. Also easy to revert.
>
> v2: Rebase, s/allow/enable/.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index ad28a72..735dd56 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -141,9 +141,9 @@ module_param_named(fastboot, i915_fastboot, bool, 0600);
>  MODULE_PARM_DESC(fastboot, "Try to skip unnecessary mode sets at boot time "
>                  "(default: false)");
>
> -int i915_enable_pc8 __read_mostly = 0;
> +int i915_enable_pc8 __read_mostly = 1;
>  module_param_named(enable_pc8, i915_enable_pc8, int, 0600);
> -MODULE_PARM_DESC(enable_pc8, "Enable support for low power package C states (PC8+) (default: false)");
> +MODULE_PARM_DESC(enable_pc8, "Enable support for low power package C states (PC8+) (default: true)");
>
>  int i915_pc8_timeout __read_mostly = 5000;
>  module_param_named(pc8_timeout, i915_pc8_timeout, int, 0600);
> --
> 1.8.1.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

Re: [PATCH 6/6] drm/i915: enable Package C8+ by default

[Daniel Vetter]

On Wed, Aug 21, 2013 at 12:43:54PM -0300, Rodrigo Vivi wrote:
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> 
> On Mon, Aug 19, 2013 at 1:18 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > This should be working, so enable it by default. Also easy to revert.
> >
> > v2: Rebase, s/allow/enable/.
> >
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Series merged to dinq with a small note for the first patch that the
discussion with hw guys is still ongoing.
-Daniel

> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index ad28a72..735dd56 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -141,9 +141,9 @@ module_param_named(fastboot, i915_fastboot, bool, 0600);
> >  MODULE_PARM_DESC(fastboot, "Try to skip unnecessary mode sets at boot time "
> >                  "(default: false)");
> >
> > -int i915_enable_pc8 __read_mostly = 0;
> > +int i915_enable_pc8 __read_mostly = 1;
> >  module_param_named(enable_pc8, i915_enable_pc8, int, 0600);
> > -MODULE_PARM_DESC(enable_pc8, "Enable support for low power package C states (PC8+) (default: false)");
> > +MODULE_PARM_DESC(enable_pc8, "Enable support for low power package C states (PC8+) (default: true)");
> >
> >  int i915_pc8_timeout __read_mostly = 5000;
> >  module_param_named(pc8_timeout, i915_pc8_timeout, int, 0600);
> > --
> > 1.8.1.2
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
> _______________________________________________
> 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