Modesetting fixes for pre-ILK, 915GM especially

Modesetting fixes for pre-ILK, 915GM especially

[Chris Wilson]

I revised the diagnostic patch (assert_fb_bound_for_plane) so that the
function comment actually matches what the function does and ran it on a
couple of machines whilst bug hunting. It scored 2 direct hits. Not only
does intel_get_load_detect_pipe() not attach a framebuffer when enabling
an unused pipe, but the intel_crtc_mode_set() enables the plane before
setting up the memory for the plane to read from.

The latter is quite a scary bug. Can anyone remember if there was a
reason for it to be so?
-Chris

[PATCH 1/3] drm/i915: Check that the plane points to the pipe's framebuffer before enabling

[Chris Wilson]

Knut Petersen reported a GPU hang when he left x11perf running
overnight. The error state quite clearly indicates that plane A was
enabled without being fully setup:

PGTBL_ER: 0x00000010
    Display A: Invalid GTT PTE
Plane [0]:
  CNTR: c1000000
  STRIDE: 00000c80
  SIZE: 03ff04ff
  POS: 00000000
  ADDR: 00000000

[That GTT offset on his system being pinned for the ringbuffer.]

This is a simple debugging patch to assert that this cannot be so!

References: https://bugs.freedesktop.org/show_bug.cgi?id=36246
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_display.c |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 73cec21..b7d63a5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1560,6 +1560,34 @@ static void intel_disable_pipe(struct drm_i915_private *dev_priv,
 	intel_wait_for_pipe_off(dev_priv->dev, pipe);
 }
 
+/* Check that the display plane points to the pipe's framebuffer. */
+static void assert_fb_bound_to_plane(struct drm_i915_private *dev_priv,
+				     enum pipe pipe, enum plane plane)
+{
+	struct drm_crtc *crtc;
+	struct intel_framebuffer *intel_fb;
+	u32 val, base, size;
+
+	crtc = intel_get_crtc_for_pipe(dev_priv->dev, pipe);
+	if (WARN(crtc->fb == NULL,
+		 "no framebuffer attached to pipe %c\n",
+		 pipe_name(pipe)))
+		return;
+
+	intel_fb = to_intel_framebuffer(crtc->fb);
+	base = intel_fb->obj->gtt_offset;
+	size = intel_fb->obj->base.size;
+
+	if (dev_priv->info->gen >= 4)
+		val = I915_READ(DSPSURF(plane));
+	else
+		val = I915_READ(DSPADDR(plane));
+	WARN(val < base || val >= base + size,
+	     "mismatching framebuffer for plane %c attached to pipe %c, expected %x-%x found %x\n",
+	     plane_name(plane), pipe_name(pipe),
+	     base, base + size, val);
+}
+
 /**
  * intel_enable_plane - enable a display plane on a given pipe
  * @dev_priv: i915 private structure
@@ -1576,6 +1604,7 @@ static void intel_enable_plane(struct drm_i915_private *dev_priv,
 
 	/* If the pipe isn't enabled, we can't pump pixels and may hang */
 	assert_pipe_enabled(dev_priv, pipe);
+	assert_fb_bound_to_plane(dev_priv, pipe, plane);
 
 	reg = DSPCNTR(plane);
 	val = I915_READ(reg);
-- 
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.

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_crt.c     |   24 ++---
 drivers/gpu/drm/i915/intel_display.c |  182 +++++++++++++++++++---------------
 drivers/gpu/drm/i915/intel_drv.h     |   25 +++--
 drivers/gpu/drm/i915/intel_tv.c      |   11 +--
 4 files changed, 132 insertions(+), 110 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index d03fc05..2a32405 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;
@@ -432,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,17 +451,18 @@ 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) {
+		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(crtc, crt);
-			intel_release_load_detect_pipe(&crt->base,
-						       connector, dpms_mode);
+				status = intel_crt_load_detect(crt);
+			intel_release_load_detect_pipe(&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 b7d63a5..b9bb20d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5541,115 +5541,133 @@ 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 *encoder,
+				struct drm_connector *connector,
+				struct drm_display_mode *mode,
+				struct intel_load_detect_pipe *old)
 {
-	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;
-	struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private;
-	struct drm_crtc_helper_funcs *crtc_funcs;
-	int i = -1;
-
-	/*
-	 * 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
-	 */
+	struct drm_device *dev = encoder->base.dev;
+	struct drm_crtc *crtc;
+	struct drm_framebuffer *fb, *old_fb;
+	int x, y;
 
 	/* 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;
-		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 crtc;
-	}
+	crtc = NULL;
+	if (encoder->base.crtc)
+		crtc = encoder->base.crtc;
 
-	/* Find an unused one (if possible) */
-	list_for_each_entry(possible_crtc, &dev->mode_config.crtc_list, head) {
-		i++;
-		if (!(encoder->possible_crtcs & (1 << i)))
-			continue;
-		if (!possible_crtc->enabled) {
-			crtc = possible_crtc;
-			break;
+	if (crtc == NULL) {
+		struct drm_crtc *pos;
+		int i = 0;
+
+		/* Find an unused one (if possible) */
+		list_for_each_entry(pos, &dev->mode_config.crtc_list, head) {
+			if (!(encoder->base.possible_crtcs & (1 << i++)))
+				continue;
+
+			if (!pos->enabled) {
+				crtc = pos;
+				break;
+			}
 		}
-		if (!supported_crtc)
-			supported_crtc = possible_crtc;
 	}
 
 	/*
 	 * If we didn't find an unused CRTC, don't use any.
 	 */
-	if (!crtc) {
-		return NULL;
-	}
+	if (crtc == NULL)
+		return false;
 
-	encoder->crtc = crtc;
-	connector->encoder = encoder;
-	intel_encoder->load_detect_temp = true;
+	/* Reuse the existing crtc->fb unless absent */
+	old_fb = crtc->fb;
+	if (old_fb == NULL || !crtc->enabled) {
+		struct drm_i915_private *dev_priv = dev->dev_private;
+		struct intel_fbdev *ifbdev = dev_priv->fbdev;
 
-	intel_crtc = to_intel_crtc(crtc);
-	*dpms_mode = intel_crtc->dpms_mode;
+		fb = &ifbdev->ifb.base;
+		x = y = 0;
 
-	if (!crtc->enabled) {
 		if (!mode)
 			mode = &load_detect_mode;
-		drm_crtc_helper_set_mode(crtc, mode, 0, 0, crtc->fb);
 	} else {
-		if (intel_crtc->dpms_mode != DRM_MODE_DPMS_ON) {
-			crtc_funcs = crtc->helper_private;
-			crtc_funcs->dpms(crtc, DRM_MODE_DPMS_ON);
-		}
+		fb = crtc->fb;
+		x = crtc->x;
+		y = crtc->y;
+		mode = &crtc->mode;
+	}
+
+	/* We may be called during ifbdev init, before the fb is bound */
+	if (fb == NULL || to_intel_framebuffer(fb)->obj == NULL)
+		return false;
+
+	/* Save the current configuration to be restored after load-detect */
+	old->connector = connector;
+	old->encoder = connector->encoder;
+	old->crtc = connector->encoder ? connector->encoder->crtc : NULL;
+	if (old->crtc) {
+		struct intel_crtc *intel_crtc;
 
-		/* Add this connector to the crtc */
-		encoder_funcs->mode_set(encoder, &crtc->mode, &crtc->mode);
-		encoder_funcs->commit(encoder);
+		intel_crtc = to_intel_crtc(connector->encoder->crtc);
+		old->crtc = &intel_crtc->base;
+		old->dpms_mode = intel_crtc->dpms_mode;
+		old->fb = intel_crtc->base.fb;
+		old->mode = intel_crtc->base.mode;
+		old->x = intel_crtc->base.x;
+		old->y = intel_crtc->base.y;
 	}
+
+	/* Set the new connector->encoder->crtc->fb configuration */
+	crtc->fb = fb;
+	encoder->base.crtc = crtc;
+	connector->encoder = &encoder->base;
+
+	if (!drm_crtc_helper_set_mode(crtc, mode, x, y, old_fb)) {
+		DRM_DEBUG_KMS("failed to set mode for load-detect\n");
+
+		old->connector->encoder = old->encoder;
+		old->encoder->crtc = old->crtc;
+		old->crtc->fb = old->fb;
+
+		crtc->fb = old_fb;
+
+		drm_helper_disable_unused_functions(dev);
+		return false;
+	}
+	connector->dpms = DRM_MODE_DPMS_ON;
+
 	/* let the connector get through one full cycle before testing */
-	intel_wait_for_vblank(dev, intel_crtc->pipe);
+	intel_wait_for_vblank(dev, to_intel_crtc(crtc)->pipe);
 
-	return crtc;
+	return true;
 }
 
-void intel_release_load_detect_pipe(struct intel_encoder *intel_encoder,
-				    struct drm_connector *connector, int dpms_mode)
+void intel_release_load_detect_pipe(struct intel_load_detect_pipe *old)
 {
-	struct drm_encoder *encoder = &intel_encoder->base;
-	struct drm_device *dev = encoder->dev;
-	struct drm_crtc *crtc = encoder->crtc;
-	struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private;
-	struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
+	struct drm_crtc *crtc = old->crtc;
+
+	old->connector->encoder = old->encoder;
+	if (old->encoder != NULL)
+		old->encoder->crtc = crtc;
+	if (crtc != NULL) {
+		struct drm_framebuffer *fb;
+
+		fb = crtc->fb;
+		crtc->fb = old->fb;
 
-	if (intel_encoder->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);
+		if (crtc->enabled) {
+			if (!drm_crtc_helper_set_mode(crtc, &old->mode,
+						      old->x, old->y, fb))
+				DRM_ERROR("failed to restore mode after load-detect\n");
+
+			old->connector = DRM_MODE_DPMS_ON;
+			if (old->dpms_mode != DRM_MODE_DPMS_ON)
+				drm_helper_connector_dpms(old->connector,
+							  old->dpms_mode);
+		}
 	}
 
-	/* Switch crtc and encoder back off if necessary */
-	if (crtc->enabled && dpms_mode != DRM_MODE_DPMS_ON) {
-		if (encoder->crtc == crtc)
-			encoder_funcs->dpms(encoder, dpms_mode);
-		crtc_funcs->dpms(crtc, dpms_mode);
-	}
+	drm_helper_disable_unused_functions(crtc->dev);
 }
 
 /* Returns the clock of the currently programmed mode of the given pipe. */
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index aeb1b98..64cfc88 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,23 @@ 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 void intel_release_load_detect_pipe(struct intel_encoder *intel_encoder,
-					   struct drm_connector *connector,
-					   int dpms_mode);
+
+struct intel_load_detect_pipe {
+	struct drm_connector *connector;
+	struct drm_encoder *encoder;
+	struct drm_crtc *crtc;
+
+	int dpms_mode;
+	struct drm_framebuffer *fb;
+	struct drm_display_mode mode;
+	int x, y;
+};
+
+extern bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
+				       struct drm_connector *connector,
+				       struct drm_display_mode *mode,
+				       struct intel_load_detect_pipe *tmp);
+extern void intel_release_load_detect_pipe(struct intel_load_detect_pipe *tmp);
 
 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 3047a66..1352acf 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1370,15 +1370,12 @@ 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;
+		struct intel_load_detect_pipe tmp;
 
-		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, &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(&tmp);
 		} else
 			return connector_status_unknown;
 	} else
-- 
1.7.4.1

[PATCH 3/3] drm/i915: Only enable the plane after setting the fb base (pre-ILK)

[Chris Wilson]

When enabling the plane, it is helpful to have already pointed that
plane to valid memory or else we may incur the wrath of a PGTBL_ER.
This code preserved the behaviour from the bad old days for unknown
reasons...

Found by assert_fb_bound_for_plane().

References: https://bugs.freedesktop.org/show_bug.cgi?id=36246
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_display.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b9bb20d..f503ad0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5212,8 +5212,6 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc,
 
 	I915_WRITE(DSPCNTR(plane), dspcntr);
 	POSTING_READ(DSPCNTR(plane));
-	if (!HAS_PCH_SPLIT(dev))
-		intel_enable_plane(dev_priv, plane, pipe);
 
 	ret = intel_pipe_set_base(crtc, x, y, old_fb);
 
-- 
1.7.4.1

Re: [PATCH 3/3] drm/i915: Only enable the plane after setting the fb base (pre-ILK)

[Jesse Barnes]

On Tue, 19 Apr 2011 21:32:03 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> When enabling the plane, it is helpful to have already pointed that
> plane to valid memory or else we may incur the wrath of a PGTBL_ER.
> This code preserved the behaviour from the bad old days for unknown
> reasons...
> 
> Found by assert_fb_bound_for_plane().
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=36246
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b9bb20d..f503ad0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5212,8 +5212,6 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc,
>  
>  	I915_WRITE(DSPCNTR(plane), dspcntr);
>  	POSTING_READ(DSPCNTR(plane));
> -	if (!HAS_PCH_SPLIT(dev))
> -		intel_enable_plane(dev_priv, plane, pipe);
>  
>  	ret = intel_pipe_set_base(crtc, x, y, old_fb);
>  

As usual, changes like this frighten me.  I think the plane enable
prior to the base set has been in there for a *long* time in various
forms.  I think Carl tried to take it out when trying to debug eDP on
ILK, but we reverted that when it broke things.

But if it works on all your test machines, I guess that's a start; it's
correct in theory at least.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 1/3] drm/i915: Check that the plane points to the pipe's framebuffer before enabling

[Jesse Barnes]

On Tue, 19 Apr 2011 21:32:01 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Knut Petersen reported a GPU hang when he left x11perf running
> overnight. The error state quite clearly indicates that plane A was
> enabled without being fully setup:
> 
> PGTBL_ER: 0x00000010
>     Display A: Invalid GTT PTE
> Plane [0]:
>   CNTR: c1000000
>   STRIDE: 00000c80
>   SIZE: 03ff04ff
>   POS: 00000000
>   ADDR: 00000000
> 
> [That GTT offset on his system being pinned for the ringbuffer.]
> 
> This is a simple debugging patch to assert that this cannot be so!
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=36246
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---

I like it.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

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

[Keith Packard]

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

On Tue, 19 Apr 2011 21:32:02 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

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

This seems to rewrite most of the load detection code, but doesn't
justify that rewrite in comments (either commit or in-line). You'll need
to break this up so that we can actually see what is changing.

-- 
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/3] drm/i915: Attach a fb to the load-detect pipe

[Chris Wilson]

On Tue, 19 Apr 2011 14:26:04 -0700, Keith Packard <keithp@keithp.com> wrote:
> On Tue, 19 Apr 2011 21:32:02 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > 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.
> 
> This seems to rewrite most of the load detection code, but doesn't
> justify that rewrite in comments (either commit or in-line). You'll need
> to break this up so that we can actually see what is changing.

I created a fresh series of patches that hopefully clarifies what needs to
be corrected in intel_get_load_detect_pipe(). It was meant to be attached
here, but I failed to drive git send-email correctly. :(
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[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