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

[Tvrtko Ursulin]

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.

Regards,

Tvrtko