[Intel-gfx] [PATCH v2] drm/i915: Mark the current context as lost on suspend

[Mika Kuoppala]

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

> [ text/plain ]
> In order to force a reload of the context image upon resume, we first
> need to mark its absence on suspend. Currently we are failing to restore
> the golden context state and any context w/a to the default context
> after resume.
>
> One oversight corrected, is that we had forgotten to reapply the L3
> remapping when restoring the lost default context.
>
> v2: Remove deprecated WARN.
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> ---

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

>  drivers/gpu/drm/i915/i915_drv.h         |  1 +
>  drivers/gpu/drm/i915/i915_gem.c         |  1 +
>  drivers/gpu/drm/i915/i915_gem_context.c | 54 ++++++++++++++-------------------
>  3 files changed, 25 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 85102ad75962..595037bec2de 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3299,6 +3299,7 @@ void i915_gem_object_save_bit_17_swizzle(struct drm_i915_gem_object *obj);
>  
>  /* i915_gem_context.c */
>  int __must_check i915_gem_context_init(struct drm_device *dev);
> +void i915_gem_context_lost(struct drm_i915_private *dev_priv);
>  void i915_gem_context_fini(struct drm_device *dev);
>  void i915_gem_context_reset(struct drm_device *dev);
>  int i915_gem_context_open(struct drm_device *dev, struct drm_file *file);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 6ce2c31b9a81..e7fe29857e23 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4711,6 +4711,7 @@ i915_gem_suspend(struct drm_device *dev)
>  	i915_gem_retire_requests(dev);
>  
>  	i915_gem_stop_engines(dev);
> +	i915_gem_context_lost(dev_priv);
>  	mutex_unlock(&dev->struct_mutex);
>  
>  	cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index e5acc3916f75..bf31ee1ed914 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -90,6 +90,8 @@
>  #include "i915_drv.h"
>  #include "i915_trace.h"
>  
> +#define ALL_L3_SLICES(dev) (1 << NUM_L3_SLICES(dev)) - 1
> +
>  /* This is a HW constraint. The value below is the largest known requirement
>   * I've seen in a spec to date, and that was a workaround for a non-shipping
>   * part. It should be safe to decrease this, but it's more future proof as is.
> @@ -249,7 +251,7 @@ __create_hw_context(struct drm_device *dev,
>  	/* NB: Mark all slices as needing a remap so that when the context first
>  	 * loads it will restore whatever remap state already exists. If there
>  	 * is no remap info, it will be a NOP. */
> -	ctx->remap_slice = (1 << NUM_L3_SLICES(dev)) - 1;
> +	ctx->remap_slice = ALL_L3_SLICES(dev_priv);
>  
>  	ctx->hang_stats.ban_period_seconds = DRM_I915_CTX_BAN_PERIOD;
>  
> @@ -336,7 +338,6 @@ static void i915_gem_context_unpin(struct intel_context *ctx,
>  void i915_gem_context_reset(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	int i;
>  
>  	if (i915.enable_execlists) {
>  		struct intel_context *ctx;
> @@ -345,17 +346,7 @@ void i915_gem_context_reset(struct drm_device *dev)
>  			intel_lr_context_reset(dev_priv, ctx);
>  	}
>  
> -	for (i = 0; i < I915_NUM_ENGINES; i++) {
> -		struct intel_engine_cs *engine = &dev_priv->engine[i];
> -
> -		if (engine->last_context) {
> -			i915_gem_context_unpin(engine->last_context, engine);
> -			engine->last_context = NULL;
> -		}
> -	}
> -
> -	/* Force the GPU state to be reinitialised on enabling */
> -	dev_priv->kernel_context->legacy_hw_ctx.initialized = false;
> +	i915_gem_context_lost(dev_priv);
>  }
>  
>  int i915_gem_context_init(struct drm_device *dev)
> @@ -403,11 +394,29 @@ int i915_gem_context_init(struct drm_device *dev)
>  	return 0;
>  }
>  
> +void i915_gem_context_lost(struct drm_i915_private *dev_priv)
> +{
> +	struct intel_engine_cs *engine;
> +
> +	for_each_engine(engine, dev_priv) {
> +		if (engine->last_context == NULL)
> +			continue;
> +
> +		i915_gem_context_unpin(engine->last_context, engine);
> +		engine->last_context = NULL;
> +	}
> +
> +	/* Force the GPU state to be reinitialised on enabling */
> +	dev_priv->kernel_context->legacy_hw_ctx.initialized = false;
> +	dev_priv->kernel_context->remap_slice = ALL_L3_SLICES(dev_priv);
> +}
> +
>  void i915_gem_context_fini(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_context *dctx = dev_priv->kernel_context;
> -	int i;
> +
> +	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
> @@ -415,26 +424,9 @@ void i915_gem_context_fini(struct drm_device *dev)
>  		 * other code, leading to spurious errors. */
>  		intel_gpu_reset(dev, ALL_ENGINES);
>  
> -		/* When default context is created and switched to, base object refcount
> -		 * will be 2 (+1 from object creation and +1 from do_switch()).
> -		 * i915_gem_context_fini() will be called after gpu_idle() has switched
> -		 * to default context. So we need to unreference the base object once
> -		 * to offset the do_switch part, so that i915_gem_context_unreference()
> -		 * can then free the base object correctly. */
> -		WARN_ON(!dev_priv->engine[RCS].last_context);
> -
>  		i915_gem_object_ggtt_unpin(dctx->legacy_hw_ctx.rcs_state);
>  	}
>  
> -	for (i = I915_NUM_ENGINES; --i >= 0;) {
> -		struct intel_engine_cs *engine = &dev_priv->engine[i];
> -
> -		if (engine->last_context) {
> -			i915_gem_context_unpin(engine->last_context, engine);
> -			engine->last_context = NULL;
> -		}
> -	}
> -
>  	i915_gem_context_unreference(dctx);
>  	dev_priv->kernel_context = NULL;
>  }
> -- 
> 2.8.0.rc3