[PATCH] gem_ctx_param: Update for context priorities

[PATCH] gem_ctx_param: Update for context priorities

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103107
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@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);
+		}
+	}
+
 	/* 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] gem_ctx_param: Update for context priorities

[Petri Latvala]

On Wed, Nov 08, 2017 at 09:47:20AM +0000, Tvrtko Ursulin wrote:

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

Can you resend with PATCH i-g-t so this gets to the correct patchwork?


-- 
Petri Latvala
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103107
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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@intel.com>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103107
>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] gem_ctx_param: Update for context priorities

[Daniel Vetter]

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@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@intel.com>
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103107
> > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Joonas Lahtinen <joonas.lahtinen@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx