On Wed, 2017-09-27 at 17:44 +0100, Chris Wilson wrote:
> Add another perma-pinned context for using for preemption at any time.
> We cannot just reuse the existing kernel context, as first and foremost
> we need to ensure that we can preempt the kernel context itself, so
> require a distinct context id. Similar to the kernel context, we may
> want to interrupt execution and switch to the preempt context at any
> time, and so it needs to be permanently pinned and available.
>
> To compensate for yet another permanent allocation, we shrink the
> existing context and the new context by reducing their ringbuffer to the
> minimum.
>
> v2: Assert that we never allocate a request from the preemption context.
> v3: Limit perma-pin to engines that may preempt.
>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Reviewed-by: Michał Winiarski <michal.winiarski@xxxxxxxxx>
<SNIP>
> @@ -2250,8 +2251,9 @@ struct drm_i915_private {
> wait_queue_head_t gmbus_wait_queue;
>
> struct pci_dev *bridge_dev;
> - struct i915_gem_context *kernel_context;
> struct intel_engine_cs *engine[I915_NUM_ENGINES];
First to touch old code gets to add kerneldoc, just formulate what's in
the commit message into here.
> + struct i915_gem_context *kernel_context;
> + struct i915_gem_context *preempt_context;
> struct i915_vma *semaphore;
>
> struct drm_dma_handle *status_page_dmah;
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 921ee369c74d..1518e110fd04 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -416,14 +416,29 @@ i915_gem_context_create_gvt(struct drm_device *dev)
> return ctx;
> }
>
> +static struct i915_gem_context *
> +create_kernel_context(struct drm_i915_private *i915, int prio)
I may vote for 'create_internal_context' because 'kernel context' is
quite an established term. But I've got no hard option, just gotta keep
the terminology coherent (eg. in the above requested kerneldoc).
> +{
> + struct i915_gem_context *ctx;
> +
> + ctx = i915_gem_create_context(i915, NULL);
> + if (IS_ERR(ctx))
> + return ctx;
> +
> + i915_gem_context_clear_bannable(ctx);
> + ctx->priority = prio;
> + ctx->ring_size = PAGE_SIZE;
> +
> + return ctx;
> +}
> +
> int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
> {
> struct i915_gem_context *ctx;
>
> /* Init should only be called once per module load. Eventually the
> * restriction on the context_disabled check can be loosened. */
Commit seems to be out of date now?
> @@ -441,23 +456,30 @@ int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
> BUILD_BUG_ON(MAX_CONTEXT_HW_ID > INT_MAX);
> ida_init(&dev_priv->contexts.hw_ida);
>
> - ctx = i915_gem_create_context(dev_priv, NULL);
> + /* lowest priority; idle task */
> + ctx = create_kernel_context(dev_priv, I915_PRIORITY_MIN);
> if (IS_ERR(ctx)) {
> DRM_ERROR("Failed to create default global context (error %ld)\n",
> PTR_ERR(ctx));
> return PTR_ERR(ctx);
> }
>
> - /* For easy recognisablity, we want the kernel context to be 0 and then
> + /*
> + * For easy recognisablity, we want the kernel context to be 0 and then
> * all user contexts will have non-zero hw_id.
> */
> GEM_BUG_ON(ctx->hw_id);
> -
> - i915_gem_context_clear_bannable(ctx);
> - ctx->priority = I915_PRIORITY_MIN; /* lowest priority; idle task */
> + GEM_BUG_ON(!i915_gem_context_is_kernel(ctx));
> dev_priv->kernel_context = ctx;
>
> - GEM_BUG_ON(!i915_gem_context_is_kernel(ctx));
> + /* highest priority; preempting task */
> + ctx = create_kernel_context(dev_priv, INT_MAX);
> + if (IS_ERR(ctx)) {
> + DRM_ERROR("Failed to create default preempt context (error %ld)\n",
> + PTR_ERR(ctx));
There's no onion teardown so kernel_context is not freed. Pleas add
> +++ b/drivers/gpu/drm/i915/i915_gem_request.c
> @@ -587,6 +587,13 @@ i915_gem_request_alloc(struct intel_engine_cs *engine,
>
> lockdep_assert_held(&dev_priv->drm.struct_mutex);
>
> + /*
> + * Preempt contexts are reserved for exclusive use to inject a
> + * preemption context switch. They are never to be used for any trivial
> + * request!
> + */
> + GEM_BUG_ON(ctx == dev_priv->preempt_context);
Maybe check the prio here too.
> +
> /* ABI: Before userspace accesses the GPU (e.g. execbuffer), report
> * EIO if the GPU is already wedged.
> */
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 21e037fc21b7..02eb25ed1064 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -613,9 +613,22 @@ int intel_engine_init_common(struct intel_engine_cs *engine)
> if (IS_ERR(ring))
> return PTR_ERR(ring);
>
> + /*
> + * Similarly the preempt context must always be available so that
> + * we can interrupt the engine at any time.
> + */
> + if (INTEL_INFO(engine->i915)->has_logical_ring_preemption) {
> + ring = engine->context_pin(engine,
> + engine->i915->preempt_context);
> + if (IS_ERR(ring)) {
> + ret = PTR_ERR(ring);
> + goto err_unpin_kernel;
> + }
> + }
Maybe add helper for the internal/kernel_context_pin and _free too,
it's unnecessary code duplication.
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
