[PATCH 1/3] drm/i915: Handle GEN6_PMINTRMSK for proper masking of RPS interrupts with GuC submission

[Chris Wilson]

On Fri, Jan 13, 2017 at 03:46:10PM +0530, Sagar Arun Kamble wrote:
> From: "sagar.a.kamble at intel.com" <sagar.a.kamble at intel.com>
> 
> Driver needs to ensure that it doesn't mask the PM interrupts, which are
> unmasked/needed by GuC firmware. For that, Driver maintains a bitmask of
> interrupts to be kept unmasked, pm_intr_keep.
> 
> By default, RP Up/Down Threshold Interrupt bits (and others) in
> GEN6_PMINTRMSK register were unmasked (by BIOS) before GuC Load. Hence
> Driver was keeping them unmasked always assuming GuC needed them.
> As an optimization, Driver needs to mask these bits in order to
> avoid redundant UP threshold interrupts when frequency is set to maximum,
> RP0 or Down threshold interrupts when frequency is set to minimum, RPn.
> 
> This patch solves bunch of SKL GT3 performance regressions as Host RPS is
> taking frequency down when GuC is enabled. This is happening due to signed
> last_adj that becomes negative after 32 UP interrupts and makes
> cur_freq=min starting the ramp again.
> 
> This patch will mask all bits in GEN6_PMINTRMSK before GuC is loaded to
> ensure pm_intr_keep reflects only interrupts that are needed by GuC.
> Post GuC load, restore bits in GEN6_PMINTRMSK based on current frequency.
> Sanitization of PMINTRMSK inside intel_guc_setup should take care of
> scenarios where standalone guc setup is done, e.g deferred load in Android.

> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Szwichtenberg, Radoslaw <radoslaw.szwichtenberg at intel.com>
> Cc: Tomi Sarvela <tomi.p.sarvela at intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97017
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h        |  1 +
>  drivers/gpu/drm/i915/intel_guc_loader.c | 14 ++++++++++++++
>  drivers/gpu/drm/i915/intel_pm.c         |  2 +-
>  3 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 84258df..2651dae 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1757,6 +1757,7 @@ bool chv_phy_powergate_ch(struct drm_i915_private *dev_priv, enum dpio_phy phy,
>  void intel_autoenable_gt_powersave(struct drm_i915_private *dev_priv);
>  void intel_disable_gt_powersave(struct drm_i915_private *dev_priv);
>  void intel_suspend_gt_powersave(struct drm_i915_private *dev_priv);
> +u32 gen6_rps_pm_mask(struct drm_i915_private *dev_priv, u8 val);
>  void gen6_rps_busy(struct drm_i915_private *dev_priv);
>  void gen6_rps_reset_ei(struct drm_i915_private *dev_priv);
>  void gen6_rps_idle(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index b889191..bb475d6 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -484,6 +484,13 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
>  	guc_interrupts_release(dev_priv);
>  	gen9_reset_guc_interrupts(dev_priv);
>  
> +	/*
> +	 * Mask all interrupts to know which interrupts are needed by GuC.
> +	 * Restore host side interrupt masks post load.
> +	 */
> +	I915_WRITE(GEN6_PMINTRMSK, gen6_sanitize_rps_pm_mask(dev_priv, ~0u));

Hmm, here you do want the full ~0u, right? The question being asked is
what bits are being cleared by the GuC when it loads, as those are the
ones we need to keep.

If you can add to the comment above to clarify that the GuC is
manipulating this register. It is not mentioned explicitly here or
guc_interrupts_capture(). Also cross-referencing this comment to
guc_interrupts_capture() would be useful.

>  	/* We need to notify the guc whenever we change the GGTT */
>  	i915_ggtt_enable_guc(dev_priv);
>  
> @@ -539,6 +546,13 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
>  		guc_interrupts_capture(dev_priv);
>  	}
>  
> +	/*
> +	 * Below write will ensure mask for RPS interrupts is restored back
> +	 * w.r.t cur_freq, particularly post reset.
> +	 */
> +	I915_WRITE(GEN6_PMINTRMSK,
> +		   gen6_rps_pm_mask(dev_priv, dev_priv->rps.cur_freq));

Should we not be now checking pm_intr_keep to see if we have the RPS
interrupts available?
WARN_ON(dev_priv->pm_rps_events & I915_READ(PMINTRMSK)) ?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre