On Thu, Jul 08, 2021 at 10:48:05AM -0500, Jason Ekstrand wrote: > Overview: > --------- > > This patch series attempts to clean up some of the IOCTL mess we've created > over the last few years. The most egregious bit being context mutability. > In summary, this series: > > 1. Drops two never-used context params: RINGSIZE and NO_ZEROMAP > 2. Drops the entire CONTEXT_CLONE API > 3. Implements SINGLE_TIMELINE with a syncobj instead of actually sharing > intel_timeline between engines. > 4. Adds a few sanity restrictions to the balancing/bonding API. > 5. Implements a proto-ctx mechanism so that the engine set and VM can only > be set early on in the lifetime of a context, before anything ever > executes on it. This effectively makes the VM and engine set > immutable. > > This series has been tested with IGT as well as the Iris, ANV, and the > Intel media driver doing an 8K decode (this uses bonding/balancing). I've > also done quite a bit of git archeology to ensure that nothing in here will > break anything that's already shipped at some point in history. It's > possible I've missed something, but I've dug quite a bit. > > > Details and motivation: > ----------------------- > > In very broad strokes, there's an effort going on right now within Intel to > try and clean up and simplify i915 anywhere we can. We obviously don't > want to break any shipping userspace but, as can be seen by this series, > there's a lot i915 theoretically supports which userspace doesn't actually > need. Some of this, like the two context params used here, were simply > oversights where we went through the usual API review process and merged > the i915 bits but the userspace bits never landed for some reason. > > Not all are so innocent, however. For instance, there's an entire context > cloning API which allows one to create a context with certain parameters > "cloned" from some other context. This entire API has never been used by > any userspace except IGT and there were never patches to any other > userspace to use it. It never should have landed. Also, when we added > support for setting explicit engine sets and sharing VMs across contexts, > people decided to do so via SET_CONTEXT_PARAM. While this allowed them to > re-use existing API, it did so at the cost of making those states mutable > which leads to a plethora of potential race conditions. There were even > IGT tests merged to cover some of theses: > > - gem_vm_create@async-destroy and gem_vm_create@destroy-race which test > swapping out the VM on a running context. > > - gem_ctx_persistence@replace* which test whether a client can escape a > non-persistent context by submitting a hanging batch and then swapping > out the engine set before the hang is detected. > > - api_intel_bb@bb-with-vm which tests the that intel_bb_assign_vm works > properly. This API is never used by any other IGT test. > > There is also an entire deferred flush and set state framework in > i915_gem_cotnext.c which exists for safely swapping out the VM while there > is work in-flight on a context. > > So, clearly people knew that this API was inherently racy and difficult to > implement but they landed it anyway. Why? The best explanation I've been > given is because it makes the API more "unified" or "symmetric" for this > stuff to go through SET_CONTEXT_PARAM. It's not because any userspace > actually wants to be able to swap out the VM or the set of engines on a > running context. That would be utterly insane. > > This patch series cleans up this particular mess by introducing the concept > of a i915_gem_proto_context data structure which contains context creation > information. When you initially call GEM_CONTEXT_CREATE, a proto-context > in created instead of an actual context. Then, the first time something is > done on the context besides SET_CONTEXT_PARAM, an actual context is > created. This allows us to keep the old drivers which use > SET_CONTEXT_PARAM to set up the engine set (see also media) while ensuring > that, once you have an i915_gem_context, the VM and the engine set are > immutable state. > > Eventually, there are more clean-ups I'd like to do on top of this which > should make working with contexts inside i915 simpler and safer: > > 1. Move the GEM handle -> vma LUT from i915_gem_context into either > i915_ppgtt or drm_i915_file_private depending on whether or not the > hardware has a full PPGTT. > > 2. Move the delayed context destruction code into intel_context or a > per-engine wrapper struct rather than i915_gem_context. > > 3. Get rid of the separation between context close and context destroy > > 4. Get rid of the RCU on i915_gem_context > > However, these should probably be done as a separate patch series as this > one is already starting to get longish, especially if you consider the 89 > IGT patches that go along with it. > > Test-with: 20210707210215.351483-1-jason@xxxxxxxxxxxxxx > > Jason Ekstrand (30): > drm/i915: Drop I915_CONTEXT_PARAM_RINGSIZE > drm/i915: Stop storing the ring size in the ring pointer (v3) > drm/i915: Drop I915_CONTEXT_PARAM_NO_ZEROMAP > drm/i915/gem: Set the watchdog timeout directly in > intel_context_set_gem (v2) > drm/i915/gem: Return void from context_apply_all > drm/i915: Drop the CONTEXT_CLONE API (v2) > drm/i915: Implement SINGLE_TIMELINE with a syncobj (v4) > drm/i915: Drop getparam support for I915_CONTEXT_PARAM_ENGINES > drm/i915/gem: Disallow bonding of virtual engines (v3) > drm/i915/gem: Remove engine auto-magic with FENCE_SUBMIT (v2) > drm/i915/request: Remove the hook from await_execution > drm/i915/gem: Disallow creating contexts with too many engines > drm/i915: Stop manually RCU banging in reset_stats_ioctl (v2) > drm/i915/gem: Add a separate validate_priority helper > drm/i915: Add gem/i915_gem_context.h to the docs > drm/i915/gem: Add an intermediate proto_context struct (v5) > drm/i915/gem: Rework error handling in default_engines > drm/i915/gem: Optionally set SSEU in intel_context_set_gem > drm/i915: Add an i915_gem_vm_lookup helper > drm/i915/gem: Make an alignment check more sensible > drm/i915/gem: Use the proto-context to handle create parameters (v5) > drm/i915/gem: Return an error ptr from context_lookup > drm/i915/gt: Drop i915_address_space::file (v2) > drm/i915/gem: Delay context creation (v3) > drm/i915/gem: Don't allow changing the VM on running contexts (v4) > drm/i915/gem: Don't allow changing the engine set on running contexts > (v3) > drm/i915/selftests: Take a VM in kernel_context() > i915/gem/selftests: Assign the VM at context creation in > igt_shared_ctx_exec > drm/i915/gem: Roll all of context creation together > drm/i915: Finalize contexts in GEM_CONTEXT_CREATE on version 13+ Applied everything, thanks a lot. Now, could we bake all this hard-earned knowledge about how the uapi actually works into some kerneldoc for the uapi header? Thanks, Daniel > > Documentation/gpu/i915.rst | 2 + > drivers/gpu/drm/i915/Makefile | 1 - > drivers/gpu/drm/i915/gem/i915_gem_context.c | 2926 ++++++++--------- > drivers/gpu/drm/i915/gem/i915_gem_context.h | 3 + > .../gpu/drm/i915/gem/i915_gem_context_types.h | 196 +- > .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 31 +- > .../drm/i915/gem/selftests/i915_gem_context.c | 127 +- > .../gpu/drm/i915/gem/selftests/mock_context.c | 67 +- > .../gpu/drm/i915/gem/selftests/mock_context.h | 4 +- > drivers/gpu/drm/i915/gt/intel_context.c | 3 +- > drivers/gpu/drm/i915/gt/intel_context.h | 5 - > drivers/gpu/drm/i915/gt/intel_context_param.c | 63 - > drivers/gpu/drm/i915/gt/intel_context_param.h | 6 +- > drivers/gpu/drm/i915/gt/intel_context_types.h | 1 + > drivers/gpu/drm/i915/gt/intel_engine_cs.c | 3 +- > drivers/gpu/drm/i915/gt/intel_engine_types.h | 7 - > .../drm/i915/gt/intel_execlists_submission.c | 114 - > .../drm/i915/gt/intel_execlists_submission.h | 8 +- > drivers/gpu/drm/i915/gt/intel_gtt.h | 11 - > drivers/gpu/drm/i915/gt/intel_lrc.c | 2 +- > drivers/gpu/drm/i915/gt/intel_migrate.c | 3 +- > drivers/gpu/drm/i915/gt/selftest_execlists.c | 251 +- > drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 2 +- > drivers/gpu/drm/i915/gt/selftest_mocs.c | 2 +- > drivers/gpu/drm/i915/gt/selftest_timeline.c | 2 +- > drivers/gpu/drm/i915/gvt/scheduler.c | 7 +- > drivers/gpu/drm/i915/i915_drv.h | 82 +- > drivers/gpu/drm/i915/i915_perf.c | 4 +- > drivers/gpu/drm/i915/i915_request.c | 42 +- > drivers/gpu/drm/i915/i915_request.h | 4 +- > .../drm/i915/selftests/i915_mock_selftests.h | 1 - > drivers/gpu/drm/i915/selftests/mock_gtt.c | 1 - > include/uapi/drm/i915_drm.h | 40 +- > 33 files changed, 1681 insertions(+), 2340 deletions(-) > delete mode 100644 drivers/gpu/drm/i915/gt/intel_context_param.c > > -- > 2.31.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
