[PATCH 0/3] Fixes for runtime PM on planes APIs

[PATCH 0/3] Fixes for runtime PM on planes APIs

[Paulo Zanoni]

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

Hi

This series fixes some bugs that happen when we're runtime suspended and try to
use the planes APIs. I also wrote IGT test cases for the bugs, so we will be
able to detect future regressions.

The controversial part of these patches is that we had previously defined that
we wanted to get/put runtime PM in the highest level of the stack, wrapping as
much code as possible, but Daniel asked me to only get/put runtime PM around the
functions that pin the objects (still on the highest level, but only around the
pin functions). This series implements Daniel's suggestions.

Thanks,
Paulo


Paulo Zanoni (3):
  drm/i915: fix cursor handling when runtime suspended
  drm/i915: get runtime PM when pinning sprite objects
  drm/i915: get runtime PM when pinning primary plane objects

 drivers/gpu/drm/i915/intel_display.c | 9 +++++++++
 drivers/gpu/drm/i915/intel_sprite.c  | 3 +++
 2 files changed, 12 insertions(+)

-- 
2.0.1

[PATCH 1/3] drm/i915: fix cursor handling when runtime suspended

[Paulo Zanoni]

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

If we're runtime suspended and try to use the cursor interfaces, we
will get a lot of WARNs saying we did the wrong thing.

For intel_crtc_update_cursor(), all we need to do is return if the
CRTC is not active, since writing the registers won't really have any
effect if the screen is not visible, and we will write the registers
later when enabling the screen.

For intel_crtc_cursor_set_obj(), we just need to wake up the hardware
then pinning the display plane.

v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)

Testcase: igt/pm_rpm/cursor
Testcase: igt/pm_rpm/cursor-dpms
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81645
Cc: stable@vger.kernel.org
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1edfd1a..f1a9b5c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8144,6 +8144,9 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
 	if (base == 0 && intel_crtc->cursor_base == 0)
 		return;
 
+	if (!intel_crtc->active)
+		return;
+
 	I915_WRITE(CURPOS(pipe), pos);
 
 	if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev) || IS_BROADWELL(dev))
@@ -8217,7 +8220,9 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 		if (need_vtd_wa(dev))
 			alignment = 64*1024;
 
+		intel_runtime_pm_get(dev_priv);
 		ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
+		intel_runtime_pm_put(dev_priv);
 		if (ret) {
 			DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
 			goto fail_locked;
-- 
2.0.1

[PATCH 1/2] tests/pm_rpm: add cursor subtests

[Paulo Zanoni]

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

These tests currently trigger WARNs on our Kernel. Let's make sure we
fix the bugs and they never come back.

v2: Reorganize the code a little bit, and improve the tests.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 tests/pm_rpm.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 113 insertions(+), 10 deletions(-)

diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c
index 869e6f3..f720f86 100644
--- a/tests/pm_rpm.c
+++ b/tests/pm_rpm.c
@@ -277,13 +277,15 @@ static uint32_t get_fb(struct mode_set_data *data, int width, int height)
 	igt_assert(false);
 }
 
-static bool enable_one_screen_with_type(struct mode_set_data *data,
-					enum screen_type type)
+static bool find_connector_for_modeset(struct mode_set_data *data,
+				       enum screen_type type,
+				       uint32_t *connector_id,
+				       drmModeModeInfoPtr *mode)
 {
-	uint32_t crtc_id = 0, buffer_id = 0, connector_id = 0;
-	drmModeModeInfoPtr mode = NULL;
-	int i, rc;
+	int i;
 
+	*connector_id = 0;
+	*mode = NULL;
 	for (i = 0; i < data->res->count_connectors; i++) {
 		drmModeConnectorPtr c = data->connectors[i];
 
@@ -296,13 +298,23 @@ static bool enable_one_screen_with_type(struct mode_set_data *data,
 			continue;
 
 		if (c->connection == DRM_MODE_CONNECTED && c->count_modes) {
-			connector_id = c->connector_id;
-			mode = &c->modes[0];
+			*connector_id = c->connector_id;
+			*mode = &c->modes[0];
 			break;
 		}
 	}
 
-	if (connector_id == 0)
+	return (*connector_id != 0);
+}
+
+static bool enable_one_screen_with_type(struct mode_set_data *data,
+					enum screen_type type)
+{
+	uint32_t crtc_id = 0, buffer_id = 0, connector_id;
+	drmModeModeInfoPtr mode;
+	int rc;
+
+	if (!find_connector_for_modeset(data, type, &connector_id, &mode))
 		return false;
 
 	crtc_id = data->res->crtcs[0];
@@ -310,8 +322,6 @@ static bool enable_one_screen_with_type(struct mode_set_data *data,
 
 	igt_assert(crtc_id);
 	igt_assert(buffer_id);
-	igt_assert(connector_id);
-	igt_assert(mode);
 
 	rc = drmModeSetCrtc(drm_fd, crtc_id, buffer_id, 0, 0, &connector_id,
 			    1, mode);
@@ -1407,6 +1417,93 @@ static void dpms_mode_unset_subtest(enum screen_type type)
 	igt_assert(wait_for_suspended());
 }
 
+static void fill_igt_fb(struct igt_fb *fb, uint32_t color)
+{
+	int i;
+	uint32_t *ptr;
+
+	ptr = gem_mmap__cpu(drm_fd, fb->gem_handle, fb->size, 0);
+	for (i = 0; i < fb->size/sizeof(uint32_t); i++)
+		ptr[i] = color;
+	igt_assert(munmap(ptr, fb->size) == 0);
+}
+
+/* At some point, this test triggered WARNs in the Kernel. */
+static void cursor_subtest(bool dpms)
+{
+	uint32_t crtc_id = 0, connector_id;
+	drmModeModeInfoPtr mode;
+	int rc;
+	struct igt_fb scanout_fb, cursor_fb1, cursor_fb2;
+
+	disable_all_screens(&ms_data);
+	igt_assert(wait_for_suspended());
+
+	igt_require(find_connector_for_modeset(&ms_data, SCREEN_TYPE_ANY,
+					       &connector_id, &mode));
+
+	crtc_id = ms_data.res->crtcs[0];
+	igt_assert(crtc_id);
+
+	igt_create_fb(drm_fd, mode->hdisplay, mode->vdisplay,
+		      DRM_FORMAT_XRGB8888, false, &scanout_fb);
+	igt_create_fb(drm_fd, 64, 64, DRM_FORMAT_ARGB8888, false, &cursor_fb1);
+	igt_create_fb(drm_fd, 64, 64, DRM_FORMAT_ARGB8888, false, &cursor_fb2);
+
+	fill_igt_fb(&scanout_fb, 0xFF);
+	fill_igt_fb(&cursor_fb1, 0xFF00FFFF);
+	fill_igt_fb(&cursor_fb2, 0xFF00FF00);
+
+	rc = drmModeSetCrtc(drm_fd, crtc_id, scanout_fb.fb_id, 0, 0,
+			    &connector_id, 1, mode);
+	igt_assert(rc == 0);
+	igt_assert(wait_for_active());
+
+	rc = drmModeSetCursor(drm_fd, crtc_id, cursor_fb1.gem_handle,
+			      cursor_fb1.width, cursor_fb1.height);
+	igt_assert(rc == 0);
+	rc = drmModeMoveCursor(drm_fd, crtc_id, 0, 0);
+	igt_assert(rc == 0);
+	igt_assert(wait_for_active());
+
+	if (dpms)
+		disable_all_screens_dpms(&ms_data);
+	else
+		disable_all_screens(&ms_data);
+	igt_assert(wait_for_suspended());
+
+	/* First, just move the cursor. */
+	rc = drmModeMoveCursor(drm_fd, crtc_id, 1, 1);
+	igt_assert(rc == 0);
+	igt_assert(wait_for_suspended());
+
+	/* Then unset it, and set a new one. */
+	rc = drmModeSetCursor(drm_fd, crtc_id, 0, 0, 0);
+	igt_assert(rc == 0);
+	igt_assert(wait_for_suspended());
+
+	rc = drmModeSetCursor(drm_fd, crtc_id, cursor_fb2.gem_handle,
+			      cursor_fb1.width, cursor_fb2.height);
+	igt_assert(rc == 0);
+	igt_assert(wait_for_suspended());
+
+	/* Move the new cursor. */
+	rc = drmModeMoveCursor(drm_fd, crtc_id, 2, 2);
+	igt_assert(rc == 0);
+	igt_assert(wait_for_suspended());
+
+	/* Now set a new one without unsetting the previous one. */
+	rc = drmModeSetCursor(drm_fd, crtc_id, cursor_fb1.gem_handle,
+			      cursor_fb1.width, cursor_fb1.height);
+	igt_assert(rc == 0);
+	igt_assert(wait_for_suspended());
+
+	/* Make sure nothing remains for the other tests. */
+	rc = drmModeSetCursor(drm_fd, crtc_id, 0, 0, 0);
+	igt_assert(rc == 0);
+	igt_assert(wait_for_suspended());
+}
+
 int rounds = 50;
 bool stay = false;
 
@@ -1480,6 +1577,12 @@ int main(int argc, char *argv[])
 	igt_subtest("gem-idle")
 		gem_idle_subtest();
 
+	/* Cursors */
+	igt_subtest("cursor")
+		cursor_subtest(false);
+	igt_subtest("cursor-dpms")
+		cursor_subtest(true);
+
 	/* Misc */
 	igt_subtest("reg-read-ioctl")
 		reg_read_ioctl_subtest();
-- 
2.0.1

[PATCH 2/3] drm/i915: get runtime PM when pinning sprite objects

[Paulo Zanoni]

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

Otherwise we may get WARNs saying we're writing registers while
runtime suspended.

Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Cc: stable@vger.kernel.org
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index d34a569..8c5a8f7 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -821,6 +821,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		   uint32_t src_w, uint32_t src_h)
 {
 	struct drm_device *dev = plane->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	enum pipe pipe = intel_crtc->pipe;
@@ -1009,7 +1010,9 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	 * primary plane requires 256KiB alignment with 64 PTE padding,
 	 * the sprite planes only require 128KiB alignment and 32 PTE padding.
 	 */
+	intel_runtime_pm_get(dev_priv);
 	ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
+	intel_runtime_pm_put(dev_priv);
 
 	i915_gem_track_fb(old_obj, obj,
 			  INTEL_FRONTBUFFER_SPRITE(pipe));
-- 
2.0.1

[PATCH 2/2] tests/pm_rpm: add planes subtests

[Paulo Zanoni]

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

Just like the cursor subtests, these also trigger WARNs on the current
Kernel.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 tests/pm_rpm.c | 212 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 211 insertions(+), 1 deletion(-)

diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c
index f720f86..048d9ad 100644
--- a/tests/pm_rpm.c
+++ b/tests/pm_rpm.c
@@ -51,6 +51,9 @@
 #include "igt_kms.h"
 #include "igt_debugfs.h"
 
+/* One day, this will be on your libdrm. */
+#define DRM_CLIENT_CAP_UNIVERSAL_PLANES 2
+
 #define MSR_PC8_RES	0x630
 #define MSR_PC9_RES	0x631
 #define MSR_PC10_RES	0x632
@@ -72,6 +75,12 @@ enum screen_type {
 	SCREEN_TYPE_ANY,
 };
 
+enum plane_type {
+	PLANE_OVERLAY,
+	PLANE_PRIMARY,
+	PLANE_CURSOR,
+};
+
 /* Wait flags */
 #define DONT_WAIT	0
 #define WAIT_STATUS	1
@@ -1504,6 +1513,199 @@ static void cursor_subtest(bool dpms)
 	igt_assert(wait_for_suspended());
 }
 
+static enum plane_type get_plane_type(uint32_t plane_id)
+{
+	drmModeObjectPropertiesPtr props;
+	int i, j;
+	enum plane_type type;
+	bool found = false;
+
+	props = drmModeObjectGetProperties(drm_fd, plane_id,
+					   DRM_MODE_OBJECT_PLANE);
+	igt_assert(props);
+
+	for (i = 0; i < props->count_props && !found; i++) {
+		drmModePropertyPtr prop;
+		const char *enum_name = NULL;
+
+		prop = drmModeGetProperty(drm_fd, props->props[i]);
+		igt_assert(prop);
+
+		if (strcmp(prop->name, "type") == 0) {
+			igt_assert(prop->flags & DRM_MODE_PROP_ENUM);
+			igt_assert(props->prop_values[i] < prop->count_enums);
+
+			for (j = 0; j < prop->count_enums; j++) {
+				if (prop->enums[j].value ==
+				    props->prop_values[i]) {
+					enum_name = prop->enums[j].name;
+					break;
+				}
+			}
+			igt_assert(enum_name);
+
+			if (strcmp(enum_name, "Overlay") == 0)
+				type = PLANE_OVERLAY;
+			else if (strcmp(enum_name, "Primary") == 0)
+				type = PLANE_PRIMARY;
+			else if (strcmp(enum_name, "Cursor") == 0)
+				type = PLANE_CURSOR;
+			else
+				igt_assert(0);
+
+			found = true;
+		}
+
+		drmModeFreeProperty(prop);
+	}
+	igt_assert(found);
+
+	drmModeFreeObjectProperties(props);
+	return type;
+}
+
+static void test_one_plane(bool dpms, uint32_t plane_id,
+			   enum plane_type plane_type)
+{
+	int rc;
+	uint32_t plane_format, plane_w, plane_h;
+	uint32_t crtc_id, connector_id;
+	struct igt_fb scanout_fb, plane_fb1, plane_fb2;
+	drmModeModeInfoPtr mode;
+	int32_t crtc_x = 0, crtc_y = 0;
+
+	disable_all_screens(&ms_data);
+	igt_assert(wait_for_suspended());
+
+	igt_require(find_connector_for_modeset(&ms_data, SCREEN_TYPE_ANY,
+					       &connector_id, &mode));
+
+	crtc_id = ms_data.res->crtcs[0];
+	igt_assert(crtc_id);
+
+	igt_create_fb(drm_fd, mode->hdisplay, mode->vdisplay,
+		      DRM_FORMAT_XRGB8888, false, &scanout_fb);
+
+	fill_igt_fb(&scanout_fb, 0xFF);
+
+	switch (plane_type) {
+	case PLANE_OVERLAY:
+		plane_format = DRM_FORMAT_XRGB8888;
+		plane_w = 64;
+		plane_h = 64;
+		break;
+	case PLANE_PRIMARY:
+		plane_format = DRM_FORMAT_XRGB8888;
+		plane_w = mode->hdisplay;
+		plane_h = mode->vdisplay;
+		break;
+	case PLANE_CURSOR:
+		plane_format = DRM_FORMAT_ARGB8888;
+		plane_w = 64;
+		plane_h = 64;
+		break;
+	default:
+		igt_assert(0);
+		break;
+	}
+
+	igt_create_fb(drm_fd, plane_w, plane_h, plane_format, false,
+		      &plane_fb1);
+	igt_create_fb(drm_fd, plane_w, plane_h, plane_format, false,
+		      &plane_fb2);
+	fill_igt_fb(&plane_fb1, 0xFF00FFFF);
+	fill_igt_fb(&plane_fb2, 0xFF00FF00);
+
+	rc = drmModeSetCrtc(drm_fd, crtc_id, scanout_fb.fb_id, 0, 0,
+			    &connector_id, 1, mode);
+	igt_assert(rc == 0);
+	igt_assert(wait_for_active());
+
+	rc = drmModeSetPlane(drm_fd, plane_id, crtc_id, plane_fb1.fb_id, 0,
+			     0, 0, plane_fb1.width, plane_fb1.height,
+			     0 << 16, 0 << 16, plane_fb1.width << 16,
+			     plane_fb1.height << 16);
+	igt_assert(rc == 0);
+
+	if (dpms)
+		disable_all_screens_dpms(&ms_data);
+	else
+		disable_all_screens(&ms_data);
+	igt_assert(wait_for_suspended());
+
+	/* Just move the plane around. */
+	if (plane_type != PLANE_PRIMARY) {
+		crtc_x++;
+		crtc_y++;
+	}
+	rc = drmModeSetPlane(drm_fd, plane_id, crtc_id, plane_fb1.fb_id, 0,
+			     crtc_x, crtc_y, plane_fb1.width, plane_fb1.height,
+			     0 << 16, 0 << 16, plane_fb1.width << 16,
+			     plane_fb1.height << 16);
+	igt_assert(rc == 0);
+
+	/* Unset, then change the plane. */
+	rc = drmModeSetPlane(drm_fd, plane_id, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0);
+	igt_assert(rc == 0);
+
+	rc = drmModeSetPlane(drm_fd, plane_id, crtc_id, plane_fb2.fb_id, 0,
+			     crtc_x, crtc_y, plane_fb2.width, plane_fb2.height,
+			     0 << 16, 0 << 16, plane_fb2.width << 16,
+			     plane_fb2.height << 16);
+	igt_assert(rc == 0);
+
+	/* Now change the plane without unsetting first. */
+	rc = drmModeSetPlane(drm_fd, plane_id, crtc_id, plane_fb1.fb_id, 0,
+			     crtc_x, crtc_y, plane_fb1.width, plane_fb1.height,
+			     0 << 16, 0 << 16, plane_fb1.width << 16,
+			     plane_fb1.height << 16);
+	igt_assert(rc == 0);
+
+	/* Make sure nothing remains for the other tests. */
+	rc = drmModeSetPlane(drm_fd, plane_id, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0);
+	igt_assert(rc == 0);
+}
+
+/* This one also triggered WARNs on our driver at some point in time. */
+static void planes_subtest(bool universal, bool dpms)
+{
+	int i, rc, planes_tested = 0;
+	drmModePlaneResPtr planes;
+
+	rc = drmSetClientCap(drm_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES,
+			     universal);
+	igt_require(rc == 0);
+
+	planes = drmModeGetPlaneResources(drm_fd);
+	for (i = 0; i < planes->count_planes; i++) {
+		drmModePlanePtr plane;
+
+		plane = drmModeGetPlane(drm_fd, planes->planes[i]);
+		igt_assert(plane);
+
+		/* We just pick the first CRTC on the list, so we can test for
+		 * 0x1 as the index. */
+		if (plane->possible_crtcs & 0x1) {
+			enum plane_type type;
+
+			type = universal ? get_plane_type(plane->plane_id) :
+					   PLANE_OVERLAY;
+			test_one_plane(dpms, plane->plane_id, type);
+			planes_tested++;
+		}
+		drmModeFreePlane(plane);
+	}
+	drmModeFreePlaneResources(planes);
+
+	rc = drmSetClientCap(drm_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 0);
+	igt_assert(rc == 0);
+
+	if (universal)
+		igt_assert(planes_tested >= 3);
+	else
+		igt_assert(planes_tested >= 1);
+}
+
 int rounds = 50;
 bool stay = false;
 
@@ -1577,11 +1779,19 @@ int main(int argc, char *argv[])
 	igt_subtest("gem-idle")
 		gem_idle_subtest();
 
-	/* Cursors */
+	/* Planes and cursors */
 	igt_subtest("cursor")
 		cursor_subtest(false);
 	igt_subtest("cursor-dpms")
 		cursor_subtest(true);
+	igt_subtest("legacy-planes")
+		planes_subtest(false, false);
+	igt_subtest("legacy-planes-dpms")
+		planes_subtest(false, true);
+	igt_subtest("universal-planes")
+		planes_subtest(true, false);
+	igt_subtest("universal-planes-dpms")
+		planes_subtest(true, true);
 
 	/* Misc */
 	igt_subtest("reg-read-ioctl")
-- 
2.0.1

[PATCH 3/3] drm/i915: get runtime PM when pinning primary plane objects

[Paulo Zanoni]

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

Otherwise we may get WARNs saying we're writing registers while
runtime suspended.

Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-dpms
Cc: stable@vger.kernel.org
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f1a9b5c..5948207 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11531,7 +11531,9 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 				  INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe));
 
 		/* Pin and return without programming hardware */
+		intel_runtime_pm_get(dev_priv);
 		ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
+		intel_runtime_pm_put(dev_priv);
 		mutex_unlock(&dev->struct_mutex);
 
 		return ret;
@@ -11553,7 +11555,9 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 		 * fail.
 		 */
 		if (plane->fb != fb) {
+			intel_runtime_pm_get(dev_priv);
 			ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
+			intel_runtime_pm_put(dev_priv);
 			if (ret) {
 				mutex_unlock(&dev->struct_mutex);
 				return ret;
-- 
2.0.1

Re: [PATCH 0/3] Fixes for runtime PM on planes APIs

[Matt Roper]

On Mon, Jul 28, 2014 at 03:37:11PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Hi
> 
> This series fixes some bugs that happen when we're runtime suspended and try to
> use the planes APIs. I also wrote IGT test cases for the bugs, so we will be
> able to detect future regressions.
> 
> The controversial part of these patches is that we had previously defined that
> we wanted to get/put runtime PM in the highest level of the stack, wrapping as
> much code as possible, but Daniel asked me to only get/put runtime PM around the
> functions that pin the objects (still on the highest level, but only around the
> pin functions). This series implements Daniel's suggestions.

These look pretty straightforward to me, so for all three:
        Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

However as a side note on the runtime PM stuff I'll admit that it's not
an area I'd previously paid too much attention to and my first reaction
looking at the patches was "I wonder if intel_runtime_pm_{get,put} could
be pushed down into intel_pin_and_fence_fb_obj()."  Until I read your
cover letter I wasn't aware of the goal of "get/put runtime PM in the
highest level of the stack, wrapping as much code as possible" and I
don't think that's really explained anywhere in the code or in the
commit log today.  Maybe we could add a comment in intel_pm.c explaining
that design goal for future contributors/reviewers?  


Matt

> 
> Thanks,
> Paulo
> 
> 
> Paulo Zanoni (3):
>   drm/i915: fix cursor handling when runtime suspended
>   drm/i915: get runtime PM when pinning sprite objects
>   drm/i915: get runtime PM when pinning primary plane objects
> 
>  drivers/gpu/drm/i915/intel_display.c | 9 +++++++++
>  drivers/gpu/drm/i915/intel_sprite.c  | 3 +++
>  2 files changed, 12 insertions(+)
> 
> -- 
> 2.0.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795

Re: [PATCH 2/2] tests/pm_rpm: add planes subtests

[Matt Roper]

On Mon, Jul 28, 2014 at 03:37:15PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Just like the cursor subtests, these also trigger WARNs on the current
> Kernel.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

I feel like a lot of the setup you have to do here is duplicating logic
we have in the igt_kms library.  Was there functionality lacking from
that library that prevented you from using it to write this test?  If
so, I can look into adding it.

Anyway, the test still looks good, just one or two minor comments inline
below.

> ---
>  tests/pm_rpm.c | 212 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 211 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c
> index f720f86..048d9ad 100644
> --- a/tests/pm_rpm.c
> +++ b/tests/pm_rpm.c
> @@ -51,6 +51,9 @@
>  #include "igt_kms.h"
>  #include "igt_debugfs.h"
>  
> +/* One day, this will be on your libdrm. */
> +#define DRM_CLIENT_CAP_UNIVERSAL_PLANES 2

I think there was just an official release of libdrm today that finally
includes this and the plane type enum below...maybe we can just bump the
libdrm requirement for igt and stop including these by hand?

> +
>  #define MSR_PC8_RES	0x630
>  #define MSR_PC9_RES	0x631
>  #define MSR_PC10_RES	0x632
> @@ -72,6 +75,12 @@ enum screen_type {
>  	SCREEN_TYPE_ANY,
>  };
>  
> +enum plane_type {
> +	PLANE_OVERLAY,
> +	PLANE_PRIMARY,
> +	PLANE_CURSOR,
> +};
> +
>  /* Wait flags */
>  #define DONT_WAIT	0
>  #define WAIT_STATUS	1
> @@ -1504,6 +1513,199 @@ static void cursor_subtest(bool dpms)
>  	igt_assert(wait_for_suspended());
>  }
>  
> +static enum plane_type get_plane_type(uint32_t plane_id)
> +{
> +	drmModeObjectPropertiesPtr props;
> +	int i, j;
> +	enum plane_type type;
> +	bool found = false;
> +
> +	props = drmModeObjectGetProperties(drm_fd, plane_id,
> +					   DRM_MODE_OBJECT_PLANE);
> +	igt_assert(props);
> +
> +	for (i = 0; i < props->count_props && !found; i++) {
> +		drmModePropertyPtr prop;
> +		const char *enum_name = NULL;
> +
> +		prop = drmModeGetProperty(drm_fd, props->props[i]);
> +		igt_assert(prop);
> +
> +		if (strcmp(prop->name, "type") == 0) {
> +			igt_assert(prop->flags & DRM_MODE_PROP_ENUM);
> +			igt_assert(props->prop_values[i] < prop->count_enums);
> +
> +			for (j = 0; j < prop->count_enums; j++) {
> +				if (prop->enums[j].value ==
> +				    props->prop_values[i]) {
> +					enum_name = prop->enums[j].name;
> +					break;
> +				}
> +			}
> +			igt_assert(enum_name);
> +
> +			if (strcmp(enum_name, "Overlay") == 0)
> +				type = PLANE_OVERLAY;
> +			else if (strcmp(enum_name, "Primary") == 0)
> +				type = PLANE_PRIMARY;
> +			else if (strcmp(enum_name, "Cursor") == 0)
> +				type = PLANE_CURSOR;
> +			else
> +				igt_assert(0);
> +
> +			found = true;
> +		}
> +
> +		drmModeFreeProperty(prop);
> +	}
> +	igt_assert(found);
> +
> +	drmModeFreeObjectProperties(props);
> +	return type;
> +}
> +
> +static void test_one_plane(bool dpms, uint32_t plane_id,
> +			   enum plane_type plane_type)
> +{
> +	int rc;
> +	uint32_t plane_format, plane_w, plane_h;
> +	uint32_t crtc_id, connector_id;
> +	struct igt_fb scanout_fb, plane_fb1, plane_fb2;
> +	drmModeModeInfoPtr mode;
> +	int32_t crtc_x = 0, crtc_y = 0;
> +
> +	disable_all_screens(&ms_data);
> +	igt_assert(wait_for_suspended());
> +
> +	igt_require(find_connector_for_modeset(&ms_data, SCREEN_TYPE_ANY,
> +					       &connector_id, &mode));
> +
> +	crtc_id = ms_data.res->crtcs[0];
> +	igt_assert(crtc_id);
> +
> +	igt_create_fb(drm_fd, mode->hdisplay, mode->vdisplay,
> +		      DRM_FORMAT_XRGB8888, false, &scanout_fb);
> +
> +	fill_igt_fb(&scanout_fb, 0xFF);
> +
> +	switch (plane_type) {
> +	case PLANE_OVERLAY:
> +		plane_format = DRM_FORMAT_XRGB8888;
> +		plane_w = 64;
> +		plane_h = 64;
> +		break;
> +	case PLANE_PRIMARY:
> +		plane_format = DRM_FORMAT_XRGB8888;
> +		plane_w = mode->hdisplay;
> +		plane_h = mode->vdisplay;
> +		break;
> +	case PLANE_CURSOR:
> +		plane_format = DRM_FORMAT_ARGB8888;
> +		plane_w = 64;
> +		plane_h = 64;
> +		break;
> +	default:
> +		igt_assert(0);
> +		break;
> +	}
> +
> +	igt_create_fb(drm_fd, plane_w, plane_h, plane_format, false,
> +		      &plane_fb1);
> +	igt_create_fb(drm_fd, plane_w, plane_h, plane_format, false,
> +		      &plane_fb2);
> +	fill_igt_fb(&plane_fb1, 0xFF00FFFF);
> +	fill_igt_fb(&plane_fb2, 0xFF00FF00);
> +
> +	rc = drmModeSetCrtc(drm_fd, crtc_id, scanout_fb.fb_id, 0, 0,
> +			    &connector_id, 1, mode);
> +	igt_assert(rc == 0);
> +	igt_assert(wait_for_active());
> +
> +	rc = drmModeSetPlane(drm_fd, plane_id, crtc_id, plane_fb1.fb_id, 0,
> +			     0, 0, plane_fb1.width, plane_fb1.height,
> +			     0 << 16, 0 << 16, plane_fb1.width << 16,
> +			     plane_fb1.height << 16);
> +	igt_assert(rc == 0);
> +
> +	if (dpms)
> +		disable_all_screens_dpms(&ms_data);
> +	else
> +		disable_all_screens(&ms_data);
> +	igt_assert(wait_for_suspended());
> +
> +	/* Just move the plane around. */
> +	if (plane_type != PLANE_PRIMARY) {
> +		crtc_x++;
> +		crtc_y++;
> +	}
> +	rc = drmModeSetPlane(drm_fd, plane_id, crtc_id, plane_fb1.fb_id, 0,
> +			     crtc_x, crtc_y, plane_fb1.width, plane_fb1.height,
> +			     0 << 16, 0 << 16, plane_fb1.width << 16,
> +			     plane_fb1.height << 16);
> +	igt_assert(rc == 0);
> +
> +	/* Unset, then change the plane. */
> +	rc = drmModeSetPlane(drm_fd, plane_id, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0);
> +	igt_assert(rc == 0);
> +
> +	rc = drmModeSetPlane(drm_fd, plane_id, crtc_id, plane_fb2.fb_id, 0,
> +			     crtc_x, crtc_y, plane_fb2.width, plane_fb2.height,
> +			     0 << 16, 0 << 16, plane_fb2.width << 16,
> +			     plane_fb2.height << 16);
> +	igt_assert(rc == 0);
> +
> +	/* Now change the plane without unsetting first. */
> +	rc = drmModeSetPlane(drm_fd, plane_id, crtc_id, plane_fb1.fb_id, 0,
> +			     crtc_x, crtc_y, plane_fb1.width, plane_fb1.height,
> +			     0 << 16, 0 << 16, plane_fb1.width << 16,
> +			     plane_fb1.height << 16);
> +	igt_assert(rc == 0);
> +
> +	/* Make sure nothing remains for the other tests. */
> +	rc = drmModeSetPlane(drm_fd, plane_id, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0);
> +	igt_assert(rc == 0);
> +}
> +
> +/* This one also triggered WARNs on our driver at some point in time. */
> +static void planes_subtest(bool universal, bool dpms)
> +{
> +	int i, rc, planes_tested = 0;
> +	drmModePlaneResPtr planes;
> +
> +	rc = drmSetClientCap(drm_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES,
> +			     universal);
> +	igt_require(rc == 0);

I think you want to make the setcap call conditional on universal.  If
we're running on an older kernel (pre-universal planes), the setcap
ioctl will return -EINVAL since it doesn't recognize
DRM_CLIENT_CAP_UNIVERSAL_PLANES and you'll never even get to try your
non-universal tests.

(I assume you still want to run on older kernels since otherwise I don't
think there'd be a need to test legacy and universal
separately...universal just adds extra stuff to the plane list, but you
still get all the same legacy planes in the list too.)


Matt

> +
> +	planes = drmModeGetPlaneResources(drm_fd);
> +	for (i = 0; i < planes->count_planes; i++) {
> +		drmModePlanePtr plane;
> +
> +		plane = drmModeGetPlane(drm_fd, planes->planes[i]);
> +		igt_assert(plane);
> +
> +		/* We just pick the first CRTC on the list, so we can test for
> +		 * 0x1 as the index. */
> +		if (plane->possible_crtcs & 0x1) {
> +			enum plane_type type;
> +
> +			type = universal ? get_plane_type(plane->plane_id) :
> +					   PLANE_OVERLAY;
> +			test_one_plane(dpms, plane->plane_id, type);
> +			planes_tested++;
> +		}
> +		drmModeFreePlane(plane);
> +	}
> +	drmModeFreePlaneResources(planes);
> +
> +	rc = drmSetClientCap(drm_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 0);
> +	igt_assert(rc == 0);
> +
> +	if (universal)
> +		igt_assert(planes_tested >= 3);
> +	else
> +		igt_assert(planes_tested >= 1);
> +}
> +
>  int rounds = 50;
>  bool stay = false;
>  
> @@ -1577,11 +1779,19 @@ int main(int argc, char *argv[])
>  	igt_subtest("gem-idle")
>  		gem_idle_subtest();
>  
> -	/* Cursors */
> +	/* Planes and cursors */
>  	igt_subtest("cursor")
>  		cursor_subtest(false);
>  	igt_subtest("cursor-dpms")
>  		cursor_subtest(true);
> +	igt_subtest("legacy-planes")
> +		planes_subtest(false, false);
> +	igt_subtest("legacy-planes-dpms")
> +		planes_subtest(false, true);
> +	igt_subtest("universal-planes")
> +		planes_subtest(true, false);
> +	igt_subtest("universal-planes-dpms")
> +		planes_subtest(true, true);
>  
>  	/* Misc */
>  	igt_subtest("reg-read-ioctl")
> -- 
> 2.0.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795

Re: [PATCH 0/3] Fixes for runtime PM on planes APIs

[Daniel Vetter]

On Tue, Jul 29, 2014 at 1:25 AM, Matt Roper <matthew.d.roper@intel.com> wrote:
> On Mon, Jul 28, 2014 at 03:37:11PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> Hi
>>
>> This series fixes some bugs that happen when we're runtime suspended and try to
>> use the planes APIs. I also wrote IGT test cases for the bugs, so we will be
>> able to detect future regressions.
>>
>> The controversial part of these patches is that we had previously defined that
>> we wanted to get/put runtime PM in the highest level of the stack, wrapping as
>> much code as possible, but Daniel asked me to only get/put runtime PM around the
>> functions that pin the objects (still on the highest level, but only around the
>> pin functions). This series implements Daniel's suggestions.
>
> These look pretty straightforward to me, so for all three:
>         Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
>
> However as a side note on the runtime PM stuff I'll admit that it's not
> an area I'd previously paid too much attention to and my first reaction
> looking at the patches was "I wonder if intel_runtime_pm_{get,put} could
> be pushed down into intel_pin_and_fence_fb_obj()."  Until I read your
> cover letter I wasn't aware of the goal of "get/put runtime PM in the
> highest level of the stack, wrapping as much code as possible" and I
> don't think that's really explained anywhere in the code or in the
> commit log today.  Maybe we could add a comment in intel_pm.c explaining
> that design goal for future contributors/reviewers?

Originally we had runtime pm calls way down, which in a few cases
meant that we'd dropped the runtime pm reference while we still
depended upon the register values surviving. Of course the usual timer
helps there. Hence why I've put out the "push it to the highest level
possible" guideline, to make sure that e.g. for a modeset sequence we
grab the runtime pm ref once, before we start touching any hardware.

Pinning stuff to the gtt (which is the only thing tricky here, after
the early bail-out for !crtc->active) is a bit different. Those
registers are magic aliases for memory regions, so even when we go
into runtime pm the register value will survive (since the ptes are
actually in main memory). We only need to wake up the gpu to make sure
they actually get there. And the "pin plane while display is off" case
is really special, all the pte writing we do in the gem code is done
as part of a userspace operation where we really need to have the gpu
on (and so where it makes sense to grab the runtime pm ref at the
top-level to make sure our setup doesn't disappear unexpectedly).

I hope this explains why these 3 cases are special.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/3] drm/i915: fix cursor handling when runtime suspended

[Ville Syrjälä]

On Mon, Jul 28, 2014 at 03:37:12PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> If we're runtime suspended and try to use the cursor interfaces, we
> will get a lot of WARNs saying we did the wrong thing.
> 
> For intel_crtc_update_cursor(), all we need to do is return if the
> CRTC is not active, since writing the registers won't really have any
> effect if the screen is not visible, and we will write the registers
> later when enabling the screen.
> 
> For intel_crtc_cursor_set_obj(), we just need to wake up the hardware
> then pinning the display plane.
> 
> v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
> 
> Testcase: igt/pm_rpm/cursor
> Testcase: igt/pm_rpm/cursor-dpms
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81645
> Cc: stable@vger.kernel.org
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1edfd1a..f1a9b5c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8144,6 +8144,9 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
>  	if (base == 0 && intel_crtc->cursor_base == 0)
>  		return;
>  
> +	if (!intel_crtc->active)
> +		return;
> +
>  	I915_WRITE(CURPOS(pipe), pos);
>  
>  	if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev) || IS_BROADWELL(dev))
> @@ -8217,7 +8220,9 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
>  		if (need_vtd_wa(dev))
>  			alignment = 64*1024;
>  
> +		intel_runtime_pm_get(dev_priv);
>  		ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
> +		intel_runtime_pm_put(dev_priv);

put_fence() would need runtime pm too I think.

>  		if (ret) {
>  			DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
>  			goto fail_locked;
> -- 
> 2.0.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/3] drm/i915: fix cursor handling when runtime suspended

[Paulo Zanoni]

2014-07-29 7:22 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> On Mon, Jul 28, 2014 at 03:37:12PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> If we're runtime suspended and try to use the cursor interfaces, we
>> will get a lot of WARNs saying we did the wrong thing.
>>
>> For intel_crtc_update_cursor(), all we need to do is return if the
>> CRTC is not active, since writing the registers won't really have any
>> effect if the screen is not visible, and we will write the registers
>> later when enabling the screen.
>>
>> For intel_crtc_cursor_set_obj(), we just need to wake up the hardware
>> then pinning the display plane.
>>
>> v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
>>
>> Testcase: igt/pm_rpm/cursor
>> Testcase: igt/pm_rpm/cursor-dpms
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81645
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 1edfd1a..f1a9b5c 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -8144,6 +8144,9 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
>>       if (base == 0 && intel_crtc->cursor_base == 0)
>>               return;
>>
>> +     if (!intel_crtc->active)
>> +             return;
>> +
>>       I915_WRITE(CURPOS(pipe), pos);
>>
>>       if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev) || IS_BROADWELL(dev))
>> @@ -8217,7 +8220,9 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
>>               if (need_vtd_wa(dev))
>>                       alignment = 64*1024;
>>
>> +             intel_runtime_pm_get(dev_priv);
>>               ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
>> +             intel_runtime_pm_put(dev_priv);
>
> put_fence() would need runtime pm too I think.

This is exactly why I wanted the get/put calls around everything on
those functions... I'll see if I can reproduce this on the test suite
too.

>
>>               if (ret) {
>>                       DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
>>                       goto fail_locked;
>> --
>> 2.0.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC



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

Re: [PATCH 2/2] tests/pm_rpm: add planes subtests

[Paulo Zanoni]

2014-07-28 20:47 GMT-03:00 Matt Roper <matthew.d.roper@intel.com>:
> On Mon, Jul 28, 2014 at 03:37:15PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> Just like the cursor subtests, these also trigger WARNs on the current
>> Kernel.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> I feel like a lot of the setup you have to do here is duplicating logic
> we have in the igt_kms library.  Was there functionality lacking from
> that library that prevented you from using it to write this test?  If
> so, I can look into adding it.

Every single ioctl we call can result in runtime PM get/put calls
inside the driver, so for pm_rpm.c I would like to keep using the low
level interfaces, to make sure the suspends and resumes are
controlled.

Anyway, I never really looked at the library before. It seems the
biggest functionality missing from it is documentation. I just took a
look at the .c file and couldn't find anything that looked like would
reduce my diffstat, since the libdrm functions we call on pm_rpm.c are
very simple. Any suggestions?

>
> Anyway, the test still looks good, just one or two minor comments inline
> below.
>
>> ---
>>  tests/pm_rpm.c | 212 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 211 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c
>> index f720f86..048d9ad 100644
>> --- a/tests/pm_rpm.c
>> +++ b/tests/pm_rpm.c
>> @@ -51,6 +51,9 @@
>>  #include "igt_kms.h"
>>  #include "igt_debugfs.h"
>>
>> +/* One day, this will be on your libdrm. */
>> +#define DRM_CLIENT_CAP_UNIVERSAL_PLANES 2
>
> I think there was just an official release of libdrm today that finally
> includes this and the plane type enum below...maybe we can just bump the
> libdrm requirement for igt and stop including these by hand?

I just noticed igt_kms.c also has this. I also have to be honest and
tell you that I get extremely annoyed when we bump the IGT
requirements to recently-released libdrm :)

>
>> +
>>  #define MSR_PC8_RES  0x630
>>  #define MSR_PC9_RES  0x631
>>  #define MSR_PC10_RES 0x632
>> @@ -72,6 +75,12 @@ enum screen_type {
>>       SCREEN_TYPE_ANY,
>>  };
>>
>> +enum plane_type {
>> +     PLANE_OVERLAY,
>> +     PLANE_PRIMARY,
>> +     PLANE_CURSOR,
>> +};
>> +
>>  /* Wait flags */
>>  #define DONT_WAIT    0
>>  #define WAIT_STATUS  1
>> @@ -1504,6 +1513,199 @@ static void cursor_subtest(bool dpms)
>>       igt_assert(wait_for_suspended());
>>  }
>>
>> +static enum plane_type get_plane_type(uint32_t plane_id)
>> +{
>> +     drmModeObjectPropertiesPtr props;
>> +     int i, j;
>> +     enum plane_type type;
>> +     bool found = false;
>> +
>> +     props = drmModeObjectGetProperties(drm_fd, plane_id,
>> +                                        DRM_MODE_OBJECT_PLANE);
>> +     igt_assert(props);
>> +
>> +     for (i = 0; i < props->count_props && !found; i++) {
>> +             drmModePropertyPtr prop;
>> +             const char *enum_name = NULL;
>> +
>> +             prop = drmModeGetProperty(drm_fd, props->props[i]);
>> +             igt_assert(prop);
>> +
>> +             if (strcmp(prop->name, "type") == 0) {
>> +                     igt_assert(prop->flags & DRM_MODE_PROP_ENUM);
>> +                     igt_assert(props->prop_values[i] < prop->count_enums);
>> +
>> +                     for (j = 0; j < prop->count_enums; j++) {
>> +                             if (prop->enums[j].value ==
>> +                                 props->prop_values[i]) {
>> +                                     enum_name = prop->enums[j].name;
>> +                                     break;
>> +                             }
>> +                     }
>> +                     igt_assert(enum_name);
>> +
>> +                     if (strcmp(enum_name, "Overlay") == 0)
>> +                             type = PLANE_OVERLAY;
>> +                     else if (strcmp(enum_name, "Primary") == 0)
>> +                             type = PLANE_PRIMARY;
>> +                     else if (strcmp(enum_name, "Cursor") == 0)
>> +                             type = PLANE_CURSOR;
>> +                     else
>> +                             igt_assert(0);
>> +
>> +                     found = true;
>> +             }
>> +
>> +             drmModeFreeProperty(prop);
>> +     }
>> +     igt_assert(found);
>> +
>> +     drmModeFreeObjectProperties(props);
>> +     return type;
>> +}
>> +
>> +static void test_one_plane(bool dpms, uint32_t plane_id,
>> +                        enum plane_type plane_type)
>> +{
>> +     int rc;
>> +     uint32_t plane_format, plane_w, plane_h;
>> +     uint32_t crtc_id, connector_id;
>> +     struct igt_fb scanout_fb, plane_fb1, plane_fb2;
>> +     drmModeModeInfoPtr mode;
>> +     int32_t crtc_x = 0, crtc_y = 0;
>> +
>> +     disable_all_screens(&ms_data);
>> +     igt_assert(wait_for_suspended());
>> +
>> +     igt_require(find_connector_for_modeset(&ms_data, SCREEN_TYPE_ANY,
>> +                                            &connector_id, &mode));
>> +
>> +     crtc_id = ms_data.res->crtcs[0];
>> +     igt_assert(crtc_id);
>> +
>> +     igt_create_fb(drm_fd, mode->hdisplay, mode->vdisplay,
>> +                   DRM_FORMAT_XRGB8888, false, &scanout_fb);
>> +
>> +     fill_igt_fb(&scanout_fb, 0xFF);
>> +
>> +     switch (plane_type) {
>> +     case PLANE_OVERLAY:
>> +             plane_format = DRM_FORMAT_XRGB8888;
>> +             plane_w = 64;
>> +             plane_h = 64;
>> +             break;
>> +     case PLANE_PRIMARY:
>> +             plane_format = DRM_FORMAT_XRGB8888;
>> +             plane_w = mode->hdisplay;
>> +             plane_h = mode->vdisplay;
>> +             break;
>> +     case PLANE_CURSOR:
>> +             plane_format = DRM_FORMAT_ARGB8888;
>> +             plane_w = 64;
>> +             plane_h = 64;
>> +             break;
>> +     default:
>> +             igt_assert(0);
>> +             break;
>> +     }
>> +
>> +     igt_create_fb(drm_fd, plane_w, plane_h, plane_format, false,
>> +                   &plane_fb1);
>> +     igt_create_fb(drm_fd, plane_w, plane_h, plane_format, false,
>> +                   &plane_fb2);
>> +     fill_igt_fb(&plane_fb1, 0xFF00FFFF);
>> +     fill_igt_fb(&plane_fb2, 0xFF00FF00);
>> +
>> +     rc = drmModeSetCrtc(drm_fd, crtc_id, scanout_fb.fb_id, 0, 0,
>> +                         &connector_id, 1, mode);
>> +     igt_assert(rc == 0);
>> +     igt_assert(wait_for_active());
>> +
>> +     rc = drmModeSetPlane(drm_fd, plane_id, crtc_id, plane_fb1.fb_id, 0,
>> +                          0, 0, plane_fb1.width, plane_fb1.height,
>> +                          0 << 16, 0 << 16, plane_fb1.width << 16,
>> +                          plane_fb1.height << 16);
>> +     igt_assert(rc == 0);
>> +
>> +     if (dpms)
>> +             disable_all_screens_dpms(&ms_data);
>> +     else
>> +             disable_all_screens(&ms_data);
>> +     igt_assert(wait_for_suspended());
>> +
>> +     /* Just move the plane around. */
>> +     if (plane_type != PLANE_PRIMARY) {
>> +             crtc_x++;
>> +             crtc_y++;
>> +     }
>> +     rc = drmModeSetPlane(drm_fd, plane_id, crtc_id, plane_fb1.fb_id, 0,
>> +                          crtc_x, crtc_y, plane_fb1.width, plane_fb1.height,
>> +                          0 << 16, 0 << 16, plane_fb1.width << 16,
>> +                          plane_fb1.height << 16);
>> +     igt_assert(rc == 0);
>> +
>> +     /* Unset, then change the plane. */
>> +     rc = drmModeSetPlane(drm_fd, plane_id, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0);
>> +     igt_assert(rc == 0);
>> +
>> +     rc = drmModeSetPlane(drm_fd, plane_id, crtc_id, plane_fb2.fb_id, 0,
>> +                          crtc_x, crtc_y, plane_fb2.width, plane_fb2.height,
>> +                          0 << 16, 0 << 16, plane_fb2.width << 16,
>> +                          plane_fb2.height << 16);
>> +     igt_assert(rc == 0);
>> +
>> +     /* Now change the plane without unsetting first. */
>> +     rc = drmModeSetPlane(drm_fd, plane_id, crtc_id, plane_fb1.fb_id, 0,
>> +                          crtc_x, crtc_y, plane_fb1.width, plane_fb1.height,
>> +                          0 << 16, 0 << 16, plane_fb1.width << 16,
>> +                          plane_fb1.height << 16);
>> +     igt_assert(rc == 0);
>> +
>> +     /* Make sure nothing remains for the other tests. */
>> +     rc = drmModeSetPlane(drm_fd, plane_id, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0);
>> +     igt_assert(rc == 0);
>> +}
>> +
>> +/* This one also triggered WARNs on our driver at some point in time. */
>> +static void planes_subtest(bool universal, bool dpms)
>> +{
>> +     int i, rc, planes_tested = 0;
>> +     drmModePlaneResPtr planes;
>> +
>> +     rc = drmSetClientCap(drm_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES,
>> +                          universal);
>> +     igt_require(rc == 0);
>
> I think you want to make the setcap call conditional on universal.  If
> we're running on an older kernel (pre-universal planes), the setcap
> ioctl will return -EINVAL since it doesn't recognize
> DRM_CLIENT_CAP_UNIVERSAL_PLANES and you'll never even get to try your
> non-universal tests.
>
> (I assume you still want to run on older kernels since otherwise I don't
> think there'd be a need to test legacy and universal
> separately...universal just adds extra stuff to the plane list, but you
> still get all the same legacy planes in the list too.)

You're right. I'll fix this. Thanks!

>
>
> Matt
>
>> +
>> +     planes = drmModeGetPlaneResources(drm_fd);
>> +     for (i = 0; i < planes->count_planes; i++) {
>> +             drmModePlanePtr plane;
>> +
>> +             plane = drmModeGetPlane(drm_fd, planes->planes[i]);
>> +             igt_assert(plane);
>> +
>> +             /* We just pick the first CRTC on the list, so we can test for
>> +              * 0x1 as the index. */
>> +             if (plane->possible_crtcs & 0x1) {
>> +                     enum plane_type type;
>> +
>> +                     type = universal ? get_plane_type(plane->plane_id) :
>> +                                        PLANE_OVERLAY;
>> +                     test_one_plane(dpms, plane->plane_id, type);
>> +                     planes_tested++;
>> +             }
>> +             drmModeFreePlane(plane);
>> +     }
>> +     drmModeFreePlaneResources(planes);
>> +
>> +     rc = drmSetClientCap(drm_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 0);
>> +     igt_assert(rc == 0);
>> +
>> +     if (universal)
>> +             igt_assert(planes_tested >= 3);
>> +     else
>> +             igt_assert(planes_tested >= 1);
>> +}
>> +
>>  int rounds = 50;
>>  bool stay = false;
>>
>> @@ -1577,11 +1779,19 @@ int main(int argc, char *argv[])
>>       igt_subtest("gem-idle")
>>               gem_idle_subtest();
>>
>> -     /* Cursors */
>> +     /* Planes and cursors */
>>       igt_subtest("cursor")
>>               cursor_subtest(false);
>>       igt_subtest("cursor-dpms")
>>               cursor_subtest(true);
>> +     igt_subtest("legacy-planes")
>> +             planes_subtest(false, false);
>> +     igt_subtest("legacy-planes-dpms")
>> +             planes_subtest(false, true);
>> +     igt_subtest("universal-planes")
>> +             planes_subtest(true, false);
>> +     igt_subtest("universal-planes-dpms")
>> +             planes_subtest(true, true);
>>
>>       /* Misc */
>>       igt_subtest("reg-read-ioctl")
>> --
>> 2.0.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795



-- 
Paulo Zanoni

Re: [PATCH 2/2] tests/pm_rpm: add planes subtests

[Matt Roper]

On Tue, Aug 05, 2014 at 06:34:38PM -0300, Paulo Zanoni wrote:
> 2014-07-28 20:47 GMT-03:00 Matt Roper <matthew.d.roper@intel.com>:
> > On Mon, Jul 28, 2014 at 03:37:15PM -0300, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> Just like the cursor subtests, these also trigger WARNs on the current
> >> Kernel.
> >>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > I feel like a lot of the setup you have to do here is duplicating logic
> > we have in the igt_kms library.  Was there functionality lacking from
> > that library that prevented you from using it to write this test?  If
> > so, I can look into adding it.
> 
> Every single ioctl we call can result in runtime PM get/put calls
> inside the driver, so for pm_rpm.c I would like to keep using the low
> level interfaces, to make sure the suspends and resumes are
> controlled.
> 
> Anyway, I never really looked at the library before. It seems the
> biggest functionality missing from it is documentation. I just took a
> look at the .c file and couldn't find anything that looked like would
> reduce my diffstat, since the libdrm functions we call on pm_rpm.c are
> very simple. Any suggestions?

The main areas where I thought it might be possible to slim down a bit
by using igt_kms were all the setup code --- grabbing plane resources,
counting/looping over planes, grabbing properties to check plane types,
etc.  igt_kms will build up the plane list internally and hide a lot of
that long, boring code from your tests.  But since you've already
written the test without it, I don't feel there's any major need to
rewrite it with igt_kms; I was just curious if there was anything
specific you thought was lacking from the library so that we could
address it in the future.

I did add some kerneldoc comments when I added the new interfaces in
preparation for universal planes & atomic modeset, but you're right that
there's still a lot that could be better documented going forward.


Matt

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795

Re: [PATCH 2/2] tests/pm_rpm: add planes subtests

[Paulo Zanoni]

2014-08-05 18:51 GMT-03:00 Matt Roper <matthew.d.roper@intel.com>:
> On Tue, Aug 05, 2014 at 06:34:38PM -0300, Paulo Zanoni wrote:
>> 2014-07-28 20:47 GMT-03:00 Matt Roper <matthew.d.roper@intel.com>:
>> > On Mon, Jul 28, 2014 at 03:37:15PM -0300, Paulo Zanoni wrote:
>> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >>
>> >> Just like the cursor subtests, these also trigger WARNs on the current
>> >> Kernel.
>> >>
>> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >
>> > I feel like a lot of the setup you have to do here is duplicating logic
>> > we have in the igt_kms library.  Was there functionality lacking from
>> > that library that prevented you from using it to write this test?  If
>> > so, I can look into adding it.
>>
>> Every single ioctl we call can result in runtime PM get/put calls
>> inside the driver, so for pm_rpm.c I would like to keep using the low
>> level interfaces, to make sure the suspends and resumes are
>> controlled.
>>
>> Anyway, I never really looked at the library before. It seems the
>> biggest functionality missing from it is documentation. I just took a
>> look at the .c file and couldn't find anything that looked like would
>> reduce my diffstat, since the libdrm functions we call on pm_rpm.c are
>> very simple. Any suggestions?
>
> The main areas where I thought it might be possible to slim down a bit
> by using igt_kms were all the setup code --- grabbing plane resources,
> counting/looping over planes, grabbing properties to check plane types,
> etc.  igt_kms will build up the plane list internally and hide a lot of
> that long, boring code from your tests.  But since you've already
> written the test without it, I don't feel there's any major need to
> rewrite it with igt_kms; I was just curious if there was anything
> specific you thought was lacking from the library so that we could
> address it in the future.

The big problem I see is that all/most functions take the
"igt_display_t" type as an argument instead of taking plain libdrm
types, so either you fully adopt the API, or you don't use it at all.
To be able to use igt_kms at all I'd have to call igt_display_init(),
which does way too much stuff, and is probably going to grow more, and
at some point do something that gets too many power refcounts and
breaks runtime PM.

I would really love if the igt_kms functions were little helpers that
accepted the plain libdrm types as arguments instead of its own types.
This way, I would, for example, probably be able to reuse
get_plane_property() or other functions. I see that on a lot of cases,
we pass igt_display_t just to use the FD, so why not just use the FD?

I'll try to write some patches to reuse the stuff I want to reuse,
then you can comment on them.

>
> I did add some kerneldoc comments when I added the new interfaces in
> preparation for universal planes & atomic modeset, but you're right that
> there's still a lot that could be better documented going forward.
>
>
> Matt
>
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795



-- 
Paulo Zanoni

Re: [PATCH 2/2] tests/pm_rpm: add planes subtests

[Daniel Vetter]

On Wed, Aug 06, 2014 at 11:11:41AM -0300, Paulo Zanoni wrote:
> 2014-08-05 18:51 GMT-03:00 Matt Roper <matthew.d.roper@intel.com>:
> > On Tue, Aug 05, 2014 at 06:34:38PM -0300, Paulo Zanoni wrote:
> >> 2014-07-28 20:47 GMT-03:00 Matt Roper <matthew.d.roper@intel.com>:
> >> > On Mon, Jul 28, 2014 at 03:37:15PM -0300, Paulo Zanoni wrote:
> >> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> >>
> >> >> Just like the cursor subtests, these also trigger WARNs on the current
> >> >> Kernel.
> >> >>
> >> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> >
> >> > I feel like a lot of the setup you have to do here is duplicating logic
> >> > we have in the igt_kms library.  Was there functionality lacking from
> >> > that library that prevented you from using it to write this test?  If
> >> > so, I can look into adding it.
> >>
> >> Every single ioctl we call can result in runtime PM get/put calls
> >> inside the driver, so for pm_rpm.c I would like to keep using the low
> >> level interfaces, to make sure the suspends and resumes are
> >> controlled.
> >>
> >> Anyway, I never really looked at the library before. It seems the
> >> biggest functionality missing from it is documentation. I just took a
> >> look at the .c file and couldn't find anything that looked like would
> >> reduce my diffstat, since the libdrm functions we call on pm_rpm.c are
> >> very simple. Any suggestions?
> >
> > The main areas where I thought it might be possible to slim down a bit
> > by using igt_kms were all the setup code --- grabbing plane resources,
> > counting/looping over planes, grabbing properties to check plane types,
> > etc.  igt_kms will build up the plane list internally and hide a lot of
> > that long, boring code from your tests.  But since you've already
> > written the test without it, I don't feel there's any major need to
> > rewrite it with igt_kms; I was just curious if there was anything
> > specific you thought was lacking from the library so that we could
> > address it in the future.
> 
> The big problem I see is that all/most functions take the
> "igt_display_t" type as an argument instead of taking plain libdrm
> types, so either you fully adopt the API, or you don't use it at all.
> To be able to use igt_kms at all I'd have to call igt_display_init(),
> which does way too much stuff, and is probably going to grow more, and
> at some point do something that gets too many power refcounts and
> breaks runtime PM.
> 
> I would really love if the igt_kms functions were little helpers that
> accepted the plain libdrm types as arguments instead of its own types.
> This way, I would, for example, probably be able to reuse
> get_plane_property() or other functions. I see that on a lot of cases,
> we pass igt_display_t just to use the FD, so why not just use the FD?
> 
> I'll try to write some patches to reuse the stuff I want to reuse,
> then you can comment on them.

igt_kms is kinda a helper library in the larva stage and still looking for
an optimal design. Personally I'm also not really happy with it, which is
why I didn't write the documentation back when I've done that for all
other igt stuff - it just wasn't clear yet what the different parts should
do.
-Daniel

> 
> >
> > I did add some kerneldoc comments when I added the new interfaces in
> > preparation for universal planes & atomic modeset, but you're right that
> > there's still a lot that could be better documented going forward.
> >
> >
> > Matt
> >
> > --
> > Matt Roper
> > Graphics Software Engineer
> > IoTG Platform Enabling & Development
> > Intel Corporation
> > (916) 356-2795
> 
> 
> 
> -- 
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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