[igt-dev] [PATCH i-g-t 6/9] tests/perf_pmu: PMU enable race test

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-02-02 18:37:51)
> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> 
> Test that the PMU can be safely enabled in face of interrupt-heavy load on
> an engine.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
>  tests/perf_pmu.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
> 
> diff --git a/tests/perf_pmu.c b/tests/perf_pmu.c
> index 27f32b8a1602..de0a1c7d936c 100644
> --- a/tests/perf_pmu.c
> +++ b/tests/perf_pmu.c
> @@ -1221,6 +1221,51 @@ test_rc6(int gem_fd)
>         assert_within_epsilon(busy - prev, 0.0, tolerance);
>  }
>  
> +static void
> +test_enable_race(int gem_fd, const struct intel_execution_engine2 *e)
> +{
> +       uint64_t config = I915_PMU_ENGINE_BUSY(e->class, e->instance);
> +       struct igt_helper_process engine_load = { };
> +       const uint32_t bbend = MI_BATCH_BUFFER_END;
> +       struct drm_i915_gem_exec_object2 obj = { };
> +       struct drm_i915_gem_execbuffer2 eb = { };
> +       int fd;
> +
> +       igt_require(intel_gen(intel_get_drm_devid(gem_fd)) >= 8);

	igt_require(gem_has_execlists(gem_fd));

may be clearer, it's still a proxy for has_stats. :|

> +       igt_require(gem_has_engine(gem_fd, e->class, e->instance));
> +
> +       obj.handle = gem_create(gem_fd, 4096);
> +       gem_write(gem_fd, obj.handle, 0, &bbend, sizeof(bbend));
> +
> +       eb.buffer_count = 1;
> +       eb.buffers_ptr = to_user_pointer(&obj);
> +       eb.flags = e2ring(gem_fd, e);
> +
> +       /*
> +        * Defeat the busy stats delayed disable, we need to guarantee we are
> +        * the first user.
> +        */

gem_quiescent_gpu(); as well?

> +       sleep(2);
> +
> +       /* Apply interrupt-heavy load on the engine. */

s/load/nop load/ ?

> +       igt_fork_helper(&engine_load) {
> +               for (;;)
> +                       gem_execbuf(gem_fd, &eb);
> +       }
> +
> +       /* Wait a bit to allow engine load to start. */
> +       usleep(500e3);

.5s, I guess we aren't too concerned with paring it down to the
minimum here as we've already spent 2s for idling.

> +
> +       /* Enable the PMU. */
> +       fd = open_pmu(config);

fd = -1;
igt_until_timeout(1) {
	close(fd);
	fd = open_pmu(config);
}

It's a race, so don't expect it to happen first time?

> +       /* Cleanup. */
> +       igt_stop_helper(&engine_load);
> +       close(fd);

Another race, right? Wrap it all in a 10x loop? Or separate out the
close race to another lop.

> +       gem_close(gem_fd, obj.handle);
> +       gem_quiescent_gpu(gem_fd);
> +}

The gist of the test looks fine, so please consider exercising the race
more than once and with that have a
Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
-Chris