On Wed, Nov 08, 2017 at 09:47:20AM +0000, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx> > > Update the test to check for context priority get and set and at the same > time bump the invalid flag tests. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103107 > Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx> > --- > lib/i915/gem_context.c | 25 ++++++++++++++++++++++--- > lib/i915/gem_context.h | 2 ++ > tests/gem_ctx_param.c | 20 +++++++++++++++++++- > 3 files changed, 43 insertions(+), 4 deletions(-) > > diff --git a/lib/i915/gem_context.c b/lib/i915/gem_context.c > index 6d9edf5e3263..778dc6ca76e3 100644 > --- a/lib/i915/gem_context.c > +++ b/lib/i915/gem_context.c > @@ -198,8 +198,6 @@ void gem_context_require_bannable(int fd) > igt_require(has_ban_period || has_bannable); > } > > -#define LOCAL_I915_CONTEXT_PARAM_PRIORITY 0x6 > - > /** > * __gem_context_set_priority: > * @fd: open i915 drm file descriptor > @@ -219,7 +217,7 @@ int __gem_context_set_priority(int fd, uint32_t ctx_id, int prio) > memset(&p, 0, sizeof(p)); > p.context = ctx_id; > p.size = 0; > - p.param = LOCAL_I915_CONTEXT_PARAM_PRIORITY; > + p.param = LOCAL_CONTEXT_PARAM_PRIORITY; > p.value = prio; > > return __gem_context_set_param(fd, &p); > @@ -237,3 +235,24 @@ void gem_context_set_priority(int fd, uint32_t ctx_id, int prio) > { > igt_assert(__gem_context_set_priority(fd, ctx_id, prio) == 0); > } > + > +/** > + * gem_context_get_priority: > + * @fd: open i915 drm file descriptor > + * @ctx_id: i915 context id > + * > + * Returns the current context priority. > + */ > +int gem_context_get_priority(int fd, uint32_t ctx_id) > +{ > + struct local_i915_gem_context_param p; > + > + memset(&p, 0, sizeof(p)); > + p.context = ctx_id; > + p.size = 0; > + p.param = LOCAL_CONTEXT_PARAM_PRIORITY; > + > + igt_assert_eq(__gem_context_get_param(fd, &p), 0); > + > + return p.value; > +} > diff --git a/lib/i915/gem_context.h b/lib/i915/gem_context.h > index a2339c4b6da2..ac89512225e5 100644 > --- a/lib/i915/gem_context.h > +++ b/lib/i915/gem_context.h > @@ -36,6 +36,7 @@ struct local_i915_gem_context_param { > #define LOCAL_CONTEXT_PARAM_GTT_SIZE 0x3 > #define LOCAL_CONTEXT_PARAM_NO_ERROR_CAPTURE 0x4 > #define LOCAL_CONTEXT_PARAM_BANNABLE 0x5 > +#define LOCAL_CONTEXT_PARAM_PRIORITY 0x6 > uint64_t value; > }; > void gem_context_require_bannable(int fd); > @@ -50,5 +51,6 @@ int __gem_context_get_param(int fd, struct local_i915_gem_context_param *p); > #define LOCAL_I915_CONTEXT_MIN_USER_PRIORITY -1023 > int __gem_context_set_priority(int fd, uint32_t ctx, int prio); > void gem_context_set_priority(int fd, uint32_t ctx, int prio); > +int gem_context_get_priority(int fd, uint32_t ctx); > > #endif /* GEM_CONTEXT_H */ > diff --git a/tests/gem_ctx_param.c b/tests/gem_ctx_param.c > index efdaf191a1ed..43f6f96e0857 100644 > --- a/tests/gem_ctx_param.c > +++ b/tests/gem_ctx_param.c > @@ -136,11 +136,29 @@ igt_main > gem_context_set_param(fd, &arg); > } > > + igt_subtest_group { > + igt_fixture { > + igt_require(gem_scheduler_enabled(fd)); > + igt_require(gem_scheduler_has_ctx_priority(fd)); > + } > + > + igt_subtest("priority-get") { > + igt_assert_eq(gem_context_get_priority(fd, ctx), 0); > + } > + > + igt_subtest("priority-set") { > + int prio = LOCAL_I915_CONTEXT_DEFAULT_PRIORITY - 1; > + > + gem_context_set_priority(fd, ctx, prio); > + igt_assert_eq(gem_context_get_priority(fd, ctx), prio); > + } Not sure this is really a useful test. This I would like to see tested would be: - invalid priorities, i.e. above and below the max - invalid priorities for non CAP_SYS_NICE, especially checking that the fd-passing (which X does) works in a reasonable way: root opens fd, passes to non-root (just change uid), do we reject higher priorities? This is the kind of stuff that I think is worth checking when adding new uapi, to catch potential security issues where the kernel code doesn't properly validate&reject invalid input. If all this test ends up doing is busywork every time a new flag is added I'll concur with Chris and it's probably best to just throw it all out. But that's my own stance, I'm not really involved in gem deeply at all, so what exactly you end up going with is up to you and Chris and Jooans and everyone. Thanks, Daniel > + } > + > /* 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 = LOCAL_CONTEXT_PARAM_BANNABLE + 1; > + arg.param = LOCAL_CONTEXT_PARAM_PRIORITY + 1; > > igt_subtest("invalid-param-get") { > arg.context = ctx; > -- > 2.14.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
