[PATCH 00/30] drm/i915/gem: ioctl clean-ups (v9)
Daniel Vetter
daniel at ffwll.ch
Thu Jul 8 17:55:19 UTC 2021
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 at async-destroy and gem_vm_create at destroy-race which test
> swapping out the VM on a running context.
>
> - gem_ctx_persistence at 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 at 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 at jlekstrand.net
>
> 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
More information about the dri-devel
mailing list