[Intel-gfx] [PATCH 21/43] drm/i915: Track the pinned kernel contexts on each engine
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Fri Mar 8 09:26:35 UTC 2019
On 06/03/2019 14:24, 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 at 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 1937128ea267..652c1b3ba190 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);
>
> 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 21f3275cc00c..b0aa1f0d4e47 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 b4ff0045a605..bec62f34b15a 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[RCS0])->state;
> + kernel_ctx_vma = dev_priv->engine[RCS0]->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 79107ab771d8..f7134d650187 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -626,7 +626,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 !=
> @@ -2321,7 +2321,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;
> }
>
> @@ -2423,14 +2423,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 d06908c9584c..03656021d04c 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1735,8 +1735,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 e483329fd533..99d7463218d0 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);
>
I am not entirely happy with the removal of preempt context pin failure
path but it is also something so unlikely so should not be considered a
blocker.
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
Regards,
Tvrtko
More information about the Intel-gfx
mailing list