On Mon, 2017-08-28 at 10:50 +0200, Katarzyna Dec wrote:
> CI is observing sporadical failures in pm_rps subtests.
> There are a couple of reasons. One of them is the fact that
> on gen6, gen7 and gen7.5, max frequency (as in the HW limit)
> is not set to RP0, but the value obtaind from PCODE (which
> may be different from RP0). Thus the test is operating under
> wrong assumptions (SOFTMAX == RP0 == BOOST which is simply
> not the case). Let's compare current frequency with BOOST
> frequency rather than SOFTMAX to get the test behaviour under control.
> In boost_freq function I set MAX freq to midium freqency, which ensures
> that we for sure reach BOOST frequency. This could help with failures
> with boost frequency failing to drop down.
> GPU reset needs to be modified so we are not dependent on kernel's low
> priority retire worker. Reset method was replaced by igt_force_gpu_reset()
> and in reset testcase we make sure that we can recover from hang.
>
> v2: Commit message, simplified waiting for boost to finish, drop
> noisy whitespace cleanup.
>
> v3: Removed reading from i915_rps_boost_info debugfs because it not
> the same on every kernel. Removed function waiting for boost.
> Instead of that I made sure we will reach in boost by setting MAX freq to
> fmid.
>
> v4: Moved proposal with making test drm master to other patch
>
> v5: Used igt_force_gpu_reset() to reset GPU. Modified "reset" testcase.
>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Jeff Mcgee <jeff.mcgee@xxxxxxxxx>
> Cc: Petri Latvala <petri.latvala@xxxxxxxxx>
> Cc: Jani Saarinen <jani.saarinen@xxxxxxxxx>
> Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@xxxxxxxxx>
> Signed-off-by: Katarzyna Dec <katarzyna.dec@xxxxxxxxx>
> ---
> tests/pm_rps.c | 63 +++++++++++++++++++++++++++++++++++--------------------
> ---
> 1 file changed, 38 insertions(+), 25 deletions(-)
>
> diff --git a/tests/pm_rps.c b/tests/pm_rps.c
> index f0455e78..a6c6f1eb 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)
> @@ -167,7 +168,7 @@ static void dump(const int *freqs)
> }
>
> enum load {
> - LOW,
> + LOW = 0,
> HIGH
> };
>
> @@ -185,9 +186,10 @@ static struct load_helper {
>
> static void load_helper_signal_handler(int sig)
> {
> - if (sig == SIGUSR2)
> - lh.load = lh.load == LOW ? HIGH : LOW;
> - else
> + if (sig == SIGUSR2) {
> + lh.load = !lh.load;
> + igt_debug("Switching background load to %s\n", lh.load ?
> "high" : "low");
> + } else
> lh.exit = true;
> }
>
> @@ -238,6 +240,7 @@ static void load_helper_run(enum load load)
> return;
> }
>
> + lh.exit = false;
> lh.load = load;
>
> igt_fork_helper(&lh.igt_proc) {
> @@ -263,6 +266,8 @@ static void load_helper_run(enum load load)
> if (intel_gen(lh.devid) >= 6)
> execbuf.flags = I915_EXEC_BLT;
>
> + igt_debug("Applying %s load...\n", lh.load ? "high" : "low");
> +
> while (!lh.exit) {
> memset(&object, 0, sizeof(object));
> object.handle = fences[val%3];
> @@ -296,6 +301,8 @@ static void load_helper_run(enum load load)
> gem_close(drm_fd, fences[0]);
> gem_close(drm_fd, fences[1]);
> gem_close(drm_fd, fences[2]);
> +
> + igt_drop_caches_set(drm_fd, DROP_RETIRE);
> }
> }
>
> @@ -553,38 +560,43 @@ static void stabilize_check(int *out)
> igt_debug("Waited %d msec to stabilize cur\n", wait);
> }
>
> -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 void boost_freq(int fd, int *boost_freqs)
> {
> int64_t timeout = 1;
> - int ring = -1;
> igt_spin_t *load;
> + unsigned int engine;
> + int fmid = (origfreqs[RPn] + origfreqs[RP0]) / 2;
>
> - load = igt_spin_batch_new(fd, ring, 0);
> + fmid = get_hw_rounded_freq(fmid);
> + //set max freq to less then boost freq
Looks good to me.
Just one very minor comment - we do use two different comment styles, should we
instead use one? Do you think we should add any simple test descriptions above
the tests or these tests are easy to understand?
> + writeval(stuff[MAX].filp, fmid);
>
> + /* put boost on the same engine as low load */
> + engine = I915_EXEC_RENDER;
Otherwise
Reviewed-by: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@xxxxxxxxx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
