Allow Package C8+

From: Paulo Zanoni <[email protected]>

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.

Quoted 9 lines

From: Paulo Zanoni <[email protected]>

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

drivers/gpu/drm/i915/i915_irq.c static irqreturn_t ironlake_irq_handler(int irq, void *arg) · line 1344

Quoted 5 lines

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

-             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

From: Paulo Zanoni <[email protected]>

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.

drivers/gpu/drm/i915/intel_pm.c +4 -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);

Quoted 6 lines

From: Paulo Zanoni <[email protected]>

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.

Quoted 6 lines

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.

Quoted 5 lines

drivers/gpu/drm/i915/intel_pm.c +4 -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);

drivers/gpu/drm/i915/intel_pm.c void intel_init_power_well(struct drm_device *dev) · line 5175

Quoted 10 lines

drivers/gpu/drm/i915/intel_pm.c +10 -0

+     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

2013/7/29 Chris Wilson <[email protected]>:

Quoted 11 lines · Paulo Zanoni

Quoted 10 lines

From: Paulo Zanoni <[email protected]>

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

Quoted 4 lines

Quoted 2 lines

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").

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 12 lines

2013/7/29 Chris Wilson <[email protected]>:

Quoted 11 lines · Paulo Zanoni

Quoted 10 lines

From: Paulo Zanoni <[email protected]>

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

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

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

Quoted 4 lines

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 7 lines

Quoted 3 lines

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

From: Paulo Zanoni <[email protected]>

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.

From: Paulo Zanoni <[email protected]>

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.

Quoted 5 lines

From: Paulo Zanoni <[email protected]>

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

2013/7/30 Chris Wilson <[email protected]>:

Quoted 7 lines · Paulo Zanoni

Quoted 5 lines

From: Paulo Zanoni <[email protected]>

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.

Quoted 2 lines

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.

From: Paulo Zanoni <[email protected]>

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.

Quoted 5 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

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.

Quoted 10 lines · Paulo Zanoni

Quoted 5 lines

From: Paulo Zanoni <[email protected]>

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.

Quoted 4 lines

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

From: Paulo Zanoni <[email protected]>

Things should be working, so enable it by default.

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

---
 drivers/gpu/drm/i915/i915_drv.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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

index 6df516b..8f6e685 100644

From: Paulo Zanoni <[email protected]>

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

---
 tests/.gitignore  |   1 +
 tests/Makefile.am |   1 +
 tests/pc8.c       | 555 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 557 insertions(+)
 create mode 100644 tests/pc8.c

tests/.gitignore +1 -0

index 451ab2d..c70fdb7 100644
@@ -90,6 +90,7 @@ getstats
 getversion
 kms_flip
 kms_render
+pc8
 prime_nv_api
 prime_nv_pcopy
 prime_nv_test

tests/Makefile.am +1 -0

index a59c25f..3507716 100644
@@ -2,6 +2,7 @@ if BUILD_TESTS
 noinst_PROGRAMS = \
 	gem_stress \
 	ddi_compute_wrpll \
+	pc8 \
 	$(TESTS_progs) \
 	$(TESTS_progs_M) \
 	$(HANG) \

tests/pc8.c new file +380 -0

new file mode 100644
index 0000000..7fdfeb5
@@ -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 <[email protected]>
+ *
+ */
+
+#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)

tests/pc8.c new file +26 -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);

tests/pc8.c new file +41 -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;
+

tests/pc8.c new file +34 -0

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

tests/pc8.c new file +24 -0

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

Signature · 6 lines

1.8.1.2

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 15 lines

From: Paulo Zanoni <[email protected]>

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?

Quoted 2 lines

- You need at least one output connected.

The test should skip (retun 77;) if that's the case.

Quoted 419 lines

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

---
 tests/.gitignore  |   1 +
 tests/Makefile.am |   1 +
 tests/pc8.c       | 555 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 557 insertions(+)
 create mode 100644 tests/pc8.c

tests/.gitignore +1 -0

index 451ab2d..c70fdb7 100644
@@ -90,6 +90,7 @@ getstats
 getversion
 kms_flip
 kms_render
+pc8
 prime_nv_api
 prime_nv_pcopy
 prime_nv_test

tests/Makefile.am +1 -0

index a59c25f..3507716 100644
@@ -2,6 +2,7 @@ if BUILD_TESTS
 noinst_PROGRAMS = \
      gem_stress \
      ddi_compute_wrpll \
+     pc8 \
      $(TESTS_progs) \
      $(TESTS_progs_M) \
      $(HANG) \

tests/pc8.c new file +380 -0

new file mode 100644
index 0000000..7fdfeb5
@@ -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 <[email protected]>
+ *
+ */
+
+#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)

tests/pc8.c line 390

Quoted 10 lines

tests/pc8.c new file +10 -0

+{
+     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.

Quoted 32 lines

tests/pc8.c new file +32 -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;

tests/pc8.c line 426

Quoted 4 lines

tests/pc8.c new file +4 -0

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

Quoted 42 lines

tests/pc8.c new file +41 -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;
+

tests/pc8.c line 477

Quoted 10 lines

tests/pc8.c new file +10 -0

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

tests/pc8.c line 487

Quoted 10 lines

tests/pc8.c new file +10 -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");
+}

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

Quoted 34 lines

tests/pc8.c new file +34 -0

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

tests/pc8.c line 531

Quoted 10 lines

tests/pc8.c new file +10 -0

+     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

Quoted 27 lines

tests/pc8.c new file +24 -1

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

Quoted 4 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2013/8/5 Daniel Vetter <[email protected]>:

Quoted 17 lines · Paulo Zanoni

Quoted 15 lines

From: Paulo Zanoni <[email protected]>

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.

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 470 lines

Quoted 3 lines

- You need at least one output connected.

The test should skip (retun 77;) if that's the case.

Quoted 419 lines

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

---
 tests/.gitignore  |   1 +
 tests/Makefile.am |   1 +
 tests/pc8.c       | 555 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 557 insertions(+)
 create mode 100644 tests/pc8.c

tests/.gitignore +1 -0

index 451ab2d..c70fdb7 100644
@@ -90,6 +90,7 @@ getstats
 getversion
 kms_flip
 kms_render
+pc8
 prime_nv_api
 prime_nv_pcopy
 prime_nv_test

tests/Makefile.am +1 -0

index a59c25f..3507716 100644
@@ -2,6 +2,7 @@ if BUILD_TESTS
 noinst_PROGRAMS = \
      gem_stress \
      ddi_compute_wrpll \
+     pc8 \
      $(TESTS_progs) \
      $(TESTS_progs_M) \
      $(HANG) \

tests/pc8.c new file +380 -0

new file mode 100644
index 0000000..7fdfeb5
@@ -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 <[email protected]>
+ *
+ */
+
+#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)

tests/pc8.c line 390

Quoted 10 lines

tests/pc8.c new file +10 -0

+{
+     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.

Quoted 32 lines

tests/pc8.c new file +32 -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;

Quoted 8 lines

tests/pc8.c line 426

Quoted 4 lines

tests/pc8.c new file +4 -0

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

Quoted 118 lines

Quoted 44 lines

tests/pc8.c new file +41 -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;
+

tests/pc8.c line 477

Quoted 10 lines

tests/pc8.c new file +10 -0

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

tests/pc8.c line 487

Quoted 10 lines

tests/pc8.c new file +10 -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");
+}

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

Quoted 34 lines

tests/pc8.c new file +34 -0

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

tests/pc8.c line 531

Quoted 10 lines

tests/pc8.c new file +10 -0

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

Quoted 7 lines

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.

Quoted 6 lines

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

Quoted 18 lines

2013/8/5 Daniel Vetter <[email protected]>:

Quoted 17 lines · Paulo Zanoni

Quoted 15 lines

From: Paulo Zanoni <[email protected]>

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.

Quoted 6 lines

Quoted 2 lines

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

Quoted 6 lines

Quoted 3 lines

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.

Quoted 14 lines

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.

Quoted 7 lines

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.

Quoted 480 lines

Quoted 471 lines

Quoted 4 lines

- You need at least one output connected.

The test should skip (retun 77;) if that's the case.

Quoted 419 lines

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

---
 tests/.gitignore  |   1 +
 tests/Makefile.am |   1 +
 tests/pc8.c       | 555 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 557 insertions(+)
 create mode 100644 tests/pc8.c

tests/.gitignore +1 -0

index 451ab2d..c70fdb7 100644
@@ -90,6 +90,7 @@ getstats
 getversion
 kms_flip
 kms_render
+pc8
 prime_nv_api
 prime_nv_pcopy
 prime_nv_test

tests/Makefile.am +1 -0

index a59c25f..3507716 100644
@@ -2,6 +2,7 @@ if BUILD_TESTS
 noinst_PROGRAMS = \
      gem_stress \
      ddi_compute_wrpll \
+     pc8 \
      $(TESTS_progs) \
      $(TESTS_progs_M) \
      $(HANG) \

tests/pc8.c new file +380 -0

new file mode 100644
index 0000000..7fdfeb5
@@ -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 <[email protected]>
+ *
+ */
+
+#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)

tests/pc8.c line 390

Quoted 10 lines

tests/pc8.c new file +10 -0

+{
+     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.

Quoted 32 lines

tests/pc8.c new file +32 -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;

Quoted 8 lines

tests/pc8.c line 426

Quoted 4 lines

tests/pc8.c new file +4 -0

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

Quoted 6 lines

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.

Quoted 126 lines

Quoted 118 lines

Quoted 44 lines

tests/pc8.c new file +41 -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;
+

tests/pc8.c line 477

Quoted 10 lines

tests/pc8.c new file +10 -0

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

tests/pc8.c line 487

Quoted 10 lines

tests/pc8.c new file +10 -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");
+}

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

Quoted 34 lines

tests/pc8.c new file +34 -0

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

tests/pc8.c line 531

Quoted 10 lines

tests/pc8.c new file +10 -0

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

Quoted 7 lines

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

Quoted 5 lines

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.

Quoted 7 lines

Quoted 6 lines

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.

Quoted 5 lines

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