Intel-GFX Archive on lore.kernel.org
 help / color / mirror / Atom feed
* Modesetting fixes for pre-ILK, 915GM especially
@ 2011-04-19 20:32 Chris Wilson
  2011-04-19 20:32 ` [PATCH 1/3] drm/i915: Check that the plane points to the pipe's framebuffer before enabling Chris Wilson
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Chris Wilson @ 2011-04-19 20:32 UTC (permalink / raw)
  To: intel-gfx

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

^ permalink raw reply	[flat|nested] 9+ messages in thread
* [PATCH] drm/i915: Attach a fb to the load-detect pipe
@ 2011-04-20 21:43 Chris Wilson
  2011-04-21  8:45 ` When in doubt, use a temporary fb Chris Wilson
  0 siblings, 1 reply; 9+ messages in thread
From: Chris Wilson @ 2011-04-20 21:43 UTC (permalink / raw)
  To: intel-gfx

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread
end of thread, other threads:[~2011-04-21  8:45 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-04-19 20:32 Modesetting fixes for pre-ILK, 915GM especially Chris Wilson
2011-04-19 20:32 ` [PATCH 1/3] drm/i915: Check that the plane points to the pipe's framebuffer before enabling Chris Wilson
2011-04-19 20:40   ` Jesse Barnes
2011-04-19 20:32 ` [PATCH 2/3] drm/i915: Attach a fb to the load-detect pipe Chris Wilson
2011-04-19 21:26   ` Keith Packard
2011-04-20  9:25     ` Chris Wilson
2011-04-19 20:32 ` [PATCH 3/3] drm/i915: Only enable the plane after setting the fb base (pre-ILK) Chris Wilson
2011-04-19 20:39   ` Jesse Barnes
  -- strict thread matches above, loose matches on Subject: below --
2011-04-20 21:43 [PATCH] drm/i915: Attach a fb to the load-detect pipe Chris Wilson
2011-04-21  8:45 ` When in doubt, use a temporary fb Chris Wilson
2011-04-21  8:45   ` [PATCH 2/3] drm/i915: Attach a fb to the load-detect pipe Chris Wilson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox