* [PATCH v2 2/3] drm/i915: Wrap a timer into a i915_sw_fence
2017-10-10 21:38 [PATCH v2 1/3] drm/i915: Fix eviction when the GGTT is idle but full Chris Wilson
@ 2017-10-10 21:38 ` Chris Wilson
2017-10-11 12:20 ` Tvrtko Ursulin
2017-10-10 21:38 ` [PATCH v2 3/3] drm/i915/selftests: Exercise adding requests to a full GGTT Chris Wilson
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Chris Wilson @ 2017-10-10 21:38 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v2 2/3] drm/i915: Wrap a timer into a i915_sw_fence
2017-10-10 21:38 ` [PATCH v2 2/3] drm/i915: Wrap a timer into a i915_sw_fence Chris Wilson
@ 2017-10-11 12:20 ` Tvrtko Ursulin
2017-10-11 12:34 ` Chris Wilson
0 siblings, 1 reply; 11+ messages in thread
From: Tvrtko Ursulin @ 2017-10-11 12:20 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2 2/3] drm/i915: Wrap a timer into a i915_sw_fence
2017-10-11 12:20 ` Tvrtko Ursulin
@ 2017-10-11 12:34 ` Chris Wilson
0 siblings, 0 replies; 11+ messages in thread
From: Chris Wilson @ 2017-10-11 12:34 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 3/3] drm/i915/selftests: Exercise adding requests to a full GGTT
2017-10-10 21:38 [PATCH v2 1/3] drm/i915: Fix eviction when the GGTT is idle but full Chris Wilson
2017-10-10 21:38 ` [PATCH v2 2/3] drm/i915: Wrap a timer into a i915_sw_fence Chris Wilson
@ 2017-10-10 21:38 ` Chris Wilson
2017-10-11 12:33 ` Tvrtko Ursulin
2017-10-10 22:09 ` ✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm/i915: Fix eviction when the GGTT is idle but full Patchwork
` (2 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Chris Wilson @ 2017-10-10 21:38 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v2 3/3] drm/i915/selftests: Exercise adding requests to a full GGTT
2017-10-10 21:38 ` [PATCH v2 3/3] drm/i915/selftests: Exercise adding requests to a full GGTT Chris Wilson
@ 2017-10-11 12:33 ` Tvrtko Ursulin
2017-10-11 12:45 ` Chris Wilson
0 siblings, 1 reply; 11+ messages in thread
From: Tvrtko Ursulin @ 2017-10-11 12:33 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2 3/3] drm/i915/selftests: Exercise adding requests to a full GGTT
2017-10-11 12:33 ` Tvrtko Ursulin
@ 2017-10-11 12:45 ` Chris Wilson
0 siblings, 0 replies; 11+ messages in thread
From: Chris Wilson @ 2017-10-11 12:45 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* ✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm/i915: Fix eviction when the GGTT is idle but full
2017-10-10 21:38 [PATCH v2 1/3] drm/i915: Fix eviction when the GGTT is idle but full Chris Wilson
2017-10-10 21:38 ` [PATCH v2 2/3] drm/i915: Wrap a timer into a i915_sw_fence Chris Wilson
2017-10-10 21:38 ` [PATCH v2 3/3] drm/i915/selftests: Exercise adding requests to a full GGTT Chris Wilson
@ 2017-10-10 22:09 ` Patchwork
2017-10-11 12:05 ` [PATCH v2 1/3] " Tvrtko Ursulin
2017-10-11 12:11 ` ✗ Fi.CI.IGT: failure for series starting with [v2,1/3] " Patchwork
4 siblings, 0 replies; 11+ messages in thread
From: Patchwork @ 2017-10-10 22:09 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] drm/i915: Fix eviction when the GGTT is idle but full
2017-10-10 21:38 [PATCH v2 1/3] drm/i915: Fix eviction when the GGTT is idle but full Chris Wilson
` (2 preceding siblings ...)
2017-10-10 22:09 ` ✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm/i915: Fix eviction when the GGTT is idle but full Patchwork
@ 2017-10-11 12:05 ` Tvrtko Ursulin
2017-10-11 12:12 ` Chris Wilson
2017-10-11 12:11 ` ✗ Fi.CI.IGT: failure for series starting with [v2,1/3] " Patchwork
4 siblings, 1 reply; 11+ messages in thread
From: Tvrtko Ursulin @ 2017-10-11 12:05 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2 1/3] drm/i915: Fix eviction when the GGTT is idle but full
2017-10-11 12:05 ` [PATCH v2 1/3] " Tvrtko Ursulin
@ 2017-10-11 12:12 ` Chris Wilson
0 siblings, 0 replies; 11+ messages in thread
From: Chris Wilson @ 2017-10-11 12:12 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* ✗ Fi.CI.IGT: failure for series starting with [v2,1/3] drm/i915: Fix eviction when the GGTT is idle but full
2017-10-10 21:38 [PATCH v2 1/3] drm/i915: Fix eviction when the GGTT is idle but full Chris Wilson
` (3 preceding siblings ...)
2017-10-11 12:05 ` [PATCH v2 1/3] " Tvrtko Ursulin
@ 2017-10-11 12:11 ` Patchwork
4 siblings, 0 replies; 11+ messages in thread
From: Patchwork @ 2017-10-11 12:11 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== 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
^ permalink raw reply [flat|nested] 11+ messages in thread