[PATCH 00/14] Enabling GEN8 full PPGTT + fixes v2

[PATCH 00/14] Enabling GEN8 full PPGTT + fixes v2

[Michel Thierry]

This is a rebase from Ben's patches originally sent on July 1st, working in
latest drm-nightly. Below is Ben's original cover-letter.

==========
Here be all the patches to make full PPGTT relatively stable on Broadwell. Most
of the work was actually to the generic PPGTT code, and not BDW specific. There
are basically 3 fixes:

1. Make the error state not horrible, but more work still needed.
2. Fix up another tricky from != from case in do_switch
3. Generally more graceful handling in ppgtt_release

1. Various forms of this subseries have shown up on the list from both
myself, and Chris (and I feel like Mika, also). For whatever reason,
none have been merged. I don't necessarily mind missing information, but
without these patches we can OOPs and GP fault in the error state, which
is not acceptable.

2. The meat of the debugging came here. Essentially this problem has
already been seen, and solved. The issue is, there is another spot in
do_switch that can invoke a switch to the default context. We probably
want to get slightly better handling of this, but for now this solves my
problems.

3. There are 2 categories addressed in this bucket. Reset, and signals.  The
patches themselves explain the situation. I take a two step approach with this
where first I make things correct (and slow as heck), and then I do the more
optimal and trickier solution. Both of these are in line to be replaced by
Daniel, but I needed something sooner.

Here is the test case that caught all of the above issues:
while [ 1 ] ;  do
        (glxgears) & pid[0]=$!
        (glxgears) & pid[1]=$!
        (glxgears) & pid[2]=$!
        sleep 3
        kill ${pid[*]}
done

Ben Widawsky (14):
  drm/i915: Split up do_switch
  drm/i915: Extract l3 remapping out of ctx switch
  drm/i915/ppgtt: Load address space after mi_set_context
  drm/i915/ctx: Return earlier on failure
  drm/i915/error: Check the potential ctx obj's vm
  drm/i915/error: vma error capture prettyify
  drm/i915/error: Do a better job of disambiguating VMAs
  drm/i915/error: Capture vmas instead of BOs
  drm/i915: Add some extra guards in evict_vm
  drm/i915: Make an uninterruptible evict
  drm/i915: Reorder ctx unref on ppgtt cleanup
  drm/i915: More correct (slower) ppgtt cleanup
  drm/i915: Defer PPGTT cleanup
  drm/i915/bdw: Enable full PPGTT

 drivers/gpu/drm/i915/i915_drv.h            |  15 +-
 drivers/gpu/drm/i915/i915_gem.c            | 110 ++++++++++++++
 drivers/gpu/drm/i915/i915_gem_context.c    | 227 ++++++++++++++++++++++-------
 drivers/gpu/drm/i915/i915_gem_evict.c      |  39 +++--
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   2 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c        |   3 +-
 drivers/gpu/drm/i915/i915_gem_gtt.h        |   4 +
 drivers/gpu/drm/i915/i915_gpu_error.c      | 126 +++++++++-------
 8 files changed, 411 insertions(+), 115 deletions(-)

-- 
1.9.0

[PATCH 01/14] drm/i915: Split up do_switch

[Michel Thierry]

From: Ben Widawsky <benjamin.widawsky@intel.com>

There are two important reasons for this patch. It should make the
existing code a lot more readable. It also makes the next patch much
easier to understand in my opinion. There are 2 main variables that
effect this function, leaving 4 permutations:
ring: RCS vs !RCS
PPGTT: full or not

I didn't find extracting the full PPGTT usage to be very beneficial at
this point, but it may be in the future.

This was originally recommended by Daniel Vetter, and in this case, I
agree. There was no intentional behavioral change.

v2: Change the pin assertion to be GGTT only. This is more accurate.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

v3: Make it work again after legacy_hw_ctx & user_handle changes.
Signed-off-by: Michel Thierry <michel.thierry@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 79 +++++++++++++++++++++------------
 1 file changed, 51 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index de72a28..a1dc885 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -611,31 +611,58 @@ mi_set_context(struct intel_engine_cs *ring,
 	return ret;
 }
 
-static int do_switch(struct intel_engine_cs *ring,
-		     struct intel_context *to)
+static void do_switch_fini_common(struct intel_engine_cs *ring,
+				  struct intel_context *from,
+				  struct intel_context *to)
+{
+	if (likely(from))
+		i915_gem_context_unreference(from);
+	i915_gem_context_reference(to);
+	ring->last_context = to;
+}
+
+static int do_switch_xcs(struct intel_engine_cs *ring,
+			 struct intel_context *from,
+			 struct intel_context *to)
+{
+	struct drm_device *dev = ring->dev;
+	struct i915_hw_ppgtt *ppgtt = ctx_to_ppgtt(to);
+	int ret;
+
+	BUG_ON(from && from->legacy_hw_ctx.rcs_state != NULL);
+
+	if (USES_FULL_PPGTT(dev)) {
+		ret = ppgtt->switch_mm(ppgtt, ring, false);
+		if (ret)
+			return ret;
+	}
+
+	if (from)
+		do_switch_fini_common(ring, from, to);
+
+	return 0;
+}
+
+static int do_switch_rcs(struct intel_engine_cs *ring,
+			 struct intel_context *from,
+			 struct intel_context *to)
 {
 	struct drm_i915_private *dev_priv = ring->dev->dev_private;
-	struct intel_context *from = ring->last_context;
 	struct i915_hw_ppgtt *ppgtt = ctx_to_ppgtt(to);
 	u32 hw_flags = 0;
 	bool uninitialized = false;
 	int ret, i;
 
-	if (from != NULL && ring == &dev_priv->ring[RCS]) {
+	if (from != NULL) {
 		BUG_ON(from->legacy_hw_ctx.rcs_state == NULL);
-		BUG_ON(!i915_gem_obj_is_pinned(from->legacy_hw_ctx.rcs_state));
+		BUG_ON(!i915_gem_obj_ggtt_bound(from->legacy_hw_ctx.rcs_state));
 	}
 
-	if (from == to && !to->remap_slice)
-		return 0;
-
 	/* Trying to pin first makes error handling easier. */
-	if (ring == &dev_priv->ring[RCS]) {
-		ret = i915_gem_obj_ggtt_pin(to->legacy_hw_ctx.rcs_state,
-					    get_context_alignment(ring->dev), 0);
-		if (ret)
-			return ret;
-	}
+	ret = i915_gem_obj_ggtt_pin(to->legacy_hw_ctx.rcs_state,
+				    get_context_alignment(ring->dev), 0);
+	if (ret)
+		return ret;
 
 	/*
 	 * Pin can switch back to the default context if we end up calling into
@@ -650,12 +677,6 @@ static int do_switch(struct intel_engine_cs *ring,
 			goto unpin_out;
 	}
 
-	if (ring != &dev_priv->ring[RCS]) {
-		if (from)
-			i915_gem_context_unreference(from);
-		goto done;
-	}
-
 	/*
 	 * Clear this page out of any CPU caches for coherent swap-in/out. Note
 	 * that thanks to write = false in this call and us not setting any gpu
@@ -714,15 +735,11 @@ static int do_switch(struct intel_engine_cs *ring,
 
 		/* obj is kept alive until the next request by its active ref */
 		i915_gem_object_ggtt_unpin(from->legacy_hw_ctx.rcs_state);
-		i915_gem_context_unreference(from);
 	}
 
 	uninitialized = !to->legacy_hw_ctx.initialized && from == NULL;
 	to->legacy_hw_ctx.initialized = true;
-
-done:
-	i915_gem_context_reference(to);
-	ring->last_context = to;
+	do_switch_fini_common(ring, from, to);
 
 	if (uninitialized) {
 		ret = i915_gem_render_state_init(ring);
@@ -733,8 +750,7 @@ done:
 	return 0;
 
 unpin_out:
-	if (ring->id == RCS)
-		i915_gem_object_ggtt_unpin(to->legacy_hw_ctx.rcs_state);
+	i915_gem_object_ggtt_unpin(to->legacy_hw_ctx.rcs_state);
 	return ret;
 }
 
@@ -752,6 +768,7 @@ int i915_switch_context(struct intel_engine_cs *ring,
 			struct intel_context *to)
 {
 	struct drm_i915_private *dev_priv = ring->dev->dev_private;
+	struct intel_context *from = ring->last_context;
 
 	WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
 
@@ -765,7 +782,13 @@ int i915_switch_context(struct intel_engine_cs *ring,
 		return 0;
 	}
 
-	return do_switch(ring, to);
+	if (from == to && !to->remap_slice)
+		return 0;
+
+	if (ring->id == RCS)
+		return do_switch_rcs(ring, from, to);
+	else
+		return do_switch_xcs(ring, from, to);
 }
 
 static bool hw_context_enabled(struct drm_device *dev)
-- 
1.9.0

[PATCH 02/14] drm/i915: Extract l3 remapping out of ctx switch

[Michel Thierry]

From: Ben Widawsky <benjamin.widawsky@intel.com>

This is just a cosmetic change to try to put do_switch_rcs on a diet. As
it stands, the function was quite complex, and error prone.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index a1dc885..9ab3dad 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -643,6 +643,24 @@ static int do_switch_xcs(struct intel_engine_cs *ring,
 	return 0;
 }
 
+static void remap_l3(struct intel_engine_cs *ring,
+		     struct intel_context *ctx)
+{
+	int ret, i;
+
+	for (i = 0; i < MAX_L3_SLICES; i++) {
+		if (!(ctx->remap_slice & (1<<i)))
+			continue;
+
+		ret = i915_gem_l3_remap(ring, i);
+		/* If it failed, try again next round */
+		if (ret)
+			DRM_DEBUG_DRIVER("L3 remapping failed\n");
+		else
+			ctx->remap_slice &= ~(1<<i);
+	}
+}
+
 static int do_switch_rcs(struct intel_engine_cs *ring,
 			 struct intel_context *from,
 			 struct intel_context *to)
@@ -651,7 +669,7 @@ static int do_switch_rcs(struct intel_engine_cs *ring,
 	struct i915_hw_ppgtt *ppgtt = ctx_to_ppgtt(to);
 	u32 hw_flags = 0;
 	bool uninitialized = false;
-	int ret, i;
+	int ret;
 
 	if (from != NULL) {
 		BUG_ON(from->legacy_hw_ctx.rcs_state == NULL);
@@ -702,17 +720,7 @@ static int do_switch_rcs(struct intel_engine_cs *ring,
 	if (ret)
 		goto unpin_out;
 
-	for (i = 0; i < MAX_L3_SLICES; i++) {
-		if (!(to->remap_slice & (1<<i)))
-			continue;
-
-		ret = i915_gem_l3_remap(ring, i);
-		/* If it failed, try again next round */
-		if (ret)
-			DRM_DEBUG_DRIVER("L3 remapping failed\n");
-		else
-			to->remap_slice &= ~(1<<i);
-	}
+	remap_l3(ring, to);
 
 	/* The backing object for the context is done after switching to the
 	 * *next* context. Therefore we cannot retire the previous context until
-- 
1.9.0

[PATCH 03/14] drm/i915/ppgtt: Load address space after mi_set_context

[Michel Thierry]

From: Ben Widawsky <benjamin.widawsky@intel.com>

The simple explanation is, the docs say to do this for GEN8. Perhaps we
want to do this for GEN7 too, I am not certain.

PDPs are saved and restored with context. Contexts (without execlists)
only exist on the render ring. The docs say that PDPs are not power
context save/restored.  I've learned that this actually means something
which SW doesn't care about. So pretend the statement doesn't exist.
For non RCS, nothing changes.

All this patch now does is change the ordering of LRI vs MI_SET_CONTEXT
for the initialization of the context. I do this because the docs say to
do it, and frankly, I cannot reason why it is necessary. I've thought
about it a lot, and tried, without success, to get a reason from design.
The answer I got more or less says, "gen7 is different than gen8." I've
given up, and am adding this little bit of code to make it in sync with
the docs.

v2: Completely rewritten commit message that addresses the requests
Ville made for v1
Only load PDPs for initial context load (Ville)

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 9ab3dad..cfec178 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -669,6 +669,7 @@ static int do_switch_rcs(struct intel_engine_cs *ring,
 	struct i915_hw_ppgtt *ppgtt = ctx_to_ppgtt(to);
 	u32 hw_flags = 0;
 	bool uninitialized = false;
+	bool needs_pd_load = (INTEL_INFO(ring->dev)->gen < 8) && USES_FULL_PPGTT(ring->dev);
 	int ret;
 
 	if (from != NULL) {
@@ -689,7 +690,10 @@ static int do_switch_rcs(struct intel_engine_cs *ring,
 	 */
 	from = ring->last_context;
 
-	if (USES_FULL_PPGTT(ring->dev)) {
+	if (needs_pd_load) {
+		/* Older GENs still want the load first, "PP_DCLV followed by
+		 * PP_DIR_BASE register through Load Register Immediate commands
+		 * in Ring Buffer before submitting a context."*/
 		ret = ppgtt->switch_mm(ppgtt, ring, false);
 		if (ret)
 			goto unpin_out;
@@ -713,13 +717,34 @@ static int do_switch_rcs(struct intel_engine_cs *ring,
 		vma->bind_vma(vma, to->legacy_hw_ctx.rcs_state->cache_level, GLOBAL_BIND);
 	}
 
-	if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to))
+	if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to)) {
 		hw_flags |= MI_RESTORE_INHIBIT;
+		needs_pd_load = USES_FULL_PPGTT(ring->dev) && IS_GEN8(ring->dev);
+	}
 
 	ret = mi_set_context(ring, to, hw_flags);
 	if (ret)
 		goto unpin_out;
 
+	/* GEN8 does *not* require an explicit reload if the PDPs have been
+	 * setup, and we do not wish to move them.
+	 *
+	 * XXX: If we implemented page directory eviction code, this
+	 * optimization needs to be removed.
+	 */
+	if (needs_pd_load) {
+		ret = ppgtt->switch_mm(ppgtt, ring, false);
+		/* The hardware context switch is emitted, but we haven't
+		 * actually changed the state - so it's probably safe to bail
+		 * here. Still, let the user know something dangerous has
+		 * happened.
+		 */
+		if (ret) {
+			DRM_ERROR("Failed to change address space on context switch\n");
+			goto unpin_out;
+		}
+	}
+
 	remap_l3(ring, to);
 
 	/* The backing object for the context is done after switching to the
-- 
1.9.0

[PATCH 04/14] drm/i915/ctx: Return earlier on failure

[Michel Thierry]

From: Ben Widawsky <benjamin.widawsky@intel.com>

As what was correctly debugged here:
commit acc240d41ea1ab9c488a79219fb313b5b46265ae
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Dec 5 15:42:34 2013 +0100

    drm/i915: Fix use-after-free in do_switch

It then becomes apparent that the default context cannot be the context
being switched to for context switch because it is always bound. It
follows that if the ring->last_context (from) has changed after the
bind_to_gtt, it will always be the default context - this is commented
in the code block.

This assertion will help catch issues without our logic sooner than
letting the system move long (which is possible for some time).

I really want this to be a BUG(), but I also want the patch to get
merged. I think the fact that none of the ERRNOs make any sense at all
is just more evidence that this shouldn't be a WARN.

//Cc: Ian Lister (don't have current email address)
Cc: Rafael Barbalho <rafael.barbalho@intel.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index cfec178..fab74d3 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -690,6 +690,15 @@ static int do_switch_rcs(struct intel_engine_cs *ring,
 	 */
 	from = ring->last_context;
 
+	/* The only context which 'from' can be, if it was changed, is the default
+	 * context. The default context cannot end up in evict everything (as
+	 * commented above) because it is always pinned.
+	 */
+	if (WARN_ON(from == to)) {
+		ret = -EPERM;
+		goto unpin_out;
+	}
+
 	if (needs_pd_load) {
 		/* Older GENs still want the load first, "PP_DCLV followed by
 		 * PP_DIR_BASE register through Load Register Immediate commands
-- 
1.9.0

[PATCH 05/14] drm/i915/error: Check the potential ctx obj's vm

[Michel Thierry]

From: Ben Widawsky <benjamin.widawsky@intel.com>

The bound list is global (all objects which back the VMAs are stored
here). Recently the BUG() in the offset lookup was demoted to a WARN,
but the fault actually lies in the caller, here.

This bug has existed since the initial introduction of PPGTT (however,
it was fixed in unmerged patches to fix up the error state).

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gpu_error.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 45b6191..9faebbc 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -920,6 +920,9 @@ static void i915_gem_record_active_context(struct intel_engine_cs *ring,
 		return;
 
 	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
+		if (!i915_gem_obj_ggtt_bound(obj))
+			continue;
+
 		if ((error->ccid & PAGE_MASK) == i915_gem_obj_ggtt_offset(obj)) {
 			ering->ctx = i915_error_ggtt_object_create(dev_priv, obj);
 			break;
-- 
1.9.0

[PATCH 06/14] drm/i915/error: vma error capture prettyify

[Michel Thierry]

From: Ben Widawsky <benjamin.widawsky@intel.com>

Rename some variables, and clean up the code a bit to make things
clearer in our error capture.

There isn't an intentional functional change here.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gpu_error.c | 55 ++++++++++++++++++++---------------
 1 file changed, 32 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 9faebbc..ac101cd 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1016,63 +1016,72 @@ static void i915_gem_record_rings(struct drm_device *dev,
 	}
 }
 
-/* FIXME: Since pin count/bound list is global, we duplicate what we capture per
+/**
+ * i915_gem_capture_vm() - Capture a VMs error state.
+ * @error:	The main error structure
+ * @vm:		The address space we're capturing.
+ * @vm_ndx:	Which vm without the buffer array
+ *
+ * FIXME: Since pin count/bound list is global, we duplicate what we capture per
  * VM.
  */
 static void i915_gem_capture_vm(struct drm_i915_private *dev_priv,
 				struct drm_i915_error_state *error,
 				struct i915_address_space *vm,
-				const int ndx)
+				const int vm_ndx)
 {
 	struct drm_i915_error_buffer *active_bo = NULL, *pinned_bo = NULL;
 	struct drm_i915_gem_object *obj;
 	struct i915_vma *vma;
-	int i;
+	int active_vma_count = 0;
 
-	i = 0;
 	list_for_each_entry(vma, &vm->active_list, mm_list)
-		i++;
-	error->active_bo_count[ndx] = i;
+		active_vma_count++;
+
+	error->active_bo_count[vm_ndx] = active_vma_count;
+
 	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list)
 		if (i915_gem_obj_is_pinned(obj))
-			i++;
-	error->pinned_bo_count[ndx] = i - error->active_bo_count[ndx];
+			active_vma_count++;
+
+	/* XXX: this is an incorrect measurement of pinned BOs */
+	error->pinned_bo_count[vm_ndx] = active_vma_count - error->active_bo_count[vm_ndx];
 
-	if (i) {
-		active_bo = kcalloc(i, sizeof(*active_bo), GFP_ATOMIC);
+	if (active_vma_count) {
+		active_bo = kcalloc(active_vma_count, sizeof(*active_bo), GFP_ATOMIC);
 		if (active_bo)
-			pinned_bo = active_bo + error->active_bo_count[ndx];
+			pinned_bo = active_bo + error->active_bo_count[vm_ndx];
 	}
 
 	if (active_bo)
-		error->active_bo_count[ndx] =
+		error->active_bo_count[vm_ndx] =
 			capture_active_bo(active_bo,
-					  error->active_bo_count[ndx],
+					  error->active_bo_count[vm_ndx],
 					  &vm->active_list);
 
 	if (pinned_bo)
-		error->pinned_bo_count[ndx] =
+		error->pinned_bo_count[vm_ndx] =
 			capture_pinned_bo(pinned_bo,
-					  error->pinned_bo_count[ndx],
+					  error->pinned_bo_count[vm_ndx],
 					  &dev_priv->mm.bound_list);
-	error->active_bo[ndx] = active_bo;
-	error->pinned_bo[ndx] = pinned_bo;
+	error->active_bo[vm_ndx] = active_bo;
+	error->pinned_bo[vm_ndx] = pinned_bo;
 }
 
 static void i915_gem_capture_buffers(struct drm_i915_private *dev_priv,
 				     struct drm_i915_error_state *error)
 {
 	struct i915_address_space *vm;
-	int cnt = 0, i = 0;
+	int vm_count = 0, i = 0;
 
 	list_for_each_entry(vm, &dev_priv->vm_list, global_link)
-		cnt++;
+		vm_count++;
 
-	error->active_bo = kcalloc(cnt, sizeof(*error->active_bo), GFP_ATOMIC);
-	error->pinned_bo = kcalloc(cnt, sizeof(*error->pinned_bo), GFP_ATOMIC);
-	error->active_bo_count = kcalloc(cnt, sizeof(*error->active_bo_count),
+	error->active_bo = kcalloc(vm_count, sizeof(*error->active_bo), GFP_ATOMIC);
+	error->pinned_bo = kcalloc(vm_count, sizeof(*error->pinned_bo), GFP_ATOMIC);
+	error->active_bo_count = kcalloc(vm_count, sizeof(*error->active_bo_count),
 					 GFP_ATOMIC);
-	error->pinned_bo_count = kcalloc(cnt, sizeof(*error->pinned_bo_count),
+	error->pinned_bo_count = kcalloc(vm_count, sizeof(*error->pinned_bo_count),
 					 GFP_ATOMIC);
 
 	list_for_each_entry(vm, &dev_priv->vm_list, global_link)
-- 
1.9.0

[PATCH 07/14] drm/i915/error: Do a better job of disambiguating VMAs

[Michel Thierry]

From: Ben Widawsky <benjamin.widawsky@intel.com>

Some of the original PPGTT patches in this area where unmerged, and this
left a lot of confusion in our error capture with regard to which vm/obj
we want to capture. There have been at least a couple of patches from
Chris, and myself to try to fix this up; so here is another shot. Nobody
running without full PPGTT is effected by this, and that is probably why
nobody has bothered to fix it yet.

Instead of using any of the global lists to find the VMAs we want to
capture, we use the union of the active, and the inactive list in the
VM. This allows us to replace our capture_bo with capture_vma, and know
all the VMAs we want to capture are valid.

I could have probably figured out a way to reuse mm_list. As we've had
bugs here before in the shrinker, I think the best way forward is to get
it working, and then optimize it later.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c   |  1 +
 drivers/gpu/drm/i915/i915_gem_gtt.h   |  2 ++
 drivers/gpu/drm/i915/i915_gpu_error.c | 39 ++++++++++++++++++++++-------------
 3 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 5188936..0b2b982 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2115,6 +2115,7 @@ static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
 		return ERR_PTR(-ENOMEM);
 
 	INIT_LIST_HEAD(&vma->vma_link);
+	INIT_LIST_HEAD(&vma->pin_capture_link);
 	INIT_LIST_HEAD(&vma->mm_list);
 	INIT_LIST_HEAD(&vma->exec_list);
 	vma->vm = vm;
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 8d6f7c1..1d75801 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -126,6 +126,8 @@ struct i915_vma {
 
 	struct list_head vma_link; /* Link in the object's VMA list */
 
+	struct list_head pin_capture_link; /* Link in the error capture */
+
 	/** This vma's place in the batchbuffer or on the eviction list */
 	struct list_head exec_list;
 
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index ac101cd..4ff819e 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -677,14 +677,14 @@ static u32 capture_active_bo(struct drm_i915_error_buffer *err,
 static u32 capture_pinned_bo(struct drm_i915_error_buffer *err,
 			     int count, struct list_head *head)
 {
-	struct drm_i915_gem_object *obj;
+	struct i915_vma *vma;
 	int i = 0;
 
-	list_for_each_entry(obj, head, global_list) {
-		if (!i915_gem_obj_is_pinned(obj))
+	list_for_each_entry(vma, head, pin_capture_link) {
+		if (!i915_gem_obj_is_pinned(vma->obj))
 			continue;
 
-		capture_bo(err++, obj);
+		capture_bo(err++, vma->obj);
 		if (++i == count)
 			break;
 	}
@@ -1031,21 +1031,32 @@ static void i915_gem_capture_vm(struct drm_i915_private *dev_priv,
 				const int vm_ndx)
 {
 	struct drm_i915_error_buffer *active_bo = NULL, *pinned_bo = NULL;
-	struct drm_i915_gem_object *obj;
 	struct i915_vma *vma;
 	int active_vma_count = 0;
+	int vma_pin_count = 0;
+	LIST_HEAD(pinned_vma);
 
-	list_for_each_entry(vma, &vm->active_list, mm_list)
+	list_for_each_entry(vma, &vm->active_list, mm_list) {
 		active_vma_count++;
+		if (vma->pin_count) {
+			vma_pin_count++;
+			list_move_tail(&vma->pin_capture_link, &pinned_vma);
+		}
+	}
 
-	error->active_bo_count[vm_ndx] = active_vma_count;
-
-	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list)
-		if (i915_gem_obj_is_pinned(obj))
-			active_vma_count++;
+	list_for_each_entry(vma, &vm->inactive_list, mm_list) {
+		/* Certain objects may be on the inactive list, but pinned, when
+		 * in the global GGTT. */
+		if (WARN_ON(!i915_is_ggtt(vm) &&
+			    vma->pin_count &&
+			    !(vma->exec_entry->flags & (1<<31)))) { /* FIXME: need the actual flag */
+			vma_pin_count++;
+			list_move_tail(&vma->pin_capture_link, &pinned_vma);
+		}
+	}
 
-	/* XXX: this is an incorrect measurement of pinned BOs */
-	error->pinned_bo_count[vm_ndx] = active_vma_count - error->active_bo_count[vm_ndx];
+	error->active_bo_count[vm_ndx] = active_vma_count;
+	error->pinned_bo_count[vm_ndx] = vma_pin_count;
 
 	if (active_vma_count) {
 		active_bo = kcalloc(active_vma_count, sizeof(*active_bo), GFP_ATOMIC);
@@ -1063,7 +1074,7 @@ static void i915_gem_capture_vm(struct drm_i915_private *dev_priv,
 		error->pinned_bo_count[vm_ndx] =
 			capture_pinned_bo(pinned_bo,
 					  error->pinned_bo_count[vm_ndx],
-					  &dev_priv->mm.bound_list);
+					  &pinned_vma);
 	error->active_bo[vm_ndx] = active_bo;
 	error->pinned_bo[vm_ndx] = pinned_bo;
 }
-- 
1.9.0

[PATCH 08/14] drm/i915/error: Capture vmas instead of BOs

[Michel Thierry]

From: Ben Widawsky <benjamin.widawsky@intel.com>

To follow up on the last patch, we can now capture the VMAs instead of
the BOs. The hope if we get more accurate error capture while debugging
PPGTT.

Note that this does not impact the previous argument about whether to
capture all VMAs, or just the guilty VMA. This merely allows the code to
do whatever we chose later.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h       |  1 +
 drivers/gpu/drm/i915/i915_gpu_error.c | 53 +++++++++++++++++++----------------
 2 files changed, 30 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 991b663..96cf5a9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -309,6 +309,7 @@ struct drm_i915_error_state {
 	char error_msg[128];
 	u32 reset_count;
 	u32 suspend_count;
+	u32 vm_count;
 
 	/* Generic register state */
 	u32 eir;
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 4ff819e..eb943ee 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -385,15 +385,19 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
 		i915_ring_error_state(m, dev, &error->ring[i]);
 	}
 
-	if (error->active_bo)
-		print_error_buffers(m, "Active",
-				    error->active_bo[0],
-				    error->active_bo_count[0]);
+	for (i = 0; i < error->vm_count; i++) {
+		if (error->active_bo[i])
+			print_error_buffers(m, "Active",
+					    error->active_bo[i],
+					    error->active_bo_count[i]);
+	}
 
-	if (error->pinned_bo)
-		print_error_buffers(m, "Pinned",
-				    error->pinned_bo[0],
-				    error->pinned_bo_count[0]);
+	for (i = 0; i < error->vm_count; i++) {
+		if (error->pinned_bo[i])
+			print_error_buffers(m, "Pinned",
+					    error->pinned_bo[i],
+					    error->pinned_bo_count[i]);
+	}
 
 	for (i = 0; i < ARRAY_SIZE(error->ring); i++) {
 		obj = error->ring[i].batchbuffer;
@@ -635,22 +639,23 @@ unwind:
 	i915_error_object_create_sized((dev_priv), (src), &(dev_priv)->gtt.base, \
 				       (src)->base.size>>PAGE_SHIFT)
 
-static void capture_bo(struct drm_i915_error_buffer *err,
-		       struct drm_i915_gem_object *obj)
+static void capture_vma(struct drm_i915_error_buffer *err, struct i915_vma *vma)
 {
+	struct drm_i915_gem_object *obj = vma->obj;
+
 	err->size = obj->base.size;
 	err->name = obj->base.name;
 	err->rseqno = obj->last_read_seqno;
 	err->wseqno = obj->last_write_seqno;
-	err->gtt_offset = i915_gem_obj_ggtt_offset(obj);
+	err->gtt_offset = vma->node.start;
 	err->read_domains = obj->base.read_domains;
 	err->write_domain = obj->base.write_domain;
 	err->fence_reg = obj->fence_reg;
-	err->pinned = 0;
-	if (i915_gem_obj_is_pinned(obj))
-		err->pinned = 1;
-	if (obj->user_pin_count > 0)
+	if (obj->user_pin_count > 0) {
+		WARN_ON(i915_is_ggtt(vma->vm));
 		err->pinned = -1;
+	} else
+		err->pinned = !!vma->pin_count;
 	err->tiling = obj->tiling_mode;
 	err->dirty = obj->dirty;
 	err->purgeable = obj->madv != I915_MADV_WILLNEED;
@@ -666,7 +671,7 @@ static u32 capture_active_bo(struct drm_i915_error_buffer *err,
 	int i = 0;
 
 	list_for_each_entry(vma, head, mm_list) {
-		capture_bo(err++, vma->obj);
+		capture_vma(err++, vma);
 		if (++i == count)
 			break;
 	}
@@ -681,10 +686,10 @@ static u32 capture_pinned_bo(struct drm_i915_error_buffer *err,
 	int i = 0;
 
 	list_for_each_entry(vma, head, pin_capture_link) {
-		if (!i915_gem_obj_is_pinned(vma->obj))
+		if (!vma->pin_count)
 			continue;
 
-		capture_bo(err++, vma->obj);
+		capture_vma(err++, vma);
 		if (++i == count)
 			break;
 	}
@@ -1083,16 +1088,16 @@ static void i915_gem_capture_buffers(struct drm_i915_private *dev_priv,
 				     struct drm_i915_error_state *error)
 {
 	struct i915_address_space *vm;
-	int vm_count = 0, i = 0;
+	int i = 0;
 
 	list_for_each_entry(vm, &dev_priv->vm_list, global_link)
-		vm_count++;
+		error->vm_count++;
 
-	error->active_bo = kcalloc(vm_count, sizeof(*error->active_bo), GFP_ATOMIC);
-	error->pinned_bo = kcalloc(vm_count, sizeof(*error->pinned_bo), GFP_ATOMIC);
-	error->active_bo_count = kcalloc(vm_count, sizeof(*error->active_bo_count),
+	error->active_bo = kcalloc(error->vm_count, sizeof(*error->active_bo), GFP_ATOMIC);
+	error->pinned_bo = kcalloc(error->vm_count, sizeof(*error->pinned_bo), GFP_ATOMIC);
+	error->active_bo_count = kcalloc(error->vm_count, sizeof(*error->active_bo_count),
 					 GFP_ATOMIC);
-	error->pinned_bo_count = kcalloc(vm_count, sizeof(*error->pinned_bo_count),
+	error->pinned_bo_count = kcalloc(error->vm_count, sizeof(*error->pinned_bo_count),
 					 GFP_ATOMIC);
 
 	list_for_each_entry(vm, &dev_priv->vm_list, global_link)
-- 
1.9.0

[PATCH 09/14] drm/i915: Add some extra guards in evict_vm

[Michel Thierry]

From: Ben Widawsky <benjamin.widawsky@intel.com>

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_evict.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
index bbf4b12..38297d3 100644
--- a/drivers/gpu/drm/i915/i915_gem_evict.c
+++ b/drivers/gpu/drm/i915/i915_gem_evict.c
@@ -214,6 +214,7 @@ int i915_gem_evict_vm(struct i915_address_space *vm, bool do_idle)
 	struct i915_vma *vma, *next;
 	int ret;
 
+	BUG_ON(!mutex_is_locked(&vm->dev->struct_mutex));
 	trace_i915_gem_evict_vm(vm);
 
 	if (do_idle) {
@@ -222,11 +223,15 @@ int i915_gem_evict_vm(struct i915_address_space *vm, bool do_idle)
 			return ret;
 
 		i915_gem_retire_requests(vm->dev);
+
+		WARN_ON(!list_empty(&vm->active_list));
 	}
 
-	list_for_each_entry_safe(vma, next, &vm->inactive_list, mm_list)
+	list_for_each_entry_safe(vma, next, &vm->inactive_list, mm_list) {
+		WARN_ON(!i915_is_ggtt(vm) && vma->pin_count);
 		if (vma->pin_count == 0)
 			WARN_ON(i915_vma_unbind(vma));
+	}
 
 	return 0;
 }
-- 
1.9.0

[PATCH 10/14] drm/i915: Make an uninterruptible evict

[Michel Thierry]

From: Ben Widawsky <benjamin.widawsky@intel.com>

There are no users of this yet, but the idea is presented and split out
to find bugs.

Also, while here, return -ERESTARTSYS to the caller, in case they want
to do something with it.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h            |  2 +-
 drivers/gpu/drm/i915/i915_gem_context.c    |  5 ++---
 drivers/gpu/drm/i915/i915_gem_evict.c      | 32 ++++++++++++++++++++++--------
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  2 +-
 4 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 96cf5a9..a5ca462 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2523,7 +2523,7 @@ int __must_check i915_gem_evict_something(struct drm_device *dev,
 					  unsigned long start,
 					  unsigned long end,
 					  unsigned flags);
-int i915_gem_evict_vm(struct i915_address_space *vm, bool do_idle);
+int i915_gem_evict_vm(struct i915_address_space *vm, bool do_idle, bool interruptible);
 int i915_gem_evict_everything(struct drm_device *dev);
 
 /* belongs in i915_gem_gtt.h */
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index fab74d3..0410bd9 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -121,11 +121,10 @@ static void do_ppgtt_cleanup(struct i915_hw_ppgtt *ppgtt)
 			if (WARN_ON(list_empty(&vma->vma_link) ||
 				    list_is_singular(&vma->vma_link)))
 				break;
-
-		i915_gem_evict_vm(&ppgtt->base, true);
+		i915_gem_evict_vm(&ppgtt->base, true, true);
 	} else {
 		i915_gem_retire_requests(dev);
-		i915_gem_evict_vm(&ppgtt->base, false);
+		i915_gem_evict_vm(&ppgtt->base, false, true);
 	}
 
 	ppgtt->base.cleanup(&ppgtt->base);
diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
index 38297d3..ac8d90f 100644
--- a/drivers/gpu/drm/i915/i915_gem_evict.c
+++ b/drivers/gpu/drm/i915/i915_gem_evict.c
@@ -197,8 +197,9 @@ found:
 /**
  * i915_gem_evict_vm - Evict all idle vmas from a vm
  *
- * @vm: Address space to cleanse
- * @do_idle: Boolean directing whether to idle first.
+ * @vm:			Address space to cleanse
+ * @do_idle:		Boolean directing whether to idle first.
+ * @interruptible:	How to wait
  *
  * This function evicts all idles vmas from a vm. If all unpinned vmas should be
  * evicted the @do_idle needs to be set to true.
@@ -209,18 +210,24 @@ found:
  * To clarify: This is for freeing up virtual address space, not for freeing
  * memory in e.g. the shrinker.
  */
-int i915_gem_evict_vm(struct i915_address_space *vm, bool do_idle)
+int i915_gem_evict_vm(struct i915_address_space *vm, bool do_idle, bool interruptible)
 {
+	struct drm_i915_private *dev_priv = to_i915(vm->dev);
+
 	struct i915_vma *vma, *next;
+	bool was_intr = dev_priv->mm.interruptible;
 	int ret;
 
 	BUG_ON(!mutex_is_locked(&vm->dev->struct_mutex));
 	trace_i915_gem_evict_vm(vm);
 
+	if (!interruptible)
+		dev_priv->mm.interruptible = false;
+
 	if (do_idle) {
 		ret = i915_gpu_idle(vm->dev);
 		if (ret)
-			return ret;
+			goto out;
 
 		i915_gem_retire_requests(vm->dev);
 
@@ -229,11 +236,20 @@ int i915_gem_evict_vm(struct i915_address_space *vm, bool do_idle)
 
 	list_for_each_entry_safe(vma, next, &vm->inactive_list, mm_list) {
 		WARN_ON(!i915_is_ggtt(vm) && vma->pin_count);
-		if (vma->pin_count == 0)
-			WARN_ON(i915_vma_unbind(vma));
+		if (vma->pin_count == 0) {
+			ret = i915_vma_unbind(vma);
+			if (ret == -ERESTARTSYS) {
+				BUG_ON(!interruptible);
+				goto out;
+			} else if (ret)
+				WARN(1, "Failed to unbind vma %d\n", ret);
+		}
 	}
+	ret = 0;
 
-	return 0;
+out:
+	dev_priv->mm.interruptible = was_intr;
+	return ret;
 }
 
 /**
@@ -276,7 +292,7 @@ i915_gem_evict_everything(struct drm_device *dev)
 
 	/* Having flushed everything, unbind() should never raise an error */
 	list_for_each_entry(vm, &dev_priv->vm_list, global_link)
-		WARN_ON(i915_gem_evict_vm(vm, false));
+		WARN_ON(i915_gem_evict_vm(vm, false, true));
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 60998fc..1420aeb 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -722,7 +722,7 @@ err:
 		list_for_each_entry(vma, vmas, exec_list)
 			i915_gem_execbuffer_unreserve_vma(vma);
 
-		ret = i915_gem_evict_vm(vm, true);
+		ret = i915_gem_evict_vm(vm, true, true);
 		if (ret)
 			return ret;
 	} while (1);
-- 
1.9.0

[PATCH 11/14] drm/i915: Reorder ctx unref on ppgtt cleanup

[Michel Thierry]

From: Ben Widawsky <benjamin.widawsky@intel.com>

The comment [which was mine] is wrong. The context object can never be
bound in a PPGTT because it is only capable of living in the Global GTT.
So, remove the comment, and reorder the unref. What's nice about the
latter is it keeps the context object alive past the PPGTT. This makes
the destroy ordering symmetric with the creation ordering.

Create:
1. Create context
2. Create PPGTT

Destroy:
1. Destroy PPGTT
2. Destroy context

As far as I know, this does not fix a bug. The code previously kept the
context data structure, only the object was gone. As the code was,
nothing tried to use the object after this point.

NOTE: If in the future we have cases where the PPGTT can/should outlive
the context (which doesn't occur today, but the code permits it), this
ordering does not matter. Even if this occurs, as it stands now, we do
not expect that to be the normal case, and having this order makes
debugging a bit easier if we're tracking object lifetimes for the
context vs ppgtt

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 0410bd9..70b3776 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -185,14 +185,12 @@ void i915_gem_context_free(struct kref *ctx_ref)
 		/* We refcount even the aliasing PPGTT to keep the code symmetric */
 		if (USES_PPGTT(ctx->legacy_hw_ctx.rcs_state->base.dev))
 			ppgtt = ctx_to_ppgtt(ctx);
-
-		/* XXX: Free up the object before tearing down the address space, in
-		 * case we're bound in the PPGTT */
-		drm_gem_object_unreference(&ctx->legacy_hw_ctx.rcs_state->base);
 	}
 
 	if (ppgtt)
 		kref_put(&ppgtt->ref, ppgtt_release);
+	if (ctx->legacy_hw_ctx.rcs_state)
+		drm_gem_object_unreference(&ctx->legacy_hw_ctx.rcs_state->base);
 	list_del(&ctx->link);
 	kfree(ctx);
 }
-- 
1.9.0

[PATCH 12/14] drm/i915: More correct (slower) ppgtt cleanup

[Michel Thierry]

From: Ben Widawsky <benjamin.widawsky@intel.com>

If a VM still have objects which are bound (exactly: have a node
reserved in the drm_mm), and we are in the middle of a reset, we have no
hope of the standard methods fixing the situation (ring idle won't
work). We must therefore let the reset handler take it's course, and
then we can resume tearing down the VM.

This logic very much duplicates^Wresembles the logic in our wait for
error code. I've decided to leave it as open coded because I expect this
bit of code to require tweaks and changes over time.

Interruptions via signal causes a really similar problem.

This should deprecate the need for the yet unmerged patch from Chris
(and an identical patch from me, which was first!!):
drm/i915: Prevent signals from interrupting close()

I have a followup patch to implement deferred free, before you complain.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 51 +++++++++++++++++++++++++++++++--
 1 file changed, 48 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 70b3776..84c65aa 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -101,6 +101,32 @@ static void do_ppgtt_cleanup(struct i915_hw_ppgtt *ppgtt)
 	struct drm_device *dev = ppgtt->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct i915_address_space *vm = &ppgtt->base;
+	bool do_idle = false;
+	int ret;
+
+	/* If we get here while in reset, we need to let the reset handler run
+	 * first, or else our VM teardown isn't going to go smoothly. There are
+	 * a could of options at this point, but letting the reset handler do
+	 * it's thing is the most desirable. The reset handler will take care of
+	 * retiring the stuck requests.
+	 */
+	if (i915_reset_in_progress(&dev_priv->gpu_error)) {
+		mutex_unlock(&dev->struct_mutex);
+#define EXIT_COND (!i915_reset_in_progress(&dev_priv->gpu_error) || \
+		   i915_terminally_wedged(&dev_priv->gpu_error))
+		ret = wait_event_timeout(dev_priv->gpu_error.reset_queue,
+					 EXIT_COND,
+					 10 * HZ);
+		if (!ret) {
+			/* it's unlikely idling will solve anything, but it
+			 * shouldn't hurt to try. */
+			do_idle = true;
+			/* TODO: go down kicking and screaming harder */
+		}
+#undef EXIT_COND
+
+		mutex_lock(&dev->struct_mutex);
+	}
 
 	if (ppgtt == dev_priv->mm.aliasing_ppgtt ||
 	    (list_empty(&vm->active_list) && list_empty(&vm->inactive_list))) {
@@ -117,14 +143,33 @@ static void do_ppgtt_cleanup(struct i915_hw_ppgtt *ppgtt)
 	if (!list_empty(&vm->active_list)) {
 		struct i915_vma *vma;
 
+		do_idle = true;
 		list_for_each_entry(vma, &vm->active_list, mm_list)
 			if (WARN_ON(list_empty(&vma->vma_link) ||
 				    list_is_singular(&vma->vma_link)))
 				break;
-		i915_gem_evict_vm(&ppgtt->base, true, true);
-	} else {
+	} else
 		i915_gem_retire_requests(dev);
-		i915_gem_evict_vm(&ppgtt->base, false, true);
+
+	/* We have a problem here where VM teardown cannot be interrupted, or
+	 * else the ppgtt cleanup will fail. As an example, a precisely timed
+	 * SIGKILL could leads to an OOPS, or worse. There are two options:
+	 * 1. Make the eviction uninterruptible
+	 * 2. Defer the eviction if it was interrupted.
+	 *
+	 * Option #1 is not the friendliest, but it's the easiest to implement,
+	 * and least error prone.
+	 * TODO: Implement option 2
+	 */
+	ret = i915_gem_evict_vm(&ppgtt->base, do_idle, !do_idle);
+	if (ret == -ERESTARTSYS)
+		ret = i915_gem_evict_vm(&ppgtt->base, do_idle, false);
+	WARN_ON(ret);
+	WARN_ON(!list_empty(&vm->active_list));
+
+	/* This is going to blow up badly if the mm is unclean */
+	if (WARN_ON(!list_empty(&ppgtt->base.mm.head_node.node_list))) {
+		/* TODO: go down kicking and screaming harder++ */
 	}
 
 	ppgtt->base.cleanup(&ppgtt->base);
-- 
1.9.0

[PATCH 13/14] drm/i915: Defer PPGTT cleanup

[Michel Thierry]

From: Ben Widawsky <benjamin.widawsky@intel.com>

The last patch made PPGTT free cases correct. It left a major problem
though where in many cases it was possible to have to idle the GPU in
order to destroy a VM. This is really unfortunate as it is stalling the
active GPU process for the dying GPU process.

The workqueue grew very tricky. I left in my original wait based version
as #if 0, and used Chris' recommendation with the seqno check. I haven't
measure one vs the other, but I am in favor of the code as it is. I am
just leaving the old code for people to observe.

NOTE: I somewhat expect this patch to not get merged because of work in
the pipeline. I won't argue much against that, and I'll simply say, this
solves a real problem, or platforms running with PPGTT. PPGTT is not
enabled by default yet, and I therefore see little harm in merging this.
Because I was expecting this to not get merged, I did not spent much
time investigating any corner cases, in particular, cases where I need
to cleanup the wq. If this patch does head in the merge direction, I
will take a closer look at that.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h         |  10 +++
 drivers/gpu/drm/i915/i915_gem.c         | 110 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem_context.c |  40 ++++++++----
 drivers/gpu/drm/i915/i915_gem_gtt.c     |   2 +-
 drivers/gpu/drm/i915/i915_gem_gtt.h     |   2 +
 5 files changed, 150 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a5ca462..0098ff9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1118,6 +1118,15 @@ struct i915_gem_mm {
 	struct delayed_work idle_work;
 
 	/**
+	 * PPGTT freeing often happens during interruptible times (fd close,
+	 * execbuf, busy_ioctl). It therefore becomes difficult to clean up the
+	 * PPGTT when the refcount reaches 0 if a signal comes in. This
+	 * workqueue defers the cleanup of the VM to a later time, and allows
+	 * userspace to continue on.
+	 */
+	struct delayed_work ppgtt_work;
+
+	/**
 	 * Are we in a non-interruptible section of code like
 	 * modesetting?
 	 */
@@ -1502,6 +1511,7 @@ struct drm_i915_private {
 	struct mutex modeset_restore_lock;
 
 	struct list_head vm_list; /* Global list of all address spaces */
+	struct list_head ppgtt_free_list;
 	struct i915_gtt gtt; /* VM representing the global address space */
 
 	struct i915_gem_mm mm;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ef047bc..2df4d86 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2707,6 +2707,112 @@ i915_gem_idle_work_handler(struct work_struct *work)
 	intel_mark_idle(dev_priv->dev);
 }
 
+static void
+i915_gem_ppgtt_work_handler(struct work_struct *work)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(work, typeof(*dev_priv), mm.ppgtt_work.work);
+	struct i915_address_space *vm, *v;
+#if 0
+	unsigned reset_counter;
+	int ret;
+#endif
+
+	if (!mutex_trylock(&dev_priv->dev->struct_mutex)) {
+		queue_delayed_work(dev_priv->wq, &dev_priv->mm.ppgtt_work,
+				   round_jiffies_up_relative(HZ));
+		return;
+	}
+
+	list_for_each_entry_safe(vm, v, &dev_priv->ppgtt_free_list, global_link) {
+		struct i915_hw_ppgtt *ppgtt = container_of(vm, struct i915_hw_ppgtt, base);
+		struct i915_vma *vma = NULL, *vma_active = NULL, *vma_inactive = NULL;
+
+		/* The following attempts to find the newest (most recently
+		 * activated/inactivated) VMA.
+		 */
+		if (!list_empty(&ppgtt->base.active_list)) {
+			vma_active = list_last_entry(&ppgtt->base.active_list,
+						     typeof(*vma), mm_list);
+		}
+		if (!list_empty(&ppgtt->base.inactive_list)) {
+			vma_inactive = list_last_entry(&ppgtt->base.inactive_list,
+						       typeof(*vma), mm_list);
+		}
+
+		if (vma_active)
+			vma = vma_active;
+		else if (vma_inactive)
+			vma = vma_inactive;
+
+		/* Sanity check */
+		if (vma_active && vma_inactive) {
+			if (WARN_ON(vma_active->retire_seqno <= vma_inactive->retire_seqno))
+				vma = vma_inactive;
+		}
+
+		if (!vma)
+			goto finish;
+
+		/* Another sanity check */
+		if (WARN_ON(!vma->retire_seqno))
+			continue;
+
+		/* NOTE: We could wait here for the seqno, but that makes things
+		 * significantly more complex. */
+		if (!i915_seqno_passed(vma->retire_ring->get_seqno(vma->retire_ring, true), vma->retire_seqno))
+			break;
+
+#if 0
+		reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
+		mutex_unlock(&dev_priv->dev->struct_mutex);
+		ret = __wait_seqno(vma->retire_ring, vma->retire_seqno,
+				   reset_counter, true, NULL, NULL);
+
+		if (i915_mutex_lock_interruptible(dev_priv->dev)) {
+			queue_delayed_work(dev_priv->wq, &dev_priv->mm.ppgtt_work,
+					   round_jiffies_up_relative(HZ));
+			return;
+		}
+
+		if (ret)
+			continue;
+
+#endif
+finish:
+		/* If the object was destroyed while our VMA was dangling, the
+		 * object free code did the synchronous cleanup for us.
+		 * Therefore every node on this list should have a living object
+		 * (and VMA), but let's try not to use it in case future code
+		 * allows a VMA to outlive an object.
+		 */
+		while (!list_empty(&ppgtt->base.mm.head_node.node_list)) {
+			struct drm_mm_node *node;
+			struct i915_vma *vma;
+			struct drm_i915_gem_object *obj;
+
+			node = list_first_entry(&ppgtt->base.mm.head_node.node_list,
+						struct drm_mm_node,
+						node_list);
+			vma = container_of(node, struct i915_vma, node);
+			obj = vma->obj;
+
+			drm_mm_remove_node(node);
+			i915_gem_vma_destroy(vma);
+			i915_gem_object_unpin_pages(obj);
+		}
+
+		ppgtt->base.cleanup(&ppgtt->base);
+		kfree(ppgtt);
+	}
+
+	if (!list_empty(&dev_priv->ppgtt_free_list))
+		queue_delayed_work(dev_priv->wq, &dev_priv->mm.ppgtt_work,
+				   round_jiffies_up_relative(HZ));
+
+	mutex_unlock(&dev_priv->dev->struct_mutex);
+}
+
 /**
  * Ensures that an object will eventually get non-busy by flushing any required
  * write domains, emitting any outstanding lazy request and retiring and
@@ -4553,6 +4659,7 @@ i915_gem_suspend(struct drm_device *dev)
 	mutex_unlock(&dev->struct_mutex);
 
 	del_timer_sync(&dev_priv->gpu_error.hangcheck_timer);
+	cancel_delayed_work_sync(&dev_priv->mm.ppgtt_work);
 	cancel_delayed_work_sync(&dev_priv->mm.retire_work);
 	cancel_delayed_work_sync(&dev_priv->mm.idle_work);
 
@@ -4894,6 +5001,7 @@ i915_gem_load(struct drm_device *dev)
 				  NULL);
 
 	INIT_LIST_HEAD(&dev_priv->vm_list);
+	INIT_LIST_HEAD(&dev_priv->ppgtt_free_list);
 	i915_init_vm(dev_priv, &dev_priv->gtt.base);
 
 	INIT_LIST_HEAD(&dev_priv->context_list);
@@ -4908,6 +5016,8 @@ i915_gem_load(struct drm_device *dev)
 			  i915_gem_retire_work_handler);
 	INIT_DELAYED_WORK(&dev_priv->mm.idle_work,
 			  i915_gem_idle_work_handler);
+	INIT_DELAYED_WORK(&dev_priv->mm.ppgtt_work,
+			  i915_gem_ppgtt_work_handler);
 	init_waitqueue_head(&dev_priv->gpu_error.reset_queue);
 
 	/* On GEN3 we really need to make sure the ARB C3 LP bit is set */
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 84c65aa..e221d5b 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -96,7 +96,7 @@
 #define GEN6_CONTEXT_ALIGN (64<<10)
 #define GEN7_CONTEXT_ALIGN 4096
 
-static void do_ppgtt_cleanup(struct i915_hw_ppgtt *ppgtt)
+static int do_ppgtt_cleanup(struct i915_hw_ppgtt *ppgtt)
 {
 	struct drm_device *dev = ppgtt->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -131,7 +131,7 @@ static void do_ppgtt_cleanup(struct i915_hw_ppgtt *ppgtt)
 	if (ppgtt == dev_priv->mm.aliasing_ppgtt ||
 	    (list_empty(&vm->active_list) && list_empty(&vm->inactive_list))) {
 		ppgtt->base.cleanup(&ppgtt->base);
-		return;
+		return 0;
 	}
 
 	/*
@@ -153,17 +153,30 @@ static void do_ppgtt_cleanup(struct i915_hw_ppgtt *ppgtt)
 
 	/* We have a problem here where VM teardown cannot be interrupted, or
 	 * else the ppgtt cleanup will fail. As an example, a precisely timed
-	 * SIGKILL could leads to an OOPS, or worse. There are two options:
-	 * 1. Make the eviction uninterruptible
-	 * 2. Defer the eviction if it was interrupted.
-	 *
-	 * Option #1 is not the friendliest, but it's the easiest to implement,
-	 * and least error prone.
-	 * TODO: Implement option 2
+	 * SIGKILL could leads to an OOPS, or worse. The real solution is to
+	 * properly track the VMA <-> OBJ relationship. This temporary bandaid
+	 * will simply defer the free until we know the seqno has passed for
+	 * this VMA.
 	 */
 	ret = i915_gem_evict_vm(&ppgtt->base, do_idle, !do_idle);
-	if (ret == -ERESTARTSYS)
-		ret = i915_gem_evict_vm(&ppgtt->base, do_idle, false);
+	if (ret == -ERESTARTSYS) {
+		struct drm_mm_node *entry;
+		/* First mark all VMAs */
+		drm_mm_for_each_node(entry, &ppgtt->base.mm) {
+			struct i915_vma *vma = container_of(entry, struct i915_vma, node);
+			vma->retire_seqno = vma->obj->last_read_seqno;
+			vma->retire_ring = vma->obj->ring;
+			/* It's okay to lose the object, we just can't lose the
+			 * VMA */
+		}
+
+		list_move_tail(&ppgtt->base.global_link, &dev_priv->ppgtt_free_list);
+		queue_delayed_work(dev_priv->wq,
+				   &dev_priv->mm.ppgtt_work,
+				   round_jiffies_up_relative(HZ));
+		return -EAGAIN;
+	}
+
 	WARN_ON(ret);
 	WARN_ON(!list_empty(&vm->active_list));
 
@@ -173,6 +186,7 @@ static void do_ppgtt_cleanup(struct i915_hw_ppgtt *ppgtt)
 	}
 
 	ppgtt->base.cleanup(&ppgtt->base);
+	return 0;
 }
 
 static void ppgtt_release(struct kref *kref)
@@ -180,8 +194,8 @@ static void ppgtt_release(struct kref *kref)
 	struct i915_hw_ppgtt *ppgtt =
 		container_of(kref, struct i915_hw_ppgtt, ref);
 
-	do_ppgtt_cleanup(ppgtt);
-	kfree(ppgtt);
+	if (!do_ppgtt_cleanup(ppgtt))
+		kfree(ppgtt);
 }
 
 static size_t get_context_alignment(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 0b2b982..ebf81cd 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1030,7 +1030,7 @@ static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
 		container_of(vm, struct i915_hw_ppgtt, base);
 
 	list_del(&vm->global_link);
-	drm_mm_takedown(&ppgtt->base.mm);
+	drm_mm_takedown(&vm->mm);
 	drm_mm_remove_node(&ppgtt->node);
 
 	gen6_ppgtt_unmap_pages(ppgtt);
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 1d75801..d0dc567 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -137,6 +137,8 @@ struct i915_vma {
 	struct hlist_node exec_node;
 	unsigned long exec_handle;
 	struct drm_i915_gem_exec_object2 *exec_entry;
+	uint32_t retire_seqno; /* Last active seqno at context desruction */
+	struct intel_engine_cs *retire_ring; /* Last ring for retire_seqno */
 
 	/**
 	 * How many users have pinned this object in GTT space. The following
-- 
1.9.0

[PATCH 14/14] drm/i915/bdw: Enable full PPGTT

[Michel Thierry]

From: Ben Widawsky <benjamin.widawsky@intel.com>

Broadwell is perfectly capable of full PPGTT. I've been using it for
some time, and seen no especially ill effects.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0098ff9..f4f0bd1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2048,7 +2048,7 @@ struct drm_i915_cmd_table {
 
 #define HAS_HW_CONTEXTS(dev)	(INTEL_INFO(dev)->gen >= 6)
 #define HAS_ALIASING_PPGTT(dev)	(INTEL_INFO(dev)->gen >= 6)
-#define HAS_PPGTT(dev)		(INTEL_INFO(dev)->gen >= 7 && !IS_GEN8(dev))
+#define HAS_PPGTT(dev)		(INTEL_INFO(dev)->gen >= 7 && !IS_CHERRYVIEW(dev))
 #define USES_PPGTT(dev)		intel_enable_ppgtt(dev, false)
 #define USES_FULL_PPGTT(dev)	intel_enable_ppgtt(dev, true)
 
-- 
1.9.0