Re: [Intel-gfx] [PATCH i-g-t 22/24] i915: Add gem_ctx_engines
Hi Chris,
sorry for the late reply, I got 5 version of this same patch and
I couldn't figure out what was what :)
Could you please add some versioning or note if version is
the same?
Some nits and questions
> +static bool has_context_engines(int i915)
> +{
> + struct drm_i915_gem_context_param param = {
> + .ctx_id = 0,
> + .param = I915_CONTEXT_PARAM_ENGINES,
> + };
> + return __gem_context_set_param(i915, ) == 0;
> +}
I had it and removed it so many times in gem_engine_topology,
shall I put it back and we take it from there? (maybe in the
future).
[...]
> + igt_assert_eq(__gem_context_set_param(i915, ), -ENOENT);
> +
> + mprotect(engines, 4096, PROT_READ);
(from the last review) mprotect can fail, do we care?
[...]
> + engines->extensions = 0;
> + igt_assert_eq(__gem_context_set_param(i915, ), 0);
> +
> + param.value = to_user_pointer(engines - 1);
> + igt_assert_eq(__gem_context_set_param(i915, ), -EFAULT);
> +
> + param.value = to_user_pointer(engines) - 1;
> + igt_assert_eq(__gem_context_set_param(i915, ), -EFAULT);
> +
> + param.value = to_user_pointer(engines) - param.size + 1;
^
just a blank more than necessary
> + idx = 0;
> + memset(, 0, sizeof(engines));
> + for_each_engine_class_instance(i915, e) {
> + engines.class_instance[idx].engine_class = e->class;
> + engines.class_instance[idx].engine_instance = e->instance;
> + idx++;
> + }
> + idx *= sizeof(*engines.class_instance);
> + p.size = base + idx;
(I normally review from bottom to top) You used at least three
different ways to calculate param's size (some unclear to who
is new to igt some more clear).
Does it make sense to have a global define and we keep it
consistent?
p.size = SIZEOF_CTX_PARAM(idx);
it's a piece of code that I think it will be ussed a lot.
> + /* Unadulterated I915_EXEC_DEFAULT should work */
> + execbuf.flags = 0;
> + igt_assert_eq(__gem_execbuf(i915, ), 0);
why aren't you using simply gem_execbuf()?
> + execbuf.flags = j;
> + err =__gem_execbuf(i915, );
> + if (j == i) {
> + igt_assert_f(err == 0,
> + "Failed to report the
> valid engine for slot %d\n",
> + i);
> + } else {
> + igt_assert_f(err == -EINVAL,
> + "Failed to report an
> invalid engine for slot %d (valid at %d)\n",
> + j, i);
> + }
> + }
> +
> + do_ioctl(i915, DRM_IOCTL_I915_GEM_BUSY, );
> + if (i != -1) {
> + igt_assert_eq(busy.busy, 1 << (e->class + 16));
> + } else {
> + igt_assert_eq(busy.busy, 0);
> + }
> +
(from the last review) this is not kernel style, not that I care
much, but I thought you did.
You can add Reviewed-by: Andi Shyti
Thanks,
Andi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t 22/24] i915: Add gem_ctx_engines
Quoting Andi Shyti (2019-03-22 16:40:07)
> Hi Chris,
>
> sorry for the late reply, I got 5 version of this same patch and
> I couldn't figure out what was what :)
>
> Could you please add some versioning or note if version is
> the same?
>
> Some nits and questions
>
> > +static bool has_context_engines(int i915)
> > +{
> > + struct drm_i915_gem_context_param param = {
> > + .ctx_id = 0,
> > + .param = I915_CONTEXT_PARAM_ENGINES,
> > + };
> > + return __gem_context_set_param(i915, ) == 0;
> > +}
>
> I had it and removed it so many times in gem_engine_topology,
> shall I put it back and we take it from there? (maybe in the
> future).
>
> [...]
> > + igt_assert_eq(__gem_context_set_param(i915, ), -ENOENT);
> > +
> > + mprotect(engines, 4096, PROT_READ);
>
> (from the last review) mprotect can fail, do we care?
Debatable, yes we care as we won't get the expected faults in the next
tests, but do we want to call this the test failure? I want something
other than igt_require/igt_assert!
> > + idx = 0;
> > + memset(, 0, sizeof(engines));
> > + for_each_engine_class_instance(i915, e) {
> > + engines.class_instance[idx].engine_class = e->class;
> > + engines.class_instance[idx].engine_instance = e->instance;
> > + idx++;
> > + }
> > + idx *= sizeof(*engines.class_instance);
> > + p.size = base + idx;
>
> (I normally review from bottom to top) You used at least three
> different ways to calculate param's size (some unclear to who
> is new to igt some more clear).
>
> Does it make sense to have a global define and we keep it
> consistent?
>
> p.size = SIZEOF_CTX_PARAM(idx);
Definitely not shouting about it. I honestly believe that a plethora
of styles within tests is a good thing, and everything using the same
code pattern reduces the amount of test serendipity.
While this is a bit of trivial math and should not affect the outcome in
anyway, I quite like having bits and pieces fall naturally out of the
code because the code should also be an example of different ways it
might be used.
> it's a piece of code that I think it will be ussed a lot.
>
> > + /* Unadulterated I915_EXEC_DEFAULT should work */
> > + execbuf.flags = 0;
> > + igt_assert_eq(__gem_execbuf(i915, ), 0);
>
> why aren't you using simply gem_execbuf()?
So the style matched the open calls to __gem_execbuf() later.
> > + execbuf.flags = j;
> > + err =__gem_execbuf(i915, );
> > + if (j == i) {
> > + igt_assert_f(err == 0,
> > + "Failed to report the
> > valid engine for slot %d\n",
> > + i);
> > + } else {
> > + igt_assert_f(err == -EINVAL,
> > + "Failed to report an
> > invalid engine for slot %d (valid at %d)\n",
> > + j, i);
> > + }
> > + }
> > +
> > + do_ioctl(i915, DRM_IOCTL_I915_GEM_BUSY, );
> > + if (i != -1) {
> > + igt_assert_eq(busy.busy, 1 << (e->class +
> > 16));
> > + } else {
> > + igt_assert_eq(busy.busy, 0);
> > + }
> > +
>
> (from the last review) this is not kernel style, not that I care
> much, but I thought you did.
Indeed, _we_ do care ;)
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t 22/24] i915: Add gem_ctx_engines
To exercise the new I915_CONTEXT_PARAM_ENGINES and interactions with
gem_execbuf().
Signed-off-by: Chris Wilson
Cc: Tvrtko Ursulin
Cc: Andi Shyti
---
tests/Makefile.sources | 1 +
tests/i915/gem_ctx_engines.c | 441 +++
tests/meson.build| 1 +
3 files changed, 443 insertions(+)
create mode 100644 tests/i915/gem_ctx_engines.c
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 7e06c969f..2f5612b07 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -22,6 +22,7 @@ TESTS_progs = \
drm_mm \
drm_read \
i915/gem_ctx_clone \
+ i915/gem_ctx_engines \
i915/gem_ctx_shared \
i915/gem_vm_create \
kms_3d \
diff --git a/tests/i915/gem_ctx_engines.c b/tests/i915/gem_ctx_engines.c
new file mode 100644
index 0..588ecf196
--- /dev/null
+++ b/tests/i915/gem_ctx_engines.c
@@ -0,0 +1,441 @@
+/*
+ * Copyright © 2018 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"
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+
+#include "i915/gem_context.h"
+
+#define engine_class(e, n) ((e)->class_instance[(n)].engine_class)
+#define engine_instance(e, n) ((e)->class_instance[(n)].engine_instance)
+
+static bool has_context_engines(int i915)
+{
+ struct drm_i915_gem_context_param param = {
+ .ctx_id = 0,
+ .param = I915_CONTEXT_PARAM_ENGINES,
+ };
+ return __gem_context_set_param(i915, ) == 0;
+}
+
+static void invalid_engines(int i915)
+{
+ struct i915_context_param_engines stack = {}, *engines;
+ struct drm_i915_gem_context_param param = {
+ .ctx_id = gem_context_create(i915),
+ .param = I915_CONTEXT_PARAM_ENGINES,
+ .value = to_user_pointer(),
+ };
+ uint32_t handle;
+ void *ptr;
+
+ param.size = 0;
+ igt_assert_eq(__gem_context_set_param(i915, ), 0);
+
+ param.size = 1;
+ igt_assert_eq(__gem_context_set_param(i915, ), -EINVAL);
+
+ param.size = sizeof(stack) - 1;
+ igt_assert_eq(__gem_context_set_param(i915, ), -EINVAL);
+
+ param.size = sizeof(stack) + 1;
+ igt_assert_eq(__gem_context_set_param(i915, ), -EINVAL);
+
+ param.size = 0;
+ igt_assert_eq(__gem_context_set_param(i915, ), 0);
+
+ /* Create a single page surrounded by inaccessible nothingness */
+ ptr = mmap(NULL, 3 * 4096, PROT_WRITE, MAP_ANON | MAP_PRIVATE, -1, 0);
+ igt_assert(ptr != MAP_FAILED);
+
+ munmap(ptr, 4096);
+ engines = ptr + 4096;
+ munmap(ptr + 2 *4096, 4096);
+
+ param.size = sizeof(*engines) + sizeof(*engines->class_instance);
+ param.value = to_user_pointer(engines);
+
+ engines->class_instance[0].engine_class = -1;
+ igt_assert_eq(__gem_context_set_param(i915, ), -ENOENT);
+
+ mprotect(engines, 4096, PROT_READ);
+ igt_assert_eq(__gem_context_set_param(i915, ), -ENOENT);
+
+ mprotect(engines, 4096, PROT_WRITE);
+ engines->class_instance[0].engine_class = 0;
+ if (__gem_context_set_param(i915, )) /* XXX needs RCS */
+ goto out;
+
+ engines->extensions = to_user_pointer(ptr);
+ igt_assert_eq(__gem_context_set_param(i915, ), -EFAULT);
+
+ engines->extensions = 0;
+ igt_assert_eq(__gem_context_set_param(i915, ), 0);
+
+ param.value = to_user_pointer(engines - 1);
+ igt_assert_eq(__gem_context_set_param(i915, ), -EFAULT);
+
+ param.value = to_user_pointer(engines) - 1;
+ igt_assert_eq(__gem_context_set_param(i915, ), -EFAULT);
+
+ param.value = to_user_pointer(engines) - param.size + 1;
+ igt_assert_eq(__gem_context_set_param(i915, ), -EFAULT);
+
+ param.value =
