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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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.

Regards,

Tvrtko



_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux