Re: [PATCH 02/18] drm/i915: Switch to kernel context before idling at runtime

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> We can reduce our exposure to random neutrinos by resting on the kernel
> context having flushed out the user contexts to system memory and
> beyond. The corollary is that we then we require two passes through the
> idle handler to go to sleep, which on a truly idle system involves an
> extra pass through the slow and irregular retire work handler.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |  7 +++++--
>  drivers/gpu/drm/i915/i915_gem.c     | 29 ++++++++++++++++++++++++-----
>  2 files changed, 29 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index a8e7761cdc7d..0fb86a8ef192 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4226,8 +4226,11 @@ i915_drop_caches_set(void *data, u64 val)
>  		i915_gem_shrink_all(dev_priv);
>  	fs_reclaim_release(GFP_KERNEL);
>  
> -	if (val & DROP_IDLE)
> -		drain_delayed_work(&dev_priv->gt.idle_work);
> +	if (val & DROP_IDLE) {
> +		do {
> +			drain_delayed_work(&dev_priv->gt.idle_work);
> +		} while (READ_ONCE(dev_priv->gt.awake));
> +	}

Should we do this on suspend path also? Perhaps better
just to warn there, like we do, and let it slide.

>  
>  	if (val & DROP_FREED)
>  		i915_gem_drain_freed_objects(dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 05f44ca35a06..c93f5dcb1d82 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -139,6 +139,8 @@ int i915_mutex_lock_interruptible(struct drm_device *dev)
>  
>  static u32 __i915_gem_park(struct drm_i915_private *i915)
>  {
> +	GEM_TRACE("\n");
> +
>  	lockdep_assert_held(&i915->drm.struct_mutex);
>  	GEM_BUG_ON(i915->gt.active_requests);
>  	GEM_BUG_ON(!list_empty(&i915->gt.active_rings));
> @@ -193,6 +195,8 @@ void i915_gem_park(struct drm_i915_private *i915)
>  
>  void i915_gem_unpark(struct drm_i915_private *i915)
>  {
> +	GEM_TRACE("\n");
> +
>  	lockdep_assert_held(&i915->drm.struct_mutex);
>  	GEM_BUG_ON(!i915->gt.active_requests);
>  
> @@ -3504,6 +3508,21 @@ i915_gem_idle_work_handler(struct work_struct *work)
>  	if (!READ_ONCE(dev_priv->gt.awake))
>  		return;
>  
> +	/*
> +	 * Flush out the last user context, leaving only the pinned
> +	 * kernel context resident. When we are idling on the kernel_context,
> +	 * no more new requests (with a context switch) are emitted and we
> +	 * can finally rest. A consequence is that the idle work handler is
> +	 * always called at least twice before idling (and if the system is
> +	 * idle that implies a round trip through the retire worker).
> +	 */

We keep gt awake a little longer and inject somtimes 'useless'
switch to kernel ctx.

It is acceptable price to pay for a sound bed to sleep on.

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

> +	mutex_lock(&dev_priv->drm.struct_mutex);
> +	i915_gem_switch_to_kernel_context(dev_priv);
> +	mutex_unlock(&dev_priv->drm.struct_mutex);
> +
> +	GEM_TRACE("active_requests=%d (after switch-to-kernel-context)\n",
> +		  READ_ONCE(dev_priv->gt.active_requests));
> +
>  	/*
>  	 * Wait for last execlists context complete, but bail out in case a
>  	 * new request is submitted. As we don't trust the hardware, we
> @@ -4914,11 +4933,9 @@ static void assert_kernel_context_is_current(struct drm_i915_private *i915)
>  
>  void i915_gem_sanitize(struct drm_i915_private *i915)
>  {
> -	if (i915_terminally_wedged(&i915->gpu_error)) {
> -		mutex_lock(&i915->drm.struct_mutex);
> +	mutex_lock(&i915->drm.struct_mutex);
> +	if (i915_terminally_wedged(&i915->gpu_error))
>  		i915_gem_unset_wedged(i915);
> -		mutex_unlock(&i915->drm.struct_mutex);
> -	}
>  
>  	/*
>  	 * If we inherit context state from the BIOS or earlier occupants
> @@ -4930,6 +4947,9 @@ void i915_gem_sanitize(struct drm_i915_private *i915)
>  	 */
>  	if (INTEL_GEN(i915) >= 5 && intel_has_gpu_reset(i915))
>  		WARN_ON(intel_gpu_reset(i915, ALL_ENGINES));
> +
> +	i915_gem_contexts_lost(i915);
> +	mutex_unlock(&i915->drm.struct_mutex);
>  }
>  
>  int i915_gem_suspend(struct drm_i915_private *dev_priv)
> @@ -4965,7 +4985,6 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
>  
>  		assert_kernel_context_is_current(dev_priv);
>  	}
> -	i915_gem_contexts_lost(dev_priv);
>  	mutex_unlock(&dev->struct_mutex);
>  
>  	intel_uc_suspend(dev_priv);
> -- 
> 2.17.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx