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

[Tvrtko Ursulin]

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.)

Regards,

Tvrtko