[PATCH 1/3] tests/pm_rps: ducttape for igt fork helper cleanup issues

[PATCH 1/3] tests/pm_rps: ducttape for igt fork helper cleanup issues

[Daniel Vetter]

We don't call cleanup handlers when exiting a subtest currently, only
when exiting the entire binary. Which means pm_rps falls over when it
fails more than one subtest.

Cc: Jeff McGee <jeff.mcgee@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 tests/pm_rps.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/tests/pm_rps.c b/tests/pm_rps.c
index a652cf580dc7..b1cd13fc33a7 100644
--- a/tests/pm_rps.c
+++ b/tests/pm_rps.c
@@ -196,9 +196,27 @@ static void emit_store_dword_imm(uint32_t val)
 }
 
 #define LOAD_HELPER_PAUSE_USEC 500
+static void load_helper_set_load(enum load load)
+{
+	assert(lh.igt_proc.running);
+
+	if (lh.load == load)
+		return;
+
+	lh.load = load;
+	kill(lh.igt_proc.pid, SIGUSR2);
+}
+
 static void load_helper_run(enum load load)
 {
-	assert(!lh.igt_proc.running);
+	/*
+	 * FIXME fork helpers won't get cleaned up when started from within a
+	 * subtest, so handle the case where it sticks around a bit too long.
+	 */
+	if (lh.igt_proc.running) {
+		load_helper_set_load(load);
+		return;
+	}
 
 	igt_require(lh.ready == true);
 
@@ -229,20 +247,8 @@ static void load_helper_run(enum load load)
 	}
 }
 
-static void load_helper_set_load(enum load load)
-{
-	assert(lh.igt_proc.running);
-
-	if (lh.load == load)
-		return;
-
-	lh.load = load;
-	kill(lh.igt_proc.pid, SIGUSR2);
-}
-
 static void load_helper_stop(void)
 {
-	assert(lh.igt_proc.running);
 	kill(lh.igt_proc.pid, SIGUSR1);
 	igt_wait_helper(&lh.igt_proc);
 }
-- 
1.8.4.rc3

[PATCH 2/3] tests/pm_rps: simplify load helper setup

[Daniel Vetter]

There's no need to be fancy here.

Cc: Jeff McGee <jeff.mcgee@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 tests/pm_rps.c | 33 +++++++--------------------------
 1 file changed, 7 insertions(+), 26 deletions(-)

diff --git a/tests/pm_rps.c b/tests/pm_rps.c
index b1cd13fc33a7..fc6bac647f4a 100644
--- a/tests/pm_rps.c
+++ b/tests/pm_rps.c
@@ -153,7 +153,6 @@ static struct load_helper {
 	drm_intel_bufmgr *bufmgr;
 	struct intel_batchbuffer *batch;
 	drm_intel_bo *target_buffer;
-	bool ready;
 	enum load load;
 	bool exit;
 	struct igt_helper_process igt_proc;
@@ -218,8 +217,6 @@ static void load_helper_run(enum load load)
 		return;
 	}
 
-	igt_require(lh.ready == true);
-
 	lh.load = load;
 
 	igt_fork_helper(&lh.igt_proc) {
@@ -253,42 +250,26 @@ static void load_helper_stop(void)
 	igt_wait_helper(&lh.igt_proc);
 }
 
-/* The load helper resource is used by only some subtests. We attempt to
- * initialize in igt_fixture but do our igt_require check only if a
- * subtest attempts to run it */
 static void load_helper_init(void)
 {
 	lh.devid = intel_get_drm_devid(drm_fd);
 	lh.has_ppgtt = gem_uses_aliasing_ppgtt(drm_fd);
 
 	/* MI_STORE_DATA can only use GTT address on gen4+/g33 and needs
-	 * snoopable mem on pre-gen6. */
-	if (intel_gen(lh.devid) < 6) {
-		igt_debug("load helper init failed: pre-gen6 not supported\n");
-		return;
-	}
-
+	 * snoopable mem on pre-gen6. Hence load-helper only works on gen6+, but
+	 * that's also all we care about for the rps testcase*/
+	igt_assert(intel_gen(lh.devid) >= 6);
 	lh.bufmgr = drm_intel_bufmgr_gem_init(drm_fd, 4096);
-	if (!lh.bufmgr) {
-		igt_debug("load helper init failed: buffer manager init\n");
-		return;
-	}
+	igt_assert(lh.bufmgr);
+
 	drm_intel_bufmgr_gem_enable_reuse(lh.bufmgr);
 
 	lh.batch = intel_batchbuffer_alloc(lh.bufmgr, lh.devid);
-	if (!lh.batch) {
-		igt_debug("load helper init failed: batch buffer alloc\n");
-		return;
-	}
+	igt_assert(lh.batch);
 
 	lh.target_buffer = drm_intel_bo_alloc(lh.bufmgr, "target bo",
 					      4096, 4096);
-	if (!lh.target_buffer) {
-		igt_debug("load helper init failed: target buffer alloc\n");
-		return;
-	}
-
-	lh.ready = true;
+	igt_assert(lh.target_buffer);
 }
 
 static void load_helper_deinit(void)
-- 
1.8.4.rc3

[PATCH 3/3] tests/pm_rps: load harder

[Daniel Vetter]

Big core platforms need some seriuos omph to break a sweat.

This fixes min-max-config-loaded here on my ivb.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75146
Cc: Jeff McGee <jeff.mcgee@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 tests/pm_rps.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/tests/pm_rps.c b/tests/pm_rps.c
index fc6bac647f4a..b5dd494443ff 100644
--- a/tests/pm_rps.c
+++ b/tests/pm_rps.c
@@ -156,6 +156,7 @@ static struct load_helper {
 	enum load load;
 	bool exit;
 	struct igt_helper_process igt_proc;
+	drm_intel_bo *src, *dst;
 } lh;
 
 static void load_helper_signal_handler(int sig)
@@ -195,6 +196,7 @@ static void emit_store_dword_imm(uint32_t val)
 }
 
 #define LOAD_HELPER_PAUSE_USEC 500
+#define LOAD_HELPER_BO_SIZE (16*1024*1024)
 static void load_helper_set_load(enum load load)
 {
 	assert(lh.igt_proc.running);
@@ -226,6 +228,10 @@ static void load_helper_run(enum load load)
 		signal(SIGUSR2, load_helper_signal_handler);
 
 		while (!lh.exit) {
+			if (lh.load == HIGH)
+				intel_copy_bo(lh.batch, lh.dst, lh.dst,
+					      LOAD_HELPER_BO_SIZE);
+
 			emit_store_dword_imm(val);
 			intel_batchbuffer_flush_on_ring(lh.batch, 0);
 			val++;
@@ -270,6 +276,13 @@ static void load_helper_init(void)
 	lh.target_buffer = drm_intel_bo_alloc(lh.bufmgr, "target bo",
 					      4096, 4096);
 	igt_assert(lh.target_buffer);
+
+	lh.dst = drm_intel_bo_alloc(lh.bufmgr, "dst bo",
+				    LOAD_HELPER_BO_SIZE, 4096);
+	igt_assert(lh.dst);
+	lh.src = drm_intel_bo_alloc(lh.bufmgr, "src bo",
+				    LOAD_HELPER_BO_SIZE, 4096);
+	igt_assert(lh.src);
 }
 
 static void load_helper_deinit(void)
-- 
1.8.4.rc3

Re: [PATCH 1/3] tests/pm_rps: ducttape for igt fork helper cleanup issues

[Jeff McGee]

On Fri, Mar 14, 2014 at 10:27:46AM +0100, Daniel Vetter wrote:
> We don't call cleanup handlers when exiting a subtest currently, only
> when exiting the entire binary. Which means pm_rps falls over when it
> fails more than one subtest.
> 
> Cc: Jeff McGee <jeff.mcgee@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  tests/pm_rps.c | 32 +++++++++++++++++++-------------
>  1 file changed, 19 insertions(+), 13 deletions(-)
> 
> diff --git a/tests/pm_rps.c b/tests/pm_rps.c
> index a652cf580dc7..b1cd13fc33a7 100644
> --- a/tests/pm_rps.c
> +++ b/tests/pm_rps.c
> @@ -196,9 +196,27 @@ static void emit_store_dword_imm(uint32_t val)
>  }
>  
>  #define LOAD_HELPER_PAUSE_USEC 500
> +static void load_helper_set_load(enum load load)
> +{
> +	assert(lh.igt_proc.running);
> +
> +	if (lh.load == load)
> +		return;
> +
> +	lh.load = load;
> +	kill(lh.igt_proc.pid, SIGUSR2);
> +}
> +
>  static void load_helper_run(enum load load)
>  {
> -	assert(!lh.igt_proc.running);
> +	/*
> +	 * FIXME fork helpers won't get cleaned up when started from within a
> +	 * subtest, so handle the case where it sticks around a bit too long.
> +	 */
> +	if (lh.igt_proc.running) {
> +		load_helper_set_load(load);
> +		return;
> +	}
>  
>  	igt_require(lh.ready == true);
>  
> @@ -229,20 +247,8 @@ static void load_helper_run(enum load load)
>  	}
>  }
>  
> -static void load_helper_set_load(enum load load)
> -{
> -	assert(lh.igt_proc.running);
> -
> -	if (lh.load == load)
> -		return;
> -
> -	lh.load = load;
> -	kill(lh.igt_proc.pid, SIGUSR2);
> -}
> -
>  static void load_helper_stop(void)
>  {
> -	assert(lh.igt_proc.running);
>  	kill(lh.igt_proc.pid, SIGUSR1);
>  	igt_wait_helper(&lh.igt_proc);
>  }
> -- 
> 1.8.4.rc3
> 

Unfortunately there are probably several ways in which a failed subtest will
contaminate subsequent subtests if all are run in the same process instance.
I asked about that earlier and you said that we don't concern too much about
it because the preferred way to run is with a test runner and each subtest
executed in a separate instance. If we do in fact care about supporting all
subtests in a single instance, can we put in place a subtest exit handler?
That would solve all issues similar to this.
-Jeff

Re: [PATCH 2/3] tests/pm_rps: simplify load helper setup

[Jeff McGee]

On Fri, Mar 14, 2014 at 10:27:47AM +0100, Daniel Vetter wrote:
> There's no need to be fancy here.
> 
> Cc: Jeff McGee <jeff.mcgee@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  tests/pm_rps.c | 33 +++++++--------------------------
>  1 file changed, 7 insertions(+), 26 deletions(-)
> 
> diff --git a/tests/pm_rps.c b/tests/pm_rps.c
> index b1cd13fc33a7..fc6bac647f4a 100644
> --- a/tests/pm_rps.c
> +++ b/tests/pm_rps.c
> @@ -153,7 +153,6 @@ static struct load_helper {
>  	drm_intel_bufmgr *bufmgr;
>  	struct intel_batchbuffer *batch;
>  	drm_intel_bo *target_buffer;
> -	bool ready;
>  	enum load load;
>  	bool exit;
>  	struct igt_helper_process igt_proc;
> @@ -218,8 +217,6 @@ static void load_helper_run(enum load load)
>  		return;
>  	}
>  
> -	igt_require(lh.ready == true);
> -
>  	lh.load = load;
>  
>  	igt_fork_helper(&lh.igt_proc) {
> @@ -253,42 +250,26 @@ static void load_helper_stop(void)
>  	igt_wait_helper(&lh.igt_proc);
>  }
>  
> -/* The load helper resource is used by only some subtests. We attempt to
> - * initialize in igt_fixture but do our igt_require check only if a
> - * subtest attempts to run it */
>  static void load_helper_init(void)
>  {
>  	lh.devid = intel_get_drm_devid(drm_fd);
>  	lh.has_ppgtt = gem_uses_aliasing_ppgtt(drm_fd);
>  
>  	/* MI_STORE_DATA can only use GTT address on gen4+/g33 and needs
> -	 * snoopable mem on pre-gen6. */
> -	if (intel_gen(lh.devid) < 6) {
> -		igt_debug("load helper init failed: pre-gen6 not supported\n");
> -		return;
> -	}
> -
> +	 * snoopable mem on pre-gen6. Hence load-helper only works on gen6+, but
> +	 * that's also all we care about for the rps testcase*/
> +	igt_assert(intel_gen(lh.devid) >= 6);
>  	lh.bufmgr = drm_intel_bufmgr_gem_init(drm_fd, 4096);
> -	if (!lh.bufmgr) {
> -		igt_debug("load helper init failed: buffer manager init\n");
> -		return;
> -	}
> +	igt_assert(lh.bufmgr);
> +
>  	drm_intel_bufmgr_gem_enable_reuse(lh.bufmgr);
>  
>  	lh.batch = intel_batchbuffer_alloc(lh.bufmgr, lh.devid);
> -	if (!lh.batch) {
> -		igt_debug("load helper init failed: batch buffer alloc\n");
> -		return;
> -	}
> +	igt_assert(lh.batch);
>  
>  	lh.target_buffer = drm_intel_bo_alloc(lh.bufmgr, "target bo",
>  					      4096, 4096);
> -	if (!lh.target_buffer) {
> -		igt_debug("load helper init failed: target buffer alloc\n");
> -		return;
> -	}
> -
> -	lh.ready = true;
> +	igt_assert(lh.target_buffer);
>  }
>  
>  static void load_helper_deinit(void)
> -- 
> 1.8.4.rc3
> 

My intent here was to allow a subtest which doesn't require the load helper to
run and pass even if the loader helper failed to initialize for whatever
reason. I'm fine if we'd rather avoid the complexity, but can we state that
decision more clearly in the commit message for future reference?
-Jeff

Re: [PATCH 3/3] tests/pm_rps: load harder

[Jeff McGee]

On Fri, Mar 14, 2014 at 10:27:48AM +0100, Daniel Vetter wrote:
> Big core platforms need some seriuos omph to break a sweat.
> 
> This fixes min-max-config-loaded here on my ivb.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75146
> Cc: Jeff McGee <jeff.mcgee@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  tests/pm_rps.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/tests/pm_rps.c b/tests/pm_rps.c
> index fc6bac647f4a..b5dd494443ff 100644
> --- a/tests/pm_rps.c
> +++ b/tests/pm_rps.c
> @@ -156,6 +156,7 @@ static struct load_helper {
>  	enum load load;
>  	bool exit;
>  	struct igt_helper_process igt_proc;
> +	drm_intel_bo *src, *dst;
>  } lh;
>  
>  static void load_helper_signal_handler(int sig)
> @@ -195,6 +196,7 @@ static void emit_store_dword_imm(uint32_t val)
>  }
>  
>  #define LOAD_HELPER_PAUSE_USEC 500
> +#define LOAD_HELPER_BO_SIZE (16*1024*1024)
>  static void load_helper_set_load(enum load load)
>  {
>  	assert(lh.igt_proc.running);
> @@ -226,6 +228,10 @@ static void load_helper_run(enum load load)
>  		signal(SIGUSR2, load_helper_signal_handler);
>  
>  		while (!lh.exit) {
> +			if (lh.load == HIGH)
> +				intel_copy_bo(lh.batch, lh.dst, lh.dst,
> +					      LOAD_HELPER_BO_SIZE);
> +
Did you mean to use lh.src here?
>  			emit_store_dword_imm(val);
>  			intel_batchbuffer_flush_on_ring(lh.batch, 0);
>  			val++;
> @@ -270,6 +276,13 @@ static void load_helper_init(void)
>  	lh.target_buffer = drm_intel_bo_alloc(lh.bufmgr, "target bo",
>  					      4096, 4096);
>  	igt_assert(lh.target_buffer);
> +
> +	lh.dst = drm_intel_bo_alloc(lh.bufmgr, "dst bo",
> +				    LOAD_HELPER_BO_SIZE, 4096);
> +	igt_assert(lh.dst);
> +	lh.src = drm_intel_bo_alloc(lh.bufmgr, "src bo",
> +				    LOAD_HELPER_BO_SIZE, 4096);
> +	igt_assert(lh.src);
>  }
>  
>  static void load_helper_deinit(void)
Should we free these in load_helper_deinit?
> -- 
> 1.8.4.rc3
> 

Re: [PATCH 2/3] tests/pm_rps: simplify load helper setup

[Daniel Vetter]

On Fri, Mar 14, 2014 at 3:34 PM, Jeff McGee <jeff.mcgee@intel.com> wrote:
> My intent here was to allow a subtest which doesn't require the load helper to
> run and pass even if the loader helper failed to initialize for whatever
> reason. I'm fine if we'd rather avoid the complexity, but can we state that
> decision more clearly in the commit message for future reference?

The set of machines where pm_rps runs and the set where where the load
helper init should succeed match. I agree the commit message is a bit
sparse, but I've pushed it out already ;-) I'll try to be better next
time around ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/3] tests/pm_rps: ducttape for igt fork helper cleanup issues

[Daniel Vetter]

On Fri, Mar 14, 2014 at 3:29 PM, Jeff McGee <jeff.mcgee@intel.com> wrote:
> Unfortunately there are probably several ways in which a failed subtest will
> contaminate subsequent subtests if all are run in the same process instance.
> I asked about that earlier and you said that we don't concern too much about
> it because the preferred way to run is with a test runner and each subtest
> executed in a separate instance. If we do in fact care about supporting all
> subtests in a single instance, can we put in place a subtest exit handler?
> That would solve all issues similar to this.

Yeah, that's my long-term idea, hence the FIXME comment. But
historically I've fumbled way too many special-cases wrt forked
children, subtests and getting the exit handler logic right. So I want
to do this carefully and have it all covered in a pile of igt
testcases to make sure it actually does what I want it to do.

Since I'm terribly busy writing igt library api docs atm I've
postponed this. The quick hack in this patch here just fixed the
annoying assert while I've hacked around on the pm_rps testcase.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/3] tests/pm_rps: load harder

[Daniel Vetter]

On Fri, Mar 14, 2014 at 09:41:13AM -0500, Jeff McGee wrote:
> On Fri, Mar 14, 2014 at 10:27:48AM +0100, Daniel Vetter wrote:
> > Big core platforms need some seriuos omph to break a sweat.
> > 
> > This fixes min-max-config-loaded here on my ivb.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75146
> > Cc: Jeff McGee <jeff.mcgee@intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  tests/pm_rps.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/tests/pm_rps.c b/tests/pm_rps.c
> > index fc6bac647f4a..b5dd494443ff 100644
> > --- a/tests/pm_rps.c
> > +++ b/tests/pm_rps.c
> > @@ -156,6 +156,7 @@ static struct load_helper {
> >  	enum load load;
> >  	bool exit;
> >  	struct igt_helper_process igt_proc;
> > +	drm_intel_bo *src, *dst;
> >  } lh;
> >  
> >  static void load_helper_signal_handler(int sig)
> > @@ -195,6 +196,7 @@ static void emit_store_dword_imm(uint32_t val)
> >  }
> >  
> >  #define LOAD_HELPER_PAUSE_USEC 500
> > +#define LOAD_HELPER_BO_SIZE (16*1024*1024)
> >  static void load_helper_set_load(enum load load)
> >  {
> >  	assert(lh.igt_proc.running);
> > @@ -226,6 +228,10 @@ static void load_helper_run(enum load load)
> >  		signal(SIGUSR2, load_helper_signal_handler);
> >  
> >  		while (!lh.exit) {
> > +			if (lh.load == HIGH)
> > +				intel_copy_bo(lh.batch, lh.dst, lh.dst,
> > +					      LOAD_HELPER_BO_SIZE);
> > +
> Did you mean to use lh.src here?
> >  			emit_store_dword_imm(val);
> >  			intel_batchbuffer_flush_on_ring(lh.batch, 0);
> >  			val++;
> > @@ -270,6 +276,13 @@ static void load_helper_init(void)
> >  	lh.target_buffer = drm_intel_bo_alloc(lh.bufmgr, "target bo",
> >  					      4096, 4096);
> >  	igt_assert(lh.target_buffer);
> > +
> > +	lh.dst = drm_intel_bo_alloc(lh.bufmgr, "dst bo",
> > +				    LOAD_HELPER_BO_SIZE, 4096);
> > +	igt_assert(lh.dst);
> > +	lh.src = drm_intel_bo_alloc(lh.bufmgr, "src bo",
> > +				    LOAD_HELPER_BO_SIZE, 4096);
> > +	igt_assert(lh.src);
> >  }
> >  
> >  static void load_helper_deinit(void)
> Should we free these in load_helper_deinit?

I've thrown a fixup patch on top with these two things fixed. Thanks a lot
for your feedback.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch