Re: [Intel-gfx] [PATCH] drm/i915: correctly restore fences with objects attached
On Wed, Jul 17, 2013 at 03:44:16PM +0100, Chris Wilson wrote: On Wed, Jul 17, 2013 at 02:51:28PM +0200, Daniel Vetter wrote: To avoid stalls we delay tiling changes and especially hold of committing the new fence state for as long as possible. Synchronization points are in the execbuf code and in our gtt fault handler. Unfortunately we've missed that tricky detail when adding proper fence restore code in commit 19b2dbde5732170a03bd82cc8bd442cf88d856f7 Author: Chris Wilson ch...@chris-wilson.co.uk Date: Wed Jun 12 10:15:12 2013 +0100 drm/i915: Restore fences after resume and GPU resets The result was that we've restored fences for objects with no tiling, since the object-fence link still existed after resume. Now that wouldn't have been too bad since any subsequent access would have fixed things up, but if we've changed from tiled to untiled real havoc happened: The tiling stride is stored -1 in the fence register, so a stride of 0 resulted in all 1s in the top 32bits, and so a completely bogus fence spanning everything from the start of the object to the top of the GTT. The tell-tale in the register dumps looks like: FENCE START 2: 0x0214d001 FENCE END 2: 0xf3ff Bit 11 isn't set since the hw doesn't store it, even when writing all 1s (at least on my snb here). To prevent such a gaffle in the future add a sanity check for fences with an untiled object attached in i915_gem_write_fence. v2: Fix the WARN, spotted by Chris. v3: Trying to reuse get_fences looked ugly and obfuscated the code. Instead reuse update_fence and to make it really dtrt also move the fence dirty state clearing into update_fence. Cc: Chris Wilson ch...@chris-wilson.co.uk Cc: Stéphane Marchesin marc...@chromium.org Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=60530 Cc: sta...@vger.kernel.org (for 3.10 only) Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch Sigh. I thought we were covered because before anything accessed this dirty object, the fence would have been rewritten. However, Daniel correctly points out that the stride==0 fence clobbers the entire GTT and so may be used by the hardware in preference to any other fence. Reviewed-by: Chris Wilson ch...@chris-wilson.co.uk Picked up for -fixes, thanks for the review. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: correctly restore fences with objects attached
To avoid stalls we delay tiling changes and especially hold of
committing the new fence state for as long as possible.
Synchronization points are in the execbuf code and in our gtt fault
handler.
Unfortunately we've missed that tricky detail when adding proper fence
restore code in
commit 19b2dbde5732170a03bd82cc8bd442cf88d856f7
Author: Chris Wilson ch...@chris-wilson.co.uk
Date: Wed Jun 12 10:15:12 2013 +0100
drm/i915: Restore fences after resume and GPU resets
The result was that we've restored fences for objects with no tiling,
since the object-fence link still existed after resume. Now that
wouldn't have been too bad since any subsequent access would have
fixed things up, but if we've changed from tiled to untiled real havoc
happened:
The tiling stride is stored -1 in the fence register, so a stride of 0
resulted in all 1s in the top 32bits, and so a completely bogus fence
spanning everything from the start of the object to the top of the
GTT. The tell-tale in the register dumps looks like:
FENCE START 2: 0x0214d001
FENCE END 2: 0xf3ff
Bit 11 isn't set since the hw doesn't store it, even when writing all
1s (at least on my snb here).
To prevent such a gaffle in the future add a sanity check for fences
with an untiled object attached in i915_gem_write_fence.
v2: Fix the WARN, spotted by Chris.
v3: Trying to reuse get_fences looked ugly and obfuscated the code.
Instead reuse update_fence and to make it really dtrt also move the
fence dirty state clearing into update_fence.
Cc: Chris Wilson ch...@chris-wilson.co.uk
Cc: Stéphane Marchesin marc...@chromium.org
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=60530
Cc: sta...@vger.kernel.org (for 3.10 only)
Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
---
drivers/gpu/drm/i915/i915_gem.c | 18 --
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c9d9d20..e2370a2 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2257,7 +2257,17 @@ void i915_gem_restore_fences(struct drm_device *dev)
for (i = 0; i dev_priv-num_fence_regs; i++) {
struct drm_i915_fence_reg *reg = dev_priv-fence_regs[i];
- i915_gem_write_fence(dev, i, reg-obj);
+
+ /*
+* Commit delayed tiling changes if we have an object still
+* attached to the fence, otherwise just clear the fence.
+*/
+ if (reg-obj) {
+ i915_gem_object_update_fence(reg-obj, reg,
+reg-obj-tiling_mode);
+ } else {
+ i915_gem_write_fence(dev, i, NULL);
+ }
}
}
@@ -2792,6 +2802,10 @@ static void i915_gem_write_fence(struct drm_device *dev,
int reg,
if (i915_gem_object_needs_mb(dev_priv-fence_regs[reg].obj))
mb();
+ WARN(obj (!obj-stride || !obj-tiling_mode),
+bogus fence setup with stride: 0x%x, tiling mode: %i\n,
+obj-stride, obj-tiling_mode);
+
switch (INTEL_INFO(dev)-gen) {
case 7:
case 6:
@@ -2833,6 +2847,7 @@ static void i915_gem_object_update_fence(struct
drm_i915_gem_object *obj,
fence-obj = NULL;
list_del_init(fence-lru_list);
}
+ obj-fence_dirty = false;
}
static int
@@ -2962,7 +2977,6 @@ i915_gem_object_get_fence(struct drm_i915_gem_object *obj)
return 0;
i915_gem_object_update_fence(obj, reg, enable);
- obj-fence_dirty = false;
return 0;
}
--
1.8.1.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: correctly restore fences with objects attached
On Wed, Jul 17, 2013 at 02:51:28PM +0200, Daniel Vetter wrote: To avoid stalls we delay tiling changes and especially hold of committing the new fence state for as long as possible. Synchronization points are in the execbuf code and in our gtt fault handler. Unfortunately we've missed that tricky detail when adding proper fence restore code in commit 19b2dbde5732170a03bd82cc8bd442cf88d856f7 Author: Chris Wilson ch...@chris-wilson.co.uk Date: Wed Jun 12 10:15:12 2013 +0100 drm/i915: Restore fences after resume and GPU resets The result was that we've restored fences for objects with no tiling, since the object-fence link still existed after resume. Now that wouldn't have been too bad since any subsequent access would have fixed things up, but if we've changed from tiled to untiled real havoc happened: The tiling stride is stored -1 in the fence register, so a stride of 0 resulted in all 1s in the top 32bits, and so a completely bogus fence spanning everything from the start of the object to the top of the GTT. The tell-tale in the register dumps looks like: FENCE START 2: 0x0214d001 FENCE END 2: 0xf3ff Bit 11 isn't set since the hw doesn't store it, even when writing all 1s (at least on my snb here). To prevent such a gaffle in the future add a sanity check for fences with an untiled object attached in i915_gem_write_fence. v2: Fix the WARN, spotted by Chris. v3: Trying to reuse get_fences looked ugly and obfuscated the code. Instead reuse update_fence and to make it really dtrt also move the fence dirty state clearing into update_fence. Cc: Chris Wilson ch...@chris-wilson.co.uk Cc: Stéphane Marchesin marc...@chromium.org Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=60530 Cc: sta...@vger.kernel.org (for 3.10 only) Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch Sigh. I thought we were covered because before anything accessed this dirty object, the fence would have been rewritten. However, Daniel correctly points out that the stride==0 fence clobbers the entire GTT and so may be used by the hardware in preference to any other fence. Reviewed-by: Chris Wilson ch...@chris-wilson.co.uk -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: correctly restore fences with objects attached
On Wed, 17 Jul 2013 14:51:28 +0200
Daniel Vetter daniel.vet...@ffwll.ch wrote:
+ if (reg-obj) {
+ i915_gem_object_update_fence(reg-obj, reg,
+ reg-obj-tiling_mode);
+ } else {
+ i915_gem_write_fence(dev, i, NULL);
+ }
Why do we look at reg-obj here? Can it be NULL? Or would
reg-obj-tiling_mode != I915_TILING_NONE do the same thing?
--
Jesse Barnes, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: correctly restore fences with objects attached
On Wed, Jul 17, 2013 at 08:34:48AM -0700, Jesse Barnes wrote:
On Wed, 17 Jul 2013 14:51:28 +0200
Daniel Vetter daniel.vet...@ffwll.ch wrote:
+ if (reg-obj) {
+ i915_gem_object_update_fence(reg-obj, reg,
+reg-obj-tiling_mode);
+ } else {
+ i915_gem_write_fence(dev, i, NULL);
+ }
Why do we look at reg-obj here? Can it be NULL? Or would
reg-obj-tiling_mode != I915_TILING_NONE do the same thing?
Yes. This is the array of fence registers, irrespective of whether an
object is using one.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
