Hi Chris,
On Fri, Mar 08, 2019 at 06:11:27PM +0000, Chris Wilson wrote:
> To exercise the new I915_CONTEXT_PARAM_ENGINES and interactions with
> gem_execbuf().
>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
> Cc: Andi Shyti <andi@xxxxxxxxxxx>
> ---
I received three times this patch, could you please add a
versioning or at least a note, so that I understand the evolution
instead of checking version by version.
[...]
> + engines->class_instance[0].engine_class = -1;
> + igt_assert_eq(__gem_context_set_param(i915, ¶m), -ENOENT);
> +
> + mprotect(engines, 4096, PROT_READ);
mprotect() can fail, do we want to check it?
[...]
> + /* Unadulterated I915_EXEC_DEFAULT should work */
> + execbuf.flags = 0;
> + igt_assert_eq(__gem_execbuf(i915, &execbuf), 0);
> + gem_sync(i915, obj.handle);
> +
> + for_each_engine_class_instance(i915, e) {
mmhhh... we have it! I thought I was the first one :)
[...]
> + for (int i = -1; i <= I915_EXEC_RING_MASK; i++) {
> + igt_spin_t *spin;
> +
> + memset(&engines, 0, sizeof(engines));
> + engine_class(&engines, 0) = e->class;
> + engine_instance(&engines, 0) = e->instance;
wouldn't it be easier to have an
__u16 *class = &engines.class_instance[0].engine_class
__u16 *instance = &e.class_instance[0].engine_instance
*class = e->class;
*instance = e->instance;
or something like
engine_class_set(&engines, 0, e->class)
engine_instance_set(&engines, 0, e->instance)
I find it more readable, because for me the '(' and ')' denote
functions.
[...]
> + 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);
> + }
Standing to the kernel coding style, you should remove the braces
here.
>From 'process/coding-style.rst'
"Do not unnecessarily use braces where
a single statement will do."
[...]
> +igt_main
> +{
> + int i915 = -1;
fd? in a first glance it's not obvious that i915 is an fd.
Besides, it's in tests/i915/ I would expect it's a i915 file
descriptor.
Andi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
