| Message ID | 20170818110844.15792-1-katarzyna.dec@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Katarzyna Dec (2017-08-18 12:08:44) > While I'm here let's also make sure that we're running as DRM > master to catch the common case, where the test is being ran > along with Xorg. Semantic changes to the test though, that implies its needs to be master to run. If you want to impose the constraint that you are the only client active, then add a lib function to check /sys/kernel/debug/dri/$card/client that it only contains one client and that client is you. -Chris
Quoting Katarzyna Dec (2017-08-18 12:08:44)
> CI is observing sporadical failures in pm_rps subtests.
For reference a real bug report looks like:
https://bugs.freedesktop.org/show_bug.cgi?id=102199
which is very much a functional regression that we don't cover.
-Chris
On Fri, Aug 18, 2017 at 02:45:32PM +0100, Chris Wilson wrote: > Quoting Katarzyna Dec (2017-08-18 12:08:44) > > While I'm here let's also make sure that we're running as DRM > > master to catch the common case, where the test is being ran > > along with Xorg. > > Semantic changes to the test though, that implies its needs to be master > to run. > > If you want to impose the constraint that you are the only client > active, then add a lib function to check > /sys/kernel/debug/dri/$card/client that it only contains one client and > that client is you. We should be doing that already, at least if you try to open the drm fd. I guess we could split that out, or maybe wrap it into a helper. -Daniel
Quoting Daniel Vetter (2017-08-18 21:28:08) > On Fri, Aug 18, 2017 at 02:45:32PM +0100, Chris Wilson wrote: > > Quoting Katarzyna Dec (2017-08-18 12:08:44) > > > While I'm here let's also make sure that we're running as DRM > > > master to catch the common case, where the test is being ran > > > along with Xorg. > > > > Semantic changes to the test though, that implies its needs to be master > > to run. > > > > If you want to impose the constraint that you are the only client > > active, then add a lib function to check > > /sys/kernel/debug/dri/$card/client that it only contains one client and > > that client is you. > > We should be doing that already, at least if you try to open the drm fd. I > guess we could split that out, or maybe wrap it into a helper. Only the first opening of the device is expected to be the singular client. We don't do that as part of drm_open_diver(), but it did used to be done for the shellscripts. -Chris
Thanks for comments. Do we need more changes then being drm-master? You wrote something about not being the one client. Is proposed approach for checking boost not good enough? Do you have any suggestions? Kasia
Quoting Dec, Katarzyna (2017-08-21 09:29:47) > Thanks for comments. > Do we need more changes then being drm-master? You wrote something about not being the one client. If you wanted to be complete you would check master, auth and render. They are different classes of fd the driver handles, and rps should work with any. You could say that render is the lowest common denominator and only test with that -- that's a much better argument that testing with master alone. > Is proposed approach for checking boost not good enough? Do you have any suggestions? Why are we checking for boost? We certainly don't wish to imply that if you follow this sequence you will be granted max gpu clocks; that's a policy decision we should not be so eager to be involved in. waitboosting is to solve a particular issue with slow clock ramp up for benchmarks and interactivity, of which interactivity is the much more noticeable. We don't test that, nor do we track the impact it has upon power consumption. Basically the test is following the implementation, and not measuring the behaviour of the system, because that is much harder to get right, and would almost certainly lead to better integration with interactive systems (i.e. so that we could apply gpu boosts ad prioritisation more intelligently than reacting to a stall which is easy to abuse). Reacting well to benchmarks should be handled by rps itself and not need a waitboost on throttling. In fact, I am very tempted to remove waitboosting for the user and only use it for catching up to missed vblanks. Along with the suggestions on how to improve reclocking for particular clients/workloads. (Such changes will break the test, because it is testing what the kernel is doing now, not how we expect the system to perform.) -Chris
I just saw comments for the code (in first patch version) > 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. So we can get rid of gem_wait in this case? >You could set the boost freq > max freq to differentiate What do you mean by that? > /* 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. This assert was changed in v2 to igt_assert(). Will this also fail on older kernels? If yes, why? Thanks, Kasia
Quoting Dec, Katarzyna (2017-08-21 11:43:35) > I just saw comments for the code (in first patch version) > > > 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. > So we can get rid of gem_wait in this case? No. Since the test is all about the wait -> boost scenario, not that high load generates high cocks. > >You could set the boost freq > max freq to differentiate > What do you mean by that? If you set the max freq to less than the boost freq, the only way to get to the boost freq is via the wait, and through the ordinary system load. It is another way to prove that boosting is in effect, and also the independence of the control knobs. > > /* 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. > This assert was changed in v2 to igt_assert(). Will this also fail on older kernels? If yes, why? The field you are looking for is a recent addition to the file. Within reason, we expect igt to be agnostic of kernel version (aiming for forwards compatibility with future kernels for minimum effort, and backwards for maximum coverage). Sometimes we can't write the test we want without relying on a new kernel. -Chris
diff --git a/tests/pm_rps.c b/tests/pm_rps.c index f0455e78..efe938e0 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) @@ -560,29 +561,46 @@ static void reset_gpu(void) close(fd); } +static bool boost_finished(void) +{ + char buf[1024]; + + igt_debugfs_read(drm_fd, "i915_rps_boost_info", buf); + return strstr(buf, "Boosts outstanding? 0"); +} + 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); /* Waiting will grant us a boost to maximum */ gem_wait(fd, load->handle, &timeout); read_freqs(boost_freqs); dump(boost_freqs); + igt_assert(!boost_finished()); + /* Avoid downlocking till boost request is pending */ + igt_spin_batch_end(load); + gem_sync(fd, load->handle); igt_spin_batch_free(fd, load); + } +#define BOOST_WAIT_TIMESTEP_MSEC 250 +#define BOOST_WAIT_TIMEOUT_MSEC 15000 static void waitboost(bool reset) { int pre_freqs[NUMFREQ]; int boost_freqs[NUMFREQ]; int post_freqs[NUMFREQ]; - int fd = drm_open_driver(DRIVER_INTEL); load_helper_run(LOW); @@ -602,6 +620,9 @@ static void waitboost(bool reset) */ boost_freq(fd, boost_freqs); + /* Wait till boost ends */ + igt_assert(igt_wait(boost_finished(), BOOST_WAIT_TIMEOUT_MSEC, BOOST_WAIT_TIMESTEP_MSEC)); + igt_debug("Apply low load again...\n"); sleep(1); stabilize_check(post_freqs); @@ -611,7 +632,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,8 +661,8 @@ 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); igt_require_gem(drm_fd); igt_require(gem_can_store_dword(drm_fd, 0)); @@ -657,7 +678,7 @@ igt_main val = readval(junk->filp); igt_assert(val >= 0); junk++; - } while(junk->name != NULL); + } while (junk->name != NULL); read_freqs(origfreqs);
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. We're also seeing failures with boost frequency failing to drop down before we start measuring on low. While I'm here let's also make sure that we're running as DRM master to catch the common case, where the test is being ran along with Xorg. Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Jeff Mcgee <jeff.mcgee@intel.com> Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com> Signed-off-by: Katarzyna Dec <katarzyna.dec@intel.com> --- tests/pm_rps.c | 39 ++++++++++++++++++++++++++++++--------- 1 file changed, 30 insertions(+), 9 deletions(-)