| Message ID | 20170222114610.5819-12-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 22/02/2017 11:46, Chris Wilson wrote: > Add a mock selftest to preempt a request and check that we cancel it, > requeue the request and then complete its execution. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/selftests/i915_gem_request.c | 59 +++++++++++++++++++++++ > drivers/gpu/drm/i915/selftests/mock_request.c | 19 ++++++++ > drivers/gpu/drm/i915/selftests/mock_request.h | 2 + > 3 files changed, 80 insertions(+) > > diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_request.c b/drivers/gpu/drm/i915/selftests/i915_gem_request.c > index 9d056a86723d..c803ef60a127 100644 > --- a/drivers/gpu/drm/i915/selftests/i915_gem_request.c > +++ b/drivers/gpu/drm/i915/selftests/i915_gem_request.c > @@ -26,6 +26,7 @@ > > #include "../i915_selftest.h" > > +#include "mock_context.h" > #include "mock_gem_device.h" > > static int igt_add_request(void *arg) > @@ -181,12 +182,70 @@ static int igt_fence_wait(void *arg) > return err; > } > > +static int igt_request_rewind(void *arg) > +{ > + struct drm_i915_private *i915 = arg; > + struct drm_i915_gem_request *request, *vip; > + struct i915_gem_context *ctx[2]; > + int err = -EINVAL; > + > + mutex_lock(&i915->drm.struct_mutex); > + ctx[0] = mock_context(i915, "A"); > + request = mock_request(i915->engine[RCS], ctx[0], 2 * HZ); > + if (!request) { > + err = -ENOMEM; > + goto err_device; Leaks ctx[0]. > + } > + i915_add_request(request); > + > + ctx[1] = mock_context(i915, "B"); > + vip = mock_request(i915->engine[RCS], ctx[1], 0); > + if (!vip) { > + err = -ENOMEM; > + goto err_locked; Leaks the request. > + } > + > + /* Simulate preemption by manual reordering */ > + if (!mock_cancel_request(request)) { > + pr_err("failed to cancel request (already executed)!\n"); > + i915_add_request(vip); > + goto err_locked; > + } > + i915_add_request(vip); > + request->engine->submit_request(request); > + > + mutex_unlock(&i915->drm.struct_mutex); > + > + if (i915_wait_request(vip, 0, HZ) == -ETIME) { > + pr_err("timed out waiting for high priority request, vip.seqno=%d, current seqno=%d\n", > + vip->global_seqno, intel_engine_get_seqno(i915->engine[RCS])); > + goto err; > + } > + > + if (i915_gem_request_completed(request)) { > + pr_err("low priority request already completed\n"); > + goto err; > + } > + > + err = 0; > +err: > + mutex_lock(&i915->drm.struct_mutex); > +err_locked: > + mock_context_close(ctx[1]); > + mock_context_close(ctx[0]); > +err_device: > + mock_device_flush(i915); > + mutex_unlock(&i915->drm.struct_mutex); > + return err; > +} > + > int i915_gem_request_mock_selftests(void) > { > static const struct i915_subtest tests[] = { > SUBTEST(igt_add_request), > SUBTEST(igt_wait_request), > SUBTEST(igt_fence_wait), > + SUBTEST(igt_request_rewind), > }; > struct drm_i915_private *i915; > int err; > diff --git a/drivers/gpu/drm/i915/selftests/mock_request.c b/drivers/gpu/drm/i915/selftests/mock_request.c > index e23242d1b88a..0e8d2e7f8c70 100644 > --- a/drivers/gpu/drm/i915/selftests/mock_request.c > +++ b/drivers/gpu/drm/i915/selftests/mock_request.c > @@ -22,6 +22,7 @@ > * > */ > > +#include "mock_engine.h" > #include "mock_request.h" > > struct drm_i915_gem_request * > @@ -42,3 +43,21 @@ mock_request(struct intel_engine_cs *engine, > > return &mock->base; > } > + > +bool mock_cancel_request(struct drm_i915_gem_request *request) > +{ > + struct mock_request *mock = container_of(request, typeof(*mock), base); > + struct mock_engine *engine = > + container_of(request->engine, typeof(*engine), base); > + bool was_queued; > + > + spin_lock_irq(&engine->hw_lock); > + was_queued = !list_empty(&mock->link); I suggest a better name for the mock request. Even just req would be better IMO. > + list_del_init(&mock->link); > + spin_unlock_irq(&engine->hw_lock); > + > + if (was_queued) > + i915_gem_request_unsubmit(request); > + > + return was_queued; > +} > diff --git a/drivers/gpu/drm/i915/selftests/mock_request.h b/drivers/gpu/drm/i915/selftests/mock_request.h > index cc76d4f4eb4e..4dea74c8e96d 100644 > --- a/drivers/gpu/drm/i915/selftests/mock_request.h > +++ b/drivers/gpu/drm/i915/selftests/mock_request.h > @@ -41,4 +41,6 @@ mock_request(struct intel_engine_cs *engine, > struct i915_gem_context *context, > unsigned long delay); > > +bool mock_cancel_request(struct drm_i915_gem_request *request); > + > #endif /* !__MOCK_REQUEST__ */ > Regards, Tvrtko
On Wed, Feb 22, 2017 at 01:46:10PM +0000, Tvrtko Ursulin wrote: > > On 22/02/2017 11:46, Chris Wilson wrote: > >Add a mock selftest to preempt a request and check that we cancel it, > >requeue the request and then complete its execution. > > > >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >--- > > drivers/gpu/drm/i915/selftests/i915_gem_request.c | 59 +++++++++++++++++++++++ > > drivers/gpu/drm/i915/selftests/mock_request.c | 19 ++++++++ > > drivers/gpu/drm/i915/selftests/mock_request.h | 2 + > > 3 files changed, 80 insertions(+) > > > >diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_request.c b/drivers/gpu/drm/i915/selftests/i915_gem_request.c > >index 9d056a86723d..c803ef60a127 100644 > >--- a/drivers/gpu/drm/i915/selftests/i915_gem_request.c > >+++ b/drivers/gpu/drm/i915/selftests/i915_gem_request.c > >@@ -26,6 +26,7 @@ > > > > #include "../i915_selftest.h" > > > >+#include "mock_context.h" > > #include "mock_gem_device.h" > > > > static int igt_add_request(void *arg) > >@@ -181,12 +182,70 @@ static int igt_fence_wait(void *arg) > > return err; > > } > > > >+static int igt_request_rewind(void *arg) > >+{ > >+ struct drm_i915_private *i915 = arg; > >+ struct drm_i915_gem_request *request, *vip; > >+ struct i915_gem_context *ctx[2]; > >+ int err = -EINVAL; > >+ > >+ mutex_lock(&i915->drm.struct_mutex); > >+ ctx[0] = mock_context(i915, "A"); > >+ request = mock_request(i915->engine[RCS], ctx[0], 2 * HZ); > >+ if (!request) { > >+ err = -ENOMEM; > >+ goto err_device; > > Leaks ctx[0]. > > >+ } > >+ i915_add_request(request); > >+ > >+ ctx[1] = mock_context(i915, "B"); > >+ vip = mock_request(i915->engine[RCS], ctx[1], 0); > >+ if (!vip) { > >+ err = -ENOMEM; > >+ goto err_locked; > > Leaks the request. That's been handed over to the device. > >+bool mock_cancel_request(struct drm_i915_gem_request *request) > >+{ > >+ struct mock_request *mock = container_of(request, typeof(*mock), base); > >+ struct mock_engine *engine = > >+ container_of(request->engine, typeof(*engine), base); > >+ bool was_queued; > >+ > >+ spin_lock_irq(&engine->hw_lock); > >+ was_queued = !list_empty(&mock->link); > > I suggest a better name for the mock request. Even just req would be > better IMO. A bit late, the pattern has been set for the file. Currently request for drm_i915_gem_request and mock for mock_request. I definitely don't like the idea of req and request in the same function. -Chris
On Wed, Feb 22, 2017 at 01:59:13PM +0000, Chris Wilson wrote: > On Wed, Feb 22, 2017 at 01:46:10PM +0000, Tvrtko Ursulin wrote: > > > > On 22/02/2017 11:46, Chris Wilson wrote: > > >+ ctx[1] = mock_context(i915, "B"); > > >+ vip = mock_request(i915->engine[RCS], ctx[1], 0); > > >+ if (!vip) { > > >+ err = -ENOMEM; > > >+ goto err_locked; > > > > Leaks the request. > > That's been handed over to the device. However, we shouldn't assume that the test harness is functioning perfectly and the request remains valid as we poke it without a reference of our own. -Chris
On 22/02/2017 13:59, Chris Wilson wrote: > On Wed, Feb 22, 2017 at 01:46:10PM +0000, Tvrtko Ursulin wrote: >> >> On 22/02/2017 11:46, Chris Wilson wrote: >>> Add a mock selftest to preempt a request and check that we cancel it, >>> requeue the request and then complete its execution. >>> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> --- >>> drivers/gpu/drm/i915/selftests/i915_gem_request.c | 59 +++++++++++++++++++++++ >>> drivers/gpu/drm/i915/selftests/mock_request.c | 19 ++++++++ >>> drivers/gpu/drm/i915/selftests/mock_request.h | 2 + >>> 3 files changed, 80 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_request.c b/drivers/gpu/drm/i915/selftests/i915_gem_request.c >>> index 9d056a86723d..c803ef60a127 100644 >>> --- a/drivers/gpu/drm/i915/selftests/i915_gem_request.c >>> +++ b/drivers/gpu/drm/i915/selftests/i915_gem_request.c >>> @@ -26,6 +26,7 @@ >>> >>> #include "../i915_selftest.h" >>> >>> +#include "mock_context.h" >>> #include "mock_gem_device.h" >>> >>> static int igt_add_request(void *arg) >>> @@ -181,12 +182,70 @@ static int igt_fence_wait(void *arg) >>> return err; >>> } >>> >>> +static int igt_request_rewind(void *arg) >>> +{ >>> + struct drm_i915_private *i915 = arg; >>> + struct drm_i915_gem_request *request, *vip; >>> + struct i915_gem_context *ctx[2]; >>> + int err = -EINVAL; >>> + >>> + mutex_lock(&i915->drm.struct_mutex); >>> + ctx[0] = mock_context(i915, "A"); >>> + request = mock_request(i915->engine[RCS], ctx[0], 2 * HZ); >>> + if (!request) { >>> + err = -ENOMEM; >>> + goto err_device; >> >> Leaks ctx[0]. >> >>> + } >>> + i915_add_request(request); >>> + >>> + ctx[1] = mock_context(i915, "B"); >>> + vip = mock_request(i915->engine[RCS], ctx[1], 0); >>> + if (!vip) { >>> + err = -ENOMEM; >>> + goto err_locked; >> >> Leaks the request. > > That's been handed over to the device. > >>> +bool mock_cancel_request(struct drm_i915_gem_request *request) >>> +{ >>> + struct mock_request *mock = container_of(request, typeof(*mock), base); >>> + struct mock_engine *engine = >>> + container_of(request->engine, typeof(*engine), base); >>> + bool was_queued; >>> + >>> + spin_lock_irq(&engine->hw_lock); >>> + was_queued = !list_empty(&mock->link); >> >> I suggest a better name for the mock request. Even just req would be >> better IMO. > > A bit late, the pattern has been set for the file. Currently > request for drm_i915_gem_request and mock for mock_request. I definitely > don't like the idea of req and request in the same function. Pattern? Try grep "mock_request *" on selftests/* ! :) Okay.. I r-b'ed some of those so I'll shut up. Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_request.c b/drivers/gpu/drm/i915/selftests/i915_gem_request.c index 9d056a86723d..c803ef60a127 100644 --- a/drivers/gpu/drm/i915/selftests/i915_gem_request.c +++ b/drivers/gpu/drm/i915/selftests/i915_gem_request.c @@ -26,6 +26,7 @@ #include "../i915_selftest.h" +#include "mock_context.h" #include "mock_gem_device.h" static int igt_add_request(void *arg) @@ -181,12 +182,70 @@ static int igt_fence_wait(void *arg) return err; } +static int igt_request_rewind(void *arg) +{ + struct drm_i915_private *i915 = arg; + struct drm_i915_gem_request *request, *vip; + struct i915_gem_context *ctx[2]; + int err = -EINVAL; + + mutex_lock(&i915->drm.struct_mutex); + ctx[0] = mock_context(i915, "A"); + request = mock_request(i915->engine[RCS], ctx[0], 2 * HZ); + if (!request) { + err = -ENOMEM; + goto err_device; + } + i915_add_request(request); + + ctx[1] = mock_context(i915, "B"); + vip = mock_request(i915->engine[RCS], ctx[1], 0); + if (!vip) { + err = -ENOMEM; + goto err_locked; + } + + /* Simulate preemption by manual reordering */ + if (!mock_cancel_request(request)) { + pr_err("failed to cancel request (already executed)!\n"); + i915_add_request(vip); + goto err_locked; + } + i915_add_request(vip); + request->engine->submit_request(request); + + mutex_unlock(&i915->drm.struct_mutex); + + if (i915_wait_request(vip, 0, HZ) == -ETIME) { + pr_err("timed out waiting for high priority request, vip.seqno=%d, current seqno=%d\n", + vip->global_seqno, intel_engine_get_seqno(i915->engine[RCS])); + goto err; + } + + if (i915_gem_request_completed(request)) { + pr_err("low priority request already completed\n"); + goto err; + } + + err = 0; +err: + mutex_lock(&i915->drm.struct_mutex); +err_locked: + mock_context_close(ctx[1]); + mock_context_close(ctx[0]); +err_device: + mock_device_flush(i915); + mutex_unlock(&i915->drm.struct_mutex); + return err; +} + int i915_gem_request_mock_selftests(void) { static const struct i915_subtest tests[] = { SUBTEST(igt_add_request), SUBTEST(igt_wait_request), SUBTEST(igt_fence_wait), + SUBTEST(igt_request_rewind), }; struct drm_i915_private *i915; int err; diff --git a/drivers/gpu/drm/i915/selftests/mock_request.c b/drivers/gpu/drm/i915/selftests/mock_request.c index e23242d1b88a..0e8d2e7f8c70 100644 --- a/drivers/gpu/drm/i915/selftests/mock_request.c +++ b/drivers/gpu/drm/i915/selftests/mock_request.c @@ -22,6 +22,7 @@ * */ +#include "mock_engine.h" #include "mock_request.h" struct drm_i915_gem_request * @@ -42,3 +43,21 @@ mock_request(struct intel_engine_cs *engine, return &mock->base; } + +bool mock_cancel_request(struct drm_i915_gem_request *request) +{ + struct mock_request *mock = container_of(request, typeof(*mock), base); + struct mock_engine *engine = + container_of(request->engine, typeof(*engine), base); + bool was_queued; + + spin_lock_irq(&engine->hw_lock); + was_queued = !list_empty(&mock->link); + list_del_init(&mock->link); + spin_unlock_irq(&engine->hw_lock); + + if (was_queued) + i915_gem_request_unsubmit(request); + + return was_queued; +} diff --git a/drivers/gpu/drm/i915/selftests/mock_request.h b/drivers/gpu/drm/i915/selftests/mock_request.h index cc76d4f4eb4e..4dea74c8e96d 100644 --- a/drivers/gpu/drm/i915/selftests/mock_request.h +++ b/drivers/gpu/drm/i915/selftests/mock_request.h @@ -41,4 +41,6 @@ mock_request(struct intel_engine_cs *engine, struct i915_gem_context *context, unsigned long delay); +bool mock_cancel_request(struct drm_i915_gem_request *request); + #endif /* !__MOCK_REQUEST__ */
Add a mock selftest to preempt a request and check that we cancel it, requeue the request and then complete its execution. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/selftests/i915_gem_request.c | 59 +++++++++++++++++++++++ drivers/gpu/drm/i915/selftests/mock_request.c | 19 ++++++++ drivers/gpu/drm/i915/selftests/mock_request.h | 2 + 3 files changed, 80 insertions(+)