Remove dead code from intel_get_load_detect_pipe

Remove dead code from intel_get_load_detect_pipe

[Chris Wilson]

One of the original chores for intel_get_load_detect_pipe() was to steal
a CRTC from an active output for the transient purpose of load detection.
Understandably this code was removed some time ago, but its legacy remains.
In order to fix a real bug, where we failed to attach a fb, I first had to
remove those extraneous complications and so make the real bug fix (and
every step towards it) as clear and as simple as possible (I hope!).
-Chris

[PATCH 1/6] drm/i915: Simplify return value from intel_get_load_detect_pipe

[Chris Wilson]

... and so remove the confusion as to whether to use the returned crtc
or intel_encoder->base.crtc with the subsequent load-detection. Even
though they were the same, the two instances of load-detection code
disagreed over which was the more correct.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_crt.c     |   17 +++++++----------
 drivers/gpu/drm/i915/intel_display.c |   22 ++++++++++++++--------
 drivers/gpu/drm/i915/intel_drv.h     |    8 ++++----
 drivers/gpu/drm/i915/intel_tv.c      |    6 ++----
 4 files changed, 27 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index d03fc05..2eb60cd 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -305,13 +305,11 @@ static bool intel_crt_detect_ddc(struct drm_connector *connector)
 }
 
 static enum drm_connector_status
-intel_crt_load_detect(struct drm_crtc *crtc, struct intel_crt *crt)
+intel_crt_load_detect(struct intel_crt *crt)
 {
-	struct drm_encoder *encoder = &crt->base.base;
-	struct drm_device *dev = encoder->dev;
+	struct drm_device *dev = crt->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	uint32_t pipe = intel_crtc->pipe;
+	uint32_t pipe = to_intel_crtc(crt->base.base.crtc)->pipe;
 	uint32_t save_bclrpat;
 	uint32_t save_vtotal;
 	uint32_t vtotal, vactive;
@@ -454,15 +452,14 @@ intel_crt_detect(struct drm_connector *connector, bool force)
 	/* for pre-945g platforms use load detect */
 	crtc = crt->base.base.crtc;
 	if (crtc && crtc->enabled) {
-		status = intel_crt_load_detect(crtc, crt);
+		status = intel_crt_load_detect(crt);
 	} else {
-		crtc = intel_get_load_detect_pipe(&crt->base, connector,
-						  NULL, &dpms_mode);
-		if (crtc) {
+		if (intel_get_load_detect_pipe(&crt->base, connector,
+					       NULL, &dpms_mode)) {
 			if (intel_crt_detect_ddc(connector))
 				status = connector_status_connected;
 			else
-				status = intel_crt_load_detect(crtc, crt);
+				status = intel_crt_load_detect(crt);
 			intel_release_load_detect_pipe(&crt->base,
 						       connector, dpms_mode);
 		} else
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e6df160..92e2f14 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5539,10 +5539,10 @@ static struct drm_display_mode load_detect_mode = {
 		 704, 832, 0, 480, 489, 491, 520, 0, DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC),
 };
 
-struct drm_crtc *intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
-					    struct drm_connector *connector,
-					    struct drm_display_mode *mode,
-					    int *dpms_mode)
+bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
+				struct drm_connector *connector,
+				struct drm_display_mode *mode,
+				int *dpms_mode)
 {
 	struct intel_crtc *intel_crtc;
 	struct drm_crtc *possible_crtc;
@@ -5575,7 +5575,7 @@ struct drm_crtc *intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 			crtc_funcs->dpms(crtc, DRM_MODE_DPMS_ON);
 			encoder_funcs->dpms(encoder, DRM_MODE_DPMS_ON);
 		}
-		return crtc;
+		return true;
 	}
 
 	/* Find an unused one (if possible) */
@@ -5595,7 +5595,8 @@ struct drm_crtc *intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 	 * If we didn't find an unused CRTC, don't use any.
 	 */
 	if (!crtc) {
-		return NULL;
+		DRM_DEBUG_KMS("no pipe available for load-detect\n");
+		return false;
 	}
 
 	encoder->crtc = crtc;
@@ -5608,7 +5609,11 @@ struct drm_crtc *intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 	if (!crtc->enabled) {
 		if (!mode)
 			mode = &load_detect_mode;
-		drm_crtc_helper_set_mode(crtc, mode, 0, 0, crtc->fb);
+
+		if (!drm_crtc_helper_set_mode(crtc, mode, 0, 0, crtc->fb)) {
+			DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
+			return false;
+		}
 	} else {
 		if (intel_crtc->dpms_mode != DRM_MODE_DPMS_ON) {
 			crtc_funcs = crtc->helper_private;
@@ -5619,10 +5624,11 @@ struct drm_crtc *intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 		encoder_funcs->mode_set(encoder, &crtc->mode, &crtc->mode);
 		encoder_funcs->commit(encoder);
 	}
+
 	/* let the connector get through one full cycle before testing */
 	intel_wait_for_vblank(dev, intel_crtc->pipe);
 
-	return crtc;
+	return true;
 }
 
 void intel_release_load_detect_pipe(struct intel_encoder *intel_encoder,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index aeb1b98..7ea4bab 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -298,10 +298,10 @@ static inline void intel_wait_for_crtc_vblank_safe(struct drm_crtc *crtc)
 		msleep(50);
 }
 extern void intel_wait_for_pipe_off(struct drm_device *dev, int pipe);
-extern struct drm_crtc *intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
-						   struct drm_connector *connector,
-						   struct drm_display_mode *mode,
-						   int *dpms_mode);
+extern bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
+				       struct drm_connector *connector,
+				       struct drm_display_mode *mode,
+				       int *dpms_mode);
 extern void intel_release_load_detect_pipe(struct intel_encoder *intel_encoder,
 					   struct drm_connector *connector,
 					   int dpms_mode);
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 3047a66..1174f77 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1370,12 +1370,10 @@ intel_tv_detect(struct drm_connector *connector, bool force)
 	if (intel_tv->base.base.crtc && intel_tv->base.base.crtc->enabled) {
 		type = intel_tv_detect_type(intel_tv, connector);
 	} else if (force) {
-		struct drm_crtc *crtc;
 		int dpms_mode;
 
-		crtc = intel_get_load_detect_pipe(&intel_tv->base, connector,
-						  &mode, &dpms_mode);
-		if (crtc) {
+		if (intel_get_load_detect_pipe(&intel_tv->base, connector,
+					       &mode, &dpms_mode)) {
 			type = intel_tv_detect_type(intel_tv, connector);
 			intel_release_load_detect_pipe(&intel_tv->base, connector,
 						       dpms_mode);
-- 
1.7.4.1

[PATCH 2/6] drm/i915: Don't store temporary load-detect variables in the generic encoder

[Chris Wilson]

Keep all the state required for undoing and restoring the previous pipe
configuration together in a single struct passed from
intel_get_load_detect_pipe() to intel_release_load_detect_pipe() rather
than stuffing them inside the common encoder structure.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_crt.c     |   11 ++++++-----
 drivers/gpu/drm/i915/intel_display.c |   26 +++++++++++++++-----------
 drivers/gpu/drm/i915/intel_drv.h     |   10 +++++++---
 drivers/gpu/drm/i915/intel_tv.c      |    9 +++++----
 4 files changed, 33 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 2eb60cd..e93f93c 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -430,7 +430,6 @@ intel_crt_detect(struct drm_connector *connector, bool force)
 	struct drm_device *dev = connector->dev;
 	struct intel_crt *crt = intel_attached_crt(connector);
 	struct drm_crtc *crtc;
-	int dpms_mode;
 	enum drm_connector_status status;
 
 	if (I915_HAS_HOTPLUG(dev)) {
@@ -454,14 +453,16 @@ intel_crt_detect(struct drm_connector *connector, bool force)
 	if (crtc && crtc->enabled) {
 		status = intel_crt_load_detect(crt);
 	} else {
-		if (intel_get_load_detect_pipe(&crt->base, connector,
-					       NULL, &dpms_mode)) {
+		struct intel_load_detect_pipe tmp;
+
+		if (intel_get_load_detect_pipe(&crt->base, connector, NULL,
+					       &tmp)) {
 			if (intel_crt_detect_ddc(connector))
 				status = connector_status_connected;
 			else
 				status = intel_crt_load_detect(crt);
-			intel_release_load_detect_pipe(&crt->base,
-						       connector, dpms_mode);
+			intel_release_load_detect_pipe(&crt->base, connector,
+						       &tmp);
 		} else
 			status = connector_status_unknown;
 	}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 92e2f14..1d7b9d4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5542,7 +5542,7 @@ static struct drm_display_mode load_detect_mode = {
 bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 				struct drm_connector *connector,
 				struct drm_display_mode *mode,
-				int *dpms_mode)
+				struct intel_load_detect_pipe *old)
 {
 	struct intel_crtc *intel_crtc;
 	struct drm_crtc *possible_crtc;
@@ -5567,14 +5567,18 @@ bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 	/* See if we already have a CRTC for this connector */
 	if (encoder->crtc) {
 		crtc = encoder->crtc;
-		/* Make sure the crtc and connector are running */
+
 		intel_crtc = to_intel_crtc(crtc);
-		*dpms_mode = intel_crtc->dpms_mode;
+		old->dpms_mode = intel_crtc->dpms_mode;
+		old->load_detect_temp = false;
+
+		/* Make sure the crtc and connector are running */
 		if (intel_crtc->dpms_mode != DRM_MODE_DPMS_ON) {
 			crtc_funcs = crtc->helper_private;
 			crtc_funcs->dpms(crtc, DRM_MODE_DPMS_ON);
 			encoder_funcs->dpms(encoder, DRM_MODE_DPMS_ON);
 		}
+
 		return true;
 	}
 
@@ -5601,10 +5605,10 @@ bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 
 	encoder->crtc = crtc;
 	connector->encoder = encoder;
-	intel_encoder->load_detect_temp = true;
 
 	intel_crtc = to_intel_crtc(crtc);
-	*dpms_mode = intel_crtc->dpms_mode;
+	old->dpms_mode = intel_crtc->dpms_mode;
+	old->load_detect_temp = true;
 
 	if (!crtc->enabled) {
 		if (!mode)
@@ -5632,7 +5636,8 @@ bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 }
 
 void intel_release_load_detect_pipe(struct intel_encoder *intel_encoder,
-				    struct drm_connector *connector, int dpms_mode)
+				    struct drm_connector *connector,
+				    struct intel_load_detect_pipe *old)
 {
 	struct drm_encoder *encoder = &intel_encoder->base;
 	struct drm_device *dev = encoder->dev;
@@ -5640,19 +5645,18 @@ void intel_release_load_detect_pipe(struct intel_encoder *intel_encoder,
 	struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private;
 	struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
 
-	if (intel_encoder->load_detect_temp) {
+	if (old->load_detect_temp) {
 		encoder->crtc = NULL;
 		connector->encoder = NULL;
-		intel_encoder->load_detect_temp = false;
 		crtc->enabled = drm_helper_crtc_in_use(crtc);
 		drm_helper_disable_unused_functions(dev);
 	}
 
 	/* Switch crtc and encoder back off if necessary */
-	if (crtc->enabled && dpms_mode != DRM_MODE_DPMS_ON) {
+	if (crtc->enabled && old->dpms_mode != DRM_MODE_DPMS_ON) {
 		if (encoder->crtc == crtc)
-			encoder_funcs->dpms(encoder, dpms_mode);
-		crtc_funcs->dpms(crtc, dpms_mode);
+			encoder_funcs->dpms(encoder, old->dpms_mode);
+		crtc_funcs->dpms(crtc, old->dpms_mode);
 	}
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 7ea4bab..9409a46 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -140,7 +140,6 @@ struct intel_fbdev {
 struct intel_encoder {
 	struct drm_encoder base;
 	int type;
-	bool load_detect_temp;
 	bool needs_tv_clock;
 	void (*hot_plug)(struct intel_encoder *);
 	int crtc_mask;
@@ -298,13 +297,18 @@ static inline void intel_wait_for_crtc_vblank_safe(struct drm_crtc *crtc)
 		msleep(50);
 }
 extern void intel_wait_for_pipe_off(struct drm_device *dev, int pipe);
+
+struct intel_load_detect_pipe {
+	bool load_detect_temp;
+	int dpms_mode;
+};
 extern bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 				       struct drm_connector *connector,
 				       struct drm_display_mode *mode,
-				       int *dpms_mode);
+				       struct intel_load_detect_pipe *old);
 extern void intel_release_load_detect_pipe(struct intel_encoder *intel_encoder,
 					   struct drm_connector *connector,
-					   int dpms_mode);
+					   struct intel_load_detect_pipe *old);
 
 extern struct drm_connector* intel_sdvo_find(struct drm_device *dev, int sdvoB);
 extern int intel_sdvo_supports_hotplug(struct drm_connector *connector);
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 1174f77..529f232 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1370,13 +1370,14 @@ intel_tv_detect(struct drm_connector *connector, bool force)
 	if (intel_tv->base.base.crtc && intel_tv->base.base.crtc->enabled) {
 		type = intel_tv_detect_type(intel_tv, connector);
 	} else if (force) {
-		int dpms_mode;
+		struct intel_load_detect_pipe tmp;
 
 		if (intel_get_load_detect_pipe(&intel_tv->base, connector,
-					       &mode, &dpms_mode)) {
+					       &mode, &tmp)) {
 			type = intel_tv_detect_type(intel_tv, connector);
-			intel_release_load_detect_pipe(&intel_tv->base, connector,
-						       dpms_mode);
+			intel_release_load_detect_pipe(&intel_tv->base,
+						       connector,
+						       &tmp);
 		} else
 			return connector_status_unknown;
 	} else
-- 
1.7.4.1

[PATCH 3/6] drm/i915: Remove unused supported_crtc from intel_load_detect_pipe

[Chris Wilson]

... and the no longer relevant comment. The code ceased stealing a pipe
for load detection a long time ago.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1d7b9d4..0d8d5e2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5546,7 +5546,6 @@ bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 {
 	struct intel_crtc *intel_crtc;
 	struct drm_crtc *possible_crtc;
-	struct drm_crtc *supported_crtc =NULL;
 	struct drm_encoder *encoder = &intel_encoder->base;
 	struct drm_crtc *crtc = NULL;
 	struct drm_device *dev = encoder->dev;
@@ -5556,12 +5555,12 @@ bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 
 	/*
 	 * Algorithm gets a little messy:
+	 *
 	 *   - if the connector already has an assigned crtc, use it (but make
 	 *     sure it's on first)
+	 *
 	 *   - try to find the first unused crtc that can drive this connector,
 	 *     and use that if we find one
-	 *   - if there are no unused crtcs available, try to use the first
-	 *     one we found that supports the connector
 	 */
 
 	/* See if we already have a CRTC for this connector */
@@ -5591,8 +5590,6 @@ bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 			crtc = possible_crtc;
 			break;
 		}
-		if (!supported_crtc)
-			supported_crtc = possible_crtc;
 	}
 
 	/*
-- 
1.7.4.1

[PATCH 4/6] drm/i915: Pass the saved adjusted_mode when adding to the load-detect crtc

[Chris Wilson]

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0d8d5e2..61fa02f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5622,7 +5622,7 @@ bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 		}
 
 		/* Add this connector to the crtc */
-		encoder_funcs->mode_set(encoder, &crtc->mode, &crtc->mode);
+		encoder_funcs->mode_set(encoder, &crtc->mode, &crtc->hwmode);
 		encoder_funcs->commit(encoder);
 	}
 
-- 
1.7.4.1

[PATCH 5/6] drm/i915: Remove dead code from intel_get_load_detect_pipe()

[Chris Wilson]

As we only allow the use of a disabled CRTC, we don't need to handle the
case we are reusing an already enabled pipe.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c |   28 ++++++++++------------------
 1 files changed, 10 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 61fa02f..9b1a3e1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5549,8 +5549,6 @@ bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 	struct drm_encoder *encoder = &intel_encoder->base;
 	struct drm_crtc *crtc = NULL;
 	struct drm_device *dev = encoder->dev;
-	struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private;
-	struct drm_crtc_helper_funcs *crtc_funcs;
 	int i = -1;
 
 	/*
@@ -5573,8 +5571,13 @@ bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 
 		/* Make sure the crtc and connector are running */
 		if (intel_crtc->dpms_mode != DRM_MODE_DPMS_ON) {
+			struct drm_encoder_helper_funcs *encoder_funcs;
+			struct drm_crtc_helper_funcs *crtc_funcs;
+
 			crtc_funcs = crtc->helper_private;
 			crtc_funcs->dpms(crtc, DRM_MODE_DPMS_ON);
+
+			encoder_funcs = encoder->helper_private;
 			encoder_funcs->dpms(encoder, DRM_MODE_DPMS_ON);
 		}
 
@@ -5607,23 +5610,12 @@ bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 	old->dpms_mode = intel_crtc->dpms_mode;
 	old->load_detect_temp = true;
 
-	if (!crtc->enabled) {
-		if (!mode)
-			mode = &load_detect_mode;
-
-		if (!drm_crtc_helper_set_mode(crtc, mode, 0, 0, crtc->fb)) {
-			DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
-			return false;
-		}
-	} else {
-		if (intel_crtc->dpms_mode != DRM_MODE_DPMS_ON) {
-			crtc_funcs = crtc->helper_private;
-			crtc_funcs->dpms(crtc, DRM_MODE_DPMS_ON);
-		}
+	if (!mode)
+		mode = &load_detect_mode;
 
-		/* Add this connector to the crtc */
-		encoder_funcs->mode_set(encoder, &crtc->mode, &crtc->hwmode);
-		encoder_funcs->commit(encoder);
+	if (!drm_crtc_helper_set_mode(crtc, mode, 0, 0, crtc->fb)) {
+		DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
+		return false;
 	}
 
 	/* let the connector get through one full cycle before testing */
-- 
1.7.4.1

[PATCH 6/6] drm/i915: Attach a fb to the load-detect pipe

[Chris Wilson]

We need to ensure that we feed valid memory into the display plane
attached to the pipe when switching the pipe on. Otherwise, the display
engine may read through an invalid PTE and so throw an PGTBL_ER
exception.

For bonus amusement value, we perform the first load detect before even
establishing our fbdev.

Reported-by: Knut Petersen <Knut_Petersen@t-online.de>
References: https://bugs.freedesktop.org/show_bug.cgi?id=36246
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c |   18 +++++++++++++++++-
 1 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9b1a3e1..e68dd08 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5549,6 +5549,8 @@ bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 	struct drm_encoder *encoder = &intel_encoder->base;
 	struct drm_crtc *crtc = NULL;
 	struct drm_device *dev = encoder->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_framebuffer *old_fb;
 	int i = -1;
 
 	/*
@@ -5613,8 +5615,22 @@ bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 	if (!mode)
 		mode = &load_detect_mode;
 
-	if (!drm_crtc_helper_set_mode(crtc, mode, 0, 0, crtc->fb)) {
+	/* Ensure we bind a framebuffer to supply the plane.
+	 * As we may be called during intel_framebuffer_init,
+	 * we need to be careful that we have actually initialised
+	 * the fbcon before using it.
+	 */
+	if (dev_priv->fbdev == NULL || dev_priv->fbdev->ifb.obj == NULL) {
+		DRM_DEBUG("no fb to bind for load-detect pipe\n");
+		return false;
+	}
+
+	old_fb = crtc->fb;
+	crtc->fb = &dev_priv->fbdev->ifb.base;
+
+	if (!drm_crtc_helper_set_mode(crtc, mode, 0, 0, old_fb)) {
 		DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
+		crtc->fb = old_fb;
 		return false;
 	}
 
-- 
1.7.4.1

Re: [PATCH 1/6] drm/i915: Simplify return value from intel_get_load_detect_pipe

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 345 bytes --]

On Wed, 20 Apr 2011 10:22:17 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> +
> +		if (!drm_crtc_helper_set_mode(crtc, mode, 0, 0, crtc->fb)) {
> +			DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
> +			return false;
> +		}

This seems like a bug-fix, separate from the API change.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 2/6] drm/i915: Don't store temporary load-detect variables in the generic encoder

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 432 bytes --]

On Wed, 20 Apr 2011 10:22:18 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Keep all the state required for undoing and restoring the previous pipe
> configuration together in a single struct passed from
> intel_get_load_detect_pipe() to intel_release_load_detect_pipe() rather
> than stuffing them inside the common encoder structure.

Reviewed-by: Keith Packard <keithp@keithp.com>

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 3/6] drm/i915: Remove unused supported_crtc from intel_load_detect_pipe

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 280 bytes --]

On Wed, 20 Apr 2011 10:22:19 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> ... and the no longer relevant comment. The code ceased stealing a pipe
> for load detection a long time ago.

Reviewed-by: Keith Packard <keithp@keithp.com>

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 4/6] drm/i915: Pass the saved adjusted_mode when adding to the load-detect crtc

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 316 bytes --]

On Wed, 20 Apr 2011 10:22:20 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Keith Packard <keithp@keithp.com>

(this might be separately tested to see if it might fix some TV load
detection issues?)

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH 1/2] drm/i915: Simplify return value from intel_get_load_detect_pipe

[Chris Wilson]

... and so remove the confusion as to whether to use the returned crtc
or intel_encoder->base.crtc with the subsequent load-detection. Even
though they were the same, the two instances of load-detection code
disagreed over which was the more correct.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_crt.c     |   17 +++++++----------
 drivers/gpu/drm/i915/intel_display.c |   16 +++++++++-------
 drivers/gpu/drm/i915/intel_drv.h     |    8 ++++----
 drivers/gpu/drm/i915/intel_tv.c      |    6 ++----
 4 files changed, 22 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index d03fc05..2eb60cd 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -305,13 +305,11 @@ static bool intel_crt_detect_ddc(struct drm_connector *connector)
 }
 
 static enum drm_connector_status
-intel_crt_load_detect(struct drm_crtc *crtc, struct intel_crt *crt)
+intel_crt_load_detect(struct intel_crt *crt)
 {
-	struct drm_encoder *encoder = &crt->base.base;
-	struct drm_device *dev = encoder->dev;
+	struct drm_device *dev = crt->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	uint32_t pipe = intel_crtc->pipe;
+	uint32_t pipe = to_intel_crtc(crt->base.base.crtc)->pipe;
 	uint32_t save_bclrpat;
 	uint32_t save_vtotal;
 	uint32_t vtotal, vactive;
@@ -454,15 +452,14 @@ intel_crt_detect(struct drm_connector *connector, bool force)
 	/* for pre-945g platforms use load detect */
 	crtc = crt->base.base.crtc;
 	if (crtc && crtc->enabled) {
-		status = intel_crt_load_detect(crtc, crt);
+		status = intel_crt_load_detect(crt);
 	} else {
-		crtc = intel_get_load_detect_pipe(&crt->base, connector,
-						  NULL, &dpms_mode);
-		if (crtc) {
+		if (intel_get_load_detect_pipe(&crt->base, connector,
+					       NULL, &dpms_mode)) {
 			if (intel_crt_detect_ddc(connector))
 				status = connector_status_connected;
 			else
-				status = intel_crt_load_detect(crtc, crt);
+				status = intel_crt_load_detect(crt);
 			intel_release_load_detect_pipe(&crt->base,
 						       connector, dpms_mode);
 		} else
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e6df160..a0df09b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5539,10 +5539,10 @@ static struct drm_display_mode load_detect_mode = {
 		 704, 832, 0, 480, 489, 491, 520, 0, DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC),
 };
 
-struct drm_crtc *intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
-					    struct drm_connector *connector,
-					    struct drm_display_mode *mode,
-					    int *dpms_mode)
+bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
+				struct drm_connector *connector,
+				struct drm_display_mode *mode,
+				int *dpms_mode)
 {
 	struct intel_crtc *intel_crtc;
 	struct drm_crtc *possible_crtc;
@@ -5575,7 +5575,7 @@ struct drm_crtc *intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 			crtc_funcs->dpms(crtc, DRM_MODE_DPMS_ON);
 			encoder_funcs->dpms(encoder, DRM_MODE_DPMS_ON);
 		}
-		return crtc;
+		return true;
 	}
 
 	/* Find an unused one (if possible) */
@@ -5595,7 +5595,8 @@ struct drm_crtc *intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 	 * If we didn't find an unused CRTC, don't use any.
 	 */
 	if (!crtc) {
-		return NULL;
+		DRM_DEBUG_KMS("no pipe available for load-detect\n");
+		return false;
 	}
 
 	encoder->crtc = crtc;
@@ -5619,10 +5620,11 @@ struct drm_crtc *intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 		encoder_funcs->mode_set(encoder, &crtc->mode, &crtc->mode);
 		encoder_funcs->commit(encoder);
 	}
+
 	/* let the connector get through one full cycle before testing */
 	intel_wait_for_vblank(dev, intel_crtc->pipe);
 
-	return crtc;
+	return true;
 }
 
 void intel_release_load_detect_pipe(struct intel_encoder *intel_encoder,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index aeb1b98..7ea4bab 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -298,10 +298,10 @@ static inline void intel_wait_for_crtc_vblank_safe(struct drm_crtc *crtc)
 		msleep(50);
 }
 extern void intel_wait_for_pipe_off(struct drm_device *dev, int pipe);
-extern struct drm_crtc *intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
-						   struct drm_connector *connector,
-						   struct drm_display_mode *mode,
-						   int *dpms_mode);
+extern bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
+				       struct drm_connector *connector,
+				       struct drm_display_mode *mode,
+				       int *dpms_mode);
 extern void intel_release_load_detect_pipe(struct intel_encoder *intel_encoder,
 					   struct drm_connector *connector,
 					   int dpms_mode);
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 3047a66..1174f77 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1370,12 +1370,10 @@ intel_tv_detect(struct drm_connector *connector, bool force)
 	if (intel_tv->base.base.crtc && intel_tv->base.base.crtc->enabled) {
 		type = intel_tv_detect_type(intel_tv, connector);
 	} else if (force) {
-		struct drm_crtc *crtc;
 		int dpms_mode;
 
-		crtc = intel_get_load_detect_pipe(&intel_tv->base, connector,
-						  &mode, &dpms_mode);
-		if (crtc) {
+		if (intel_get_load_detect_pipe(&intel_tv->base, connector,
+					       &mode, &dpms_mode)) {
 			type = intel_tv_detect_type(intel_tv, connector);
 			intel_release_load_detect_pipe(&intel_tv->base, connector,
 						       dpms_mode);
-- 
1.7.4.1

[PATCH 2/2] drm/i915: Propagate failure to set mode for load-detect pipe

[Chris Wilson]

Check the return value from drm_crtc_set_mode(), report the failure
via a debug message and propagate the error back to the caller. This
prevents us from blissfully continuing to do the load detection on a
disabled pipe. Fortunately actual failure for modesetting is very rare,
and reported failures even rarer.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a0df09b..92e2f14 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5609,7 +5609,11 @@ bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 	if (!crtc->enabled) {
 		if (!mode)
 			mode = &load_detect_mode;
-		drm_crtc_helper_set_mode(crtc, mode, 0, 0, crtc->fb);
+
+		if (!drm_crtc_helper_set_mode(crtc, mode, 0, 0, crtc->fb)) {
+			DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
+			return false;
+		}
 	} else {
 		if (intel_crtc->dpms_mode != DRM_MODE_DPMS_ON) {
 			crtc_funcs = crtc->helper_private;
-- 
1.7.4.1

Re: [PATCH 5/6] drm/i915: Remove dead code from intel_get_load_detect_pipe()

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 292 bytes --]

On Wed, 20 Apr 2011 10:22:21 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> As we only allow the use of a disabled CRTC, we don't need to handle the
> case we are reusing an already enabled pipe.

Reviewed-by: Keith Packard <keithp@keithp.com>

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 6/6] drm/i915: Attach a fb to the load-detect pipe

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 646 bytes --]

On Wed, 20 Apr 2011 10:22:22 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> For bonus amusement value, we perform the first load detect before even
> establishing our fbdev.

It seems like we need to be able to perform load detection in this
case to create a suitable frame buffer at boot time.

I also don't see a check to ensure that the fbdev frame buffer is large
enough to cover the load detect mode; of course, it probably is given
that the load detect mode is usually quite small, but...

Seems like we need to allocate a temporary object for load detect mode
setting in some cases.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 6/6] drm/i915: Attach a fb to the load-detect pipe

[Chris Wilson]

On Wed, 20 Apr 2011 11:43:38 -0700, Keith Packard <keithp@keithp.com> wrote:
> On Wed, 20 Apr 2011 10:22:22 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > For bonus amusement value, we perform the first load detect before even
> > establishing our fbdev.
> 
> It seems like we need to be able to perform load detection in this
> case to create a suitable frame buffer at boot time.

Aye, a classic chicken and egg problem. I wanted to ignore it as
everything would have been resized upon the first probe by userspace
(hotplug polling being disabled for load-detect paths), but only after the
probe completes so there is the issue that the fb may be too small and so
we don't prevent all potential PGTBL_ER.  And VGA is laso likely to be a
boot device.

I'll spin up a patch for a temporary buffer and see what you think.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/6] drm/i915: Pass the saved adjusted_mode when adding to the load-detect crtc

[Chris Wilson]

On Wed, 20 Apr 2011 11:23:01 -0700, Keith Packard <keithp@keithp.com> wrote:
> On Wed, 20 Apr 2011 10:22:20 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Reviewed-by: Keith Packard <keithp@keithp.com>
> 
> (this might be separately tested to see if it might fix some TV load
> detection issues?)

It is an old bug along a dead code path. It can simply be squashed with
the next patch that removes it entirely, if you prefer.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915: Attach a fb to the load-detect pipe

[Chris Wilson]

We need to ensure that we feed valid memory into the display plane
attached to the pipe when switching the pipe on. Otherwise, the display
engine may read through an invalid PTE and so throw an PGTBL_ER
exception.

As we need to perform load detection before even the first object is
allocated for the fbdev, there is no pre-existing object large enough
for us to borrow to use as the framebuffer. So we need to create one
and cleanup afterwards.

Found by assert_fb_bound_for_plane().

Reported-by: Knut Petersen <Knut_Petersen@t-online.de>
References: https://bugs.freedesktop.org/show_bug.cgi?id=36246
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---

I've only compile tested this so far. I manage to break my grub
installation on the only 915GM I have (and the debian installer doesn't),
so I haven't been able to test this yet.

We can refactor half of intel_framebuffer_create_for_mode and share the
kzalloc+init with intel_user_framebuffer_create.

But I think this is close.
-Chris

---
 drivers/gpu/drm/i915/intel_display.c |   57 +++++++++++++++++++++++++++++++++-
 1 files changed, 56 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9b1a3e1..f1cb91e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5539,6 +5539,43 @@ static struct drm_display_mode load_detect_mode = {
 		 704, 832, 0, 480, 489, 491, 520, 0, DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC),
 };
 
+static struct drm_framebuffer *
+intel_framebuffer_create_for_mode(struct drm_device *dev,
+				  struct drm_display_mode *mode)
+{
+	struct drm_i915_gem_object *obj;
+	struct intel_framebuffer *intel_fb;
+	struct drm_mode_fb_cmd mode_cmd;
+	u32 size;
+
+	/* Presume a 32-bpp fb is correct */
+	mode_cmd.width = mode->hdisplay;
+	mode_cmd.height = mode->vdisplay;
+	mode_cmd.depth = 24;
+	mode_cmd.bpp = 32;
+	mode_cmd.pitch = ALIGN(mode->hdisplay * 4, 64);
+
+	size = ALIGN(mode_cmd.pitch * mode_cmd.height, PAGE_SIZE);
+
+	obj = i915_gem_alloc_object(dev, size);
+	if (obj == NULL)
+		return NULL;
+
+	intel_fb = kzalloc(sizeof(*intel_fb), GFP_KERNEL);
+	if (!intel_fb) {
+		drm_gem_object_unreference_unlocked(&obj->base);
+		return NULL;
+	}
+
+	if (intel_framebuffer_init(dev, intel_fb, &mode_cmd, obj)) {
+		drm_gem_object_unreference_unlocked(&obj->base);
+		kfree(intel_fb);
+		return NULL;
+	}
+
+	return &intel_fb->base;
+}
+
 bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 				struct drm_connector *connector,
 				struct drm_display_mode *mode,
@@ -5549,6 +5586,7 @@ bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 	struct drm_encoder *encoder = &intel_encoder->base;
 	struct drm_crtc *crtc = NULL;
 	struct drm_device *dev = encoder->dev;
+	struct drm_framebuffer *old_fb;
 	int i = -1;
 
 	/*
@@ -5613,8 +5651,21 @@ bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 	if (!mode)
 		mode = &load_detect_mode;
 
-	if (!drm_crtc_helper_set_mode(crtc, mode, 0, 0, crtc->fb)) {
+	/* We need a framebuffer large enough to accommodate all accesses
+	 * that the plane may generate whilst we perform load detection.
+	 */
+	old_fb = crtc->fb;
+	crtc->fb = intel_framebuffer_create_for_mode(dev, mode);
+	if (crtc->fb == NULL) {
+		DRM_DEBUG_KMS("failed to allocate framebuffer for load-detection\n");
+		crtc->fb = old_fb;
+		return false;
+	}
+
+	if (!drm_crtc_helper_set_mode(crtc, mode, 0, 0, old_fb)) {
 		DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
+		crtc->fb->funcs->destroy(crtc->fb);
+		crtc->fb = old_fb;
 		return false;
 	}
 
@@ -5635,10 +5686,14 @@ void intel_release_load_detect_pipe(struct intel_encoder *intel_encoder,
 	struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
 
 	if (old->load_detect_temp) {
+		struct drm_framebuffer *fb = crtc->fb;
+
 		encoder->crtc = NULL;
 		connector->encoder = NULL;
 		crtc->enabled = drm_helper_crtc_in_use(crtc);
 		drm_helper_disable_unused_functions(dev);
+
+		fb->funcs->destroy(fb);
 	}
 
 	/* Switch crtc and encoder back off if necessary */
-- 
1.7.4.1

Re: [PATCH 4/6] drm/i915: Pass the saved adjusted_mode when adding to the load-detect crtc

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 770 bytes --]

On Wed, 20 Apr 2011 20:38:54 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Wed, 20 Apr 2011 11:23:01 -0700, Keith Packard <keithp@keithp.com> wrote:
> > On Wed, 20 Apr 2011 10:22:20 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > 
> > Reviewed-by: Keith Packard <keithp@keithp.com>
> > 
> > (this might be separately tested to see if it might fix some TV load
> > detection issues?)
> 
> It is an old bug along a dead code path. It can simply be squashed with
> the next patch that removes it entirely, if you prefer.

Right, I didn't look forward to see that it was in 're-use existing
pipe' path which has been dead for a long time now.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 6/6] drm/i915: Attach a fb to the load-detect pipe

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 358 bytes --]

On Wed, 20 Apr 2011 19:55:10 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> I'll spin up a patch for a temporary buffer and see what you think.

Ok. I'd love to be able to use that code path all of the time, but I
don't like the thought of the cost of a temporary allocation every time
you do load detection.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH] drm/i915: Attach a fb to the load-detect pipe

[Chris Wilson]

We need to ensure that we feed valid memory into the display plane
attached to the pipe when switching the pipe on. Otherwise, the display
engine may read through an invalid PTE and so throw an PGTBL_ER
exception.

As we need to perform load detection before even the first object is
allocated for the fbdev, there is no pre-existing object large enough
for us to borrow to use as the framebuffer. So we need to create one
and cleanup afterwards. At other times, the current fbcon may be large
enough for us to borrow it for duration of load detection.

Found by assert_fb_bound_for_plane().

Reported-by: Knut Petersen <Knut_Petersen@t-online.de>
References: https://bugs.freedesktop.org/show_bug.cgi?id=36246
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---

915GM is back online, so I'll be able to test this tomorrow. In the
meantime, if anybody can spot anything else they'd like to change, now
is the perfect opportunity to do so...
-Chris

---
 drivers/gpu/drm/i915/intel_display.c |  111 ++++++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_drv.h     |    1 +
 2 files changed, 95 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9b1a3e1..d993b95 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5539,6 +5539,66 @@ static struct drm_display_mode load_detect_mode = {
 		 704, 832, 0, 480, 489, 491, 520, 0, DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC),
 };
 
+static struct drm_framebuffer *
+intel_framebuffer_create(struct drm_device *dev,
+			 struct drm_mode_fb_cmd *mode_cmd,
+			 struct drm_i915_gem_object *obj)
+{
+	struct intel_framebuffer *intel_fb;
+	int ret;
+
+	intel_fb = kzalloc(sizeof(*intel_fb), GFP_KERNEL);
+	if (!intel_fb) {
+		drm_gem_object_unreference_unlocked(&obj->base);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	ret = intel_framebuffer_init(dev, intel_fb, mode_cmd, obj);
+	if (ret) {
+		drm_gem_object_unreference_unlocked(&obj->base);
+		kfree(intel_fb);
+		return ERR_PTR(ret);
+	}
+
+	return &intel_fb->base;
+}
+
+static u32
+intel_framebuffer_pitch_for_width(int width, int bpp)
+{
+	u32 pitch = DIV_ROUND_UP(width * bpp, 8);
+	return ALIGN(pitch, 64);
+}
+
+static u32
+intel_framebuffer_size_for_mode(struct drm_display_mode *mode, int bpp)
+{
+	u32 pitch = intel_framebuffer_pitch_for_width(mode->hdisplay, bpp);
+	return ALIGN(pitch * mode->vdisplay, PAGE_SIZE);
+}
+
+static struct drm_framebuffer *
+intel_framebuffer_create_for_mode(struct drm_device *dev,
+				  struct drm_display_mode *mode,
+				  int depth, int bpp)
+{
+	struct drm_i915_gem_object *obj;
+	struct drm_mode_fb_cmd mode_cmd;
+
+	obj = i915_gem_alloc_object(dev,
+				    intel_framebuffer_size_for_mode(mode, bpp));
+	if (obj == NULL)
+		return ERR_PTR(-ENOMEM);
+
+	mode_cmd.width = mode->hdisplay;
+	mode_cmd.height = mode->vdisplay;
+	mode_cmd.depth = depth;
+	mode_cmd.bpp = bpp;
+	mode_cmd.pitch = intel_framebuffer_pitch_for_width(mode_cmd.width, bpp);
+
+	return intel_framebuffer_create(dev, &mode_cmd, obj);
+}
+
 bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 				struct drm_connector *connector,
 				struct drm_display_mode *mode,
@@ -5549,6 +5609,8 @@ bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 	struct drm_encoder *encoder = &intel_encoder->base;
 	struct drm_crtc *crtc = NULL;
 	struct drm_device *dev = encoder->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_framebuffer *old_fb;
 	int i = -1;
 
 	/*
@@ -5609,12 +5671,39 @@ bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 	intel_crtc = to_intel_crtc(crtc);
 	old->dpms_mode = intel_crtc->dpms_mode;
 	old->load_detect_temp = true;
+	old->release_fb = NULL;
 
 	if (!mode)
 		mode = &load_detect_mode;
 
-	if (!drm_crtc_helper_set_mode(crtc, mode, 0, 0, crtc->fb)) {
+	old_fb = crtc->fb;
+
+	/* We need a framebuffer large enough to accommodate all accesses
+	 * that the plane may generate whilst we perform load detection.
+	 * We can not rely on the fbcon either being present (we get called
+	 * during its initialisation to detect all boot displays, or it may
+	 * not even exist) or that it is large enough to satisfy the
+	 * requested mode.
+	 */
+	crtc->fb = NULL;
+	if (dev_priv->fbdev &&
+	    dev_priv->fbdev->ifb.obj &&
+	    dev_priv->fbdev->ifb.obj->base.size >= intel_framebuffer_size_for_mode(mode, 32))
+		crtc->fb = &dev_priv->fbdev->ifb.base;
+	if (crtc->fb == NULL) {
+		crtc->fb = intel_framebuffer_create_for_mode(dev, mode, 24, 32);
+		old->release_fb = crtc->fb;
+	}
+	if (IS_ERR_OR_NULL(crtc->fb)) {
+		DRM_DEBUG_KMS("failed to allocate framebuffer for load-detection\n");
+		crtc->fb = old_fb;
+		return false;
+	}
+
+	if (!drm_crtc_helper_set_mode(crtc, mode, 0, 0, old_fb)) {
 		DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
+		crtc->fb->funcs->destroy(crtc->fb);
+		crtc->fb = old_fb;
 		return false;
 	}
 
@@ -5639,6 +5728,9 @@ void intel_release_load_detect_pipe(struct intel_encoder *intel_encoder,
 		connector->encoder = NULL;
 		crtc->enabled = drm_helper_crtc_in_use(crtc);
 		drm_helper_disable_unused_functions(dev);
+
+		if (old->release_fb)
+			old->release_fb->funcs->destroy(old->release_fb);
 	}
 
 	/* Switch crtc and encoder back off if necessary */
@@ -6629,27 +6721,12 @@ intel_user_framebuffer_create(struct drm_device *dev,
 			      struct drm_mode_fb_cmd *mode_cmd)
 {
 	struct drm_i915_gem_object *obj;
-	struct intel_framebuffer *intel_fb;
-	int ret;
 
 	obj = to_intel_bo(drm_gem_object_lookup(dev, filp, mode_cmd->handle));
 	if (&obj->base == NULL)
 		return ERR_PTR(-ENOENT);
 
-	intel_fb = kzalloc(sizeof(*intel_fb), GFP_KERNEL);
-	if (!intel_fb) {
-		drm_gem_object_unreference_unlocked(&obj->base);
-		return ERR_PTR(-ENOMEM);
-	}
-
-	ret = intel_framebuffer_init(dev, intel_fb, mode_cmd, obj);
-	if (ret) {
-		drm_gem_object_unreference_unlocked(&obj->base);
-		kfree(intel_fb);
-		return ERR_PTR(ret);
-	}
-
-	return &intel_fb->base;
+	return intel_framebuffer_create(dev, mode_cmd, obj);
 }
 
 static const struct drm_mode_config_funcs intel_mode_funcs = {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 9409a46..0aa9d54 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -299,6 +299,7 @@ static inline void intel_wait_for_crtc_vblank_safe(struct drm_crtc *crtc)
 extern void intel_wait_for_pipe_off(struct drm_device *dev, int pipe);
 
 struct intel_load_detect_pipe {
+	struct drm_framebuffer *release_fb;
 	bool load_detect_temp;
 	int dpms_mode;
 };
-- 
1.7.4.1

When in doubt, use a temporary fb

[Chris Wilson]

I've had the opportunity to test this now on my 915GM. Lo and behold,
the very first thing it did was report a PGTBL_ER. Some head scratching
later, it became clear that we were never actually disabling the pipe
after load detection, leaving the plane pointing off into never-never
land.
-Chris

[PATCH 1/3] drm/i915: Remove dead code from intel_release_load_detect_pipe()

[Chris Wilson]

As we now never attempt to steal a crtc for load detection, we either
set a mode on a new pipe, or change the dpms mode on an existing pipe.
Never both, so we can simplify the code slightly.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9b1a3e1..1384765 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5635,16 +5635,14 @@ void intel_release_load_detect_pipe(struct intel_encoder *intel_encoder,
 	struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
 
 	if (old->load_detect_temp) {
-		encoder->crtc = NULL;
 		connector->encoder = NULL;
-		crtc->enabled = drm_helper_crtc_in_use(crtc);
 		drm_helper_disable_unused_functions(dev);
+		return;
 	}
 
 	/* Switch crtc and encoder back off if necessary */
-	if (crtc->enabled && old->dpms_mode != DRM_MODE_DPMS_ON) {
-		if (encoder->crtc == crtc)
-			encoder_funcs->dpms(encoder, old->dpms_mode);
+	if (old->dpms_mode != DRM_MODE_DPMS_ON) {
+		encoder_funcs->dpms(encoder, old->dpms_mode);
 		crtc_funcs->dpms(crtc, old->dpms_mode);
 	}
 }
-- 
1.7.4.1

[PATCH 2/3] drm/i915: Attach a fb to the load-detect pipe

[Chris Wilson]

We need to ensure that we feed valid memory into the display plane
attached to the pipe when switching the pipe on. Otherwise, the display
engine may read through an invalid PTE and so throw an PGTBL_ER
exception.

As we need to perform load detection before even the first object is
allocated for the fbdev, there is no pre-existing object large enough
for us to borrow to use as the framebuffer. So we need to create one
and cleanup afterwards. At other times, the current fbcon may be large
enough for us to borrow it for duration of load detection.

Found by assert_fb_bound_for_plane().

Reported-by: Knut Petersen <Knut_Petersen@t-online.de>
References: https://bugs.freedesktop.org/show_bug.cgi?id=36246
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c |  144 ++++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_drv.h     |    1 +
 2 files changed, 128 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1384765..78c38e2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5539,6 +5539,92 @@ static struct drm_display_mode load_detect_mode = {
 		 704, 832, 0, 480, 489, 491, 520, 0, DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC),
 };
 
+static struct drm_framebuffer *
+intel_framebuffer_create(struct drm_device *dev,
+			 struct drm_mode_fb_cmd *mode_cmd,
+			 struct drm_i915_gem_object *obj)
+{
+	struct intel_framebuffer *intel_fb;
+	int ret;
+
+	intel_fb = kzalloc(sizeof(*intel_fb), GFP_KERNEL);
+	if (!intel_fb) {
+		drm_gem_object_unreference_unlocked(&obj->base);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	ret = intel_framebuffer_init(dev, intel_fb, mode_cmd, obj);
+	if (ret) {
+		drm_gem_object_unreference_unlocked(&obj->base);
+		kfree(intel_fb);
+		return ERR_PTR(ret);
+	}
+
+	return &intel_fb->base;
+}
+
+static u32
+intel_framebuffer_pitch_for_width(int width, int bpp)
+{
+	u32 pitch = DIV_ROUND_UP(width * bpp, 8);
+	return ALIGN(pitch, 64);
+}
+
+static u32
+intel_framebuffer_size_for_mode(struct drm_display_mode *mode, int bpp)
+{
+	u32 pitch = intel_framebuffer_pitch_for_width(mode->hdisplay, bpp);
+	return ALIGN(pitch * mode->vdisplay, PAGE_SIZE);
+}
+
+static struct drm_framebuffer *
+intel_framebuffer_create_for_mode(struct drm_device *dev,
+				  struct drm_display_mode *mode,
+				  int depth, int bpp)
+{
+	struct drm_i915_gem_object *obj;
+	struct drm_mode_fb_cmd mode_cmd;
+
+	obj = i915_gem_alloc_object(dev,
+				    intel_framebuffer_size_for_mode(mode, bpp));
+	if (obj == NULL)
+		return ERR_PTR(-ENOMEM);
+
+	mode_cmd.width = mode->hdisplay;
+	mode_cmd.height = mode->vdisplay;
+	mode_cmd.depth = depth;
+	mode_cmd.bpp = bpp;
+	mode_cmd.pitch = intel_framebuffer_pitch_for_width(mode_cmd.width, bpp);
+
+	return intel_framebuffer_create(dev, &mode_cmd, obj);
+}
+
+static struct drm_framebuffer *
+mode_fits_in_fbdev(struct drm_device *dev,
+		   struct drm_display_mode *mode)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_gem_object *obj;
+	struct drm_framebuffer *fb;
+
+	if (dev_priv->fbdev == NULL)
+		return NULL;
+
+	obj = dev_priv->fbdev->ifb.obj;
+	if (obj == NULL)
+		return NULL;
+
+	fb = &dev_priv->fbdev->ifb.base;
+	if (fb->pitch < intel_framebuffer_pitch_for_width(mode->hdisplay,
+							  fb->bits_per_pixel))
+		return NULL;
+
+	if (obj->base.size < mode->vdisplay * fb->pitch)
+		return NULL;
+
+	return fb;
+}
+
 bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 				struct drm_connector *connector,
 				struct drm_display_mode *mode,
@@ -5549,8 +5635,13 @@ bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 	struct drm_encoder *encoder = &intel_encoder->base;
 	struct drm_crtc *crtc = NULL;
 	struct drm_device *dev = encoder->dev;
+	struct drm_framebuffer *old_fb;
 	int i = -1;
 
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
+		      connector->base.id, drm_get_connector_name(connector),
+		      encoder->base.id, drm_get_encoder_name(encoder));
+
 	/*
 	 * Algorithm gets a little messy:
 	 *
@@ -5609,12 +5700,38 @@ bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 	intel_crtc = to_intel_crtc(crtc);
 	old->dpms_mode = intel_crtc->dpms_mode;
 	old->load_detect_temp = true;
+	old->release_fb = NULL;
 
 	if (!mode)
 		mode = &load_detect_mode;
 
-	if (!drm_crtc_helper_set_mode(crtc, mode, 0, 0, crtc->fb)) {
+	old_fb = crtc->fb;
+
+	/* We need a framebuffer large enough to accommodate all accesses
+	 * that the plane may generate whilst we perform load detection.
+	 * We can not rely on the fbcon either being present (we get called
+	 * during its initialisation to detect all boot displays, or it may
+	 * not even exist) or that it is large enough to satisfy the
+	 * requested mode.
+	 */
+	crtc->fb = mode_fits_in_fbdev(dev, mode);
+	if (crtc->fb == NULL) {
+		DRM_DEBUG_KMS("creating tmp fb for load-detection\n");
+		crtc->fb = intel_framebuffer_create_for_mode(dev, mode, 24, 32);
+		old->release_fb = crtc->fb;
+	} else
+		DRM_DEBUG_KMS("reusing fbdev for load-detection framebuffer\n");
+	if (IS_ERR(crtc->fb)) {
+		DRM_DEBUG_KMS("failed to allocate framebuffer for load-detection\n");
+		crtc->fb = old_fb;
+		return false;
+	}
+
+	if (!drm_crtc_helper_set_mode(crtc, mode, 0, 0, old_fb)) {
 		DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
+		if (old->release_fb)
+			old->release_fb->funcs->destroy(old->release_fb);
+		crtc->fb = old_fb;
 		return false;
 	}
 
@@ -5634,9 +5751,17 @@ void intel_release_load_detect_pipe(struct intel_encoder *intel_encoder,
 	struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private;
 	struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
 
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
+		      connector->base.id, drm_get_connector_name(connector),
+		      encoder->base.id, drm_get_encoder_name(encoder));
+
 	if (old->load_detect_temp) {
 		connector->encoder = NULL;
 		drm_helper_disable_unused_functions(dev);
+
+		if (old->release_fb)
+			old->release_fb->funcs->destroy(old->release_fb);
+
 		return;
 	}
 
@@ -6627,27 +6752,12 @@ intel_user_framebuffer_create(struct drm_device *dev,
 			      struct drm_mode_fb_cmd *mode_cmd)
 {
 	struct drm_i915_gem_object *obj;
-	struct intel_framebuffer *intel_fb;
-	int ret;
 
 	obj = to_intel_bo(drm_gem_object_lookup(dev, filp, mode_cmd->handle));
 	if (&obj->base == NULL)
 		return ERR_PTR(-ENOENT);
 
-	intel_fb = kzalloc(sizeof(*intel_fb), GFP_KERNEL);
-	if (!intel_fb) {
-		drm_gem_object_unreference_unlocked(&obj->base);
-		return ERR_PTR(-ENOMEM);
-	}
-
-	ret = intel_framebuffer_init(dev, intel_fb, mode_cmd, obj);
-	if (ret) {
-		drm_gem_object_unreference_unlocked(&obj->base);
-		kfree(intel_fb);
-		return ERR_PTR(ret);
-	}
-
-	return &intel_fb->base;
+	return intel_framebuffer_create(dev, mode_cmd, obj);
 }
 
 static const struct drm_mode_config_funcs intel_mode_funcs = {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 9409a46..0aa9d54 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -299,6 +299,7 @@ static inline void intel_wait_for_crtc_vblank_safe(struct drm_crtc *crtc)
 extern void intel_wait_for_pipe_off(struct drm_device *dev, int pipe);
 
 struct intel_load_detect_pipe {
+	struct drm_framebuffer *release_fb;
 	bool load_detect_temp;
 	int dpms_mode;
 };
-- 
1.7.4.1

[PATCH 3/3] drm/i915: Move the tracking of dpms_mode down into crtc enable/disable

[Chris Wilson]

As we failed to update the dpms_mode after modeset, where it is presumed
to have been changed to DRM_MODE_DPMS_ON by the upper layers the dpms state
on the crtc became inconsistent with its actual active state. This
notably confused code and left the pipe active when it was meant to be
disabled, leading to PGTBL_ER if the fb was subsequently moved.

As we use the dpms_mode state for restoring the crtc after load-detection,
we can not simply remove it in favour of simply using the active state.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c |   20 ++++++++++++++++++--
 1 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 78c38e2..e7822a6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2904,6 +2904,8 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	intel_crtc_load_lut(crtc);
 	intel_update_fbc(dev);
 	intel_crtc_update_cursor(crtc, true);
+
+	intel_crtc->dpms_mode = DRM_MODE_DPMS_ON;
 }
 
 static void ironlake_crtc_disable(struct drm_crtc *crtc)
@@ -3000,6 +3002,8 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 	intel_update_watermarks(dev);
 	intel_update_fbc(dev);
 	intel_clear_scanline_wait(dev);
+
+	intel_crtc->dpms_mode = DRM_MODE_DPMS_OFF;
 }
 
 static void ironlake_crtc_dpms(struct drm_crtc *crtc, int mode)
@@ -3052,6 +3056,9 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 	int pipe = intel_crtc->pipe;
 	int plane = intel_crtc->plane;
 
+	DRM_DEBUG_KMS("[CRTC:%d] active: %d\n",
+		      crtc->base.id, intel_crtc->active);
+
 	if (intel_crtc->active)
 		return;
 
@@ -3068,6 +3075,8 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 	/* Give the overlay scaler a chance to enable if it's on this pipe */
 	intel_crtc_dpms_overlay(intel_crtc, true);
 	intel_crtc_update_cursor(crtc, true);
+
+	intel_crtc->dpms_mode = DRM_MODE_DPMS_ON;
 }
 
 static void i9xx_crtc_disable(struct drm_crtc *crtc)
@@ -3078,6 +3087,9 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	int pipe = intel_crtc->pipe;
 	int plane = intel_crtc->plane;
 
+	DRM_DEBUG_KMS("[CRTC:%d] active: %d\n",
+		      crtc->base.id, intel_crtc->active);
+
 	if (!intel_crtc->active)
 		return;
 
@@ -3099,6 +3111,8 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	intel_update_fbc(dev);
 	intel_update_watermarks(dev);
 	intel_clear_scanline_wait(dev);
+
+	intel_crtc->dpms_mode = DRM_MODE_DPMS_OFF;
 }
 
 static void i9xx_crtc_dpms(struct drm_crtc *crtc, int mode)
@@ -3130,11 +3144,13 @@ static void intel_crtc_dpms(struct drm_crtc *crtc, int mode)
 	int pipe = intel_crtc->pipe;
 	bool enabled;
 
+	DRM_DEBUG_KMS("[CRTC:%d] current dpms %d, new %d\n",
+		      crtc->base.id,
+		      intel_crtc->dpms_mode, mode);
+
 	if (intel_crtc->dpms_mode == mode)
 		return;
 
-	intel_crtc->dpms_mode = mode;
-
 	dev_priv->display.dpms(crtc, mode);
 
 	if (!dev->primary->master)
-- 
1.7.4.1