Re: [PATCH i-g-t 17/19] i915: Add gem_ctx_engines

[Andi Shyti <andi@etezian.org>]

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@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Andi Shyti <andi@etezian.org>
> ---

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, &param), -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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx