Re: [PATCH 07/19] drm/i915/execlists: Handle copying default context state for atomic reset

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

On 17/05/2018 08:40, Chris Wilson wrote:
> We want to be able to reset the GPU from inside a timer callback
> (hardirq context). One step requires us to copy the default context
> state over to the guilty context, which means we need to plan in advance
> to have that object accessible from within an atomic context. The atomic
> context prevents us from pinning the object or in peeking into the
> shmemfs backing store (all may sleep), so we choose to pin the
> default_state into memory when the engine becomes active. This
> compromise allows us to swap out the default state when idle, if
> required.
> 
> References: 5692251c254a ("drm/i915/lrc: Scrub the GPU state of the guilty hanging request")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/intel_engine_cs.c  | 15 +++++++++++++++
>   drivers/gpu/drm/i915/intel_lrc.c        | 15 ++++-----------
>   drivers/gpu/drm/i915/intel_ringbuffer.h |  1 +
>   3 files changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 333318b340e1..b1a1ca0758ce 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -1082,6 +1082,11 @@ void intel_engines_park(struct drm_i915_private *i915)
>   		if (engine->park)
>   			engine->park(engine);
>   
> +		if (engine->pinned_default_state) {
> +			i915_gem_object_unpin_map(engine->default_state);
> +			engine->pinned_default_state = NULL;
> +		}
> +
>   		i915_gem_batch_pool_fini(&engine->batch_pool);
>   		engine->execlists.no_priolist = false;
>   	}
> @@ -1099,6 +1104,16 @@ void intel_engines_unpark(struct drm_i915_private *i915)
>   	enum intel_engine_id id;
>   
>   	for_each_engine(engine, i915, id) {
> +		void *map;
> +
> +		/* Pin the default state for fast resets from atomic context. */
> +		map = NULL;
> +		if (engine->default_state)
> +			map = i915_gem_object_pin_map(engine->default_state,
> +						      I915_MAP_WB);
> +		if (!IS_ERR_OR_NULL(map))
> +			engine->pinned_default_state = map;
> +
>   		if (engine->unpark)
>   			engine->unpark(engine);
>   
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index f3470b95d64e..49283b3d3ebb 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1966,17 +1966,10 @@ static void execlists_reset(struct intel_engine_cs *engine,
>   	 * to recreate its own state.
>   	 */
>   	regs = request->hw_context->lrc_reg_state;
> -	if (engine->default_state) {
> -		void *defaults;
> -
> -		defaults = i915_gem_object_pin_map(engine->default_state,
> -						   I915_MAP_WB);
> -		if (!IS_ERR(defaults)) {
> -			memcpy(regs, /* skip restoring the vanilla PPHWSP */
> -			       defaults + LRC_STATE_PN * PAGE_SIZE,
> -			       engine->context_size - PAGE_SIZE);
> -			i915_gem_object_unpin_map(engine->default_state);
> -		}
> +	if (engine->pinned_default_state) {
> +		memcpy(regs, /* skip restoring the vanilla PPHWSP */
> +		       engine->pinned_default_state + LRC_STATE_PN * PAGE_SIZE,
> +		       engine->context_size - PAGE_SIZE);
>   	}
>   	execlists_init_reg_state(regs,
>   				 request->gem_context, engine, request->ring);
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 20c4e13efc0d..acef385c4c80 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -342,6 +342,7 @@ struct intel_engine_cs {
>   	struct i915_timeline timeline;
>   
>   	struct drm_i915_gem_object *default_state;
> +	void *pinned_default_state;
>   
>   	atomic_t irq_count;
>   	unsigned long irq_posted;
> 

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