[Intel-gfx] [PATCH i-g-t] pm_rps: Changes in waitboost scenario

[Chris Wilson]

Quoting Katarzyna Dec (2017-08-18 08:33:11)
> Waitboost and reset test cases were failing because maximum
> frequency is not always boost frequency (sometimes overcloking
> occurs).
> Moreover more time is needed for boost to be finished.
> Changed boost_freq function so boost occurred on the same engine
> as low load.
> Added function waiting for boost to be finished.
> Made test DRM master to make sure that the test isn't ran along Xorg
> 
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Jeff Mcgee <jeff.mcgee at intel.com>
> Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg at intel.com>
> 
> Signed-off-by: Katarzyna Dec <katarzyna.dec at intel.com>
> ---
>  tests/pm_rps.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 50 insertions(+), 8 deletions(-)
> 
> diff --git a/tests/pm_rps.c b/tests/pm_rps.c
> index f0455e78..08a73f5d 100644
> --- a/tests/pm_rps.c
> +++ b/tests/pm_rps.c
> @@ -50,6 +50,7 @@ enum {
>         RP0,
>         RP1,
>         RPn,
> +       BOOST,
>         NUMFREQ
>  };
>  
> @@ -60,7 +61,7 @@ struct junk {
>         const char *mode;
>         FILE *filp;
>  } stuff[] = {
> -       { "cur", "r", NULL }, { "min", "rb+", NULL }, { "max", "rb+", NULL }, { "RP0", "r", NULL }, { "RP1", "r", NULL }, { "RPn", "r", NULL }, { NULL, NULL, NULL }
> +       { "cur", "r", NULL }, { "min", "rb+", NULL }, { "max", "rb+", NULL }, { "RP0", "r", NULL }, { "RP1", "r", NULL }, { "RPn", "r", NULL }, { "boost", "rb+", NULL }, { NULL, NULL, NULL }
>  };
>  
>  static int readval(FILE *filp)
> @@ -556,25 +557,61 @@ static void stabilize_check(int *out)
>  static void reset_gpu(void)
>  {
>         int fd = drm_open_driver(DRIVER_INTEL);
> +
>         igt_post_hang_ring(fd, igt_hang_ring(fd, I915_EXEC_DEFAULT));
>         close(fd);
>  }
>  
> +static bool is_in_boost(void)
> +{
> +       char buf[1024];
> +
> +       igt_debugfs_read(drm_fd, "i915_rps_boost_info", buf);
> +       return strstr(buf, "Boosts outstanding? 1");

Might as well make this generic and return the count. Then you can use
it to return -1 if either i915_rps_boost_info doesn't exist (oops, buf
used unterminated!) or doesn't contain the count.

> +}
> +
>  static void boost_freq(int fd, int *boost_freqs)
>  {
>         int64_t timeout = 1;
> -       int ring = -1;
>         igt_spin_t *load;
> +       unsigned int engine;
>  
> -       load = igt_spin_batch_new(fd, ring, 0);
> -
> +       /* put boost on the same engine as low load */
> +       engine = I915_EXEC_RENDER;
> +       if (intel_gen(lh.devid) >= 6)
> +               engine = I915_EXEC_BLT;
> +       load = igt_spin_batch_new(fd, engine, 0);

Something to note is that spin-batch will also force the GPU to maximum.

You could set the boost freq > max freq to differentiate

>         /* Waiting will grant us a boost to maximum */
>         gem_wait(fd, load->handle, &timeout);
>  
>         read_freqs(boost_freqs);
>         dump(boost_freqs);
> +       igt_assert_eq(is_in_boost(), 1);

Will fail on older kernels.

> +       /* Avoid downlocking till boost request is pending */
> +       igt_spin_batch_end(load);
> +       gem_sync(fd, load->handle);
>         igt_spin_batch_free(fd, load);
> +

Misplaced whitespace.

> +}
> +
> +#define BOOST_WAIT_TIMESTEP_MSEC 250
> +#define BOOST_WAIT_TIMEOUT_MSEC 15000
> +static void fall_from_boost_wait(int *freqs)
> +{
> +       int wait = 0;
> +
> +       do {
> +               read_freqs(freqs);
> +               dump(freqs);

Why pass in freqs? After this function you overwrite them, and by its
nature the value of those freqs whilst in this function is either
boosted or some other value.

> +               if (!is_in_boost())
> +                       break;
> +               usleep(1000 * BOOST_WAIT_TIMESTEP_MSEC);
> +               wait += BOOST_WAIT_TIMESTEP_MSEC;
> +
> +       } while (wait < BOOST_WAIT_TIMEOUT_MSEC);
> +
> +       igt_debug("Waited %d till falling from boost\n", wait);

Units! "Waited %dms"

>  }
>  
>  static void waitboost(bool reset)
> @@ -601,6 +638,7 @@ static void waitboost(bool reset)
>          * to maximum.
>          */
>         boost_freq(fd, boost_freqs);
> +       fall_from_boost_wait(post_freqs);

wait_for_fall_from_grace() ?

>  
>         igt_debug("Apply low load again...\n");
>         sleep(1);
> @@ -611,7 +649,7 @@ static void waitboost(bool reset)
>         idle_check();
>  
>         igt_assert_lt(pre_freqs[CUR], pre_freqs[MAX]);
> -       igt_assert_eq(boost_freqs[CUR], boost_freqs[MAX]);
> +       igt_assert_eq(boost_freqs[CUR], boost_freqs[BOOST]);
>         igt_assert_lt(post_freqs[CUR], post_freqs[MAX]);
>  
>         close(fd);
> @@ -640,14 +678,15 @@ igt_main
>                 struct junk *junk = stuff;
>                 int ret;
>  
> -               /* Use drm_open_driver to verify device existence */
> -               drm_fd = drm_open_driver(DRIVER_INTEL);
> +               /* Use drm_open_driver to to force running tests as drm master */
> +               drm_fd = drm_open_driver_master(DRIVER_INTEL);

?? This interface / policy does not require the display.

>                 igt_require_gem(drm_fd);
>                 igt_require(gem_can_store_dword(drm_fd, 0));
>  
>                 do {
>                         int val = -1;
>                         char *path;
> +
>                         ret = asprintf(&path, sysfs_base_path, device, junk->name);
>                         igt_assert(ret != -1);
>                         junk->filp = fopen(path, junk->mode);
> @@ -657,7 +696,7 @@ igt_main
>                         val = readval(junk->filp);
>                         igt_assert(val >= 0);
>                         junk++;
> -               } while(junk->name != NULL);
> +               } while (junk->name != NULL);
>  
>                 read_freqs(origfreqs);
>  
> @@ -684,4 +723,7 @@ igt_main
>         igt_subtest("reset")
>                 waitboost(true);
>  
> +       igt_fixture {
> +               intel_register_access_fini();

??