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@xxxxxxxxx>
> >>>>
> >>>> 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@xxxxxxxxx>
> >>>> ---
> >>>> 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@xxxxxxxxxxxxxxxxxx>
> >>
> >> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
