From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ej1-x62e.google.com (mail-ej1-x62e.google.com [IPv6:2a00:1450:4864:20::62e]) by gabe.freedesktop.org (Postfix) with ESMTPS id EF85A89C33 for ; Tue, 13 Apr 2021 16:19:21 +0000 (UTC) Received: by mail-ej1-x62e.google.com with SMTP id w3so26917746ejc.4 for ; Tue, 13 Apr 2021 09:19:21 -0700 (PDT) MIME-Version: 1.0 References: <20210413035350.261794-1-jason@jlekstrand.net> In-Reply-To: <20210413035350.261794-1-jason@jlekstrand.net> From: Jason Ekstrand Date: Tue, 13 Apr 2021 11:19:08 -0500 Message-ID: Subject: Re: [igt-dev] [PATCH i-g-t 00/74] Stop depending on context mutation List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" To: IGT GPU Tools Cc: Daniel Vetter List-ID: I just rebased on top of the reloc/allocator stuff and sent to trybot again. On Mon, Apr 12, 2021 at 10:53 PM Jason Ekstrand wrote: > > I'm trying to clean up some of our uAPI technical debt in i915. One of the > biggest areas we have right now is context mutability. There's no good > reason why things like the set of engines or the VM should be able to be > changed on the fly and no "real" userspace actually relies on this > functionality. It does, however, make for a good excuse for tests and lots > of bug reports as things like swapping out the set of engines under load > break randomly. The solution here is to stop allowing that behavior and > simplify the i915 internals. > > In particular, we'd like to remove the following from the i915 API: > > 1. I915_CONTEXT_CLONE_*. These are only used by IGT and have never been > used by any "real" userspace. > > 2. Changing the VM or set of engines via SETPARAM after they've been > "used" by an execbuf or similar. This would effectively make those > parameters create params rather than mutable state. We can't drop > setparam entirely for those because media does use it but we can > enforce some rules. > > 3. Unused (by non-IGT userspace) GETPARAM for things like engines. > > As much as we'd love to do that, we have a bit of a problem in IGT. The > way we handle multi-engine testing today relies heavily on this soon-to-be- > deprecated functionality. In particular, the standard flow is usually > something like this: > > static void run_test1(int fd, uint32_t engine) > { > igt_spin_t *spin; > > ctx = = gem_context_clone_with_engines(fd, 0); > __igt_spin_new(fd, ctx, .engine = engine); > > /* do some testing with ctx */ > > igt_spin_free(fd, spin); > gem_destroy_context(fd, ctx); > } > > igt_main > { > struct intel_execution_engine2 *e; > > /* Usual fixture code */ > > __for_each_physical_engine(fd, e) > run_test1(fd, e->flags); > > __for_each_physical_engine(fd, e) > run_test2(fd, e->flags); > } > > Let's walk through what this does: > > 1. __for_each_physical_engine calls intel_init_engine_list() which resets > the set of engines on ctx0 to the full set of engines available as per > the engine query. On older kernels/hardware where we don't have the > engines query, it leaves the set alone. > > 2. intel_init_engine_list() also returns a set of engines for iteration > and __for_each_physical_engine() sets up a for loop to walk the set. > > 3. gem_context_clone_with_engines() creates a new context using > I915_CONTEXT_CONTEXT_CLONE_ENGINES (not used by anything other than > IGT) to ask that the newly created context has the same set of engines > as ctx0. Remember we changed that at the start of loop iteration! > > 4. When the context is passed to __igt_spin_new(), it calls > gem_context_lookup_engine which does a GETPARAM to introspet the set of > engines on the context and figure out the engine class. > > If you've been keeping track, this trivial and extremely common example > uses every single one of these soon-to-be-deprecated APIs even though the > test author may be completely obvious to it. It also means that getting > rid of IGT's use of them is going to require some fairly deep surgery. > > The approach proposed and partially implemented here is to add a new > wrapper struct intel_ctx_t which wraps a GEM context handle as well as the > full set of parameters used to create it, represented by intel_ctx_cfg_t. > We can then use the context anywhere we would regularly use a context, we > just have to do ctx->id. If we want to clone it, we can do so by re-using > the create parameters by calling intel_ctx_create(fd, &old_ctx->cfg); > > Along with the above rework (which got long, sorry) I've got a few other > patches in here which delete tests which exist expressly to test APIs that > are on the chopping block. > > --Jason > > > Cc: Daniel Vetter > > Jason Ekstrand (74): > tests/i915: Drop gem_ctx_ringsize > tests/i915/gem_exec_balancer: Drop the ringsize subtest > tests/i915/gem_exec_endless: Stop setting the ring size > tests/i915/gem_ctx_param: Drop the zeromap subtests > tests/i915: Drop gem_ctx_clone > lib/i915/gem_engine_topology: Expose the __query_engines helper > lib/i915/gem_context: Add gem_context_create_ext helpers > lib: Add an intel_ctx wrapper struct and helpers (v2) > lib/i915/gem_engine_topology: Rework query_engine_list() > lib/i915/gem_engine_topology: Factor out static engine listing > lib/i915/gem_engine_topology: Add an iterator which doesn't munge > contexts > lib/i915/gem_engine_topology: Add an iterator for intel_ctx_t > tests/i915/gem_exec_basic: Convert to intel_ctx_t > lib/igt_spin: Rename igt_spin_factory::ctx to ctx_id > lib/igt_spin: Support intel_ctx_t > tests/i915/gem_exec_fence: Move the engine data into > inter_engine_context > tests/i915/gem_exec_fence: Convert to intel_ctx_t > tests/i915/gem_exec_schedule: Convert to intel_ctx_t > tests/i915/perf_pmu: Convert to intel_ctx_t > tests/i915/gem_exec_nop: Convert to intel_ctx_t > tests/i915/gem_exec_reloc: Convert to intel_ctx_t > tests/i915/gem_busy: Convert to intel_ctx_t > tests/i915/gem_ctx_isolation: Convert to intel_ctx_t > tests/i915/gem_exec_async: Convert to intel_ctx_t > tests/i915/sysfs_clients: Convert to intel_ctx_t > tests/i915/gem_exec_fair: Convert to intel_ctx_t > tests/i915/gem_spin_batch: Convert to intel_ctx_t > tests/i915/gem_exec_store: Convert to intel_ctx_t > tests/amdgpu/amd_prime: Convert to intel_ctx_t > tests/i915/i915_hangman: Convert to intel_ctx_t > tests/i915/gem_ringfill: Convert to intel_ctx_t > tests/prime_busy: Convert to intel_ctx_t > tests/prime_vgem: Convert to intel_ctx_t > tests/gem_exec_whisper: Convert to intel_ctx_t > tests/i915/gem_ctx_exec: Stop cloning contexts in close_race > tests/i915/gem_ctx_exec: Convert to intel_ctx_t > tests/i915/gem_exec_suspend: Convert to intel_ctx_t > tests/i915/gem_sync: Convert to intel_ctx_t > tests/i915/gem_userptr_blits: Convert to intel_ctx_t > tests/i915/gem_wait: Convert to intel_ctx_t > tests/i915/gem_request_retire: Convert to intel_ctx_t > tests/i915/gem_ctx_shared: Convert to intel_ctx_t > tests/i915/gem_ctx_shared: Stop cloning contexts > tests/i915/gem_create: Convert to intel_ctx_t > tests/i915/gem_ctx_switch: Convert to intel_ctx_t > tests/i915/gem_exec_parallel: Convert to intel_ctx_t > tests/i915/gem_exec_latency: Convert to intel_ctx_t > tests/i915/gem_watchdog: Convert to intel_ctx_t > tests/i915/gem_shrink: Convert to intel_ctx_t > tests/i915/gem_exec_params: Convert to intel_ctx_t > tests/i915/gem_exec_gttfill: Convert to intel_ctx_t > tests/i915/gem_exec_capture: Convert to intel_ctx_t > tests/i915/gem_exec_create: Convert to intel_ctx_t > tests/i915/gem_exec_await: Convert to intel_ctx_t > tests/i915/gem_ctx_persistence: Drop the clone subtest > tests/i915/gem_ctx_persistence: Drop the engine replace subtests > tests/i915/gem_ctx_persistence: Convert to intel_ctx_t > tests/i915/module_load: Convert to intel_ctx_t > tests/i915/pm_rc6_residency: Convert to intel_ctx_t > tests/i915/gem_cs_tlb: Convert to intel_ctx_t > tests/core_hotplug: Convert to intel_ctx_t > tests/i915/gem_exec_balancer: Stop cloning engines > tests/i915/gem_exec_balancer: Don't reset engines on a context > tests/i915/gem_exec_balancer: Stop munging ctx0 engines > tests/i915/gem_exec_endless: Stop munging ctx0 engines > lib/i915: Use for_each_physical_ring for submission tests > tests/i915/gem_ctx_engines: Rework execute-one* > tests/i915/gem_ctx_engines: Use better engine iteration > tests/i915/gem_ctx_engines: Drop the idempotent subtest > tests/i915/gem_ctx_create: Convert benchmarks to intel_ctx_t > lib/i915/gem_context: Delete all the context clone/copy stuff > tests/i915/gem_ctx_engines: Delete the libapi subtest > lib/igt_dummyload: Stop supporting ALL_ENGINES without an intel_ctx_t > lib/i915/gem_engine_topology: Delete the old physical engine iterators > > lib/i915/gem_context.c | 206 ++----- > lib/i915/gem_context.h | 19 +- > lib/i915/gem_engine_topology.c | 142 +++-- > lib/i915/gem_engine_topology.h | 29 +- > lib/i915/gem_submission.c | 13 +- > lib/igt_dummyload.c | 13 +- > lib/igt_dummyload.h | 6 +- > lib/igt_gt.c | 2 +- > lib/intel_ctx.c | 177 ++++++ > lib/intel_ctx.h | 112 ++++ > lib/meson.build | 1 + > tests/amdgpu/amd_prime.c | 10 +- > tests/core_hotunplug.c | 6 +- > tests/i915/gem_busy.c | 77 +-- > tests/i915/gem_create.c | 14 +- > tests/i915/gem_cs_tlb.c | 10 +- > tests/i915/gem_ctx_clone.c | 450 --------------- > tests/i915/gem_ctx_create.c | 76 +-- > tests/i915/gem_ctx_engines.c | 239 ++------ > tests/i915/gem_ctx_exec.c | 19 +- > tests/i915/gem_ctx_isolation.c | 112 ++-- > tests/i915/gem_ctx_param.c | 33 -- > tests/i915/gem_ctx_persistence.c | 435 ++++---------- > tests/i915/gem_ctx_ringsize.c | 345 ------------ > tests/i915/gem_ctx_shared.c | 335 ++++++----- > tests/i915/gem_ctx_switch.c | 115 ++-- > tests/i915/gem_eio.c | 2 +- > tests/i915/gem_exec_async.c | 32 +- > tests/i915/gem_exec_await.c | 20 +- > tests/i915/gem_exec_balancer.c | 293 ++++------ > tests/i915/gem_exec_basic.c | 7 +- > tests/i915/gem_exec_capture.c | 30 +- > tests/i915/gem_exec_create.c | 9 +- > tests/i915/gem_exec_endless.c | 14 +- > tests/i915/gem_exec_fair.c | 112 ++-- > tests/i915/gem_exec_fence.c | 302 +++++----- > tests/i915/gem_exec_gttfill.c | 15 +- > tests/i915/gem_exec_latency.c | 118 ++-- > tests/i915/gem_exec_nop.c | 156 ++--- > tests/i915/gem_exec_parallel.c | 29 +- > tests/i915/gem_exec_params.c | 4 +- > tests/i915/gem_exec_reloc.c | 102 ++-- > tests/i915/gem_exec_schedule.c | 876 +++++++++++++++-------------- > tests/i915/gem_exec_store.c | 36 +- > tests/i915/gem_exec_suspend.c | 52 +- > tests/i915/gem_exec_whisper.c | 83 ++- > tests/i915/gem_request_retire.c | 17 +- > tests/i915/gem_ringfill.c | 48 +- > tests/i915/gem_shrink.c | 37 +- > tests/i915/gem_spin_batch.c | 79 +-- > tests/i915/gem_sync.c | 159 +++--- > tests/i915/gem_userptr_blits.c | 25 +- > tests/i915/gem_vm_create.c | 4 +- > tests/i915/gem_wait.c | 20 +- > tests/i915/gem_watchdog.c | 167 ++---- > tests/i915/gem_workarounds.c | 2 +- > tests/i915/i915_hangman.c | 37 +- > tests/i915/i915_module_load.c | 7 +- > tests/i915/i915_pm_rc6_residency.c | 7 +- > tests/i915/perf_pmu.c | 226 ++++---- > tests/i915/sysfs_clients.c | 87 +-- > tests/meson.build | 2 - > tests/prime_busy.c | 19 +- > tests/prime_vgem.c | 35 +- > 64 files changed, 2785 insertions(+), 3481 deletions(-) > create mode 100644 lib/intel_ctx.c > create mode 100644 lib/intel_ctx.h > delete mode 100644 tests/i915/gem_ctx_clone.c > delete mode 100644 tests/i915/gem_ctx_ringsize.c > > -- > 2.31.1 > _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev