[Intel-gfx] [PATCH 1/2] drm/i915: Make sure engines are idle during GPU idling in LR mode

[Tvrtko Ursulin]

On 03/11/2016 16:19, Imre Deak wrote:
> We assume that the GPU is idle once receiving the seqno via the last
> request's user interrupt. In execlist mode the corresponding context
> completed interrupt can be delayed though and until this latter
> interrupt arrives we consider the request to be pending on the ELSP
> submit port. This can cause a problem during system suspend where this
> last request will be seen by the resume code as still pending. Such
> pending requests are normally replayed after a GPU reset, but during
> resume we reset both SW and HW tracking of the ring head/tail pointers,
> so replaying the pending request with its stale tale pointer will leave
> the ring in an inconsistent state. A subsequent request submission can
> lead then to the GPU executing from uninitialized area in the ring
> behind the above stale tail pointer.
>
> Fix this by making sure any pending request on the ELSP port is
> completed before suspending. I used a polling wait since the completion
> time I measured was <1ms and since normally we only need to wait during
> system suspend. GPU idling during runtime suspend is scheduled with a
> delay (currently 50-100ms) after the retirement of the last request at
> which point the context completed interrupt must have arrived already.
>
> The chance of this bug was increased by
>
> commit 1c777c5d1dcdf8fa0223fcff35fb387b5bb9517a
> Author: Imre Deak <imre.deak at intel.com>
> Date:   Wed Oct 12 17:46:37 2016 +0300
>
>     drm/i915/hsw: Fix GPU hang during resume from S3-devices state
>
> but it could happen even without the explicit GPU reset, since we
> disable interrupts afterwards during the suspend sequence.
>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala at intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98470
> Signed-off-by: Imre Deak <imre.deak at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c  |  3 +++
>  drivers/gpu/drm/i915/intel_lrc.c | 12 ++++++++++++
>  drivers/gpu/drm/i915/intel_lrc.h |  1 +
>  3 files changed, 16 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 1f995ce..5ff02b5 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2766,6 +2766,9 @@ i915_gem_idle_work_handler(struct work_struct *work)
>  	if (dev_priv->gt.active_requests)
>  		goto out_unlock;
>
> +	if (i915.enable_execlists)
> +		intel_lr_wait_engines_idle(dev_priv);
> +
>  	for_each_engine(engine, dev_priv, id)
>  		i915_gem_batch_pool_fini(&engine->batch_pool);
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index fa3012c..ee4aaf1 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -522,6 +522,18 @@ static bool execlists_elsp_idle(struct intel_engine_cs *engine)
>  	return !engine->execlist_port[0].request;
>  }
>
> +void intel_lr_wait_engines_idle(struct drm_i915_private *dev_priv)
> +{
> +	struct intel_engine_cs *engine;
> +	enum intel_engine_id id;
> +
> +	for_each_engine(engine, dev_priv, id) {
> +		if (wait_for(execlists_elsp_idle(engine), 10))
> +			DRM_ERROR("Timeout waiting for engine %s to idle\n",
> +				  engine->name);

Just noticed engine names are currently like "render ring",etc, so you 
can drop the 'engine' from the message.

> +	}
> +}
> +
>  static bool execlists_elsp_ready(struct intel_engine_cs *engine)
>  {
>  	int port;
> diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
> index 4fed816..bf3775e 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.h
> +++ b/drivers/gpu/drm/i915/intel_lrc.h
> @@ -87,6 +87,7 @@ void intel_lr_context_unpin(struct i915_gem_context *ctx,
>
>  struct drm_i915_private;
>
> +void intel_lr_wait_engines_idle(struct drm_i915_private *dev_priv);
>  void intel_lr_context_resume(struct drm_i915_private *dev_priv);
>  uint64_t intel_lr_context_descriptor(struct i915_gem_context *ctx,
>  				     struct intel_engine_cs *engine);
>

Regards,

Tvrtko