[PATCH 3/4] drm/i915/guc: Update rps.intr_keep_unmasked in guc_interrupts_capture/release

[Chris Wilson]

On Fri, Mar 10, 2017 at 10:22:27PM +0530, Sagar Arun Kamble wrote:
> Different state is to be maintained for rps.intr_keep_unmasked for GuC and
> Execlists. Updating it inside guc_interrupts_* routines as in those
> routines GuC load/submission params are sanitized and it should not be set
> based on HAS_GUC_SCHED during intel_irq_init.
> 
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Oscar Mateo <oscar.mateo at intel.com>
> Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg at intel.com>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_guc_submission.c | 29 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_irq.c            | 24 ------------------------
>  2 files changed, 29 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 2282d83..314880f 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -954,6 +954,30 @@ static void guc_interrupts_capture(struct drm_i915_private *dev_priv)
>  	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.
> +	 *
> +	 * 'intr_keep_unmasked' 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 intr_keep_unmasked so that it's left enabled for the GuC.
> +	 * GuC needs ARAT expired interrupt unmasked hence it is set in
> +	 * intr_keep_unmasked.
> +	 *
> +	 * Here we CLEAR REDIRECT_TO_GUC bit in intr_keep_unmasked, which will
> +	 * result in the register bit being left SET!
> +	 */
> +	if (HAS_GUC_SCHED(dev_priv)) {

Cen we get here without HAS_GUC_SCHED() ?

> +		dev_priv->rps.intr_keep_unmasked |= ARAT_EXPIRED_INTRMSK;
> +		dev_priv->rps.intr_keep_unmasked &= ~GEN8_PMINTR_REDIRECT_TO_GUC;
> +	}
>  }
>  
>  int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
> @@ -1011,6 +1035,11 @@ static void guc_interrupts_release(struct drm_i915_private *dev_priv)
>  	I915_WRITE(GUC_BCS_RCS_IER, 0);
>  	I915_WRITE(GUC_VCS2_VCS1_IER, 0);
>  	I915_WRITE(GUC_WD_VECS_IER, 0);
> +
> +	dev_priv->rps.intr_keep_unmasked = 0;
> +
> +	if (INTEL_INFO(dev_priv)->gen >= 8)
> +		dev_priv->rps.intr_keep_unmasked |= GEN8_PMINTR_REDIRECT_TO_GUC;

Here we can safely say that should be unconditional.

I think symmetry would be better, i.e.
dev_priv->rps.intr_keep_unmasked |= GEN8_PMINTR_REDIRECT_TO_GUC;
dev_priv->rps.intr_keep_unmasked &= ~ARAT_EXPIRED_INTRMSK;

Looks ok to me here. We did at one point talk about trying to keep the
rps together, but adding entry points just to set/clear these bits seems
overkill right now.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre