[Intel-gfx] [PATCH] drm/i915/guc: Fix request re-submission after reset

[Chris Wilson]

On Thu, Mar 09, 2017 at 10:05:21AM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> 
> In order to ensure no missed interrupts we must first re-direct
> the interrupts to GuC, and only then re-submit the requests to
> be replayed after a GPU reset. Otherwise context switch can fire
> before GuC has been set up to receive it triggering more hangs.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> Cc: Oscar Mateo <oscar.mateo at intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> Cc: Arkadiusz Hiler <arkadiusz.hiler at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_guc_submission.c | 58 +++++++++++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_guc_loader.c    | 47 ------------------------
>  2 files changed, 54 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index beb38e30d0e9..5b8ec0fab563 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -936,6 +936,52 @@ static void guc_reset_wq(struct i915_guc_client *client)
>  	client->wq_tail = 0;
>  }
>  
> +static void guc_interrupts_capture(struct drm_i915_private *dev_priv)
> +{
> +	struct intel_engine_cs *engine;
> +	enum intel_engine_id id;
> +	int irqs;
> +	u32 tmp;
> +
> +	/* tell all command streamers to forward interrupts (but not vblank) to GuC */
> +	irqs = _MASKED_BIT_ENABLE(GFX_INTERRUPT_STEERING);
> +	for_each_engine(engine, dev_priv, id)
> +		I915_WRITE(RING_MODE_GEN7(engine), irqs);
> +
> +	/* route USER_INTERRUPT to Host, all others are sent to GuC. */
> +	irqs = GT_RENDER_USER_INTERRUPT << GEN8_RCS_IRQ_SHIFT |
> +	       GT_RENDER_USER_INTERRUPT << GEN8_BCS_IRQ_SHIFT;
> +	/* These three registers have the same bit definitions */
> +	I915_WRITE(GUC_BCS_RCS_IER, ~irqs);
> +	I915_WRITE(GUC_VCS2_VCS1_IER, ~irqs);
> +	I915_WRITE(GUC_WD_VECS_IER, ~irqs);
> +
> +	/*
> +	 * The REDIRECT_TO_GUC bit of the PMINTRMSK register directs all
> +	 * (unmasked) PM interrupts to the GuC. All other bits of this
> +	 * register *disable* generation of a specific interrupt.
> +	 *
> +	 * 'pm_intr_keep' indicates bits that are NOT to be set when
> +	 * writing to the PM interrupt mask register, i.e. interrupts
> +	 * that must not be disabled.
> +	 *
> +	 * If the GuC is handling these interrupts, then we must not let
> +	 * the PM code disable ANY interrupt that the GuC is expecting.
> +	 * So for each ENABLED (0) bit in this register, we must SET the
> +	 * bit in pm_intr_keep so that it's left enabled for the GuC.
> +	 *
> +	 * OTOH the REDIRECT_TO_GUC bit is initially SET in pm_intr_keep
> +	 * (so interrupts go to the DISPLAY unit at first); but here we
> +	 * need to CLEAR that bit, which will result in the register bit
> +	 * being left SET!
> +	 */
> +	tmp = I915_READ(GEN6_PMINTRMSK);
> +	if (tmp & GEN8_PMINTR_REDIRECT_TO_GUC) {
> +		dev_priv->rps.pm_intr_keep |= ~tmp;
> +		dev_priv->rps.pm_intr_keep &= ~GEN8_PMINTR_REDIRECT_TO_GUC;
> +	}

Let me just remove this code first (drm/i915: Initialize pm_intr_keep
during intel_irq_init for GuC) depending on the outcome of
https://patchwork.freedesktop.org/series/20980/.

> +}
> +
>  int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_guc *guc = &dev_priv->guc;
> @@ -953,13 +999,17 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
>  
>  	/* Take over from manual control of ELSP (execlists) */
>  	for_each_engine(engine, dev_priv, id) {
> -		const int wqi_size = sizeof(struct guc_wq_item);
> -		struct drm_i915_gem_request *rq;
> -
>  		engine->submit_request = i915_guc_submit;
>  		engine->schedule = NULL;
> +	}
> +
> +	guc_interrupts_capture(dev_priv);
> +
> +	/* Replay the current set of previously submitted requests */
> +	for_each_engine(engine, dev_priv, id) {
> +		const int wqi_size = sizeof(struct guc_wq_item);
> +		struct drm_i915_gem_request *rq;
>  
> -		/* Replay the current set of previously submitted requests */
>  		spin_lock_irq(&engine->timeline->lock);
>  		list_for_each_entry(rq, &engine->timeline->requests, link) {
>  			guc_client_update_wq_rsvd(client, wqi_size);

>  static u32 get_gttype(struct drm_i915_private *dev_priv)
>  {
>  	/* XXX: GT type based on PCI device ID? field seems unused by fw */
> @@ -529,7 +483,6 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
>  		err = i915_guc_submission_enable(dev_priv);
>  		if (err)
>  			goto fail;
> -		guc_interrupts_capture(dev_priv);
>  	}

Looks good to me.
R-b for v2 after Sagar :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre