Message ID | 20170512091423.26085-1-chris@chris-wilson.co.uk (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Fri, May 12, 2017 at 10:14:23AM +0100, Chris Wilson wrote: > From: Matthew Auld <matthew.auld@intel.com> > > If a vma is already bound to a ppgtt, we incorrectly call > allocate_va_range again when doing a PIN_UPDATE, which will result in > over accounting within our paging structures, such that when we do > unbind something we don't actually destroy the structures and end up > inadvertently recycling them. In reality this probably isn't too bad, > but once we start touching PDEs and PDPEs for 64K/2M/1G pages this > apparent recycling will manifest into lots of really, really subtle > bugs. > > v2: Fix the testing of vma->flags for aliasing_ppgtt_bind_vma > > Fixes: ff685975d97f ("drm/i915: Move allocate_va_range to GTT") > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> So, we are missing coverage of PIN_UPDATE and set-cache-level from the kselftests. Ideas? Matthew, do you have any clue how to reproduce those subtle errors? -Chris
On 12 May 2017 at 10:31, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Fri, May 12, 2017 at 10:14:23AM +0100, Chris Wilson wrote: >> From: Matthew Auld <matthew.auld@intel.com> >> >> If a vma is already bound to a ppgtt, we incorrectly call >> allocate_va_range again when doing a PIN_UPDATE, which will result in >> over accounting within our paging structures, such that when we do >> unbind something we don't actually destroy the structures and end up >> inadvertently recycling them. In reality this probably isn't too bad, >> but once we start touching PDEs and PDPEs for 64K/2M/1G pages this >> apparent recycling will manifest into lots of really, really subtle >> bugs. >> >> v2: Fix the testing of vma->flags for aliasing_ppgtt_bind_vma >> >> Fixes: ff685975d97f ("drm/i915: Move allocate_va_range to GTT") >> Signed-off-by: Matthew Auld <matthew.auld@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > So, we are missing coverage of PIN_UPDATE and set-cache-level from the > kselftests. Ideas? > > Matthew, do you have any clue how to reproduce those subtle errors? The errors I encountered were only visible with huge-pages, for example hitting the 4K PTE insertion path while the PDE still points to a stale 2M page and not a page-table. I wanted to add a GEM_BUG_ON(pt->used_ptes > GEN8_PTES) as part of this patch but the appgtt would make that a pain iiuc. Although I don't really understand why we bother with allocate_va_range/clear_range for the appgtt bind/unbind given that we preallocate it anyway... > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, May 12, 2017 at 11:01:02AM -0000, Patchwork wrote: > == Series Details == > > Series: drm/i915: don't do allocate_va_range again on pin update > URL : https://patchwork.freedesktop.org/series/24342/ > State : warning > > == Summary == > > Series 24342v1 drm/i915: don't do allocate_va_range again on pin update > https://patchwork.freedesktop.org/api/1.0/series/24342/revisions/1/mbox/ > > Test gem_exec_suspend: > Subgroup basic-s4-devices: > pass -> DMESG-WARN (fi-kbl-7560u) fdo#100125 > Test kms_force_connector_basic: > Subgroup prune-stale-modes: > pass -> SKIP (fi-snb-2520m) > > fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125 Thanks for the fix Matthew, pushed it to dinq so we can get into the fixes queue promptly. -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 62871cd50605..6354fe78238f 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -193,9 +193,12 @@ static int ppgtt_bind_vma(struct i915_vma *vma, u32 pte_flags; int ret; - ret = vma->vm->allocate_va_range(vma->vm, vma->node.start, vma->size); - if (ret) - return ret; + if (!(vma->flags & I915_VMA_LOCAL_BIND)) { + ret = vma->vm->allocate_va_range(vma->vm, vma->node.start, + vma->size); + if (ret) + return ret; + } vma->pages = vma->obj->mm.pages; @@ -2304,7 +2307,8 @@ static int aliasing_gtt_bind_vma(struct i915_vma *vma, if (flags & I915_VMA_LOCAL_BIND) { struct i915_hw_ppgtt *appgtt = i915->mm.aliasing_ppgtt; - if (appgtt->base.allocate_va_range) { + if (!(vma->flags & I915_VMA_LOCAL_BIND) && + appgtt->base.allocate_va_range) { ret = appgtt->base.allocate_va_range(&appgtt->base, vma->node.start, vma->node.size);