From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by gabe.freedesktop.org (Postfix) with ESMTPS id 94B596ECE4 for ; Fri, 16 Apr 2021 21:03:39 +0000 (UTC) From: "Ruhl, Michael J" Date: Fri, 16 Apr 2021 21:03:37 +0000 Message-ID: <42c89caa5cbd4d4cab63c2a5c4c09c87@intel.com> References: <20210415191145.2137858-1-jason@jlekstrand.net> In-Reply-To: <20210415191145.2137858-1-jason@jlekstrand.net> Content-Language: en-US MIME-Version: 1.0 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: Jason Ekstrand , "igt-dev@lists.freedesktop.org" Cc: Daniel Vetter List-ID: >-----Original Message----- >From: igt-dev On Behalf Of Jason >Ekstrand >Sent: Thursday, April 15, 2021 3:11 PM >To: igt-dev@lists.freedesktop.org >Cc: Daniel Vetter >Subject: [igt-dev] [PATCH i-g-t 00/74] Stop depending on context mutation > >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 I think you mean s/obvious/oblivious/ ? M >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 _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev