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

Mon Jan 16 10:38:39 UTC 2017


On 1/16/2017 3:43 PM, Chris Wilson wrote:
> On Mon, Jan 16, 2017 at 03:08:58PM +0530, Kamble, Sagar A wrote:
>>
>> On 1/16/2017 1:54 PM, Chris Wilson wrote:
>>> On Mon, Jan 16, 2017 at 11:55:10AM +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 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 other than pm_intr_keep.
>>>> Sanitization of PMINTRMSK inside intel_guc_setup should take care of
>>>> scenarios where standalone guc setup is done, e.g deferred load in Android.
>>>>
>>>> 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.
>>>>
>>>> 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>
>>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>>>> ---
>>>>   drivers/gpu/drm/i915/intel_drv.h        |  1 +
>>>>   drivers/gpu/drm/i915/intel_guc_loader.c | 22 ++++++++++++++++++++++
>>>>   drivers/gpu/drm/i915/intel_pm.c         |  2 +-
>>>>   3 files changed, 24 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..e162b23 100644
>>>> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
>>>> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
>>>> @@ -159,6 +159,15 @@ static void guc_interrupts_capture(struct drm_i915_private *dev_priv)
>>>>   		dev_priv->rps.pm_intr_keep |= ~tmp;
>>>>   		dev_priv->rps.pm_intr_keep &= ~GEN8_PMINTR_REDIRECT_TO_GUC;
>>>>   	}
>>>> +
>>>> +	/*
>>>> +	 * Prior to GuC load, interrupts were masked to determine pm_intr_keep
>>>> +	 * needed with GuC. Check if we have RPS interrupts available. Might
>>>> +	 * need to check if other interrupts available too that Host would
>>>> +	 * expect to control. If RPS interrupts routed to GuC in future this
>>>> +	 * WARN needs to be removed.
>>>> +	 */
>>> If we are not doing RPS, then pm_rps_events should not be set. Fix the
>>> comment.
>> yes. will update.
>>>> +	WARN_ON(dev_priv->pm_rps_events & dev_priv->rps.pm_intr_keep);
>>>>   }
>>>>   static u32 get_gttype(struct drm_i915_private *dev_priv)
>>>> @@ -451,6 +460,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
>>>>   	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
>>>>   	const char *fw_path = guc_fw->guc_fw_path;
>>>>   	int retries, ret, err;
>>>> +	u32 pre_load_mask;
>>> Typically called saved_pmintrmsk.
>> will change this.
>>>>   	DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n",
>>>>   		fw_path,
>>>> @@ -484,6 +494,14 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
>>>>   	guc_interrupts_release(dev_priv);
>>>>   	gen9_reset_guc_interrupts(dev_priv);
>>>> +	/*
>>>> +	 * GuC will clear the PM interrupt mask for interrupts it needs. It
>>>> +	 * needs ARAT timer interrupt and might use other interrupts in future.
>>>> +	 * Mask all interrupts to know which interrupts are needed by GuC.
>>> Missed the cross ref.
>> Could you please clarify on cross referencing comment. Is there some
>> syntax for this. Sorry I thought
>> pm_intr_keep related comment in guc_interrupt_capture would suffice.
>>>> +	 */
>>>> +	pre_load_mask = I915_READ(GEN6_PMINTRMSK);
>>>> +	I915_WRITE(GEN6_PMINTRMSK, gen6_sanitize_rps_pm_mask(dev_priv, ~0u));
>>> Again, why not just ~0u since our goal is to capture the hw
>>> requirements, not a mismash of our own.
>> will update this.
>>>> +
>>>>   	/* We need to notify the guc whenever we change the GGTT */
>>>>   	i915_ggtt_enable_guc(dev_priv);
>>>> @@ -539,6 +557,10 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
>>>>   		guc_interrupts_capture(dev_priv);
>>>>   	}
>>>> +	/* Restore mask for interrupts other than those needed by GuC. */
>>>> +	I915_WRITE(GEN6_PMINTRMSK, gen6_sanitize_rps_pm_mask(dev_priv,
>>>> +							     pre_load_mask));
>>> Why? Either restore exactly what was there or recreate it. Don't mix
>>> both. Save + restore is a good idea.
>> using sanitize_rps_pm_mask we are actually recreating our old bits
>> values that are not in pm_intr_keep.
>> so this is restore of bits other than pm_intr_keep. could you please
>> clarify more.
> I realised the same afterwards. I think the sequence should be
>
> WARN_ON(dev_priv->gt.awake);
Could you please clarify why this check is needed.
During reset, when GuC is getting loaded, GT.awake can be true.
> WARN_ON(I915_READ(GEN6_PMINTRMSK) != ~dev_priv->rps.pm_intr_keep);
GEN6_PMINTRMSK will have varying bitmasks based on freq request so very 
likely this WARN will be hit.
> I915_WRITE(GEN6_PMINTRMSK, ~GEN8_PMINTR_REDIRECT_TO_GUC); /* clear for guc_interrupts capture() */
> ...
> I915_WRITE(GEN6_PMINTRMSK, ~dev_priv->rps.pm_intr_keep);
Agree with above two writes logic.
>
> One argument is that the capture logic should be clear, and as tight as
> possible. Just debating whether this messes around with rps
> encapsulation too much - atm, this looks about the right level of
> mixingt the two.
> -Chris
>