[PATCH v2 1/3] drm/i915: Fix eviction when the GGTT is idle but full

[PATCH v2 1/3] drm/i915: Fix eviction when the GGTT is idle but full

[Chris Wilson]

In the full-ppgtt world, we can fill the GGTT full of context objects.
These context objects are currently implicitly tracked by the requests
that pin them i.e. they are only unpinned when the request is completed
and retired, but we do not have the link from the vma to the request
(anymore). In order to unpin those contexts, we have to issue another
request and wait upon the switch to the kernel context.

The bug during eviction was that we assumed that a full GGTT meant we
would have requests on the GGTT timeline, and so we missed situations
where those requests where merely in flight (and when even they have not
yet been submitted to hw yet). The fix employed here is to change the
already-is-idle test to no look at the execution timeline, but count the
outstanding requests and then check that we have switched to the kernel
context. Erring on the side of overkill here just means that we stall a
little longer than may be strictly required, but we only expect to hit
this path in extreme corner cases where returning an erroneous error is
worse than the delay.

v2: Logical inversion when swapping over branches.

Fixes: 80b204bce8f2 ("drm/i915: Enable multiple timelines")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem_evict.c | 63 ++++++++++++++++++++++-------------
 1 file changed, 39 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
index a5a5b7e6daae..ee4811ffb7aa 100644
--- a/drivers/gpu/drm/i915/i915_gem_evict.c
+++ b/drivers/gpu/drm/i915/i915_gem_evict.c
@@ -33,21 +33,20 @@
 #include "intel_drv.h"
 #include "i915_trace.h"
 
-static bool ggtt_is_idle(struct drm_i915_private *dev_priv)
+static bool ggtt_is_idle(struct drm_i915_private *i915)
 {
-	struct i915_ggtt *ggtt = &dev_priv->ggtt;
-	struct intel_engine_cs *engine;
-	enum intel_engine_id id;
+       struct intel_engine_cs *engine;
+       enum intel_engine_id id;
 
-	for_each_engine(engine, dev_priv, id) {
-		struct intel_timeline *tl;
+       if (i915->gt.active_requests)
+	       return false;
 
-		tl = &ggtt->base.timeline.engine[engine->id];
-		if (i915_gem_active_isset(&tl->last_request))
-			return false;
-	}
+       for_each_engine(engine, i915, id) {
+	       if (engine->last_retired_context != i915->kernel_context)
+		       return false;
+       }
 
-	return true;
+       return true;
 }
 
 static int ggtt_flush(struct drm_i915_private *i915)
@@ -157,7 +156,8 @@ i915_gem_evict_something(struct i915_address_space *vm,
 				    min_size, alignment, cache_level,
 				    start, end, mode);
 
-	/* Retire before we search the active list. Although we have
+	/*
+	 * Retire before we search the active list. Although we have
 	 * reasonable accuracy in our retirement lists, we may have
 	 * a stray pin (preventing eviction) that can only be resolved by
 	 * retiring.
@@ -182,7 +182,8 @@ i915_gem_evict_something(struct i915_address_space *vm,
 		BUG_ON(ret);
 	}
 
-	/* Can we unpin some objects such as idle hw contents,
+	/*
+	 * Can we unpin some objects such as idle hw contents,
 	 * or pending flips? But since only the GGTT has global entries
 	 * such as scanouts, rinbuffers and contexts, we can skip the
 	 * purge when inspecting per-process local address spaces.
@@ -190,19 +191,33 @@ i915_gem_evict_something(struct i915_address_space *vm,
 	if (!i915_is_ggtt(vm) || flags & PIN_NONBLOCK)
 		return -ENOSPC;
 
-	if (ggtt_is_idle(dev_priv)) {
-		/* If we still have pending pageflip completions, drop
-		 * back to userspace to give our workqueues time to
-		 * acquire our locks and unpin the old scanouts.
-		 */
-		return intel_has_pending_fb_unpin(dev_priv) ? -EAGAIN : -ENOSPC;
-	}
+	/*
+	 * Not everything in the GGTT is tracked via VMA using
+	 * i915_vma_move_to_active(), otherwise we could evict as required
+	 * with minimal stalling. Instead we are forced to idle the GPU and
+	 * explicitly retire outstanding requests which will then remove
+	 * the pinning for active objects such as contexts and ring,
+	 * enabling us to evict them on the next iteration.
+	 *
+	 * To ensure that all user contexts are evictable, we perform
+	 * a switch to the perma-pinned kernel context. This all also gives
+	 * us a termination condition, when the last retired context is
+	 * the kernel's there is no more we can evict.
+	 */
+	if (!ggtt_is_idle(dev_priv)) {
+		ret = ggtt_flush(dev_priv);
+		if (ret)
+			return ret;
 
-	ret = ggtt_flush(dev_priv);
-	if (ret)
-		return ret;
+		goto search_again;
+	}
 
-	goto search_again;
+	/*
+	 * If we still have pending pageflip completions, drop
+	 * back to userspace to give our workqueues time to
+	 * acquire our locks and unpin the old scanouts.
+	 */
+	return intel_has_pending_fb_unpin(dev_priv) ? -EAGAIN : -ENOSPC;
 
 found:
 	/* drm_mm doesn't allow any other other operations while
-- 
2.15.0.rc0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 2/3] drm/i915: Wrap a timer into a i915_sw_fence

[Chris Wilson]

For some selftests, we want to issue requests but delay them going to
hardware. Furthermore, we don't want those requests to block
indefinitely (or else we may hang the driver and block testing) so we
want to employ a timeout. So naturally we want a fence that is
automatically signaled by a timer.

v2: Add kselftests.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_sw_fence.c           | 64 ++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_sw_fence.h           |  3 ++
 drivers/gpu/drm/i915/selftests/i915_sw_fence.c | 43 +++++++++++++++++
 3 files changed, 110 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c
index 808ea4d5b962..388424a95ac9 100644
--- a/drivers/gpu/drm/i915/i915_sw_fence.c
+++ b/drivers/gpu/drm/i915/i915_sw_fence.c
@@ -506,6 +506,70 @@ int i915_sw_fence_await_reservation(struct i915_sw_fence *fence,
 	return ret;
 }
 
+struct timer_fence {
+	struct i915_sw_fence base;
+	struct timer_list timer;
+	struct kref ref;
+};
+
+static void timer_fence_wake(unsigned long data)
+{
+	struct timer_fence *tf = (struct timer_fence *)data;
+
+	i915_sw_fence_complete(&tf->base);
+}
+
+static void i915_sw_fence_timer_free(struct kref *ref)
+{
+	struct timer_fence *tf = container_of(ref, typeof(*tf), ref);
+
+	kfree(tf);
+}
+
+static void i915_sw_fence_timer_put(struct i915_sw_fence *fence)
+{
+	struct timer_fence *tf = container_of(fence, typeof(*tf), base);
+
+	kref_put(&tf->ref, i915_sw_fence_timer_free);
+}
+
+static int __i915_sw_fence_call
+timer_fence_notify(struct i915_sw_fence *fence, enum i915_sw_fence_notify state)
+{
+	if (state == FENCE_FREE)
+		i915_sw_fence_timer_put(fence);
+
+	return NOTIFY_DONE;
+}
+
+struct i915_sw_fence *i915_sw_fence_create_timer(long timeout, gfp_t gfp)
+{
+	struct timer_fence *tf;
+
+	tf = kmalloc(sizeof(*tf), gfp);
+	if (!tf)
+		return ERR_PTR(-ENOMEM);
+
+	i915_sw_fence_init(&tf->base, timer_fence_notify);
+	kref_init(&tf->ref);
+
+	setup_timer(&tf->timer, timer_fence_wake, (unsigned long)tf);
+	mod_timer(&tf->timer, timeout);
+
+	kref_get(&tf->ref);
+	return &tf->base;
+}
+
+void i915_sw_fence_timer_flush(struct i915_sw_fence *fence)
+{
+	struct timer_fence *tf = container_of(fence, typeof(*tf), base);
+
+	if (del_timer_sync(&tf->timer))
+		i915_sw_fence_complete(&tf->base);
+
+	i915_sw_fence_timer_put(fence);
+}
+
 #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
 #include "selftests/i915_sw_fence.c"
 #endif
diff --git a/drivers/gpu/drm/i915/i915_sw_fence.h b/drivers/gpu/drm/i915/i915_sw_fence.h
index fe2ef4dadfc6..c111a89a927a 100644
--- a/drivers/gpu/drm/i915/i915_sw_fence.h
+++ b/drivers/gpu/drm/i915/i915_sw_fence.h
@@ -61,6 +61,9 @@ void i915_sw_fence_fini(struct i915_sw_fence *fence);
 static inline void i915_sw_fence_fini(struct i915_sw_fence *fence) {}
 #endif
 
+struct i915_sw_fence *i915_sw_fence_create_timer(long timeout, gfp_t gfp);
+void i915_sw_fence_timer_flush(struct i915_sw_fence *fence);
+
 void i915_sw_fence_commit(struct i915_sw_fence *fence);
 
 int i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence,
diff --git a/drivers/gpu/drm/i915/selftests/i915_sw_fence.c b/drivers/gpu/drm/i915/selftests/i915_sw_fence.c
index 19d145d6bf52..e51ab4310e1e 100644
--- a/drivers/gpu/drm/i915/selftests/i915_sw_fence.c
+++ b/drivers/gpu/drm/i915/selftests/i915_sw_fence.c
@@ -24,6 +24,7 @@
 
 #include <linux/completion.h>
 #include <linux/delay.h>
+#include <linux/prime_numbers.h>
 
 #include "../i915_selftest.h"
 
@@ -565,6 +566,47 @@ static int test_ipc(void *arg)
 	return ret;
 }
 
+static int test_timer(void *arg)
+{
+	struct i915_sw_fence *fence;
+	unsigned long target, delay;
+
+	fence = i915_sw_fence_create_timer(target = jiffies, GFP_KERNEL);
+	if (!i915_sw_fence_done(fence)) {
+		pr_err("Fence with immediate expiration not signaled\n");
+		goto err;
+	}
+	i915_sw_fence_timer_flush(fence);
+
+	for_each_prime_number(delay, HZ/2) {
+		fence = i915_sw_fence_create_timer(target = jiffies + delay,
+						   GFP_KERNEL);
+		if (i915_sw_fence_done(fence)) {
+			pr_err("Fence with future expiration (%lu jiffies) already signaled\n", delay);
+			goto err;
+		}
+
+		i915_sw_fence_wait(fence);
+		if (!i915_sw_fence_done(fence)) {
+			pr_err("Fence not signaled after wait\n");
+			goto err;
+		}
+		if (time_before(jiffies, target)) {
+			pr_err("Fence signaled too early, target=%lu, now=%lu\n",
+			       target, jiffies);
+			goto err;
+		}
+
+		i915_sw_fence_timer_flush(fence);
+	}
+
+	return 0;
+
+err:
+	i915_sw_fence_timer_flush(fence);
+	return -EINVAL;
+}
+
 int i915_sw_fence_mock_selftests(void)
 {
 	static const struct i915_subtest tests[] = {
@@ -576,6 +618,7 @@ int i915_sw_fence_mock_selftests(void)
 		SUBTEST(test_C_AB),
 		SUBTEST(test_chain),
 		SUBTEST(test_ipc),
+		SUBTEST(test_timer),
 	};
 
 	return i915_subtests(tests, NULL);
-- 
2.15.0.rc0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 3/3] drm/i915/selftests: Exercise adding requests to a full GGTT

[Chris Wilson]

A bug recently encountered involved the issue where are we were
submitting requests to different ppGTT, each would pin a segment of the
GGTT for its logical context and ring. However, this is invisible to
eviction as we do not tie the context/ring VMA to a request and so do
not automatically wait upon it them (instead they are marked as pinned,
prevent eviction entirely). Instead the eviction code must flush those
contexts by switching to the kernel context. This selftest tries to
fill the GGTT with contexts to exercise a path where the
switch-to-kernel-context failed to make forward progress and we fail
with ENOSPC.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/selftests/i915_gem_evict.c    | 121 +++++++++++++++++++++
 .../gpu/drm/i915/selftests/i915_live_selftests.h   |   1 +
 drivers/gpu/drm/i915/selftests/mock_context.c      |   6 +-
 3 files changed, 123 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_evict.c b/drivers/gpu/drm/i915/selftests/i915_gem_evict.c
index 5ea373221f49..53df8926be7f 100644
--- a/drivers/gpu/drm/i915/selftests/i915_gem_evict.c
+++ b/drivers/gpu/drm/i915/selftests/i915_gem_evict.c
@@ -24,6 +24,8 @@
 
 #include "../i915_selftest.h"
 
+#include "mock_context.h"
+#include "mock_drm.h"
 #include "mock_gem_device.h"
 
 static int populate_ggtt(struct drm_i915_private *i915)
@@ -325,6 +327,116 @@ static int igt_evict_vm(void *arg)
 	return err;
 }
 
+static int igt_evict_contexts(void *arg)
+{
+	struct drm_i915_private *i915 = arg;
+	struct intel_engine_cs *engine;
+	enum intel_engine_id id;
+	struct reserved {
+		struct drm_mm_node node;
+		struct reserved *next;
+	} *reserved = NULL;
+	unsigned long count;
+	int err = 0;
+
+	/* Make the GGTT appear small (but leave just enough to function) */
+	count = 0;
+	mutex_lock(&i915->drm.struct_mutex);
+	do {
+		struct reserved *r;
+
+		r = kcalloc(1, sizeof(*r), GFP_KERNEL);
+		if (!r) {
+			err = -ENOMEM;
+			goto out_locked;
+		}
+
+		if (i915_gem_gtt_insert(&i915->ggtt.base, &r->node,
+					1ul << 20, 0, I915_COLOR_UNEVICTABLE,
+					16ul << 20, i915->ggtt.base.total,
+					PIN_NOEVICT)) {
+			kfree(r);
+			break;
+		}
+
+		r->next = reserved;
+		reserved = r;
+
+		count++;
+	} while (1);
+	mutex_unlock(&i915->drm.struct_mutex);
+	pr_info("Filled GGTT with %lu 1MiB nodes\n", count);
+
+	/* Overfill the GGTT with context objects and so try to evict one. */
+	for_each_engine(engine, i915, id) {
+		struct i915_sw_fence *fence;
+		struct drm_file *file;
+		unsigned long count = 0;
+		unsigned long timeout;
+
+		file = mock_file(i915);
+		if (IS_ERR(file))
+			return PTR_ERR(file);
+
+		timeout = round_jiffies_up(jiffies + HZ/2);
+		fence = i915_sw_fence_create_timer(timeout, GFP_KERNEL);
+		if (IS_ERR(fence))
+			return PTR_ERR(fence);
+
+		count = 0;
+		mutex_lock(&i915->drm.struct_mutex);
+		do {
+			struct drm_i915_gem_request *rq;
+			struct i915_gem_context *ctx;
+
+			ctx = live_context(i915, file);
+			if (!ctx)
+				break;
+
+			rq = i915_gem_request_alloc(engine, ctx);
+			if (IS_ERR(rq)) {
+				if (PTR_ERR(rq) != -ENOMEM) {
+					pr_err("Unexpected error from request alloc (ctx hw id %u, on %s): %d\n",
+					       ctx->hw_id, engine->name,
+					       (int)PTR_ERR(rq));
+					err = PTR_ERR(rq);
+				}
+				break;
+			}
+
+			i915_sw_fence_await_sw_fence_gfp(&rq->submit, fence,
+							 GFP_KERNEL);
+
+			i915_add_request(rq);
+			count++;
+		} while(!i915_sw_fence_done(fence));
+		mutex_unlock(&i915->drm.struct_mutex);
+
+		i915_sw_fence_timer_flush(fence);
+		pr_info("Submitted %lu contexts/requests on %s\n",
+			count, engine->name);
+
+		mock_file_free(i915, file);
+
+		if (err)
+			break;
+	}
+
+	mutex_lock(&i915->drm.struct_mutex);
+out_locked:
+	while (reserved) {
+		struct reserved *next = reserved->next;
+
+		drm_mm_remove_node(&reserved->node);
+		kfree(reserved);
+
+		reserved = next;
+	}
+	mutex_unlock(&i915->drm.struct_mutex);
+
+	return err;
+}
+
 int i915_gem_evict_mock_selftests(void)
 {
 	static const struct i915_subtest tests[] = {
@@ -348,3 +460,12 @@ int i915_gem_evict_mock_selftests(void)
 	drm_dev_unref(&i915->drm);
 	return err;
 }
+
+int i915_gem_evict_live_selftests(struct drm_i915_private *i915)
+{
+	static const struct i915_subtest tests[] = {
+		SUBTEST(igt_evict_contexts),
+	};
+
+	return i915_subtests(tests, i915);
+}
diff --git a/drivers/gpu/drm/i915/selftests/i915_live_selftests.h b/drivers/gpu/drm/i915/selftests/i915_live_selftests.h
index 64acd7eccc5c..54a73534b37e 100644
--- a/drivers/gpu/drm/i915/selftests/i915_live_selftests.h
+++ b/drivers/gpu/drm/i915/selftests/i915_live_selftests.h
@@ -15,6 +15,7 @@ selftest(objects, i915_gem_object_live_selftests)
 selftest(dmabuf, i915_gem_dmabuf_live_selftests)
 selftest(coherency, i915_gem_coherency_live_selftests)
 selftest(gtt, i915_gem_gtt_live_selftests)
+selftest(evict, i915_gem_evict_live_selftests)
 selftest(hugepages, i915_gem_huge_page_live_selftests)
 selftest(contexts, i915_gem_context_live_selftests)
 selftest(hangcheck, intel_hangcheck_live_selftests)
diff --git a/drivers/gpu/drm/i915/selftests/mock_context.c b/drivers/gpu/drm/i915/selftests/mock_context.c
index 098ce643ad07..bbf80d42e793 100644
--- a/drivers/gpu/drm/i915/selftests/mock_context.c
+++ b/drivers/gpu/drm/i915/selftests/mock_context.c
@@ -73,11 +73,7 @@ mock_context(struct drm_i915_private *i915,
 
 void mock_context_close(struct i915_gem_context *ctx)
 {
-	i915_gem_context_set_closed(ctx);
-
-	i915_ppgtt_close(&ctx->ppgtt->base);
-
-	i915_gem_context_put(ctx);
+	context_close(ctx);
 }
 
 void mock_init_contexts(struct drm_i915_private *i915)
-- 
2.15.0.rc0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm/i915: Fix eviction when the GGTT is idle but full

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/3] drm/i915: Fix eviction when the GGTT is idle but full
URL   : https://patchwork.freedesktop.org/series/31682/
State : success

== Summary ==

Series 31682v1 series starting with [v2,1/3] drm/i915: Fix eviction when the GGTT is idle but full
https://patchwork.freedesktop.org/api/1.0/series/31682/revisions/1/mbox/

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:460s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:473s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:391s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:563s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:283s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:526s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:517s
fi-byt-j1900     total:289  pass:253  dwarn:1   dfail:0   fail:0   skip:35  time:530s
fi-cfl-s         total:289  pass:253  dwarn:4   dfail:0   fail:0   skip:32  time:564s
fi-cnl-y         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:613s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:435s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:596s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:435s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:415s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:460s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:503s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:472s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:501s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:584s
fi-kbl-7567u     total:289  pass:265  dwarn:4   dfail:0   fail:0   skip:20  time:489s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:599s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:661s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:467s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:656s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:531s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:573s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:479s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:580s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:430s
fi-byt-n2820 failed to connect after reboot

c880c004bb05a4a530f1a07e65075743ad316f68 drm-tip: 2017y-10m-10d-20h-31m-39s UTC integration manifest
0671eb9df800 drm/i915/selftests: Exercise adding requests to a full GGTT
b3d0e9173b26 drm/i915: Wrap a timer into a i915_sw_fence
d9e1ddd337be drm/i915: Fix eviction when the GGTT is idle but full

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5985/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/3] drm/i915: Fix eviction when the GGTT is idle but full

[Tvrtko Ursulin]

On 10/10/2017 22:38, Chris Wilson wrote:
> In the full-ppgtt world, we can fill the GGTT full of context objects.
> These context objects are currently implicitly tracked by the requests
> that pin them i.e. they are only unpinned when the request is completed
> and retired, but we do not have the link from the vma to the request
> (anymore). In order to unpin those contexts, we have to issue another
> request and wait upon the switch to the kernel context.
> 
> The bug during eviction was that we assumed that a full GGTT meant we
> would have requests on the GGTT timeline, and so we missed situations
> where those requests where merely in flight (and when even they have not
> yet been submitted to hw yet). The fix employed here is to change the
> already-is-idle test to no look at the execution timeline, but count the
> outstanding requests and then check that we have switched to the kernel
> context. Erring on the side of overkill here just means that we stall a
> little longer than may be strictly required, but we only expect to hit
> this path in extreme corner cases where returning an erroneous error is
> worse than the delay.
> 
> v2: Logical inversion when swapping over branches.
> 
> Fixes: 80b204bce8f2 ("drm/i915: Enable multiple timelines")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_evict.c | 63 ++++++++++++++++++++++-------------
>   1 file changed, 39 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
> index a5a5b7e6daae..ee4811ffb7aa 100644
> --- a/drivers/gpu/drm/i915/i915_gem_evict.c
> +++ b/drivers/gpu/drm/i915/i915_gem_evict.c
> @@ -33,21 +33,20 @@
>   #include "intel_drv.h"
>   #include "i915_trace.h"
>   
> -static bool ggtt_is_idle(struct drm_i915_private *dev_priv)
> +static bool ggtt_is_idle(struct drm_i915_private *i915)
>   {
> -	struct i915_ggtt *ggtt = &dev_priv->ggtt;
> -	struct intel_engine_cs *engine;
> -	enum intel_engine_id id;
> +       struct intel_engine_cs *engine;
> +       enum intel_engine_id id;
>   
> -	for_each_engine(engine, dev_priv, id) {
> -		struct intel_timeline *tl;
> +       if (i915->gt.active_requests)
> +	       return false;
>   
> -		tl = &ggtt->base.timeline.engine[engine->id];
> -		if (i915_gem_active_isset(&tl->last_request))
> -			return false;
> -	}
> +       for_each_engine(engine, i915, id) {
> +	       if (engine->last_retired_context != i915->kernel_context)
> +		       return false;
> +       }
>   
> -	return true;
> +       return true;
>   }
>   
>   static int ggtt_flush(struct drm_i915_private *i915)
> @@ -157,7 +156,8 @@ i915_gem_evict_something(struct i915_address_space *vm,
>   				    min_size, alignment, cache_level,
>   				    start, end, mode);
>   
> -	/* Retire before we search the active list. Although we have
> +	/*
> +	 * Retire before we search the active list. Although we have
>   	 * reasonable accuracy in our retirement lists, we may have
>   	 * a stray pin (preventing eviction) that can only be resolved by
>   	 * retiring.
> @@ -182,7 +182,8 @@ i915_gem_evict_something(struct i915_address_space *vm,
>   		BUG_ON(ret);
>   	}
>   
> -	/* Can we unpin some objects such as idle hw contents,
> +	/*
> +	 * Can we unpin some objects such as idle hw contents,
>   	 * or pending flips? But since only the GGTT has global entries
>   	 * such as scanouts, rinbuffers and contexts, we can skip the
>   	 * purge when inspecting per-process local address spaces.
> @@ -190,19 +191,33 @@ i915_gem_evict_something(struct i915_address_space *vm,
>   	if (!i915_is_ggtt(vm) || flags & PIN_NONBLOCK)
>   		return -ENOSPC;
>   
> -	if (ggtt_is_idle(dev_priv)) {
> -		/* If we still have pending pageflip completions, drop
> -		 * back to userspace to give our workqueues time to
> -		 * acquire our locks and unpin the old scanouts.
> -		 */
> -		return intel_has_pending_fb_unpin(dev_priv) ? -EAGAIN : -ENOSPC;
> -	}
> +	/*
> +	 * Not everything in the GGTT is tracked via VMA using
> +	 * i915_vma_move_to_active(), otherwise we could evict as required
> +	 * with minimal stalling. Instead we are forced to idle the GPU and
> +	 * explicitly retire outstanding requests which will then remove
> +	 * the pinning for active objects such as contexts and ring,
> +	 * enabling us to evict them on the next iteration.
> +	 *
> +	 * To ensure that all user contexts are evictable, we perform
> +	 * a switch to the perma-pinned kernel context. This all also gives
> +	 * us a termination condition, when the last retired context is
> +	 * the kernel's there is no more we can evict.
> +	 */
> +	if (!ggtt_is_idle(dev_priv)) {
> +		ret = ggtt_flush(dev_priv);
> +		if (ret)
> +			return ret;
>   
> -	ret = ggtt_flush(dev_priv);
> -	if (ret)
> -		return ret;
> +		goto search_again;
> +	}
>   
> -	goto search_again;
> +	/*
> +	 * If we still have pending pageflip completions, drop
> +	 * back to userspace to give our workqueues time to
> +	 * acquire our locks and unpin the old scanouts.
> +	 */
> +	return intel_has_pending_fb_unpin(dev_priv) ? -EAGAIN : -ENOSPC;
>   
>   found:
>   	/* drm_mm doesn't allow any other other operations while
> 

Looks like it will fix the bug and can't spot that it introduces a 
problem. Was there an igt which was failing or any bugzilla?

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: failure for series starting with [v2,1/3] drm/i915: Fix eviction when the GGTT is idle but full

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/3] drm/i915: Fix eviction when the GGTT is idle but full
URL   : https://patchwork.freedesktop.org/series/31682/
State : failure

== Summary ==

Test gem_userptr_blits:
        Subgroup sync-unmap-cycles:
                pass       -> DMESG-WARN (shard-hsw)
Test gem_tiled_blits:
        Subgroup normal:
                pass       -> INCOMPLETE (shard-hsw)
Test kms_setmode:
        Subgroup basic:
                fail       -> PASS       (shard-hsw) fdo#99912
Test gem_flink_race:
        Subgroup flink_close:
                pass       -> FAIL       (shard-hsw) fdo#102655

fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
fdo#102655 https://bugs.freedesktop.org/show_bug.cgi?id=102655

shard-hsw        total:2502 pass:1397 dwarn:6   dfail:0   fail:12  skip:1086 time:9270s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5985/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/3] drm/i915: Fix eviction when the GGTT is idle but full

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-10-11 13:05:02)
> 
> On 10/10/2017 22:38, Chris Wilson wrote:
> > In the full-ppgtt world, we can fill the GGTT full of context objects.
> > These context objects are currently implicitly tracked by the requests
> > that pin them i.e. they are only unpinned when the request is completed
> > and retired, but we do not have the link from the vma to the request
> > (anymore). In order to unpin those contexts, we have to issue another
> > request and wait upon the switch to the kernel context.
> > 
> > The bug during eviction was that we assumed that a full GGTT meant we
> > would have requests on the GGTT timeline, and so we missed situations
> > where those requests where merely in flight (and when even they have not
> > yet been submitted to hw yet). The fix employed here is to change the
> > already-is-idle test to no look at the execution timeline, but count the
> > outstanding requests and then check that we have switched to the kernel
> > context. Erring on the side of overkill here just means that we stall a
> > little longer than may be strictly required, but we only expect to hit
> > this path in extreme corner cases where returning an erroneous error is
> > worse than the delay.
> > 
> > v2: Logical inversion when swapping over branches.
> > 
> > Fixes: 80b204bce8f2 ("drm/i915: Enable multiple timelines")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_gem_evict.c | 63 ++++++++++++++++++++++-------------
> >   1 file changed, 39 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
> > index a5a5b7e6daae..ee4811ffb7aa 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_evict.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_evict.c
> > @@ -33,21 +33,20 @@
> >   #include "intel_drv.h"
> >   #include "i915_trace.h"
> >   
> > -static bool ggtt_is_idle(struct drm_i915_private *dev_priv)
> > +static bool ggtt_is_idle(struct drm_i915_private *i915)
> >   {
> > -     struct i915_ggtt *ggtt = &dev_priv->ggtt;
> > -     struct intel_engine_cs *engine;
> > -     enum intel_engine_id id;
> > +       struct intel_engine_cs *engine;
> > +       enum intel_engine_id id;
> >   
> > -     for_each_engine(engine, dev_priv, id) {
> > -             struct intel_timeline *tl;
> > +       if (i915->gt.active_requests)
> > +            return false;
> >   
> > -             tl = &ggtt->base.timeline.engine[engine->id];
> > -             if (i915_gem_active_isset(&tl->last_request))
> > -                     return false;
> > -     }
> > +       for_each_engine(engine, i915, id) {
> > +            if (engine->last_retired_context != i915->kernel_context)
> > +                    return false;
> > +       }
> >   
> > -     return true;
> > +       return true;
> >   }
> >   
> >   static int ggtt_flush(struct drm_i915_private *i915)
> > @@ -157,7 +156,8 @@ i915_gem_evict_something(struct i915_address_space *vm,
> >                                   min_size, alignment, cache_level,
> >                                   start, end, mode);
> >   
> > -     /* Retire before we search the active list. Although we have
> > +     /*
> > +      * Retire before we search the active list. Although we have
> >        * reasonable accuracy in our retirement lists, we may have
> >        * a stray pin (preventing eviction) that can only be resolved by
> >        * retiring.
> > @@ -182,7 +182,8 @@ i915_gem_evict_something(struct i915_address_space *vm,
> >               BUG_ON(ret);
> >       }
> >   
> > -     /* Can we unpin some objects such as idle hw contents,
> > +     /*
> > +      * Can we unpin some objects such as idle hw contents,
> >        * or pending flips? But since only the GGTT has global entries
> >        * such as scanouts, rinbuffers and contexts, we can skip the
> >        * purge when inspecting per-process local address spaces.
> > @@ -190,19 +191,33 @@ i915_gem_evict_something(struct i915_address_space *vm,
> >       if (!i915_is_ggtt(vm) || flags & PIN_NONBLOCK)
> >               return -ENOSPC;
> >   
> > -     if (ggtt_is_idle(dev_priv)) {
> > -             /* If we still have pending pageflip completions, drop
> > -              * back to userspace to give our workqueues time to
> > -              * acquire our locks and unpin the old scanouts.
> > -              */
> > -             return intel_has_pending_fb_unpin(dev_priv) ? -EAGAIN : -ENOSPC;
> > -     }
> > +     /*
> > +      * Not everything in the GGTT is tracked via VMA using
> > +      * i915_vma_move_to_active(), otherwise we could evict as required
> > +      * with minimal stalling. Instead we are forced to idle the GPU and
> > +      * explicitly retire outstanding requests which will then remove
> > +      * the pinning for active objects such as contexts and ring,
> > +      * enabling us to evict them on the next iteration.
> > +      *
> > +      * To ensure that all user contexts are evictable, we perform
> > +      * a switch to the perma-pinned kernel context. This all also gives
> > +      * us a termination condition, when the last retired context is
> > +      * the kernel's there is no more we can evict.
> > +      */
> > +     if (!ggtt_is_idle(dev_priv)) {
> > +             ret = ggtt_flush(dev_priv);
> > +             if (ret)
> > +                     return ret;
> >   
> > -     ret = ggtt_flush(dev_priv);
> > -     if (ret)
> > -             return ret;
> > +             goto search_again;
> > +     }
> >   
> > -     goto search_again;
> > +     /*
> > +      * If we still have pending pageflip completions, drop
> > +      * back to userspace to give our workqueues time to
> > +      * acquire our locks and unpin the old scanouts.
> > +      */
> > +     return intel_has_pending_fb_unpin(dev_priv) ? -EAGAIN : -ENOSPC;
> >   
> >   found:
> >       /* drm_mm doesn't allow any other other operations while
> > 
> 
> Looks like it will fix the bug and can't spot that it introduces a 
> problem. Was there an igt which was failing or any bugzilla?

No, there was an odd flip during shard testing of hugepages. Papered
over before we landed the hugepages work, but still deserved a real fix
for the bug uncovered.

I didn't succeed in writing an igt that didn't take too long or run out
of memory (8G ram trying to fill a 4G GGTT, we have some overcommit
issues ;). So I resorted to making the selftest instead, where we can
pretend like the GGTT is only a few megabytes to make the testing
tractable.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/3] drm/i915: Wrap a timer into a i915_sw_fence

[Tvrtko Ursulin]

On 10/10/2017 22:38, Chris Wilson wrote:
> For some selftests, we want to issue requests but delay them going to
> hardware. Furthermore, we don't want those requests to block
> indefinitely (or else we may hang the driver and block testing) so we
> want to employ a timeout. So naturally we want a fence that is
> automatically signaled by a timer.
> 
> v2: Add kselftests.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_sw_fence.c           | 64 ++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/i915_sw_fence.h           |  3 ++
>   drivers/gpu/drm/i915/selftests/i915_sw_fence.c | 43 +++++++++++++++++
>   3 files changed, 110 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c
> index 808ea4d5b962..388424a95ac9 100644
> --- a/drivers/gpu/drm/i915/i915_sw_fence.c
> +++ b/drivers/gpu/drm/i915/i915_sw_fence.c
> @@ -506,6 +506,70 @@ int i915_sw_fence_await_reservation(struct i915_sw_fence *fence,
>   	return ret;
>   }
>   
> +struct timer_fence {

timed fence?

> +	struct i915_sw_fence base;
> +	struct timer_list timer;
> +	struct kref ref;
> +};
> +
> +static void timer_fence_wake(unsigned long data)
> +{
> +	struct timer_fence *tf = (struct timer_fence *)data;
> +
> +	i915_sw_fence_complete(&tf->base);
> +}
> +
> +static void i915_sw_fence_timer_free(struct kref *ref)
> +{
> +	struct timer_fence *tf = container_of(ref, typeof(*tf), ref);
> +
> +	kfree(tf);
> +}
> +
> +static void i915_sw_fence_timer_put(struct i915_sw_fence *fence)
> +{
> +	struct timer_fence *tf = container_of(fence, typeof(*tf), base);
> +
> +	kref_put(&tf->ref, i915_sw_fence_timer_free);
> +}
> +
> +static int __i915_sw_fence_call
> +timer_fence_notify(struct i915_sw_fence *fence, enum i915_sw_fence_notify state)
> +{
> +	if (state == FENCE_FREE)
> +		i915_sw_fence_timer_put(fence);
> +
> +	return NOTIFY_DONE;
> +}
> +
> +struct i915_sw_fence *i915_sw_fence_create_timer(long timeout, gfp_t gfp)

Depending on the outcome of the public vs test private question (see 
below), you might want to rename timeout to expired to signify it is in 
absolute jiffies. And also add kernel doc if it will be public.

> +{
> +	struct timer_fence *tf;
> +
> +	tf = kmalloc(sizeof(*tf), gfp);
> +	if (!tf)
> +		return ERR_PTR(-ENOMEM);
> +
> +	i915_sw_fence_init(&tf->base, timer_fence_notify);
> +	kref_init(&tf->ref);
> +
> +	setup_timer(&tf->timer, timer_fence_wake, (unsigned long)tf);
> +	mod_timer(&tf->timer, timeout);
> +
> +	kref_get(&tf->ref);
> +	return &tf->base;
> +}
> +
> +void i915_sw_fence_timer_flush(struct i915_sw_fence *fence)
> +{
> +	struct timer_fence *tf = container_of(fence, typeof(*tf), base);
> +
> +	if (del_timer_sync(&tf->timer))
> +		i915_sw_fence_complete(&tf->base);
> +
> +	i915_sw_fence_timer_put(fence);

A bit unusual that flush destroys an object? Should i915_sw_fence_fini 
just be made work on these to come close to OO design it partially tries 
to do?

> +}
> +

I reckon there are some private functions it uses so it has to live in 
i915_sw_fence.c or it could be moved under selftests completely?

Quick glance - only i915_sw_fence_complete it seems? Could you use 
i915_sw_fence_commit instead? No idea what the interaction with debug 
objects there would mean.

But I think it would be much better to move it under selftests/ if possible.

Regards,

Tvrtko

>   #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
>   #include "selftests/i915_sw_fence.c"
>   #endif
> diff --git a/drivers/gpu/drm/i915/i915_sw_fence.h b/drivers/gpu/drm/i915/i915_sw_fence.h
> index fe2ef4dadfc6..c111a89a927a 100644
> --- a/drivers/gpu/drm/i915/i915_sw_fence.h
> +++ b/drivers/gpu/drm/i915/i915_sw_fence.h
> @@ -61,6 +61,9 @@ void i915_sw_fence_fini(struct i915_sw_fence *fence);
>   static inline void i915_sw_fence_fini(struct i915_sw_fence *fence) {}
>   #endif
>   
> +struct i915_sw_fence *i915_sw_fence_create_timer(long timeout, gfp_t gfp);
> +void i915_sw_fence_timer_flush(struct i915_sw_fence *fence);
> +
>   void i915_sw_fence_commit(struct i915_sw_fence *fence);
>   
>   int i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence,
> diff --git a/drivers/gpu/drm/i915/selftests/i915_sw_fence.c b/drivers/gpu/drm/i915/selftests/i915_sw_fence.c
> index 19d145d6bf52..e51ab4310e1e 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_sw_fence.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_sw_fence.c
> @@ -24,6 +24,7 @@
>   
>   #include <linux/completion.h>
>   #include <linux/delay.h>
> +#include <linux/prime_numbers.h>
>   
>   #include "../i915_selftest.h"
>   
> @@ -565,6 +566,47 @@ static int test_ipc(void *arg)
>   	return ret;
>   }
>   
> +static int test_timer(void *arg)
> +{
> +	struct i915_sw_fence *fence;
> +	unsigned long target, delay;
> +
> +	fence = i915_sw_fence_create_timer(target = jiffies, GFP_KERNEL);
> +	if (!i915_sw_fence_done(fence)) {
> +		pr_err("Fence with immediate expiration not signaled\n");
> +		goto err;
> +	}
> +	i915_sw_fence_timer_flush(fence);
> +
> +	for_each_prime_number(delay, HZ/2) {
> +		fence = i915_sw_fence_create_timer(target = jiffies + delay,
> +						   GFP_KERNEL);
> +		if (i915_sw_fence_done(fence)) {
> +			pr_err("Fence with future expiration (%lu jiffies) already signaled\n", delay);
> +			goto err;
> +		}
> +
> +		i915_sw_fence_wait(fence);
> +		if (!i915_sw_fence_done(fence)) {
> +			pr_err("Fence not signaled after wait\n");
> +			goto err;
> +		}
> +		if (time_before(jiffies, target)) {
> +			pr_err("Fence signaled too early, target=%lu, now=%lu\n",
> +			       target, jiffies);
> +			goto err;
> +		}
> +
> +		i915_sw_fence_timer_flush(fence);
> +	}
> +
> +	return 0;
> +
> +err:
> +	i915_sw_fence_timer_flush(fence);
> +	return -EINVAL;
> +}
> +
>   int i915_sw_fence_mock_selftests(void)
>   {
>   	static const struct i915_subtest tests[] = {
> @@ -576,6 +618,7 @@ int i915_sw_fence_mock_selftests(void)
>   		SUBTEST(test_C_AB),
>   		SUBTEST(test_chain),
>   		SUBTEST(test_ipc),
> +		SUBTEST(test_timer),
>   	};
>   
>   	return i915_subtests(tests, NULL);
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/3] drm/i915/selftests: Exercise adding requests to a full GGTT

[Tvrtko Ursulin]

On 10/10/2017 22:38, Chris Wilson wrote:
> A bug recently encountered involved the issue where are we were
> submitting requests to different ppGTT, each would pin a segment of the
> GGTT for its logical context and ring. However, this is invisible to
> eviction as we do not tie the context/ring VMA to a request and so do
> not automatically wait upon it them (instead they are marked as pinned,
> prevent eviction entirely). Instead the eviction code must flush those

preventing?

> contexts by switching to the kernel context. This selftest tries to
> fill the GGTT with contexts to exercise a path where the
> switch-to-kernel-context failed to make forward progress and we fail
> with ENOSPC.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/selftests/i915_gem_evict.c    | 121 +++++++++++++++++++++
>   .../gpu/drm/i915/selftests/i915_live_selftests.h   |   1 +
>   drivers/gpu/drm/i915/selftests/mock_context.c      |   6 +-
>   3 files changed, 123 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_evict.c b/drivers/gpu/drm/i915/selftests/i915_gem_evict.c
> index 5ea373221f49..53df8926be7f 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_gem_evict.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_gem_evict.c
> @@ -24,6 +24,8 @@
>   
>   #include "../i915_selftest.h"
>   
> +#include "mock_context.h"
> +#include "mock_drm.h"
>   #include "mock_gem_device.h"
>   
>   static int populate_ggtt(struct drm_i915_private *i915)
> @@ -325,6 +327,116 @@ static int igt_evict_vm(void *arg)
>   	return err;
>   }
>   
> +static int igt_evict_contexts(void *arg)
> +{
> +	struct drm_i915_private *i915 = arg;
> +	struct intel_engine_cs *engine;
> +	enum intel_engine_id id;
> +	struct reserved {
> +		struct drm_mm_node node;
> +		struct reserved *next;
> +	} *reserved = NULL;
> +	unsigned long count;
> +	int err = 0;
> +
> +	/* Make the GGTT appear small (but leave just enough to function) */

How does it do that?

> +	count = 0;
> +	mutex_lock(&i915->drm.struct_mutex);
> +	do {
> +		struct reserved *r;
> +
> +		r = kcalloc(1, sizeof(*r), GFP_KERNEL);
> +		if (!r) {
> +			err = -ENOMEM;
> +			goto out_locked;
> +		}
> +
> +		if (i915_gem_gtt_insert(&i915->ggtt.base, &r->node,
> +					1ul << 20, 0, I915_COLOR_UNEVICTABLE,
> +					16ul << 20, i915->ggtt.base.total,
> +					PIN_NOEVICT)) {
> +			kfree(r);
> +			break;
> +		}
> +
> +		r->next = reserved;
> +		reserved = r;
> +
> +		count++;
> +	} while (1);
> +	mutex_unlock(&i915->drm.struct_mutex);
> +	pr_info("Filled GGTT with %lu 1MiB nodes\n", count);

Oh right, this helps. :)

> +
> +	/* Overfill the GGTT with context objects and so try to evict one. */

Can GGTT be divisible by 1MiB in which case the above filling would fill 
everything and make any eviction impossible? Do you need an assert that 
there is a little bit of space left for below to work?

> +	for_each_engine(engine, i915, id) {
> +		struct i915_sw_fence *fence;
> +		struct drm_file *file;
> +		unsigned long count = 0;

No shadows variable warnings here?

> +		unsigned long timeout;
> +
> +		file = mock_file(i915);
> +		if (IS_ERR(file))
> +			return PTR_ERR(file);
> +
> +		timeout = round_jiffies_up(jiffies + HZ/2);
> +		fence = i915_sw_fence_create_timer(timeout, GFP_KERNEL);
> +		if (IS_ERR(fence))
> +			return PTR_ERR(fence);
> +
> +		count = 0;

Set to zero already above.

> +		mutex_lock(&i915->drm.struct_mutex);
> +		do {
> +			struct drm_i915_gem_request *rq;
> +			struct i915_gem_context *ctx;
> +
> +			ctx = live_context(i915, file);
> +			if (!ctx)
> +				break;
> +
> +			rq = i915_gem_request_alloc(engine, ctx);
> +			if (IS_ERR(rq)) {
> +				if (PTR_ERR(rq) != -ENOMEM) {
> +					pr_err("Unexpected error from request alloc (ctx hw id %u, on %s): %d\n",
> +					       ctx->hw_id, engine->name,
> +					       (int)PTR_ERR(rq));
> +					err = PTR_ERR(rq);
> +				}
> +				break;

Comment on why it is OK to stop the test on ENOMEM would be good.

> +			}
> +
> +			i915_sw_fence_await_sw_fence_gfp(&rq->submit, fence,
> +							 GFP_KERNEL);
> +
> +			i915_add_request(rq);
> +			count++;
> +		} while(!i915_sw_fence_done(fence));
> +		mutex_unlock(&i915->drm.struct_mutex);
> +
> +		i915_sw_fence_timer_flush(fence);
> +		pr_info("Submitted %lu contexts/requests on %s\n",
> +			count, engine->name);
> +
> +		mock_file_free(i915, file);
> +
> +		if (err)
> +			break;
> +	}
> +
> +	mutex_lock(&i915->drm.struct_mutex);
> +out_locked:
> +	while (reserved) {
> +		struct reserved *next = reserved->next;
> +
> +		drm_mm_remove_node(&reserved->node);
> +		kfree(reserved);
> +
> +		reserved = next;
> +	}
> +	mutex_unlock(&i915->drm.struct_mutex);
> +
> +	return err;
> +}
> +
>   int i915_gem_evict_mock_selftests(void)
>   {
>   	static const struct i915_subtest tests[] = {
> @@ -348,3 +460,12 @@ int i915_gem_evict_mock_selftests(void)
>   	drm_dev_unref(&i915->drm);
>   	return err;
>   }
> +
> +int i915_gem_evict_live_selftests(struct drm_i915_private *i915)
> +{
> +	static const struct i915_subtest tests[] = {
> +		SUBTEST(igt_evict_contexts),
> +	};
> +
> +	return i915_subtests(tests, i915);
> +}
> diff --git a/drivers/gpu/drm/i915/selftests/i915_live_selftests.h b/drivers/gpu/drm/i915/selftests/i915_live_selftests.h
> index 64acd7eccc5c..54a73534b37e 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_live_selftests.h
> +++ b/drivers/gpu/drm/i915/selftests/i915_live_selftests.h
> @@ -15,6 +15,7 @@ selftest(objects, i915_gem_object_live_selftests)
>   selftest(dmabuf, i915_gem_dmabuf_live_selftests)
>   selftest(coherency, i915_gem_coherency_live_selftests)
>   selftest(gtt, i915_gem_gtt_live_selftests)
> +selftest(evict, i915_gem_evict_live_selftests)
>   selftest(hugepages, i915_gem_huge_page_live_selftests)
>   selftest(contexts, i915_gem_context_live_selftests)
>   selftest(hangcheck, intel_hangcheck_live_selftests)
> diff --git a/drivers/gpu/drm/i915/selftests/mock_context.c b/drivers/gpu/drm/i915/selftests/mock_context.c
> index 098ce643ad07..bbf80d42e793 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_context.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_context.c
> @@ -73,11 +73,7 @@ mock_context(struct drm_i915_private *i915,
>   
>   void mock_context_close(struct i915_gem_context *ctx)
>   {
> -	i915_gem_context_set_closed(ctx);
> -
> -	i915_ppgtt_close(&ctx->ppgtt->base);
> -
> -	i915_gem_context_put(ctx);
> +	context_close(ctx);
>   }
>   
>   void mock_init_contexts(struct drm_i915_private *i915)
> 

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/3] drm/i915: Wrap a timer into a i915_sw_fence

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-10-11 13:20:25)
> 
> On 10/10/2017 22:38, Chris Wilson wrote:
> > For some selftests, we want to issue requests but delay them going to
> > hardware. Furthermore, we don't want those requests to block
> > indefinitely (or else we may hang the driver and block testing) so we
> > want to employ a timeout. So naturally we want a fence that is
> > automatically signaled by a timer.
> > 
> > v2: Add kselftests.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >   drivers/gpu/drm/i915/i915_sw_fence.c           | 64 ++++++++++++++++++++++++++
> >   drivers/gpu/drm/i915/i915_sw_fence.h           |  3 ++
> >   drivers/gpu/drm/i915/selftests/i915_sw_fence.c | 43 +++++++++++++++++
> >   3 files changed, 110 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c
> > index 808ea4d5b962..388424a95ac9 100644
> > --- a/drivers/gpu/drm/i915/i915_sw_fence.c
> > +++ b/drivers/gpu/drm/i915/i915_sw_fence.c
> > @@ -506,6 +506,70 @@ int i915_sw_fence_await_reservation(struct i915_sw_fence *fence,
> >       return ret;
> >   }
> >   
> > +struct timer_fence {
> 
> timed fence?
> 
> > +     struct i915_sw_fence base;
> > +     struct timer_list timer;
> > +     struct kref ref;
> > +};
> > +
> > +static void timer_fence_wake(unsigned long data)
> > +{
> > +     struct timer_fence *tf = (struct timer_fence *)data;
> > +
> > +     i915_sw_fence_complete(&tf->base);
> > +}
> > +
> > +static void i915_sw_fence_timer_free(struct kref *ref)
> > +{
> > +     struct timer_fence *tf = container_of(ref, typeof(*tf), ref);
> > +
> > +     kfree(tf);
> > +}
> > +
> > +static void i915_sw_fence_timer_put(struct i915_sw_fence *fence)
> > +{
> > +     struct timer_fence *tf = container_of(fence, typeof(*tf), base);
> > +
> > +     kref_put(&tf->ref, i915_sw_fence_timer_free);
> > +}
> > +
> > +static int __i915_sw_fence_call
> > +timer_fence_notify(struct i915_sw_fence *fence, enum i915_sw_fence_notify state)
> > +{
> > +     if (state == FENCE_FREE)
> > +             i915_sw_fence_timer_put(fence);
> > +
> > +     return NOTIFY_DONE;
> > +}
> > +
> > +struct i915_sw_fence *i915_sw_fence_create_timer(long timeout, gfp_t gfp)
> 
> Depending on the outcome of the public vs test private question (see 
> below), you might want to rename timeout to expired to signify it is in 
> absolute jiffies. And also add kernel doc if it will be public.

timed_fence + expires are good suggestions.

> > +{
> > +     struct timer_fence *tf;
> > +
> > +     tf = kmalloc(sizeof(*tf), gfp);
> > +     if (!tf)
> > +             return ERR_PTR(-ENOMEM);
> > +
> > +     i915_sw_fence_init(&tf->base, timer_fence_notify);
> > +     kref_init(&tf->ref);
> > +
> > +     setup_timer(&tf->timer, timer_fence_wake, (unsigned long)tf);
> > +     mod_timer(&tf->timer, timeout);
> > +
> > +     kref_get(&tf->ref);
> > +     return &tf->base;
> > +}
> > +
> > +void i915_sw_fence_timer_flush(struct i915_sw_fence *fence)
> > +{
> > +     struct timer_fence *tf = container_of(fence, typeof(*tf), base);
> > +
> > +     if (del_timer_sync(&tf->timer))
> > +             i915_sw_fence_complete(&tf->base);
> > +
> > +     i915_sw_fence_timer_put(fence);
> 
> A bit unusual that flush destroys an object? Should i915_sw_fence_fini 
> just be made work on these to come close to OO design it partially tries 
> to do?

I repeated the pattern "flush; put" everytime so I decided to hide the ref
and just have the second API point consume the fence.

Flush is the right semantic for the first action on the fence, but not a
great name for the combined operation.

I did consider doing i915_sw_fence_init_timer() and timed_fence_fini()
but wasn't as keen on exporting the struct. Though really if we follow
the selftests/ direction, that isn't an issue.
 
> I reckon there are some private functions it uses so it has to live in 
> i915_sw_fence.c or it could be moved under selftests completely?

We can push it into selftests so that it's only built then.
 
> Quick glance - only i915_sw_fence_complete it seems? Could you use 
> i915_sw_fence_commit instead? No idea what the interaction with debug 
> objects there would mean.

Originally kfence exposed both the await / complete semantics. And also
provided a kfence wrapper for a timer. Since then that timer was
absorbed into the external fence wrapper, and I'm not keen on exporting
await / complete functions and choose to export the timer instead.
(Mostly because we already have the timer interaction inside
i915_sw_fence.c so it felt a reasonable extension of that.)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/3] drm/i915/selftests: Exercise adding requests to a full GGTT

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-10-11 13:33:28)
> 
> On 10/10/2017 22:38, Chris Wilson wrote:
> > A bug recently encountered involved the issue where are we were
> > submitting requests to different ppGTT, each would pin a segment of the
> > GGTT for its logical context and ring. However, this is invisible to
> > eviction as we do not tie the context/ring VMA to a request and so do
> > not automatically wait upon it them (instead they are marked as pinned,
> > prevent eviction entirely). Instead the eviction code must flush those
> 
> preventing?
> 
> > contexts by switching to the kernel context. This selftest tries to
> > fill the GGTT with contexts to exercise a path where the
> > switch-to-kernel-context failed to make forward progress and we fail
> > with ENOSPC.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >   drivers/gpu/drm/i915/selftests/i915_gem_evict.c    | 121 +++++++++++++++++++++
> >   .../gpu/drm/i915/selftests/i915_live_selftests.h   |   1 +
> >   drivers/gpu/drm/i915/selftests/mock_context.c      |   6 +-
> >   3 files changed, 123 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_evict.c b/drivers/gpu/drm/i915/selftests/i915_gem_evict.c
> > index 5ea373221f49..53df8926be7f 100644
> > --- a/drivers/gpu/drm/i915/selftests/i915_gem_evict.c
> > +++ b/drivers/gpu/drm/i915/selftests/i915_gem_evict.c
> > @@ -24,6 +24,8 @@
> >   
> >   #include "../i915_selftest.h"
> >   
> > +#include "mock_context.h"
> > +#include "mock_drm.h"
> >   #include "mock_gem_device.h"
> >   
> >   static int populate_ggtt(struct drm_i915_private *i915)
> > @@ -325,6 +327,116 @@ static int igt_evict_vm(void *arg)
> >       return err;
> >   }
> >   
> > +static int igt_evict_contexts(void *arg)
> > +{
> > +     struct drm_i915_private *i915 = arg;
> > +     struct intel_engine_cs *engine;
> > +     enum intel_engine_id id;
> > +     struct reserved {
> > +             struct drm_mm_node node;
> > +             struct reserved *next;
> > +     } *reserved = NULL;
> > +     unsigned long count;
> > +     int err = 0;
> > +
> > +     /* Make the GGTT appear small (but leave just enough to function) */
> 
> How does it do that?
> 
> > +     count = 0;
> > +     mutex_lock(&i915->drm.struct_mutex);
> > +     do {
> > +             struct reserved *r;
> > +
> > +             r = kcalloc(1, sizeof(*r), GFP_KERNEL);
> > +             if (!r) {
> > +                     err = -ENOMEM;
> > +                     goto out_locked;
> > +             }
> > +
> > +             if (i915_gem_gtt_insert(&i915->ggtt.base, &r->node,
> > +                                     1ul << 20, 0, I915_COLOR_UNEVICTABLE,
> > +                                     16ul << 20, i915->ggtt.base.total,
> > +                                     PIN_NOEVICT)) {
> > +                     kfree(r);
> > +                     break;
> > +             }
> > +
> > +             r->next = reserved;
> > +             reserved = r;
> > +
> > +             count++;
> > +     } while (1);
> > +     mutex_unlock(&i915->drm.struct_mutex);
> > +     pr_info("Filled GGTT with %lu 1MiB nodes\n", count);
> 
> Oh right, this helps. :)
> 
> > +
> > +     /* Overfill the GGTT with context objects and so try to evict one. */
> 
> Can GGTT be divisible by 1MiB in which case the above filling would fill 
> everything and make any eviction impossible? Do you need an assert that 
> there is a little bit of space left for below to work?

There's a subtle restriction that we only try to fill above 16MiB in the
GGTT. So the assumption is that leaves us enough space to fit at least
one request/context (and its preliminary setup, such as golden render
state). Failure to have enough GGTT space to issue that first request
leads to fun. Might have to make that a little more tolerant in future.

> > +     for_each_engine(engine, i915, id) {
> > +             struct i915_sw_fence *fence;
> > +             struct drm_file *file;
> > +             unsigned long count = 0;
> 
> No shadows variable warnings here?

W=1, brave man! My bad forgot to remove as I reused it for the counter
above.

> > +             mutex_lock(&i915->drm.struct_mutex);
> > +             do {
> > +                     struct drm_i915_gem_request *rq;
> > +                     struct i915_gem_context *ctx;
> > +
> > +                     ctx = live_context(i915, file);
> > +                     if (!ctx)
> > +                             break;
> > +
> > +                     rq = i915_gem_request_alloc(engine, ctx);
> > +                     if (IS_ERR(rq)) {
> > +                             if (PTR_ERR(rq) != -ENOMEM) {
> > +                                     pr_err("Unexpected error from request alloc (ctx hw id %u, on %s): %d\n",
> > +                                            ctx->hw_id, engine->name,
> > +                                            (int)PTR_ERR(rq));
> > +                                     err = PTR_ERR(rq);
> > +                             }
> > +                             break;
> 
> Comment on why it is OK to stop the test on ENOMEM would be good.

Because the first versions ran out of memory before filling the GGTT :)
Now that there is a reasonable cap on the GGTT size, we can just let it
ENOMEM and fixup later if required.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx