drm/i915: don't do allocate_va_range again on pin update

Commit Message

Chris Wilson May 12, 2017, 9:14 a.m. UTC

From: Matthew Auld <[e-mail]>

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 <[e-mail]>
Cc: Chris Wilson <[e-mail]>
Cc: Joonas Lahtinen <[e-mail]>
Reviewed-by: Chris Wilson <[e-mail]>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Comments

Chris Wilson May 12, 2017, 9:31 a.m. UTC | #1

On Fri, May 12, 2017 at 10:14:23AM +0100, Chris Wilson wrote:
> From: Matthew Auld <[e-mail]>
> 
> 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 <[e-mail]>
> Cc: Chris Wilson <[e-mail]>
> Cc: Joonas Lahtinen <[e-mail]>
> Reviewed-by: Chris Wilson <[e-mail]>

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

Matthew Auld May 12, 2017, 10:58 a.m. UTC | #2

On 12 May 2017 at 10:31, Chris Wilson <[e-mail]> wrote:
> On Fri, May 12, 2017 at 10:14:23AM +0100, Chris Wilson wrote:
>> From: Matthew Auld <[e-mail]>
>>
>> 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 <[e-mail]>
>> Cc: Chris Wilson <[e-mail]>
>> Cc: Joonas Lahtinen <[e-mail]>
>> Reviewed-by: Chris Wilson <[e-mail]>
>
> 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
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Chris Wilson May 12, 2017, 12:39 p.m. UTC | #3

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

Patch

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);