[PATCH] Allocate a correctly sized framebuffer when tiling by using libdrm's support.

[PATCH] Allocate a correctly sized framebuffer when tiling by using libdrm's support.

[Eric Anholt]

When I made libdrm stop overallocating so much memory for the purpose
of bo caching, things started scribbling on the bottom of my
frontbuffer (and vice versa, leading to GPU hangs).  We had the usual
mistake of size = tiled_pitch * height instead of size = tiled_pitch *
tile_aligned_height.
---
 src/drmmode_display.c |   11 +++---
 src/i830.h            |    2 -
 src/i830_driver.c     |   65 ++++----------------------------------
 src/i830_memory.c     |   82 +++++++++++--------------------------------------
 4 files changed, 30 insertions(+), 130 deletions(-)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 3ea9b0f..df0282e 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -1263,11 +1263,6 @@ drmmode_xf86crtc_resize (ScrnInfoPtr scrn, int width, int height)
 		return TRUE;
 
 	w = i830_pad_drawable_width(width);
-	i830_tiled_width(intel, &w, intel->cpp);
-	pitch = w * intel->cpp;
-	xf86DrvMsg(scrn->scrnIndex, X_INFO,
-		   "Allocate new frame buffer %dx%d stride %d\n",
-		   width, height, pitch);
 
 	old_width = scrn->virtualX;
 	old_height = scrn->virtualY;
@@ -1277,10 +1272,14 @@ drmmode_xf86crtc_resize (ScrnInfoPtr scrn, int width, int height)
 
 	scrn->virtualX = width;
 	scrn->virtualY = height;
-	scrn->displayWidth = w;
 	intel->front_buffer = i830_allocate_framebuffer(scrn);
 	if (!intel->front_buffer)
 		goto fail;
+	pitch = scrn->displayWidth * intel->cpp;
+
+	xf86DrvMsg(scrn->scrnIndex, X_INFO,
+		   "Allocated new frame buffer %dx%d stride %d\n",
+		   width, height, scrn->displayWidth * intel->cpp);
 
 	ret = drmModeAddFB(drmmode->fd, width, height, scrn->depth,
 			   scrn->bitsPerPixel, pitch,
diff --git a/src/i830.h b/src/i830.h
index 8c96841..07fbbaf 100644
--- a/src/i830.h
+++ b/src/i830.h
@@ -459,8 +459,6 @@ extern Bool I830AccelInit(ScreenPtr pScreen);
 void i830_reset_allocations(ScrnInfoPtr scrn);
 void i830_init_bufmgr(ScrnInfoPtr scrn);
 
-Bool i830_tiled_width(intel_screen_private *intel, int *width, int cpp);
-
 /* i830_memory.c */
 unsigned long i830_get_fence_size(intel_screen_private *intel, unsigned long size);
 unsigned long i830_get_fence_pitch(intel_screen_private *intel, unsigned long pitch,
diff --git a/src/i830_driver.c b/src/i830_driver.c
index 89a4525..c3f3062 100644
--- a/src/i830_driver.c
+++ b/src/i830_driver.c
@@ -348,49 +348,6 @@ static void PreInitCleanup(ScrnInfoPtr scrn)
 }
 
 /*
- * Adjust *width to allow for tiling if possible
- */
-Bool i830_tiled_width(intel_screen_private *intel, int *width, int cpp)
-{
-	Bool tiled = FALSE;
-
-	/*
-	 * Adjust the display width to allow for front buffer tiling if possible
-	 */
-	if (intel->tiling) {
-		if (IS_I965G(intel)) {
-			int tile_pixels = 512 / cpp;
-			*width = (*width + tile_pixels - 1) &
-			    ~(tile_pixels - 1);
-			tiled = TRUE;
-		} else {
-			/* Good pitches to allow tiling.  Don't care about pitches < 1024
-			 * pixels.
-			 */
-			static const int pitches[] = {
-				1024,
-				2048,
-				4096,
-				8192,
-				0
-			};
-			int pitch;
-			int i;
-
-			pitch = *width * cpp;
-			for (i = 0; pitches[i] != 0; i++) {
-				if (pitches[i] >= pitch) {
-					*width = pitches[i] / cpp;
-					tiled = TRUE;
-					break;
-				}
-			}
-		}
-	}
-	return tiled;
-}
-
-/*
  * DRM mode setting Linux only at this point... later on we could
  * add a wrapper here.
  */
@@ -1006,31 +963,23 @@ static Bool i830_try_memory_allocation(ScrnInfoPtr scrn)
 static Bool i830_memory_init(ScrnInfoPtr scrn)
 {
 	intel_screen_private *intel = intel_get_screen_private(scrn);
-	int savedDisplayWidth = scrn->displayWidth;
-	Bool tiled = FALSE;
-
-	tiled = i830_tiled_width(intel, &scrn->displayWidth, intel->cpp);
 
 	xf86DrvMsg(scrn->scrnIndex,
 		   intel->pEnt->device->videoRam ? X_CONFIG : X_DEFAULT,
 		   "VideoRam: %d KB\n", scrn->videoRam);
 
+	if (i830_try_memory_allocation(scrn))
+		return TRUE;
+
 	/* Tiled first if we got a good displayWidth */
-	if (tiled) {
+	if (intel->tiling) {
+		intel->tiling = FALSE;
+		i830_reset_allocations(scrn);
+
 		if (i830_try_memory_allocation(scrn))
 			return TRUE;
-		else {
-			i830_reset_allocations(scrn);
-			intel->tiling = FALSE;
-		}
 	}
 
-	/* If tiling fails we have to disable FBC */
-	scrn->displayWidth = savedDisplayWidth;
-
-	if (i830_try_memory_allocation(scrn))
-		return TRUE;
-
 	return FALSE;
 }
 
diff --git a/src/i830_memory.c b/src/i830_memory.c
index 611b548..e71cbde 100644
--- a/src/i830_memory.c
+++ b/src/i830_memory.c
@@ -177,39 +177,6 @@ void i830_reset_allocations(ScrnInfoPtr scrn)
 	intel->front_buffer = NULL;
 }
 
-static Bool IsTileable(ScrnInfoPtr scrn, int pitch)
-{
-	intel_screen_private *intel = intel_get_screen_private(scrn);
-
-	if (IS_I965G(intel)) {
-		if (pitch / 512 * 512 == pitch && pitch <= KB(128))
-			return TRUE;
-		else
-			return FALSE;
-	}
-
-	/*
-	 * Allow tiling for pitches that are a power of 2 multiple of 128 bytes,
-	 * up to 64 * 128 (= 8192) bytes.
-	 */
-	switch (pitch) {
-	case 128:
-	case 256:
-		if (IS_I945G(intel) || IS_I945GM(intel) || IS_G33CLASS(intel))
-			return TRUE;
-		else
-			return FALSE;
-	case 512:
-	case KB(1):
-	case KB(2):
-	case KB(4):
-	case KB(8):
-		return TRUE;
-	default:
-		return FALSE;
-	}
-}
-
 /**
  * Allocates a framebuffer for a screen.
  *
@@ -220,51 +187,38 @@ drm_intel_bo *i830_allocate_framebuffer(ScrnInfoPtr scrn)
 {
 	intel_screen_private *intel = intel_get_screen_private(scrn);
 	drm_intel_bo *front_buffer;
-	long size, fb_height;
-	unsigned int pitch;
-	uint32_t tiling_mode, requested_tiling_mode;
-	int ret;
+	unsigned long pitch;
+	uint32_t tiling_mode;
 
-	/* We'll allocate the fb such that the root window will fit regardless of
-	 * rotation.
-	 */
-	fb_height = scrn->virtualY;
-
-	pitch = scrn->displayWidth * intel->cpp;
-	size = ROUND_TO_PAGE(pitch * fb_height);
-
-	if (intel->tiling && IsTileable(scrn, pitch))
+	if (intel->tiling)
 		tiling_mode = I915_TILING_X;
 	else
 		tiling_mode = I915_TILING_NONE;
 
-	if (!i830_check_display_stride(scrn, pitch,
-				       tiling_mode != I915_TILING_NONE)) {
+	front_buffer = drm_intel_bo_alloc_tiled(intel->bufmgr, "front buffer",
+						scrn->displayWidth,
+						scrn->virtualY, intel->cpp,
+						&tiling_mode, &pitch, 0);
+	if (front_buffer == NULL) {
 		xf86DrvMsg(scrn->scrnIndex, X_ERROR,
-			   "Front buffer stride %d kB "
-			   "exceed display limit\n", pitch / 1024);
+			   "Failed to allocate framebuffer.\n");
 		return NULL;
 	}
 
-	if (tiling_mode != I915_TILING_NONE) {
-		/* round to size necessary for the fence register to work */
-		size = i830_get_fence_size(intel, size);
-	}
-
-	front_buffer = drm_intel_bo_alloc(intel->bufmgr, "front buffer",
-					  size, GTT_PAGE_SIZE);
-	if (front_buffer == NULL) {
+	if (!i830_check_display_stride(scrn, pitch,
+				       tiling_mode != I915_TILING_NONE)) {
 		xf86DrvMsg(scrn->scrnIndex, X_ERROR,
-			   "Failed to allocate framebuffer.\n");
+			   "Front buffer stride %ld kB "
+			   "exceeds display limit\n", pitch / 1024);
+		drm_intel_bo_unreference(front_buffer);
 		return NULL;
 	}
 
-	requested_tiling_mode = tiling_mode;
-	ret = drm_intel_bo_set_tiling(front_buffer, &tiling_mode, pitch);
-	if (ret != 0 || tiling_mode != requested_tiling_mode) {
+	scrn->displayWidth = pitch / intel->cpp;
+
+	if (intel->tiling && tiling_mode != I915_TILING_X) {
 		xf86DrvMsg(scrn->scrnIndex, X_ERROR,
-			   "Failed to set tiling on frontbuffer: %s\n",
-			   ret == 0 ? "rejected by kernel" : strerror(-ret));
+			   "Failed to set tiling on frontbuffer.\n");
 	}
 
 	drm_intel_bo_disable_reuse(front_buffer);
-- 
1.7.1

Re: [PATCH] Allocate a correctly sized framebuffer when tiling by using libdrm support.

[Chris Wilson]

On Sun,  6 Jun 2010 23:49:09 -0700, Eric Anholt <eric@anholt.net> wrote:
> When I made libdrm stop overallocating so much memory for the purpose
> of bo caching, things started scribbling on the bottom of my
> frontbuffer (and vice versa, leading to GPU hangs).  We had the usual
> mistake of size = tiled_pitch * height instead of size = tiled_pitch *
> tile_aligned_height.

Hmm, something is fishy after applying the patch. The framebuffer pitch
for the 1920x1080 external display looks to be off by 2.
-ickle

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] Allocate a correctly sized framebuffer when tiling by using libdrm support.

[Chris Wilson]

On Sun,  6 Jun 2010 23:49:09 -0700, Eric Anholt <eric@anholt.net> wrote:
> When I made libdrm stop overallocating so much memory for the purpose
> +	front_buffer = drm_intel_bo_alloc_tiled(intel->bufmgr, "front buffer",
> +						scrn->displayWidth,
> +						scrn->virtualY, intel->cpp,
> +						&tiling_mode, &pitch, 0);

Using displayWidth here instead of virtualX. Makes xrandr much more
exciting than normal!

I think you can go much further with your cleanup, as using displayWidth
is now almost always a bug and we want to enable/disable frontbuffer
tiling depending upon the allocation size and so need to retry when
changing mode in case the frontbuffer can only be supported with an untiled
allocation.

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index df0282e..c61c6d9 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -318,9 +318,9 @@ drmmode_set_mode_major(xf86CrtcPtr crtc, DisplayModePtr mode,
 	int i;
 	int fb_id;
 	drmModeModeInfo kmode;
-	unsigned int pitch = scrn->displayWidth * intel->cpp;
 
 	if (drmmode->fb_id == 0) {
+		unsigned int pitch = scrn->displayWidth * intel->cpp;
 		ret = drmModeAddFB(drmmode->fd,
 				   scrn->virtualX, scrn->virtualY,
 				   scrn->depth, scrn->bitsPerPixel,
@@ -1257,13 +1257,11 @@ drmmode_xf86crtc_resize (ScrnInfoPtr scrn, int width, int height)
 	ScreenPtr   screen = screenInfo.screens[scrn->scrnIndex];
 	PixmapPtr   pixmap;
 	uint32_t    old_fb_id;
-	int	    i, w, pitch, old_width, old_height, old_pitch;
+	int	    i, pitch, old_width, old_height, old_pitch;
 
 	if (scrn->virtualX == width && scrn->virtualY == height)
 		return TRUE;
 
-	w = i830_pad_drawable_width(width);
-
 	old_width = scrn->virtualX;
 	old_height = scrn->virtualY;
 	old_pitch = scrn->displayWidth;
@@ -1279,7 +1277,7 @@ drmmode_xf86crtc_resize (ScrnInfoPtr scrn, int width, int height)
 
 	xf86DrvMsg(scrn->scrnIndex, X_INFO,
 		   "Allocated new frame buffer %dx%d stride %d\n",
-		   width, height, scrn->displayWidth * intel->cpp);
+		   width, height, pitch);
 
 	ret = drmModeAddFB(drmmode->fd, width, height, scrn->depth,
 			   scrn->bitsPerPixel, pitch,
diff --git a/src/i830_driver.c b/src/i830_driver.c
index 016a781..accc547 100644
--- a/src/i830_driver.c
+++ b/src/i830_driver.c
@@ -929,28 +929,6 @@ static void i830_fixup_mtrrs(ScrnInfoPtr scrn)
 #endif
 }
 
-static Bool i830_try_memory_allocation(ScrnInfoPtr scrn)
-{
-	intel_screen_private *intel = intel_get_screen_private(scrn);
-	Bool tiled = intel->tiling;
-
-	xf86DrvMsg(scrn->scrnIndex, X_INFO,
-		   "Attempting memory allocation with %stiled buffers.\n",
-		   tiled ? "" : "un");
-
-	intel->front_buffer = i830_allocate_framebuffer(scrn);
-	if (!intel->front_buffer) {
-		xf86DrvMsg(scrn->scrnIndex, X_INFO,
-			   "%siled allocation failed.\n",
-			   tiled ? "T" : "Unt");
-		return FALSE;
-	}
-
-	xf86DrvMsg(scrn->scrnIndex, X_INFO, "%siled allocation successful.\n",
-		   tiled ? "T" : "Unt");
-	return TRUE;
-}
-
 /*
  * Try to allocate memory in several ways:
  *  1) If direct rendering is enabled, try to allocate enough memory for tiled
@@ -968,19 +946,8 @@ static Bool i830_memory_init(ScrnInfoPtr scrn)
 		   intel->pEnt->device->videoRam ? X_CONFIG : X_DEFAULT,
 		   "VideoRam: %d KB\n", scrn->videoRam);
 
-	if (i830_try_memory_allocation(scrn))
-		return TRUE;
-
-	/* Tiled first if we got a good displayWidth */
-	if (intel->tiling) {
-		intel->tiling = FALSE;
-		i830_reset_allocations(scrn);
-
-		if (i830_try_memory_allocation(scrn))
-			return TRUE;
-	}
-
-	return FALSE;
+	intel->front_buffer = i830_allocate_framebuffer(scrn);
+	return intel->front_buffer != NULL;
 }
 
 void i830_init_bufmgr(ScrnInfoPtr scrn)
@@ -1045,8 +1012,6 @@ I830ScreenInit(int scrnIndex, ScreenPtr screen, int argc, char **argv)
 	struct pci_device *const device = intel->PciInfo;
 	int fb_bar = IS_I9XX(intel) ? 2 : 0;
 
-	scrn->displayWidth = i830_pad_drawable_width(scrn->virtualX);
-
 	/*
 	 * The "VideoRam" config file parameter specifies the maximum amount of
 	 * memory that will be used/allocated.  When not present, we allow the
diff --git a/src/i830_memory.c b/src/i830_memory.c
index e71cbde..0101af2 100644
--- a/src/i830_memory.c
+++ b/src/i830_memory.c
@@ -195,9 +195,10 @@ drm_intel_bo *i830_allocate_framebuffer(ScrnInfoPtr scrn)
 	else
 		tiling_mode = I915_TILING_NONE;
 
+retry:
 	front_buffer = drm_intel_bo_alloc_tiled(intel->bufmgr, "front buffer",
-						scrn->displayWidth,
-						scrn->virtualY, intel->cpp,
+						scrn->virtualX, scrn->virtualY,
+						intel->cpp,
 						&tiling_mode, &pitch, 0);
 	if (front_buffer == NULL) {
 		xf86DrvMsg(scrn->scrnIndex, X_ERROR,
@@ -207,10 +208,19 @@ drm_intel_bo *i830_allocate_framebuffer(ScrnInfoPtr scrn)
 
 	if (!i830_check_display_stride(scrn, pitch,
 				       tiling_mode != I915_TILING_NONE)) {
+		drm_intel_bo_unreference(front_buffer);
+
+		if (tiling_mode != I915_TILING_NONE) {
+			/* The older chips have larger support for
+			 * untiled surfaces, so try again without tiling.
+			 */
+			tiling_mode = I915_TILING_NONE;
+			goto retry;
+		}
+
 		xf86DrvMsg(scrn->scrnIndex, X_ERROR,
 			   "Front buffer stride %ld kB "
 			   "exceeds display limit\n", pitch / 1024);
-		drm_intel_bo_unreference(front_buffer);
 		return NULL;
 	}
 

-- 
Chris Wilson, Intel Open Source Technology Centre