| Message ID | 20190322092155.1656-13-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [i-g-t,01/24] i915/gem_exec_latency: Measure the latency of context switching | expand |
On 22/03/2019 09:21, Chris Wilson wrote: > Exercise reusing the GTT of one ctx in another. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > tests/i915/gem_ctx_param.c | 83 ++++++++++++++++++++++++++++++++------ > 1 file changed, 71 insertions(+), 12 deletions(-) > > diff --git a/tests/i915/gem_ctx_param.c b/tests/i915/gem_ctx_param.c > index b3f8637df..54ade8b4b 100644 > --- a/tests/i915/gem_ctx_param.c > +++ b/tests/i915/gem_ctx_param.c > @@ -36,17 +36,6 @@ IGT_TEST_DESCRIPTION("Basic test for context set/get param input validation."); > #define NEW_CTX BIT(0) > #define USER BIT(1) > > -static int reopen_driver(int fd) > -{ > - char path[256]; > - > - snprintf(path, sizeof(path), "/proc/self/fd/%d", fd); > - fd = open(path, O_RDWR); > - igt_assert_lte(0, fd); > - > - return fd; > -} > - > static void set_priority(int i915) > { > static const int64_t test_values[] = { > @@ -91,7 +80,7 @@ static void set_priority(int i915) > igt_permute_array(values, size, igt_exchange_int64); > > igt_fork(flags, NEW_CTX | USER) { > - int fd = reopen_driver(i915); > + int fd = gem_reopen_driver(i915); > struct drm_i915_gem_context_param arg = { > .param = I915_CONTEXT_PARAM_PRIORITY, > .ctx_id = flags & NEW_CTX ? gem_context_create(fd) : 0, > @@ -143,6 +132,73 @@ static void set_priority(int i915) > free(values); > } > > +static uint32_t __batch_create(int i915, uint32_t offset) > +{ > + const uint32_t bbe = MI_BATCH_BUFFER_END; > + uint32_t handle; > + > + handle = gem_create(i915, ALIGN(offset + 4, 4096)); > + gem_write(i915, handle, offset, &bbe, sizeof(bbe)); > + > + return handle; > +} > + > +static uint32_t batch_create(int i915) > +{ > + return __batch_create(i915, 0); > +} Looks familiar. :) > + > +static void test_vm(int i915) > +{ > + struct drm_i915_gem_exec_object2 batch = { > + .handle = batch_create(i915), > + }; > + struct drm_i915_gem_execbuffer2 eb = { > + .buffers_ptr = to_user_pointer(&batch), > + .buffer_count = 1, > + }; > + struct drm_i915_gem_context_param arg = { > + .param = I915_CONTEXT_PARAM_VM, > + }; > + uint32_t parent, child; > + > + arg.value = -1ull; > + igt_require(__gem_context_set_param(i915, &arg) == -ENOENT); > + > + parent = gem_context_create(i915); > + child = gem_context_create(i915); > + > + eb.rsvd1 = parent; > + batch.offset = 48 << 20; > + gem_execbuf(i915, &eb); > + igt_assert_eq_u64(batch.offset, 48 << 20); > + > + eb.rsvd1 = child; > + batch.offset = 0; > + gem_execbuf(i915, &eb); > + igt_assert_eq_u64(batch.offset, 0); > + > + eb.rsvd1 = parent; > + gem_execbuf(i915, &eb); > + igt_assert_eq_u64(batch.offset, 48 << 20); Please drop a comment at the start of these execbuf operations to explain what and why. We don't need softpin to guarantee they will get pinned to where we want them to? > + > + arg.ctx_id = parent; > + gem_context_get_param(i915, &arg); > + > + arg.ctx_id = child; > + gem_context_set_param(i915, &arg); Another get param to assert child vm id is the same as the parent? Also, try self-assign? I mean set the same vm id as already have? > + > + eb.rsvd1 = child; > + gem_execbuf(i915, &eb); > + igt_assert_eq_u64(batch.offset, 48 << 20); Interesting, for me at least. Please put a comment here. > + > + gem_context_destroy(i915, child); > + gem_context_destroy(i915, parent); > + > + gem_sync(i915, batch.handle); > + gem_close(i915, batch.handle); > +} > + > igt_main > { > struct drm_i915_gem_context_param arg; > @@ -253,6 +309,9 @@ igt_main > gem_context_set_param(fd, &arg); > } > > + igt_subtest("vm") > + test_vm(fd); > + > arg.param = I915_CONTEXT_PARAM_PRIORITY; > > igt_subtest("set-priority-not-supported") { > Add to basic test list? Or call basic-vm? Honestly don't remember how we do it these days.. Regards, Tvrtko
On 26/03/2019 10:22, Tvrtko Ursulin wrote: > > On 22/03/2019 09:21, Chris Wilson wrote: >> Exercise reusing the GTT of one ctx in another. >> >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >> --- >> tests/i915/gem_ctx_param.c | 83 ++++++++++++++++++++++++++++++++------ >> 1 file changed, 71 insertions(+), 12 deletions(-) >> >> diff --git a/tests/i915/gem_ctx_param.c b/tests/i915/gem_ctx_param.c >> index b3f8637df..54ade8b4b 100644 >> --- a/tests/i915/gem_ctx_param.c >> +++ b/tests/i915/gem_ctx_param.c >> @@ -36,17 +36,6 @@ IGT_TEST_DESCRIPTION("Basic test for context >> set/get param input validation."); >> #define NEW_CTX BIT(0) >> #define USER BIT(1) >> -static int reopen_driver(int fd) >> -{ >> - char path[256]; >> - >> - snprintf(path, sizeof(path), "/proc/self/fd/%d", fd); >> - fd = open(path, O_RDWR); >> - igt_assert_lte(0, fd); >> - >> - return fd; >> -} >> - >> static void set_priority(int i915) >> { >> static const int64_t test_values[] = { >> @@ -91,7 +80,7 @@ static void set_priority(int i915) >> igt_permute_array(values, size, igt_exchange_int64); >> igt_fork(flags, NEW_CTX | USER) { >> - int fd = reopen_driver(i915); >> + int fd = gem_reopen_driver(i915); >> struct drm_i915_gem_context_param arg = { >> .param = I915_CONTEXT_PARAM_PRIORITY, >> .ctx_id = flags & NEW_CTX ? gem_context_create(fd) : 0, >> @@ -143,6 +132,73 @@ static void set_priority(int i915) >> free(values); >> } >> +static uint32_t __batch_create(int i915, uint32_t offset) >> +{ >> + const uint32_t bbe = MI_BATCH_BUFFER_END; >> + uint32_t handle; >> + >> + handle = gem_create(i915, ALIGN(offset + 4, 4096)); >> + gem_write(i915, handle, offset, &bbe, sizeof(bbe)); >> + >> + return handle; >> +} >> + >> +static uint32_t batch_create(int i915) >> +{ >> + return __batch_create(i915, 0); >> +} > > Looks familiar. :) > >> + >> +static void test_vm(int i915) >> +{ >> + struct drm_i915_gem_exec_object2 batch = { >> + .handle = batch_create(i915), >> + }; >> + struct drm_i915_gem_execbuffer2 eb = { >> + .buffers_ptr = to_user_pointer(&batch), >> + .buffer_count = 1, >> + }; >> + struct drm_i915_gem_context_param arg = { >> + .param = I915_CONTEXT_PARAM_VM, >> + }; >> + uint32_t parent, child; >> + >> + arg.value = -1ull; >> + igt_require(__gem_context_set_param(i915, &arg) == -ENOENT); >> + >> + parent = gem_context_create(i915); >> + child = gem_context_create(i915); >> + >> + eb.rsvd1 = parent; >> + batch.offset = 48 << 20; >> + gem_execbuf(i915, &eb); >> + igt_assert_eq_u64(batch.offset, 48 << 20); >> + >> + eb.rsvd1 = child; >> + batch.offset = 0; >> + gem_execbuf(i915, &eb); >> + igt_assert_eq_u64(batch.offset, 0); >> + >> + eb.rsvd1 = parent; >> + gem_execbuf(i915, &eb); >> + igt_assert_eq_u64(batch.offset, 48 << 20); > > Please drop a comment at the start of these execbuf operations to > explain what and why. We don't need softpin to guarantee they will get > pinned to where we want them to? > >> + >> + arg.ctx_id = parent; >> + gem_context_get_param(i915, &arg); >> + >> + arg.ctx_id = child; >> + gem_context_set_param(i915, &arg); > > Another get param to assert child vm id is the same as the parent? > > Also, try self-assign? I mean set the same vm id as already have? And a test to check vm id space is per fd - that same id can be obtained in two fds, if not too fragile/white-box wrt idr allocator. And also that different id from one fd cannot be passed to set_vm in another. This one should be robust. Regards, Tvrtko >> + >> + eb.rsvd1 = child; >> + gem_execbuf(i915, &eb); >> + igt_assert_eq_u64(batch.offset, 48 << 20); > > Interesting, for me at least. Please put a comment here. > >> + >> + gem_context_destroy(i915, child); >> + gem_context_destroy(i915, parent); >> + >> + gem_sync(i915, batch.handle); >> + gem_close(i915, batch.handle); >> +} >> + >> igt_main >> { >> struct drm_i915_gem_context_param arg; >> @@ -253,6 +309,9 @@ igt_main >> gem_context_set_param(fd, &arg); >> } >> + igt_subtest("vm") >> + test_vm(fd); >> + >> arg.param = I915_CONTEXT_PARAM_PRIORITY; >> igt_subtest("set-priority-not-supported") { >> > > Add to basic test list? Or call basic-vm? Honestly don't remember how we > do it these days.. > > Regards, > > Tvrtko >
Quoting Tvrtko Ursulin (2019-03-26 10:33:04) > > On 26/03/2019 10:22, Tvrtko Ursulin wrote: > > > > On 22/03/2019 09:21, Chris Wilson wrote: > >> +static void test_vm(int i915) > >> +{ > >> + struct drm_i915_gem_exec_object2 batch = { > >> + .handle = batch_create(i915), > >> + }; > >> + struct drm_i915_gem_execbuffer2 eb = { > >> + .buffers_ptr = to_user_pointer(&batch), > >> + .buffer_count = 1, > >> + }; > >> + struct drm_i915_gem_context_param arg = { > >> + .param = I915_CONTEXT_PARAM_VM, > >> + }; > >> + uint32_t parent, child; > >> + > >> + arg.value = -1ull; > >> + igt_require(__gem_context_set_param(i915, &arg) == -ENOENT); > >> + > >> + parent = gem_context_create(i915); > >> + child = gem_context_create(i915); > >> + > >> + eb.rsvd1 = parent; > >> + batch.offset = 48 << 20; > >> + gem_execbuf(i915, &eb); > >> + igt_assert_eq_u64(batch.offset, 48 << 20); > >> + > >> + eb.rsvd1 = child; > >> + batch.offset = 0; > >> + gem_execbuf(i915, &eb); > >> + igt_assert_eq_u64(batch.offset, 0); > >> + > >> + eb.rsvd1 = parent; > >> + gem_execbuf(i915, &eb); > >> + igt_assert_eq_u64(batch.offset, 48 << 20); > > > > Please drop a comment at the start of these execbuf operations to > > explain what and why. We don't need softpin to guarantee they will get > > pinned to where we want them to? Using explict softpinning would make the test so much harder, since it will end up exactly where you told it to be. If we kill the implicit offset, I will shed a tear but hopefully the world moves on and makes execbuf2 defunct before then. > >> + arg.ctx_id = parent; > >> + gem_context_get_param(i915, &arg); > >> + > >> + arg.ctx_id = child; > >> + gem_context_set_param(i915, &arg); > > > > Another get param to assert child vm id is the same as the parent? Seem fair, you didn't like my proof by checking the vma offset :) > > Also, try self-assign? I mean set the same vm id as already have? Yup. > And a test to check vm id space is per fd - that same id can be obtained > in two fds, if not too fragile/white-box wrt idr allocator. That's meh. I don't want to encode any information about the id themselves as part of the uABI, with the exception that 0 is the invalid value. > And also that different id from one fd cannot be passed to set_vm in > another. This one should be robust. That's just an ENOENT check for set_param. Combining the two, checking that if we obtain the same id from a second fd, that those VM are distinct does should a useful challenge. > >> + eb.rsvd1 = child; > >> + gem_execbuf(i915, &eb); > >> + igt_assert_eq_u64(batch.offset, 48 << 20); > > > > Interesting, for me at least. Please put a comment here. > > > >> + > >> + gem_context_destroy(i915, child); > >> + gem_context_destroy(i915, parent); > >> + > >> + gem_sync(i915, batch.handle); > >> + gem_close(i915, batch.handle); > >> +} > >> + > >> igt_main > >> { > >> struct drm_i915_gem_context_param arg; > >> @@ -253,6 +309,9 @@ igt_main > >> gem_context_set_param(fd, &arg); > >> } > >> + igt_subtest("vm") > >> + test_vm(fd); > >> + > >> arg.param = I915_CONTEXT_PARAM_PRIORITY; > >> igt_subtest("set-priority-not-supported") { > >> > > > > Add to basic test list? Or call basic-vm? Honestly don't remember how we > > do it these days.. This can live on the shards for exercising a corner of the API. I think the second batch of vm tests should make more interesting BAT with a bit (lot) more work to develop those into better HW coverage. -Chris
diff --git a/tests/i915/gem_ctx_param.c b/tests/i915/gem_ctx_param.c index b3f8637df..54ade8b4b 100644 --- a/tests/i915/gem_ctx_param.c +++ b/tests/i915/gem_ctx_param.c @@ -36,17 +36,6 @@ IGT_TEST_DESCRIPTION("Basic test for context set/get param input validation."); #define NEW_CTX BIT(0) #define USER BIT(1) -static int reopen_driver(int fd) -{ - char path[256]; - - snprintf(path, sizeof(path), "/proc/self/fd/%d", fd); - fd = open(path, O_RDWR); - igt_assert_lte(0, fd); - - return fd; -} - static void set_priority(int i915) { static const int64_t test_values[] = { @@ -91,7 +80,7 @@ static void set_priority(int i915) igt_permute_array(values, size, igt_exchange_int64); igt_fork(flags, NEW_CTX | USER) { - int fd = reopen_driver(i915); + int fd = gem_reopen_driver(i915); struct drm_i915_gem_context_param arg = { .param = I915_CONTEXT_PARAM_PRIORITY, .ctx_id = flags & NEW_CTX ? gem_context_create(fd) : 0, @@ -143,6 +132,73 @@ static void set_priority(int i915) free(values); } +static uint32_t __batch_create(int i915, uint32_t offset) +{ + const uint32_t bbe = MI_BATCH_BUFFER_END; + uint32_t handle; + + handle = gem_create(i915, ALIGN(offset + 4, 4096)); + gem_write(i915, handle, offset, &bbe, sizeof(bbe)); + + return handle; +} + +static uint32_t batch_create(int i915) +{ + return __batch_create(i915, 0); +} + +static void test_vm(int i915) +{ + struct drm_i915_gem_exec_object2 batch = { + .handle = batch_create(i915), + }; + struct drm_i915_gem_execbuffer2 eb = { + .buffers_ptr = to_user_pointer(&batch), + .buffer_count = 1, + }; + struct drm_i915_gem_context_param arg = { + .param = I915_CONTEXT_PARAM_VM, + }; + uint32_t parent, child; + + arg.value = -1ull; + igt_require(__gem_context_set_param(i915, &arg) == -ENOENT); + + parent = gem_context_create(i915); + child = gem_context_create(i915); + + eb.rsvd1 = parent; + batch.offset = 48 << 20; + gem_execbuf(i915, &eb); + igt_assert_eq_u64(batch.offset, 48 << 20); + + eb.rsvd1 = child; + batch.offset = 0; + gem_execbuf(i915, &eb); + igt_assert_eq_u64(batch.offset, 0); + + eb.rsvd1 = parent; + gem_execbuf(i915, &eb); + igt_assert_eq_u64(batch.offset, 48 << 20); + + arg.ctx_id = parent; + gem_context_get_param(i915, &arg); + + arg.ctx_id = child; + gem_context_set_param(i915, &arg); + + eb.rsvd1 = child; + gem_execbuf(i915, &eb); + igt_assert_eq_u64(batch.offset, 48 << 20); + + gem_context_destroy(i915, child); + gem_context_destroy(i915, parent); + + gem_sync(i915, batch.handle); + gem_close(i915, batch.handle); +} + igt_main { struct drm_i915_gem_context_param arg; @@ -253,6 +309,9 @@ igt_main gem_context_set_param(fd, &arg); } + igt_subtest("vm") + test_vm(fd); + arg.param = I915_CONTEXT_PARAM_PRIORITY; igt_subtest("set-priority-not-supported") {
Exercise reusing the GTT of one ctx in another. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- tests/i915/gem_ctx_param.c | 83 ++++++++++++++++++++++++++++++++------ 1 file changed, 71 insertions(+), 12 deletions(-)