[igt-dev] [PATCH i-g-t v3 2/2] tests/gem_ctx_param: Update invalid param
Chris Wilson
chris at chris-wilson.co.uk
Fri Jan 19 09:31:41 UTC 2018
Quoting Antonio Argenziano (2018-01-18 22:15:43)
> Since commit: drm/i915/scheduler: Support user-defined priorities, the
> driver support an extra context param to set context's priority. Add
> tests for that interface and update invalid tests.
I haven't seen patch 1/2, but this patch needs a touch of
Documentation/process/coding-style.rst
> v2:
> - Add arg size validation test. (Chris)
> - Add arg value overflow test. (Chris)
> - Add test for unsupported platforms. (Chris)
> - Feed interface with all priority values and in random order. (Chris)
>
> v3:
> - Parametrize tests. (Chris)
>
> Signed-off-by: Antonio Argenziano <antonio.argenziano at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Michal Winiarski <michal.winiarski at intel.com>
> ---
> tests/gem_ctx_param.c | 166 +++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 165 insertions(+), 1 deletion(-)
>
> diff --git a/tests/gem_ctx_param.c b/tests/gem_ctx_param.c
> index c20ae1ee..3571c004 100644
> --- a/tests/gem_ctx_param.c
> +++ b/tests/gem_ctx_param.c
> @@ -25,9 +25,122 @@
> */
>
> #include "igt.h"
> +#include <limits.h>
>
> IGT_TEST_DESCRIPTION("Basic test for context set/get param input validation.");
>
> +#define MAX_USER_SET_PRIO I915_CONTEXT_DEFAULT_PRIORITY /* Current max prio for non-root users */
> +#define PRIO_RANGE (I915_CONTEXT_MAX_USER_PRIORITY - I915_CONTEXT_MIN_USER_PRIORITY)
> +#define USER_PRIO_RANGE (MAX_USER_SET_PRIO - I915_CONTEXT_MIN_USER_PRIORITY)
> +
> +#define ROOT (0x1 << 3)
> +#define NEW_CTX (0x1 << 2)
> +#define VALID_PRIO (0x1 << 1)
> +#define OVERFLOW_PRIO (0x1 << 0)
> +
> +static int is_priority_valid(int64_t value, unsigned flags) {
> + if (flags & ROOT) {
> + if ((value - I915_CONTEXT_MIN_USER_PRIORITY) <= PRIO_RANGE &&
> + (value - I915_CONTEXT_MIN_USER_PRIORITY) >= 0)
> + return 0;
> +
> + return -EINVAL;
> + } else {
> + if ((value - I915_CONTEXT_MIN_USER_PRIORITY) <= USER_PRIO_RANGE &&
> + (value - I915_CONTEXT_MIN_USER_PRIORITY) >= 0)
> + return 0;
> +
> + if ((value - I915_CONTEXT_MIN_USER_PRIORITY) <= PRIO_RANGE &&
> + (value - I915_CONTEXT_MIN_USER_PRIORITY) >= 0)
> + return -EPERM;
> +
> + return -EINVAL;
> + }
> +}
> +
> +static void
> +get_prio_values_valid(int64_t **prio_values, unsigned *size, unsigned flags) {
> + *size = ((flags & ROOT) ? PRIO_RANGE : USER_PRIO_RANGE) + 1;
> + *prio_values = (int64_t*) calloc(*size, sizeof(int64_t));
const int64_t overflow = flags & OVERFLOW ? (int64_t)1 << 32 : 0;
int64_t values = calloc(*size, sizeof(int64_t);
igt_assert(values);
for (unsigned i = 0; i < size; i++)
values[i] = overflow | (i + I915_CONTEXT_MIN_USER_PRIORITY);
*prio_values = values;
> +
> + for (int i = 0; i < *size; i++)
> + (*prio_values)[i] = ((int64_t)(0x1 & flags) << 32) + (i + I915_CONTEXT_MIN_USER_PRIORITY);
> +}
> +
> +static void
> +get_prio_values_invalid(int64_t **prio_values, unsigned *size, unsigned flags) {
> + if (flags & ROOT) {
> + int64_t test_values[]
> + = {INT_MIN,
> + I915_CONTEXT_MIN_USER_PRIORITY - 1,
> + I915_CONTEXT_MAX_USER_PRIORITY + 1,
> + INT_MAX}; /* Test space too big pick significant values */
Reasonable choices.
int64_t test_values[] = { /* Test space too big pick significant values */
INT_MIN,
I915_CONTEXT_MIN_USER_PRIORITY - 1,
I915_CONTEXT_MAX_USER_PRIORITY + 1,
INT_MAX,
};
> +
> + *size = ARRAY_SIZE(test_values);
> + *prio_values = (int64_t*) calloc(*size, sizeof(int64_t));
> + for (int i = 0; i < *size; i++)
> + (*prio_values)[i] = test_values[i];
But I was expecting to see some overflow values in here as well.
> + } else {
> + *size = PRIO_RANGE - USER_PRIO_RANGE;
> + *prio_values = (int64_t*) calloc(*size, sizeof(int64_t));
> +
> + for (int i = 0; i < *size; i++)
> + (*prio_values)[i] = i + (MAX_USER_SET_PRIO + 1);
> + }
> +}
> +
> +static void
> +get_prio_values(int64_t **prio_values, unsigned *size, unsigned flags) {
> + if (flags & VALID_PRIO)
> + get_prio_values_valid(prio_values, size, flags);
> + else
> + get_prio_values_invalid(prio_values, size, flags);
> +
> + igt_permute_array(*prio_values, *size, igt_exchange_int64);
> +}
> +
> +static void
> +set_priority(int fd, struct drm_i915_gem_context_param arg, unsigned flags) {
> + uint32_t ctx = 0;
> + int expected_ret;
> + int64_t old_value;
> + int64_t *prio_values;
> + unsigned size;
> +
> + igt_fork(child, 1) {
> + get_prio_values(&prio_values, &size, flags);
> +
> + if (flags & NEW_CTX)
> + ctx = gem_context_create(fd);
> + arg.ctx_id = ctx;
> +
> + if (!(flags & ROOT)) {
> + igt_drop_root();
> + }
Still expecting a
if (!(flags & NICE))
capset(~CAP_SYS_NICE);
pass
> +
> + gem_context_get_param(fd, &arg);
> + old_value = arg.value;
> +
> + for (int i = 0; i < size; i++) {
int expected_result;
> + arg.value = prio_values[i];
> + expected_ret = is_priority_valid(arg.value, flags);
> + igt_assert_eq(__gem_context_set_param(fd, &arg), expected_ret);
> +
> + gem_context_get_param(fd, &arg);
> + igt_assert_eq(arg.value, (expected_ret) ? old_value : prio_values[i]); /* Verify prio was set */
Please consider how the assert looks when stringified after a failure.
if (expected_result) {
igt_assert_eq(arg.value, old_value); /* unchanged */
} else {
igt_assert_eq(arg.value, prio_values[i]); /* updated */
}
> + }
> +
> + arg.value = 0;
> + gem_context_set_param(fd, &arg);
> +
> + if (flags & NEW_CTX)
> + gem_context_destroy(fd, ctx);
> + }
> +
> + igt_waitchildren();
> +}
> +
> igt_main
> {
> struct drm_i915_gem_context_param arg;
> @@ -136,11 +249,62 @@ igt_main
> gem_context_set_param(fd, &arg);
> }
>
> + arg.param = I915_CONTEXT_PARAM_PRIORITY;
> +
> + igt_subtest("set-priority-not-supported") {
> + igt_require(!gem_scheduler_has_ctx_priority(fd));
> +
> + arg.ctx_id = ctx;
> + arg.size = 0;
> +
> + igt_assert_eq(__gem_context_set_param(fd, &arg), -ENODEV);
> + }
> +
> + igt_subtest_group {
> + igt_fixture {
> + igt_require(gem_scheduler_has_ctx_priority(fd));
> + }
> +
> + igt_subtest("get-priority-new-ctx") {
> + struct drm_i915_gem_context_param local_arg = arg;
> + uint32_t local_ctx = gem_context_create(fd);
> +
> + local_arg.ctx_id = local_ctx;
> +
> + gem_context_get_param(fd, &local_arg);
> + igt_assert_eq(local_arg.value, 0);
> +
> + gem_context_destroy(fd, local_ctx);
> + }
> +
> + igt_subtest("set-priority-invalid-size") {
> + struct drm_i915_gem_context_param local_arg = arg;
> + local_arg.ctx_id = ctx;
> + local_arg.value = 0;
> + local_arg.size = ~0;
> +
> + igt_assert_eq(__gem_context_set_param(fd, &local_arg), -EINVAL);
> + }
> +
> + for (unsigned flags = 0; flags <= (ROOT | NEW_CTX | VALID_PRIO | OVERFLOW_PRIO); flags++) {
> + if ((flags & OVERFLOW_PRIO) && !(flags & VALID_PRIO))
> + continue; /* overflow values are already invalid */
Oh, now that's a silly assumption! We're trying to make sure we aren't
guilty of those silly assumptions in the kernel across the uABI ;)
> +
> + igt_subtest_f("set-priority%s%s%s%s",
> + (flags & ROOT) ? "-root" : "-user",
> + (flags & NEW_CTX) ? "-new-ctx" : "-default-ctx",
> + (flags & VALID_PRIO) ? "" : "-invalid",
> + (flags & OVERFLOW_PRIO) ? "-overflow" : "")
> + set_priority(fd, arg, flags);
> + }
> +
> + }
> +
> /* NOTE: This testcase intentionally tests for the next free parameter
> * to catch ABI extensions. Don't "fix" this testcase without adding all
> * the tests for the new param first.
> */
> - arg.param = I915_CONTEXT_PARAM_BANNABLE + 1;
> + arg.param = I915_CONTEXT_PARAM_PRIORITY + 1;
>
> igt_subtest("invalid-param-get") {
> arg.ctx_id = ctx;
> --
> 2.14.2
>
More information about the igt-dev
mailing list