[igt-dev] [Intel-gfx] [PATCH i-g-t 1/2] tests/gem_ctx_bad_exec: Consolidate to gem_ctx_exec

[Chris Wilson]

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 at 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 at 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 at 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