[i-g-t] igt/pm_rps: current freq < user specified min is no longer a fail
diff mbox

Message ID 1447277879-4106-1-git-send-email-bob.j.paauwe@intel.com
State New
Headers show

Commit Message

Bob Paauwe Nov. 11, 2015, 9:37 p.m. UTC
Since commit

  commit aed242ff7ebb697e4dff912bd4dc7ec7192f7581
  Author: Chris Wilson <chris@chris-wilson.co.uk>
  Date:   Wed Mar 18 09:48:21 2015 +0000

      drm/i915: Relax RPS contraints to allows setting minfreq on idle

it is now possible that the current frequency will drop below the user
specified minimum frequency. Update the pm_rps tests to reflect that
this is no longer considered a failure.

Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com>
---
 tests/pm_rps.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Imre Deak Nov. 12, 2015, 9:18 a.m. UTC | #1
On ke, 2015-11-11 at 13:37 -0800, Bob Paauwe wrote:
> Since commit
> 
>   commit aed242ff7ebb697e4dff912bd4dc7ec7192f7581
>   Author: Chris Wilson <chris@chris-wilson.co.uk>
>   Date:   Wed Mar 18 09:48:21 2015 +0000
> 
>       drm/i915: Relax RPS contraints to allows setting minfreq on
> idle
> 
> it is now possible that the current frequency will drop below the
> user
> specified minimum frequency. Update the pm_rps tests to reflect that
> this is no longer considered a failure.

Yes, in case the GPU is idle the frequency will drop now to the idle fr
equency regardless of the minimum limit set by the user. We should
still check though if the frequency drops to the idle value not
something higher. You can use the i915_frequency_info in debugfs for
that.

> 
> Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com>
> ---
>  tests/pm_rps.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/pm_rps.c b/tests/pm_rps.c
> index 74f08f4..e92ca3b 100644
> --- a/tests/pm_rps.c
> +++ b/tests/pm_rps.c
> @@ -146,7 +146,6 @@ static void checkit(const int *freqs)
>  {
>  	igt_assert_lte(freqs[MIN], freqs[MAX]);
>  	igt_assert_lte(freqs[CUR], freqs[MAX]);
> -	igt_assert_lte(freqs[MIN], freqs[CUR]);
>  	igt_assert_lte(freqs[RPn], freqs[MIN]);
>  	igt_assert_lte(freqs[MAX], freqs[RP0]);
>  	igt_assert_lte(freqs[RP1], freqs[RP0]);
> @@ -472,14 +471,14 @@ static void idle_check(void)
>  		read_freqs(freqs);
>  		dump(freqs);
>  		checkit(freqs);
> -		if (freqs[CUR] == freqs[MIN])
> +		if (freqs[CUR] <= freqs[MIN])
>  			break;
>  		usleep(1000 * IDLE_WAIT_TIMESTEP_MSEC);
>  		wait += IDLE_WAIT_TIMESTEP_MSEC;
>  	} while (wait < IDLE_WAIT_TIMEOUT_MSEC);
>  
> -	igt_assert_eq(freqs[CUR], freqs[MIN]);
> -	igt_debug("Required %d msec to reach cur=min\n", wait);
> +	igt_assert_lte(freqs[CUR], freqs[MIN]);
> +	igt_debug("Required %d msec to reach cur<=min\n", wait);
>  }
>  
>  #define LOADED_WAIT_TIMESTEP_MSEC 100
Bob Paauwe Nov. 12, 2015, 6:17 p.m. UTC | #2
On Thu, 12 Nov 2015 11:18:11 +0200
Imre Deak <imre.deak@intel.com> wrote:

> On ke, 2015-11-11 at 13:37 -0800, Bob Paauwe wrote:
> > Since commit
> > 
> >   commit aed242ff7ebb697e4dff912bd4dc7ec7192f7581
> >   Author: Chris Wilson <chris@chris-wilson.co.uk>
> >   Date:   Wed Mar 18 09:48:21 2015 +0000
> > 
> >       drm/i915: Relax RPS contraints to allows setting minfreq on
> > idle
> > 
> > it is now possible that the current frequency will drop below the
> > user
> > specified minimum frequency. Update the pm_rps tests to reflect that
> > this is no longer considered a failure.
> 
> Yes, in case the GPU is idle the frequency will drop now to the idle fr
> equency regardless of the minimum limit set by the user. We should
> still check though if the frequency drops to the idle value not
> something higher. You can use the i915_frequency_info in debugfs for
> that.
> 

Yeah, good idea.  

Right now, the idle freq is the same as the hardware minimum.  So I
could have it check against the hardware min.   Adding support to read
the info out i915_frequency_info would be a larger change to the test.
But that might be a better long term solution.

> > 
> > Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com>
> > ---
> >  tests/pm_rps.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tests/pm_rps.c b/tests/pm_rps.c
> > index 74f08f4..e92ca3b 100644
> > --- a/tests/pm_rps.c
> > +++ b/tests/pm_rps.c
> > @@ -146,7 +146,6 @@ static void checkit(const int *freqs)
> >  {
> >  	igt_assert_lte(freqs[MIN], freqs[MAX]);
> >  	igt_assert_lte(freqs[CUR], freqs[MAX]);
> > -	igt_assert_lte(freqs[MIN], freqs[CUR]);
> >  	igt_assert_lte(freqs[RPn], freqs[MIN]);
> >  	igt_assert_lte(freqs[MAX], freqs[RP0]);
> >  	igt_assert_lte(freqs[RP1], freqs[RP0]);
> > @@ -472,14 +471,14 @@ static void idle_check(void)
> >  		read_freqs(freqs);
> >  		dump(freqs);
> >  		checkit(freqs);
> > -		if (freqs[CUR] == freqs[MIN])
> > +		if (freqs[CUR] <= freqs[MIN])
> >  			break;
> >  		usleep(1000 * IDLE_WAIT_TIMESTEP_MSEC);
> >  		wait += IDLE_WAIT_TIMESTEP_MSEC;
> >  	} while (wait < IDLE_WAIT_TIMEOUT_MSEC);
> >  
> > -	igt_assert_eq(freqs[CUR], freqs[MIN]);
> > -	igt_debug("Required %d msec to reach cur=min\n", wait);
> > +	igt_assert_lte(freqs[CUR], freqs[MIN]);
> > +	igt_debug("Required %d msec to reach cur<=min\n", wait);
> >  }
> >  
> >  #define LOADED_WAIT_TIMESTEP_MSEC 100
Imre Deak Nov. 30, 2015, 11:30 a.m. UTC | #3
On ke, 2015-11-11 at 13:37 -0800, Bob Paauwe wrote:
> Since commit
> 
>   commit aed242ff7ebb697e4dff912bd4dc7ec7192f7581
>   Author: Chris Wilson <chris@chris-wilson.co.uk>
>   Date:   Wed Mar 18 09:48:21 2015 +0000
> 
>       drm/i915: Relax RPS contraints to allows setting minfreq on
> idle
> 
> it is now possible that the current frequency will drop below the
> user
> specified minimum frequency. Update the pm_rps tests to reflect that
> this is no longer considered a failure.
> 
> Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com>

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89728

Bob could you send an updated version of this patch?

> ---
>  tests/pm_rps.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/pm_rps.c b/tests/pm_rps.c
> index 74f08f4..e92ca3b 100644
> --- a/tests/pm_rps.c
> +++ b/tests/pm_rps.c
> @@ -146,7 +146,6 @@ static void checkit(const int *freqs)
>  {
>  	igt_assert_lte(freqs[MIN], freqs[MAX]);
>  	igt_assert_lte(freqs[CUR], freqs[MAX]);
> -	igt_assert_lte(freqs[MIN], freqs[CUR]);
>  	igt_assert_lte(freqs[RPn], freqs[MIN]);
>  	igt_assert_lte(freqs[MAX], freqs[RP0]);
>  	igt_assert_lte(freqs[RP1], freqs[RP0]);
> @@ -472,14 +471,14 @@ static void idle_check(void)
>  		read_freqs(freqs);
>  		dump(freqs);
>  		checkit(freqs);
> -		if (freqs[CUR] == freqs[MIN])
> +		if (freqs[CUR] <= freqs[MIN])
>  			break;
>  		usleep(1000 * IDLE_WAIT_TIMESTEP_MSEC);
>  		wait += IDLE_WAIT_TIMESTEP_MSEC;
>  	} while (wait < IDLE_WAIT_TIMEOUT_MSEC);
>  
> -	igt_assert_eq(freqs[CUR], freqs[MIN]);
> -	igt_debug("Required %d msec to reach cur=min\n", wait);
> +	igt_assert_lte(freqs[CUR], freqs[MIN]);
> +	igt_debug("Required %d msec to reach cur<=min\n", wait);
>  }
>  
>  #define LOADED_WAIT_TIMESTEP_MSEC 100
Bob Paauwe Dec. 1, 2015, 12:23 a.m. UTC | #4
The first patch updates the test to work now that the frequency can drop
below the user specified minimum to the idle frequency. 

The second patch attempts to improve the checking and actually checks
that the frequency has dropped to RPn in some cases.  I'm not real
happy with this patch but didn't want to re-write the entire test program
to make handle these cases more elegantly.  Other ideas welcome.

Bob Paauwe (2):
  igt/pm_rps: current freq < user specified min is not a fail (v2)
  igt/pm_rps: Add checks for freq = idle (RPn) in specific cases.

 tests/pm_rps.c | 55 ++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 38 insertions(+), 17 deletions(-)

Patch
diff mbox

diff --git a/tests/pm_rps.c b/tests/pm_rps.c
index 74f08f4..e92ca3b 100644
--- a/tests/pm_rps.c
+++ b/tests/pm_rps.c
@@ -146,7 +146,6 @@  static void checkit(const int *freqs)
 {
 	igt_assert_lte(freqs[MIN], freqs[MAX]);
 	igt_assert_lte(freqs[CUR], freqs[MAX]);
-	igt_assert_lte(freqs[MIN], freqs[CUR]);
 	igt_assert_lte(freqs[RPn], freqs[MIN]);
 	igt_assert_lte(freqs[MAX], freqs[RP0]);
 	igt_assert_lte(freqs[RP1], freqs[RP0]);
@@ -472,14 +471,14 @@  static void idle_check(void)
 		read_freqs(freqs);
 		dump(freqs);
 		checkit(freqs);
-		if (freqs[CUR] == freqs[MIN])
+		if (freqs[CUR] <= freqs[MIN])
 			break;
 		usleep(1000 * IDLE_WAIT_TIMESTEP_MSEC);
 		wait += IDLE_WAIT_TIMESTEP_MSEC;
 	} while (wait < IDLE_WAIT_TIMEOUT_MSEC);
 
-	igt_assert_eq(freqs[CUR], freqs[MIN]);
-	igt_debug("Required %d msec to reach cur=min\n", wait);
+	igt_assert_lte(freqs[CUR], freqs[MIN]);
+	igt_debug("Required %d msec to reach cur<=min\n", wait);
 }
 
 #define LOADED_WAIT_TIMESTEP_MSEC 100