Skip to contentSkip to conversation

drm/i915: fix plane/cursor handling when runtime suspended

PATCH In review Paulo Zanoni 5 participants 26 messages Aug 6, 2014 – Aug 26, 2014

Subsystems (2)
DRM drivers Intel i915
lore
Share & tools
lore

Paulo Zanoni

author
#0 lore raw

PATCH 1/1 drm/i915: fix plane/cursor handling when runtime suspended

From: Paulo Zanoni <[email protected]>

If we're runtime suspended and try to use the plane 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 all the other cases, we need to get runtime PM references to pin/unpin the objects, and to change the fences. The pin/unpin functions are the ideal places for this, but intel_crtc_cursor_set_obj() doesn't call them, so we also have to add get/put calls inside it. There is no problem if we runtime suspend right after these functions are finished, because the registers weitten are forwarded to system memory.

v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
v3: - Make get/put also surround the fence and unpin calls (Daniel and
Ville).
- Merge all the plane changes into a single patch since they're the same fix.
- Add the comment requested by Daniel.

Testcase: igt/pm_rpm/cursor
Testcase: igt/pm_rpm/cursor-dpms
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-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 | 39 +++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/intel_display.c +38 -1
index 4f659eb..a86d67c 100644
@@ -2212,6 +2212,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
 	if (need_vtd_wa(dev) && alignment < 256 * 1024)
 		alignment = 256 * 1024;
 
+	/*
+	 * Global gtt pte registers are special registers which actually forward
+	 * writes to a chunk of system memory. Which means that there is no risk
+	 * that the register values disappear as soon as we call
+	 * intel_runtime_pm_put(), so it is correct to wrap only the
+	 * pin/unpin/fence and not more.
+	 */
+	intel_runtime_pm_get(dev_priv);
+
 	dev_priv->mm.interruptible = false;
 	ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
 	if (ret)

@@ -2229,21 +2238,30 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
 	i915_gem_object_pin_fence(obj);
 
 	dev_priv->mm.interruptible = true;
+	intel_runtime_pm_put(dev_priv);
 	return 0;
 
 err_unpin:
 	i915_gem_object_unpin_from_display_plane(obj);
 err_interruptible:
 	dev_priv->mm.interruptible = true;
+	intel_runtime_pm_put(dev_priv);
 	return ret;
 }
 
 void intel_unpin_fb_obj(struct drm_i915_gem_object *obj)
 {
-	WARN_ON(!mutex_is_locked(&obj->base.dev->struct_mutex));
+	struct drm_device *dev = obj->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
+	intel_runtime_pm_get(dev_priv);
 
 	i915_gem_object_unpin_fence(obj);
 	i915_gem_object_unpin_from_display_plane(obj);
+
+	intel_runtime_pm_put(dev_priv);
 }
 
 /* Computes the linear offset to the base tile and adjusts x, y. bytes per pixel

@@ -8285,6 +8303,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))

@@ -8340,9 +8361,20 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 
 	/* we only need to pin inside GTT if cursor is non-phy */
 	mutex_lock(&dev->struct_mutex);
+
+	/*
+	 * Global gtt pte registers are special registers which actually forward
+	 * writes to a chunk of system memory. Which means that there is no risk
+	 * that the register values disappear as soon as we call
+	 * intel_runtime_pm_put(), so it is correct to wrap only the
+	 * pin/unpin/fence and not more.
+	 */
+	intel_runtime_pm_get(dev_priv);
+
 	if (!INTEL_INFO(dev)->cursor_needs_physical) {
 		unsigned alignment;
 
+
 		if (obj->tiling_mode) {
 			DRM_DEBUG_KMS("cursor cannot be tiled\n");
 			ret = -EINVAL;

@@ -8392,6 +8424,10 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 
 	i915_gem_track_fb(intel_crtc->cursor_bo, obj,
 			  INTEL_FRONTBUFFER_CURSOR(pipe));
+
+	if (obj)
+		intel_runtime_pm_put(dev_priv);
+
 	mutex_unlock(&dev->struct_mutex);
 
 	old_width = intel_crtc->cursor_width;

@@ -8413,6 +8449,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 fail_unpin:
 	i915_gem_object_unpin_from_display_plane(obj);
 fail_locked:
+	intel_runtime_pm_put(dev_priv);
 	mutex_unlock(&dev->struct_mutex);
 fail:
 	drm_gem_object_unreference_unlocked(&obj->base);
Signature · 1 line
2.0.1
author
#1 lore raw

PATCH 1/1 tests/pm_rpm: add subtests for planes and cursors

From: Paulo Zanoni <[email protected]>

These tests should trigger WARNs on Kernels that don't have the most recent fixes.

v2: - Merge both the cursor and planes patches into one so it's easier
to update them.
- Extend the tests a little bit to exercise fences.
- Fix bug found by Matt to enable the test to run on older Kernels

Signed-off-by Paulo Zanoni <[email protected]>
---
 tests/pm_rpm.c | 354 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 344 insertions(+), 10 deletions(-)
tests/pm_rpm.c +344 -10
index 869e6f3..0633476 100644
@@ -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

@@ -277,13 +286,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 +307,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 +331,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 +1426,307 @@ 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__gtt(drm_fd, fb->gem_handle, fb->size, PROT_WRITE);
+	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, cursor_fb3;
+
+	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);
+	igt_create_fb(drm_fd, 64, 64, DRM_FORMAT_ARGB8888, true, &cursor_fb3);
+
+	fill_igt_fb(&scanout_fb, 0xFF);
+	fill_igt_fb(&cursor_fb1, 0xFF00FFFF);
+	fill_igt_fb(&cursor_fb2, 0xFF00FF00);
+	fill_igt_fb(&cursor_fb3, 0xFFFF0000);
+
+	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());
+
+	/* Cursor 3 was created with tiling and painted with a GTT mmap, so
+	 * hopefully it has some fences around it. */
+	rc = drmModeRmFB(drm_fd, cursor_fb3.fb_id);
+	igt_assert(rc == 0);
+	gem_set_tiling(drm_fd, cursor_fb3.gem_handle, false, cursor_fb3.stride);
+	igt_assert(wait_for_suspended());
+
+	rc = drmModeSetCursor(drm_fd, crtc_id, cursor_fb3.gem_handle,
+			      cursor_fb3.width, cursor_fb3.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());
+}
+
+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;
+	bool tiling;
+
+	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;
+		tiling = true;
+		break;
+	case PLANE_PRIMARY:
+		plane_format = DRM_FORMAT_XRGB8888;
+		plane_w = mode->hdisplay;
+		plane_h = mode->vdisplay;
+		tiling = true;
+		break;
+	case PLANE_CURSOR:
+		plane_format = DRM_FORMAT_ARGB8888;
+		plane_w = 64;
+		plane_h = 64;
+		tiling = false;
+		break;
+	default:
+		igt_assert(0);
+		break;
+	}
+
+	igt_create_fb(drm_fd, plane_w, plane_h, plane_format, tiling,
+		      &plane_fb1);
+	igt_create_fb(drm_fd, plane_w, plane_h, plane_format, tiling,
+		      &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;
+
+	if (universal) {
+		rc = drmSetClientCap(drm_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES,
+				     1);
+		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);
+
+	if (universal) {
+		rc = drmSetClientCap(drm_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 0);
+		igt_assert(rc == 0);
+
+		igt_assert(planes_tested >= 3);
+	} else {
+		igt_assert(planes_tested >= 1);
+	}
+}
+
 int rounds = 50;
 bool stay = false;
 

@@ -1480,6 +1800,20 @@ int main(int argc, char *argv[])
 	igt_subtest("gem-idle")
 		gem_idle_subtest();
 
+	/* 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")
 		reg_read_ioctl_subtest();
Signature · 1 line
2.0.1
#2 lore raw
In reply to Paulo Zanoni
Quoted 77 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

If we're runtime suspended and try to use the plane 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 all the other cases, we need to get runtime PM references to pin/unpin the objects, and to change the fences. The pin/unpin functions are the ideal places for this, but intel_crtc_cursor_set_obj() doesn't call them, so we also have to add get/put calls inside it. There is no problem if we runtime suspend right after these functions are finished, because the registers weitten are forwarded to system memory.

v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
v3: - Make get/put also surround the fence and unpin calls (Daniel and
Ville).
- Merge all the plane changes into a single patch since they're the same fix.
- Add the comment requested by Daniel.

Testcase: igt/pm_rpm/cursor
Testcase: igt/pm_rpm/cursor-dpms
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-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 | 39 +++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/intel_display.c +12 -1
index 4f659eb..a86d67c 100644
@@ -2212,6 +2212,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
 	if (need_vtd_wa(dev) && alignment < 256 * 1024)
 		alignment = 256 * 1024;
 
+	/*
+	 * Global gtt pte registers are special registers which actually forward
+	 * writes to a chunk of system memory. Which means that there is no risk
+	 * that the register values disappear as soon as we call
+	 * intel_runtime_pm_put(), so it is correct to wrap only the
+	 * pin/unpin/fence and not more.
+	 */
+	intel_runtime_pm_get(dev_priv);
+
 	dev_priv->mm.interruptible = false;
 	ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
 	if (ret)

@@ -2229,21 +2238,30 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
 	i915_gem_object_pin_fence(obj);
 
 	dev_priv->mm.interruptible = true;
+	intel_runtime_pm_put(dev_priv);
 	return 0;
 
 err_unpin:
 	i915_gem_object_unpin_from_display_plane(obj);
 err_interruptible:
 	dev_priv->mm.interruptible = true;
+	intel_runtime_pm_put(dev_priv);
 	return ret;
 }
 
 void intel_unpin_fb_obj(struct drm_i915_gem_object *obj)
 {
-	WARN_ON(!mutex_is_locked(&obj->base.dev->struct_mutex));
+	struct drm_device *dev = obj->base.dev;

drivers/gpu/drm/i915/intel_display.c intel_pin_and_fence_fb_obj(struct drm_device *dev, · line 2264

Quoted 10 lines
drivers/gpu/drm/i915/intel_display.c +7 -0
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
+	intel_runtime_pm_get(dev_priv);
 
 	i915_gem_object_unpin_fence(obj);
 	i915_gem_object_unpin_from_display_plane(obj);
+
+	intel_runtime_pm_put(dev_priv);

I don't think we touch the hardware during unpin so these aren't strictly needed.

drivers/gpu/drm/i915/intel_display.c static void intel_crtc_update_cursor(struct drm_crtc *crtc, · line 8308

Quoted 10 lines
drivers/gpu/drm/i915/intel_display.c +3 -0
 }
 
 /* Computes the linear offset to the base tile and adjusts x, y. bytes per pixel

@@ -8285,6 +8303,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;
+

Did you actually manage to get by the base==0 check above with a disabled pipe? I don't think this should happen.

Quoted 11 lines
drivers/gpu/drm/i915/intel_display.c +4 -0
 	I915_WRITE(CURPOS(pipe), pos);
 
 	if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev) || IS_BROADWELL(dev))

@@ -8340,9 +8361,20 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 
 	/* we only need to pin inside GTT if cursor is non-phy */
 	mutex_lock(&dev->struct_mutex);
+
+	/*
+	 * Global gtt pte registers are special registers which actually forward
+	 * writes to a chunk of system memory. Which means that there is no risk

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

Quoted 10 lines
drivers/gpu/drm/i915/intel_display.c +7 -0
+	 * that the register values disappear as soon as we call
+	 * intel_runtime_pm_put(), so it is correct to wrap only the
+	 * pin/unpin/fence and not more.
+	 */
+	intel_runtime_pm_get(dev_priv);
+
 	if (!INTEL_INFO(dev)->cursor_needs_physical) {
 		unsigned alignment;
 
+

Spurious whitespace

And now that I look at our cursor code I see we're doing dubious stuff like unpinning the old bo before even writing the cursor registers to use the new bo (really we should wait for vblank before unpinning). But that's just something else we have to fix at some point.

Quoted 29 lines
drivers/gpu/drm/i915/intel_display.c +5 -0
 		if (obj->tiling_mode) {
 			DRM_DEBUG_KMS("cursor cannot be tiled\n");
 			ret = -EINVAL;

@@ -8392,6 +8424,10 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 
 	i915_gem_track_fb(intel_crtc->cursor_bo, obj,
 			  INTEL_FRONTBUFFER_CURSOR(pipe));
+
+	if (obj)
+		intel_runtime_pm_put(dev_priv);
+
 	mutex_unlock(&dev->struct_mutex);
 
 	old_width = intel_crtc->cursor_width;

@@ -8413,6 +8449,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 fail_unpin:
 	i915_gem_object_unpin_from_display_plane(obj);
 fail_locked:
+	intel_runtime_pm_put(dev_priv);
 	mutex_unlock(&dev->struct_mutex);
 fail:
 	drm_gem_object_unreference_unlocked(&obj->base);
Signature · 6 lines
2.0.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 2 lines
Ville Syrjälä
Intel OTC
author
#3 lore raw

2014-08-11 8:32 GMT-03:00 Ville Syrjälä <[email protected]>:

Quoted 84 lines · Paulo Zanoni
Quoted 83 lines

From: Paulo Zanoni <[email protected]>

If we're runtime suspended and try to use the plane 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 all the other cases, we need to get runtime PM references to pin/unpin the objects, and to change the fences. The pin/unpin functions are the ideal places for this, but intel_crtc_cursor_set_obj() doesn't call them, so we also have to add get/put calls inside it. There is no problem if we runtime suspend right after these functions are finished, because the registers weitten are forwarded to system memory.

v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
v3: - Make get/put also surround the fence and unpin calls (Daniel and
Ville).
- Merge all the plane changes into a single patch since they're the same fix.
- Add the comment requested by Daniel.

Testcase: igt/pm_rpm/cursor
Testcase: igt/pm_rpm/cursor-dpms
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-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 | 39 +++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/intel_display.c +17 -1
index 4f659eb..a86d67c 100644
@@ -2212,6 +2212,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
      if (need_vtd_wa(dev) && alignment < 256 * 1024)
              alignment = 256 * 1024;
 
+     /*
+      * Global gtt pte registers are special registers which actually forward
+      * writes to a chunk of system memory. Which means that there is no risk
+      * that the register values disappear as soon as we call
+      * intel_runtime_pm_put(), so it is correct to wrap only the
+      * pin/unpin/fence and not more.
+      */
+     intel_runtime_pm_get(dev_priv);
+
      dev_priv->mm.interruptible = false;
      ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
      if (ret)

@@ -2229,21 +2238,30 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
      i915_gem_object_pin_fence(obj);
 
      dev_priv->mm.interruptible = true;
+     intel_runtime_pm_put(dev_priv);
      return 0;
 
 err_unpin:
      i915_gem_object_unpin_from_display_plane(obj);
 err_interruptible:
      dev_priv->mm.interruptible = true;
+     intel_runtime_pm_put(dev_priv);
      return ret;
 }
 
 void intel_unpin_fb_obj(struct drm_i915_gem_object *obj)
 {
-     WARN_ON(!mutex_is_locked(&obj->base.dev->struct_mutex));
+     struct drm_device *dev = obj->base.dev;
+     struct drm_i915_private *dev_priv = dev->dev_private;
+
+     WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
+     intel_runtime_pm_get(dev_priv);
Quoted 8 lines

drivers/gpu/drm/i915/intel_display.c intel_pin_and_fence_fb_obj(struct drm_device *dev, · line 2264

Quoted 4 lines
drivers/gpu/drm/i915/intel_display.c +2 -0
      i915_gem_object_unpin_fence(obj);
      i915_gem_object_unpin_from_display_plane(obj);
+
+     intel_runtime_pm_put(dev_priv);

I don't think we touch the hardware during unpin so these aren't strictly needed.

Daniel requested them.

Quoted 7 lines
Quoted 7 lines
drivers/gpu/drm/i915/intel_display.c +0 -0
 }
 
 /* Computes the linear offset to the base tile and adjusts x, y. bytes per pixel

@@ -8285,6 +8303,9 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
      if (base == 0 && intel_crtc->cursor_base == 0)
              return;
Quoted 6 lines

drivers/gpu/drm/i915/intel_display.c static void intel_crtc_update_cursor(struct drm_crtc *crtc, · line 8308

Quoted 3 lines
drivers/gpu/drm/i915/intel_display.c +3 -0
+     if (!intel_crtc->active)
+             return;
+

Did you actually manage to get by the base==0 check above with a disabled pipe? I don't think this should happen.

Yes, since we enabled runtime suspend during DPMS. Remember that crtc->active != crtc->enabled.

To hit this return, just run "sudo ./pm_rpm --run-subtest cursor-dpms".

Quoted 21 lines
Quoted 21 lines
drivers/gpu/drm/i915/intel_display.c +10 -0
      I915_WRITE(CURPOS(pipe), pos);
 
      if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev) || IS_BROADWELL(dev))

@@ -8340,9 +8361,20 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 
      /* we only need to pin inside GTT if cursor is non-phy */
      mutex_lock(&dev->struct_mutex);
+
+     /*
+      * Global gtt pte registers are special registers which actually forward
+      * writes to a chunk of system memory. Which means that there is no risk
+      * that the register values disappear as soon as we call
+      * intel_runtime_pm_put(), so it is correct to wrap only the
+      * pin/unpin/fence and not more.
+      */
+     intel_runtime_pm_get(dev_priv);
+
      if (!INTEL_INFO(dev)->cursor_needs_physical) {
              unsigned alignment;
Quoted 3 lines

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

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

Spurious whitespace

Oops... Will fix.

Quoted 5 lines

And now that I look at our cursor code I see we're doing dubious stuff like unpinning the old bo before even writing the cursor registers to use the new bo (really we should wait for vblank before unpinning). But that's just something else we have to fix at some point.

Yeah, these should be in other patches.

Thanks,
Paulo

Quoted 34 lines
Quoted 26 lines
drivers/gpu/drm/i915/intel_display.c +5 -1
              if (obj->tiling_mode) {
                      DRM_DEBUG_KMS("cursor cannot be tiled\n");
                      ret = -EINVAL;

@@ -8392,6 +8424,10 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 
      i915_gem_track_fb(intel_crtc->cursor_bo, obj,
                        INTEL_FRONTBUFFER_CURSOR(pipe));
+
+     if (obj)
+             intel_runtime_pm_put(dev_priv);
+
      mutex_unlock(&dev->struct_mutex);
 
      old_width = intel_crtc->cursor_width;

@@ -8413,6 +8449,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 fail_unpin:
      i915_gem_object_unpin_from_display_plane(obj);
 fail_locked:
+     intel_runtime_pm_put(dev_priv);
      mutex_unlock(&dev->struct_mutex);
 fail:
      drm_gem_object_unreference_unlocked(&obj->base);
--

2.0.1

Quoted 4 lines

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

--
Ville Syrjälä
Intel OTC

Signature · 1 line
Paulo Zanoni
author
#4 lore raw

PATCH 1/1 drm/i915: fix plane/cursor handling when runtime suspended

From: Paulo Zanoni <[email protected]>

If we're runtime suspended and try to use the plane 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 all the other cases, we need to get runtime PM references to pin/unpin the objects, and to change the fences. The pin/unpin functions are the ideal places for this, but intel_crtc_cursor_set_obj() doesn't call them, so we also have to add get/put calls inside it. There is no problem if we runtime suspend right after these functions are finished, because the registers weitten are forwarded to system memory.

v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
v3: - Make get/put also surround the fence and unpin calls (Daniel and
Ville).
- Merge all the plane changes into a single patch since they're the same fix.
- Add the comment requested by Daniel.
v4: - Remove spurious whitespace (Ville).

Testcase: igt/pm_rpm/cursor
Testcase: igt/pm_rpm/cursor-dpms
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-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 | 38 +++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/intel_display.c +37 -1
index e9b578e..4e1c957 100644
@@ -2149,6 +2149,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
 	if (need_vtd_wa(dev) && alignment < 256 * 1024)
 		alignment = 256 * 1024;
 
+	/*
+	 * Global gtt pte registers are special registers which actually forward
+	 * writes to a chunk of system memory. Which means that there is no risk
+	 * that the register values disappear as soon as we call
+	 * intel_runtime_pm_put(), so it is correct to wrap only the
+	 * pin/unpin/fence and not more.
+	 */
+	intel_runtime_pm_get(dev_priv);
+
 	dev_priv->mm.interruptible = false;
 	ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
 	if (ret)

@@ -2166,21 +2175,30 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
 	i915_gem_object_pin_fence(obj);
 
 	dev_priv->mm.interruptible = true;
+	intel_runtime_pm_put(dev_priv);
 	return 0;
 
 err_unpin:
 	i915_gem_object_unpin_from_display_plane(obj);
 err_interruptible:
 	dev_priv->mm.interruptible = true;
+	intel_runtime_pm_put(dev_priv);
 	return ret;
 }
 
 void intel_unpin_fb_obj(struct drm_i915_gem_object *obj)
 {
-	WARN_ON(!mutex_is_locked(&obj->base.dev->struct_mutex));
+	struct drm_device *dev = obj->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
+	intel_runtime_pm_get(dev_priv);
 
 	i915_gem_object_unpin_fence(obj);
 	i915_gem_object_unpin_from_display_plane(obj);
+
+	intel_runtime_pm_put(dev_priv);
 }
 
 /* Computes the linear offset to the base tile and adjusts x, y. bytes per pixel

@@ -8154,6 +8172,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))

@@ -8209,6 +8230,16 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 
 	/* we only need to pin inside GTT if cursor is non-phy */
 	mutex_lock(&dev->struct_mutex);
+
+	/*
+	 * Global gtt pte registers are special registers which actually forward
+	 * writes to a chunk of system memory. Which means that there is no risk
+	 * that the register values disappear as soon as we call
+	 * intel_runtime_pm_put(), so it is correct to wrap only the
+	 * pin/unpin/fence and not more.
+	 */
+	intel_runtime_pm_get(dev_priv);
+
 	if (!INTEL_INFO(dev)->cursor_needs_physical) {
 		unsigned alignment;
 

@@ -8261,6 +8292,10 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 
 	i915_gem_track_fb(intel_crtc->cursor_bo, obj,
 			  INTEL_FRONTBUFFER_CURSOR(pipe));
+
+	if (obj)
+		intel_runtime_pm_put(dev_priv);
+
 	mutex_unlock(&dev->struct_mutex);
 
 	old_width = intel_crtc->cursor_width;

@@ -8282,6 +8317,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 fail_unpin:
 	i915_gem_object_unpin_from_display_plane(obj);
 fail_locked:
+	intel_runtime_pm_put(dev_priv);
 	mutex_unlock(&dev->struct_mutex);
 fail:
 	drm_gem_object_unreference_unlocked(&obj->base);
Signature · 1 line
2.0.1
#5 lore raw
In reply to Paulo Zanoni
Quoted 111 lines · Paulo Zanoni

2014-08-11 8:32 GMT-03:00 Ville Syrjälä <[email protected]>:

Quoted 84 lines · Paulo Zanoni
Quoted 83 lines

From: Paulo Zanoni <[email protected]>

If we're runtime suspended and try to use the plane 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 all the other cases, we need to get runtime PM references to pin/unpin the objects, and to change the fences. The pin/unpin functions are the ideal places for this, but intel_crtc_cursor_set_obj() doesn't call them, so we also have to add get/put calls inside it. There is no problem if we runtime suspend right after these functions are finished, because the registers weitten are forwarded to system memory.

v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
v3: - Make get/put also surround the fence and unpin calls (Daniel and
Ville).
- Merge all the plane changes into a single patch since they're the same fix.
- Add the comment requested by Daniel.

Testcase: igt/pm_rpm/cursor
Testcase: igt/pm_rpm/cursor-dpms
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-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 | 39 +++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/intel_display.c +17 -1
index 4f659eb..a86d67c 100644
@@ -2212,6 +2212,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
      if (need_vtd_wa(dev) && alignment < 256 * 1024)
              alignment = 256 * 1024;
 
+     /*
+      * Global gtt pte registers are special registers which actually forward
+      * writes to a chunk of system memory. Which means that there is no risk
+      * that the register values disappear as soon as we call
+      * intel_runtime_pm_put(), so it is correct to wrap only the
+      * pin/unpin/fence and not more.
+      */
+     intel_runtime_pm_get(dev_priv);
+
      dev_priv->mm.interruptible = false;
      ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
      if (ret)

@@ -2229,21 +2238,30 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
      i915_gem_object_pin_fence(obj);
 
      dev_priv->mm.interruptible = true;
+     intel_runtime_pm_put(dev_priv);
      return 0;
 
 err_unpin:
      i915_gem_object_unpin_from_display_plane(obj);
 err_interruptible:
      dev_priv->mm.interruptible = true;
+     intel_runtime_pm_put(dev_priv);
      return ret;
 }
 
 void intel_unpin_fb_obj(struct drm_i915_gem_object *obj)
 {
-     WARN_ON(!mutex_is_locked(&obj->base.dev->struct_mutex));
+     struct drm_device *dev = obj->base.dev;
+     struct drm_i915_private *dev_priv = dev->dev_private;
+
+     WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
+     intel_runtime_pm_get(dev_priv);
Quoted 8 lines

drivers/gpu/drm/i915/intel_display.c intel_pin_and_fence_fb_obj(struct drm_device *dev, · line 2264

Quoted 4 lines
drivers/gpu/drm/i915/intel_display.c +2 -0
      i915_gem_object_unpin_fence(obj);
      i915_gem_object_unpin_from_display_plane(obj);
+
+     intel_runtime_pm_put(dev_priv);

I don't think we touch the hardware during unpin so these aren't strictly needed.

Daniel requested them.

Quoted 7 lines
Quoted 7 lines
drivers/gpu/drm/i915/intel_display.c +0 -0
 }
 
 /* Computes the linear offset to the base tile and adjusts x, y. bytes per pixel

@@ -8285,6 +8303,9 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
      if (base == 0 && intel_crtc->cursor_base == 0)
              return;
Quoted 6 lines

drivers/gpu/drm/i915/intel_display.c static void intel_crtc_update_cursor(struct drm_crtc *crtc, · line 8308

Quoted 3 lines
drivers/gpu/drm/i915/intel_display.c +3 -0
+     if (!intel_crtc->active)
+             return;
+

Did you actually manage to get by the base==0 check above with a disabled pipe? I don't think this should happen.

Quoted 2 lines

Yes, since we enabled runtime suspend during DPMS. Remember that crtc->active != crtc->enabled.

Then I think there's a bug somewhere a bit earlier. We should consider the cursor to be invisible when crtc->active==false. That's how we deal with all other planes.

Signature · 2 lines
Ville Syrjälä
Intel OTC
author
#6 lore raw

2014-08-11 11:42 GMT-03:00 Ville Syrjälä <[email protected]>:

Quoted 112 lines · Paulo Zanoni
Quoted 111 lines

2014-08-11 8:32 GMT-03:00 Ville Syrjälä <[email protected]>:

Quoted 84 lines · Paulo Zanoni
Quoted 83 lines

From: Paulo Zanoni <[email protected]>

If we're runtime suspended and try to use the plane 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 all the other cases, we need to get runtime PM references to pin/unpin the objects, and to change the fences. The pin/unpin functions are the ideal places for this, but intel_crtc_cursor_set_obj() doesn't call them, so we also have to add get/put calls inside it. There is no problem if we runtime suspend right after these functions are finished, because the registers weitten are forwarded to system memory.

v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
v3: - Make get/put also surround the fence and unpin calls (Daniel and
Ville).
- Merge all the plane changes into a single patch since they're the same fix.
- Add the comment requested by Daniel.

Testcase: igt/pm_rpm/cursor
Testcase: igt/pm_rpm/cursor-dpms
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-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 | 39 +++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/intel_display.c +17 -1
index 4f659eb..a86d67c 100644
@@ -2212,6 +2212,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
      if (need_vtd_wa(dev) && alignment < 256 * 1024)
              alignment = 256 * 1024;
 
+     /*
+      * Global gtt pte registers are special registers which actually forward
+      * writes to a chunk of system memory. Which means that there is no risk
+      * that the register values disappear as soon as we call
+      * intel_runtime_pm_put(), so it is correct to wrap only the
+      * pin/unpin/fence and not more.
+      */
+     intel_runtime_pm_get(dev_priv);
+
      dev_priv->mm.interruptible = false;
      ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
      if (ret)

@@ -2229,21 +2238,30 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
      i915_gem_object_pin_fence(obj);
 
      dev_priv->mm.interruptible = true;
+     intel_runtime_pm_put(dev_priv);
      return 0;
 
 err_unpin:
      i915_gem_object_unpin_from_display_plane(obj);
 err_interruptible:
      dev_priv->mm.interruptible = true;
+     intel_runtime_pm_put(dev_priv);
      return ret;
 }
 
 void intel_unpin_fb_obj(struct drm_i915_gem_object *obj)
 {
-     WARN_ON(!mutex_is_locked(&obj->base.dev->struct_mutex));
+     struct drm_device *dev = obj->base.dev;
+     struct drm_i915_private *dev_priv = dev->dev_private;
+
+     WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
+     intel_runtime_pm_get(dev_priv);
Quoted 8 lines

drivers/gpu/drm/i915/intel_display.c intel_pin_and_fence_fb_obj(struct drm_device *dev, · line 2264

Quoted 4 lines
drivers/gpu/drm/i915/intel_display.c +2 -0
      i915_gem_object_unpin_fence(obj);
      i915_gem_object_unpin_from_display_plane(obj);
+
+     intel_runtime_pm_put(dev_priv);

I don't think we touch the hardware during unpin so these aren't strictly needed.

Daniel requested them.

Quoted 13 lines

drivers/gpu/drm/i915/intel_display.c static void intel_crtc_update_cursor(struct drm_crtc *crtc, · line 8308

Quoted 10 lines
drivers/gpu/drm/i915/intel_display.c +3 -0
 }
 
 /* Computes the linear offset to the base tile and adjusts x, y. bytes per pixel

@@ -8285,6 +8303,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;
+

Did you actually manage to get by the base==0 check above with a disabled pipe? I don't think this should happen.

Quoted 6 lines
Quoted 2 lines

Yes, since we enabled runtime suspend during DPMS. Remember that crtc->active != crtc->enabled.

Then I think there's a bug somewhere a bit earlier. We should consider the cursor to be invisible when crtc->active==false. That's how we deal with all other planes.

Why? I am not very familiar with the cursor code and I don't see what's the problem here. Please explain more: what exactly is the problem, where is it and what is your suggested solution? Also, notice that this is a patch to -stable, so I don't think we should block this fix based on complicated rework ideas, or something that won't really apply to kernels older than 5 hours.

When I look at the backtrace, I see that we are already calling these functions through drm_mode_cursor_universal(), so this should already be handling the cursor plane in the same way it handles the other planes.

Also, I invoke Daniel to discuss here since he's the one that introduced the difference between ->active and ->enabled.

Quoted 4 lines

--
Ville Syrjälä
Intel OTC

Signature · 1 line
Paulo Zanoni
#7 lore raw
Quoted 116 lines · Paulo Zanoni

2014-08-11 11:42 GMT-03:00 Ville Syrjälä <[email protected]>:

Quoted 115 lines · Paulo Zanoni
Quoted 113 lines

2014-08-11 8:32 GMT-03:00 Ville Syrjälä <[email protected]>:

Quoted 84 lines · Paulo Zanoni
Quoted 83 lines

From: Paulo Zanoni <[email protected]>

If we're runtime suspended and try to use the plane 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 all the other cases, we need to get runtime PM references to pin/unpin the objects, and to change the fences. The pin/unpin functions are the ideal places for this, but intel_crtc_cursor_set_obj() doesn't call them, so we also have to add get/put calls inside it. There is no problem if we runtime suspend right after these functions are finished, because the registers weitten are forwarded to system memory.

v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
v3: - Make get/put also surround the fence and unpin calls (Daniel and
Ville).
- Merge all the plane changes into a single patch since they're the same fix.
- Add the comment requested by Daniel.

Testcase: igt/pm_rpm/cursor
Testcase: igt/pm_rpm/cursor-dpms
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-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 | 39 +++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/intel_display.c +17 -1
index 4f659eb..a86d67c 100644
@@ -2212,6 +2212,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
      if (need_vtd_wa(dev) && alignment < 256 * 1024)
              alignment = 256 * 1024;
 
+     /*
+      * Global gtt pte registers are special registers which actually forward
+      * writes to a chunk of system memory. Which means that there is no risk
+      * that the register values disappear as soon as we call
+      * intel_runtime_pm_put(), so it is correct to wrap only the
+      * pin/unpin/fence and not more.
+      */
+     intel_runtime_pm_get(dev_priv);
+
      dev_priv->mm.interruptible = false;
      ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
      if (ret)

@@ -2229,21 +2238,30 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
      i915_gem_object_pin_fence(obj);
 
      dev_priv->mm.interruptible = true;
+     intel_runtime_pm_put(dev_priv);
      return 0;
 
 err_unpin:
      i915_gem_object_unpin_from_display_plane(obj);
 err_interruptible:
      dev_priv->mm.interruptible = true;
+     intel_runtime_pm_put(dev_priv);
      return ret;
 }
 
 void intel_unpin_fb_obj(struct drm_i915_gem_object *obj)
 {
-     WARN_ON(!mutex_is_locked(&obj->base.dev->struct_mutex));
+     struct drm_device *dev = obj->base.dev;
+     struct drm_i915_private *dev_priv = dev->dev_private;
+
+     WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
+     intel_runtime_pm_get(dev_priv);
Quoted 8 lines

drivers/gpu/drm/i915/intel_display.c intel_pin_and_fence_fb_obj(struct drm_device *dev, · line 2264

Quoted 4 lines
drivers/gpu/drm/i915/intel_display.c +2 -0
      i915_gem_object_unpin_fence(obj);
      i915_gem_object_unpin_from_display_plane(obj);
+
+     intel_runtime_pm_put(dev_priv);

I don't think we touch the hardware during unpin so these aren't strictly needed.

Daniel requested them.

Quoted 7 lines
Quoted 7 lines
drivers/gpu/drm/i915/intel_display.c +0 -0
 }
 
 /* Computes the linear offset to the base tile and adjusts x, y. bytes per pixel

@@ -8285,6 +8303,9 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
      if (base == 0 && intel_crtc->cursor_base == 0)
              return;
Quoted 6 lines

drivers/gpu/drm/i915/intel_display.c static void intel_crtc_update_cursor(struct drm_crtc *crtc, · line 8308

Quoted 3 lines
drivers/gpu/drm/i915/intel_display.c +3 -0
+     if (!intel_crtc->active)
+             return;
+

Did you actually manage to get by the base==0 check above with a disabled pipe? I don't think this should happen.

Yes, since we enabled runtime suspend during DPMS. Remember that crtc->active != crtc->enabled.

Quoted 6 lines
Quoted 3 lines

Then I think there's a bug somewhere a bit earlier. We should consider the cursor to be invisible when crtc->active==false. That's how we deal with all other planes.

Why? I am not very familiar with the cursor code and I don't see what's the problem here.

Just feels like duct tape for something that should have been caught by some earlier piece of code.

Quoted 2 lines

Please explain more: what exactly is the problem, where is it and what is your suggested solution?

I think this ought to fix it, and is exactly what we do with other planes:

drivers/gpu/drm/i915/intel_display.c +2 -2
index e9b578e..c9f733d 100644
@@ -11699,8 +11699,8 @@ intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
        };
        const struct drm_rect clip = {
                /* integer pixels */
-               .x2 = intel_crtc->config.pipe_src_w,
-               .y2 = intel_crtc->config.pipe_src_h,
+               .x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0,
+               .y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0,
        };
        bool visible;
        int ret;
Quoted 22 lines

Also, notice
that this is a patch to -stable, so I don't think we should block this fix based on complicated rework ideas, or something that won't really apply to kernels older than 5 hours.

When I look at the backtrace, I see that we are already calling these functions through drm_mode_cursor_universal(), so this should already be handling the cursor plane in the same way it handles the other planes.

Also, I invoke Daniel to discuss here since he's the one that introduced the difference between ->active and ->enabled.

Quoted 4 lines

--
Ville Syrjälä
Intel OTC

Signature · 1 line
Paulo Zanoni
Signature · 2 lines
Ville Syrjälä
Intel OTC
author
#8 lore raw

PATCH 1/1 drm/i915: fix plane/cursor handling when runtime suspended

From: Paulo Zanoni <[email protected]>

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

We need to get runtime PM references to pin/unpin the objects, and to change the fences. The pin/unpin functions are the ideal places for this, but intel_crtc_cursor_set_obj() doesn't call them, so we also have to add get/put calls inside it. There is no problem if we runtime suspend right after these functions are finished, because the registers written are forwarded to system memory.

Note: for a complete fix of the cursor-dpms test case, we also need the patch named "drm/i915: Don't try to enable cursor from setplane when crtc is disabled".

v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
v3: - Make get/put also surround the fence and unpin calls (Daniel and
Ville).
- Merge all the plane changes into a single patch since they're the same fix.
- Add the comment requested by Daniel.
v4: - Remove spurious whitespace (Ville).
v5: - Remove intel_crtc_update_cursor() chunk since Ville did an
equivalent fix in another patch (Ville).

Testcase: igt/pm_rpm/cursor
Testcase: igt/pm_rpm/cursor-dpms
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-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 | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/intel_display.c +34 -1
index a1cf052..2db9e06 100644
@@ -2149,6 +2149,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
 	if (need_vtd_wa(dev) && alignment < 256 * 1024)
 		alignment = 256 * 1024;
 
+	/*
+	 * Global gtt pte registers are special registers which actually forward
+	 * writes to a chunk of system memory. Which means that there is no risk
+	 * that the register values disappear as soon as we call
+	 * intel_runtime_pm_put(), so it is correct to wrap only the
+	 * pin/unpin/fence and not more.
+	 */
+	intel_runtime_pm_get(dev_priv);
+
 	dev_priv->mm.interruptible = false;
 	ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
 	if (ret)

@@ -2166,21 +2175,30 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
 	i915_gem_object_pin_fence(obj);
 
 	dev_priv->mm.interruptible = true;
+	intel_runtime_pm_put(dev_priv);
 	return 0;
 
 err_unpin:
 	i915_gem_object_unpin_from_display_plane(obj);
 err_interruptible:
 	dev_priv->mm.interruptible = true;
+	intel_runtime_pm_put(dev_priv);
 	return ret;
 }
 
 void intel_unpin_fb_obj(struct drm_i915_gem_object *obj)
 {
-	WARN_ON(!mutex_is_locked(&obj->base.dev->struct_mutex));
+	struct drm_device *dev = obj->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
+	intel_runtime_pm_get(dev_priv);
 
 	i915_gem_object_unpin_fence(obj);
 	i915_gem_object_unpin_from_display_plane(obj);
+
+	intel_runtime_pm_put(dev_priv);
 }
 
 /* Computes the linear offset to the base tile and adjusts x, y. bytes per pixel

@@ -8170,6 +8188,16 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 
 	/* we only need to pin inside GTT if cursor is non-phy */
 	mutex_lock(&dev->struct_mutex);
+
+	/*
+	 * Global gtt pte registers are special registers which actually forward
+	 * writes to a chunk of system memory. Which means that there is no risk
+	 * that the register values disappear as soon as we call
+	 * intel_runtime_pm_put(), so it is correct to wrap only the
+	 * pin/unpin/fence and not more.
+	 */
+	intel_runtime_pm_get(dev_priv);
+
 	if (!INTEL_INFO(dev)->cursor_needs_physical) {
 		unsigned alignment;
 

@@ -8219,6 +8247,10 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 
 	i915_gem_track_fb(intel_crtc->cursor_bo, obj,
 			  INTEL_FRONTBUFFER_CURSOR(pipe));
+
+	if (obj)
+		intel_runtime_pm_put(dev_priv);
+
 	mutex_unlock(&dev->struct_mutex);
 
 	old_width = intel_crtc->cursor_width;

@@ -8240,6 +8272,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 fail_unpin:
 	i915_gem_object_unpin_from_display_plane(obj);
 fail_locked:
+	intel_runtime_pm_put(dev_priv);
 	mutex_unlock(&dev->struct_mutex);
 fail:
 	drm_gem_object_unreference_unlocked(&obj->base);
Signature · 1 line
2.0.1
#9 lore raw
Quoted 79 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

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

We need to get runtime PM references to pin/unpin the objects, and to change the fences. The pin/unpin functions are the ideal places for this, but intel_crtc_cursor_set_obj() doesn't call them, so we also have to add get/put calls inside it. There is no problem if we runtime suspend right after these functions are finished, because the registers written are forwarded to system memory.

Note: for a complete fix of the cursor-dpms test case, we also need the patch named "drm/i915: Don't try to enable cursor from setplane when crtc is disabled".

v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
v3: - Make get/put also surround the fence and unpin calls (Daniel and
Ville).
- Merge all the plane changes into a single patch since they're the same fix.
- Add the comment requested by Daniel.
v4: - Remove spurious whitespace (Ville).
v5: - Remove intel_crtc_update_cursor() chunk since Ville did an
equivalent fix in another patch (Ville).

Testcase: igt/pm_rpm/cursor
Testcase: igt/pm_rpm/cursor-dpms
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-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 | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/intel_display.c +13 -1
index a1cf052..2db9e06 100644
@@ -2149,6 +2149,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
 	if (need_vtd_wa(dev) && alignment < 256 * 1024)
 		alignment = 256 * 1024;
 
+	/*
+	 * Global gtt pte registers are special registers which actually forward
+	 * writes to a chunk of system memory. Which means that there is no risk
+	 * that the register values disappear as soon as we call
+	 * intel_runtime_pm_put(), so it is correct to wrap only the
+	 * pin/unpin/fence and not more.
+	 */
+	intel_runtime_pm_get(dev_priv);
+
 	dev_priv->mm.interruptible = false;
 	ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
 	if (ret)

@@ -2166,21 +2175,30 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
 	i915_gem_object_pin_fence(obj);
 
 	dev_priv->mm.interruptible = true;
+	intel_runtime_pm_put(dev_priv);
 	return 0;
 
 err_unpin:
 	i915_gem_object_unpin_from_display_plane(obj);
 err_interruptible:
 	dev_priv->mm.interruptible = true;
+	intel_runtime_pm_put(dev_priv);
 	return ret;
 }
 
 void intel_unpin_fb_obj(struct drm_i915_gem_object *obj)
 {
-	WARN_ON(!mutex_is_locked(&obj->base.dev->struct_mutex));
+	struct drm_device *dev = obj->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;

drivers/gpu/drm/i915/intel_display.c intel_pin_and_fence_fb_obj(struct drm_device *dev, · line 2202

Quoted 10 lines
drivers/gpu/drm/i915/intel_display.c +6 -0
+
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
+	intel_runtime_pm_get(dev_priv);
 
 	i915_gem_object_unpin_fence(obj);
 	i915_gem_object_unpin_from_display_plane(obj);
+
+	intel_runtime_pm_put(dev_priv);
 }

framebuffer objects are pinned for a very long time, and the fbcon is permanently pinned. This should have the effect of disabling rpm entirely.
-Chris

Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
author
#10 lore raw

2014-08-12 16:09 GMT-03:00 Chris Wilson <[email protected]>:

Quoted 85 lines · Paulo Zanoni
Quoted 84 lines

From: Paulo Zanoni <[email protected]>

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

We need to get runtime PM references to pin/unpin the objects, and to change the fences. The pin/unpin functions are the ideal places for this, but intel_crtc_cursor_set_obj() doesn't call them, so we also have to add get/put calls inside it. There is no problem if we runtime suspend right after these functions are finished, because the registers written are forwarded to system memory.

Note: for a complete fix of the cursor-dpms test case, we also need the patch named "drm/i915: Don't try to enable cursor from setplane when crtc is disabled".

v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
v3: - Make get/put also surround the fence and unpin calls (Daniel and
Ville).
- Merge all the plane changes into a single patch since they're the same fix.
- Add the comment requested by Daniel.
v4: - Remove spurious whitespace (Ville).
v5: - Remove intel_crtc_update_cursor() chunk since Ville did an
equivalent fix in another patch (Ville).

Testcase: igt/pm_rpm/cursor
Testcase: igt/pm_rpm/cursor-dpms
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-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 | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/intel_display.c +17 -1
index a1cf052..2db9e06 100644
@@ -2149,6 +2149,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
      if (need_vtd_wa(dev) && alignment < 256 * 1024)
              alignment = 256 * 1024;
 
+     /*
+      * Global gtt pte registers are special registers which actually forward
+      * writes to a chunk of system memory. Which means that there is no risk
+      * that the register values disappear as soon as we call
+      * intel_runtime_pm_put(), so it is correct to wrap only the
+      * pin/unpin/fence and not more.
+      */
+     intel_runtime_pm_get(dev_priv);
+
      dev_priv->mm.interruptible = false;
      ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
      if (ret)

@@ -2166,21 +2175,30 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
      i915_gem_object_pin_fence(obj);
 
      dev_priv->mm.interruptible = true;
+     intel_runtime_pm_put(dev_priv);
      return 0;
 
 err_unpin:
      i915_gem_object_unpin_from_display_plane(obj);
 err_interruptible:
      dev_priv->mm.interruptible = true;
+     intel_runtime_pm_put(dev_priv);
      return ret;
 }
 
 void intel_unpin_fb_obj(struct drm_i915_gem_object *obj)
 {
-     WARN_ON(!mutex_is_locked(&obj->base.dev->struct_mutex));
+     struct drm_device *dev = obj->base.dev;
+     struct drm_i915_private *dev_priv = dev->dev_private;
+
+     WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
+     intel_runtime_pm_get(dev_priv);
Quoted 9 lines

drivers/gpu/drm/i915/intel_display.c intel_pin_and_fence_fb_obj(struct drm_device *dev, · line 2202

Quoted 5 lines
drivers/gpu/drm/i915/intel_display.c +2 -0
      i915_gem_object_unpin_fence(obj);
      i915_gem_object_unpin_from_display_plane(obj);
+
+     intel_runtime_pm_put(dev_priv);
 }

framebuffer objects are pinned for a very long time, and the fbcon is permanently pinned. This should have the effect of disabling rpm entirely.

But we just get/put RPM around this function, not for the whole time while the object is pinned.

Quoted 4 lines

-Chris

--
Chris Wilson, Intel Open Source Technology Centre

Signature · 1 line
Paulo Zanoni
#11 lore raw
Quoted 2 lines · Paulo Zanoni

But we just get/put RPM around this function, not for the whole time while the object is pinned.

Ah misread, saw pin->get, unpin->put and assumed the symmetry. But why unpin then? It doesn't touch any hardware registers.
-Chris

Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
author
#12 lore raw

2014-08-12 16:28 GMT-03:00 Chris Wilson <[email protected]>:

Quoted 6 lines · Paulo Zanoni
Quoted 2 lines

But we just get/put RPM around this function, not for the whole time while the object is pinned.

Ah misread, saw pin->get, unpin->put and assumed the symmetry. But why unpin then? It doesn't touch any hardware registers.

Only because Daniel asked it on a conversation we had on IRC, and I automatically assumed the patch would be rejected if I didn't include it :)

Since both you and VIlle pointed that out, I should probably submit yet another version, without the unpin part, and let Daniel choose which one to merge...

Quoted 4 lines

-Chris

--
Chris Wilson, Intel Open Source Technology Centre

Signature · 1 line
Paulo Zanoni
#13 lore raw
Quoted 8 lines · Paulo Zanoni

2014-08-12 16:28 GMT-03:00 Chris Wilson <[email protected]>:

Quoted 6 lines · Paulo Zanoni
Quoted 2 lines

But we just get/put RPM around this function, not for the whole time while the object is pinned.

Ah misread, saw pin->get, unpin->put and assumed the symmetry. But why unpin then? It doesn't touch any hardware registers.

Quoted 7 lines

Only because Daniel asked it on a conversation we had on IRC, and I automatically assumed the patch would be rejected if I didn't include it :)

Since both you and VIlle pointed that out, I should probably submit yet another version, without the unpin part, and let Daniel choose which one to merge...

Hm, I've indeed forgotten about the lazy unbinding. But that poses the question about the final bo unref. For example:
1) create bo, gtt mmap it to force it into existence (and into the global gtt)
2) unmap binding
3) wait for rpm entry
4) unref bo ... causing pte writes for the global gtt unbinding while runtime suspended or not?

boom or not boom?

Maybe the bug is simply in a different function ;-) -Daniel

Signature · 3 lines
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
#14 lore raw
Quoted 25 lines · Daniel Vetter
Quoted 8 lines · Paulo Zanoni

2014-08-12 16:28 GMT-03:00 Chris Wilson <[email protected]>:

Quoted 6 lines · Paulo Zanoni
Quoted 2 lines

But we just get/put RPM around this function, not for the whole time while the object is pinned.

Ah misread, saw pin->get, unpin->put and assumed the symmetry. But why unpin then? It doesn't touch any hardware registers.

Quoted 7 lines

Only because Daniel asked it on a conversation we had on IRC, and I automatically assumed the patch would be rejected if I didn't include it :)

Since both you and VIlle pointed that out, I should probably submit yet another version, without the unpin part, and let Daniel choose which one to merge...

Hm, I've indeed forgotten about the lazy unbinding. But that poses the question about the final bo unref. For example:
1) create bo, gtt mmap it to force it into existence (and into the global gtt)
2) unmap binding
3) wait for rpm entry
4) unref bo ... causing pte writes for the global gtt unbinding while runtime suspended or not?

Quoted 3 lines

boom or not boom?

Maybe the bug is simply in a different function ;-)

Yes. If you get serious about it, you will want to move the lazy stuff into its own workqueue to be run the next time the device is awake. -Chris

Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
#15 lore raw
Quoted 26 lines · Chris Wilson
Quoted 25 lines · Daniel Vetter
Quoted 8 lines · Paulo Zanoni

2014-08-12 16:28 GMT-03:00 Chris Wilson <[email protected]>:

Quoted 6 lines · Paulo Zanoni
Quoted 2 lines

But we just get/put RPM around this function, not for the whole time while the object is pinned.

Ah misread, saw pin->get, unpin->put and assumed the symmetry. But why unpin then? It doesn't touch any hardware registers.

Quoted 7 lines

Only because Daniel asked it on a conversation we had on IRC, and I automatically assumed the patch would be rejected if I didn't include it :)

Since both you and VIlle pointed that out, I should probably submit yet another version, without the unpin part, and let Daniel choose which one to merge...

Hm, I've indeed forgotten about the lazy unbinding. But that poses the question about the final bo unref. For example:
1) create bo, gtt mmap it to force it into existence (and into the global gtt)
2) unmap binding
3) wait for rpm entry
4) unref bo ... causing pte writes for the global gtt unbinding while runtime suspended or not?

Quoted 6 lines
Quoted 3 lines

boom or not boom?

Maybe the bug is simply in a different function ;-)

Yes. If you get serious about it, you will want to move the lazy stuff into its own workqueue to be run the next time the device is awake.

4b) shrinker happens and unbinds (potentially purgeable) buffer objects.

In that case I don't think the core mm would be happy if we'd indefinitely delay this until someone wiggles the mouse. Especially if the compositor wants that memory to render the frame it needs to switch everything on again ...
-Daniel

Signature · 3 lines
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
#16 lore raw
Quoted 31 lines · Daniel Vetter
Quoted 30 lines · Chris Wilson
Quoted 28 lines · Daniel Vetter
Quoted 8 lines · Paulo Zanoni

2014-08-12 16:28 GMT-03:00 Chris Wilson <[email protected]>:

Quoted 6 lines · Paulo Zanoni
Quoted 2 lines

But we just get/put RPM around this function, not for the whole time while the object is pinned.

Ah misread, saw pin->get, unpin->put and assumed the symmetry. But why unpin then? It doesn't touch any hardware registers.

Quoted 7 lines

Only because Daniel asked it on a conversation we had on IRC, and I automatically assumed the patch would be rejected if I didn't include it :)

Since both you and VIlle pointed that out, I should probably submit yet another version, without the unpin part, and let Daniel choose which one to merge...

Hm, I've indeed forgotten about the lazy unbinding. But that poses the question about the final bo unref. For example:
1) create bo, gtt mmap it to force it into existence (and into the global gtt)
2) unmap binding
3) wait for rpm entry
4) unref bo ... causing pte writes for the global gtt unbinding while runtime suspended or not?

boom or not boom?

Maybe the bug is simply in a different function ;-)

Quoted 7 lines
Quoted 2 lines

Yes. If you get serious about it, you will want to move the lazy stuff into its own workqueue to be run the next time the device is awake.

4b) shrinker happens and unbinds (potentially purgeable) buffer objects.

In that case I don't think the core mm would be happy if we'd indefinitely delay this until someone wiggles the mouse.

You underestimate just how much we can delay it ;-) But for your next trick, you could unbind the buffer without touching the ptes since the gpu is not using those pages... Diminishing returns I guess.

Quoted 3 lines

Especially if
the compositor wants that memory to render the frame it needs to switch everything on again ...

But's true without rpm anyway. It would need to enable the device to render, whether or not the system is thrashing.
-Chris

Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
#17 lore raw
Quoted 35 lines · Chris Wilson
Quoted 34 lines · Daniel Vetter
Quoted 32 lines · Chris Wilson
Quoted 25 lines · Daniel Vetter
Quoted 8 lines · Paulo Zanoni

2014-08-12 16:28 GMT-03:00 Chris Wilson <[email protected]>:

Quoted 6 lines · Paulo Zanoni
Quoted 2 lines

But we just get/put RPM around this function, not for the whole time while the object is pinned.

Ah misread, saw pin->get, unpin->put and assumed the symmetry. But why unpin then? It doesn't touch any hardware registers.

Quoted 7 lines

Only because Daniel asked it on a conversation we had on IRC, and I automatically assumed the patch would be rejected if I didn't include it :)

Since both you and VIlle pointed that out, I should probably submit yet another version, without the unpin part, and let Daniel choose which one to merge...

Hm, I've indeed forgotten about the lazy unbinding. But that poses the question about the final bo unref. For example:
1) create bo, gtt mmap it to force it into existence (and into the global gtt)
2) unmap binding
3) wait for rpm entry
4) unref bo ... causing pte writes for the global gtt unbinding while runtime suspended or not?

Quoted 3 lines

boom or not boom?

Maybe the bug is simply in a different function ;-)

Yes. If you get serious about it, you will want to move the lazy stuff into its own workqueue to be run the next time the device is awake.

Quoted 8 lines
Quoted 4 lines

4b) shrinker happens and unbinds (potentially purgeable) buffer objects.

In that case I don't think the core mm would be happy if we'd indefinitely delay this until someone wiggles the mouse.

You underestimate just how much we can delay it ;-) But for your next trick, you could unbind the buffer without touching the ptes since the gpu is not using those pages... Diminishing returns I guess.

That's actually something I've considered - on gen6+ we don't use the global gtt any more for rendering, so it's fully isolated from whatever userspace can get at. Well ignoring an icky regression from full ppgtt for the aliasing ppgtt binding rules.

So we /could/ just leave the stale pte hanging in the air forever. But I'm not sure whether we want to do that for general robustness reasons. Clearing all ptes on device wake-up isn't an option since it takes too much time, and delaying the clearing doesn't look like worth it from a complexity pov.

Quoted 6 lines
Quoted 3 lines

Especially if
the compositor wants that memory to render the frame it needs to switch everything on again ...

But's true without rpm anyway. It would need to enable the device to render, whether or not the system is thrashing.

Yeah, which is also why I don't think just waking the device in the shrinker/bo_free callback is harmful - very likely we didn't wake it for nothing anyway. Oh any my scenario can be fixed with some software rendering into an Xshm or so, and if you assume something else running overnight has thrashed all the memory to swap.
-Daniel

Signature · 3 lines
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
author
#18 lore raw

PATCH 1/1 drm/i915: fix plane/cursor handling when runtime suspended

From: Paulo Zanoni <[email protected]>

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

We need to get runtime PM references to pin the objects, and to change the fences. The pin functions are the ideal places for this, but intel_crtc_cursor_set_obj() doesn't call them, so we also have to add get/put calls inside it. There is no problem if we runtime suspend right after these functions are finished, because the registers written are forwarded to system memory.

Note: for a complete fix of the cursor-dpms test case, we also need the patch named "drm/i915: Don't try to enable cursor from setplane when crtc is disabled".

v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
v3: - Make get/put also surround the fence and unpin calls (Daniel and
Ville).
- Merge all the plane changes into a single patch since they're the same fix.
- Add the comment requested by Daniel.
v4: - Remove spurious whitespace (Ville).
v5: - Remove intel_crtc_update_cursor() chunk since Ville did an
equivalent fix in another patch (Ville).
v6: - Remove unpin chunk: it will be on a separate patch (Ville,
Chris, Daniel).

Testcase: igt/pm_rpm/cursor
Testcase: igt/pm_rpm/cursor-dpms
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-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 | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

I talked with Daniel and we agreed to leave any possible fixes related with the "unpin" functions to separate patches, possibly requiring separate IGT test cases.

drivers/gpu/drm/i915/intel_display.c +27 -0
index 3813526..17bc661 100644
@@ -2149,6 +2149,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
 	if (need_vtd_wa(dev) && alignment < 256 * 1024)
 		alignment = 256 * 1024;
 
+	/*
+	 * Global gtt pte registers are special registers which actually forward
+	 * writes to a chunk of system memory. Which means that there is no risk
+	 * that the register values disappear as soon as we call
+	 * intel_runtime_pm_put(), so it is correct to wrap only the
+	 * pin/unpin/fence and not more.
+	 */
+	intel_runtime_pm_get(dev_priv);
+
 	dev_priv->mm.interruptible = false;
 	ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
 	if (ret)

@@ -2166,12 +2175,14 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
 	i915_gem_object_pin_fence(obj);
 
 	dev_priv->mm.interruptible = true;
+	intel_runtime_pm_put(dev_priv);
 	return 0;
 
 err_unpin:
 	i915_gem_object_unpin_from_display_plane(obj);
 err_interruptible:
 	dev_priv->mm.interruptible = true;
+	intel_runtime_pm_put(dev_priv);
 	return ret;
 }
 

@@ -8201,6 +8212,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 				     uint32_t width, uint32_t height)
 {
 	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	enum pipe pipe = intel_crtc->pipe;
 	unsigned old_width, stride;

@@ -8231,6 +8243,16 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 
 	/* we only need to pin inside GTT if cursor is non-phy */
 	mutex_lock(&dev->struct_mutex);
+
+	/*
+	 * Global gtt pte registers are special registers which actually forward
+	 * writes to a chunk of system memory. Which means that there is no risk
+	 * that the register values disappear as soon as we call
+	 * intel_runtime_pm_put(), so it is correct to wrap only the
+	 * pin/unpin/fence and not more.
+	 */
+	intel_runtime_pm_get(dev_priv);
+
 	if (!INTEL_INFO(dev)->cursor_needs_physical) {
 		unsigned alignment;
 

@@ -8280,6 +8302,10 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 
 	i915_gem_track_fb(intel_crtc->cursor_bo, obj,
 			  INTEL_FRONTBUFFER_CURSOR(pipe));
+
+	if (obj)
+		intel_runtime_pm_put(dev_priv);
+
 	mutex_unlock(&dev->struct_mutex);
 
 	old_width = intel_crtc->cursor_width;

@@ -8301,6 +8327,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 fail_unpin:
 	i915_gem_object_unpin_from_display_plane(obj);
 fail_locked:
+	intel_runtime_pm_put(dev_priv);
 	mutex_unlock(&dev->struct_mutex);
 fail:
 	drm_gem_object_unreference_unlocked(&obj->base);
Signature · 1 line
2.0.1
author
#19 lore raw

2014-08-14 12:06 GMT-03:00 Paulo Zanoni <[email protected]>:

Quoted 28 lines

From: Paulo Zanoni <[email protected]>

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

We need to get runtime PM references to pin the objects, and to change the fences. The pin functions are the ideal places for this, but intel_crtc_cursor_set_obj() doesn't call them, so we also have to add get/put calls inside it. There is no problem if we runtime suspend right after these functions are finished, because the registers written are forwarded to system memory.

Note: for a complete fix of the cursor-dpms test case, we also need the patch named "drm/i915: Don't try to enable cursor from setplane when crtc is disabled".

v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
v3: - Make get/put also surround the fence and unpin calls (Daniel and
Ville).
- Merge all the plane changes into a single patch since they're the same fix.
- Add the comment requested by Daniel.
v4: - Remove spurious whitespace (Ville).
v5: - Remove intel_crtc_update_cursor() chunk since Ville did an
equivalent fix in another patch (Ville).
v6: - Remove unpin chunk: it will be on a separate patch (Ville,
Chris, Daniel).

Quoted 7 lines

Testcase: igt/pm_rpm/cursor
Testcase: igt/pm_rpm/cursor-dpms
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-dpms
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81645

And now we also have a report from QA:
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82603

Quoted 93 lines
Cc [email protected]
Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

I talked with Daniel and we agreed to leave any possible fixes related with the "unpin" functions to separate patches, possibly requiring separate IGT test cases.

drivers/gpu/drm/i915/intel_display.c +27 -1
index 3813526..17bc661 100644
@@ -2149,6 +2149,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
        if (need_vtd_wa(dev) && alignment < 256 * 1024)
                alignment = 256 * 1024;
 
+       /*
+        * Global gtt pte registers are special registers which actually forward
+        * writes to a chunk of system memory. Which means that there is no risk
+        * that the register values disappear as soon as we call
+        * intel_runtime_pm_put(), so it is correct to wrap only the
+        * pin/unpin/fence and not more.
+        */
+       intel_runtime_pm_get(dev_priv);
+
        dev_priv->mm.interruptible = false;
        ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
        if (ret)

@@ -2166,12 +2175,14 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
        i915_gem_object_pin_fence(obj);
 
        dev_priv->mm.interruptible = true;
+       intel_runtime_pm_put(dev_priv);
        return 0;
 
 err_unpin:
        i915_gem_object_unpin_from_display_plane(obj);
 err_interruptible:
        dev_priv->mm.interruptible = true;
+       intel_runtime_pm_put(dev_priv);
        return ret;
 }
 

@@ -8201,6 +8212,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
                                     uint32_t width, uint32_t height)
 {
        struct drm_device *dev = crtc->dev;
+       struct drm_i915_private *dev_priv = dev->dev_private;
        struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
        enum pipe pipe = intel_crtc->pipe;
        unsigned old_width, stride;

@@ -8231,6 +8243,16 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 
        /* we only need to pin inside GTT if cursor is non-phy */
        mutex_lock(&dev->struct_mutex);
+
+       /*
+        * Global gtt pte registers are special registers which actually forward
+        * writes to a chunk of system memory. Which means that there is no risk
+        * that the register values disappear as soon as we call
+        * intel_runtime_pm_put(), so it is correct to wrap only the
+        * pin/unpin/fence and not more.
+        */
+       intel_runtime_pm_get(dev_priv);
+
        if (!INTEL_INFO(dev)->cursor_needs_physical) {
                unsigned alignment;
 

@@ -8280,6 +8302,10 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 
        i915_gem_track_fb(intel_crtc->cursor_bo, obj,
                          INTEL_FRONTBUFFER_CURSOR(pipe));
+
+       if (obj)
+               intel_runtime_pm_put(dev_priv);
+
        mutex_unlock(&dev->struct_mutex);
 
        old_width = intel_crtc->cursor_width;

@@ -8301,6 +8327,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 fail_unpin:
        i915_gem_object_unpin_from_display_plane(obj);
 fail_locked:
+       intel_runtime_pm_put(dev_priv);
        mutex_unlock(&dev->struct_mutex);
 fail:
        drm_gem_object_unreference_unlocked(&obj->base);
--

2.0.1

Signature · 1 line
Paulo Zanoni
#20 lore raw
In reply to Paulo Zanoni
Quoted 93 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

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

We need to get runtime PM references to pin the objects, and to change the fences. The pin functions are the ideal places for this, but intel_crtc_cursor_set_obj() doesn't call them, so we also have to add get/put calls inside it. There is no problem if we runtime suspend right after these functions are finished, because the registers written are forwarded to system memory.

Note: for a complete fix of the cursor-dpms test case, we also need the patch named "drm/i915: Don't try to enable cursor from setplane when crtc is disabled".

v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
v3: - Make get/put also surround the fence and unpin calls (Daniel and
Ville).
- Merge all the plane changes into a single patch since they're the same fix.
- Add the comment requested by Daniel.
v4: - Remove spurious whitespace (Ville).
v5: - Remove intel_crtc_update_cursor() chunk since Ville did an
equivalent fix in another patch (Ville).
v6: - Remove unpin chunk: it will be on a separate patch (Ville,
Chris, Daniel).

Testcase: igt/pm_rpm/cursor
Testcase: igt/pm_rpm/cursor-dpms
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-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 | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

I talked with Daniel and we agreed to leave any possible fixes related with the "unpin" functions to separate patches, possibly requiring separate IGT test cases.

drivers/gpu/drm/i915/intel_display.c +12 -0
index 3813526..17bc661 100644
@@ -2149,6 +2149,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
 	if (need_vtd_wa(dev) && alignment < 256 * 1024)
 		alignment = 256 * 1024;
 
+	/*
+	 * Global gtt pte registers are special registers which actually forward
+	 * writes to a chunk of system memory. Which means that there is no risk
+	 * that the register values disappear as soon as we call
+	 * intel_runtime_pm_put(), so it is correct to wrap only the
+	 * pin/unpin/fence and not more.
+	 */
+	intel_runtime_pm_get(dev_priv);
+
 	dev_priv->mm.interruptible = false;
 	ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
 	if (ret)

@@ -2166,12 +2175,14 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
 	i915_gem_object_pin_fence(obj);
 
 	dev_priv->mm.interruptible = true;
+	intel_runtime_pm_put(dev_priv);
 	return 0;
 
 err_unpin:
 	i915_gem_object_unpin_from_display_plane(obj);
 err_interruptible:
 	dev_priv->mm.interruptible = true;
+	intel_runtime_pm_put(dev_priv);
 	return ret;
 }
 

@@ -8201,6 +8212,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 				     uint32_t width, uint32_t height)
 {
 	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	enum pipe pipe = intel_crtc->pipe;
 	unsigned old_width, stride;

@@ -8231,6 +8243,16 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 
 	/* we only need to pin inside GTT if cursor is non-phy */
 	mutex_lock(&dev->struct_mutex);

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

Quoted 10 lines
drivers/gpu/drm/i915/intel_display.c +10 -0
+
+	/*
+	 * Global gtt pte registers are special registers which actually forward
+	 * writes to a chunk of system memory. Which means that there is no risk
+	 * that the register values disappear as soon as we call
+	 * intel_runtime_pm_put(), so it is correct to wrap only the
+	 * pin/unpin/fence and not more.
+	 */
+	intel_runtime_pm_get(dev_priv);
+

Only the !cursor_needs_physical case need runtime pm get/put. I thought the calls were there originally, but I guess they got moved out. I suppose it's not a huge deal either way, but the current approach does give the reader the wrong impression that the unpin and frontbuffer tracking would also need a rpm reference.

Quoted 29 lines
drivers/gpu/drm/i915/intel_display.c +5 -0
 	if (!INTEL_INFO(dev)->cursor_needs_physical) {
 		unsigned alignment;
 

@@ -8280,6 +8302,10 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 
 	i915_gem_track_fb(intel_crtc->cursor_bo, obj,
 			  INTEL_FRONTBUFFER_CURSOR(pipe));
+
+	if (obj)
+		intel_runtime_pm_put(dev_priv);
+
 	mutex_unlock(&dev->struct_mutex);
 
 	old_width = intel_crtc->cursor_width;

@@ -8301,6 +8327,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 fail_unpin:
 	i915_gem_object_unpin_from_display_plane(obj);
 fail_locked:
+	intel_runtime_pm_put(dev_priv);
 	mutex_unlock(&dev->struct_mutex);
 fail:
 	drm_gem_object_unreference_unlocked(&obj->base);
Signature · 6 lines
2.0.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 2 lines
Ville Syrjälä
Intel OTC
author
#21 lore raw

2014-08-15 5:39 GMT-03:00 Ville Syrjälä <[email protected]>:

Quoted 102 lines · Paulo Zanoni
Quoted 101 lines

From: Paulo Zanoni <[email protected]>

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

We need to get runtime PM references to pin the objects, and to change the fences. The pin functions are the ideal places for this, but intel_crtc_cursor_set_obj() doesn't call them, so we also have to add get/put calls inside it. There is no problem if we runtime suspend right after these functions are finished, because the registers written are forwarded to system memory.

Note: for a complete fix of the cursor-dpms test case, we also need the patch named "drm/i915: Don't try to enable cursor from setplane when crtc is disabled".

v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
v3: - Make get/put also surround the fence and unpin calls (Daniel and
Ville).
- Merge all the plane changes into a single patch since they're the same fix.
- Add the comment requested by Daniel.
v4: - Remove spurious whitespace (Ville).
v5: - Remove intel_crtc_update_cursor() chunk since Ville did an
equivalent fix in another patch (Ville).
v6: - Remove unpin chunk: it will be on a separate patch (Ville,
Chris, Daniel).

Testcase: igt/pm_rpm/cursor
Testcase: igt/pm_rpm/cursor-dpms
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-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 | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

I talked with Daniel and we agreed to leave any possible fixes related with the "unpin" functions to separate patches, possibly requiring separate IGT test cases.

drivers/gpu/drm/i915/intel_display.c +20 -0
index 3813526..17bc661 100644
@@ -2149,6 +2149,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
      if (need_vtd_wa(dev) && alignment < 256 * 1024)
              alignment = 256 * 1024;
 
+     /*
+      * Global gtt pte registers are special registers which actually forward
+      * writes to a chunk of system memory. Which means that there is no risk
+      * that the register values disappear as soon as we call
+      * intel_runtime_pm_put(), so it is correct to wrap only the
+      * pin/unpin/fence and not more.
+      */
+     intel_runtime_pm_get(dev_priv);
+
      dev_priv->mm.interruptible = false;
      ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
      if (ret)

@@ -2166,12 +2175,14 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
      i915_gem_object_pin_fence(obj);
 
      dev_priv->mm.interruptible = true;
+     intel_runtime_pm_put(dev_priv);
      return 0;
 
 err_unpin:
      i915_gem_object_unpin_from_display_plane(obj);
 err_interruptible:
      dev_priv->mm.interruptible = true;
+     intel_runtime_pm_put(dev_priv);
      return ret;
 }
 

@@ -8201,6 +8212,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
                                   uint32_t width, uint32_t height)
 {
      struct drm_device *dev = crtc->dev;
+     struct drm_i915_private *dev_priv = dev->dev_private;
      struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
      enum pipe pipe = intel_crtc->pipe;
      unsigned old_width, stride;

@@ -8231,6 +8243,16 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 
      /* we only need to pin inside GTT if cursor is non-phy */
      mutex_lock(&dev->struct_mutex);
+
+     /*
+      * Global gtt pte registers are special registers which actually forward
+      * writes to a chunk of system memory. Which means that there is no risk
+      * that the register values disappear as soon as we call
+      * intel_runtime_pm_put(), so it is correct to wrap only the
+      * pin/unpin/fence and not more.
+      */
Quoted 8 lines

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

Quoted 2 lines
drivers/gpu/drm/i915/intel_display.c +2 -0
+     intel_runtime_pm_get(dev_priv);
+

Only the !cursor_needs_physical case need runtime pm get/put. I thought the calls were there originally, but I guess they got moved out. I suppose it's not a huge deal either way, but the current approach does give the reader the wrong impression that the unpin and frontbuffer tracking would also need a rpm reference.

"It's not a huge deal either way", yet you don't give a reviewed-by tag :)

I thought about just putting the get/put inside cursor_needs_physical, but then I'd need a new bool variable to track whether we need to put or not at the failure code paths. And we have so much code churn that maybe additional changes could leave the get/put calls in the wrong place, so having them wrap a wider piece of code could be better. Also, this is not like a mutex/spinlock get/put where we have to try to just get/put the smallest amount of things to not block another
thread: if this code is running, it's very likely that something else is going to wake up the graphics driver anyway - and this is why my first version just wrapped the whole function. Anyway, since I couldn't imagine what style the code reviewers would prefer - I'm fine with both ways - I had to chose one, but I guess I chose the wrong one :)

Quoted 34 lines
Quoted 26 lines
drivers/gpu/drm/i915/intel_display.c +5 -1
      if (!INTEL_INFO(dev)->cursor_needs_physical) {
              unsigned alignment;
 

@@ -8280,6 +8302,10 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 
      i915_gem_track_fb(intel_crtc->cursor_bo, obj,
                        INTEL_FRONTBUFFER_CURSOR(pipe));
+
+     if (obj)
+             intel_runtime_pm_put(dev_priv);
+
      mutex_unlock(&dev->struct_mutex);
 
      old_width = intel_crtc->cursor_width;

@@ -8301,6 +8327,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 fail_unpin:
      i915_gem_object_unpin_from_display_plane(obj);
 fail_locked:
+     intel_runtime_pm_put(dev_priv);
      mutex_unlock(&dev->struct_mutex);
 fail:
      drm_gem_object_unreference_unlocked(&obj->base);
--

2.0.1

Quoted 4 lines

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

--
Ville Syrjälä
Intel OTC

Signature · 5 lines
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
#22 lore raw
Quoted 112 lines · Paulo Zanoni

2014-08-15 5:39 GMT-03:00 Ville Syrjälä <[email protected]>:

Quoted 102 lines · Paulo Zanoni
Quoted 101 lines

From: Paulo Zanoni <[email protected]>

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

We need to get runtime PM references to pin the objects, and to change the fences. The pin functions are the ideal places for this, but intel_crtc_cursor_set_obj() doesn't call them, so we also have to add get/put calls inside it. There is no problem if we runtime suspend right after these functions are finished, because the registers written are forwarded to system memory.

Note: for a complete fix of the cursor-dpms test case, we also need the patch named "drm/i915: Don't try to enable cursor from setplane when crtc is disabled".

v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
v3: - Make get/put also surround the fence and unpin calls (Daniel and
Ville).
- Merge all the plane changes into a single patch since they're the same fix.
- Add the comment requested by Daniel.
v4: - Remove spurious whitespace (Ville).
v5: - Remove intel_crtc_update_cursor() chunk since Ville did an
equivalent fix in another patch (Ville).
v6: - Remove unpin chunk: it will be on a separate patch (Ville,
Chris, Daniel).

Testcase: igt/pm_rpm/cursor
Testcase: igt/pm_rpm/cursor-dpms
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-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 | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

I talked with Daniel and we agreed to leave any possible fixes related with the "unpin" functions to separate patches, possibly requiring separate IGT test cases.

drivers/gpu/drm/i915/intel_display.c +20 -0
index 3813526..17bc661 100644
@@ -2149,6 +2149,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
      if (need_vtd_wa(dev) && alignment < 256 * 1024)
              alignment = 256 * 1024;
 
+     /*
+      * Global gtt pte registers are special registers which actually forward
+      * writes to a chunk of system memory. Which means that there is no risk
+      * that the register values disappear as soon as we call
+      * intel_runtime_pm_put(), so it is correct to wrap only the
+      * pin/unpin/fence and not more.
+      */
+     intel_runtime_pm_get(dev_priv);
+
      dev_priv->mm.interruptible = false;
      ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
      if (ret)

@@ -2166,12 +2175,14 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
      i915_gem_object_pin_fence(obj);
 
      dev_priv->mm.interruptible = true;
+     intel_runtime_pm_put(dev_priv);
      return 0;
 
 err_unpin:
      i915_gem_object_unpin_from_display_plane(obj);
 err_interruptible:
      dev_priv->mm.interruptible = true;
+     intel_runtime_pm_put(dev_priv);
      return ret;
 }
 

@@ -8201,6 +8212,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
                                   uint32_t width, uint32_t height)
 {
      struct drm_device *dev = crtc->dev;
+     struct drm_i915_private *dev_priv = dev->dev_private;
      struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
      enum pipe pipe = intel_crtc->pipe;
      unsigned old_width, stride;

@@ -8231,6 +8243,16 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 
      /* we only need to pin inside GTT if cursor is non-phy */
      mutex_lock(&dev->struct_mutex);
+
+     /*
+      * Global gtt pte registers are special registers which actually forward
+      * writes to a chunk of system memory. Which means that there is no risk
+      * that the register values disappear as soon as we call
+      * intel_runtime_pm_put(), so it is correct to wrap only the
+      * pin/unpin/fence and not more.
+      */
Quoted 8 lines

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

Quoted 2 lines
drivers/gpu/drm/i915/intel_display.c +2 -0
+     intel_runtime_pm_get(dev_priv);
+

Only the !cursor_needs_physical case need runtime pm get/put. I thought the calls were there originally, but I guess they got moved out. I suppose it's not a huge deal either way, but the current approach does give the reader the wrong impression that the unpin and frontbuffer tracking would also need a rpm reference.

Quoted 1 line

"It's not a huge deal either way", yet you don't give a reviewed-by tag :)

I wanted to read your reply if there's a good reason for it ;)

Quoted 4 lines

I thought about just putting the get/put inside cursor_needs_physical, but then I'd need a new bool variable to track whether we need to put or not at the failure code paths.

Just put before goto is best for this kind of thing IMO.

Quoted 11 lines

And we have so much code churn that
maybe additional changes could leave the get/put calls in the wrong place, so having them wrap a wider piece of code could be better. Also, this is not like a mutex/spinlock get/put where we have to try to just get/put the smallest amount of things to not block another
thread: if this code is running, it's very likely that something else is going to wake up the graphics driver anyway - and this is why my first version just wrapped the whole function. Anyway, since I couldn't imagine what style the code reviewers would prefer - I'm fine with both ways - I had to chose one, but I guess I chose the wrong one :)

I figured since rpm get/put are a bit special (even the comment says so) that we'd have them exactly where needed.

Quoted 40 lines
Quoted 35 lines
Quoted 27 lines
drivers/gpu/drm/i915/intel_display.c +5 -1
      if (!INTEL_INFO(dev)->cursor_needs_physical) {
              unsigned alignment;
 

@@ -8280,6 +8302,10 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 
      i915_gem_track_fb(intel_crtc->cursor_bo, obj,
                        INTEL_FRONTBUFFER_CURSOR(pipe));
+
+     if (obj)
+             intel_runtime_pm_put(dev_priv);
+
      mutex_unlock(&dev->struct_mutex);
 
      old_width = intel_crtc->cursor_width;

@@ -8301,6 +8327,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 fail_unpin:
      i915_gem_object_unpin_from_display_plane(obj);
 fail_locked:
+     intel_runtime_pm_put(dev_priv);
      mutex_unlock(&dev->struct_mutex);
 fail:
      drm_gem_object_unreference_unlocked(&obj->base);
--

2.0.1

Quoted 4 lines

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

--
Ville Syrjälä
Intel OTC

Signature · 1 line
Paulo Zanoni
Signature · 2 lines
Ville Syrjälä
Intel OTC
author
#23 lore raw

PATCH 1/1 drm/i915: fix plane/cursor handling when runtime suspended

From: Paulo Zanoni <[email protected]>

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

We need to get runtime PM references to pin the objects, and to change the fences. The pin functions are the ideal places for this, but intel_crtc_cursor_set_obj() doesn't call them, so we also have to add get/put calls inside it. There is no problem if we runtime suspend right after these functions are finished, because the registers written are forwarded to system memory.

Note: for a complete fix of the cursor-dpms test case, we also need the patch named "drm/i915: Don't try to enable cursor from setplane when crtc is disabled".

v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
v3: - Make get/put also surround the fence and unpin calls (Daniel and
Ville).
- Merge all the plane changes into a single patch since they're the same fix.
- Add the comment requested by Daniel.
v4: - Remove spurious whitespace (Ville).
v5: - Remove intel_crtc_update_cursor() chunk since Ville did an
equivalent fix in another patch (Ville).
v6: - Remove unpin chunk: it will be on a separate patch (Ville,
Chris, Daniel).
v7: - Same thing, new color.

Testcase: igt/pm_rpm/cursor
Testcase: igt/pm_rpm/cursor-dpms
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-dpms
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81645
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82603

Cc [email protected]
Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/intel_display.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)
drivers/gpu/drm/i915/intel_display.c +25 -0
index 3f8e037..15fe3eb 100644
@@ -2201,6 +2201,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
 	if (need_vtd_wa(dev) && alignment < 256 * 1024)
 		alignment = 256 * 1024;
 
+	/*
+	 * Global gtt pte registers are special registers which actually forward
+	 * writes to a chunk of system memory. Which means that there is no risk
+	 * that the register values disappear as soon as we call
+	 * intel_runtime_pm_put(), so it is correct to wrap only the
+	 * pin/unpin/fence and not more.
+	 */
+	intel_runtime_pm_get(dev_priv);
+
 	dev_priv->mm.interruptible = false;
 	ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
 	if (ret)

@@ -2218,12 +2227,14 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
 	i915_gem_object_pin_fence(obj);
 
 	dev_priv->mm.interruptible = true;
+	intel_runtime_pm_put(dev_priv);
 	return 0;
 
 err_unpin:
 	i915_gem_object_unpin_from_display_plane(obj);
 err_interruptible:
 	dev_priv->mm.interruptible = true;
+	intel_runtime_pm_put(dev_priv);
 	return ret;
 }
 

@@ -8253,6 +8264,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 				     uint32_t width, uint32_t height)
 {
 	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	enum pipe pipe = intel_crtc->pipe;
 	unsigned old_width, stride;

@@ -8292,6 +8304,15 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 			goto fail_locked;
 		}
 
+		/*
+		 * Global gtt pte registers are special registers which actually
+		 * forward writes to a chunk of system memory. Which means that
+		 * there is no risk that the register values disappear as soon
+		 * as we call intel_runtime_pm_put(), so it is correct to wrap
+		 * only the pin/unpin/fence and not more.
+		 */
+		intel_runtime_pm_get(dev_priv);
+
 		/* Note that the w/a also requires 2 PTE of padding following
 		 * the bo. We currently fill all unused PTE with the shadow
 		 * page and so we should always have valid PTE following the

@@ -8304,16 +8325,20 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 		ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
 		if (ret) {
 			DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
+			intel_runtime_pm_put(dev_priv);
 			goto fail_locked;
 		}
 
 		ret = i915_gem_object_put_fence(obj);
 		if (ret) {
 			DRM_DEBUG_KMS("failed to release fence for cursor");
+			intel_runtime_pm_put(dev_priv);
 			goto fail_unpin;
 		}
 
 		addr = i915_gem_obj_ggtt_offset(obj);
+
+		intel_runtime_pm_put(dev_priv);
 	} else {
 		int align = IS_I830(dev) ? 16 * 1024 : 256;
 		ret = i915_gem_object_attach_phys(obj, align);
Signature · 1 line
2.0.1
Reviewed-by on v1 1/1
#24 lore raw
Quoted 37 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

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

We need to get runtime PM references to pin the objects, and to change the fences. The pin functions are the ideal places for this, but intel_crtc_cursor_set_obj() doesn't call them, so we also have to add get/put calls inside it. There is no problem if we runtime suspend right after these functions are finished, because the registers written are forwarded to system memory.

Note: for a complete fix of the cursor-dpms test case, we also need the patch named "drm/i915: Don't try to enable cursor from setplane when crtc is disabled".

v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
v3: - Make get/put also surround the fence and unpin calls (Daniel and
Ville).
- Merge all the plane changes into a single patch since they're the same fix.
- Add the comment requested by Daniel.
v4: - Remove spurious whitespace (Ville).
v5: - Remove intel_crtc_update_cursor() chunk since Ville did an
equivalent fix in another patch (Ville).
v6: - Remove unpin chunk: it will be on a separate patch (Ville,
Chris, Daniel).
v7: - Same thing, new color.

Testcase: igt/pm_rpm/cursor
Testcase: igt/pm_rpm/cursor-dpms
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-dpms
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81645
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82603

Quoted 2 lines
Cc [email protected]
Signed-off-by Paulo Zanoni <[email protected]>
Reviewed-by Ville Syrjälä <[email protected]>
Quoted 91 lines
---
 drivers/gpu/drm/i915/intel_display.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)
drivers/gpu/drm/i915/intel_display.c +25 -0
index 3f8e037..15fe3eb 100644
@@ -2201,6 +2201,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
 	if (need_vtd_wa(dev) && alignment < 256 * 1024)
 		alignment = 256 * 1024;
 
+	/*
+	 * Global gtt pte registers are special registers which actually forward
+	 * writes to a chunk of system memory. Which means that there is no risk
+	 * that the register values disappear as soon as we call
+	 * intel_runtime_pm_put(), so it is correct to wrap only the
+	 * pin/unpin/fence and not more.
+	 */
+	intel_runtime_pm_get(dev_priv);
+
 	dev_priv->mm.interruptible = false;
 	ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
 	if (ret)

@@ -2218,12 +2227,14 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
 	i915_gem_object_pin_fence(obj);
 
 	dev_priv->mm.interruptible = true;
+	intel_runtime_pm_put(dev_priv);
 	return 0;
 
 err_unpin:
 	i915_gem_object_unpin_from_display_plane(obj);
 err_interruptible:
 	dev_priv->mm.interruptible = true;
+	intel_runtime_pm_put(dev_priv);
 	return ret;
 }
 

@@ -8253,6 +8264,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 				     uint32_t width, uint32_t height)
 {
 	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	enum pipe pipe = intel_crtc->pipe;
 	unsigned old_width, stride;

@@ -8292,6 +8304,15 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 			goto fail_locked;
 		}
 
+		/*
+		 * Global gtt pte registers are special registers which actually
+		 * forward writes to a chunk of system memory. Which means that
+		 * there is no risk that the register values disappear as soon
+		 * as we call intel_runtime_pm_put(), so it is correct to wrap
+		 * only the pin/unpin/fence and not more.
+		 */
+		intel_runtime_pm_get(dev_priv);
+
 		/* Note that the w/a also requires 2 PTE of padding following
 		 * the bo. We currently fill all unused PTE with the shadow
 		 * page and so we should always have valid PTE following the

@@ -8304,16 +8325,20 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 		ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
 		if (ret) {
 			DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
+			intel_runtime_pm_put(dev_priv);
 			goto fail_locked;
 		}
 
 		ret = i915_gem_object_put_fence(obj);
 		if (ret) {
 			DRM_DEBUG_KMS("failed to release fence for cursor");
+			intel_runtime_pm_put(dev_priv);
 			goto fail_unpin;
 		}
 
 		addr = i915_gem_obj_ggtt_offset(obj);
+
+		intel_runtime_pm_put(dev_priv);
 	} else {
 		int align = IS_I830(dev) ? 16 * 1024 : 256;
 		ret = i915_gem_object_attach_phys(obj, align);
Signature · 6 lines
2.0.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 2 lines
Ville Syrjälä
Intel OTC
maintainer Applied
#25 lore raw
In reply to Paulo Zanoni
Quoted 37 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

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

We need to get runtime PM references to pin the objects, and to change the fences. The pin functions are the ideal places for this, but intel_crtc_cursor_set_obj() doesn't call them, so we also have to add get/put calls inside it. There is no problem if we runtime suspend right after these functions are finished, because the registers written are forwarded to system memory.

Note: for a complete fix of the cursor-dpms test case, we also need the patch named "drm/i915: Don't try to enable cursor from setplane when crtc is disabled".

v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
v3: - Make get/put also surround the fence and unpin calls (Daniel and
Ville).
- Merge all the plane changes into a single patch since they're the same fix.
- Add the comment requested by Daniel.
v4: - Remove spurious whitespace (Ville).
v5: - Remove intel_crtc_update_cursor() chunk since Ville did an
equivalent fix in another patch (Ville).
v6: - Remove unpin chunk: it will be on a separate patch (Ville,
Chris, Daniel).
v7: - Same thing, new color.

Testcase: igt/pm_rpm/cursor
Testcase: igt/pm_rpm/cursor-dpms
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-dpms
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81645
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82603

Quoted 2 lines
Cc [email protected]
Signed-off-by Paulo Zanoni <[email protected]>

Pushed to drm-intel-fixes, thanks for the patch and review.

BR,
Jani.

Quoted 91 lines
---
 drivers/gpu/drm/i915/intel_display.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)
drivers/gpu/drm/i915/intel_display.c +25 -0
index 3f8e037..15fe3eb 100644
@@ -2201,6 +2201,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
 	if (need_vtd_wa(dev) && alignment < 256 * 1024)
 		alignment = 256 * 1024;
 
+	/*
+	 * Global gtt pte registers are special registers which actually forward
+	 * writes to a chunk of system memory. Which means that there is no risk
+	 * that the register values disappear as soon as we call
+	 * intel_runtime_pm_put(), so it is correct to wrap only the
+	 * pin/unpin/fence and not more.
+	 */
+	intel_runtime_pm_get(dev_priv);
+
 	dev_priv->mm.interruptible = false;
 	ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
 	if (ret)

@@ -2218,12 +2227,14 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
 	i915_gem_object_pin_fence(obj);
 
 	dev_priv->mm.interruptible = true;
+	intel_runtime_pm_put(dev_priv);
 	return 0;
 
 err_unpin:
 	i915_gem_object_unpin_from_display_plane(obj);
 err_interruptible:
 	dev_priv->mm.interruptible = true;
+	intel_runtime_pm_put(dev_priv);
 	return ret;
 }
 

@@ -8253,6 +8264,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 				     uint32_t width, uint32_t height)
 {
 	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	enum pipe pipe = intel_crtc->pipe;
 	unsigned old_width, stride;

@@ -8292,6 +8304,15 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 			goto fail_locked;
 		}
 
+		/*
+		 * Global gtt pte registers are special registers which actually
+		 * forward writes to a chunk of system memory. Which means that
+		 * there is no risk that the register values disappear as soon
+		 * as we call intel_runtime_pm_put(), so it is correct to wrap
+		 * only the pin/unpin/fence and not more.
+		 */
+		intel_runtime_pm_get(dev_priv);
+
 		/* Note that the w/a also requires 2 PTE of padding following
 		 * the bo. We currently fill all unused PTE with the shadow
 		 * page and so we should always have valid PTE following the

@@ -8304,16 +8325,20 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 		ret = i915_gem_object_pin_to_display_plane(obj, alignment, NULL);
 		if (ret) {
 			DRM_DEBUG_KMS("failed to move cursor bo into the GTT\n");
+			intel_runtime_pm_put(dev_priv);
 			goto fail_locked;
 		}
 
 		ret = i915_gem_object_put_fence(obj);
 		if (ret) {
 			DRM_DEBUG_KMS("failed to release fence for cursor");
+			intel_runtime_pm_put(dev_priv);
 			goto fail_unpin;
 		}
 
 		addr = i915_gem_obj_ggtt_offset(obj);
+
+		intel_runtime_pm_put(dev_priv);
 	} else {
 		int align = IS_I830(dev) ? 16 * 1024 : 256;
 		ret = i915_gem_object_attach_phys(obj, align);
Signature · 6 lines
2.0.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 1 line
Jani Nikula, Intel Open Source Technology Center