[Intel-gfx] [PATCH] gem_ctx_param: Update for context priorities

[Daniel Vetter]

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
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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch