Re: [PATCH 07/43] drm/i915: Remove has-kernel-context

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

On 06/03/2019 14:24, Chris Wilson wrote:
> We can no longer assume execution ordering, and in particular we cannot
> assume which context will execute last. One side-effect of this is that
> we cannot determine if the kernel-context is resident on the GPU, so
> remove the routines that claimed to do so.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_active.h      | 13 -----------
>   drivers/gpu/drm/i915/i915_gem.c         | 18 --------------
>   drivers/gpu/drm/i915/i915_gem_evict.c   | 16 +++----------
>   drivers/gpu/drm/i915/intel_engine_cs.c  | 31 -------------------------
>   drivers/gpu/drm/i915/intel_ringbuffer.h |  1 -
>   5 files changed, 3 insertions(+), 76 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_active.h b/drivers/gpu/drm/i915/i915_active.h
> index 8142a334b37b..7d758719ce39 100644
> --- a/drivers/gpu/drm/i915/i915_active.h
> +++ b/drivers/gpu/drm/i915/i915_active.h
> @@ -108,19 +108,6 @@ i915_active_request_set_retire_fn(struct i915_active_request *active,
>   	active->retire = fn ?: i915_active_retire_noop;
>   }
>   
> -static inline struct i915_request *
> -__i915_active_request_peek(const struct i915_active_request *active)
> -{
> -	/*
> -	 * Inside the error capture (running with the driver in an unknown
> -	 * state), we want to bend the rules slightly (a lot).
> -	 *
> -	 * Work is in progress to make it safer, in the meantime this keeps
> -	 * the known issue from spamming the logs.
> -	 */
> -	return rcu_dereference_protected(active->request, 1);
> -}
> -
>   /**
>    * i915_active_request_raw - return the active request
>    * @active - the active tracker
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index ed7695fd444a..8d5918b99fc9 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2828,23 +2828,6 @@ i915_gem_retire_work_handler(struct work_struct *work)
>   				   round_jiffies_up_relative(HZ));
>   }
>   
> -static void assert_kernel_context_is_current(struct drm_i915_private *i915)
> -{
> -	struct intel_engine_cs *engine;
> -	enum intel_engine_id id;
> -
> -	if (i915_reset_failed(i915))
> -		return;
> -
> -	i915_retire_requests(i915);
> -
> -	for_each_engine(engine, i915, id) {
> -		GEM_BUG_ON(__i915_active_request_peek(&engine->timeline.last_request));
> -		GEM_BUG_ON(engine->last_retired_context !=
> -			   to_intel_context(i915->kernel_context, engine));
> -	}
> -}
> -
>   static bool switch_to_kernel_context_sync(struct drm_i915_private *i915,
>   					  unsigned long mask)
>   {
> @@ -2857,7 +2840,6 @@ static bool switch_to_kernel_context_sync(struct drm_i915_private *i915,
>   				   HZ / 10))
>   		return false;
>   
> -	assert_kernel_context_is_current(i915);
>   	return true;
>   }
>   
> diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
> index 7d8e90dfca84..060f5903544a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_evict.c
> +++ b/drivers/gpu/drm/i915/i915_gem_evict.c
> @@ -38,25 +38,15 @@ I915_SELFTEST_DECLARE(static struct igt_evict_ctl {
>   
>   static bool ggtt_is_idle(struct drm_i915_private *i915)
>   {
> -       struct intel_engine_cs *engine;
> -       enum intel_engine_id id;
> -
> -       if (i915->gt.active_requests)
> -	       return false;
> -
> -       for_each_engine(engine, i915, id) {
> -	       if (!intel_engine_has_kernel_context(engine))
> -		       return false;
> -       }
> -
> -       return true;
> +	return !i915->gt.active_requests;
>   }
>   
>   static int ggtt_flush(struct drm_i915_private *i915)
>   {
>   	int err;
>   
> -	/* Not everything in the GGTT is tracked via vma (otherwise we
> +	/*
> +	 * Not everything in the GGTT is tracked via vma (otherwise we
>   	 * could evict as required with minimal stalling) so we are forced
>   	 * to idle the GPU and explicitly retire outstanding requests in
>   	 * the hopes that we can then remove contexts and the like only
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 18174f808fd8..8e326556499e 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -1090,37 +1090,6 @@ bool intel_engines_are_idle(struct drm_i915_private *i915)
>   	return true;
>   }
>   
> -/**
> - * intel_engine_has_kernel_context:
> - * @engine: the engine
> - *
> - * Returns true if the last context to be executed on this engine, or has been
> - * executed if the engine is already idle, is the kernel context
> - * (#i915.kernel_context).
> - */
> -bool intel_engine_has_kernel_context(const struct intel_engine_cs *engine)
> -{
> -	const struct intel_context *kernel_context =
> -		to_intel_context(engine->i915->kernel_context, engine);
> -	struct i915_request *rq;
> -
> -	lockdep_assert_held(&engine->i915->drm.struct_mutex);
> -
> -	if (!engine->context_size)
> -		return true;
> -
> -	/*
> -	 * Check the last context seen by the engine. If active, it will be
> -	 * the last request that remains in the timeline. When idle, it is
> -	 * the last executed context as tracked by retirement.
> -	 */
> -	rq = __i915_active_request_peek(&engine->timeline.last_request);
> -	if (rq)
> -		return rq->hw_context == kernel_context;
> -	else
> -		return engine->last_retired_context == kernel_context;
> -}
> -
>   void intel_engines_reset_default_submission(struct drm_i915_private *i915)
>   {
>   	struct intel_engine_cs *engine;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 84b7047e2df5..9ccbe63d46e3 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -935,7 +935,6 @@ void intel_engines_sanitize(struct drm_i915_private *i915, bool force);
>   bool intel_engine_is_idle(struct intel_engine_cs *engine);
>   bool intel_engines_are_idle(struct drm_i915_private *dev_priv);
>   
> -bool intel_engine_has_kernel_context(const struct intel_engine_cs *engine);
>   void intel_engine_lost_context(struct intel_engine_cs *engine);
>   
>   void intel_engines_park(struct drm_i915_private *i915);
> 

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