Re: [PATCH 21/43] drm/i915: Track the pinned kernel contexts on each engine

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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@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@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx