Fixes for runtime PM on planes APIs

From: Paulo Zanoni <[email protected]>

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

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

---
 drivers/gpu/drm/i915/intel_display.c | 5 +++++
 1 file changed, 5 insertions(+)

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

index 1edfd1a..f1a9b5c 100644
@@ -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))

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

 		if (ret) {
 			DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
 			goto fail_locked;

Signature · 6 lines

2.0.1

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

Quoted 42 lines

From: Paulo Zanoni <[email protected]>

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

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

---
 drivers/gpu/drm/i915/intel_display.c | 5 +++++
 1 file changed, 5 insertions(+)

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

index 1edfd1a..f1a9b5c 100644
@@ -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;
 

drivers/gpu/drm/i915/intel_display.c static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc, · line 8225

Quoted 3 lines

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

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

Quoted 11 lines

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

              if (ret) {
                      DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
                      goto fail_locked;
--

2.0.1

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

--
Ville Syrjälä
Intel OTC

From: Paulo Zanoni <[email protected]>

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

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

---
 tests/pm_rpm.c | 212 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 211 insertions(+), 1 deletion(-)

tests/pm_rpm.c +0 -0

index f720f86..048d9ad 100644

tests/pm_rpm.c +159 -0

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

tests/pm_rpm.c +40 -1

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

Signature · 6 lines

2.0.1

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

Quoted 5 lines

From: Paulo Zanoni <[email protected]>

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

Quoted 1 line

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

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.

Quoted 12 lines

---
 tests/pm_rpm.c | 212 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 211 insertions(+), 1 deletion(-)

tests/pm_rpm.c +0 -0

index f720f86..048d9ad 100644
@@ -51,6 +51,9 @@
 #include "igt_kms.h"
 #include "igt_debugfs.h"
 

tests/pm_rpm.c line 55

Quoted 2 lines

tests/pm_rpm.c +2 -0

+/* 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?

Quoted 174 lines

tests/pm_rpm.c +159 -0

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

tests/pm_rpm.c static void cursor_subtest(bool dpms) · line 1677

Quoted 10 lines

tests/pm_rpm.c +10 -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

Quoted 58 lines

tests/pm_rpm.c +40 -2

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

Quoted 4 lines

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

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

2014-07-28 20:47 GMT-03:00 Matt Roper <[email protected]>:

Quoted 6 lines · Paulo Zanoni

Quoted 5 lines

From: Paulo Zanoni <[email protected]>

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

Quoted 6 lines

Quoted 1 line

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

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?

Quoted 19 lines

2014-07-28 20:47 GMT-03:00 Matt Roper <[email protected]>:

Quoted 6 lines · Paulo Zanoni

Quoted 5 lines

From: Paulo Zanoni <[email protected]>

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

Quoted 6 lines

Quoted 1 line

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

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.

Quoted 5 lines

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

2014-08-05 18:51 GMT-03:00 Matt Roper <[email protected]>:

Quoted 26 lines · Paulo Zanoni

Quoted 19 lines

2014-07-28 20:47 GMT-03:00 Matt Roper <[email protected]>:

Quoted 6 lines · Paulo Zanoni

Quoted 5 lines

From: Paulo Zanoni <[email protected]>

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

Quoted 6 lines

Quoted 1 line

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

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.

Quoted 5 lines

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?

Quoted 9 lines

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.

Quoted 15 lines

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

Signature · 5 lines

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

From: Paulo Zanoni <[email protected]>

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

Signature · 6 lines

2.0.1

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

Quoted 8 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

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.

Quoted 5 lines

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

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?