Quoting Tvrtko Ursulin (2018-02-21 10:39:29) > > On 20/02/2018 13:50, Chris Wilson wrote: > > Convert the busy pwm from using a single calibration pass with a fixed > > target into a self-correcting pwm that tries to adjust how long to sleep > > on each pwm in order to converge at the target busy %%. > > > > Being self-correcting, it should fare better against the more variable > > systems CI presents. > > > > v2: Be fair and equally strict for low/high busy %% > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105157 > > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx> > > --- > > tests/perf_pmu.c | 64 ++++++++++++++++++++++---------------------------------- > > 1 file changed, 25 insertions(+), 39 deletions(-) > > > > diff --git a/tests/perf_pmu.c b/tests/perf_pmu.c > > index 7fab73e2..0beb9197 100644 > > --- a/tests/perf_pmu.c > > +++ b/tests/perf_pmu.c > > @@ -1422,9 +1422,10 @@ accuracy(int gem_fd, const struct intel_execution_engine2 *e, > > busy_us / 100) / target_busy_pct; > > unsigned long pwm_calibration_us; > > unsigned long test_us; > > - double busy_r; > > + double busy_r, expected; > > uint64_t val[2]; > > uint64_t ts[2]; > > + int link[2]; > > int fd; > > > > /* Sampling platforms cannot reach the high accuracy criteria. */ > > @@ -1450,14 +1451,16 @@ accuracy(int gem_fd, const struct intel_execution_engine2 *e, > > assert_within_epsilon((double)busy_us / (busy_us + idle_us), > > (double)target_busy_pct / 100.0, tolerance); > > > > + igt_assert(pipe(link) == 0); > > + > > /* Emit PWM pattern on the engine from a child. */ > > igt_fork(child, 1) { > > struct sched_param rt = { .sched_priority = 99 }; > > - const unsigned long timeout[] = { pwm_calibration_us * 1000, > > - test_us * 2 * 1000 }; > > - unsigned long sleep_busy = busy_us; > > - unsigned long sleep_idle = idle_us; > > + const unsigned long timeout[] = { > > + pwm_calibration_us * 1000, test_us * 2 * 1000 > > + }; > > struct drm_i915_gem_exec_object2 obj = {}; > > + uint64_t busy_ns = 0, idle_ns = 0; > > You deliberately moved this one level up? I find it confusing that the > log message below logs accumulated time from both passes now. Yes. It continues on from the calibration pass metrics for its baseline. > > igt_spin_t *spin; > > int ret; > > > > @@ -1478,76 +1481,59 @@ accuracy(int gem_fd, const struct intel_execution_engine2 *e, > > > > /* 1st pass is calibration, second pass is the test. */ > > for (int pass = 0; pass < ARRAY_SIZE(timeout); pass++) { > > - unsigned long busy_ns = 0, idle_ns = 0; > > struct timespec test_start = { }; > > - unsigned long loops = 0; > > - double err_busy, err_idle; > > > > igt_nsec_elapsed(&test_start); > > do { > > struct timespec t_busy = { }; > > + unsigned int target; > > target_idle_us ? Ok. > > > > igt_nsec_elapsed(&t_busy); > > > > /* Restart the spinbatch. */ > > __rearm_spin_batch(spin); > > __submit_spin_batch(gem_fd, &obj, e); > > - measured_usleep(sleep_busy); > > + measured_usleep(busy_us); > > igt_spin_batch_end(spin); > > gem_sync(gem_fd, obj.handle); > > > > busy_ns += igt_nsec_elapsed(&t_busy); > > > > - idle_ns += measured_usleep(sleep_idle); > > - > > - loops++; > > + target = (100 * busy_ns / target_busy_pct - (busy_ns + idle_ns)) / 1000; > > + idle_ns += measured_usleep(target); > > } while (igt_nsec_elapsed(&test_start) < timeout[pass]); > > > > - busy_ns = div_round_up(busy_ns, loops); > > - idle_ns = div_round_up(idle_ns, loops); > > - > > - err_busy = __error(busy_ns / 1000, busy_us); > > - err_idle = __error(idle_ns / 1000, idle_us); > > - > > - igt_info("%u: busy %lu/%lu %.2f%%, idle %lu/%lu %.2f%%\n", > > - pass, > > - busy_ns / 1000, busy_us, err_busy, > > - idle_ns / 1000, idle_us, err_idle); > > - > > - if (pass == 0) { > > - sleep_busy = (double)busy_us - > > - (double)busy_us * err_busy / 100.0; > > - sleep_idle = (double)idle_us - > > - (double)idle_us * err_idle / 100.0; > > - igt_info("calibrated sleeps ratio %.2f%% (%lu/%lu)\n", > > - (double)sleep_busy / > > - (sleep_busy + sleep_idle) * 100.0, > > - sleep_busy, sleep_idle); > > - } > > + expected = (double)busy_ns / (busy_ns + idle_ns); > > + igt_info("%u: busy %luus, idle %luus: %.2f%% (target: %lu%%)\n", > > + pass, busy_ns / 1000, idle_ns / 1000, > > + 100 * expected, target_busy_pct); > > + write(link[1], &expected, sizeof(expected)); > > } > > > > igt_spin_batch_free(gem_fd, spin); > > } > > > > /* Let the child run. */ > > - usleep(pwm_calibration_us * 2); > > + close(link[1]); > > + read(link[0], &expected, sizeof(expected)); > > I like this. :) Can't think who gave me the eureka moment. ;) > > /* Collect engine busyness for an interesting part of child runtime. */ > > fd = open_pmu(I915_PMU_ENGINE_BUSY(e->class, e->instance)); > > val[0] = __pmu_read_single(fd, &ts[0]); > > - usleep(test_us / 2); > > + read(link[0], &expected, sizeof(expected)); > > val[1] = __pmu_read_single(fd, &ts[1]); > > close(fd); > > + close(link[0]); > > > > igt_waitchildren(); > > > > busy_r = (double)(val[1] - val[0]) / (ts[1] - ts[0]); > > > > - igt_info("error=%.2f%% (%.2f%% vs %lu%%)\n", > > - __error(busy_r, target_busy_pct / 100.0), > > - busy_r * 100.0, target_busy_pct); > > + igt_info("error=%.2f%% (%.2f%% vs %.2f%%)\n", > > + __error(busy_r, expected), 100 * busy_r, 100 * expected); > > > > - assert_within_epsilon(busy_r, (double)target_busy_pct / 100.0, 0.15); > > + assert_within_epsilon(busy_r, expected, 0.15); > > + assert_within_epsilon(1 - busy_r, 1 - expected, 0.15); > > Hm I don't quite understand how this is increasing fairness since the > first assert is the same (ignoring self-correcting bit). Fairness, being equally strict for 2% and 98% targets? I haven't tried it on paper, just the results seemed to show higher skew for 2%. > What I was thinking about a few times is to switch to absolute > tolerance. Just say +/- 2% on the percentage, instead of +/- 15% > relative to the percentage. I agree. > But in general I like these improvements. With a clearer variable name > and not logging accumulated time between passes: > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx> > > Regards, > > Tvrtko > > P.S. > Pity GLK results were not in. Or the kasan runs... They (ivb in particular fares badly) are next on the hit list. -Chris _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
