From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 31/38] drm/i915: Track the pinned kernel contexts on each engine
Date: Tue, 5 Mar 2019 18:07:39 +0000 [thread overview]
Message-ID: <a1e5b2db-4949-2e32-345a-e2e4d35eb6ba@linux.intel.com> (raw)
In-Reply-To: <20190301140404.26690-31-chris@chris-wilson.co.uk>
On 01/03/2019 14:03, Chris Wilson wrote:
> Each engine acquires a pin on the kernel contexts (normal and preempt)
> so that the logical state is always available on demand. Keep track of
> each engines pin by storing the returned pointer on the engine for quick
> access.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/intel_engine_cs.c | 65 ++++++++++----------
> drivers/gpu/drm/i915/intel_engine_types.h | 8 +--
> drivers/gpu/drm/i915/intel_guc_ads.c | 3 +-
> drivers/gpu/drm/i915/intel_lrc.c | 13 ++--
> drivers/gpu/drm/i915/intel_ringbuffer.c | 3 +-
> drivers/gpu/drm/i915/selftests/mock_engine.c | 5 +-
> 6 files changed, 45 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 112301560745..0f311752ee08 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -641,12 +641,6 @@ void intel_engines_set_scheduler_caps(struct drm_i915_private *i915)
> i915->caps.scheduler = 0;
> }
>
> -static void __intel_context_unpin(struct i915_gem_context *ctx,
> - struct intel_engine_cs *engine)
> -{
> - intel_context_unpin(intel_context_lookup(ctx, engine));
> -}
> -
> struct measure_breadcrumb {
> struct i915_request rq;
> struct i915_timeline timeline;
> @@ -697,6 +691,20 @@ static int measure_breadcrumb_dw(struct intel_engine_cs *engine)
> return dw;
> }
>
> +static int pin_context(struct i915_gem_context *ctx,
> + struct intel_engine_cs *engine,
> + struct intel_context **out)
> +{
> + struct intel_context *ce;
> +
> + ce = intel_context_pin(ctx, engine);
> + if (IS_ERR(ce))
> + return PTR_ERR(ce);
> +
> + *out = ce;
> + return 0;
> +}
> +
> /**
> * intel_engines_init_common - initialize cengine state which might require hw access
> * @engine: Engine to initialize.
> @@ -711,11 +719,8 @@ static int measure_breadcrumb_dw(struct intel_engine_cs *engine)
> int intel_engine_init_common(struct intel_engine_cs *engine)
> {
> struct drm_i915_private *i915 = engine->i915;
> - struct intel_context *ce;
> int ret;
>
> - engine->set_default_submission(engine);
> -
> /* We may need to do things with the shrinker which
> * require us to immediately switch back to the default
> * context. This can cause a problem as pinning the
> @@ -723,36 +728,34 @@ int intel_engine_init_common(struct intel_engine_cs *engine)
> * be available. To avoid this we always pin the default
> * context.
> */
> - ce = intel_context_pin(i915->kernel_context, engine);
> - if (IS_ERR(ce))
> - return PTR_ERR(ce);
> + ret = pin_context(i915->kernel_context, engine,
> + &engine->kernel_context);
> + if (ret)
> + return ret;
>
> /*
> * Similarly the preempt context must always be available so that
> - * we can interrupt the engine at any time.
> + * we can interrupt the engine at any time. However, as preemption
> + * is optional, we allow it to fail.
> */
> - if (i915->preempt_context) {
> - ce = intel_context_pin(i915->preempt_context, engine);
> - if (IS_ERR(ce)) {
> - ret = PTR_ERR(ce);
> - goto err_unpin_kernel;
> - }
> - }
> + if (i915->preempt_context)
> + pin_context(i915->preempt_context, engine,
> + &engine->preempt_context);
You lost the failure path here. I suspect deliberately? But I am not
convinced we want to silently lose preemption when keeping the failure
path is so easy.
>
> ret = measure_breadcrumb_dw(engine);
> if (ret < 0)
> - goto err_unpin_preempt;
> + goto err_unpin;
>
> engine->emit_fini_breadcrumb_dw = ret;
>
> - return 0;
> + engine->set_default_submission(engine);
>
> -err_unpin_preempt:
> - if (i915->preempt_context)
> - __intel_context_unpin(i915->preempt_context, engine);
> + return 0;
>
> -err_unpin_kernel:
> - __intel_context_unpin(i915->kernel_context, engine);
> +err_unpin:
> + if (engine->preempt_context)
> + intel_context_unpin(engine->preempt_context);
> + intel_context_unpin(engine->kernel_context);
> return ret;
> }
>
> @@ -765,8 +768,6 @@ int intel_engine_init_common(struct intel_engine_cs *engine)
> */
> void intel_engine_cleanup_common(struct intel_engine_cs *engine)
> {
> - struct drm_i915_private *i915 = engine->i915;
> -
> cleanup_status_page(engine);
>
> intel_engine_fini_breadcrumbs(engine);
> @@ -776,9 +777,9 @@ void intel_engine_cleanup_common(struct intel_engine_cs *engine)
> if (engine->default_state)
> i915_gem_object_put(engine->default_state);
>
> - if (i915->preempt_context)
> - __intel_context_unpin(i915->preempt_context, engine);
> - __intel_context_unpin(i915->kernel_context, engine);
> + if (engine->preempt_context)
> + intel_context_unpin(engine->preempt_context);
> + intel_context_unpin(engine->kernel_context);
>
> i915_timeline_fini(&engine->timeline);
>
> diff --git a/drivers/gpu/drm/i915/intel_engine_types.h b/drivers/gpu/drm/i915/intel_engine_types.h
> index aefc66605729..85158e119638 100644
> --- a/drivers/gpu/drm/i915/intel_engine_types.h
> +++ b/drivers/gpu/drm/i915/intel_engine_types.h
> @@ -231,11 +231,6 @@ struct intel_engine_execlists {
> */
> u32 *csb_status;
>
> - /**
> - * @preempt_context: the HW context for injecting preempt-to-idle
> - */
> - struct intel_context *preempt_context;
> -
> /**
> * @preempt_complete_status: expected CSB upon completing preemption
> */
> @@ -271,6 +266,9 @@ struct intel_engine_cs {
>
> struct i915_timeline timeline;
>
> + struct intel_context *kernel_context; /* pinned */
> + struct intel_context *preempt_context; /* pinned; optional */
> +
> struct drm_i915_gem_object *default_state;
> void *pinned_default_state;
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_ads.c b/drivers/gpu/drm/i915/intel_guc_ads.c
> index da220561ac41..5fa169dadd67 100644
> --- a/drivers/gpu/drm/i915/intel_guc_ads.c
> +++ b/drivers/gpu/drm/i915/intel_guc_ads.c
> @@ -121,8 +121,7 @@ int intel_guc_ads_create(struct intel_guc *guc)
> * to find it. Note that we have to skip our header (1 page),
> * because our GuC shared data is there.
> */
> - kernel_ctx_vma = intel_context_lookup(dev_priv->kernel_context,
> - dev_priv->engine[RCS])->state;
> + kernel_ctx_vma = dev_priv->engine[RCS]->kernel_context->state;
> blob->ads.golden_context_lrca =
> intel_guc_ggtt_offset(guc, kernel_ctx_vma) + skipped_offset;
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 8dddea8e1c97..473cfea4c503 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -620,7 +620,7 @@ static void port_assign(struct execlist_port *port, struct i915_request *rq)
> static void inject_preempt_context(struct intel_engine_cs *engine)
> {
> struct intel_engine_execlists *execlists = &engine->execlists;
> - struct intel_context *ce = execlists->preempt_context;
> + struct intel_context *ce = engine->preempt_context;
> unsigned int n;
>
> GEM_BUG_ON(execlists->preempt_complete_status !=
> @@ -2316,7 +2316,7 @@ void intel_execlists_set_default_submission(struct intel_engine_cs *engine)
>
> engine->flags |= I915_ENGINE_HAS_SEMAPHORES;
> engine->flags |= I915_ENGINE_SUPPORTS_STATS;
> - if (engine->i915->preempt_context)
> + if (engine->preempt_context)
> engine->flags |= I915_ENGINE_HAS_PREEMPTION;
> }
>
> @@ -2418,14 +2418,9 @@ static int logical_ring_init(struct intel_engine_cs *engine)
> }
>
> execlists->preempt_complete_status = ~0u;
> - if (i915->preempt_context) {
> - struct intel_context *ce =
> - intel_context_lookup(i915->preempt_context, engine);
> -
> - execlists->preempt_context = ce;
> + if (engine->preempt_context)
> execlists->preempt_complete_status =
> - upper_32_bits(ce->lrc_desc);
> - }
> + upper_32_bits(engine->preempt_context->lrc_desc);
>
> execlists->csb_status =
> &engine->status_page.addr[I915_HWS_CSB_BUF0_INDEX];
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 6af2d393b7ff..4f137d0a5316 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1734,8 +1734,7 @@ static inline int mi_set_context(struct i915_request *rq, u32 flags)
> * placeholder we use to flush other contexts.
> */
> *cs++ = MI_SET_CONTEXT;
> - *cs++ = i915_ggtt_offset(intel_context_lookup(i915->kernel_context,
> - engine)->state) |
> + *cs++ = i915_ggtt_offset(engine->kernel_context->state) |
> MI_MM_SPACE_GTT |
> MI_RESTORE_INHIBIT;
> }
> diff --git a/drivers/gpu/drm/i915/selftests/mock_engine.c b/drivers/gpu/drm/i915/selftests/mock_engine.c
> index 5a5c3ba22061..6434e968ecb0 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_engine.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_engine.c
> @@ -233,7 +233,8 @@ struct intel_engine_cs *mock_engine(struct drm_i915_private *i915,
> timer_setup(&engine->hw_delay, hw_delay_complete, 0);
> INIT_LIST_HEAD(&engine->hw_queue);
>
> - if (IS_ERR(intel_context_pin(i915->kernel_context, &engine->base)))
> + if (pin_context(i915->kernel_context, &engine->base,
> + &engine->base.kernel_context))
> goto err_breadcrumbs;
>
> return &engine->base;
> @@ -276,7 +277,7 @@ void mock_engine_free(struct intel_engine_cs *engine)
> if (ce)
> intel_context_unpin(ce);
>
> - __intel_context_unpin(engine->i915->kernel_context, engine);
> + intel_context_unpin(engine->kernel_context);
>
> intel_engine_fini_breadcrumbs(engine);
> i915_timeline_fini(&engine->timeline);
>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2019-03-05 18:07 UTC|newest]
Thread overview: 88+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-01 14:03 [PATCH 01/38] drm/i915/execlists: Suppress redundant preemption Chris Wilson
2019-03-01 14:03 ` [PATCH 02/38] drm/i915: Introduce i915_timeline.mutex Chris Wilson
2019-03-01 15:09 ` Tvrtko Ursulin
2019-03-01 14:03 ` [PATCH 03/38] drm/i915: Keep timeline HWSP allocated until idle across the system Chris Wilson
2019-03-01 14:03 ` [PATCH 04/38] drm/i915: Use HW semaphores for inter-engine synchronisation on gen8+ Chris Wilson
2019-03-01 15:11 ` Tvrtko Ursulin
2019-03-01 14:03 ` [PATCH 05/38] drm/i915: Prioritise non-busywait semaphore workloads Chris Wilson
2019-03-01 15:12 ` Tvrtko Ursulin
2019-03-01 14:03 ` [PATCH 06/38] drm/i915/selftests: Check that whitelisted registers are accessible Chris Wilson
2019-03-01 15:18 ` Michał Winiarski
2019-03-01 15:25 ` Chris Wilson
2019-03-01 14:03 ` [PATCH 07/38] drm/i915: Force GPU idle on suspend Chris Wilson
2019-03-01 14:03 ` [PATCH 08/38] drm/i915/selftests: Improve switch-to-kernel-context checking Chris Wilson
2019-03-01 14:03 ` [PATCH 09/38] drm/i915: Do a synchronous switch-to-kernel-context on idling Chris Wilson
2019-03-01 14:03 ` [PATCH 10/38] drm/i915: Store the BIT(engine->id) as the engine's mask Chris Wilson
2019-03-01 15:25 ` Tvrtko Ursulin
2019-03-01 14:03 ` [PATCH 11/38] drm/i915: Refactor common code to load initial power context Chris Wilson
2019-03-01 14:03 ` [PATCH 12/38] drm/i915: Reduce presumption of request ordering for barriers Chris Wilson
2019-03-01 14:03 ` [PATCH 13/38] drm/i915: Remove has-kernel-context Chris Wilson
2019-03-01 14:03 ` [PATCH 14/38] drm/i915: Introduce the i915_user_extension_method Chris Wilson
2019-03-01 15:39 ` Tvrtko Ursulin
2019-03-01 18:57 ` Chris Wilson
2019-03-04 8:54 ` Tvrtko Ursulin
2019-03-04 9:04 ` Chris Wilson
2019-03-04 9:35 ` Tvrtko Ursulin
2019-03-04 9:45 ` Tvrtko Ursulin
2019-03-01 14:03 ` [PATCH 15/38] drm/i915: Track active engines within a context Chris Wilson
2019-03-01 15:46 ` Tvrtko Ursulin
2019-03-01 14:03 ` [PATCH 16/38] drm/i915: Introduce a context barrier callback Chris Wilson
2019-03-01 16:12 ` Tvrtko Ursulin
2019-03-01 19:03 ` Chris Wilson
2019-03-04 8:55 ` Tvrtko Ursulin
2019-03-02 10:01 ` Chris Wilson
2019-03-01 14:03 ` [PATCH 17/38] drm/i915: Create/destroy VM (ppGTT) for use with contexts Chris Wilson
2019-03-06 11:27 ` Tvrtko Ursulin
2019-03-06 11:36 ` Chris Wilson
2019-03-01 14:03 ` [PATCH 18/38] drm/i915: Extend CONTEXT_CREATE to set parameters upon construction Chris Wilson
2019-03-01 16:36 ` Tvrtko Ursulin
2019-03-01 19:10 ` Chris Wilson
2019-03-04 8:57 ` Tvrtko Ursulin
2019-03-01 14:03 ` [PATCH 19/38] drm/i915: Allow contexts to share a single timeline across all engines Chris Wilson
2019-03-05 15:54 ` Tvrtko Ursulin
2019-03-05 16:26 ` Chris Wilson
2019-03-01 14:03 ` [PATCH 20/38] drm/i915: Allow userspace to clone contexts on creation Chris Wilson
2019-03-01 14:03 ` [PATCH 21/38] drm/i915: Fix I915_EXEC_RING_MASK Chris Wilson
2019-03-01 15:29 ` Tvrtko Ursulin
2019-03-01 14:03 ` [PATCH 22/38] drm/i915: Remove last traces of exec-id (GEM_BUSY) Chris Wilson
2019-03-01 14:03 ` [PATCH 23/38] drm/i915: Re-arrange execbuf so context is known before engine Chris Wilson
2019-03-01 15:33 ` Tvrtko Ursulin
2019-03-01 19:11 ` Chris Wilson
2019-03-01 14:03 ` [PATCH 24/38] drm/i915: Allow a context to define its set of engines Chris Wilson
2019-03-01 14:03 ` [PATCH 25/38] drm/i915: Extend I915_CONTEXT_PARAM_SSEU to support local ctx->engine[] Chris Wilson
2019-03-01 14:03 ` [PATCH 26/38] drm/i915: Pass around the intel_context Chris Wilson
2019-03-05 16:16 ` Tvrtko Ursulin
2019-03-05 16:33 ` Chris Wilson
2019-03-05 19:23 ` Chris Wilson
2019-03-01 14:03 ` [PATCH 27/38] drm/i915: Split struct intel_context definition to its own header Chris Wilson
2019-03-05 16:19 ` Tvrtko Ursulin
2019-03-05 16:35 ` Chris Wilson
2019-03-01 14:03 ` [PATCH 28/38] drm/i915: Store the intel_context_ops in the intel_engine_cs Chris Wilson
2019-03-05 16:27 ` Tvrtko Ursulin
2019-03-05 16:45 ` Chris Wilson
2019-03-05 18:27 ` Chris Wilson
2019-03-01 14:03 ` [PATCH 29/38] drm/i915: Move over to intel_context_lookup() Chris Wilson
2019-03-05 17:01 ` Tvrtko Ursulin
2019-03-05 17:10 ` Chris Wilson
2019-03-01 14:03 ` [PATCH 30/38] drm/i915: Make context pinning part of intel_context_ops Chris Wilson
2019-03-05 17:31 ` Tvrtko Ursulin
2019-03-05 18:00 ` Chris Wilson
2019-03-01 14:03 ` [PATCH 31/38] drm/i915: Track the pinned kernel contexts on each engine Chris Wilson
2019-03-05 18:07 ` Tvrtko Ursulin [this message]
2019-03-05 18:10 ` Chris Wilson
2019-03-05 18:17 ` Tvrtko Ursulin
2019-03-05 19:26 ` Chris Wilson
2019-03-01 14:03 ` [PATCH 32/38] drm/i915: Introduce intel_context.pin_mutex for pin management Chris Wilson
2019-03-06 9:45 ` Tvrtko Ursulin
2019-03-06 10:15 ` Chris Wilson
2019-03-01 14:03 ` [PATCH 33/38] drm/i915: Load balancing across a virtual engine Chris Wilson
2019-03-01 14:04 ` [PATCH 34/38] drm/i915: Extend execution fence to support a callback Chris Wilson
2019-03-01 14:04 ` [PATCH 35/38] drm/i915/execlists: Virtual engine bonding Chris Wilson
2019-03-01 14:04 ` [PATCH 36/38] drm/i915: Allow specification of parallel execbuf Chris Wilson
2019-03-01 14:04 ` [PATCH 37/38] drm/i915/selftests: Check preemption support on each engine Chris Wilson
2019-03-06 11:29 ` Tvrtko Ursulin
2019-03-01 14:04 ` [PATCH 38/38] drm/i915/execlists: Skip direct submission if only lite-restore Chris Wilson
2019-03-01 14:15 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [01/38] drm/i915/execlists: Suppress redundant preemption Patchwork
2019-03-01 14:32 ` ✗ Fi.CI.SPARSE: " Patchwork
2019-03-01 14:36 ` ✓ Fi.CI.BAT: success " Patchwork
2019-03-01 18:03 ` ✗ Fi.CI.IGT: failure " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a1e5b2db-4949-2e32-345a-e2e4d35eb6ba@linux.intel.com \
--to=tvrtko.ursulin@linux.intel.com \
--cc=chris@chris-wilson.co.uk \
--cc=intel-gfx@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.