[Intel-gfx] [PATCH] gem_ctx_param: Update for context priorities
Daniel Vetter
daniel at ffwll.ch
Wed Nov 8 12:09:56 UTC 2017
On Wed, Nov 08, 2017 at 10:24:50AM +0000, Tvrtko Ursulin wrote:
>
> On 08/11/2017 10:11, Daniel Vetter wrote:
> > On Wed, Nov 08, 2017 at 09:47:20AM +0000, Tvrtko Ursulin wrote:
> > > From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > >
> > > 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 at intel.com>
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103107
> > > Cc: Daniel Vetter <daniel.vetter at intel.com>
> > > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > > Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> > > ---
> > > 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
>
> "Better than nothing" and better than a broken one, just that. My re-action
> to this morning's bickering.
>
> > 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.
>
> Agreed, the more context params the less sense it makes to have it in
> gem_ctx_param and more sense to handle those details in dedicated tests for
> each feature.
>
> > 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.
>
> No complaints from me to remove it, but it needs to go on the big GEM IGT
> TODO list to look at what's missing elsewhere, fill it in, and then remove.
> In the meantime, the sole contribution of this patch was that it stops an
> always failing test in a less aggressive manner than removing it completely
> before first doing the above.
If all you want is not forget about the possible gaps, then remove the
subtest Chris Wilson doesn't like and add it to the gem igt todo list.
I'm simply fed up bickering with Chris every time this comes up, it's not
worth it, hence why I prefer to just nuke it and it's gone. We can also
let it keep failing, CI has it greylisted ever since the regression was
merged anyway.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Intel-gfx
mailing list