Re: [igt-dev] [PATCH i-g-t 22/24] i915: Add gem_ctx_engines

[Andi Shyti <andi@etezian.org>]

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, &param) == 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, &param), -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, &param), 0);
> +
> +	param.value = to_user_pointer(engines - 1);
> +	igt_assert_eq(__gem_context_set_param(i915, &param), -EFAULT);
> +
> +	param.value = to_user_pointer(engines) - 1;
> +	igt_assert_eq(__gem_context_set_param(i915, &param), -EFAULT);
> +
> +	param.value = to_user_pointer(engines) - param.size +  1;
                                                              ^
just a blank more than necessary

> +	idx = 0;
> +	memset(&engines, 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, &execbuf), 0);

why aren't you using simply gem_execbuf()?

> +				execbuf.flags = j;
> +				err =__gem_execbuf(i915, &execbuf);
> +				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, &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 <andi@etezian.org>

Thanks,
Andi
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev