| Message ID | 1374065488-4485-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <chris@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: 0xfffff3ff > > 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 <chris@chris-wilson.co.uk> > Cc: Stéphane Marchesin <marcheu@chromium.org> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=60530 > Cc: stable@vger.kernel.org (for 3.10 only) > Signed-off-by: Daniel Vetter <daniel.vetter@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 <chris@chris-wilson.co.uk> -Chris
On Wed, 17 Jul 2013 14:51:28 +0200 Daniel Vetter <daniel.vetter@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?
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.vetter@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
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 <chris@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: 0xfffff3ff > > > > 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 <chris@chris-wilson.co.uk> > > Cc: Stéphane Marchesin <marcheu@chromium.org> > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=60530 > > Cc: stable@vger.kernel.org (for 3.10 only) > > Signed-off-by: Daniel Vetter <daniel.vetter@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 <chris@chris-wilson.co.uk> Picked up for -fixes, thanks for the review. -Daniel
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; }
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 <chris@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: 0xfffff3ff 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 <chris@chris-wilson.co.uk> Cc: Stéphane Marchesin <marcheu@chromium.org> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=60530 Cc: stable@vger.kernel.org (for 3.10 only) Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_gem.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)