[PATCH 0/8] Allow Package C8+

[PATCH 0/8] Allow Package C8+

[Paulo Zanoni]

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

Hi

Some time ago I sent the first versions of patches to allow PC8+ and received
some feedback from Daniel. Based on his feedback and subsequent conversations,
we agreed that the right way to go would be to modify our "irq_uninstall"
function so it would allow uninstalling "everything but PCH HPD and master".
Then when we allow PC8+ we'd call the modified irq_uninstall, and then when we
disallow PC8+ we'call irq_postinstall.

So I started studying our interrupt code and noticed that if we wanted to allow
irq_uninstall/irq_postinstall multiple times on our code we'd have to change a
lot of stuff. So I wrote that code and also fixed other issues I also
identified. Some of these patches were already sent to the mailing list (and
even merged), but the part that allows us to call irq_uninstall+irq_postinstall
multiple times was not sent to the list yet.

In the end I felt unhappy with the resulting code: it looked ugly and still
didn't look bullet-proof (disable_irq is not cool). So I had a new idea: instead
of completely disabling the interrupts I decided to only disable the IMR
registers. We already touch the IMR registers everywhere on our code and avoid
the save/restore procedures on the IRQ handlers, so it should be safe to deal
with them. I added a small save/restore procedure for the IMR registers and
also added some WARNs that will tell us in case anybody tries to touch those
registers while PC8 is allowed. Even if we're still using a small register
save/restore file, I think this solution is much cleaner than the original patch
I proposed, so maybe Daniel thinks it's acceptable.

A small notice about the regfile: if you take a close look you'll see that we
also save/restore GTIER. This is because the GTIER register is reset when we
enter PC8. It should be safe to save/restore GTIER since it's only written at
our preinstall/postinstall/uninstall functions.

Patch 3 is the big patch which you should take a look. I couldn't find a nice
way to break it into more patches without writing patches that would just add
static functions that no one calls. It's probably easier to review this patch as
a whole than to review each piece of the puzzle in separate. But I'm open to
suggestions here. The other patches are very small and self-explanatory.

Notice that I won't just discard all the IRQ cleanups I already wrote, I will
still send them. I will just skip the ugly controversial patches that were only
useful for the PC8 work.

Thanks,
Paulo


Paulo Zanoni (8):
  drm/i915: add the FCLK case to intel_ddi_get_cdclk_freq
  drm/i915: don't disable/reenable IVB error interrupts when not needed
  drm/i915: allow package C8+ states on Haswell (disabled)
  drm/i915: avoid waking up from PC8 on DP AUX operations
  drm/i915: avoid waking up from PC8 on GMBUS operations
  drm/i915: silence message about the DDI buffers
  drm/i915: add i915_pc8_status debugfs file
  drm/i915: allow Package C8+ by default

 drivers/gpu/drm/i915/i915_debugfs.c  |  21 +++++++
 drivers/gpu/drm/i915/i915_dma.c      |   4 ++
 drivers/gpu/drm/i915/i915_drv.c      |   4 ++
 drivers/gpu/drm/i915/i915_drv.h      |  23 +++++++
 drivers/gpu/drm/i915/i915_irq.c      |  73 ++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_ddi.c     |  13 ++--
 drivers/gpu/drm/i915/intel_display.c | 112 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dp.c      |  25 +++++++-
 drivers/gpu/drm/i915/intel_drv.h     |   6 ++
 drivers/gpu/drm/i915/intel_i2c.c     |  73 ++++++++++++++++-------
 drivers/gpu/drm/i915/intel_pm.c      |  15 +++++
 11 files changed, 336 insertions(+), 33 deletions(-)

-- 
1.8.1.2

[PATCH 1/8] drm/i915: add the FCLK case to intel_ddi_get_cdclk_freq

[Paulo Zanoni]

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

We already have code to disable LCPLL and switch to FCLK, so we need this too.
We still don't call the code to disable LCPLL, but we'll call it when we add
support for Package C8+.

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

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index b361c08..9e77e32 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1153,10 +1153,13 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
 
 int intel_ddi_get_cdclk_freq(struct drm_i915_private *dev_priv)
 {
-	if (I915_READ(HSW_FUSE_STRAP) & HSW_CDCLK_LIMIT)
+	uint32_t lcpll = I915_READ(LCPLL_CTL);
+
+	if (lcpll & LCPLL_CD_SOURCE_FCLK)
+		return 800000;
+	else if (I915_READ(HSW_FUSE_STRAP) & HSW_CDCLK_LIMIT)
 		return 450000;
-	else if ((I915_READ(LCPLL_CTL) & LCPLL_CLK_FREQ_MASK) ==
-		 LCPLL_CLK_FREQ_450)
+	else if ((lcpll & LCPLL_CLK_FREQ_MASK) == LCPLL_CLK_FREQ_450)
 		return 450000;
 	else if (IS_ULT(dev_priv->dev))
 		return 337500;
-- 
1.8.1.2

[PATCH 2/8] drm/i915: don't disable/reenable IVB error interrupts when not needed

[Paulo Zanoni]

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

If the error interrupts are already disabled, don't disable and
reenable them. This is going to be needed when we're in PC8+, where
all the interrupts are disabled so we won't risk re-enabling
DE_ERR_INT_IVB.

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 6a1c207..2b0690a 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1309,6 +1309,7 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
 	u32 de_iir, gt_iir, de_ier, sde_ier = 0;
 	irqreturn_t ret = IRQ_NONE;
+	bool err_int_reenable = false;
 
 	atomic_inc(&dev_priv->irq_received);
 
@@ -1337,7 +1338,10 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
 	 * handler. */
 	if (IS_HASWELL(dev)) {
 		spin_lock(&dev_priv->irq_lock);
-		ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
+		if (!(I915_READ(DEIMR) & DE_ERR_INT_IVB)) {
+			ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
+			err_int_reenable = true;
+		}
 		spin_unlock(&dev_priv->irq_lock);
 	}
 
@@ -1373,7 +1377,7 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
 		}
 	}
 
-	if (IS_HASWELL(dev)) {
+	if (err_int_reenable) {
 		spin_lock(&dev_priv->irq_lock);
 		if (ivb_can_enable_err_int(dev))
 			ironlake_enable_display_irq(dev_priv, DE_ERR_INT_IVB);
-- 
1.8.1.2

[PATCH 3/8] 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.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c      |   4 ++
 drivers/gpu/drm/i915/i915_drv.c      |   4 ++
 drivers/gpu/drm/i915/i915_drv.h      |  23 +++++++
 drivers/gpu/drm/i915/i915_irq.c      |  65 ++++++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c | 112 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dp.c      |   3 +
 drivers/gpu/drm/i915/intel_drv.h     |   6 ++
 drivers/gpu/drm/i915/intel_i2c.c     |   2 +
 drivers/gpu/drm/i915/intel_pm.c      |  15 +++++
 9 files changed, 234 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index f48f1c4..1f87dc2 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1483,6 +1483,10 @@ 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.allowing = false;
+	dev_priv->pc8.forbid_refcnt = 1;
+
 	i915_dump_device_info(dev_priv);
 
 	INIT_LIST_HEAD(&dev_priv->vm_list);
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 01d63a0..6df516b 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_allow_pc8 __read_mostly = 0;
+module_param_named(allow_pc8, i915_allow_pc8, int, 0600);
+MODULE_PARM_DESC(allow_pc8, "Allow package states > 8 (default: false)");
+
 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 82ea281..3cdd07c0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1045,6 +1045,26 @@ struct intel_vbt_data {
 	struct child_device_config *child_dev;
 };
 
+struct i915_package_c8 {
+	/* Ideally every piece of our code would be using the forbid_refcnt and
+	 * we wouldn't need this "allowing" variable. The problem is that
+	 * there's no appropriate place to get/put the refcounts when enabling
+	 * and disabling CRTCs/encoders, so this "allowing" variable is used to
+	 * track whether the code that runs on haswell_modeset_global_resouces
+	 * has decided we should allow PC8 or not. */
+	bool allowing;
+	int forbid_refcnt;
+	struct mutex lock;
+
+	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;
@@ -1214,6 +1234,8 @@ typedef struct drm_i915_private {
 
 	struct i915_suspend_saved_registers regfile;
 
+	struct i915_package_c8 pc8;
+
 	/* Old dri1 support infrastructure, beware the dragons ya fools entering
 	 * here! */
 	struct i915_dri1_state dri1;
@@ -1631,6 +1653,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_allow_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_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 2b0690a..29819c6 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3085,3 +3085,68 @@ 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);
+	I915_WRITE(GTIMR, 0xffffffff);
+	I915_WRITE(GEN6_PMIMR, 0xffffffff);
+
+	WARN(I915_READ(DEIIR), "DEIIR is not 0\n");
+	WARN(I915_READ(SDEIIR), "SDEIIR is not 0\n");
+	WARN(I915_READ(GTIIR), "GTIIR is not 0\n");
+	WARN(I915_READ(GEN6_PMIIR), "GEN6_PMIIR is not 0\n");
+
+	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);
+
+	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);
+	I915_WRITE(GTIMR, dev_priv->pc8.regsave.gtimr);
+	I915_WRITE(GTIER, dev_priv->pc8.regsave.gtier);
+	I915_WRITE(GEN6_PMIMR, dev_priv->pc8.regsave.gen6_pmimr);
+
+	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 e1e50df..4825c64 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6076,6 +6076,116 @@ void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
 	}
 }
 
+void hsw_allow_package_c8(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	uint32_t val;
+
+	WARN_ON(!mutex_is_locked(&dev_priv->pc8.lock));
+	WARN(dev_priv->pc8.forbid_refcnt < 1,
+	     "pc8.forbid_refcnt: %d\n", dev_priv->pc8.forbid_refcnt);
+
+	dev_priv->pc8.forbid_refcnt--;
+	if (dev_priv->pc8.forbid_refcnt != 0)
+		return;
+
+	DRM_DEBUG_KMS("Allowing package C8+\n");
+
+	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);
+	}
+
+	cancel_delayed_work_sync(&dev_priv->rps.delayed_resume_work);
+	lpt_disable_clkout_dp(dev);
+	hsw_pc8_disable_interrupts(dev);
+	hsw_disable_lcpll(dev_priv, true, true);
+}
+
+void hsw_disallow_package_c8(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	uint32_t val;
+
+	WARN_ON(!mutex_is_locked(&dev_priv->pc8.lock));
+	WARN(dev_priv->pc8.forbid_refcnt < 0,
+	     "pc8.forbid_refcnt: %d\n", dev_priv->pc8.forbid_refcnt);
+
+	dev_priv->pc8.forbid_refcnt++;
+	if (dev_priv->pc8.forbid_refcnt != 1)
+		return;
+
+	DRM_DEBUG_KMS("Disallowing 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->struct_mutex);
+	intel_enable_gt_powersave(dev);
+	mutex_unlock(&dev->struct_mutex);
+}
+
+static bool hsw_can_allow_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 allowing 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 refcnt, so we use
+ * dev_priv->pc8.allowing to track whether we already have the refcnt or not.
+ */
+static void hsw_set_package_c8(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	bool allow;
+
+	if (!i915_allow_pc8)
+		return;
+
+	mutex_lock(&dev_priv->pc8.lock);
+
+	allow = hsw_can_allow_package_c8(dev_priv);
+
+	if (allow == dev_priv->pc8.allowing)
+		goto done;
+
+	dev_priv->pc8.allowing = allow;
+
+	if (allow)
+		hsw_allow_package_c8(dev);
+	else
+		hsw_disallow_package_c8(dev);
+
+done:
+	mutex_unlock(&dev_priv->pc8.lock);
+}
+
 static void haswell_modeset_global_resources(struct drm_device *dev)
 {
 	bool enable = false;
@@ -6091,6 +6201,8 @@ static void haswell_modeset_global_resources(struct drm_device *dev)
 	}
 
 	intel_set_power_well(dev, enable);
+
+	hsw_set_package_c8(dev);
 }
 
 static int haswell_crtc_mode_set(struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 9282321e..a531912 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 474797b..ac860d9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -836,5 +836,11 @@ extern void intel_edp_psr_update(struct drm_device *dev);
 extern void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
 			      bool switch_to_fclk, bool allow_power_down);
 extern void hsw_restore_lcpll(struct drm_i915_private *dev_priv);
+extern void hsw_allow_package_c8(struct drm_device *dev);
+extern void hsw_disallow_package_c8(struct drm_device *dev);
+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 0a5ba92..0945034 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5159,6 +5159,21 @@ void intel_init_power_well(struct drm_device *dev)
 		I915_WRITE(HSW_PWR_WELL_BIOS, 0);
 }
 
+/* Disallows PC8 so we can use the GMBUS and DP AUX interrupts. */
+void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv)
+{
+	mutex_lock(&dev_priv->pc8.lock);
+	hsw_disallow_package_c8(dev_priv->dev);
+	mutex_unlock(&dev_priv->pc8.lock);
+}
+
+void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv)
+{
+	mutex_lock(&dev_priv->pc8.lock);
+	hsw_allow_package_c8(dev_priv->dev);
+	mutex_unlock(&dev_priv->pc8.lock);
+}
+
 /* Set up chip specific power management-related functions */
 void intel_init_pm(struct drm_device *dev)
 {
-- 
1.8.1.2

[PATCH 4/8] drm/i915: avoid waking up from PC8 on DP AUX operations

[Paulo Zanoni]

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

If we're already allowing PC8, just don't use the IRQs, so we won't
need to wake from PC8. Waking up from PC8 is a slow thing, so avoid it
when we can.

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a531912..88ccbd5 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -315,6 +315,24 @@ static uint32_t get_aux_clock_divider(struct intel_dp *intel_dp,
 	}
 }
 
+static bool intel_dp_use_aux_irq(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	bool ret;
+
+	if (INTEL_INFO(dev)->gen <= 4 || IS_VALLEYVIEW(dev))
+		return false;
+
+	/* In case we're already allowing PC8, just don't use IRQs, so we won't
+	 * need to call intel_aux_display_runtime_get and then we won't wake up
+	 * from PC8. */
+	mutex_lock(&dev_priv->pc8.lock);
+	ret = (dev_priv->pc8.forbid_refcnt != 0);
+	mutex_unlock(&dev_priv->pc8.lock);
+
+	return ret;
+}
+
 static int
 intel_dp_aux_ch(struct intel_dp *intel_dp,
 		uint8_t *send, int send_bytes,
@@ -329,7 +347,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 	int i, ret, recv_bytes;
 	uint32_t status;
 	int try, precharge, clock = 0;
-	bool has_aux_irq = INTEL_INFO(dev)->gen >= 5 && !IS_VALLEYVIEW(dev);
+	bool has_aux_irq = intel_dp_use_aux_irq(dev);
 
 	/* dp aux is extremely sensitive to irq latency, hence request the
 	 * lowest possible wakeup latency and so prevent the cpu from going into
@@ -344,7 +362,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 	else
 		precharge = 5;
 
-	intel_aux_display_runtime_get(dev_priv);
+	if (has_aux_irq)
+		intel_aux_display_runtime_get(dev_priv);
 
 	/* Try to wait for any previous AUX channel activity */
 	for (try = 0; try < 3; try++) {
@@ -436,7 +455,8 @@ 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);
+	if (has_aux_irq)
+		intel_aux_display_runtime_put(dev_priv);
 
 	return ret;
 }
-- 
1.8.1.2

[PATCH 5/8] drm/i915: avoid waking up from PC8 on GMBUS operations

[Paulo Zanoni]

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

If we're already allowing PC8, just don't use the IRQs, so we won't
need to wake from PC8. Waking up from PC8 is a slow thing, so avoid it
when we can.

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

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index d1c1e0f7..2f35c10 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -203,24 +203,18 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin)
 	algo->data = bus;
 }
 
-/*
- * gmbus on gen4 seems to be able to generate legacy interrupts even when in MSI
- * mode. This results in spurious interrupt warnings if the legacy irq no. is
- * shared with another device. The kernel then disables that interrupt source
- * and so prevents the other device from working properly.
- */
-#define HAS_GMBUS_IRQ(dev) (INTEL_INFO(dev)->gen >= 5)
 static int
 gmbus_wait_hw_status(struct drm_i915_private *dev_priv,
 		     u32 gmbus2_status,
-		     u32 gmbus4_irq_en)
+		     u32 gmbus4_irq_en,
+		     bool use_irq)
 {
 	int i;
 	int reg_offset = dev_priv->gpio_mmio_base;
 	u32 gmbus2 = 0;
 	DEFINE_WAIT(wait);
 
-	if (!HAS_GMBUS_IRQ(dev_priv->dev))
+	if (!use_irq)
 		gmbus4_irq_en = 0;
 
 	/* Important: The hw handles only the first bit, so set only one! Since
@@ -250,14 +244,14 @@ gmbus_wait_hw_status(struct drm_i915_private *dev_priv,
 }
 
 static int
-gmbus_wait_idle(struct drm_i915_private *dev_priv)
+gmbus_wait_idle(struct drm_i915_private *dev_priv, bool use_irq)
 {
 	int ret;
 	int reg_offset = dev_priv->gpio_mmio_base;
 
 #define C ((I915_READ_NOTRACE(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0)
 
-	if (!HAS_GMBUS_IRQ(dev_priv->dev))
+	if (!use_irq)
 		return wait_for(C, 10);
 
 	/* Important: The hw handles only the first bit, so set only one! */
@@ -277,7 +271,7 @@ gmbus_wait_idle(struct drm_i915_private *dev_priv)
 
 static int
 gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
-		u32 gmbus1_index)
+		u32 gmbus1_index, bool use_irq)
 {
 	int reg_offset = dev_priv->gpio_mmio_base;
 	u16 len = msg->len;
@@ -294,7 +288,7 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
 		u32 val, loop = 0;
 
 		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY,
-					   GMBUS_HW_RDY_EN);
+					   GMBUS_HW_RDY_EN, use_irq);
 		if (ret)
 			return ret;
 
@@ -309,7 +303,8 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
 }
 
 static int
-gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg)
+gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
+		 bool use_irq)
 {
 	int reg_offset = dev_priv->gpio_mmio_base;
 	u16 len = msg->len;
@@ -339,7 +334,7 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg)
 		I915_WRITE(GMBUS3 + reg_offset, val);
 
 		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY,
-					   GMBUS_HW_RDY_EN);
+					   GMBUS_HW_RDY_EN, use_irq);
 		if (ret)
 			return ret;
 	}
@@ -359,7 +354,8 @@ gmbus_is_index_read(struct i2c_msg *msgs, int i, int num)
 }
 
 static int
-gmbus_xfer_index_read(struct drm_i915_private *dev_priv, struct i2c_msg *msgs)
+gmbus_xfer_index_read(struct drm_i915_private *dev_priv, struct i2c_msg *msgs,
+		      bool use_irq)
 {
 	int reg_offset = dev_priv->gpio_mmio_base;
 	u32 gmbus1_index = 0;
@@ -377,7 +373,7 @@ gmbus_xfer_index_read(struct drm_i915_private *dev_priv, struct i2c_msg *msgs)
 	if (gmbus5)
 		I915_WRITE(GMBUS5 + reg_offset, gmbus5);
 
-	ret = gmbus_xfer_read(dev_priv, &msgs[1], gmbus1_index);
+	ret = gmbus_xfer_read(dev_priv, &msgs[1], gmbus1_index, use_irq);
 
 	/* Clear GMBUS5 after each index transfer */
 	if (gmbus5)
@@ -386,6 +382,31 @@ gmbus_xfer_index_read(struct drm_i915_private *dev_priv, struct i2c_msg *msgs)
 	return ret;
 }
 
+static bool gmbus_use_irq(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	bool ret;
+
+	/*
+	 * gmbus on gen4 seems to be able to generate legacy interrupts even
+	 * when in MSI mode. This results in spurious interrupt warnings if the
+	 * legacy irq no. is shared with another device. The kernel then
+	 * disables that interrupt source and so prevents the other device from
+	 * working properly.
+	 */
+	if (INTEL_INFO(dev)->gen <= 4)
+		return false;
+
+	/* In case we're already allowing PC8, just don't use IRQs, so we won't
+	 * need to call intel_aux_display_runtime_get and then we won't wake up
+	 * from PC8. */
+	mutex_lock(&dev_priv->pc8.lock);
+	ret = (dev_priv->pc8.forbid_refcnt != 0);
+	mutex_unlock(&dev_priv->pc8.lock);
+
+	return ret;
+}
+
 static int
 gmbus_xfer(struct i2c_adapter *adapter,
 	   struct i2c_msg *msgs,
@@ -397,8 +418,10 @@ gmbus_xfer(struct i2c_adapter *adapter,
 	struct drm_i915_private *dev_priv = bus->dev_priv;
 	int i, reg_offset;
 	int ret = 0;
+	bool use_irq = gmbus_use_irq(dev_priv->dev);
 
-	intel_aux_display_runtime_get(dev_priv);
+	if (use_irq)
+		intel_aux_display_runtime_get(dev_priv);
 	mutex_lock(&dev_priv->gmbus_mutex);
 
 	if (bus->force_bit) {
@@ -412,12 +435,13 @@ gmbus_xfer(struct i2c_adapter *adapter,
 
 	for (i = 0; i < num; i++) {
 		if (gmbus_is_index_read(msgs, i, num)) {
-			ret = gmbus_xfer_index_read(dev_priv, &msgs[i]);
+			ret = gmbus_xfer_index_read(dev_priv, &msgs[i],
+						    use_irq);
 			i += 1;  /* set i to the index of the read xfer */
 		} else if (msgs[i].flags & I2C_M_RD) {
-			ret = gmbus_xfer_read(dev_priv, &msgs[i], 0);
+			ret = gmbus_xfer_read(dev_priv, &msgs[i], 0, use_irq);
 		} else {
-			ret = gmbus_xfer_write(dev_priv, &msgs[i]);
+			ret = gmbus_xfer_write(dev_priv, &msgs[i], use_irq);
 		}
 
 		if (ret == -ETIMEDOUT)
@@ -426,7 +450,7 @@ gmbus_xfer(struct i2c_adapter *adapter,
 			goto clear_err;
 
 		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_WAIT_PHASE,
-					   GMBUS_HW_WAIT_EN);
+					   GMBUS_HW_WAIT_EN, use_irq);
 		if (ret == -ENXIO)
 			goto clear_err;
 		if (ret)
@@ -443,7 +467,7 @@ gmbus_xfer(struct i2c_adapter *adapter,
 	 * We will re-enable it at the start of the next xfer,
 	 * till then let it sleep.
 	 */
-	if (gmbus_wait_idle(dev_priv)) {
+	if (gmbus_wait_idle(dev_priv, use_irq)) {
 		DRM_DEBUG_KMS("GMBUS [%s] timed out waiting for idle\n",
 			 adapter->name);
 		ret = -ETIMEDOUT;
@@ -467,7 +491,7 @@ clear_err:
 	 * it's slow responding and only answers on the 2nd retry.
 	 */
 	ret = -ENXIO;
-	if (gmbus_wait_idle(dev_priv)) {
+	if (gmbus_wait_idle(dev_priv, use_irq)) {
 		DRM_DEBUG_KMS("GMBUS [%s] timed out after NAK\n",
 			      adapter->name);
 		ret = -ETIMEDOUT;
@@ -498,7 +522,8 @@ timeout:
 
 out:
 	mutex_unlock(&dev_priv->gmbus_mutex);
-	intel_aux_display_runtime_put(dev_priv);
+	if (use_irq)
+		intel_aux_display_runtime_put(dev_priv);
 	return ret;
 }
 
-- 
1.8.1.2

[PATCH 6/8] drm/i915: silence message about the DDI buffers

[Paulo Zanoni]

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

This message is not really useful since it's very easy to check which
mode is used for each port, and if you allow/disallow PC8+ many times
it will eat your dmesg buffers.

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

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 9e77e32..cb82c8d 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -94,10 +94,6 @@ static void intel_prepare_ddi_buffers(struct drm_device *dev, enum port port,
 		hsw_ddi_translations_fdi :
 		hsw_ddi_translations_dp);
 
-	DRM_DEBUG_DRIVER("Initializing DDI buffers for port %c in %s mode\n",
-			port_name(port),
-			use_fdi_mode ? "FDI" : "DP");
-
 	WARN((use_fdi_mode && (port != PORT_E)),
 		"Programming port %c in FDI mode, this probably will not work.\n",
 		port_name(port));
-- 
1.8.1.2

[PATCH 7/8] drm/i915: add i915_pc8_status debugfs file

[Paulo Zanoni]

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

For now it just prints the value of the forbid_refcount and allowing
variables.

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index ed72fe0..320f4c2 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1674,6 +1674,26 @@ 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, "Core display allowing: %s\n",
+		   yesno(dev_priv->pc8.allowing));
+	seq_printf(m, "Forbid refcount: %d\n", dev_priv->pc8.forbid_refcnt);
+	mutex_unlock(&dev_priv->pc8.lock);
+
+	return 0;
+}
+
 static int
 i915_wedged_get(void *data, u64 *val)
 {
@@ -2107,6 +2127,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 8/8] drm/i915: allow Package C8+ by default

[Paulo Zanoni]

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

Things should be working, so enable it by default.

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 6df516b..8f6e685 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_allow_pc8 __read_mostly = 0;
+int i915_allow_pc8 __read_mostly = 1;
 module_param_named(allow_pc8, i915_allow_pc8, int, 0600);
-MODULE_PARM_DESC(allow_pc8, "Allow package states > 8 (default: false)");
+MODULE_PARM_DESC(allow_pc8, "Allow package states > 8 (default: true)");
 
 bool i915_prefault_disable __read_mostly;
 module_param_named(prefault_disable, i915_prefault_disable, bool, 0600);
-- 
1.8.1.2

[PATCH i-g-t] 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.
  - Before running the test, the machine needs to have 0 PC8+
    residency. If you run the test after the machine already has PC8+
    residency, it means you'll be comparing post-PC8+ with post-PC8+,
    so you won't be able to get things lost after the first time we
    enter PC8+.
      - This means that after you run the test once, you have to
	reboot, unless you use the "--skip-zero-pc8-check" option, but
	you really need to know what you're doing.
  - You need at least one output connected.

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

diff --git a/tests/.gitignore b/tests/.gitignore
index 451ab2d..c70fdb7 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -90,6 +90,7 @@ getstats
 getversion
 kms_flip
 kms_render
+pc8
 prime_nv_api
 prime_nv_pcopy
 prime_nv_test
diff --git a/tests/Makefile.am b/tests/Makefile.am
index a59c25f..3507716 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -2,6 +2,7 @@ if BUILD_TESTS
 noinst_PROGRAMS = \
 	gem_stress \
 	ddi_compute_wrpll \
+	pc8 \
 	$(TESTS_progs) \
 	$(TESTS_progs_M) \
 	$(HANG) \
diff --git a/tests/pc8.c b/tests/pc8.c
new file mode 100644
index 0000000..7fdfeb5
--- /dev/null
+++ b/tests/pc8.c
@@ -0,0 +1,555 @@
+/*
+ * 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 <assert.h>
+#include <stdint.h>
+#include <stdbool.h>
+#include <string.h>
+
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+
+#include "drm.h"
+#include "drmtest.h"
+#include "intel_batchbuffer.h"
+#include "intel_gpu_tools.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;
+
+/* Stuff used when creating FBs and mode setting. */
+struct mode_set_data {
+	drmModeResPtr res;
+	drmModeConnectorPtr connectors[MAX_CONNECTORS];
+
+	drm_intel_bufmgr *bufmgr;
+	uint32_t devid;
+};
+
+enum compare_step {
+	STEP_RESET,
+	STEP_RESTORED,
+};
+
+/* 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 {
+		/* 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;
+	} regs;
+};
+
+static uint64_t get_residency(int fd, uint32_t type)
+{
+	int rc;
+	uint64_t ret;
+
+	rc = pread(fd, &ret, sizeof(uint64_t), type);
+	assert(rc == sizeof(uint64_t));
+
+	return ret;
+}
+
+static bool pc8_plus_residency_is_zero(int fd)
+{
+	uint64_t res_pc8, res_pc9, res_pc10;
+
+	res_pc8 = get_residency(fd, MSR_PC8_RES);
+	res_pc9 = get_residency(fd, MSR_PC9_RES);
+	res_pc10 = get_residency(fd, MSR_PC10_RES);
+
+	if (res_pc8 != 0 || res_pc9 != 0 || res_pc10 != 0)
+		return false;
+
+	return true;
+}
+
+static bool pc8_plus_residency_changed(int fd, unsigned int timeout_sec)
+{
+	unsigned int i;
+	uint64_t res_pc8, res_pc9, res_pc10;
+	int to_sleep = 100 * 1000;
+
+	res_pc8 = get_residency(fd, MSR_PC8_RES);
+	res_pc9 = get_residency(fd, MSR_PC9_RES);
+	res_pc10 = get_residency(fd, MSR_PC10_RES);
+
+	for (i = 0; i < timeout_sec * 1000 * 1000; i += to_sleep) {
+		if (res_pc8 != get_residency(fd, MSR_PC8_RES) ||
+		    res_pc9 != get_residency(fd, MSR_PC9_RES) ||
+		    res_pc10 != get_residency(fd, MSR_PC10_RES)) {
+			return true;
+		}
+		usleep(to_sleep);
+	}
+
+	return false;
+}
+
+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);
+		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);
+
+	assert(crtc_id);
+	assert(buffer_id);
+	assert(connector_id);
+	assert(mode);
+
+	rc = drmModeSetCrtc(drm_fd, crtc_id, buffer_id, 0, 0, &connector_id,
+			    1, mode);
+	assert(rc == 0);
+}
+
+static void get_connector_edid(struct compare_data *data, int index)
+{
+	unsigned int i;
+	drmModeConnectorPtr connector = data->connectors[index];
+	drmModeObjectPropertiesPtr props;
+
+	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) {
+			assert(prop->flags & DRM_MODE_PROP_BLOB);
+			assert(prop->count_blobs == 0);
+			data->edids[index] = drmModeGetPropertyBlob(drm_fd,
+							props->prop_values[i]);
+		}
+
+		drmModeFreeProperty(prop);
+	}
+
+	drmModeFreeObjectProperties(props);
+}
+
+static void init_mode_set_data(struct mode_set_data *data)
+{
+	int i;
+
+	data->res = drmModeGetResources(drm_fd);
+	assert(data->res);
+	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->bufmgr = drm_intel_bufmgr_gem_init(drm_fd, 4096);
+	data->devid = intel_get_drm_devid(drm_fd);
+
+	do_or_die(drmtest_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]);
+	drmModeFreeResources(data->res);
+}
+
+static void get_drm_info(struct compare_data *data)
+{
+	int i;
+
+	data->res = drmModeGetResources(drm_fd);
+	assert(data->res);
+
+	assert(data->res->count_connectors <= MAX_CONNECTORS);
+	assert(data->res->count_encoders <= MAX_ENCODERS);
+	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]);
+		get_connector_edid(data, 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]);
+
+	intel_register_access_init(intel_get_pci_device(), 0);
+	data->regs.arb_mode = INREG(0x4030);
+	data->regs.tilectl = INREG(0x101000);
+	data->regs.gen6_ucgctl2 = INREG(0x9404);
+	data->regs.gen7_l3cntlreg1 = INREG(0xB0C1);
+	data->regs.transa_chicken1 = INREG(0xF0060);
+	data->regs.deier = INREG(0x4400C);
+	data->regs.gtier = INREG(0x4401C);
+	data->regs.ddi_buf_trans_a_1 = INREG(0x64E00);
+	data->regs.ddi_buf_trans_b_5 = INREG(0x64E70);
+	data->regs.ddi_buf_trans_c_10 = INREG(0x64EE0);
+	data->regs.ddi_buf_trans_d_15 = INREG(0x64F58);
+	data->regs.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) assert(d1->data == d2->data)
+#define COMPARE_ARRAY(d1, d2, size, data) do { \
+	for (i = 0; i < size; i++) \
+		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);
+	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;
+	assert(e1 && e2);
+
+	COMPARE(e1, e2, id);
+	COMPARE(e1, e2, length);
+
+	assert(memcmp(e1->data, e2->data, e1->length) == 0);
+}
+
+static void compare_registers(struct compare_data *d1, struct compare_data *d2,
+			      enum compare_step step)
+{
+	/* Things that shouldn't change at all. */
+	COMPARE(d1, d2, regs.gen6_ucgctl2);
+	COMPARE(d1, d2, regs.gen7_l3cntlreg1);
+	COMPARE(d1, d2, regs.transa_chicken1);
+	COMPARE(d1, d2, regs.arb_mode);
+	COMPARE(d1, d2, regs.tilectl);
+	assert(d1->regs.deier & (1 << 31));
+	assert(d2->regs.deier & (1 << 31));
+
+	switch (step) {
+	case STEP_RESET:
+		assert(d2->regs.gtier == 0);
+		assert(d2->regs.ddi_buf_trans_a_1 == 0);
+		assert(d2->regs.ddi_buf_trans_b_5 == 0);
+		assert(d2->regs.ddi_buf_trans_c_10 == 0);
+		assert(d2->regs.ddi_buf_trans_d_15 == 0);
+		assert(d2->regs.ddi_buf_trans_e_20 == 0);
+		break;
+	case STEP_RESTORED:
+		COMPARE(d1, d2, regs.arb_mode);
+		COMPARE(d1, d2, regs.tilectl);
+		COMPARE(d1, d2, regs.gtier);
+		COMPARE(d1, d2, regs.ddi_buf_trans_a_1);
+		COMPARE(d1, d2, regs.ddi_buf_trans_b_5);
+		COMPARE(d1, d2, regs.ddi_buf_trans_c_10);
+		COMPARE(d1, d2, regs.ddi_buf_trans_d_15);
+		COMPARE(d1, d2, regs.ddi_buf_trans_e_20);
+		break;
+	default:
+		assert(0);
+	}
+}
+
+static void assert_drm_infos_equal(struct compare_data *d1,
+				   struct compare_data *d2,
+				   enum compare_step step)
+{
+	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]);
+
+	compare_registers(d1, d2, step);
+}
+
+#define BUF_SIZE	(8 << 20)
+
+/* to avoid stupid depencies on libdrm, copy&paste */
+struct local_drm_i915_gem_wait {
+	/** Handle of BO we shall wait on */
+	__u32 bo_handle;
+	__u32 flags;
+	/** Number of nanoseconds to wait, Returns time remaining. */
+	__u64 timeout_ns;
+};
+#define WAIT_IOCTL DRM_IOWR(DRM_COMMAND_BASE + 0x2c, struct local_drm_i915_gem_wait)
+
+static int gem_bo_wait_timeout(int fd, uint32_t handle, uint64_t *timeout_ns)
+{
+	struct local_drm_i915_gem_wait wait;
+	int ret;
+
+	assert(timeout_ns);
+
+	wait.bo_handle = handle;
+	wait.timeout_ns = *timeout_ns;
+	wait.flags = 0;
+	ret = drmIoctl(fd, WAIT_IOCTL, &wait);
+	*timeout_ns = wait.timeout_ns;
+
+	return ret ? -errno : 0;
+}
+
+static void test_batch(struct mode_set_data *data)
+{
+	uint64_t timeout = 1000 * 1000 * 1000 * 2;
+	drm_intel_bo *dst;
+
+	dst = drm_intel_bo_alloc(data->bufmgr, "dst", BUF_SIZE, 4096);
+	if (gem_bo_wait_timeout(drm_fd, dst->handle, &timeout) == -EINVAL)
+		printf("Kernel doesn't support wait_timeout\n");
+}
+
+int main(int argc, char *argv[])
+{
+	struct mode_set_data ms_data;
+	struct compare_data pre_pc8, during_pc8, post_pc8;
+	int msr_fd;
+	bool skip_zero_pc8_check = false;
+
+	if (argc > 1) {
+		/* With this option we can test/debug/develop the tool without
+		 * needing to reboot every time. */
+		if (strcmp(argv[1], "--skip-zero-pc8-check") == 0)
+			skip_zero_pc8_check = true;
+	}
+
+	drm_fd = drm_open_any();
+	assert(drm_fd > 0);
+
+	init_mode_set_data(&ms_data);
+
+	if (!IS_HASWELL(ms_data.devid)) {
+		printf("PC8+ feature only supported on Haswell.\n");
+		return 77;
+	}
+
+	msr_fd = open("/dev/cpu/0/msr", O_RDONLY);
+	assert(msr_fd != -1);
+
+	/* Make sure the machine still didn't enter PC8+, otherwise we won't be
+	 * able to really compare pre-PC8+ with post-PC8+. */
+	printf("Pre-PC8+\n");
+	assert(skip_zero_pc8_check || pc8_plus_residency_is_zero(msr_fd));
+	get_drm_info(&pre_pc8);
+	test_batch(&ms_data);
+	assert(skip_zero_pc8_check || pc8_plus_residency_is_zero(msr_fd));
+
+	printf("PC8+\n");
+	disable_all_screens(&ms_data);
+	sleep(1);
+
+	assert(pc8_plus_residency_changed(msr_fd, 10));
+	get_drm_info(&during_pc8);
+	test_batch(&ms_data);
+	assert(pc8_plus_residency_changed(msr_fd, 10));
+
+	assert_drm_infos_equal(&pre_pc8, &during_pc8, STEP_RESET);
+
+	printf("Post-PC8+\n");
+	enable_one_screen(&ms_data);
+	sleep(1);
+
+	assert(!pc8_plus_residency_changed(msr_fd, 5));
+	get_drm_info(&post_pc8);
+	test_batch(&ms_data);
+	assert(!pc8_plus_residency_changed(msr_fd, 5));
+
+	assert_drm_infos_equal(&pre_pc8, &post_pc8, STEP_RESTORED);
+
+	free_drm_info(&pre_pc8);
+	free_drm_info(&during_pc8);
+	free_drm_info(&post_pc8);
+	fini_mode_set_data(&ms_data);
+	close(msr_fd);
+	drmClose(drm_fd);
+
+	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 3/8] drm/i915: allow package C8+ states on Haswell (disabled)

[Chris Wilson]

On Mon, Jul 29, 2013 at 05:48:22PM -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.

Still dislike the names. hsw_pc8 is good, so use it consistently.

> +/* Disallows PC8 so we can use the GMBUS and DP AUX interrupts. */
> +void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv)
> +{
> +	mutex_lock(&dev_priv->pc8.lock);
> +	hsw_disallow_package_c8(dev_priv->dev);
> +	mutex_unlock(&dev_priv->pc8.lock);
> +}
> +
> +void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv)
> +{
> +	mutex_lock(&dev_priv->pc8.lock);
> +	hsw_allow_package_c8(dev_priv->dev);
> +	mutex_unlock(&dev_priv->pc8.lock);
> +}

Move the locking from into hsw_(allow|disallow)_package_c8, and rename
the current hsw_alloc_package_c8 i.e

  void hsw_allow_package_c8(struct drm_i915_private *dev_priv)
  {
	mutex_lock(&dev_priv->pc8.lock);
	__hsw_allow_package_c8(dev_priv);
	mutex_unlock(&dev_priv->pc8.lock);
  }

then allow the lock manipulation is close together and the doesn't leak
across layers. (The locking is definitely not clear atm.) Whilst you are
at is, I do not see any reason not to call them hsw_pc8_enable() and
hsw_pc8_disable(), and hsw_set_package_c8() becomes hsw_pc8_update().

Just call forbid_refcnt, forbid_count (though I'm still liking
wake_count). And replace allowing with display_power_well_active,
verbosity is good here. s/i915_allow_pc8/i915_enable_pc8/ for
consistency.
 
So other than those minor issues, being freaked out by the locking and
the WARNs, I want at least a paragraph explaining why that is even
remotely safe, the logic at least looks sane.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Chris Wilson]

On Mon, Jul 29, 2013 at 05:48:22PM -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.

This explanation needs to be kept in the code somewhere, either as a
small theory of operation before the pc8 logic (if you want to expand it
some more), but this sentence can quite aptly go inside struct
drm_i915_private {
   /* When all display outputs are disabled, and the display power well
    * turned off, we can enable pc8 for further power savings.
    */
   struct i915_package_c8 package_c8;
 };
We just need some statement of what we are tracking and why.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 8/8] drm/i915: allow Package C8+ by default

[Chris Wilson]

On Mon, Jul 29, 2013 at 05:48:27PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Things should be working, so enable it by default.
> 
> 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 6df516b..8f6e685 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_allow_pc8 __read_mostly = 0;
> +int i915_allow_pc8 __read_mostly = 1;
>  module_param_named(allow_pc8, i915_allow_pc8, int, 0600);
> -MODULE_PARM_DESC(allow_pc8, "Allow package states > 8 (default: false)");
> +MODULE_PARM_DESC(allow_pc8, "Allow package states > 8 (default: true)");

How about (after changing the variable name as earlier):
"Enable support for low power package sleep states (pc8) [default: true])"

Hmm, I thought we were consistent in using [default], alas not.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 5/8] drm/i915: avoid waking up from PC8 on GMBUS operations

[Chris Wilson]

On Mon, Jul 29, 2013 at 05:48:24PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> If we're already allowing PC8, just don't use the IRQs, so we won't
> need to wake from PC8. Waking up from PC8 is a slow thing, so avoid it
> when we can.

You would also need to explain that the GMBUS is outside of the display
power well.

Looks reasonable, the only bit is moving the read of forbid_count into
hsw_pc8_enabled() so that the gmbus code isn't poking around with
someone else's locks, and we can safely do an unlocked optimistic read
here.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/8] drm/i915: avoid waking up from PC8 on DP AUX operations

[Chris Wilson]

On Mon, Jul 29, 2013 at 05:48:23PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> If we're already allowing PC8, just don't use the IRQs, so we won't
> need to wake from PC8. Waking up from PC8 is a slow thing, so avoid it
> when we can.

This raises the question of why we where holding the power well wake
lock if we weren't using IRQs in the first place. This tells me the 
intel_aux_display_runtime_get() interface is indequate, imo. It would
need to be more expressive of why/what part of aux display runtime you
want. You could even move the state checking there so that GMBUS could
use the same interface:

unsigned long intel_aux_display_runtime_get(dev_priv, want, need)
 {
  if (need) {
     __intel_aux_display_runtime_get();
     return need;
   }

   if (want) {
   	if (hsw_pc8_enabled()) {
     	   __intel_aux_display_runtime_get();
     	   return want;
	}
   }

   return 0;
 }
 
And pass the return value to _put for it to decide if it needs to do
anything (also it can then select its path based on whether or not it
holds a wakeref). Making the caller decide whether or not to act based
on information inside pc8 looks very fragile to me, and easy to break in
future.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/8] drm/i915: don't disable/reenable IVB error interrupts when not needed

[Chris Wilson]

On Mon, Jul 29, 2013 at 05:48:21PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> If the error interrupts are already disabled, don't disable and
> reenable them. This is going to be needed when we're in PC8+, where
> all the interrupts are disabled so we won't risk re-enabling
> DE_ERR_INT_IVB.

>  	if (IS_HASWELL(dev)) {
>  		spin_lock(&dev_priv->irq_lock);
> -		ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
> +		if (!(I915_READ(DEIMR) & DE_ERR_INT_IVB)) {
> +			ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
> +			err_int_reenable = true;
> +		}

Or just make ironlake_disable_display_irq() return a bool. So this then
raises the question: please justify why the deimr state tracker is out
of sync with the register. If it is possible that we write to this
register whilst under pc8 and then restore a different value, that is
scary. I would rather have a big BUG_ON(!pc8) inside
ironlake_disable_display_irq() and move the pc8 handling logic there
(i.e. if we get a request for something whilst under pc8, modify the
saved bits rather than the actual register).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Paulo Zanoni]

2013/7/29 Chris Wilson <chris@chris-wilson.co.uk>:
> On Mon, Jul 29, 2013 at 05:48:22PM -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.
>
> Still dislike the names. hsw_pc8 is good, so use it consistently.

Do you mean i915.allow_hsw_pc8? Or i915.enable_pc8? Or
i915.enable_hsw_pc8? (You suggested to change from "allow" to
"enable").

>
>> +/* Disallows PC8 so we can use the GMBUS and DP AUX interrupts. */
>> +void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv)
>> +{
>> +     mutex_lock(&dev_priv->pc8.lock);
>> +     hsw_disallow_package_c8(dev_priv->dev);
>> +     mutex_unlock(&dev_priv->pc8.lock);
>> +}
>> +
>> +void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv)
>> +{
>> +     mutex_lock(&dev_priv->pc8.lock);
>> +     hsw_allow_package_c8(dev_priv->dev);
>> +     mutex_unlock(&dev_priv->pc8.lock);
>> +}
>
> Move the locking from into hsw_(allow|disallow)_package_c8, and rename
> the current hsw_alloc_package_c8 i.e
>
>   void hsw_allow_package_c8(struct drm_i915_private *dev_priv)
>   {
>         mutex_lock(&dev_priv->pc8.lock);
>         __hsw_allow_package_c8(dev_priv);
>         mutex_unlock(&dev_priv->pc8.lock);
>   }
>
> then allow the lock manipulation is close together and the doesn't leak
> across layers. (The locking is definitely not clear atm.) Whilst you are
> at is, I do not see any reason not to call them hsw_pc8_enable() and
> hsw_pc8_disable(), and hsw_set_package_c8() becomes hsw_pc8_update().
>
> Just call forbid_refcnt, forbid_count (though I'm still liking
> wake_count). And replace allowing with display_power_well_active,
> verbosity is good here.

You mean replace dev_priv->pc8.allowing with
dev->priv->pc8.display_power_well_active? That's not good, because
when you have eDP-only the display power well is disabled but you
can't allow PC8, and then you have more than one output the power well
is enabled but you can't allow PC8.

> s/i915_allow_pc8/i915_enable_pc8/ for
> consistency.

I use the word "allow" because even if we allow PC8 it doesn't mean it
will actually be enabled, other drivers also need to allow it. But, of
course, I could change this anyway.


>
> So other than those minor issues, being freaked out by the locking and
> the WARNs, I want at least a paragraph explaining why that is even
> remotely safe, the logic at least looks sane.

I'll try to write something in the log message. The basic idea is that
we set specific values to the IMR registers before we allow PC8, and
when we get disallow PC8 we check if the values changed and print a
nice WARN in case they did. We can't get interrupts (except for HPD)
while we're allowing PC8, we could get machine hangs with this.


> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



-- 
Paulo Zanoni

Re: [PATCH 4/8] drm/i915: avoid waking up from PC8 on DP AUX operations

[Paulo Zanoni]

2013/7/30 Chris Wilson <chris@chris-wilson.co.uk>:
> On Mon, Jul 29, 2013 at 05:48:23PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> If we're already allowing PC8, just don't use the IRQs, so we won't
>> need to wake from PC8. Waking up from PC8 is a slow thing, so avoid it
>> when we can.
>
> This raises the question of why we where holding the power well wake
> lock if we weren't using IRQs in the first place.

I can't understand the sentence above. Also notice that the power well
is kinda orthogonal to allowing/disallowing PC8. We need the power
well disabled to allow PC8, but we won't allow PC8 whenever the power
well is disabled. And the GMBUS/DP AUX registers are on the PCH, so
they don't get affected by the state of the power well.

> This tells me the
> intel_aux_display_runtime_get() interface is indequate, imo. It would
> need to be more expressive of why/what part of aux display runtime you
> want. You could even move the state checking there so that GMBUS could
> use the same interface:
>
> unsigned long intel_aux_display_runtime_get(dev_priv, want, need)
>  {
>   if (need) {
>      __intel_aux_display_runtime_get();
>      return need;
>    }
>
>    if (want) {
>         if (hsw_pc8_enabled()) {
>            __intel_aux_display_runtime_get();
>            return want;
>         }
>    }
>
>    return 0;
>  }
>
> And pass the return value to _put for it to decide if it needs to do
> anything (also it can then select its path based on whether or not it
> holds a wakeref). Making the caller decide whether or not to act based
> on information inside pc8 looks very fragile to me, and easy to break in
> future.

Yeah, sounds like a good idea.


> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



-- 
Paulo Zanoni

Re: [PATCH 4/8] drm/i915: avoid waking up from PC8 on DP AUX operations

[Chris Wilson]

On Wed, Jul 31, 2013 at 11:31:42AM -0300, Paulo Zanoni wrote:
> 2013/7/30 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Mon, Jul 29, 2013 at 05:48:23PM -0300, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> If we're already allowing PC8, just don't use the IRQs, so we won't
> >> need to wake from PC8. Waking up from PC8 is a slow thing, so avoid it
> >> when we can.
> >
> > This raises the question of why we where holding the power well wake
> > lock if we weren't using IRQs in the first place.
> 
> I can't understand the sentence above. Also notice that the power well
> is kinda orthogonal to allowing/disallowing PC8. We need the power
> well disabled to allow PC8, but we won't allow PC8 whenever the power
> well is disabled.

This is the key sentence that is missing that explains why this patch is
relevant:

> And the GMBUS/DP AUX registers are on the PCH, so
> they don't get affected by the state of the power well.

That is we can use the gpio pins irrespective of whether either the package
state or display power well is enabled. So if the package is asleep, we
can use the slightly slower, higher CPU, method to communicate with the
display device rather than incur the overhead of powering up the package
to monitor IRQs.

I have not looked at what is covered by the power wells and by the
package cstate - so I am relying on the patch series to teach me and be
consistent with the story it tells. (You explain the rationale of the
patch in the changelog, and then I can check that the implementation
follows suit. And can then also go back and check the rationale against
the power management guides, etc.)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Chris Wilson]

On Wed, Jul 31, 2013 at 11:24:22AM -0300, Paulo Zanoni wrote:
> 2013/7/29 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Mon, Jul 29, 2013 at 05:48:22PM -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.
> >
> > Still dislike the names. hsw_pc8 is good, so use it consistently.
> 
> Do you mean i915.allow_hsw_pc8? Or i915.enable_pc8? Or
> i915.enable_hsw_pc8? (You suggested to change from "allow" to
> "enable").

i915.enable_pc8 to be consistent with
i915.enable_psr
i915.enable_fbc
i915.enable_rc6
i915.enable_rps

> > Just call forbid_refcnt, forbid_count (though I'm still liking
> > wake_count). And replace allowing with display_power_well_active,
> > verbosity is good here.
> 
> You mean replace dev_priv->pc8.allowing with
> dev->priv->pc8.display_power_well_active? That's not good, because
> when you have eDP-only the display power well is disabled but you
> can't allow PC8, and then you have more than one output the power well
> is enabled but you can't allow PC8.

That is not what your code says.

> > s/i915_allow_pc8/i915_enable_pc8/ for
> > consistency.
> 
> I use the word "allow" because even if we allow PC8 it doesn't mean it
> will actually be enabled, other drivers also need to allow it. But, of
> course, I could change this anyway.

Right. But as far as we are concerned, and more importantly our
bookkeeping, we enable it. Whether it is enabled is up to the hardware.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Paulo Zanoni]

2013/7/31 Chris Wilson <chris@chris-wilson.co.uk>:
> On Wed, Jul 31, 2013 at 11:24:22AM -0300, Paulo Zanoni wrote:
>> 2013/7/29 Chris Wilson <chris@chris-wilson.co.uk>:
>> > On Mon, Jul 29, 2013 at 05:48:22PM -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.
>> >
>> > Still dislike the names. hsw_pc8 is good, so use it consistently.
>>
>> Do you mean i915.allow_hsw_pc8? Or i915.enable_pc8? Or
>> i915.enable_hsw_pc8? (You suggested to change from "allow" to
>> "enable").
>
> i915.enable_pc8 to be consistent with
> i915.enable_psr
> i915.enable_fbc
> i915.enable_rc6
> i915.enable_rps
>
>> > Just call forbid_refcnt, forbid_count (though I'm still liking
>> > wake_count). And replace allowing with display_power_well_active,
>> > verbosity is good here.
>>
>> You mean replace dev_priv->pc8.allowing with
>> dev->priv->pc8.display_power_well_active? That's not good, because
>> when you have eDP-only the display power well is disabled but you
>> can't allow PC8, and then you have more than one output the power well
>> is enabled but you can't allow PC8.
>
> That is not what your code says.

The code says "power well disabled and all outputs disabled". But I
agree that just "allowing" is a bad name, I'll try to think on a good
name.

>
>> > s/i915_allow_pc8/i915_enable_pc8/ for
>> > consistency.
>>
>> I use the word "allow" because even if we allow PC8 it doesn't mean it
>> will actually be enabled, other drivers also need to allow it. But, of
>> course, I could change this anyway.
>
> Right. But as far as we are concerned, and more importantly our
> bookkeeping, we enable it. Whether it is enabled is up to the hardware.
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



-- 
Paulo Zanoni

Re: [PATCH 2/8] drm/i915: don't disable/reenable IVB error interrupts when not needed

[Paulo Zanoni]

2013/7/30 Chris Wilson <chris@chris-wilson.co.uk>:
> On Mon, Jul 29, 2013 at 05:48:21PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> If the error interrupts are already disabled, don't disable and
>> reenable them. This is going to be needed when we're in PC8+, where
>> all the interrupts are disabled so we won't risk re-enabling
>> DE_ERR_INT_IVB.
>
>>       if (IS_HASWELL(dev)) {
>>               spin_lock(&dev_priv->irq_lock);
>> -             ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
>> +             if (!(I915_READ(DEIMR) & DE_ERR_INT_IVB)) {
>> +                     ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
>> +                     err_int_reenable = true;
>> +             }
>
> Or just make ironlake_disable_display_irq() return a bool.

Will do.

> So this then
> raises the question: please justify why the deimr state tracker is out
> of sync with the register.

It shouldn't be.

> If it is possible that we write to this
> register whilst under pc8 and then restore a different value, that is
> scary. I would rather have a big BUG_ON(!pc8) inside
> ironlake_disable_display_irq() and move the pc8 handling logic there
> (i.e. if we get a request for something whilst under pc8, modify the
> saved bits rather than the actual register).

Your concerns are valid and some of the WARNs I have are exactly to
catch these things, but we catch them *after* we disallow/disable PC8.
On my new series I'll port GTIMR, GEN6_PMIMR and SDEIMR to the same
model as ironlake_enable/disable_display_irq and then add some more
WARNs in case we're calling them with PC8 enabled. Maybe the safest
thing to do would actually disable PC8 in case that happens (and also
print an error message so we can fix these cases).

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



-- 
Paulo Zanoni

Re: [PATCH i-g-t] tests: add pc8

[Daniel Vetter]

On Mon, Jul 29, 2013 at 05:53:27PM -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.
>   - Before running the test, the machine needs to have 0 PC8+
>     residency. If you run the test after the machine already has PC8+
>     residency, it means you'll be comparing post-PC8+ with post-PC8+,
>     so you won't be able to get things lost after the first time we
>     enter PC8+.
>       - This means that after you run the test once, you have to
> 	reboot, unless you use the "--skip-zero-pc8-check" option, but
> 	you really need to know what you're doing.

This needs to be made more robust since I want to include this test into
the normal -nightly testruns. Does comparing the difference of the
registers not work instead of checking for 0?

>   - You need at least one output connected.

The test should skip (retun 77;) if that's the case.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  tests/.gitignore  |   1 +
>  tests/Makefile.am |   1 +
>  tests/pc8.c       | 555 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 557 insertions(+)
>  create mode 100644 tests/pc8.c
> 
> diff --git a/tests/.gitignore b/tests/.gitignore
> index 451ab2d..c70fdb7 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -90,6 +90,7 @@ getstats
>  getversion
>  kms_flip
>  kms_render
> +pc8
>  prime_nv_api
>  prime_nv_pcopy
>  prime_nv_test
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index a59c25f..3507716 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -2,6 +2,7 @@ if BUILD_TESTS
>  noinst_PROGRAMS = \
>  	gem_stress \
>  	ddi_compute_wrpll \
> +	pc8 \
>  	$(TESTS_progs) \
>  	$(TESTS_progs_M) \
>  	$(HANG) \
> diff --git a/tests/pc8.c b/tests/pc8.c
> new file mode 100644
> index 0000000..7fdfeb5
> --- /dev/null
> +++ b/tests/pc8.c
> @@ -0,0 +1,555 @@
> +/*
> + * 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 <assert.h>
> +#include <stdint.h>
> +#include <stdbool.h>
> +#include <string.h>
> +
> +#include <unistd.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +
> +#include "drm.h"
> +#include "drmtest.h"
> +#include "intel_batchbuffer.h"
> +#include "intel_gpu_tools.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;
> +
> +/* Stuff used when creating FBs and mode setting. */
> +struct mode_set_data {
> +	drmModeResPtr res;
> +	drmModeConnectorPtr connectors[MAX_CONNECTORS];
> +
> +	drm_intel_bufmgr *bufmgr;
> +	uint32_t devid;
> +};
> +
> +enum compare_step {
> +	STEP_RESET,
> +	STEP_RESTORED,
> +};
> +
> +/* 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 {
> +		/* 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;
> +	} regs;
> +};
> +
> +static uint64_t get_residency(int fd, uint32_t type)
> +{
> +	int rc;
> +	uint64_t ret;
> +
> +	rc = pread(fd, &ret, sizeof(uint64_t), type);
> +	assert(rc == sizeof(uint64_t));
> +
> +	return ret;
> +}
> +
> +static bool pc8_plus_residency_is_zero(int fd)
> +{
> +	uint64_t res_pc8, res_pc9, res_pc10;
> +
> +	res_pc8 = get_residency(fd, MSR_PC8_RES);
> +	res_pc9 = get_residency(fd, MSR_PC9_RES);
> +	res_pc10 = get_residency(fd, MSR_PC10_RES);
> +
> +	if (res_pc8 != 0 || res_pc9 != 0 || res_pc10 != 0)
> +		return false;
> +
> +	return true;
> +}
> +
> +static bool pc8_plus_residency_changed(int fd, unsigned int timeout_sec)
> +{
> +	unsigned int i;
> +	uint64_t res_pc8, res_pc9, res_pc10;
> +	int to_sleep = 100 * 1000;
> +
> +	res_pc8 = get_residency(fd, MSR_PC8_RES);
> +	res_pc9 = get_residency(fd, MSR_PC9_RES);
> +	res_pc10 = get_residency(fd, MSR_PC10_RES);
> +
> +	for (i = 0; i < timeout_sec * 1000 * 1000; i += to_sleep) {
> +		if (res_pc8 != get_residency(fd, MSR_PC8_RES) ||
> +		    res_pc9 != get_residency(fd, MSR_PC9_RES) ||
> +		    res_pc10 != get_residency(fd, MSR_PC10_RES)) {
> +			return true;
> +		}
> +		usleep(to_sleep);
> +	}
> +
> +	return false;
> +}
> +
> +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);
> +		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);
> +
> +	assert(crtc_id);
> +	assert(buffer_id);
> +	assert(connector_id);
> +	assert(mode);
> +
> +	rc = drmModeSetCrtc(drm_fd, crtc_id, buffer_id, 0, 0, &connector_id,
> +			    1, mode);
> +	assert(rc == 0);
> +}
> +
> +static void get_connector_edid(struct compare_data *data, int index)
> +{
> +	unsigned int i;
> +	drmModeConnectorPtr connector = data->connectors[index];
> +	drmModeObjectPropertiesPtr props;
> +
> +	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) {
> +			assert(prop->flags & DRM_MODE_PROP_BLOB);
> +			assert(prop->count_blobs == 0);
> +			data->edids[index] = drmModeGetPropertyBlob(drm_fd,
> +							props->prop_values[i]);
> +		}
> +
> +		drmModeFreeProperty(prop);
> +	}
> +
> +	drmModeFreeObjectProperties(props);
> +}
> +
> +static void init_mode_set_data(struct mode_set_data *data)
> +{
> +	int i;
> +
> +	data->res = drmModeGetResources(drm_fd);
> +	assert(data->res);
> +	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->bufmgr = drm_intel_bufmgr_gem_init(drm_fd, 4096);
> +	data->devid = intel_get_drm_devid(drm_fd);
> +
> +	do_or_die(drmtest_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]);
> +	drmModeFreeResources(data->res);
> +}
> +
> +static void get_drm_info(struct compare_data *data)
> +{
> +	int i;
> +
> +	data->res = drmModeGetResources(drm_fd);
> +	assert(data->res);
> +
> +	assert(data->res->count_connectors <= MAX_CONNECTORS);
> +	assert(data->res->count_encoders <= MAX_ENCODERS);
> +	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]);
> +		get_connector_edid(data, 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]);
> +
> +	intel_register_access_init(intel_get_pci_device(), 0);
> +	data->regs.arb_mode = INREG(0x4030);
> +	data->regs.tilectl = INREG(0x101000);
> +	data->regs.gen6_ucgctl2 = INREG(0x9404);
> +	data->regs.gen7_l3cntlreg1 = INREG(0xB0C1);
> +	data->regs.transa_chicken1 = INREG(0xF0060);
> +	data->regs.deier = INREG(0x4400C);
> +	data->regs.gtier = INREG(0x4401C);
> +	data->regs.ddi_buf_trans_a_1 = INREG(0x64E00);
> +	data->regs.ddi_buf_trans_b_5 = INREG(0x64E70);
> +	data->regs.ddi_buf_trans_c_10 = INREG(0x64EE0);
> +	data->regs.ddi_buf_trans_d_15 = INREG(0x64F58);
> +	data->regs.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) assert(d1->data == d2->data)
> +#define COMPARE_ARRAY(d1, d2, size, data) do { \
> +	for (i = 0; i < size; i++) \
> +		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);
> +	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;
> +	assert(e1 && e2);
> +
> +	COMPARE(e1, e2, id);
> +	COMPARE(e1, e2, length);
> +
> +	assert(memcmp(e1->data, e2->data, e1->length) == 0);
> +}

Imo the important part is just checking whether the EDID is still the
same, but I guess we can keep all the other checks, too.

> +
> +static void compare_registers(struct compare_data *d1, struct compare_data *d2,
> +			      enum compare_step step)
> +{
> +	/* Things that shouldn't change at all. */
> +	COMPARE(d1, d2, regs.gen6_ucgctl2);
> +	COMPARE(d1, d2, regs.gen7_l3cntlreg1);
> +	COMPARE(d1, d2, regs.transa_chicken1);
> +	COMPARE(d1, d2, regs.arb_mode);
> +	COMPARE(d1, d2, regs.tilectl);
> +	assert(d1->regs.deier & (1 << 31));
> +	assert(d2->regs.deier & (1 << 31));
> +
> +	switch (step) {
> +	case STEP_RESET:
> +		assert(d2->regs.gtier == 0);
> +		assert(d2->regs.ddi_buf_trans_a_1 == 0);
> +		assert(d2->regs.ddi_buf_trans_b_5 == 0);
> +		assert(d2->regs.ddi_buf_trans_c_10 == 0);
> +		assert(d2->regs.ddi_buf_trans_d_15 == 0);
> +		assert(d2->regs.ddi_buf_trans_e_20 == 0);
> +		break;
> +	case STEP_RESTORED:
> +		COMPARE(d1, d2, regs.arb_mode);
> +		COMPARE(d1, d2, regs.tilectl);
> +		COMPARE(d1, d2, regs.gtier);
> +		COMPARE(d1, d2, regs.ddi_buf_trans_a_1);
> +		COMPARE(d1, d2, regs.ddi_buf_trans_b_5);
> +		COMPARE(d1, d2, regs.ddi_buf_trans_c_10);
> +		COMPARE(d1, d2, regs.ddi_buf_trans_d_15);
> +		COMPARE(d1, d2, regs.ddi_buf_trans_e_20);
> +		break;
> +	default:
> +		assert(0);
> +	}
> +}

This is really risky since we've just demonstrated that we can hard-hang
hsw if someone concurrently accesses registers. I guess hiding this behind
a debug switch would be good.

> +
> +static void assert_drm_infos_equal(struct compare_data *d1,
> +				   struct compare_data *d2,
> +				   enum compare_step step)
> +{
> +	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]);
> +
> +	compare_registers(d1, d2, step);
> +}
> +
> +#define BUF_SIZE	(8 << 20)
> +
> +/* to avoid stupid depencies on libdrm, copy&paste */
> +struct local_drm_i915_gem_wait {
> +	/** Handle of BO we shall wait on */
> +	__u32 bo_handle;
> +	__u32 flags;
> +	/** Number of nanoseconds to wait, Returns time remaining. */
> +	__u64 timeout_ns;
> +};
> +#define WAIT_IOCTL DRM_IOWR(DRM_COMMAND_BASE + 0x2c, struct local_drm_i915_gem_wait)
> +
> +static int gem_bo_wait_timeout(int fd, uint32_t handle, uint64_t *timeout_ns)
> +{
> +	struct local_drm_i915_gem_wait wait;
> +	int ret;
> +
> +	assert(timeout_ns);
> +
> +	wait.bo_handle = handle;
> +	wait.timeout_ns = *timeout_ns;
> +	wait.flags = 0;
> +	ret = drmIoctl(fd, WAIT_IOCTL, &wait);
> +	*timeout_ns = wait.timeout_ns;
> +
> +	return ret ? -errno : 0;
> +}

This should be extracted to lib/drmtest.c for all testcases.

> +
> +static void test_batch(struct mode_set_data *data)
> +{
> +	uint64_t timeout = 1000 * 1000 * 1000 * 2;
> +	drm_intel_bo *dst;
> +
> +	dst = drm_intel_bo_alloc(data->bufmgr, "dst", BUF_SIZE, 4096);
> +	if (gem_bo_wait_timeout(drm_fd, dst->handle, &timeout) == -EINVAL)
> +		printf("Kernel doesn't support wait_timeout\n");
> +}

Since I don't submit a "submit a batch" anywhere this won't actually test
much ...

> +
> +int main(int argc, char *argv[])
> +{
> +	struct mode_set_data ms_data;
> +	struct compare_data pre_pc8, during_pc8, post_pc8;
> +	int msr_fd;
> +	bool skip_zero_pc8_check = false;
> +
> +	if (argc > 1) {
> +		/* With this option we can test/debug/develop the tool without
> +		 * needing to reboot every time. */
> +		if (strcmp(argv[1], "--skip-zero-pc8-check") == 0)
> +			skip_zero_pc8_check = true;
> +	}
> +
> +	drm_fd = drm_open_any();
> +	assert(drm_fd > 0);
> +
> +	init_mode_set_data(&ms_data);
> +
> +	if (!IS_HASWELL(ms_data.devid)) {
> +		printf("PC8+ feature only supported on Haswell.\n");
> +		return 77;
> +	}
> +
> +	msr_fd = open("/dev/cpu/0/msr", O_RDONLY);
> +	assert(msr_fd != -1);
> +
> +	/* Make sure the machine still didn't enter PC8+, otherwise we won't be
> +	 * able to really compare pre-PC8+ with post-PC8+. */
> +	printf("Pre-PC8+\n");
> +	assert(skip_zero_pc8_check || pc8_plus_residency_is_zero(msr_fd));
> +	get_drm_info(&pre_pc8);
> +	test_batch(&ms_data);
> +	assert(skip_zero_pc8_check || pc8_plus_residency_is_zero(msr_fd));
> +
> +	printf("PC8+\n");
> +	disable_all_screens(&ms_data);
> +	sleep(1);
> +
> +	assert(pc8_plus_residency_changed(msr_fd, 10));
> +	get_drm_info(&during_pc8);
> +	test_batch(&ms_data);
> +	assert(pc8_plus_residency_changed(msr_fd, 10));

Imo we should test each functional piece seperately in it's own loop, i.e.

for each foo in test_batch, test_edid_connector1, test_edid_connector2,
... ; do
	enter pc8+
	sleep 1
	run $foo
	exit pc8+
done

In the current implementation this doesn't matter, but if we change it
so that some of the tests will exit pc8+ and if we furthermore add a
slight delay for reentering pc8+ then the test won't test a lot any more.
So I think we need this for test robustness.

Also I think we should add a basic raw i2c transaction probe test. There
are a bunch of platforms out there which use GMBUS for other fun stuff
than reading EDIDs from external screens.

I think it would be good to also convert this test to the subtest
infrastructure so that QA can track the different pieces better. Note
though that the list of subtests can't change at runtime, so we can only
have one generic edid/raw-i2c subtest.

Cheers, Daniel

> +
> +	assert_drm_infos_equal(&pre_pc8, &during_pc8, STEP_RESET);
> +
> +	printf("Post-PC8+\n");
> +	enable_one_screen(&ms_data);
> +	sleep(1);
> +
> +	assert(!pc8_plus_residency_changed(msr_fd, 5));
> +	get_drm_info(&post_pc8);
> +	test_batch(&ms_data);
> +	assert(!pc8_plus_residency_changed(msr_fd, 5));
> +
> +	assert_drm_infos_equal(&pre_pc8, &post_pc8, STEP_RESTORED);
> +
> +	free_drm_info(&pre_pc8);
> +	free_drm_info(&during_pc8);
> +	free_drm_info(&post_pc8);
> +	fini_mode_set_data(&ms_data);
> +	close(msr_fd);
> +	drmClose(drm_fd);
> +
> +	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 5/8] drm/i915: avoid waking up from PC8 on GMBUS operations

[Daniel Vetter]

On Tue, Jul 30, 2013 at 10:30:41AM +0100, Chris Wilson wrote:
> On Mon, Jul 29, 2013 at 05:48:24PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > If we're already allowing PC8, just don't use the IRQs, so we won't
> > need to wake from PC8. Waking up from PC8 is a slow thing, so avoid it
> > when we can.
> 
> You would also need to explain that the GMBUS is outside of the display
> power well.
> 
> Looks reasonable, the only bit is moving the read of forbid_count into
> hsw_pc8_enabled() so that the gmbus code isn't poking around with
> someone else's locks, and we can safely do an unlocked optimistic read
> here.

IIrc EDID reads with interrupts take 22ms, without them they can easily
take 100ms. Is pc8+ exit indeed longer than that difference?

If the issue is that we flip-flop between pc8+ allow/deny too often then
we could just add a slight delay.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 5/8] drm/i915: avoid waking up from PC8 on GMBUS operations

[Paulo Zanoni]

2013/8/5 Daniel Vetter <daniel@ffwll.ch>:
> On Tue, Jul 30, 2013 at 10:30:41AM +0100, Chris Wilson wrote:
>> On Mon, Jul 29, 2013 at 05:48:24PM -0300, Paulo Zanoni wrote:
>> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >
>> > If we're already allowing PC8, just don't use the IRQs, so we won't
>> > need to wake from PC8. Waking up from PC8 is a slow thing, so avoid it
>> > when we can.
>>
>> You would also need to explain that the GMBUS is outside of the display
>> power well.
>>
>> Looks reasonable, the only bit is moving the read of forbid_count into
>> hsw_pc8_enabled() so that the gmbus code isn't poking around with
>> someone else's locks, and we can safely do an unlocked optimistic read
>> here.
>
> IIrc EDID reads with interrupts take 22ms, without them they can easily
> take 100ms. Is pc8+ exit indeed longer than that difference?

My tests consisted of running "xrandr" in a loop, When you do that,
you allow/disallow PC8 many many many times per xrandr invocation, so
it's not about pc8+ exit taking longer than 100ms, it's about adding
up all the pc8 allow/disallow times that happen in between everything.
I didn't measure the times, but I can notice the difference by seeing
how faster my terminal scrolls with the new code.

>
> If the issue is that we flip-flop between pc8+ allow/deny too often then
> we could just add a slight delay.

Yeah, if we delay the code to reallow PC8+ we'll be able to reduce the
slowdown. I even think that 1 second for a delayed PC8 reallow seems
reasonable (after all, the screen is off). But I though that the
current "do everything without even waking up from PC8" looked much
much prettier :)


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



-- 
Paulo Zanoni

Re: [PATCH i-g-t] tests: add pc8

[Paulo Zanoni]

2013/8/5 Daniel Vetter <daniel@ffwll.ch>:
> On Mon, Jul 29, 2013 at 05:53:27PM -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.
>>   - Before running the test, the machine needs to have 0 PC8+
>>     residency. If you run the test after the machine already has PC8+
>>     residency, it means you'll be comparing post-PC8+ with post-PC8+,
>>     so you won't be able to get things lost after the first time we
>>     enter PC8+.
>>       - This means that after you run the test once, you have to
>>       reboot, unless you use the "--skip-zero-pc8-check" option, but
>>       you really need to know what you're doing.
>
> This needs to be made more robust since I want to include this test into
> the normal -nightly testruns.

Notice I added the test to noinst_PROGRAMS, so QA won't be running it
for now, even if we merge it. Why? Because of these restrictions
mentioned above.

> Does comparing the difference of the
> registers not work instead of checking for 0?

Can you please elaborate more on this sentence? I'm not sure what
you're suggesting here.

The problem is: let's imagine there's some important register that we
initialize when starting the driver, but then we don't touch it
anymore. And this register is one of the registers that get reset when
we enter PC8, but our code doesn't fix it after leaving PC8. So if you
boot your machine, do something to allow PC8+ and then disallow it,
the register will go back to the "reset" state (which isn't guaranteed
to be 0x0, especially on display registers). Then you run tests/pc8:
it reads the register (which contains the "reset" value instead of the
real value set by our driver when it got loaded, because we already
entered PC8+ once), enters PC8+, leaves PC8+, reads the register
again, notices the value is the same and then gives us a "PASS". On
the other hand, if you didn't reach PC8+ before running tests/pc8,
then it would have noticed the value of the register changes.

In other words: if some register gets initialized by our driver to a
non-default value, but this value gets lost after we enter/leave PC8+,
we will *only* be able to notice the difference when comparing a state
where we *never* entered PC8+ against a state where we "already
entered PC8+ at least once", because after the first time you
enter/leave PC8+ you'll already have reset the register, so you'll be
comparing bugged state against bugged state.

>
>>   - You need at least one output connected.
>
> The test should skip (retun 77;) if that's the case.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  tests/.gitignore  |   1 +
>>  tests/Makefile.am |   1 +
>>  tests/pc8.c       | 555 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 557 insertions(+)
>>  create mode 100644 tests/pc8.c
>>
>> diff --git a/tests/.gitignore b/tests/.gitignore
>> index 451ab2d..c70fdb7 100644
>> --- a/tests/.gitignore
>> +++ b/tests/.gitignore
>> @@ -90,6 +90,7 @@ getstats
>>  getversion
>>  kms_flip
>>  kms_render
>> +pc8
>>  prime_nv_api
>>  prime_nv_pcopy
>>  prime_nv_test
>> diff --git a/tests/Makefile.am b/tests/Makefile.am
>> index a59c25f..3507716 100644
>> --- a/tests/Makefile.am
>> +++ b/tests/Makefile.am
>> @@ -2,6 +2,7 @@ if BUILD_TESTS
>>  noinst_PROGRAMS = \
>>       gem_stress \
>>       ddi_compute_wrpll \
>> +     pc8 \
>>       $(TESTS_progs) \
>>       $(TESTS_progs_M) \
>>       $(HANG) \
>> diff --git a/tests/pc8.c b/tests/pc8.c
>> new file mode 100644
>> index 0000000..7fdfeb5
>> --- /dev/null
>> +++ b/tests/pc8.c
>> @@ -0,0 +1,555 @@
>> +/*
>> + * 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 <assert.h>
>> +#include <stdint.h>
>> +#include <stdbool.h>
>> +#include <string.h>
>> +
>> +#include <unistd.h>
>> +#include <sys/types.h>
>> +#include <sys/stat.h>
>> +#include <fcntl.h>
>> +
>> +#include "drm.h"
>> +#include "drmtest.h"
>> +#include "intel_batchbuffer.h"
>> +#include "intel_gpu_tools.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;
>> +
>> +/* Stuff used when creating FBs and mode setting. */
>> +struct mode_set_data {
>> +     drmModeResPtr res;
>> +     drmModeConnectorPtr connectors[MAX_CONNECTORS];
>> +
>> +     drm_intel_bufmgr *bufmgr;
>> +     uint32_t devid;
>> +};
>> +
>> +enum compare_step {
>> +     STEP_RESET,
>> +     STEP_RESTORED,
>> +};
>> +
>> +/* 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 {
>> +             /* 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;
>> +     } regs;
>> +};
>> +
>> +static uint64_t get_residency(int fd, uint32_t type)
>> +{
>> +     int rc;
>> +     uint64_t ret;
>> +
>> +     rc = pread(fd, &ret, sizeof(uint64_t), type);
>> +     assert(rc == sizeof(uint64_t));
>> +
>> +     return ret;
>> +}
>> +
>> +static bool pc8_plus_residency_is_zero(int fd)
>> +{
>> +     uint64_t res_pc8, res_pc9, res_pc10;
>> +
>> +     res_pc8 = get_residency(fd, MSR_PC8_RES);
>> +     res_pc9 = get_residency(fd, MSR_PC9_RES);
>> +     res_pc10 = get_residency(fd, MSR_PC10_RES);
>> +
>> +     if (res_pc8 != 0 || res_pc9 != 0 || res_pc10 != 0)
>> +             return false;
>> +
>> +     return true;
>> +}
>> +
>> +static bool pc8_plus_residency_changed(int fd, unsigned int timeout_sec)
>> +{
>> +     unsigned int i;
>> +     uint64_t res_pc8, res_pc9, res_pc10;
>> +     int to_sleep = 100 * 1000;
>> +
>> +     res_pc8 = get_residency(fd, MSR_PC8_RES);
>> +     res_pc9 = get_residency(fd, MSR_PC9_RES);
>> +     res_pc10 = get_residency(fd, MSR_PC10_RES);
>> +
>> +     for (i = 0; i < timeout_sec * 1000 * 1000; i += to_sleep) {
>> +             if (res_pc8 != get_residency(fd, MSR_PC8_RES) ||
>> +                 res_pc9 != get_residency(fd, MSR_PC9_RES) ||
>> +                 res_pc10 != get_residency(fd, MSR_PC10_RES)) {
>> +                     return true;
>> +             }
>> +             usleep(to_sleep);
>> +     }
>> +
>> +     return false;
>> +}
>> +
>> +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);
>> +             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);
>> +
>> +     assert(crtc_id);
>> +     assert(buffer_id);
>> +     assert(connector_id);
>> +     assert(mode);
>> +
>> +     rc = drmModeSetCrtc(drm_fd, crtc_id, buffer_id, 0, 0, &connector_id,
>> +                         1, mode);
>> +     assert(rc == 0);
>> +}
>> +
>> +static void get_connector_edid(struct compare_data *data, int index)
>> +{
>> +     unsigned int i;
>> +     drmModeConnectorPtr connector = data->connectors[index];
>> +     drmModeObjectPropertiesPtr props;
>> +
>> +     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) {
>> +                     assert(prop->flags & DRM_MODE_PROP_BLOB);
>> +                     assert(prop->count_blobs == 0);
>> +                     data->edids[index] = drmModeGetPropertyBlob(drm_fd,
>> +                                                     props->prop_values[i]);
>> +             }
>> +
>> +             drmModeFreeProperty(prop);
>> +     }
>> +
>> +     drmModeFreeObjectProperties(props);
>> +}
>> +
>> +static void init_mode_set_data(struct mode_set_data *data)
>> +{
>> +     int i;
>> +
>> +     data->res = drmModeGetResources(drm_fd);
>> +     assert(data->res);
>> +     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->bufmgr = drm_intel_bufmgr_gem_init(drm_fd, 4096);
>> +     data->devid = intel_get_drm_devid(drm_fd);
>> +
>> +     do_or_die(drmtest_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]);
>> +     drmModeFreeResources(data->res);
>> +}
>> +
>> +static void get_drm_info(struct compare_data *data)
>> +{
>> +     int i;
>> +
>> +     data->res = drmModeGetResources(drm_fd);
>> +     assert(data->res);
>> +
>> +     assert(data->res->count_connectors <= MAX_CONNECTORS);
>> +     assert(data->res->count_encoders <= MAX_ENCODERS);
>> +     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]);
>> +             get_connector_edid(data, 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]);
>> +
>> +     intel_register_access_init(intel_get_pci_device(), 0);
>> +     data->regs.arb_mode = INREG(0x4030);
>> +     data->regs.tilectl = INREG(0x101000);
>> +     data->regs.gen6_ucgctl2 = INREG(0x9404);
>> +     data->regs.gen7_l3cntlreg1 = INREG(0xB0C1);
>> +     data->regs.transa_chicken1 = INREG(0xF0060);
>> +     data->regs.deier = INREG(0x4400C);
>> +     data->regs.gtier = INREG(0x4401C);
>> +     data->regs.ddi_buf_trans_a_1 = INREG(0x64E00);
>> +     data->regs.ddi_buf_trans_b_5 = INREG(0x64E70);
>> +     data->regs.ddi_buf_trans_c_10 = INREG(0x64EE0);
>> +     data->regs.ddi_buf_trans_d_15 = INREG(0x64F58);
>> +     data->regs.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) assert(d1->data == d2->data)
>> +#define COMPARE_ARRAY(d1, d2, size, data) do { \
>> +     for (i = 0; i < size; i++) \
>> +             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);
>> +     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;
>> +     assert(e1 && e2);
>> +
>> +     COMPARE(e1, e2, id);
>> +     COMPARE(e1, e2, length);
>> +
>> +     assert(memcmp(e1->data, e2->data, e1->length) == 0);
>> +}
>
> Imo the important part is just checking whether the EDID is still the
> same, but I guess we can keep all the other checks, too.
>
>> +
>> +static void compare_registers(struct compare_data *d1, struct compare_data *d2,
>> +                           enum compare_step step)
>> +{
>> +     /* Things that shouldn't change at all. */
>> +     COMPARE(d1, d2, regs.gen6_ucgctl2);
>> +     COMPARE(d1, d2, regs.gen7_l3cntlreg1);
>> +     COMPARE(d1, d2, regs.transa_chicken1);
>> +     COMPARE(d1, d2, regs.arb_mode);
>> +     COMPARE(d1, d2, regs.tilectl);
>> +     assert(d1->regs.deier & (1 << 31));
>> +     assert(d2->regs.deier & (1 << 31));
>> +
>> +     switch (step) {
>> +     case STEP_RESET:
>> +             assert(d2->regs.gtier == 0);
>> +             assert(d2->regs.ddi_buf_trans_a_1 == 0);
>> +             assert(d2->regs.ddi_buf_trans_b_5 == 0);
>> +             assert(d2->regs.ddi_buf_trans_c_10 == 0);
>> +             assert(d2->regs.ddi_buf_trans_d_15 == 0);
>> +             assert(d2->regs.ddi_buf_trans_e_20 == 0);
>> +             break;
>> +     case STEP_RESTORED:
>> +             COMPARE(d1, d2, regs.arb_mode);
>> +             COMPARE(d1, d2, regs.tilectl);
>> +             COMPARE(d1, d2, regs.gtier);
>> +             COMPARE(d1, d2, regs.ddi_buf_trans_a_1);
>> +             COMPARE(d1, d2, regs.ddi_buf_trans_b_5);
>> +             COMPARE(d1, d2, regs.ddi_buf_trans_c_10);
>> +             COMPARE(d1, d2, regs.ddi_buf_trans_d_15);
>> +             COMPARE(d1, d2, regs.ddi_buf_trans_e_20);
>> +             break;
>> +     default:
>> +             assert(0);
>> +     }
>> +}
>
> This is really risky since we've just demonstrated that we can hard-hang
> hsw if someone concurrently accesses registers. I guess hiding this behind
> a debug switch would be good.

Shouldn't we add some sort of debugfs interface to read/write
registers then? We don't want to hang the machine while running
intel_reg_dumper and all our other tools... These tests are important
since they catch bugs that actually *happened* while I was developing
this feature. Most of the other tests are for things people have
suggested, not bugs that really happened...


>
>> +
>> +static void assert_drm_infos_equal(struct compare_data *d1,
>> +                                struct compare_data *d2,
>> +                                enum compare_step step)
>> +{
>> +     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]);
>> +
>> +     compare_registers(d1, d2, step);
>> +}
>> +
>> +#define BUF_SIZE     (8 << 20)
>> +
>> +/* to avoid stupid depencies on libdrm, copy&paste */
>> +struct local_drm_i915_gem_wait {
>> +     /** Handle of BO we shall wait on */
>> +     __u32 bo_handle;
>> +     __u32 flags;
>> +     /** Number of nanoseconds to wait, Returns time remaining. */
>> +     __u64 timeout_ns;
>> +};
>> +#define WAIT_IOCTL DRM_IOWR(DRM_COMMAND_BASE + 0x2c, struct local_drm_i915_gem_wait)
>> +
>> +static int gem_bo_wait_timeout(int fd, uint32_t handle, uint64_t *timeout_ns)
>> +{
>> +     struct local_drm_i915_gem_wait wait;
>> +     int ret;
>> +
>> +     assert(timeout_ns);
>> +
>> +     wait.bo_handle = handle;
>> +     wait.timeout_ns = *timeout_ns;
>> +     wait.flags = 0;
>> +     ret = drmIoctl(fd, WAIT_IOCTL, &wait);
>> +     *timeout_ns = wait.timeout_ns;
>> +
>> +     return ret ? -errno : 0;
>> +}
>
> This should be extracted to lib/drmtest.c for all testcases.
>
>> +
>> +static void test_batch(struct mode_set_data *data)
>> +{
>> +     uint64_t timeout = 1000 * 1000 * 1000 * 2;
>> +     drm_intel_bo *dst;
>> +
>> +     dst = drm_intel_bo_alloc(data->bufmgr, "dst", BUF_SIZE, 4096);
>> +     if (gem_bo_wait_timeout(drm_fd, dst->handle, &timeout) == -EINVAL)
>> +             printf("Kernel doesn't support wait_timeout\n");
>> +}
>
> Since I don't submit a "submit a batch" anywhere this won't actually test
> much ...
>
>> +
>> +int main(int argc, char *argv[])
>> +{
>> +     struct mode_set_data ms_data;
>> +     struct compare_data pre_pc8, during_pc8, post_pc8;
>> +     int msr_fd;
>> +     bool skip_zero_pc8_check = false;
>> +
>> +     if (argc > 1) {
>> +             /* With this option we can test/debug/develop the tool without
>> +              * needing to reboot every time. */
>> +             if (strcmp(argv[1], "--skip-zero-pc8-check") == 0)
>> +                     skip_zero_pc8_check = true;
>> +     }
>> +
>> +     drm_fd = drm_open_any();
>> +     assert(drm_fd > 0);
>> +
>> +     init_mode_set_data(&ms_data);
>> +
>> +     if (!IS_HASWELL(ms_data.devid)) {
>> +             printf("PC8+ feature only supported on Haswell.\n");
>> +             return 77;
>> +     }
>> +
>> +     msr_fd = open("/dev/cpu/0/msr", O_RDONLY);
>> +     assert(msr_fd != -1);
>> +
>> +     /* Make sure the machine still didn't enter PC8+, otherwise we won't be
>> +      * able to really compare pre-PC8+ with post-PC8+. */
>> +     printf("Pre-PC8+\n");
>> +     assert(skip_zero_pc8_check || pc8_plus_residency_is_zero(msr_fd));
>> +     get_drm_info(&pre_pc8);
>> +     test_batch(&ms_data);
>> +     assert(skip_zero_pc8_check || pc8_plus_residency_is_zero(msr_fd));
>> +
>> +     printf("PC8+\n");
>> +     disable_all_screens(&ms_data);
>> +     sleep(1);
>> +
>> +     assert(pc8_plus_residency_changed(msr_fd, 10));
>> +     get_drm_info(&during_pc8);
>> +     test_batch(&ms_data);
>> +     assert(pc8_plus_residency_changed(msr_fd, 10));
>
> Imo we should test each functional piece seperately in it's own loop, i.e.
>
> for each foo in test_batch, test_edid_connector1, test_edid_connector2,
> ... ; do
>         enter pc8+
>         sleep 1
>         run $foo
>         exit pc8+
> done

This is not possible since the whole goal is to compare the state
"before we have ever entered PC8+" with "after we have entered PC8+".
See the big explanation above. But I could try to split the "collect
data" with the  "compare data" steps, so the "compare data" could
become the individual tests you requested.


>
> In the current implementation this doesn't matter, but if we change it
> so that some of the tests will exit pc8+ and if we furthermore add a
> slight delay for reentering pc8+ then the test won't test a lot any more.

That's why we have functions that sleep for up to 10 seconds waiting
for the PC8+ residency to move. If we take too much time to go back to
PC8, then we should increase those 10second timeouts. In fact we could
make those timeouts become even more than 1 minute since we expect to
return before that (i.e., when the PC8+ residency moves).



> So I think we need this for test robustness.
>
> Also I think we should add a basic raw i2c transaction probe test. There
> are a bunch of platforms out there which use GMBUS for other fun stuff
> than reading EDIDs from external screens.
>
> I think it would be good to also convert this test to the subtest
> infrastructure so that QA can track the different pieces better. Note
> though that the list of subtests can't change at runtime, so we can only
> have one generic edid/raw-i2c subtest.
>
> Cheers, Daniel
>
>> +
>> +     assert_drm_infos_equal(&pre_pc8, &during_pc8, STEP_RESET);
>> +
>> +     printf("Post-PC8+\n");
>> +     enable_one_screen(&ms_data);
>> +     sleep(1);
>> +
>> +     assert(!pc8_plus_residency_changed(msr_fd, 5));
>> +     get_drm_info(&post_pc8);
>> +     test_batch(&ms_data);
>> +     assert(!pc8_plus_residency_changed(msr_fd, 5));
>> +
>> +     assert_drm_infos_equal(&pre_pc8, &post_pc8, STEP_RESTORED);
>> +
>> +     free_drm_info(&pre_pc8);
>> +     free_drm_info(&during_pc8);
>> +     free_drm_info(&post_pc8);
>> +     fini_mode_set_data(&ms_data);
>> +     close(msr_fd);
>> +     drmClose(drm_fd);
>> +
>> +     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



-- 
Paulo Zanoni

Re: [PATCH i-g-t] tests: add pc8

[Daniel Vetter]

On Mon, Aug 05, 2013 at 10:42:08AM -0300, Paulo Zanoni wrote:
> 2013/8/5 Daniel Vetter <daniel@ffwll.ch>:
> > On Mon, Jul 29, 2013 at 05:53:27PM -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.
> >>   - Before running the test, the machine needs to have 0 PC8+
> >>     residency. If you run the test after the machine already has PC8+
> >>     residency, it means you'll be comparing post-PC8+ with post-PC8+,
> >>     so you won't be able to get things lost after the first time we
> >>     enter PC8+.
> >>       - This means that after you run the test once, you have to
> >>       reboot, unless you use the "--skip-zero-pc8-check" option, but
> >>       you really need to know what you're doing.
> >
> > This needs to be made more robust since I want to include this test into
> > the normal -nightly testruns.
> 
> Notice I added the test to noinst_PROGRAMS, so QA won't be running it
> for now, even if we merge it. Why? Because of these restrictions
> mentioned above.

Yep I've spotted that, but that needs to be fixed ;-)
> 
> > Does comparing the difference of the
> > registers not work instead of checking for 0?
> 
> Can you please elaborate more on this sentence? I'm not sure what
> you're suggesting here.

Atm you check that the pc8 residency counters are 0, so we need to reboot
after having run the test once. For other similar stuff (e.g. the rc6
residency test) we take a snapshot before/after and check the differences
only. That way the test should work always.

> The problem is: let's imagine there's some important register that we
> initialize when starting the driver, but then we don't touch it
> anymore. And this register is one of the registers that get reset when
> we enter PC8, but our code doesn't fix it after leaving PC8. So if you
> boot your machine, do something to allow PC8+ and then disallow it,
> the register will go back to the "reset" state (which isn't guaranteed
> to be 0x0, especially on display registers). Then you run tests/pc8:
> it reads the register (which contains the "reset" value instead of the
> real value set by our driver when it got loaded, because we already
> entered PC8+ once), enters PC8+, leaves PC8+, reads the register
> again, notices the value is the same and then gives us a "PASS". On
> the other hand, if you didn't reach PC8+ before running tests/pc8,
> then it would have noticed the value of the register changes.
> 
> In other words: if some register gets initialized by our driver to a
> non-default value, but this value gets lost after we enter/leave PC8+,
> we will *only* be able to notice the difference when comparing a state
> where we *never* entered PC8+ against a state where we "already
> entered PC8+ at least once", because after the first time you
> enter/leave PC8+ you'll already have reset the register, so you'll be
> comparing bugged state against bugged state.

Ok, I see your point. But imo igt testcases shouldn't (at least with the
testcases run by default) have such detailed knowledge of what the kernel
actually does with the registers. So a depency like "this test needs to be
run after a fresh boot when we've never entered pc8+ yet" isn't good since
it'll make the testcase fragile.

Instead the test should check whether everything still works after we've
been in pc8+ for a bit. One exception could be w/a bits if we have a
common igt testcase to check for all of them. Then we could just rerun
that testcase.

But if e.g. the swizzling settings get lost over pc8+ it's better to add
an explicit functional swizzle test here instead of checking registers.

> 
> >
> >>   - You need at least one output connected.
> >
> > The test should skip (retun 77;) if that's the case.
> >>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> ---
> >>  tests/.gitignore  |   1 +
> >>  tests/Makefile.am |   1 +
> >>  tests/pc8.c       | 555 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 557 insertions(+)
> >>  create mode 100644 tests/pc8.c
> >>
> >> diff --git a/tests/.gitignore b/tests/.gitignore
> >> index 451ab2d..c70fdb7 100644
> >> --- a/tests/.gitignore
> >> +++ b/tests/.gitignore
> >> @@ -90,6 +90,7 @@ getstats
> >>  getversion
> >>  kms_flip
> >>  kms_render
> >> +pc8
> >>  prime_nv_api
> >>  prime_nv_pcopy
> >>  prime_nv_test
> >> diff --git a/tests/Makefile.am b/tests/Makefile.am
> >> index a59c25f..3507716 100644
> >> --- a/tests/Makefile.am
> >> +++ b/tests/Makefile.am
> >> @@ -2,6 +2,7 @@ if BUILD_TESTS
> >>  noinst_PROGRAMS = \
> >>       gem_stress \
> >>       ddi_compute_wrpll \
> >> +     pc8 \
> >>       $(TESTS_progs) \
> >>       $(TESTS_progs_M) \
> >>       $(HANG) \
> >> diff --git a/tests/pc8.c b/tests/pc8.c
> >> new file mode 100644
> >> index 0000000..7fdfeb5
> >> --- /dev/null
> >> +++ b/tests/pc8.c
> >> @@ -0,0 +1,555 @@
> >> +/*
> >> + * 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 <assert.h>
> >> +#include <stdint.h>
> >> +#include <stdbool.h>
> >> +#include <string.h>
> >> +
> >> +#include <unistd.h>
> >> +#include <sys/types.h>
> >> +#include <sys/stat.h>
> >> +#include <fcntl.h>
> >> +
> >> +#include "drm.h"
> >> +#include "drmtest.h"
> >> +#include "intel_batchbuffer.h"
> >> +#include "intel_gpu_tools.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;
> >> +
> >> +/* Stuff used when creating FBs and mode setting. */
> >> +struct mode_set_data {
> >> +     drmModeResPtr res;
> >> +     drmModeConnectorPtr connectors[MAX_CONNECTORS];
> >> +
> >> +     drm_intel_bufmgr *bufmgr;
> >> +     uint32_t devid;
> >> +};
> >> +
> >> +enum compare_step {
> >> +     STEP_RESET,
> >> +     STEP_RESTORED,
> >> +};
> >> +
> >> +/* 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 {
> >> +             /* 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;
> >> +     } regs;
> >> +};
> >> +
> >> +static uint64_t get_residency(int fd, uint32_t type)
> >> +{
> >> +     int rc;
> >> +     uint64_t ret;
> >> +
> >> +     rc = pread(fd, &ret, sizeof(uint64_t), type);
> >> +     assert(rc == sizeof(uint64_t));
> >> +
> >> +     return ret;
> >> +}
> >> +
> >> +static bool pc8_plus_residency_is_zero(int fd)
> >> +{
> >> +     uint64_t res_pc8, res_pc9, res_pc10;
> >> +
> >> +     res_pc8 = get_residency(fd, MSR_PC8_RES);
> >> +     res_pc9 = get_residency(fd, MSR_PC9_RES);
> >> +     res_pc10 = get_residency(fd, MSR_PC10_RES);
> >> +
> >> +     if (res_pc8 != 0 || res_pc9 != 0 || res_pc10 != 0)
> >> +             return false;
> >> +
> >> +     return true;
> >> +}
> >> +
> >> +static bool pc8_plus_residency_changed(int fd, unsigned int timeout_sec)
> >> +{
> >> +     unsigned int i;
> >> +     uint64_t res_pc8, res_pc9, res_pc10;
> >> +     int to_sleep = 100 * 1000;
> >> +
> >> +     res_pc8 = get_residency(fd, MSR_PC8_RES);
> >> +     res_pc9 = get_residency(fd, MSR_PC9_RES);
> >> +     res_pc10 = get_residency(fd, MSR_PC10_RES);
> >> +
> >> +     for (i = 0; i < timeout_sec * 1000 * 1000; i += to_sleep) {
> >> +             if (res_pc8 != get_residency(fd, MSR_PC8_RES) ||
> >> +                 res_pc9 != get_residency(fd, MSR_PC9_RES) ||
> >> +                 res_pc10 != get_residency(fd, MSR_PC10_RES)) {
> >> +                     return true;
> >> +             }
> >> +             usleep(to_sleep);
> >> +     }
> >> +
> >> +     return false;
> >> +}
> >> +
> >> +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);
> >> +             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);
> >> +
> >> +     assert(crtc_id);
> >> +     assert(buffer_id);
> >> +     assert(connector_id);
> >> +     assert(mode);
> >> +
> >> +     rc = drmModeSetCrtc(drm_fd, crtc_id, buffer_id, 0, 0, &connector_id,
> >> +                         1, mode);
> >> +     assert(rc == 0);
> >> +}
> >> +
> >> +static void get_connector_edid(struct compare_data *data, int index)
> >> +{
> >> +     unsigned int i;
> >> +     drmModeConnectorPtr connector = data->connectors[index];
> >> +     drmModeObjectPropertiesPtr props;
> >> +
> >> +     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) {
> >> +                     assert(prop->flags & DRM_MODE_PROP_BLOB);
> >> +                     assert(prop->count_blobs == 0);
> >> +                     data->edids[index] = drmModeGetPropertyBlob(drm_fd,
> >> +                                                     props->prop_values[i]);
> >> +             }
> >> +
> >> +             drmModeFreeProperty(prop);
> >> +     }
> >> +
> >> +     drmModeFreeObjectProperties(props);
> >> +}
> >> +
> >> +static void init_mode_set_data(struct mode_set_data *data)
> >> +{
> >> +     int i;
> >> +
> >> +     data->res = drmModeGetResources(drm_fd);
> >> +     assert(data->res);
> >> +     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->bufmgr = drm_intel_bufmgr_gem_init(drm_fd, 4096);
> >> +     data->devid = intel_get_drm_devid(drm_fd);
> >> +
> >> +     do_or_die(drmtest_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]);
> >> +     drmModeFreeResources(data->res);
> >> +}
> >> +
> >> +static void get_drm_info(struct compare_data *data)
> >> +{
> >> +     int i;
> >> +
> >> +     data->res = drmModeGetResources(drm_fd);
> >> +     assert(data->res);
> >> +
> >> +     assert(data->res->count_connectors <= MAX_CONNECTORS);
> >> +     assert(data->res->count_encoders <= MAX_ENCODERS);
> >> +     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]);
> >> +             get_connector_edid(data, 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]);
> >> +
> >> +     intel_register_access_init(intel_get_pci_device(), 0);
> >> +     data->regs.arb_mode = INREG(0x4030);
> >> +     data->regs.tilectl = INREG(0x101000);
> >> +     data->regs.gen6_ucgctl2 = INREG(0x9404);
> >> +     data->regs.gen7_l3cntlreg1 = INREG(0xB0C1);
> >> +     data->regs.transa_chicken1 = INREG(0xF0060);
> >> +     data->regs.deier = INREG(0x4400C);
> >> +     data->regs.gtier = INREG(0x4401C);
> >> +     data->regs.ddi_buf_trans_a_1 = INREG(0x64E00);
> >> +     data->regs.ddi_buf_trans_b_5 = INREG(0x64E70);
> >> +     data->regs.ddi_buf_trans_c_10 = INREG(0x64EE0);
> >> +     data->regs.ddi_buf_trans_d_15 = INREG(0x64F58);
> >> +     data->regs.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) assert(d1->data == d2->data)
> >> +#define COMPARE_ARRAY(d1, d2, size, data) do { \
> >> +     for (i = 0; i < size; i++) \
> >> +             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);
> >> +     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;
> >> +     assert(e1 && e2);
> >> +
> >> +     COMPARE(e1, e2, id);
> >> +     COMPARE(e1, e2, length);
> >> +
> >> +     assert(memcmp(e1->data, e2->data, e1->length) == 0);
> >> +}
> >
> > Imo the important part is just checking whether the EDID is still the
> > same, but I guess we can keep all the other checks, too.
> >
> >> +
> >> +static void compare_registers(struct compare_data *d1, struct compare_data *d2,
> >> +                           enum compare_step step)
> >> +{
> >> +     /* Things that shouldn't change at all. */
> >> +     COMPARE(d1, d2, regs.gen6_ucgctl2);
> >> +     COMPARE(d1, d2, regs.gen7_l3cntlreg1);
> >> +     COMPARE(d1, d2, regs.transa_chicken1);
> >> +     COMPARE(d1, d2, regs.arb_mode);
> >> +     COMPARE(d1, d2, regs.tilectl);
> >> +     assert(d1->regs.deier & (1 << 31));
> >> +     assert(d2->regs.deier & (1 << 31));
> >> +
> >> +     switch (step) {
> >> +     case STEP_RESET:
> >> +             assert(d2->regs.gtier == 0);
> >> +             assert(d2->regs.ddi_buf_trans_a_1 == 0);
> >> +             assert(d2->regs.ddi_buf_trans_b_5 == 0);
> >> +             assert(d2->regs.ddi_buf_trans_c_10 == 0);
> >> +             assert(d2->regs.ddi_buf_trans_d_15 == 0);
> >> +             assert(d2->regs.ddi_buf_trans_e_20 == 0);
> >> +             break;
> >> +     case STEP_RESTORED:
> >> +             COMPARE(d1, d2, regs.arb_mode);
> >> +             COMPARE(d1, d2, regs.tilectl);
> >> +             COMPARE(d1, d2, regs.gtier);
> >> +             COMPARE(d1, d2, regs.ddi_buf_trans_a_1);
> >> +             COMPARE(d1, d2, regs.ddi_buf_trans_b_5);
> >> +             COMPARE(d1, d2, regs.ddi_buf_trans_c_10);
> >> +             COMPARE(d1, d2, regs.ddi_buf_trans_d_15);
> >> +             COMPARE(d1, d2, regs.ddi_buf_trans_e_20);
> >> +             break;
> >> +     default:
> >> +             assert(0);
> >> +     }
> >> +}
> >
> > This is really risky since we've just demonstrated that we can hard-hang
> > hsw if someone concurrently accesses registers. I guess hiding this behind
> > a debug switch would be good.
> 
> Shouldn't we add some sort of debugfs interface to read/write
> registers then? We don't want to hang the machine while running
> intel_reg_dumper and all our other tools... These tests are important
> since they catch bugs that actually *happened* while I was developing
> this feature. Most of the other tests are for things people have
> suggested, not bugs that really happened...

On a quick look we have

- arb_mode: This could be caught by a generic "have we lost w/a bits"
  test. Similar issues apply for suspend/resume (we have a little helper
  for that now) and after gpu resets.
- tilectl: we have tons of swizzling tests. One gaping hole is currently
  what happens with the display swizzling, but if we get that wrong people
  will notice _really_ fast ;-) And I think we should be able to plug this
  hole with crc checksums. But as long as we don't yet have crc checksum
  infrastructure I'm willing to live with the gaps.
- gtier: I guess that results in seqno waits not really working. Like I've
  mentioned below I think your current test doesn't cover that properly
  yet.
- ddi_buf_trans control registers: I guess we could subsume that into the
  w/a bit checker simply as another pile of registers which need to have a
  special value.

For the register checker Eric started to work on one, but unfornately it's
not yet integrated anywhere into all relevant igt tests.

> 
> 
> >
> >> +
> >> +static void assert_drm_infos_equal(struct compare_data *d1,
> >> +                                struct compare_data *d2,
> >> +                                enum compare_step step)
> >> +{
> >> +     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]);
> >> +
> >> +     compare_registers(d1, d2, step);
> >> +}
> >> +
> >> +#define BUF_SIZE     (8 << 20)
> >> +
> >> +/* to avoid stupid depencies on libdrm, copy&paste */
> >> +struct local_drm_i915_gem_wait {
> >> +     /** Handle of BO we shall wait on */
> >> +     __u32 bo_handle;
> >> +     __u32 flags;
> >> +     /** Number of nanoseconds to wait, Returns time remaining. */
> >> +     __u64 timeout_ns;
> >> +};
> >> +#define WAIT_IOCTL DRM_IOWR(DRM_COMMAND_BASE + 0x2c, struct local_drm_i915_gem_wait)
> >> +
> >> +static int gem_bo_wait_timeout(int fd, uint32_t handle, uint64_t *timeout_ns)
> >> +{
> >> +     struct local_drm_i915_gem_wait wait;
> >> +     int ret;
> >> +
> >> +     assert(timeout_ns);
> >> +
> >> +     wait.bo_handle = handle;
> >> +     wait.timeout_ns = *timeout_ns;
> >> +     wait.flags = 0;
> >> +     ret = drmIoctl(fd, WAIT_IOCTL, &wait);
> >> +     *timeout_ns = wait.timeout_ns;
> >> +
> >> +     return ret ? -errno : 0;
> >> +}
> >
> > This should be extracted to lib/drmtest.c for all testcases.
> >
> >> +
> >> +static void test_batch(struct mode_set_data *data)
> >> +{
> >> +     uint64_t timeout = 1000 * 1000 * 1000 * 2;
> >> +     drm_intel_bo *dst;
> >> +
> >> +     dst = drm_intel_bo_alloc(data->bufmgr, "dst", BUF_SIZE, 4096);
> >> +     if (gem_bo_wait_timeout(drm_fd, dst->handle, &timeout) == -EINVAL)
> >> +             printf("Kernel doesn't support wait_timeout\n");
> >> +}
> >
> > Since I don't submit a "submit a batch" anywhere this won't actually test
> > much ...
> >
> >> +
> >> +int main(int argc, char *argv[])
> >> +{
> >> +     struct mode_set_data ms_data;
> >> +     struct compare_data pre_pc8, during_pc8, post_pc8;
> >> +     int msr_fd;
> >> +     bool skip_zero_pc8_check = false;
> >> +
> >> +     if (argc > 1) {
> >> +             /* With this option we can test/debug/develop the tool without
> >> +              * needing to reboot every time. */
> >> +             if (strcmp(argv[1], "--skip-zero-pc8-check") == 0)
> >> +                     skip_zero_pc8_check = true;
> >> +     }
> >> +
> >> +     drm_fd = drm_open_any();
> >> +     assert(drm_fd > 0);
> >> +
> >> +     init_mode_set_data(&ms_data);
> >> +
> >> +     if (!IS_HASWELL(ms_data.devid)) {
> >> +             printf("PC8+ feature only supported on Haswell.\n");
> >> +             return 77;
> >> +     }
> >> +
> >> +     msr_fd = open("/dev/cpu/0/msr", O_RDONLY);
> >> +     assert(msr_fd != -1);
> >> +
> >> +     /* Make sure the machine still didn't enter PC8+, otherwise we won't be
> >> +      * able to really compare pre-PC8+ with post-PC8+. */
> >> +     printf("Pre-PC8+\n");
> >> +     assert(skip_zero_pc8_check || pc8_plus_residency_is_zero(msr_fd));
> >> +     get_drm_info(&pre_pc8);
> >> +     test_batch(&ms_data);
> >> +     assert(skip_zero_pc8_check || pc8_plus_residency_is_zero(msr_fd));
> >> +
> >> +     printf("PC8+\n");
> >> +     disable_all_screens(&ms_data);
> >> +     sleep(1);
> >> +
> >> +     assert(pc8_plus_residency_changed(msr_fd, 10));
> >> +     get_drm_info(&during_pc8);
> >> +     test_batch(&ms_data);
> >> +     assert(pc8_plus_residency_changed(msr_fd, 10));
> >
> > Imo we should test each functional piece seperately in it's own loop, i.e.
> >
> > for each foo in test_batch, test_edid_connector1, test_edid_connector2,
> > ... ; do
> >         enter pc8+
> >         sleep 1
> >         run $foo
> >         exit pc8+
> > done
> 
> This is not possible since the whole goal is to compare the state
> "before we have ever entered PC8+" with "after we have entered PC8+".
> See the big explanation above. But I could try to split the "collect
> data" with the  "compare data" steps, so the "compare data" could
> become the individual tests you requested.

I don't think you can make the test robust enough with the current
approach, see above for a few ideas how this could be remedied.
> 
> 
> >
> > In the current implementation this doesn't matter, but if we change it
> > so that some of the tests will exit pc8+ and if we furthermore add a
> > slight delay for reentering pc8+ then the test won't test a lot any more.
> 
> That's why we have functions that sleep for up to 10 seconds waiting
> for the PC8+ residency to move. If we take too much time to go back to
> PC8, then we should increase those 10second timeouts. In fact we could
> make those timeouts become even more than 1 minute since we expect to
> return before that (i.e., when the PC8+ residency moves).

Hm, I guess I've missed that part of the test. But my point was that we
should check that we've reentered pc8+ between each subtest, i.e. every
time we've tried to read and edid, called test_batch.

But as long as we don't wake upthe chip out of pc8+ for these tests I
guess the current test is paranoid enough, but it needs to be changed
before we can frob the code. And since we currently have some aux power
domain get/put lingering that is essentially what can happen I think.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH i-g-t] tests: add pc8

[Paulo Zanoni]

2013/8/5 Daniel Vetter <daniel@ffwll.ch>:
> On Mon, Aug 05, 2013 at 10:42:08AM -0300, Paulo Zanoni wrote:
>> 2013/8/5 Daniel Vetter <daniel@ffwll.ch>:
>> > On Mon, Jul 29, 2013 at 05:53:27PM -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.
>> >>   - Before running the test, the machine needs to have 0 PC8+
>> >>     residency. If you run the test after the machine already has PC8+
>> >>     residency, it means you'll be comparing post-PC8+ with post-PC8+,
>> >>     so you won't be able to get things lost after the first time we
>> >>     enter PC8+.
>> >>       - This means that after you run the test once, you have to
>> >>       reboot, unless you use the "--skip-zero-pc8-check" option, but
>> >>       you really need to know what you're doing.
>> >
>> > This needs to be made more robust since I want to include this test into
>> > the normal -nightly testruns.
>>
>> Notice I added the test to noinst_PROGRAMS, so QA won't be running it
>> for now, even if we merge it. Why? Because of these restrictions
>> mentioned above.
>
> Yep I've spotted that, but that needs to be fixed ;-)
>>
>> > Does comparing the difference of the
>> > registers not work instead of checking for 0?
>>
>> Can you please elaborate more on this sentence? I'm not sure what
>> you're suggesting here.
>
> Atm you check that the pc8 residency counters are 0, so we need to reboot
> after having run the test once. For other similar stuff (e.g. the rc6
> residency test) we take a snapshot before/after and check the differences
> only. That way the test should work always.
>
>> The problem is: let's imagine there's some important register that we
>> initialize when starting the driver, but then we don't touch it
>> anymore. And this register is one of the registers that get reset when
>> we enter PC8, but our code doesn't fix it after leaving PC8. So if you
>> boot your machine, do something to allow PC8+ and then disallow it,
>> the register will go back to the "reset" state (which isn't guaranteed
>> to be 0x0, especially on display registers). Then you run tests/pc8:
>> it reads the register (which contains the "reset" value instead of the
>> real value set by our driver when it got loaded, because we already
>> entered PC8+ once), enters PC8+, leaves PC8+, reads the register
>> again, notices the value is the same and then gives us a "PASS". On
>> the other hand, if you didn't reach PC8+ before running tests/pc8,
>> then it would have noticed the value of the register changes.
>>
>> In other words: if some register gets initialized by our driver to a
>> non-default value, but this value gets lost after we enter/leave PC8+,
>> we will *only* be able to notice the difference when comparing a state
>> where we *never* entered PC8+ against a state where we "already
>> entered PC8+ at least once", because after the first time you
>> enter/leave PC8+ you'll already have reset the register, so you'll be
>> comparing bugged state against bugged state.
>
> Ok, I see your point. But imo igt testcases shouldn't (at least with the
> testcases run by default) have such detailed knowledge of what the kernel
> actually does with the registers. So a depency like "this test needs to be
> run after a fresh boot when we've never entered pc8+ yet" isn't good since
> it'll make the testcase fragile.
>
> Instead the test should check whether everything still works after we've
> been in pc8+ for a bit. One exception could be w/a bits if we have a
> common igt testcase to check for all of them. Then we could just rerun
> that testcase.
>
> But if e.g. the swizzling settings get lost over pc8+ it's better to add
> an explicit functional swizzle test here instead of checking registers.

So instead of doing this, why not just make sure the very first i-g-t
test gets us to PC8+, ant then run the full i-g-t suite after that?
This should cover all the stuff that breaks due to us not restoring
registers properly. Then we only have to worry about the things that
run while PC8 is enabled, we can do that in another test.

>
>>
>> >
>> >>   - You need at least one output connected.
>> >
>> > The test should skip (retun 77;) if that's the case.
>> >>
>> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >> ---
>> >>  tests/.gitignore  |   1 +
>> >>  tests/Makefile.am |   1 +
>> >>  tests/pc8.c       | 555 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> >>  3 files changed, 557 insertions(+)
>> >>  create mode 100644 tests/pc8.c
>> >>
>> >> diff --git a/tests/.gitignore b/tests/.gitignore
>> >> index 451ab2d..c70fdb7 100644
>> >> --- a/tests/.gitignore
>> >> +++ b/tests/.gitignore
>> >> @@ -90,6 +90,7 @@ getstats
>> >>  getversion
>> >>  kms_flip
>> >>  kms_render
>> >> +pc8
>> >>  prime_nv_api
>> >>  prime_nv_pcopy
>> >>  prime_nv_test
>> >> diff --git a/tests/Makefile.am b/tests/Makefile.am
>> >> index a59c25f..3507716 100644
>> >> --- a/tests/Makefile.am
>> >> +++ b/tests/Makefile.am
>> >> @@ -2,6 +2,7 @@ if BUILD_TESTS
>> >>  noinst_PROGRAMS = \
>> >>       gem_stress \
>> >>       ddi_compute_wrpll \
>> >> +     pc8 \
>> >>       $(TESTS_progs) \
>> >>       $(TESTS_progs_M) \
>> >>       $(HANG) \
>> >> diff --git a/tests/pc8.c b/tests/pc8.c
>> >> new file mode 100644
>> >> index 0000000..7fdfeb5
>> >> --- /dev/null
>> >> +++ b/tests/pc8.c
>> >> @@ -0,0 +1,555 @@
>> >> +/*
>> >> + * 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 <assert.h>
>> >> +#include <stdint.h>
>> >> +#include <stdbool.h>
>> >> +#include <string.h>
>> >> +
>> >> +#include <unistd.h>
>> >> +#include <sys/types.h>
>> >> +#include <sys/stat.h>
>> >> +#include <fcntl.h>
>> >> +
>> >> +#include "drm.h"
>> >> +#include "drmtest.h"
>> >> +#include "intel_batchbuffer.h"
>> >> +#include "intel_gpu_tools.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;
>> >> +
>> >> +/* Stuff used when creating FBs and mode setting. */
>> >> +struct mode_set_data {
>> >> +     drmModeResPtr res;
>> >> +     drmModeConnectorPtr connectors[MAX_CONNECTORS];
>> >> +
>> >> +     drm_intel_bufmgr *bufmgr;
>> >> +     uint32_t devid;
>> >> +};
>> >> +
>> >> +enum compare_step {
>> >> +     STEP_RESET,
>> >> +     STEP_RESTORED,
>> >> +};
>> >> +
>> >> +/* 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 {
>> >> +             /* 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;
>> >> +     } regs;
>> >> +};
>> >> +
>> >> +static uint64_t get_residency(int fd, uint32_t type)
>> >> +{
>> >> +     int rc;
>> >> +     uint64_t ret;
>> >> +
>> >> +     rc = pread(fd, &ret, sizeof(uint64_t), type);
>> >> +     assert(rc == sizeof(uint64_t));
>> >> +
>> >> +     return ret;
>> >> +}
>> >> +
>> >> +static bool pc8_plus_residency_is_zero(int fd)
>> >> +{
>> >> +     uint64_t res_pc8, res_pc9, res_pc10;
>> >> +
>> >> +     res_pc8 = get_residency(fd, MSR_PC8_RES);
>> >> +     res_pc9 = get_residency(fd, MSR_PC9_RES);
>> >> +     res_pc10 = get_residency(fd, MSR_PC10_RES);
>> >> +
>> >> +     if (res_pc8 != 0 || res_pc9 != 0 || res_pc10 != 0)
>> >> +             return false;
>> >> +
>> >> +     return true;
>> >> +}
>> >> +
>> >> +static bool pc8_plus_residency_changed(int fd, unsigned int timeout_sec)
>> >> +{
>> >> +     unsigned int i;
>> >> +     uint64_t res_pc8, res_pc9, res_pc10;
>> >> +     int to_sleep = 100 * 1000;
>> >> +
>> >> +     res_pc8 = get_residency(fd, MSR_PC8_RES);
>> >> +     res_pc9 = get_residency(fd, MSR_PC9_RES);
>> >> +     res_pc10 = get_residency(fd, MSR_PC10_RES);
>> >> +
>> >> +     for (i = 0; i < timeout_sec * 1000 * 1000; i += to_sleep) {
>> >> +             if (res_pc8 != get_residency(fd, MSR_PC8_RES) ||
>> >> +                 res_pc9 != get_residency(fd, MSR_PC9_RES) ||
>> >> +                 res_pc10 != get_residency(fd, MSR_PC10_RES)) {
>> >> +                     return true;
>> >> +             }
>> >> +             usleep(to_sleep);
>> >> +     }
>> >> +
>> >> +     return false;
>> >> +}
>> >> +
>> >> +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);
>> >> +             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);
>> >> +
>> >> +     assert(crtc_id);
>> >> +     assert(buffer_id);
>> >> +     assert(connector_id);
>> >> +     assert(mode);
>> >> +
>> >> +     rc = drmModeSetCrtc(drm_fd, crtc_id, buffer_id, 0, 0, &connector_id,
>> >> +                         1, mode);
>> >> +     assert(rc == 0);
>> >> +}
>> >> +
>> >> +static void get_connector_edid(struct compare_data *data, int index)
>> >> +{
>> >> +     unsigned int i;
>> >> +     drmModeConnectorPtr connector = data->connectors[index];
>> >> +     drmModeObjectPropertiesPtr props;
>> >> +
>> >> +     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) {
>> >> +                     assert(prop->flags & DRM_MODE_PROP_BLOB);
>> >> +                     assert(prop->count_blobs == 0);
>> >> +                     data->edids[index] = drmModeGetPropertyBlob(drm_fd,
>> >> +                                                     props->prop_values[i]);
>> >> +             }
>> >> +
>> >> +             drmModeFreeProperty(prop);
>> >> +     }
>> >> +
>> >> +     drmModeFreeObjectProperties(props);
>> >> +}
>> >> +
>> >> +static void init_mode_set_data(struct mode_set_data *data)
>> >> +{
>> >> +     int i;
>> >> +
>> >> +     data->res = drmModeGetResources(drm_fd);
>> >> +     assert(data->res);
>> >> +     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->bufmgr = drm_intel_bufmgr_gem_init(drm_fd, 4096);
>> >> +     data->devid = intel_get_drm_devid(drm_fd);
>> >> +
>> >> +     do_or_die(drmtest_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]);
>> >> +     drmModeFreeResources(data->res);
>> >> +}
>> >> +
>> >> +static void get_drm_info(struct compare_data *data)
>> >> +{
>> >> +     int i;
>> >> +
>> >> +     data->res = drmModeGetResources(drm_fd);
>> >> +     assert(data->res);
>> >> +
>> >> +     assert(data->res->count_connectors <= MAX_CONNECTORS);
>> >> +     assert(data->res->count_encoders <= MAX_ENCODERS);
>> >> +     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]);
>> >> +             get_connector_edid(data, 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]);
>> >> +
>> >> +     intel_register_access_init(intel_get_pci_device(), 0);
>> >> +     data->regs.arb_mode = INREG(0x4030);
>> >> +     data->regs.tilectl = INREG(0x101000);
>> >> +     data->regs.gen6_ucgctl2 = INREG(0x9404);
>> >> +     data->regs.gen7_l3cntlreg1 = INREG(0xB0C1);
>> >> +     data->regs.transa_chicken1 = INREG(0xF0060);
>> >> +     data->regs.deier = INREG(0x4400C);
>> >> +     data->regs.gtier = INREG(0x4401C);
>> >> +     data->regs.ddi_buf_trans_a_1 = INREG(0x64E00);
>> >> +     data->regs.ddi_buf_trans_b_5 = INREG(0x64E70);
>> >> +     data->regs.ddi_buf_trans_c_10 = INREG(0x64EE0);
>> >> +     data->regs.ddi_buf_trans_d_15 = INREG(0x64F58);
>> >> +     data->regs.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) assert(d1->data == d2->data)
>> >> +#define COMPARE_ARRAY(d1, d2, size, data) do { \
>> >> +     for (i = 0; i < size; i++) \
>> >> +             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);
>> >> +     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;
>> >> +     assert(e1 && e2);
>> >> +
>> >> +     COMPARE(e1, e2, id);
>> >> +     COMPARE(e1, e2, length);
>> >> +
>> >> +     assert(memcmp(e1->data, e2->data, e1->length) == 0);
>> >> +}
>> >
>> > Imo the important part is just checking whether the EDID is still the
>> > same, but I guess we can keep all the other checks, too.
>> >
>> >> +
>> >> +static void compare_registers(struct compare_data *d1, struct compare_data *d2,
>> >> +                           enum compare_step step)
>> >> +{
>> >> +     /* Things that shouldn't change at all. */
>> >> +     COMPARE(d1, d2, regs.gen6_ucgctl2);
>> >> +     COMPARE(d1, d2, regs.gen7_l3cntlreg1);
>> >> +     COMPARE(d1, d2, regs.transa_chicken1);
>> >> +     COMPARE(d1, d2, regs.arb_mode);
>> >> +     COMPARE(d1, d2, regs.tilectl);
>> >> +     assert(d1->regs.deier & (1 << 31));
>> >> +     assert(d2->regs.deier & (1 << 31));
>> >> +
>> >> +     switch (step) {
>> >> +     case STEP_RESET:
>> >> +             assert(d2->regs.gtier == 0);
>> >> +             assert(d2->regs.ddi_buf_trans_a_1 == 0);
>> >> +             assert(d2->regs.ddi_buf_trans_b_5 == 0);
>> >> +             assert(d2->regs.ddi_buf_trans_c_10 == 0);
>> >> +             assert(d2->regs.ddi_buf_trans_d_15 == 0);
>> >> +             assert(d2->regs.ddi_buf_trans_e_20 == 0);
>> >> +             break;
>> >> +     case STEP_RESTORED:
>> >> +             COMPARE(d1, d2, regs.arb_mode);
>> >> +             COMPARE(d1, d2, regs.tilectl);
>> >> +             COMPARE(d1, d2, regs.gtier);
>> >> +             COMPARE(d1, d2, regs.ddi_buf_trans_a_1);
>> >> +             COMPARE(d1, d2, regs.ddi_buf_trans_b_5);
>> >> +             COMPARE(d1, d2, regs.ddi_buf_trans_c_10);
>> >> +             COMPARE(d1, d2, regs.ddi_buf_trans_d_15);
>> >> +             COMPARE(d1, d2, regs.ddi_buf_trans_e_20);
>> >> +             break;
>> >> +     default:
>> >> +             assert(0);
>> >> +     }
>> >> +}
>> >
>> > This is really risky since we've just demonstrated that we can hard-hang
>> > hsw if someone concurrently accesses registers. I guess hiding this behind
>> > a debug switch would be good.
>>
>> Shouldn't we add some sort of debugfs interface to read/write
>> registers then? We don't want to hang the machine while running
>> intel_reg_dumper and all our other tools... These tests are important
>> since they catch bugs that actually *happened* while I was developing
>> this feature. Most of the other tests are for things people have
>> suggested, not bugs that really happened...
>
> On a quick look we have
>
> - arb_mode: This could be caught by a generic "have we lost w/a bits"
>   test. Similar issues apply for suspend/resume (we have a little helper
>   for that now) and after gpu resets.
> - tilectl: we have tons of swizzling tests. One gaping hole is currently
>   what happens with the display swizzling, but if we get that wrong people
>   will notice _really_ fast ;-) And I think we should be able to plug this
>   hole with crc checksums. But as long as we don't yet have crc checksum
>   infrastructure I'm willing to live with the gaps.
> - gtier: I guess that results in seqno waits not really working. Like I've
>   mentioned below I think your current test doesn't cover that properly
>   yet.
> - ddi_buf_trans control registers: I guess we could subsume that into the
>   w/a bit checker simply as another pile of registers which need to have a
>   special value.
>
> For the register checker Eric started to work on one, but unfornately it's
> not yet integrated anywhere into all relevant igt tests.
>
>>
>>
>> >
>> >> +
>> >> +static void assert_drm_infos_equal(struct compare_data *d1,
>> >> +                                struct compare_data *d2,
>> >> +                                enum compare_step step)
>> >> +{
>> >> +     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]);
>> >> +
>> >> +     compare_registers(d1, d2, step);
>> >> +}
>> >> +
>> >> +#define BUF_SIZE     (8 << 20)
>> >> +
>> >> +/* to avoid stupid depencies on libdrm, copy&paste */
>> >> +struct local_drm_i915_gem_wait {
>> >> +     /** Handle of BO we shall wait on */
>> >> +     __u32 bo_handle;
>> >> +     __u32 flags;
>> >> +     /** Number of nanoseconds to wait, Returns time remaining. */
>> >> +     __u64 timeout_ns;
>> >> +};
>> >> +#define WAIT_IOCTL DRM_IOWR(DRM_COMMAND_BASE + 0x2c, struct local_drm_i915_gem_wait)
>> >> +
>> >> +static int gem_bo_wait_timeout(int fd, uint32_t handle, uint64_t *timeout_ns)
>> >> +{
>> >> +     struct local_drm_i915_gem_wait wait;
>> >> +     int ret;
>> >> +
>> >> +     assert(timeout_ns);
>> >> +
>> >> +     wait.bo_handle = handle;
>> >> +     wait.timeout_ns = *timeout_ns;
>> >> +     wait.flags = 0;
>> >> +     ret = drmIoctl(fd, WAIT_IOCTL, &wait);
>> >> +     *timeout_ns = wait.timeout_ns;
>> >> +
>> >> +     return ret ? -errno : 0;
>> >> +}
>> >
>> > This should be extracted to lib/drmtest.c for all testcases.
>> >
>> >> +
>> >> +static void test_batch(struct mode_set_data *data)
>> >> +{
>> >> +     uint64_t timeout = 1000 * 1000 * 1000 * 2;
>> >> +     drm_intel_bo *dst;
>> >> +
>> >> +     dst = drm_intel_bo_alloc(data->bufmgr, "dst", BUF_SIZE, 4096);
>> >> +     if (gem_bo_wait_timeout(drm_fd, dst->handle, &timeout) == -EINVAL)
>> >> +             printf("Kernel doesn't support wait_timeout\n");
>> >> +}
>> >
>> > Since I don't submit a "submit a batch" anywhere this won't actually test
>> > much ...
>> >
>> >> +
>> >> +int main(int argc, char *argv[])
>> >> +{
>> >> +     struct mode_set_data ms_data;
>> >> +     struct compare_data pre_pc8, during_pc8, post_pc8;
>> >> +     int msr_fd;
>> >> +     bool skip_zero_pc8_check = false;
>> >> +
>> >> +     if (argc > 1) {
>> >> +             /* With this option we can test/debug/develop the tool without
>> >> +              * needing to reboot every time. */
>> >> +             if (strcmp(argv[1], "--skip-zero-pc8-check") == 0)
>> >> +                     skip_zero_pc8_check = true;
>> >> +     }
>> >> +
>> >> +     drm_fd = drm_open_any();
>> >> +     assert(drm_fd > 0);
>> >> +
>> >> +     init_mode_set_data(&ms_data);
>> >> +
>> >> +     if (!IS_HASWELL(ms_data.devid)) {
>> >> +             printf("PC8+ feature only supported on Haswell.\n");
>> >> +             return 77;
>> >> +     }
>> >> +
>> >> +     msr_fd = open("/dev/cpu/0/msr", O_RDONLY);
>> >> +     assert(msr_fd != -1);
>> >> +
>> >> +     /* Make sure the machine still didn't enter PC8+, otherwise we won't be
>> >> +      * able to really compare pre-PC8+ with post-PC8+. */
>> >> +     printf("Pre-PC8+\n");
>> >> +     assert(skip_zero_pc8_check || pc8_plus_residency_is_zero(msr_fd));
>> >> +     get_drm_info(&pre_pc8);
>> >> +     test_batch(&ms_data);
>> >> +     assert(skip_zero_pc8_check || pc8_plus_residency_is_zero(msr_fd));
>> >> +
>> >> +     printf("PC8+\n");
>> >> +     disable_all_screens(&ms_data);
>> >> +     sleep(1);
>> >> +
>> >> +     assert(pc8_plus_residency_changed(msr_fd, 10));
>> >> +     get_drm_info(&during_pc8);
>> >> +     test_batch(&ms_data);
>> >> +     assert(pc8_plus_residency_changed(msr_fd, 10));
>> >
>> > Imo we should test each functional piece seperately in it's own loop, i.e.
>> >
>> > for each foo in test_batch, test_edid_connector1, test_edid_connector2,
>> > ... ; do
>> >         enter pc8+
>> >         sleep 1
>> >         run $foo
>> >         exit pc8+
>> > done
>>
>> This is not possible since the whole goal is to compare the state
>> "before we have ever entered PC8+" with "after we have entered PC8+".
>> See the big explanation above. But I could try to split the "collect
>> data" with the  "compare data" steps, so the "compare data" could
>> become the individual tests you requested.
>
> I don't think you can make the test robust enough with the current
> approach, see above for a few ideas how this could be remedied.
>>
>>
>> >
>> > In the current implementation this doesn't matter, but if we change it
>> > so that some of the tests will exit pc8+ and if we furthermore add a
>> > slight delay for reentering pc8+ then the test won't test a lot any more.
>>
>> That's why we have functions that sleep for up to 10 seconds waiting
>> for the PC8+ residency to move. If we take too much time to go back to
>> PC8, then we should increase those 10second timeouts. In fact we could
>> make those timeouts become even more than 1 minute since we expect to
>> return before that (i.e., when the PC8+ residency moves).
>
> Hm, I guess I've missed that part of the test. But my point was that we
> should check that we've reentered pc8+ between each subtest, i.e. every
> time we've tried to read and edid, called test_batch.
>
> But as long as we don't wake upthe chip out of pc8+ for these tests I
> guess the current test is paranoid enough, but it needs to be changed
> before we can frob the code. And since we currently have some aux power
> domain get/put lingering that is essentially what can happen I think.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni

Re: [PATCH i-g-t] tests: add pc8

[Daniel Vetter]

On Tue, Aug 06, 2013 at 04:33:53PM -0300, Paulo Zanoni wrote:
> 2013/8/5 Daniel Vetter <daniel@ffwll.ch>:
> > On Mon, Aug 05, 2013 at 10:42:08AM -0300, Paulo Zanoni wrote:
> >> 2013/8/5 Daniel Vetter <daniel@ffwll.ch>:
> >> > On Mon, Jul 29, 2013 at 05:53:27PM -0300, Paulo Zanoni wrote:
> >> The problem is: let's imagine there's some important register that we
> >> initialize when starting the driver, but then we don't touch it
> >> anymore. And this register is one of the registers that get reset when
> >> we enter PC8, but our code doesn't fix it after leaving PC8. So if you
> >> boot your machine, do something to allow PC8+ and then disallow it,
> >> the register will go back to the "reset" state (which isn't guaranteed
> >> to be 0x0, especially on display registers). Then you run tests/pc8:
> >> it reads the register (which contains the "reset" value instead of the
> >> real value set by our driver when it got loaded, because we already
> >> entered PC8+ once), enters PC8+, leaves PC8+, reads the register
> >> again, notices the value is the same and then gives us a "PASS". On
> >> the other hand, if you didn't reach PC8+ before running tests/pc8,
> >> then it would have noticed the value of the register changes.
> >>
> >> In other words: if some register gets initialized by our driver to a
> >> non-default value, but this value gets lost after we enter/leave PC8+,
> >> we will *only* be able to notice the difference when comparing a state
> >> where we *never* entered PC8+ against a state where we "already
> >> entered PC8+ at least once", because after the first time you
> >> enter/leave PC8+ you'll already have reset the register, so you'll be
> >> comparing bugged state against bugged state.
> >
> > Ok, I see your point. But imo igt testcases shouldn't (at least with the
> > testcases run by default) have such detailed knowledge of what the kernel
> > actually does with the registers. So a depency like "this test needs to be
> > run after a fresh boot when we've never entered pc8+ yet" isn't good since
> > it'll make the testcase fragile.
> >
> > Instead the test should check whether everything still works after we've
> > been in pc8+ for a bit. One exception could be w/a bits if we have a
> > common igt testcase to check for all of them. Then we could just rerun
> > that testcase.
> >
> > But if e.g. the swizzling settings get lost over pc8+ it's better to add
> > an explicit functional swizzle test here instead of checking registers.
> 
> So instead of doing this, why not just make sure the very first i-g-t
> test gets us to PC8+, ant then run the full i-g-t suite after that?
> This should cover all the stuff that breaks due to us not restoring
> registers properly. Then we only have to worry about the things that
> run while PC8 is enabled, we can do that in another test.

It would be nice if we could do this (e.g. also for re-running igt after
gpu hangs or after a suspend/resume). But I think atm QA runs testcases
essentially in a random order (or not in one we can control) so doing that
is pretty hard.

Since the major usecase here would be to sanity-check some registers (for
w/a and sane settings, e.g. the ddi transalation buffer settings) and we
don't yet really have a good integration of the existing wa setting
checker tool I think we can just punt here. I'll add a note to my igt
wishlist about this. And of course pls keep the special test mode in your
testcase so that developers could manually check that we correctly restore
registers.

Imo a big issue with making the w/a checker work is that it'll be a giant
pain to keep it in sync with the kernel sources. But maybe we can extend
the existing w/a sourcecode comment grabber in a clever way. Damien, any
ideas?

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

Re: [PATCH i-g-t] tests: add pc8

[Daniel Vetter]

Mabye I should actually cc Damien when I poke him ...
-Daniel

On Tue, Aug 06, 2013 at 10:11:02PM +0200, Daniel Vetter wrote:
> On Tue, Aug 06, 2013 at 04:33:53PM -0300, Paulo Zanoni wrote:
> > 2013/8/5 Daniel Vetter <daniel@ffwll.ch>:
> > > On Mon, Aug 05, 2013 at 10:42:08AM -0300, Paulo Zanoni wrote:
> > >> 2013/8/5 Daniel Vetter <daniel@ffwll.ch>:
> > >> > On Mon, Jul 29, 2013 at 05:53:27PM -0300, Paulo Zanoni wrote:
> > >> The problem is: let's imagine there's some important register that we
> > >> initialize when starting the driver, but then we don't touch it
> > >> anymore. And this register is one of the registers that get reset when
> > >> we enter PC8, but our code doesn't fix it after leaving PC8. So if you
> > >> boot your machine, do something to allow PC8+ and then disallow it,
> > >> the register will go back to the "reset" state (which isn't guaranteed
> > >> to be 0x0, especially on display registers). Then you run tests/pc8:
> > >> it reads the register (which contains the "reset" value instead of the
> > >> real value set by our driver when it got loaded, because we already
> > >> entered PC8+ once), enters PC8+, leaves PC8+, reads the register
> > >> again, notices the value is the same and then gives us a "PASS". On
> > >> the other hand, if you didn't reach PC8+ before running tests/pc8,
> > >> then it would have noticed the value of the register changes.
> > >>
> > >> In other words: if some register gets initialized by our driver to a
> > >> non-default value, but this value gets lost after we enter/leave PC8+,
> > >> we will *only* be able to notice the difference when comparing a state
> > >> where we *never* entered PC8+ against a state where we "already
> > >> entered PC8+ at least once", because after the first time you
> > >> enter/leave PC8+ you'll already have reset the register, so you'll be
> > >> comparing bugged state against bugged state.
> > >
> > > Ok, I see your point. But imo igt testcases shouldn't (at least with the
> > > testcases run by default) have such detailed knowledge of what the kernel
> > > actually does with the registers. So a depency like "this test needs to be
> > > run after a fresh boot when we've never entered pc8+ yet" isn't good since
> > > it'll make the testcase fragile.
> > >
> > > Instead the test should check whether everything still works after we've
> > > been in pc8+ for a bit. One exception could be w/a bits if we have a
> > > common igt testcase to check for all of them. Then we could just rerun
> > > that testcase.
> > >
> > > But if e.g. the swizzling settings get lost over pc8+ it's better to add
> > > an explicit functional swizzle test here instead of checking registers.
> > 
> > So instead of doing this, why not just make sure the very first i-g-t
> > test gets us to PC8+, ant then run the full i-g-t suite after that?
> > This should cover all the stuff that breaks due to us not restoring
> > registers properly. Then we only have to worry about the things that
> > run while PC8 is enabled, we can do that in another test.
> 
> It would be nice if we could do this (e.g. also for re-running igt after
> gpu hangs or after a suspend/resume). But I think atm QA runs testcases
> essentially in a random order (or not in one we can control) so doing that
> is pretty hard.
> 
> Since the major usecase here would be to sanity-check some registers (for
> w/a and sane settings, e.g. the ddi transalation buffer settings) and we
> don't yet really have a good integration of the existing wa setting
> checker tool I think we can just punt here. I'll add a note to my igt
> wishlist about this. And of course pls keep the special test mode in your
> testcase so that developers could manually check that we correctly restore
> registers.
> 
> Imo a big issue with making the w/a checker work is that it'll be a giant
> pain to keep it in sync with the kernel sources. But maybe we can extend
> the existing w/a sourcecode comment grabber in a clever way. Damien, any
> ideas?
> 
> Cheers, Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

Re: [PATCH i-g-t] tests: add pc8

[Paulo Zanoni]

2013/8/6 Daniel Vetter <daniel@ffwll.ch>:
> On Tue, Aug 06, 2013 at 04:33:53PM -0300, Paulo Zanoni wrote:
>> 2013/8/5 Daniel Vetter <daniel@ffwll.ch>:
>> > On Mon, Aug 05, 2013 at 10:42:08AM -0300, Paulo Zanoni wrote:
>> >> 2013/8/5 Daniel Vetter <daniel@ffwll.ch>:
>> >> > On Mon, Jul 29, 2013 at 05:53:27PM -0300, Paulo Zanoni wrote:
>> >> The problem is: let's imagine there's some important register that we
>> >> initialize when starting the driver, but then we don't touch it
>> >> anymore. And this register is one of the registers that get reset when
>> >> we enter PC8, but our code doesn't fix it after leaving PC8. So if you
>> >> boot your machine, do something to allow PC8+ and then disallow it,
>> >> the register will go back to the "reset" state (which isn't guaranteed
>> >> to be 0x0, especially on display registers). Then you run tests/pc8:
>> >> it reads the register (which contains the "reset" value instead of the
>> >> real value set by our driver when it got loaded, because we already
>> >> entered PC8+ once), enters PC8+, leaves PC8+, reads the register
>> >> again, notices the value is the same and then gives us a "PASS". On
>> >> the other hand, if you didn't reach PC8+ before running tests/pc8,
>> >> then it would have noticed the value of the register changes.
>> >>
>> >> In other words: if some register gets initialized by our driver to a
>> >> non-default value, but this value gets lost after we enter/leave PC8+,
>> >> we will *only* be able to notice the difference when comparing a state
>> >> where we *never* entered PC8+ against a state where we "already
>> >> entered PC8+ at least once", because after the first time you
>> >> enter/leave PC8+ you'll already have reset the register, so you'll be
>> >> comparing bugged state against bugged state.
>> >
>> > Ok, I see your point. But imo igt testcases shouldn't (at least with the
>> > testcases run by default) have such detailed knowledge of what the kernel
>> > actually does with the registers. So a depency like "this test needs to be
>> > run after a fresh boot when we've never entered pc8+ yet" isn't good since
>> > it'll make the testcase fragile.
>> >
>> > Instead the test should check whether everything still works after we've
>> > been in pc8+ for a bit. One exception could be w/a bits if we have a
>> > common igt testcase to check for all of them. Then we could just rerun
>> > that testcase.
>> >
>> > But if e.g. the swizzling settings get lost over pc8+ it's better to add
>> > an explicit functional swizzle test here instead of checking registers.
>>
>> So instead of doing this, why not just make sure the very first i-g-t
>> test gets us to PC8+, ant then run the full i-g-t suite after that?
>> This should cover all the stuff that breaks due to us not restoring
>> registers properly. Then we only have to worry about the things that
>> run while PC8 is enabled, we can do that in another test.
>
> It would be nice if we could do this (e.g. also for re-running igt after
> gpu hangs or after a suspend/resume). But I think atm QA runs testcases
> essentially in a random order (or not in one we can control) so doing that
> is pretty hard.

Can't we just tell them we need this one first?

>
> Since the major usecase here would be to sanity-check some registers (for
> w/a and sane settings, e.g. the ddi transalation buffer settings) and we
> don't yet really have a good integration of the existing wa setting
> checker tool I think we can just punt here. I'll add a note to my igt
> wishlist about this. And of course pls keep the special test mode in your
> testcase so that developers could manually check that we correctly restore
> registers.

Well, the ddi buffer translation problem leads to big screen
corruption on testdisplay.

>
> Imo a big issue with making the w/a checker work is that it'll be a giant
> pain to keep it in sync with the kernel sources. But maybe we can extend
> the existing w/a sourcecode comment grabber in a clever way. Damien, any
> ideas?
>
> Cheers, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni

Re: [PATCH i-g-t] tests: add pc8

[Daniel Vetter]

On Tue, Aug 06, 2013 at 05:43:23PM -0300, Paulo Zanoni wrote:
> 2013/8/6 Daniel Vetter <daniel@ffwll.ch>:
> > On Tue, Aug 06, 2013 at 04:33:53PM -0300, Paulo Zanoni wrote:
> >> 2013/8/5 Daniel Vetter <daniel@ffwll.ch>:
> >> > On Mon, Aug 05, 2013 at 10:42:08AM -0300, Paulo Zanoni wrote:
> >> >> 2013/8/5 Daniel Vetter <daniel@ffwll.ch>:
> >> >> > On Mon, Jul 29, 2013 at 05:53:27PM -0300, Paulo Zanoni wrote:
> >> >> The problem is: let's imagine there's some important register that we
> >> >> initialize when starting the driver, but then we don't touch it
> >> >> anymore. And this register is one of the registers that get reset when
> >> >> we enter PC8, but our code doesn't fix it after leaving PC8. So if you
> >> >> boot your machine, do something to allow PC8+ and then disallow it,
> >> >> the register will go back to the "reset" state (which isn't guaranteed
> >> >> to be 0x0, especially on display registers). Then you run tests/pc8:
> >> >> it reads the register (which contains the "reset" value instead of the
> >> >> real value set by our driver when it got loaded, because we already
> >> >> entered PC8+ once), enters PC8+, leaves PC8+, reads the register
> >> >> again, notices the value is the same and then gives us a "PASS". On
> >> >> the other hand, if you didn't reach PC8+ before running tests/pc8,
> >> >> then it would have noticed the value of the register changes.
> >> >>
> >> >> In other words: if some register gets initialized by our driver to a
> >> >> non-default value, but this value gets lost after we enter/leave PC8+,
> >> >> we will *only* be able to notice the difference when comparing a state
> >> >> where we *never* entered PC8+ against a state where we "already
> >> >> entered PC8+ at least once", because after the first time you
> >> >> enter/leave PC8+ you'll already have reset the register, so you'll be
> >> >> comparing bugged state against bugged state.
> >> >
> >> > Ok, I see your point. But imo igt testcases shouldn't (at least with the
> >> > testcases run by default) have such detailed knowledge of what the kernel
> >> > actually does with the registers. So a depency like "this test needs to be
> >> > run after a fresh boot when we've never entered pc8+ yet" isn't good since
> >> > it'll make the testcase fragile.
> >> >
> >> > Instead the test should check whether everything still works after we've
> >> > been in pc8+ for a bit. One exception could be w/a bits if we have a
> >> > common igt testcase to check for all of them. Then we could just rerun
> >> > that testcase.
> >> >
> >> > But if e.g. the swizzling settings get lost over pc8+ it's better to add
> >> > an explicit functional swizzle test here instead of checking registers.
> >>
> >> So instead of doing this, why not just make sure the very first i-g-t
> >> test gets us to PC8+, ant then run the full i-g-t suite after that?
> >> This should cover all the stuff that breaks due to us not restoring
> >> registers properly. Then we only have to worry about the things that
> >> run while PC8 is enabled, we can do that in another test.
> >
> > It would be nice if we could do this (e.g. also for re-running igt after
> > gpu hangs or after a suspend/resume). But I think atm QA runs testcases
> > essentially in a random order (or not in one we can control) so doing that
> > is pretty hard.
> 
> Can't we just tell them we need this one first?

Not really since such testcase would be really fragile. If you really
need/want a reliable starting point you could simply suspend/resume the
machine. lib/drmtest.c has a little helper for that, see
tests/gem_suspend.c

> > Since the major usecase here would be to sanity-check some registers (for
> > w/a and sane settings, e.g. the ddi transalation buffer settings) and we
> > don't yet really have a good integration of the existing wa setting
> > checker tool I think we can just punt here. I'll add a note to my igt
> > wishlist about this. And of course pls keep the special test mode in your
> > testcase so that developers could manually check that we correctly restore
> > registers.
> 
> Well, the ddi buffer translation problem leads to big screen
> corruption on testdisplay.

There are always limits to what we can reliably test, but such issues
could be caught by QA's QR code reader support in testdisplay.
Unfortunately that support isn't readily available as a little drmtest.c
helper function, mostly since I don't have the setup myself and so can't
test it.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch