| Message ID | 20180917154619.30261-1-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [i-g-t,1/2] tests/gem_ctx_bad_exec: Consolidate to gem_ctx_exec | expand |
Quoting Tvrtko Ursulin (2018-09-17 16:46:18) > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Move a really small test that invalid context is rejected under the > gem_ctx_exec umbrella. > > v2: > * And actually fix the test so it does what it claims. And add more > variety in the invalid context id's it tests with. (Chris Wilson) > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > tests/Makefile.sources | 1 - > tests/gem_ctx_bad_exec.c | 60 ---------------------------------------- > tests/gem_ctx_exec.c | 34 +++++++++++++++++++++++ > tests/meson.build | 1 - > 4 files changed, 34 insertions(+), 62 deletions(-) > delete mode 100644 tests/gem_ctx_bad_exec.c > > diff --git a/tests/Makefile.sources b/tests/Makefile.sources > index c84933f1d971..269336ad3150 100644 > --- a/tests/Makefile.sources > +++ b/tests/Makefile.sources > @@ -51,7 +51,6 @@ TESTS_progs = \ > gem_cs_prefetch \ > gem_cs_tlb \ > gem_ctx_bad_destroy \ > - gem_ctx_bad_exec \ > gem_ctx_create \ > gem_ctx_exec \ > gem_ctx_isolation \ > diff --git a/tests/gem_ctx_bad_exec.c b/tests/gem_ctx_bad_exec.c > deleted file mode 100644 > index e3ccc5be46a0..000000000000 > --- a/tests/gem_ctx_bad_exec.c > +++ /dev/null > @@ -1,60 +0,0 @@ > -/* > - * Copyright © 2012 Intel Corporation > - * > - * Permission is hereby granted, free of charge, to any person obtaining a > - * copy of this software and associated documentation files (the "Software"), > - * to deal in the Software without restriction, including without limitation > - * the rights to use, copy, modify, merge, publish, distribute, sublicense, > - * and/or sell copies of the Software, and to permit persons to whom the > - * Software is furnished to do so, subject to the following conditions: > - * > - * The above copyright notice and this permission notice (including the next > - * paragraph) shall be included in all copies or substantial portions of the > - * Software. > - * > - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > - * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > - * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > - * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > - * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS > - * IN THE SOFTWARE. > - * > - */ > - > -#include "igt.h" > - > -IGT_TEST_DESCRIPTION("Test that context cannot be submitted to any ring"); > - > -static int exec(int fd, unsigned ring) > -{ > - struct drm_i915_gem_execbuffer2 execbuf; > - struct drm_i915_gem_exec_object2 obj; > - > - memset(&obj, 0, sizeof(obj)); > - memset(&execbuf, 0, sizeof(execbuf)); > - > - execbuf.buffers_ptr = to_user_pointer(&obj); > - execbuf.buffer_count = 1; > - i915_execbuffer2_set_context_id(execbuf, 1); > - > - return __gem_execbuf(fd, &execbuf); > -} > - > -igt_main > -{ > - const struct intel_execution_engine *e; > - int fd = -1; > - > - igt_skip_on_simulation(); > - > - igt_fixture > - fd = drm_open_driver_render(DRIVER_INTEL); > - > - for (e = intel_execution_engines; e->name; e++) { > - igt_subtest_f("%s", e->name) { > - gem_require_ring(fd, e->exec_id | e->flags); > - igt_assert_eq(exec(fd, e->exec_id | e->flags), -ENOENT); > - } > - } > -} > diff --git a/tests/gem_ctx_exec.c b/tests/gem_ctx_exec.c > index 1f8ed64d4bd3..4ed6febe12b8 100644 > --- a/tests/gem_ctx_exec.c > +++ b/tests/gem_ctx_exec.c > @@ -30,6 +30,7 @@ > */ > > #include "igt.h" > +#include <limits.h> > #include <unistd.h> > #include <stdlib.h> > #include <stdint.h> > @@ -142,6 +143,30 @@ static void big_exec(int fd, uint32_t handle, int ring) > gem_sync(fd, handle); > } > > +static void invalid_context(int fd, unsigned ring, uint32_t handle) > +{ > + struct drm_i915_gem_exec_object2 obj = { > + .handle = handle, > + }; > + struct drm_i915_gem_execbuffer2 execbuf = { > + .buffers_ptr = to_user_pointer(&obj), > + .buffer_count = 1, > + .flags = ring, > + }; > + const uint64_t invalid[] = { 1, INT_MAX, UINT_MAX, LONG_MAX, ULONG_MAX, > + LLONG_MAX, ULLONG_MAX }; The field is only a u32. Strange you didn't notice that amidst the absence of documentation ;) Trim this array to suite, and Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On 17/09/2018 16:52, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2018-09-17 16:46:18) >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> Move a really small test that invalid context is rejected under the >> gem_ctx_exec umbrella. >> >> v2: >> * And actually fix the test so it does what it claims. And add more >> variety in the invalid context id's it tests with. (Chris Wilson) >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> --- >> tests/Makefile.sources | 1 - >> tests/gem_ctx_bad_exec.c | 60 ---------------------------------------- >> tests/gem_ctx_exec.c | 34 +++++++++++++++++++++++ >> tests/meson.build | 1 - >> 4 files changed, 34 insertions(+), 62 deletions(-) >> delete mode 100644 tests/gem_ctx_bad_exec.c >> >> diff --git a/tests/Makefile.sources b/tests/Makefile.sources >> index c84933f1d971..269336ad3150 100644 >> --- a/tests/Makefile.sources >> +++ b/tests/Makefile.sources >> @@ -51,7 +51,6 @@ TESTS_progs = \ >> gem_cs_prefetch \ >> gem_cs_tlb \ >> gem_ctx_bad_destroy \ >> - gem_ctx_bad_exec \ >> gem_ctx_create \ >> gem_ctx_exec \ >> gem_ctx_isolation \ >> diff --git a/tests/gem_ctx_bad_exec.c b/tests/gem_ctx_bad_exec.c >> deleted file mode 100644 >> index e3ccc5be46a0..000000000000 >> --- a/tests/gem_ctx_bad_exec.c >> +++ /dev/null >> @@ -1,60 +0,0 @@ >> -/* >> - * Copyright © 2012 Intel Corporation >> - * >> - * Permission is hereby granted, free of charge, to any person obtaining a >> - * copy of this software and associated documentation files (the "Software"), >> - * to deal in the Software without restriction, including without limitation >> - * the rights to use, copy, modify, merge, publish, distribute, sublicense, >> - * and/or sell copies of the Software, and to permit persons to whom the >> - * Software is furnished to do so, subject to the following conditions: >> - * >> - * The above copyright notice and this permission notice (including the next >> - * paragraph) shall be included in all copies or substantial portions of the >> - * Software. >> - * >> - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR >> - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, >> - * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL >> - * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER >> - * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING >> - * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS >> - * IN THE SOFTWARE. >> - * >> - */ >> - >> -#include "igt.h" >> - >> -IGT_TEST_DESCRIPTION("Test that context cannot be submitted to any ring"); >> - >> -static int exec(int fd, unsigned ring) >> -{ >> - struct drm_i915_gem_execbuffer2 execbuf; >> - struct drm_i915_gem_exec_object2 obj; >> - >> - memset(&obj, 0, sizeof(obj)); >> - memset(&execbuf, 0, sizeof(execbuf)); >> - >> - execbuf.buffers_ptr = to_user_pointer(&obj); >> - execbuf.buffer_count = 1; >> - i915_execbuffer2_set_context_id(execbuf, 1); >> - >> - return __gem_execbuf(fd, &execbuf); >> -} >> - >> -igt_main >> -{ >> - const struct intel_execution_engine *e; >> - int fd = -1; >> - >> - igt_skip_on_simulation(); >> - >> - igt_fixture >> - fd = drm_open_driver_render(DRIVER_INTEL); >> - >> - for (e = intel_execution_engines; e->name; e++) { >> - igt_subtest_f("%s", e->name) { >> - gem_require_ring(fd, e->exec_id | e->flags); >> - igt_assert_eq(exec(fd, e->exec_id | e->flags), -ENOENT); >> - } >> - } >> -} >> diff --git a/tests/gem_ctx_exec.c b/tests/gem_ctx_exec.c >> index 1f8ed64d4bd3..4ed6febe12b8 100644 >> --- a/tests/gem_ctx_exec.c >> +++ b/tests/gem_ctx_exec.c >> @@ -30,6 +30,7 @@ >> */ >> >> #include "igt.h" >> +#include <limits.h> >> #include <unistd.h> >> #include <stdlib.h> >> #include <stdint.h> >> @@ -142,6 +143,30 @@ static void big_exec(int fd, uint32_t handle, int ring) >> gem_sync(fd, handle); >> } >> >> +static void invalid_context(int fd, unsigned ring, uint32_t handle) >> +{ >> + struct drm_i915_gem_exec_object2 obj = { >> + .handle = handle, >> + }; >> + struct drm_i915_gem_execbuffer2 execbuf = { >> + .buffers_ptr = to_user_pointer(&obj), >> + .buffer_count = 1, >> + .flags = ring, >> + }; >> + const uint64_t invalid[] = { 1, INT_MAX, UINT_MAX, LONG_MAX, ULONG_MAX, >> + LLONG_MAX, ULLONG_MAX }; > > The field is only a u32. Strange you didn't notice that amidst the > absence of documentation ;) > > Trim this array to suite, and > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Can I say it is testing the i915_execbuffer2_set_context_id as well by knowing underlying ABI field is 64-bit wide and keep the r-b? (No trash in unused part of rsvd1.) Regards, Tvrtko
Quoting Tvrtko Ursulin (2018-09-18 10:38:44) > > On 17/09/2018 16:52, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2018-09-17 16:46:18) > >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >> > >> Move a really small test that invalid context is rejected under the > >> gem_ctx_exec umbrella. > >> > >> v2: > >> * And actually fix the test so it does what it claims. And add more > >> variety in the invalid context id's it tests with. (Chris Wilson) > >> > >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >> --- > >> tests/Makefile.sources | 1 - > >> tests/gem_ctx_bad_exec.c | 60 ---------------------------------------- > >> tests/gem_ctx_exec.c | 34 +++++++++++++++++++++++ > >> tests/meson.build | 1 - > >> 4 files changed, 34 insertions(+), 62 deletions(-) > >> delete mode 100644 tests/gem_ctx_bad_exec.c > >> > >> diff --git a/tests/Makefile.sources b/tests/Makefile.sources > >> index c84933f1d971..269336ad3150 100644 > >> --- a/tests/Makefile.sources > >> +++ b/tests/Makefile.sources > >> @@ -51,7 +51,6 @@ TESTS_progs = \ > >> gem_cs_prefetch \ > >> gem_cs_tlb \ > >> gem_ctx_bad_destroy \ > >> - gem_ctx_bad_exec \ > >> gem_ctx_create \ > >> gem_ctx_exec \ > >> gem_ctx_isolation \ > >> diff --git a/tests/gem_ctx_bad_exec.c b/tests/gem_ctx_bad_exec.c > >> deleted file mode 100644 > >> index e3ccc5be46a0..000000000000 > >> --- a/tests/gem_ctx_bad_exec.c > >> +++ /dev/null > >> @@ -1,60 +0,0 @@ > >> -/* > >> - * Copyright © 2012 Intel Corporation > >> - * > >> - * Permission is hereby granted, free of charge, to any person obtaining a > >> - * copy of this software and associated documentation files (the "Software"), > >> - * to deal in the Software without restriction, including without limitation > >> - * the rights to use, copy, modify, merge, publish, distribute, sublicense, > >> - * and/or sell copies of the Software, and to permit persons to whom the > >> - * Software is furnished to do so, subject to the following conditions: > >> - * > >> - * The above copyright notice and this permission notice (including the next > >> - * paragraph) shall be included in all copies or substantial portions of the > >> - * Software. > >> - * > >> - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > >> - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > >> - * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > >> - * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > >> - * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > >> - * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS > >> - * IN THE SOFTWARE. > >> - * > >> - */ > >> - > >> -#include "igt.h" > >> - > >> -IGT_TEST_DESCRIPTION("Test that context cannot be submitted to any ring"); > >> - > >> -static int exec(int fd, unsigned ring) > >> -{ > >> - struct drm_i915_gem_execbuffer2 execbuf; > >> - struct drm_i915_gem_exec_object2 obj; > >> - > >> - memset(&obj, 0, sizeof(obj)); > >> - memset(&execbuf, 0, sizeof(execbuf)); > >> - > >> - execbuf.buffers_ptr = to_user_pointer(&obj); > >> - execbuf.buffer_count = 1; > >> - i915_execbuffer2_set_context_id(execbuf, 1); > >> - > >> - return __gem_execbuf(fd, &execbuf); > >> -} > >> - > >> -igt_main > >> -{ > >> - const struct intel_execution_engine *e; > >> - int fd = -1; > >> - > >> - igt_skip_on_simulation(); > >> - > >> - igt_fixture > >> - fd = drm_open_driver_render(DRIVER_INTEL); > >> - > >> - for (e = intel_execution_engines; e->name; e++) { > >> - igt_subtest_f("%s", e->name) { > >> - gem_require_ring(fd, e->exec_id | e->flags); > >> - igt_assert_eq(exec(fd, e->exec_id | e->flags), -ENOENT); > >> - } > >> - } > >> -} > >> diff --git a/tests/gem_ctx_exec.c b/tests/gem_ctx_exec.c > >> index 1f8ed64d4bd3..4ed6febe12b8 100644 > >> --- a/tests/gem_ctx_exec.c > >> +++ b/tests/gem_ctx_exec.c > >> @@ -30,6 +30,7 @@ > >> */ > >> > >> #include "igt.h" > >> +#include <limits.h> > >> #include <unistd.h> > >> #include <stdlib.h> > >> #include <stdint.h> > >> @@ -142,6 +143,30 @@ static void big_exec(int fd, uint32_t handle, int ring) > >> gem_sync(fd, handle); > >> } > >> > >> +static void invalid_context(int fd, unsigned ring, uint32_t handle) > >> +{ > >> + struct drm_i915_gem_exec_object2 obj = { > >> + .handle = handle, > >> + }; > >> + struct drm_i915_gem_execbuffer2 execbuf = { > >> + .buffers_ptr = to_user_pointer(&obj), > >> + .buffer_count = 1, > >> + .flags = ring, > >> + }; > >> + const uint64_t invalid[] = { 1, INT_MAX, UINT_MAX, LONG_MAX, ULONG_MAX, > >> + LLONG_MAX, ULLONG_MAX }; > > > > The field is only a u32. Strange you didn't notice that amidst the > > absence of documentation ;) > > > > Trim this array to suite, and > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > Can I say it is testing the i915_execbuffer2_set_context_id as well by > knowing underlying ABI field is 64-bit wide and keep the r-b? (No trash > in unused part of rsvd1.) The field isn't 64-bit wide, so the test would be misleading unfortunately. -Chris
On 18/09/2018 10:44, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2018-09-18 10:38:44) >> >> On 17/09/2018 16:52, Chris Wilson wrote: >>> Quoting Tvrtko Ursulin (2018-09-17 16:46:18) >>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>> >>>> Move a really small test that invalid context is rejected under the >>>> gem_ctx_exec umbrella. >>>> >>>> v2: >>>> * And actually fix the test so it does what it claims. And add more >>>> variety in the invalid context id's it tests with. (Chris Wilson) >>>> >>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>> --- >>>> tests/Makefile.sources | 1 - >>>> tests/gem_ctx_bad_exec.c | 60 ---------------------------------------- >>>> tests/gem_ctx_exec.c | 34 +++++++++++++++++++++++ >>>> tests/meson.build | 1 - >>>> 4 files changed, 34 insertions(+), 62 deletions(-) >>>> delete mode 100644 tests/gem_ctx_bad_exec.c >>>> >>>> diff --git a/tests/Makefile.sources b/tests/Makefile.sources >>>> index c84933f1d971..269336ad3150 100644 >>>> --- a/tests/Makefile.sources >>>> +++ b/tests/Makefile.sources >>>> @@ -51,7 +51,6 @@ TESTS_progs = \ >>>> gem_cs_prefetch \ >>>> gem_cs_tlb \ >>>> gem_ctx_bad_destroy \ >>>> - gem_ctx_bad_exec \ >>>> gem_ctx_create \ >>>> gem_ctx_exec \ >>>> gem_ctx_isolation \ >>>> diff --git a/tests/gem_ctx_bad_exec.c b/tests/gem_ctx_bad_exec.c >>>> deleted file mode 100644 >>>> index e3ccc5be46a0..000000000000 >>>> --- a/tests/gem_ctx_bad_exec.c >>>> +++ /dev/null >>>> @@ -1,60 +0,0 @@ >>>> -/* >>>> - * Copyright © 2012 Intel Corporation >>>> - * >>>> - * Permission is hereby granted, free of charge, to any person obtaining a >>>> - * copy of this software and associated documentation files (the "Software"), >>>> - * to deal in the Software without restriction, including without limitation >>>> - * the rights to use, copy, modify, merge, publish, distribute, sublicense, >>>> - * and/or sell copies of the Software, and to permit persons to whom the >>>> - * Software is furnished to do so, subject to the following conditions: >>>> - * >>>> - * The above copyright notice and this permission notice (including the next >>>> - * paragraph) shall be included in all copies or substantial portions of the >>>> - * Software. >>>> - * >>>> - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR >>>> - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, >>>> - * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL >>>> - * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER >>>> - * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING >>>> - * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS >>>> - * IN THE SOFTWARE. >>>> - * >>>> - */ >>>> - >>>> -#include "igt.h" >>>> - >>>> -IGT_TEST_DESCRIPTION("Test that context cannot be submitted to any ring"); >>>> - >>>> -static int exec(int fd, unsigned ring) >>>> -{ >>>> - struct drm_i915_gem_execbuffer2 execbuf; >>>> - struct drm_i915_gem_exec_object2 obj; >>>> - >>>> - memset(&obj, 0, sizeof(obj)); >>>> - memset(&execbuf, 0, sizeof(execbuf)); >>>> - >>>> - execbuf.buffers_ptr = to_user_pointer(&obj); >>>> - execbuf.buffer_count = 1; >>>> - i915_execbuffer2_set_context_id(execbuf, 1); >>>> - >>>> - return __gem_execbuf(fd, &execbuf); >>>> -} >>>> - >>>> -igt_main >>>> -{ >>>> - const struct intel_execution_engine *e; >>>> - int fd = -1; >>>> - >>>> - igt_skip_on_simulation(); >>>> - >>>> - igt_fixture >>>> - fd = drm_open_driver_render(DRIVER_INTEL); >>>> - >>>> - for (e = intel_execution_engines; e->name; e++) { >>>> - igt_subtest_f("%s", e->name) { >>>> - gem_require_ring(fd, e->exec_id | e->flags); >>>> - igt_assert_eq(exec(fd, e->exec_id | e->flags), -ENOENT); >>>> - } >>>> - } >>>> -} >>>> diff --git a/tests/gem_ctx_exec.c b/tests/gem_ctx_exec.c >>>> index 1f8ed64d4bd3..4ed6febe12b8 100644 >>>> --- a/tests/gem_ctx_exec.c >>>> +++ b/tests/gem_ctx_exec.c >>>> @@ -30,6 +30,7 @@ >>>> */ >>>> >>>> #include "igt.h" >>>> +#include <limits.h> >>>> #include <unistd.h> >>>> #include <stdlib.h> >>>> #include <stdint.h> >>>> @@ -142,6 +143,30 @@ static void big_exec(int fd, uint32_t handle, int ring) >>>> gem_sync(fd, handle); >>>> } >>>> >>>> +static void invalid_context(int fd, unsigned ring, uint32_t handle) >>>> +{ >>>> + struct drm_i915_gem_exec_object2 obj = { >>>> + .handle = handle, >>>> + }; >>>> + struct drm_i915_gem_execbuffer2 execbuf = { >>>> + .buffers_ptr = to_user_pointer(&obj), >>>> + .buffer_count = 1, >>>> + .flags = ring, >>>> + }; >>>> + const uint64_t invalid[] = { 1, INT_MAX, UINT_MAX, LONG_MAX, ULONG_MAX, >>>> + LLONG_MAX, ULLONG_MAX }; >>> >>> The field is only a u32. Strange you didn't notice that amidst the >>> absence of documentation ;) >>> >>> Trim this array to suite, and >>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> >> >> Can I say it is testing the i915_execbuffer2_set_context_id as well by >> knowing underlying ABI field is 64-bit wide and keep the r-b? (No trash >> in unused part of rsvd1.) > > The field isn't 64-bit wide, so the test would be misleading unfortunately. rsvd1? It is 64-bit in i915_drm.h I am looking at. And ABI is free to set it directly. But true, it seems we are not checking for trash in upper 32-bits in execbuf paths.. :( So this means regardless of what helper does the ABI is actually 64-bit. So test should actually bypass the helper I think and set rsvd1 directly I think. And keep the 64-bit invalid values. Regards, Tvrtko
Quoting Tvrtko Ursulin (2018-09-18 10:59:11) > > On 18/09/2018 10:44, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2018-09-18 10:38:44) > >> > >> On 17/09/2018 16:52, Chris Wilson wrote: > >>> Quoting Tvrtko Ursulin (2018-09-17 16:46:18) > >>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >>>> > >>>> Move a really small test that invalid context is rejected under the > >>>> gem_ctx_exec umbrella. > >>>> > >>>> v2: > >>>> * And actually fix the test so it does what it claims. And add more > >>>> variety in the invalid context id's it tests with. (Chris Wilson) > >>>> > >>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >>>> --- > >>>> tests/Makefile.sources | 1 - > >>>> tests/gem_ctx_bad_exec.c | 60 ---------------------------------------- > >>>> tests/gem_ctx_exec.c | 34 +++++++++++++++++++++++ > >>>> tests/meson.build | 1 - > >>>> 4 files changed, 34 insertions(+), 62 deletions(-) > >>>> delete mode 100644 tests/gem_ctx_bad_exec.c > >>>> > >>>> diff --git a/tests/Makefile.sources b/tests/Makefile.sources > >>>> index c84933f1d971..269336ad3150 100644 > >>>> --- a/tests/Makefile.sources > >>>> +++ b/tests/Makefile.sources > >>>> @@ -51,7 +51,6 @@ TESTS_progs = \ > >>>> gem_cs_prefetch \ > >>>> gem_cs_tlb \ > >>>> gem_ctx_bad_destroy \ > >>>> - gem_ctx_bad_exec \ > >>>> gem_ctx_create \ > >>>> gem_ctx_exec \ > >>>> gem_ctx_isolation \ > >>>> diff --git a/tests/gem_ctx_bad_exec.c b/tests/gem_ctx_bad_exec.c > >>>> deleted file mode 100644 > >>>> index e3ccc5be46a0..000000000000 > >>>> --- a/tests/gem_ctx_bad_exec.c > >>>> +++ /dev/null > >>>> @@ -1,60 +0,0 @@ > >>>> -/* > >>>> - * Copyright © 2012 Intel Corporation > >>>> - * > >>>> - * Permission is hereby granted, free of charge, to any person obtaining a > >>>> - * copy of this software and associated documentation files (the "Software"), > >>>> - * to deal in the Software without restriction, including without limitation > >>>> - * the rights to use, copy, modify, merge, publish, distribute, sublicense, > >>>> - * and/or sell copies of the Software, and to permit persons to whom the > >>>> - * Software is furnished to do so, subject to the following conditions: > >>>> - * > >>>> - * The above copyright notice and this permission notice (including the next > >>>> - * paragraph) shall be included in all copies or substantial portions of the > >>>> - * Software. > >>>> - * > >>>> - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > >>>> - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > >>>> - * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > >>>> - * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > >>>> - * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > >>>> - * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS > >>>> - * IN THE SOFTWARE. > >>>> - * > >>>> - */ > >>>> - > >>>> -#include "igt.h" > >>>> - > >>>> -IGT_TEST_DESCRIPTION("Test that context cannot be submitted to any ring"); > >>>> - > >>>> -static int exec(int fd, unsigned ring) > >>>> -{ > >>>> - struct drm_i915_gem_execbuffer2 execbuf; > >>>> - struct drm_i915_gem_exec_object2 obj; > >>>> - > >>>> - memset(&obj, 0, sizeof(obj)); > >>>> - memset(&execbuf, 0, sizeof(execbuf)); > >>>> - > >>>> - execbuf.buffers_ptr = to_user_pointer(&obj); > >>>> - execbuf.buffer_count = 1; > >>>> - i915_execbuffer2_set_context_id(execbuf, 1); > >>>> - > >>>> - return __gem_execbuf(fd, &execbuf); > >>>> -} > >>>> - > >>>> -igt_main > >>>> -{ > >>>> - const struct intel_execution_engine *e; > >>>> - int fd = -1; > >>>> - > >>>> - igt_skip_on_simulation(); > >>>> - > >>>> - igt_fixture > >>>> - fd = drm_open_driver_render(DRIVER_INTEL); > >>>> - > >>>> - for (e = intel_execution_engines; e->name; e++) { > >>>> - igt_subtest_f("%s", e->name) { > >>>> - gem_require_ring(fd, e->exec_id | e->flags); > >>>> - igt_assert_eq(exec(fd, e->exec_id | e->flags), -ENOENT); > >>>> - } > >>>> - } > >>>> -} > >>>> diff --git a/tests/gem_ctx_exec.c b/tests/gem_ctx_exec.c > >>>> index 1f8ed64d4bd3..4ed6febe12b8 100644 > >>>> --- a/tests/gem_ctx_exec.c > >>>> +++ b/tests/gem_ctx_exec.c > >>>> @@ -30,6 +30,7 @@ > >>>> */ > >>>> > >>>> #include "igt.h" > >>>> +#include <limits.h> > >>>> #include <unistd.h> > >>>> #include <stdlib.h> > >>>> #include <stdint.h> > >>>> @@ -142,6 +143,30 @@ static void big_exec(int fd, uint32_t handle, int ring) > >>>> gem_sync(fd, handle); > >>>> } > >>>> > >>>> +static void invalid_context(int fd, unsigned ring, uint32_t handle) > >>>> +{ > >>>> + struct drm_i915_gem_exec_object2 obj = { > >>>> + .handle = handle, > >>>> + }; > >>>> + struct drm_i915_gem_execbuffer2 execbuf = { > >>>> + .buffers_ptr = to_user_pointer(&obj), > >>>> + .buffer_count = 1, > >>>> + .flags = ring, > >>>> + }; > >>>> + const uint64_t invalid[] = { 1, INT_MAX, UINT_MAX, LONG_MAX, ULONG_MAX, > >>>> + LLONG_MAX, ULLONG_MAX }; > >>> > >>> The field is only a u32. Strange you didn't notice that amidst the > >>> absence of documentation ;) > >>> > >>> Trim this array to suite, and > >>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > >> > >> Can I say it is testing the i915_execbuffer2_set_context_id as well by > >> knowing underlying ABI field is 64-bit wide and keep the r-b? (No trash > >> in unused part of rsvd1.) > > > > The field isn't 64-bit wide, so the test would be misleading unfortunately. > > rsvd1? It is 64-bit in i915_drm.h I am looking at. And ABI is free to > set it directly. > > But true, it seems we are not checking for trash in upper 32-bits in > execbuf paths.. :( > > So this means regardless of what helper does the ABI is actually 64-bit. > So test should actually bypass the helper I think and set rsvd1 directly > I think. And keep the 64-bit invalid values. We are not checking 64b. Think of one value that invalidates the test. -Chris
Quoting Chris Wilson (2018-09-18 11:02:09) > Quoting Tvrtko Ursulin (2018-09-18 10:59:11) > > > > On 18/09/2018 10:44, Chris Wilson wrote: > > > Quoting Tvrtko Ursulin (2018-09-18 10:38:44) > > >> Can I say it is testing the i915_execbuffer2_set_context_id as well by > > >> knowing underlying ABI field is 64-bit wide and keep the r-b? (No trash > > >> in unused part of rsvd1.) > > > > > > The field isn't 64-bit wide, so the test would be misleading unfortunately. > > > > rsvd1? It is 64-bit in i915_drm.h I am looking at. And ABI is free to > > set it directly. > > > > But true, it seems we are not checking for trash in upper 32-bits in > > execbuf paths.. :( > > > > So this means regardless of what helper does the ABI is actually 64-bit. > > So test should actually bypass the helper I think and set rsvd1 directly > > I think. And keep the 64-bit invalid values. > > We are not checking 64b. Think of one value that invalidates the test. We can construct more values that would fail if we create a few contexts and destroy them testing that that are invalid after use. -Chris
On 18/09/2018 11:03, Chris Wilson wrote: > Quoting Chris Wilson (2018-09-18 11:02:09) >> Quoting Tvrtko Ursulin (2018-09-18 10:59:11) >>> >>> On 18/09/2018 10:44, Chris Wilson wrote: >>>> Quoting Tvrtko Ursulin (2018-09-18 10:38:44) >>>>> Can I say it is testing the i915_execbuffer2_set_context_id as well by >>>>> knowing underlying ABI field is 64-bit wide and keep the r-b? (No trash >>>>> in unused part of rsvd1.) >>>> >>>> The field isn't 64-bit wide, so the test would be misleading unfortunately. >>> >>> rsvd1? It is 64-bit in i915_drm.h I am looking at. And ABI is free to >>> set it directly. >>> >>> But true, it seems we are not checking for trash in upper 32-bits in >>> execbuf paths.. :( >>> >>> So this means regardless of what helper does the ABI is actually 64-bit. >>> So test should actually bypass the helper I think and set rsvd1 directly >>> I think. And keep the 64-bit invalid values. >> >> We are not checking 64b. Think of one value that invalidates the test. eb.rsdvd = ctx << 32; always gives you the default context? But is ABI the macro helper or the comment in i915_drm.h against rsvd1? Probably the main problem is that we forgot to check for trash in upper bits.. Or in other words, what do you want to see here in terms of invalid values and helper or not? Stick to 32-bits just because that's where we are now with the historical implementation? > We can construct more values that would fail if we create a few contexts > and destroy them testing that that are invalid after use. Hello scope creep, okay, I'll add it. Regards, Tvrtko
Quoting Tvrtko Ursulin (2018-09-18 11:33:14) > > On 18/09/2018 11:03, Chris Wilson wrote: > > Quoting Chris Wilson (2018-09-18 11:02:09) > >> Quoting Tvrtko Ursulin (2018-09-18 10:59:11) > >>> > >>> On 18/09/2018 10:44, Chris Wilson wrote: > >>>> Quoting Tvrtko Ursulin (2018-09-18 10:38:44) > >>>>> Can I say it is testing the i915_execbuffer2_set_context_id as well by > >>>>> knowing underlying ABI field is 64-bit wide and keep the r-b? (No trash > >>>>> in unused part of rsvd1.) > >>>> > >>>> The field isn't 64-bit wide, so the test would be misleading unfortunately. > >>> > >>> rsvd1? It is 64-bit in i915_drm.h I am looking at. And ABI is free to > >>> set it directly. > >>> > >>> But true, it seems we are not checking for trash in upper 32-bits in > >>> execbuf paths.. :( > >>> > >>> So this means regardless of what helper does the ABI is actually 64-bit. > >>> So test should actually bypass the helper I think and set rsvd1 directly > >>> I think. And keep the 64-bit invalid values. > >> > >> We are not checking 64b. Think of one value that invalidates the test. > > eb.rsdvd = ctx << 32; always gives you the default context? > > But is ABI the macro helper or the comment in i915_drm.h against rsvd1? > > Probably the main problem is that we forgot to check for trash in upper > bits.. > > Or in other words, what do you want to see here in terms of invalid > values and helper or not? Stick to 32-bits just because that's where we > are now with the historical implementation? The context id ABI has always been u32, and that's the field under test. > > We can construct more values that would fail if we create a few contexts > > and destroy them testing that that are invalid after use. > > Hello scope creep, okay, I'll add it. The test is "invalid context id" :-p Feature creep would be identifying that we have several tests that poke at IDRs through the ABI that could be refactored into a framework for light fuzzing. -Chris
diff --git a/tests/Makefile.sources b/tests/Makefile.sources index c84933f1d971..269336ad3150 100644 --- a/tests/Makefile.sources +++ b/tests/Makefile.sources @@ -51,7 +51,6 @@ TESTS_progs = \ gem_cs_prefetch \ gem_cs_tlb \ gem_ctx_bad_destroy \ - gem_ctx_bad_exec \ gem_ctx_create \ gem_ctx_exec \ gem_ctx_isolation \ diff --git a/tests/gem_ctx_bad_exec.c b/tests/gem_ctx_bad_exec.c deleted file mode 100644 index e3ccc5be46a0..000000000000 --- a/tests/gem_ctx_bad_exec.c +++ /dev/null @@ -1,60 +0,0 @@ -/* - * Copyright © 2012 Intel Corporation - * - * Permission is hereby granted, free of charge, to any person obtaining a - * copy of this software and associated documentation files (the "Software"), - * to deal in the Software without restriction, including without limitation - * the rights to use, copy, modify, merge, publish, distribute, sublicense, - * and/or sell copies of the Software, and to permit persons to whom the - * Software is furnished to do so, subject to the following conditions: - * - * The above copyright notice and this permission notice (including the next - * paragraph) shall be included in all copies or substantial portions of the - * Software. - * - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, - * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL - * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER - * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING - * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS - * IN THE SOFTWARE. - * - */ - -#include "igt.h" - -IGT_TEST_DESCRIPTION("Test that context cannot be submitted to any ring"); - -static int exec(int fd, unsigned ring) -{ - struct drm_i915_gem_execbuffer2 execbuf; - struct drm_i915_gem_exec_object2 obj; - - memset(&obj, 0, sizeof(obj)); - memset(&execbuf, 0, sizeof(execbuf)); - - execbuf.buffers_ptr = to_user_pointer(&obj); - execbuf.buffer_count = 1; - i915_execbuffer2_set_context_id(execbuf, 1); - - return __gem_execbuf(fd, &execbuf); -} - -igt_main -{ - const struct intel_execution_engine *e; - int fd = -1; - - igt_skip_on_simulation(); - - igt_fixture - fd = drm_open_driver_render(DRIVER_INTEL); - - for (e = intel_execution_engines; e->name; e++) { - igt_subtest_f("%s", e->name) { - gem_require_ring(fd, e->exec_id | e->flags); - igt_assert_eq(exec(fd, e->exec_id | e->flags), -ENOENT); - } - } -} diff --git a/tests/gem_ctx_exec.c b/tests/gem_ctx_exec.c index 1f8ed64d4bd3..4ed6febe12b8 100644 --- a/tests/gem_ctx_exec.c +++ b/tests/gem_ctx_exec.c @@ -30,6 +30,7 @@ */ #include "igt.h" +#include <limits.h> #include <unistd.h> #include <stdlib.h> #include <stdint.h> @@ -142,6 +143,30 @@ static void big_exec(int fd, uint32_t handle, int ring) gem_sync(fd, handle); } +static void invalid_context(int fd, unsigned ring, uint32_t handle) +{ + struct drm_i915_gem_exec_object2 obj = { + .handle = handle, + }; + struct drm_i915_gem_execbuffer2 execbuf = { + .buffers_ptr = to_user_pointer(&obj), + .buffer_count = 1, + .flags = ring, + }; + const uint64_t invalid[] = { 1, INT_MAX, UINT_MAX, LONG_MAX, ULONG_MAX, + LLONG_MAX, ULLONG_MAX }; + int i; + + /* Verify all works. */ + gem_execbuf(fd, &execbuf); + + /* Go through some non-existent context id's. */ + for (i = 0; i < ARRAY_SIZE(invalid); i++) { + i915_execbuffer2_set_context_id(execbuf, invalid[i]); + igt_assert_eq(__gem_execbuf(fd, &execbuf), -ENOENT); + } +} + uint32_t handle; uint32_t batch[2] = {0, MI_BATCH_BUFFER_END}; uint32_t ctx_id, ctx_id2; @@ -149,6 +174,8 @@ int fd; igt_main { + const struct intel_execution_engine *e; + igt_fixture { fd = drm_open_driver_render(DRIVER_INTEL); igt_require_gem(fd); @@ -174,6 +201,13 @@ igt_main gem_sync(fd, handle); } + for (e = intel_execution_engines; e->name; e++) { + igt_subtest_f("invalid-context-%s", e->name) { + gem_require_ring(fd, e->exec_id | e->flags); + invalid_context(fd, e->exec_id | e->flags, handle); + } + } + igt_subtest("eviction") big_exec(fd, handle, I915_EXEC_RENDER); diff --git a/tests/meson.build b/tests/meson.build index 17deb945ec95..d22d59e0837d 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -28,7 +28,6 @@ test_progs = [ 'gem_cs_prefetch', 'gem_cs_tlb', 'gem_ctx_bad_destroy', - 'gem_ctx_bad_exec', 'gem_ctx_create', 'gem_ctx_exec', 'gem_ctx_isolation',