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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox