diff mbox

[PATCHv3,i-g-t] lib: substitute cxt BAN_PERIOD with BANNABLE

Message ID 1478872503-24747-1-git-send-email-mika.kuoppala@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Mika Kuoppala Nov. 11, 2016, 1:55 p.m. UTC 
Context BAN_PERIOD will get depracated so subsitute it with BANNABLE
property. Make ctx param test to accept both variants for now
until kernel changes have landed, to not break BAT.

v2: check against - EINVAL on get/set ban as it can return -EPERM
v3: better naming for get/set (Chris)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 lib/igt_gt.c          | 77 ++++++++++++++++++++++++++++++---------------------
 lib/ioctl_wrappers.c  | 16 +++++++++--
 lib/ioctl_wrappers.h  |  3 +-
 tests/gem_ctx_param.c |  9 ++++--
 4 files changed, 68 insertions(+), 37 deletions(-)

Comments

Chris Wilson Nov. 11, 2016, 2:05 p.m. UTC | #1 
On Fri, Nov 11, 2016 at 03:55:03PM +0200, Mika Kuoppala wrote:
> Context BAN_PERIOD will get depracated so subsitute it with BANNABLE
> property. Make ctx param test to accept both variants for now
> until kernel changes have landed, to not break BAT.
> 
> v2: check against - EINVAL on get/set ban as it can return -EPERM
> v3: better naming for get/set (Chris)
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>

There's a small assumption of symmetry between the get/set paths, but it
looks ok as a transitional patch.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
Mika Kuoppala Nov. 11, 2016, 2:07 p.m. UTC | #2 
Mika Kuoppala <mika.kuoppala@linux.intel.com> writes:

> Context BAN_PERIOD will get depracated so subsitute it with BANNABLE
> property. Make ctx param test to accept both variants for now
> until kernel changes have landed, to not break BAT.
>

The kernel parts are in:
https://cgit.freedesktop.org/~miku/drm-intel/log/?h=hangcheck
https://cgit.freedesktop.org/~miku/drm-intel/commit/?h=hangcheck&id=9f3c978720a82f3043060b5e43cfd7d4fa79f2bd

-Mika

> v2: check against - EINVAL on get/set ban as it can return -EPERM
> v3: better naming for get/set (Chris)
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  lib/igt_gt.c          | 77 ++++++++++++++++++++++++++++++---------------------
>  lib/ioctl_wrappers.c  | 16 +++++++++--
>  lib/ioctl_wrappers.h  |  3 +-
>  tests/gem_ctx_param.c |  9 ++++--
>  4 files changed, 68 insertions(+), 37 deletions(-)
>
> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> index a165932..468c771 100644
> --- a/lib/igt_gt.c
> +++ b/lib/igt_gt.c
> @@ -119,11 +119,43 @@ void igt_require_hang_ring(int fd, int ring)
>  		igt_skip("hang injection disabled by user");
>  
>  	gem_require_ring(fd, ring);
> -	gem_context_require_ban_period(fd);
> +	gem_context_require_bannable(fd);
>  	if (!igt_check_boolean_env_var("IGT_HANG_WITHOUT_RESET", false))
>  		igt_require(has_gpu_reset(fd));
>  }
>  
> +static unsigned context_get_ban(int fd, unsigned ctx)
> +{
> +	struct local_i915_gem_context_param param;
> +
> +	param.param = LOCAL_CONTEXT_PARAM_BANNABLE;
> +	param.value = 0;
> +	param.size = 0;
> +
> +	if (__gem_context_get_param(fd, &param) == -EINVAL) {
> +		igt_assert(param.value == 0);
> +		param.param = LOCAL_CONTEXT_PARAM_BAN_PERIOD;
> +		gem_context_get_param(fd, &param);
> +	}
> +
> +	return param.value;
> +}
> +
> +static void context_set_ban(int fd, unsigned ctx, unsigned ban)
> +{
> +	struct local_i915_gem_context_param param;
> +
> +	param.value = ban;
> +	param.size = 0;
> +	param.param = LOCAL_CONTEXT_PARAM_BANNABLE;
> +
> +	if(__gem_context_set_param(fd, &param) == -EINVAL) {
> +		igt_assert(param.value == ban);
> +		param.param = LOCAL_CONTEXT_PARAM_BAN_PERIOD;
> +		gem_context_set_param(fd, &param);
> +	}
> +}
> +
>  igt_hang_t igt_allow_hang(int fd, unsigned ctx, unsigned flags)
>  {
>  	struct local_i915_gem_context_param param;
> @@ -131,7 +163,7 @@ igt_hang_t igt_allow_hang(int fd, unsigned ctx, unsigned flags)
>  
>  	if (!igt_check_boolean_env_var("IGT_HANG", true))
>  		igt_skip("hang injection disabled by user");
> -	gem_context_require_ban_period(fd);
> +	gem_context_require_bannable(fd);
>  	if (!igt_check_boolean_env_var("IGT_HANG_WITHOUT_RESET", false))
>  		igt_require(has_gpu_reset(fd));
>  
> @@ -148,16 +180,9 @@ igt_hang_t igt_allow_hang(int fd, unsigned ctx, unsigned flags)
>  		__gem_context_set_param(fd, &param);
>  	}
>  
> -	param.param = LOCAL_CONTEXT_PARAM_BAN_PERIOD;
> -	param.value = 0;
> -	gem_context_get_param(fd, &param);
> -	ban = param.value;
> -
> -	if ((flags & HANG_ALLOW_BAN) == 0) {
> -		param.param = LOCAL_CONTEXT_PARAM_BAN_PERIOD;
> -		param.value = 0;
> -		gem_context_set_param(fd, &param);
> -	}
> +	ban = context_get_ban(fd, ctx);
> +	if ((flags & HANG_ALLOW_BAN) == 0)
> +		context_set_ban(fd, ctx, 0);
>  
>  	return (struct igt_hang){ 0, ctx, ban, flags };
>  }
> @@ -166,13 +191,11 @@ void igt_disallow_hang(int fd, igt_hang_t arg)
>  {
>  	struct local_i915_gem_context_param param;
>  
> -	param.context = arg.ctx;
> -	param.size = 0;
> -	param.param = LOCAL_CONTEXT_PARAM_BAN_PERIOD;
> -	param.value = arg.ban;
> -	gem_context_set_param(fd, &param);
> +	context_set_ban(fd, arg.ctx, arg.ban);
>  
>  	if ((arg.flags & HANG_ALLOW_CAPTURE) == 0) {
> +		param.context = arg.ctx;
> +		param.size = 0;
>  		param.param = LOCAL_CONTEXT_PARAM_NO_ERROR_CAPTURE;
>  		param.value = 0;
>  		if (__gem_context_set_param(fd, &param))
> @@ -227,16 +250,10 @@ igt_hang_t igt_hang_ctx(int fd,
>  		__gem_context_set_param(fd, &param);
>  	}
>  
> -	param.param = LOCAL_CONTEXT_PARAM_BAN_PERIOD;
> -	param.value = 0;
> -	gem_context_get_param(fd, &param);
> -	ban = param.value;
> +	ban = context_get_ban(fd, ctx);
>  
> -	if ((flags & HANG_ALLOW_BAN) == 0) {
> -		param.param = LOCAL_CONTEXT_PARAM_BAN_PERIOD;
> -		param.value = 0;
> -		gem_context_set_param(fd, &param);
> -	}
> +	if ((flags & HANG_ALLOW_BAN) == 0)
> +		context_set_ban(fd, ctx, 0);
>  
>  	memset(&reloc, 0, sizeof(reloc));
>  	memset(&exec, 0, sizeof(exec));
> @@ -323,13 +340,11 @@ void igt_post_hang_ring(int fd, igt_hang_t arg)
>  		       I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
>  	gem_close(fd, arg.handle);
>  
> -	param.context = arg.ctx;
> -	param.size = 0;
> -	param.param = LOCAL_CONTEXT_PARAM_BAN_PERIOD;
> -	param.value = arg.ban;
> -	gem_context_set_param(fd, &param);
> +	context_set_ban(fd, arg.ctx, arg.ban);
>  
>  	if ((arg.flags & HANG_ALLOW_CAPTURE) == 0) {
> +		param.context = arg.ctx;
> +		param.size = 0;
>  		param.param = LOCAL_CONTEXT_PARAM_NO_ERROR_CAPTURE;
>  		param.value = 0;
>  		if (__gem_context_set_param(fd, &param))
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index eabf3ee..06638d4 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -961,9 +961,21 @@ void gem_context_require_param(int fd, uint64_t param)
>  	igt_require(igt_ioctl(fd, LOCAL_IOCTL_I915_GEM_CONTEXT_GETPARAM, &p) == 0);
>  }
>  
> -void gem_context_require_ban_period(int fd)
> +void gem_context_require_bannable(int fd)
>  {
>  	static int has_ban_period = -1;
> +	static int has_bannable = -1;
> +
> +	if (has_bannable < 0) {
> +		struct local_i915_gem_context_param p;
> +
> +		p.context = 0;
> +		p.param = LOCAL_CONTEXT_PARAM_BANNABLE;
> +		p.value = 0;
> +		p.size = 0;
> +
> +		has_bannable = igt_ioctl(fd, LOCAL_IOCTL_I915_GEM_CONTEXT_GETPARAM, &p) == 0;
> +	}
>  
>  	if (has_ban_period < 0) {
>  		struct local_i915_gem_context_param p;
> @@ -976,7 +988,7 @@ void gem_context_require_ban_period(int fd)
>  		has_ban_period = igt_ioctl(fd, LOCAL_IOCTL_I915_GEM_CONTEXT_GETPARAM, &p) == 0;
>  	}
>  
> -	igt_require(has_ban_period);
> +	igt_require(has_ban_period || has_bannable);
>  }
>  
>  int __gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, uint32_t *handle)
> diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
> index 465f760..2627097 100644
> --- a/lib/ioctl_wrappers.h
> +++ b/lib/ioctl_wrappers.h
> @@ -120,9 +120,10 @@ struct local_i915_gem_context_param {
>  #define LOCAL_CONTEXT_PARAM_NO_ZEROMAP	0x2
>  #define LOCAL_CONTEXT_PARAM_GTT_SIZE	0x3
>  #define LOCAL_CONTEXT_PARAM_NO_ERROR_CAPTURE	0x4
> +#define LOCAL_CONTEXT_PARAM_BANNABLE	0x5
>  	uint64_t value;
>  };
> -void gem_context_require_ban_period(int fd);
> +void gem_context_require_bannable(int fd);
>  void gem_context_require_param(int fd, uint64_t param);
>  void gem_context_get_param(int fd, struct local_i915_gem_context_param *p);
>  void gem_context_set_param(int fd, struct local_i915_gem_context_param *p);
> diff --git a/tests/gem_ctx_param.c b/tests/gem_ctx_param.c
> index 57d5e36..efdaf19 100644
> --- a/tests/gem_ctx_param.c
> +++ b/tests/gem_ctx_param.c
> @@ -43,6 +43,11 @@ igt_main
>  
>  	arg.param = LOCAL_CONTEXT_PARAM_BAN_PERIOD;
>  
> +	/* XXX start to enforce ban period returning -EINVAL when
> +	 * transition has been done */
> +	if (__gem_context_get_param(fd, &arg) == -EINVAL)
> +		arg.param = LOCAL_CONTEXT_PARAM_BANNABLE;
> +
>  	igt_subtest("basic") {
>  		arg.context = ctx;
>  		gem_context_get_param(fd, &arg);
> @@ -82,8 +87,6 @@ igt_main
>  		arg.size = 0;
>  	}
>  
> -	arg.param = LOCAL_CONTEXT_PARAM_BAN_PERIOD;
> -
>  	igt_subtest("non-root-set") {
>  		igt_fork(child, 1) {
>  			igt_drop_root();
> @@ -137,7 +140,7 @@ igt_main
>  	 * to catch ABI extensions. Don't "fix" this testcase without adding all
>  	 * the tests for the new param first.
>  	 */
> -	arg.param = LOCAL_CONTEXT_PARAM_NO_ERROR_CAPTURE + 1;
> +	arg.param = LOCAL_CONTEXT_PARAM_BANNABLE + 1;
>  
>  	igt_subtest("invalid-param-get") {
>  		arg.context = ctx;
> -- 
> 2.7.4
Mika Kuoppala Nov. 11, 2016, 2:36 p.m. UTC | #3 
Chris Wilson <chris@chris-wilson.co.uk> writes:

> On Fri, Nov 11, 2016 at 03:55:03PM +0200, Mika Kuoppala wrote:
>> Context BAN_PERIOD will get depracated so subsitute it with BANNABLE
>> property. Make ctx param test to accept both variants for now
>> until kernel changes have landed, to not break BAT.
>> 
>> v2: check against - EINVAL on get/set ban as it can return -EPERM
>> v3: better naming for get/set (Chris)
>> 
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>
> There's a small assumption of symmetry between the get/set paths, but it
> looks ok as a transitional patch.
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Pushed. Thanks for review.

-Mika


> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre
diff mbox

Patch

diff --git a/lib/igt_gt.c b/lib/igt_gt.c
index a165932..468c771 100644
--- a/lib/igt_gt.c
+++ b/lib/igt_gt.c
@@ -119,11 +119,43 @@  void igt_require_hang_ring(int fd, int ring)
 		igt_skip("hang injection disabled by user");
 
 	gem_require_ring(fd, ring);
-	gem_context_require_ban_period(fd);
+	gem_context_require_bannable(fd);
 	if (!igt_check_boolean_env_var("IGT_HANG_WITHOUT_RESET", false))
 		igt_require(has_gpu_reset(fd));
 }
 
+static unsigned context_get_ban(int fd, unsigned ctx)
+{
+	struct local_i915_gem_context_param param;
+
+	param.param = LOCAL_CONTEXT_PARAM_BANNABLE;
+	param.value = 0;
+	param.size = 0;
+
+	if (__gem_context_get_param(fd, &param) == -EINVAL) {
+		igt_assert(param.value == 0);
+		param.param = LOCAL_CONTEXT_PARAM_BAN_PERIOD;
+		gem_context_get_param(fd, &param);
+	}
+
+	return param.value;
+}
+
+static void context_set_ban(int fd, unsigned ctx, unsigned ban)
+{
+	struct local_i915_gem_context_param param;
+
+	param.value = ban;
+	param.size = 0;
+	param.param = LOCAL_CONTEXT_PARAM_BANNABLE;
+
+	if(__gem_context_set_param(fd, &param) == -EINVAL) {
+		igt_assert(param.value == ban);
+		param.param = LOCAL_CONTEXT_PARAM_BAN_PERIOD;
+		gem_context_set_param(fd, &param);
+	}
+}
+
 igt_hang_t igt_allow_hang(int fd, unsigned ctx, unsigned flags)
 {
 	struct local_i915_gem_context_param param;
@@ -131,7 +163,7 @@  igt_hang_t igt_allow_hang(int fd, unsigned ctx, unsigned flags)
 
 	if (!igt_check_boolean_env_var("IGT_HANG", true))
 		igt_skip("hang injection disabled by user");
-	gem_context_require_ban_period(fd);
+	gem_context_require_bannable(fd);
 	if (!igt_check_boolean_env_var("IGT_HANG_WITHOUT_RESET", false))
 		igt_require(has_gpu_reset(fd));
 
@@ -148,16 +180,9 @@  igt_hang_t igt_allow_hang(int fd, unsigned ctx, unsigned flags)
 		__gem_context_set_param(fd, &param);
 	}
 
-	param.param = LOCAL_CONTEXT_PARAM_BAN_PERIOD;
-	param.value = 0;
-	gem_context_get_param(fd, &param);
-	ban = param.value;
-
-	if ((flags & HANG_ALLOW_BAN) == 0) {
-		param.param = LOCAL_CONTEXT_PARAM_BAN_PERIOD;
-		param.value = 0;
-		gem_context_set_param(fd, &param);
-	}
+	ban = context_get_ban(fd, ctx);
+	if ((flags & HANG_ALLOW_BAN) == 0)
+		context_set_ban(fd, ctx, 0);
 
 	return (struct igt_hang){ 0, ctx, ban, flags };
 }
@@ -166,13 +191,11 @@  void igt_disallow_hang(int fd, igt_hang_t arg)
 {
 	struct local_i915_gem_context_param param;
 
-	param.context = arg.ctx;
-	param.size = 0;
-	param.param = LOCAL_CONTEXT_PARAM_BAN_PERIOD;
-	param.value = arg.ban;
-	gem_context_set_param(fd, &param);
+	context_set_ban(fd, arg.ctx, arg.ban);
 
 	if ((arg.flags & HANG_ALLOW_CAPTURE) == 0) {
+		param.context = arg.ctx;
+		param.size = 0;
 		param.param = LOCAL_CONTEXT_PARAM_NO_ERROR_CAPTURE;
 		param.value = 0;
 		if (__gem_context_set_param(fd, &param))
@@ -227,16 +250,10 @@  igt_hang_t igt_hang_ctx(int fd,
 		__gem_context_set_param(fd, &param);
 	}
 
-	param.param = LOCAL_CONTEXT_PARAM_BAN_PERIOD;
-	param.value = 0;
-	gem_context_get_param(fd, &param);
-	ban = param.value;
+	ban = context_get_ban(fd, ctx);
 
-	if ((flags & HANG_ALLOW_BAN) == 0) {
-		param.param = LOCAL_CONTEXT_PARAM_BAN_PERIOD;
-		param.value = 0;
-		gem_context_set_param(fd, &param);
-	}
+	if ((flags & HANG_ALLOW_BAN) == 0)
+		context_set_ban(fd, ctx, 0);
 
 	memset(&reloc, 0, sizeof(reloc));
 	memset(&exec, 0, sizeof(exec));
@@ -323,13 +340,11 @@  void igt_post_hang_ring(int fd, igt_hang_t arg)
 		       I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
 	gem_close(fd, arg.handle);
 
-	param.context = arg.ctx;
-	param.size = 0;
-	param.param = LOCAL_CONTEXT_PARAM_BAN_PERIOD;
-	param.value = arg.ban;
-	gem_context_set_param(fd, &param);
+	context_set_ban(fd, arg.ctx, arg.ban);
 
 	if ((arg.flags & HANG_ALLOW_CAPTURE) == 0) {
+		param.context = arg.ctx;
+		param.size = 0;
 		param.param = LOCAL_CONTEXT_PARAM_NO_ERROR_CAPTURE;
 		param.value = 0;
 		if (__gem_context_set_param(fd, &param))
diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
index eabf3ee..06638d4 100644
--- a/lib/ioctl_wrappers.c
+++ b/lib/ioctl_wrappers.c
@@ -961,9 +961,21 @@  void gem_context_require_param(int fd, uint64_t param)
 	igt_require(igt_ioctl(fd, LOCAL_IOCTL_I915_GEM_CONTEXT_GETPARAM, &p) == 0);
 }
 
-void gem_context_require_ban_period(int fd)
+void gem_context_require_bannable(int fd)
 {
 	static int has_ban_period = -1;
+	static int has_bannable = -1;
+
+	if (has_bannable < 0) {
+		struct local_i915_gem_context_param p;
+
+		p.context = 0;
+		p.param = LOCAL_CONTEXT_PARAM_BANNABLE;
+		p.value = 0;
+		p.size = 0;
+
+		has_bannable = igt_ioctl(fd, LOCAL_IOCTL_I915_GEM_CONTEXT_GETPARAM, &p) == 0;
+	}
 
 	if (has_ban_period < 0) {
 		struct local_i915_gem_context_param p;
@@ -976,7 +988,7 @@  void gem_context_require_ban_period(int fd)
 		has_ban_period = igt_ioctl(fd, LOCAL_IOCTL_I915_GEM_CONTEXT_GETPARAM, &p) == 0;
 	}
 
-	igt_require(has_ban_period);
+	igt_require(has_ban_period || has_bannable);
 }
 
 int __gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t flags, uint32_t *handle)
diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
index 465f760..2627097 100644
--- a/lib/ioctl_wrappers.h
+++ b/lib/ioctl_wrappers.h
@@ -120,9 +120,10 @@  struct local_i915_gem_context_param {
 #define LOCAL_CONTEXT_PARAM_NO_ZEROMAP	0x2
 #define LOCAL_CONTEXT_PARAM_GTT_SIZE	0x3
 #define LOCAL_CONTEXT_PARAM_NO_ERROR_CAPTURE	0x4
+#define LOCAL_CONTEXT_PARAM_BANNABLE	0x5
 	uint64_t value;
 };
-void gem_context_require_ban_period(int fd);
+void gem_context_require_bannable(int fd);
 void gem_context_require_param(int fd, uint64_t param);
 void gem_context_get_param(int fd, struct local_i915_gem_context_param *p);
 void gem_context_set_param(int fd, struct local_i915_gem_context_param *p);
diff --git a/tests/gem_ctx_param.c b/tests/gem_ctx_param.c
index 57d5e36..efdaf19 100644
--- a/tests/gem_ctx_param.c
+++ b/tests/gem_ctx_param.c
@@ -43,6 +43,11 @@  igt_main
 
 	arg.param = LOCAL_CONTEXT_PARAM_BAN_PERIOD;
 
+	/* XXX start to enforce ban period returning -EINVAL when
+	 * transition has been done */
+	if (__gem_context_get_param(fd, &arg) == -EINVAL)
+		arg.param = LOCAL_CONTEXT_PARAM_BANNABLE;
+
 	igt_subtest("basic") {
 		arg.context = ctx;
 		gem_context_get_param(fd, &arg);
@@ -82,8 +87,6 @@  igt_main
 		arg.size = 0;
 	}
 
-	arg.param = LOCAL_CONTEXT_PARAM_BAN_PERIOD;
-
 	igt_subtest("non-root-set") {
 		igt_fork(child, 1) {
 			igt_drop_root();
@@ -137,7 +140,7 @@  igt_main
 	 * to catch ABI extensions. Don't "fix" this testcase without adding all
 	 * the tests for the new param first.
 	 */
-	arg.param = LOCAL_CONTEXT_PARAM_NO_ERROR_CAPTURE + 1;
+	arg.param = LOCAL_CONTEXT_PARAM_BANNABLE + 1;
 
 	igt_subtest("invalid-param-get") {
 		arg.context = ctx;