[Intel-gfx] [PATCH] drm/i915: Only allocate preempt context when required
Lis, Tomasz
tomasz.lis at intel.com
Tue Jan 30 18:47:55 UTC 2018
On 2018-01-27 21:28, Chris Wilson wrote:
> 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 <chris at chris-wilson.co.uk>
> Cc: Michal Winiarski <michal.winiarski at intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Arkadiusz Hiler <arkadiusz.hiler at intel.com>
> Cc: Mika Kuoppala <mika.kuoppala at intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.c | 2 +-
> drivers/gpu/drm/i915/i915_gem_context.c | 24 +++++++++---------------
> 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 | 15 ++++++++++-----
> drivers/gpu/drm/i915/intel_ringbuffer.h | 5 +++++
> drivers/gpu/drm/i915/selftests/mock_gem_device.c | 6 ------
> 7 files changed, 41 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 1ec12add34b2..a7fc87e87fcf 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -376,7 +376,7 @@ static int i915_getparam(struct drm_device *dev, void *data,
> if (dev_priv->engine[RCS] && dev_priv->engine[RCS]->schedule) {
> value |= I915_SCHEDULER_CAP_ENABLED;
> value |= I915_SCHEDULER_CAP_PRIORITY;
> - if (HAS_LOGICAL_RING_PREEMPTION(dev_priv))
> + if (dev_priv->preempt_context)
> value |= I915_SCHEDULER_CAP_PREEMPTION;
> }
> break;
We cannot put equality to not having preempt_context and having
preemption disabled. The preempt_context will not be required on
platforms which support preempting directly to idle.
There is no need to send anything to the ring for these platforms, and
the information about preemption finish is send back to us via interrupt
(or GuC message in case it's enabled).
Checking whether preempt_context is null before it is to be accessed is
a good idea, but for checking for feature availability,
HAS_LOGICAL_RING_PREEMPTION() will be needed.
The inject_preempt_context() functions will definitely not be called
when preempting to idle is available, so any changes there do not
collide with the new functionality.
-Tomasz
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 648e7536ff51..ebb1d01b4f4e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -452,7 +452,6 @@ destroy_kernel_context(struct i915_gem_context **ctxp)
> 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);
>
> @@ -468,8 +467,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 +477,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;
> + if (HAS_LOGICAL_RING_PREEMPTION(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 +514,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 1f3a8786bbdc..3de1eacb59c0 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];
>
> @@ -979,17 +979,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 2fa328d512fc..6ed90f28e9e0 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);
> @@ -1982,6 +1982,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;
> + if (engine->i915->preempt_context)
> + engine->execlists.preempt_complete_status =
> + upper_32_bits(engine->i915->preempt_context->engine[engine->id].lrc_desc);
> +
> return 0;
>
> error:
> @@ -2244,7 +2249,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);
More information about the Intel-gfx
mailing list