[igt-dev] [PATCH i-g-t 41/77] tests/i915/gem_ctx_persistence: Drop the engine replace subtests
Zbigniew Kempczyński
zbigniew.kempczynski at intel.com
Tue Jun 15 07:45:50 UTC 2021
On Mon, Jun 14, 2021 at 11:36:56AM -0500, Jason Ekstrand wrote:
> We're going to start disallowing non-trivial uses of setparam for
> engines precisely to make races like this impossible. It'll also make
> these test cases invalid.
>
> Signed-off-by: Jason Ekstrand <jason at jlekstrand.net>
> ---
> tests/i915/gem_ctx_persistence.c | 210 -------------------------------
> 1 file changed, 210 deletions(-)
>
> diff --git a/tests/i915/gem_ctx_persistence.c b/tests/i915/gem_ctx_persistence.c
> index 91c301769..59077cc01 100644
> --- a/tests/i915/gem_ctx_persistence.c
> +++ b/tests/i915/gem_ctx_persistence.c
> @@ -1085,191 +1085,6 @@ static void many_contexts(int i915)
> gem_quiescent_gpu(i915);
> }
>
> -static void replace_engines(int i915, const struct intel_execution_engine2 *e)
> -{
> - I915_DEFINE_CONTEXT_PARAM_ENGINES(engines, 1) = {
> - .engines = {{ e->class, e->instance }}
> - };
> - struct drm_i915_gem_context_param param = {
> - .ctx_id = gem_context_create(i915),
> - .param = I915_CONTEXT_PARAM_ENGINES,
> - .value = to_user_pointer(&engines),
> - .size = sizeof(engines),
> - };
> - igt_spin_t *spin[2];
> - int64_t timeout;
> -
> - /*
> - * Suppose the user tries to hide a hanging batch by replacing
> - * the set of engines on the context so that it's not visible
> - * at the time of closure? Then we must act when they replace
> - * the engines!
> - */
> -
> - gem_context_set_persistence(i915, param.ctx_id, false);
> -
> - gem_context_set_param(i915, ¶m);
> - spin[0] = igt_spin_new(i915, param.ctx_id);
> -
> - gem_context_set_param(i915, ¶m);
> - spin[1] = igt_spin_new(i915, param.ctx_id);
> -
> - gem_context_destroy(i915, param.ctx_id);
> -
> - timeout = reset_timeout_ms * NSEC_PER_MSEC;
> - igt_assert_eq(gem_wait(i915, spin[1]->handle, &timeout), 0);
> -
> - timeout = reset_timeout_ms * NSEC_PER_MSEC;
> - igt_assert_eq(gem_wait(i915, spin[0]->handle, &timeout), 0);
> -
> - igt_spin_free(i915, spin[1]);
> - igt_spin_free(i915, spin[0]);
> - gem_quiescent_gpu(i915);
> -}
> -
> -static void race_set_engines(int i915, int in, int out)
> -{
> - I915_DEFINE_CONTEXT_PARAM_ENGINES(engines, 1) = {
> - .engines = {}
> - };
> - struct drm_i915_gem_context_param param = {
> - .param = I915_CONTEXT_PARAM_ENGINES,
> - .value = to_user_pointer(&engines),
> - .size = sizeof(engines),
> - };
> - igt_spin_t *spin;
> -
> - spin = igt_spin_new(i915);
> - igt_spin_end(spin);
> -
> - while (read(in, ¶m.ctx_id, sizeof(param.ctx_id)) > 0) {
> - if (!param.ctx_id)
> - break;
> -
> - __gem_context_set_param(i915, ¶m);
> -
> - spin->execbuf.rsvd1 = param.ctx_id;
> - __gem_execbuf(i915, &spin->execbuf);
> -
> - write(out, ¶m.ctx_id, sizeof(param.ctx_id));
> - }
> -
> - igt_spin_free(i915, spin);
> -}
> -
> -static void close_replace_race(int i915)
> -{
> - const int ncpus = sysconf(_SC_NPROCESSORS_ONLN);
> - int fence = -1;
> - int out[2], in[2];
> -
> - cleanup(i915);
> -
> - /*
> - * If we time the submission of a hanging batch to one set of engines
> - * and then simultaneously replace the engines in one thread, and
> - * close the context in another, it might be possible for the kernel
> - * to lose track of the old engines believing that the non-persisten
> - * context is already closed and the hanging requests cancelled.
> - *
> - * Our challenge is try and expose any such race condition.
> - */
> -
> - igt_assert(pipe(out) == 0);
> - igt_assert(pipe(in) == 0);
> - igt_fork(child, ncpus) {
> - close(out[1]);
> - close(in[0]);
> - race_set_engines(i915, out[0], in[1]);
> - }
> - for (int i = 0; i < ncpus; i++)
> - close(out[0]);
> -
> - igt_until_timeout(5) {
> - igt_spin_t *spin;
> - uint32_t ctx;
> -
> - ctx = gem_context_clone_with_engines(i915, 0);
> - gem_context_set_persistence(i915, ctx, false);
> -
> - spin = igt_spin_new(i915, ctx, .flags = IGT_SPIN_FENCE_OUT);
> - for (int i = 0; i < ncpus; i++)
> - write(out[1], &ctx, sizeof(ctx));
> -
> - gem_context_destroy(i915, ctx);
> - for (int i = 0; i < ncpus; i++)
> - read(in[0], &ctx, sizeof(ctx));
> -
> - if (fence < 0) {
> - fence = spin->out_fence;
> - } else {
> - int tmp;
> -
> - tmp = sync_fence_merge(fence, spin->out_fence);
> - close(fence);
> - close(spin->out_fence);
> -
> - fence = tmp;
> - }
> - spin->out_fence = -1;
> - }
> - close(in[0]);
> -
> - for (int i = 0; i < ncpus; i++) {
> - uint32_t end = 0;
> -
> - write(out[1], &end, sizeof(end));
> - }
> - close(out[1]);
> -
> - if (sync_fence_wait(fence, MSEC_PER_SEC / 2)) {
> - igt_debugfs_dump(i915, "i915_engine_info");
> - igt_assert(sync_fence_wait(fence, MSEC_PER_SEC / 2) == 0);
> - }
> - close(fence);
> -
> - igt_waitchildren();
> - gem_quiescent_gpu(i915);
> -}
> -
> -static void replace_engines_hostile(int i915,
> - const struct intel_execution_engine2 *e)
> -{
> - I915_DEFINE_CONTEXT_PARAM_ENGINES(engines, 1) = {
> - .engines = {{ e->class, e->instance }}
> - };
> - struct drm_i915_gem_context_param param = {
> - .ctx_id = gem_context_create(i915),
> - .param = I915_CONTEXT_PARAM_ENGINES,
> - .value = to_user_pointer(&engines),
> - .size = sizeof(engines),
> - };
> - int64_t timeout = reset_timeout_ms * NSEC_PER_MSEC;
> - igt_spin_t *spin;
> -
> - /*
> - * Suppose the user tries to hide a hanging batch by replacing
> - * the set of engines on the context so that it's not visible
> - * at the time of closure? Then we must act when they replace
> - * the engines!
> - */
> -
> - gem_context_set_persistence(i915, param.ctx_id, false);
> -
> - gem_context_set_param(i915, ¶m);
> - spin = igt_spin_new(i915, param.ctx_id,
> - .flags = IGT_SPIN_NO_PREEMPTION);
> -
> - param.size = 8;
> - gem_context_set_param(i915, ¶m);
> - gem_context_destroy(i915, param.ctx_id);
> -
> - igt_assert_eq(gem_wait(i915, spin->handle, &timeout), 0);
> -
> - igt_spin_free(i915, spin);
> - gem_quiescent_gpu(i915);
> -}
> -
> static void do_test(void (*test)(int i915, unsigned int engine),
> int i915, unsigned int engine,
> const char *name)
> @@ -1422,31 +1237,6 @@ igt_main
> smoketest(i915);
> }
>
> - /* Check interactions with set-engines */
> - igt_subtest_group {
> - const struct intel_execution_engine2 *e;
> -
> - igt_fixture
> - gem_require_contexts(i915);
> -
> - igt_subtest_with_dynamic("replace") {
> - __for_each_physical_engine(i915, e) {
> - igt_dynamic_f("%s", e->name)
> - replace_engines(i915, e);
> - }
> - }
> -
> - igt_subtest_with_dynamic("replace-hostile") {
> - __for_each_physical_engine(i915, e) {
> - igt_dynamic_f("%s", e->name)
> - replace_engines_hostile(i915, e);
> - }
> - }
> -
> - igt_subtest("close-replace-race")
> - close_replace_race(i915);
> - }
> -
> igt_fixture {
> close(i915);
> }
With upcoming set_param(engines) blocked:
Reviewed-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
--
Zbigniew
> --
> 2.31.1
>
> _______________________________________________
> igt-dev mailing list
> igt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
More information about the igt-dev
mailing list