On Tue, Aug 29, 2017 at 07:43:20AM +0000, Szwichtenberg, Radoslaw wrote: > 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 <[email protected]> > > Cc: Jeff Mcgee <[email protected]> > > Cc: Petri Latvala <[email protected]> > > Cc: Jani Saarinen <[email protected]> > > Cc: Radoslaw Szwichtenberg <[email protected]> > > Signed-off-by: Katarzyna Dec <[email protected]> > > --- > > 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?
We try to follow the kernel's CodingStyle, see https://01.org/linuxgraphics/gfx-docs/drm/process/coding-style.html This means /* */ C style comments everywhere. Please fix. > > + writeval(stuff[MAX].filp, fmid); > > > > + /* put boost on the same engine as low load */ > > + engine = I915_EXEC_RENDER; > > Otherwise > Reviewed-by: Radoslaw Szwichtenberg <[email protected] When you resubmit, plus include Radek's r-b tag. Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
