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 <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?
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 <radoslaw.szwichtenberg@xxxxxxxxx
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
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
