Re: [git pull] drm: previous pull req + 1.

[Chris Wilson]

On Mon, 2009-06-22 at 21:09 +0300, Maxim Levitsky wrote:
> Nope, same thing.
> 
> I use commit 87ef92092fd092936535ba057ee19b97bb6a709a + this patch
> Note that GE doesn't hang the system when maximizing it.
> 
> It is for sure tiled textures accessed incorrectly, same behavior
> observed in many games (they still run though)

Sorry for the delay, I was travelling last week and was sure I had
nailed the problem. Aside from the missing flush on i965 when a batch
buffer might be using fenced commands, the only other issue I found was
that we did not zap the mapping range on clear - which could also cause
tiling errors if textures were being written via a GTT mmap.

So please can you try this patch:

>From 20b7c9322914213bb589035afbbc03faf1ae3bf0 Mon Sep 17 00:00:00 2001
From: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Date: Mon, 29 Jun 2009 08:45:31 +0100
Subject: [PATCH] drm/i915: Remove mappings on clearing fence register

As the fence register is not locked whilst the user has mmaped the buffer
through the GTT, in order for the buffer to reacquire a fence register we
need to cause a fresh page-fault on the next user access. In order to
cause the page fault, we zap the current mapping on clearing the register.
We also ensure that all potential outstanding access via the fence
register is flushed before release as well.

Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
---
 drivers/gpu/drm/i915/i915_gem.c |   41 ++++++++++++++++++--------------------
 1 files changed, 19 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 685a876..6dc74c8 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1946,12 +1946,6 @@ i915_gem_object_unbind(struct drm_gem_object *obj)
 		obj_priv->agp_mem = NULL;
 	}
 
-
-	/* blow away mappings if mapped through GTT */
-	if (obj_priv->mmap_offset && dev->dev_mapping)
-		unmap_mapping_range(dev->dev_mapping,
-				    obj_priv->mmap_offset, obj->size, 1);
-
 	i915_gem_object_put_pages(obj);
 	BUG_ON(obj_priv->pages);
 
@@ -2350,8 +2344,7 @@ try_again:
 			if (old_obj_priv->pin_count)
 				continue;
 
-			/* i915 uses fences for GPU access to tiled buffers */
-			if (IS_I965G(dev) || !old_obj_priv->active)
+			if (!old_obj_priv->active)
 				break;
 
 			/* find the seqno of the first available fence */
@@ -2440,6 +2433,8 @@ i915_gem_clear_fence_reg(struct drm_gem_object *obj)
 		 obj_priv->gtt_offset, obj->size);
 #endif
 
+	BUG_ON(obj_priv->active);
+
 	if (IS_I965G(dev))
 		I915_WRITE64(FENCE_REG_965_0 + (obj_priv->fence_reg * 8), 0);
 	else {
@@ -2456,6 +2451,11 @@ i915_gem_clear_fence_reg(struct drm_gem_object *obj)
 
 	dev_priv->fence_regs[obj_priv->fence_reg].obj = NULL;
 	obj_priv->fence_reg = I915_FENCE_REG_NONE;
+
+	/* blow away mappings if mapped through GTT */
+	if (obj_priv->mmap_offset && dev->dev_mapping)
+		unmap_mapping_range(dev->dev_mapping,
+				    obj_priv->mmap_offset, obj->size, 1);
 }
 
 /**
@@ -2469,27 +2469,24 @@ i915_gem_clear_fence_reg(struct drm_gem_object *obj)
 int
 i915_gem_object_put_fence_reg(struct drm_gem_object *obj)
 {
-	struct drm_device *dev = obj->dev;
 	struct drm_i915_gem_object *obj_priv = obj->driver_private;
+	int ret;
 
 	if (obj_priv->fence_reg == I915_FENCE_REG_NONE)
 		return 0;
 
-	/* On the i915, GPU access to tiled buffers is via a fence,
-	 * therefore we must wait for any outstanding access to complete
-	 * before clearing the fence.
+	/* If there is outstanding activity on the buffer whilst it holds
+	 * a fence register we must assume that it requires that fence for
+	 * correct operation. Therefore we must wait for any outstanding
+	 * access to complete before clearing the fence.
 	 */
-	if (!IS_I965G(dev)) {
-		int ret;
-
-		i915_gem_object_flush_gpu_write_domain(obj);
-		i915_gem_object_flush_gtt_write_domain(obj);
-		ret = i915_gem_object_wait_rendering(obj);
-		if (ret != 0)
-			return ret;
-	}
+	i915_gem_object_flush_gpu_write_domain(obj);
+	i915_gem_object_flush_gtt_write_domain(obj);
+	ret = i915_gem_object_wait_rendering(obj);
+	if (ret != 0)
+		return ret;
 
-	i915_gem_clear_fence_reg (obj);
+	i915_gem_clear_fence_reg(obj);
 
 	return 0;
 }
-- 
1.6.3.1



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/