Chris Wilson <ch...@chris-wilson.co.uk> writes:
> If we remove some hardcoded assumptions about the preempt context having
> a fixed id, reserved from use by normal user contexts, we may only
> allocate the i915_gem_context when required. Then the subsequent
> decisions on using preemption reduce to having the preempt context
> available.
>
> Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
> Cc: Michal Winiarski <michal.winiar...@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursu...@intel.com>
> Cc: Arkadiusz Hiler <arkadiusz.hi...@intel.com>
> Cc: Mika Kuoppala <mika.kuopp...@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospu...@intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem_context.c | 29
> ++++++++++++------------
> drivers/gpu/drm/i915/intel_engine_cs.c | 6 ++---
> drivers/gpu/drm/i915/intel_guc_submission.c | 24 +++++++++++---------
> drivers/gpu/drm/i915/intel_lrc.c | 17 +++++++++-----
> drivers/gpu/drm/i915/intel_ringbuffer.h | 5 ++++
> drivers/gpu/drm/i915/selftests/mock_gem_device.c | 6 -----
> 6 files changed, 46 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
> b/drivers/gpu/drm/i915/i915_gem_context.c
> index 648e7536ff51..d69c8f1a4e78 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -449,10 +449,14 @@ destroy_kernel_context(struct i915_gem_context **ctxp)
> i915_gem_context_free(ctx);
> }
>
> +static bool needs_preempt_context(struct drm_i915_private *i915)
> +{
> + return HAS_LOGICAL_RING_PREEMPTION(i915);
> +}
> +
> int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
> {
> struct i915_gem_context *ctx;
> - int err;
>
> GEM_BUG_ON(dev_priv->kernel_context);
Please consider adding
GEM_BUG_ON(dev_priv->preempt_context);
here even tho the kernel_context should act as a master guard for init ordering
errors. Even if no other than documenting that we expect a clean slate
in this regard also.
>
> @@ -468,8 +472,7 @@ int i915_gem_contexts_init(struct drm_i915_private
> *dev_priv)
> ctx = i915_gem_context_create_kernel(dev_priv, I915_PRIORITY_MIN);
> if (IS_ERR(ctx)) {
> DRM_ERROR("Failed to create default global context\n");
> - err = PTR_ERR(ctx);
> - goto err;
> + return PTR_ERR(ctx);
> }
> /*
> * For easy recognisablity, we want the kernel context to be 0 and then
> @@ -479,23 +482,18 @@ int i915_gem_contexts_init(struct drm_i915_private
> *dev_priv)
> dev_priv->kernel_context = ctx;
>
> /* highest priority; preempting task */
> - ctx = i915_gem_context_create_kernel(dev_priv, INT_MAX);
> - if (IS_ERR(ctx)) {
> - DRM_ERROR("Failed to create default preempt context\n");
> - err = PTR_ERR(ctx);
> - goto err_kernel_context;
It seems this error path has been wrong all along.
> + if (needs_preempt_context(dev_priv)) {
> + ctx = i915_gem_context_create_kernel(dev_priv, INT_MAX);
> + if (!IS_ERR(ctx))
> + dev_priv->preempt_context = ctx;
> + else
> + DRM_ERROR("Failed to create preempt context; disabling
> preemption\n");
> }
> - dev_priv->preempt_context = ctx;
>
> DRM_DEBUG_DRIVER("%s context support initialized\n",
> dev_priv->engine[RCS]->context_size ? "logical" :
> "fake");
> return 0;
> -
> -err_kernel_context:
> - destroy_kernel_context(&dev_priv->kernel_context);
> -err:
> - return err;
> }
>
> void i915_gem_contexts_lost(struct drm_i915_private *dev_priv)
> @@ -521,7 +519,8 @@ void i915_gem_contexts_fini(struct drm_i915_private *i915)
> {
> lockdep_assert_held(&i915->drm.struct_mutex);
>
> - destroy_kernel_context(&i915->preempt_context);
> + if (i915->preempt_context)
> + destroy_kernel_context(&i915->preempt_context);
> destroy_kernel_context(&i915->kernel_context);
>
> /* Must free all deferred contexts (via flush_workqueue) first */
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c
> b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 7eebfbb95e89..bf634432c9c6 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -631,7 +631,7 @@ int intel_engine_init_common(struct intel_engine_cs
> *engine)
> * Similarly the preempt context must always be available so that
> * we can interrupt the engine at any time.
> */
> - if (HAS_LOGICAL_RING_PREEMPTION(engine->i915)) {
> + if (engine->i915->preempt_context) {
> ring = engine->context_pin(engine,
> engine->i915->preempt_context);
> if (IS_ERR(ring)) {
> @@ -656,7 +656,7 @@ int intel_engine_init_common(struct intel_engine_cs
> *engine)
> err_breadcrumbs:
> intel_engine_fini_breadcrumbs(engine);
> err_unpin_preempt:
> - if (HAS_LOGICAL_RING_PREEMPTION(engine->i915))
> + if (engine->i915->preempt_context)
> engine->context_unpin(engine, engine->i915->preempt_context);
> err_unpin_kernel:
> engine->context_unpin(engine, engine->i915->kernel_context);
> @@ -686,7 +686,7 @@ void intel_engine_cleanup_common(struct intel_engine_cs
> *engine)
> if (engine->default_state)
> i915_gem_object_put(engine->default_state);
>
> - if (HAS_LOGICAL_RING_PREEMPTION(engine->i915))
> + if (engine->i915->preempt_context)
> engine->context_unpin(engine, engine->i915->preempt_context);
> engine->context_unpin(engine, engine->i915->kernel_context);
> }
> diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c
> b/drivers/gpu/drm/i915/intel_guc_submission.c
> index 4ea65df05e02..b43b58cc599b 100644
> --- a/drivers/gpu/drm/i915/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/intel_guc_submission.c
> @@ -688,7 +688,7 @@ static void guc_dequeue(struct intel_engine_cs *engine)
> goto unlock;
>
> if (port_isset(port)) {
> - if (HAS_LOGICAL_RING_PREEMPTION(engine->i915)) {
> + if (engine->i915->preempt_context) {
> struct guc_preempt_work *preempt_work =
> &engine->i915->guc.preempt_work[engine->id];
>
> @@ -984,17 +984,19 @@ static int guc_clients_create(struct intel_guc *guc)
> }
> guc->execbuf_client = client;
>
> - client = guc_client_alloc(dev_priv,
> - INTEL_INFO(dev_priv)->ring_mask,
> - GUC_CLIENT_PRIORITY_KMD_HIGH,
> - dev_priv->preempt_context);
> - if (IS_ERR(client)) {
> - DRM_ERROR("Failed to create GuC client for preemption!\n");
> - guc_client_free(guc->execbuf_client);
> - guc->execbuf_client = NULL;
> - return PTR_ERR(client);
> + if (dev_priv->preempt_context) {
> + client = guc_client_alloc(dev_priv,
> + INTEL_INFO(dev_priv)->ring_mask,
> + GUC_CLIENT_PRIORITY_KMD_HIGH,
> + dev_priv->preempt_context);
> + if (IS_ERR(client)) {
> + DRM_ERROR("Failed to create GuC client for
> preemption!\n");
> + guc_client_free(guc->execbuf_client);
> + guc->execbuf_client = NULL;
> + return PTR_ERR(client);
> + }
> + guc->preempt_client = client;
> }
> - guc->preempt_client = client;
>
> return 0;
> }
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c
> b/drivers/gpu/drm/i915/intel_lrc.c
> index 5390894001f0..221b62d72c72 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -161,7 +161,6 @@
> #define EXECLISTS_REQUEST_SIZE 64 /* bytes */
> #define WA_TAIL_DWORDS 2
> #define WA_TAIL_BYTES (sizeof(u32) * WA_TAIL_DWORDS)
> -#define PREEMPT_ID 0x1
>
> static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
> struct intel_engine_cs *engine);
> @@ -448,7 +447,8 @@ static void inject_preempt_context(struct intel_engine_cs
> *engine)
> &engine->i915->preempt_context->engine[engine->id];
> unsigned int n;
>
> - GEM_BUG_ON(engine->i915->preempt_context->hw_id != PREEMPT_ID);
> + GEM_BUG_ON(engine->execlists.preempt_complete_status !=
> + upper_32_bits(ce->lrc_desc));
> GEM_BUG_ON(!IS_ALIGNED(ce->ring->size, WA_TAIL_BYTES));
>
> memset(ce->ring->vaddr + ce->ring->tail, 0, WA_TAIL_BYTES);
> @@ -528,7 +528,7 @@ static void execlists_dequeue(struct intel_engine_cs
> *engine)
> if (!execlists_is_active(execlists, EXECLISTS_ACTIVE_HWACK))
> goto unlock;
>
> - if (HAS_LOGICAL_RING_PREEMPTION(engine->i915) &&
> + if (engine->i915->preempt_context &&
> rb_entry(rb, struct i915_priolist, node)->priority >
> max(last->priotree.priority, 0)) {
> /*
> @@ -844,7 +844,7 @@ static void execlists_submission_tasklet(unsigned long
> data)
> GEM_BUG_ON(status & GEN8_CTX_STATUS_IDLE_ACTIVE);
>
> if (status & GEN8_CTX_STATUS_COMPLETE &&
> - buf[2*head + 1] == PREEMPT_ID) {
> + buf[2*head + 1] ==
> execlists->preempt_complete_status) {
> GEM_TRACE("%s preempt-idle\n", engine->name);
>
> execlists_cancel_port_requests(execlists);
> @@ -1910,7 +1910,7 @@ static void execlists_set_default_submission(struct
> intel_engine_cs *engine)
> engine->i915->caps.scheduler =
> I915_SCHEDULER_CAP_ENABLED |
> I915_SCHEDULER_CAP_PRIORITY;
> - if (HAS_LOGICAL_RING_PREEMPTION(engine->i915))
> + if (engine->i915->preempt_context)
> engine->i915->caps.scheduler |= I915_SCHEDULER_CAP_PREEMPTION;
> }
>
> @@ -1988,6 +1988,11 @@ static int logical_ring_init(struct intel_engine_cs
> *engine)
> engine->execlists.elsp =
> engine->i915->regs + i915_mmio_reg_offset(RING_ELSP(engine));
>
> + engine->execlists.preempt_complete_status = ~0u;
This is to ensure that we catch a rogue status? Atleast we will
bug on of preempting with this status as the EXECLIST_ACTIVE_PREEMPT
will be false.
> + if (engine->i915->preempt_context)
> + engine->execlists.preempt_complete_status =
> +
> upper_32_bits(engine->i915->preempt_context->engine[engine->id].lrc_desc);
We have not upgraded the descriptor yet, so just use preempt_context->hw_id;
I would also like duplicate, from i915_gem_context.c, the assertion that
hw_id needs to be <= INT_MAX but didn't find a good spot.
-Mika
> +
> return 0;
>
> error:
> @@ -2250,7 +2255,7 @@ populate_lr_context(struct i915_gem_context *ctx,
> if (!engine->default_state)
> regs[CTX_CONTEXT_CONTROL + 1] |=
> _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT);
> - if (ctx->hw_id == PREEMPT_ID)
> + if (ctx == ctx->i915->preempt_context)
> regs[CTX_CONTEXT_CONTROL + 1] |=
> _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT |
> CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT);
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h
> b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index c5ff203e42d6..03be8995f415 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -279,6 +279,11 @@ struct intel_engine_execlists {
> * @csb_use_mmio: access csb through mmio, instead of hwsp
> */
> bool csb_use_mmio;
> +
> + /**
> + * @preempt_complete_status: expected CSB upon completing preemption
> + */
> + u32 preempt_complete_status;
> };
>
> #define INTEL_ENGINE_CS_MAX_NAME 8
> diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> index 1bc61f3f76fc..3175db70cc6e 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> @@ -243,16 +243,10 @@ struct drm_i915_private *mock_gem_device(void)
> if (!i915->kernel_context)
> goto err_engine;
>
> - i915->preempt_context = mock_context(i915, NULL);
> - if (!i915->preempt_context)
> - goto err_kernel_context;
> -
> WARN_ON(i915_gemfs_init(i915));
>
> return i915;
>
> -err_kernel_context:
> - i915_gem_context_put(i915->kernel_context);
> err_engine:
> for_each_engine(engine, i915, id)
> mock_engine_free(engine);
> --
> 2.15.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
