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

[Kamble, Sagar A]

On 1/14/2017 1:41 AM, Chris Wilson wrote:
> 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.
Floating new patch with more elaborate comments here. RPS interrupt 
mask/uncore to core trap might be used in
future with SLPC.
>
>>   	/* 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)) ?
Yes. Added WARN for this in guc_interrupts_capture to check based on 
pm_intr_keep which should be equivalent.
Writing mask here to not depend on enable_guc_submission and to not 
cache the mask prior to load for access
in guc_interrupts_capture.
> -Chris
>