[Intel-gfx] [PATCH i-g-t] tests/gem_ctx_param: Update invalid param

[Antonio Argenziano]

On 15/12/17 16:14, Chris Wilson wrote:
> Quoting Antonio Argenziano (2017-12-15 19:01:11)
>> 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.
>>
>> 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 | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 76 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/gem_ctx_param.c b/tests/gem_ctx_param.c
>> index c20ae1ee..9a222e60 100644
>> --- a/tests/gem_ctx_param.c
>> +++ b/tests/gem_ctx_param.c
>> @@ -25,6 +25,7 @@
>>    */
>>   
>>   #include "igt.h"
>> +#include <limits.h>
>>   
>>   IGT_TEST_DESCRIPTION("Basic test for context set/get param input validation.");
>>   
>> @@ -136,11 +137,85 @@ igt_main
>>                  gem_context_set_param(fd, &arg);
>>          }
>>   
>> +#define MAX_PRIO 1023
>> +#define MIN_PRIO -MAX_PRIO
>> +#define DEF_PRIO 0
> 
> Take these from the uapi header.
> 
>> +
>> +       arg.param = I915_CONTEXT_PARAM_PRIORITY;
>> +
>> +       igt_subtest("root-set-priority") {
>> +               arg.ctx_id = ctx;
>> +               arg.size = 0;
>> +
> 
> Bonus points for CAP_SYS_NICE checking.
> 
> arg.size validation checking. > arg.value > u32 (checking for overflows)

What is the expectation for overflows? It looks like we only cast value 
to int.

> 
> gem_context_get_param() of a new context should return DEF_PRIO
> 
> set_param on older machines returns -ENODEV
> 
> And that's not even trying to fuzz bad values beyond the sanity checks
> of I915_CONTEXT_PARAM_PRIORITY.
> 
>> +               for (int i = MIN_PRIO; i <= MAX_PRIO; i += 1023) {
> 
> It's a couple of ioctls, do all 1024. Do them in a random order, trust
> no one.

Do we have a lib for doing that (generate a random permutation) already?

> 
>> +                       arg.value = i;
>> +                       gem_context_set_param(fd, &arg);
>> +
>> +                       gem_context_get_param(fd, &arg);
>> +                       igt_assert(arg.value == i); /* Verify prio was set */
> 
> igt_assert_eq(arg.value, i);
> 
> But doesn't verify the priority does anything. Just the RTT in the API,
> which is a nice verification nevertheless.
> 
>> +               }
>> +       }
>> +
>> +       igt_subtest("root-set-priority-invalid-value") {
>> +               int prio_levels[] = {INT_MIN, MIN_PRIO - 1, MAX_PRIO + 1, INT_MAX};
>> +               int old_value;
>> +               arg.ctx_id = ctx;
>> +
>> +               gem_context_get_param(fd, &arg);
>> +               old_value = arg.value;
>> +
>> +               for (int i = 0; i < (sizeof(prio_levels) / sizeof(int)); i++) {
> 
> ARRAY_SIZE(prio_levels);
> 
>> +                       arg.value = prio_levels[i];
>> +                       igt_assert_eq(__gem_context_set_param(fd, &arg), -EINVAL);
>> +
>> +                       gem_context_get_param(fd, &arg);
>> +                       igt_assert(arg.value == old_value); /* Verify prio was not set */
>> +               }
>> +       }
>> +
>> +       igt_subtest("user-set-priority") {
>> +               arg.size = 0;
>> +
>> +               igt_fork(child, 1) {
>> +                       igt_drop_root();
>> +                       for (int i = MIN_PRIO; i <= DEF_PRIO; i += 1023) {
>> +                               arg.value = i;
>> +                               gem_context_set_param(fd, &arg);
>> +
>> +                               gem_context_get_param(fd, &arg);
>> +                               igt_assert(arg.value == i); /* Verify prio was set */
> 
> I wonder if the CAP_SYS_NICE limit might be adjusted in the future.
> Certainly preemption rules are flexible, as we haven't told anyone about
> them yet.

I guess when the change comes we will have a new constant we could swap 
DEF_PRIO with.

-Antonio

> -Chris
>