Re: [PATCH v3 21/21] drm/i915: Unify GPU resets upon shutdown

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

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

> [ text/plain ]
> Both execlists and legacy need to reset the context (and mode) of the
> GPU before we lose control of the system. By resetting the GPU, we
> revert back to default settings. This simplifies the life of any
> subsequent driver (in particular for virtualized setups) as it does not
> then have to try and recover from an unknown condition. As both paths
> need to reset for the same reason, move the reset to a common point.
>
> This unifies the resets added in a647828afc (drm/i915: Also perform gpu
> reset under execlist mode) and 8e96d9c4d9 (drm/i915: reset the GPU on
> context fini).
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> CC: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: "Niu, Bing" <bing.niu@intel.com>

We should assume that we inherit the mess and thus reset on init.
But it is also proper that we clean up the house when we leave.

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

> ---
>  drivers/gpu/drm/i915/i915_dma.c         | 41 ++++++++++++++++++++++++++-------
>  drivers/gpu/drm/i915/i915_gem.c         |  8 -------
>  drivers/gpu/drm/i915/i915_gem_context.c |  8 +------
>  3 files changed, 34 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 5c7615041b31..68c60f4fd44d 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -425,6 +425,37 @@ static const struct vga_switcheroo_client_ops i915_switcheroo_ops = {
>  	.can_switch = i915_switcheroo_can_switch,
>  };
>  
> +static void i915_gem_fini(struct drm_device *dev)
> +{
> +	int reset;
> +
> +	/*
> +	 * Neither the BIOS, ourselves or any other kernel
> +	 * expects the system to be in execlists mode on startup,
> +	 * so we need to reset the GPU back to legacy mode. And the only
> +	 * known way to disable logical contexts is through a GPU reset.
> +	 *
> +	 * So in order to leave the system in a known default configuration,
> +	 * always reset the GPU upon unload. Afterwards we then clean up the
> +	 * GEM state tracking, flushing off the requests and leaving the
> +	 * system in a known idle state.
> +	 *
> +	 * Note that is of the upmost importance that the GPU is idle and
> +	 * all stray writes are flushed *before* we dismantle the backing
> +	 * storage for the pinned objects.
> +	 */
> +	reset = intel_gpu_reset(dev, ALL_ENGINES);
> +	WARN_ON(reset && reset != -ENODEV);
> +
> +	mutex_lock(&dev->struct_mutex);
> +	i915_gem_reset(dev);
> +	i915_gem_cleanup_engines(dev);
> +	i915_gem_context_fini(dev);
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	WARN_ON(!list_empty(&to_i915(dev)->context_list));
> +}
> +
>  static int i915_load_modeset_init(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -506,10 +537,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  	return 0;
>  
>  cleanup_gem:
> -	mutex_lock(&dev->struct_mutex);
> -	i915_gem_cleanup_engines(dev);
> -	i915_gem_context_fini(dev);
> -	mutex_unlock(&dev->struct_mutex);
> +	i915_gem_fini(dev);
>  cleanup_irq:
>  	intel_guc_ucode_fini(dev);
>  	drm_irq_uninstall(dev);
> @@ -1456,10 +1484,7 @@ int i915_driver_unload(struct drm_device *dev)
>  	flush_workqueue(dev_priv->wq);
>  
>  	intel_guc_ucode_fini(dev);
> -	mutex_lock(&dev->struct_mutex);
> -	i915_gem_cleanup_engines(dev);
> -	i915_gem_context_fini(dev);
> -	mutex_unlock(&dev->struct_mutex);
> +	i915_gem_fini(dev);
>  	intel_fbc_cleanup_cfb(dev_priv);
>  
>  	intel_power_domains_fini(dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index a88aa7f11a9c..82e41eeb36bb 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4970,14 +4970,6 @@ i915_gem_cleanup_engines(struct drm_device *dev)
>  
>  	for_each_engine(engine, dev_priv)
>  		dev_priv->gt.cleanup_engine(engine);
> -
> -	if (i915.enable_execlists)
> -		/*
> -		 * Neither the BIOS, ourselves or any other kernel
> -		 * expects the system to be in execlists mode on startup,
> -		 * so we need to reset the GPU back to legacy mode.
> -		 */
> -		intel_gpu_reset(dev, ALL_ENGINES);
>  }
>  
>  static void
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index fef33a3ce3b7..26122de365b2 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -452,14 +452,8 @@ void i915_gem_context_fini(struct drm_device *dev)
>  
>  	i915_gem_context_lost(dev_priv);
>  
> -	if (dctx->legacy_hw_ctx.rcs_state) {
> -		/* The only known way to stop the gpu from accessing the hw context is
> -		 * to reset it. Do this as the very last operation to avoid confusing
> -		 * other code, leading to spurious errors. */
> -		intel_gpu_reset(dev, ALL_ENGINES);
> -
> +	if (dctx->legacy_hw_ctx.rcs_state)
>  		i915_gem_object_ggtt_unpin(dctx->legacy_hw_ctx.rcs_state);
> -	}
>  
>  	i915_gem_context_unreference(dctx);
>  	dev_priv->kernel_context = NULL;
> -- 
> 2.8.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx