[igt-dev] [PATCH i-g-t v3 2/2] tests/gem_ctx_param: Update invalid param
Antonio Argenziano
antonio.argenziano at intel.com
Tue Jan 23 19:15:23 UTC 2018
On 19/01/18 01:31, Chris Wilson wrote:
> 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
Patch 1 (https://patchwork.freedesktop.org/patch/198689/) just adds a
function to swap int64_t.
>
>> 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
>
Sorry, I missed the suggestion from previous reviews. I am not sure if
it would be best to have one single patch (like
http://paste.debian.net/1006666/) or split out the introduction of libcap.
>> +
>> + 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 ;)
Will include overflow tests for invalid values also.
-Antonio
>
>> +
>> + 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