Re: [PATCH 1/2] drm/i915: Stop using RP_DOWN_EI on Baytrail

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> On Baytrail, we manually calculate busyness over the evaluation interval
> to avoid issues with miscaluations with RC6 enabled. However, it turns
> out that the DOWN_EI interrupt generator is completely bust - it
> operates in two modes, continuous or never. Neither of which are
> conducive to good behaviour. Stop unmask the DOWN_EI interrupt and just
> compute everything from the UP_EI which does seem to correspond to the
> desired interval.
>
> v2: Fixup gen6_rps_pm_mask() as well
>
> Fixes: 43cf3bf084ba ("drm/i915: Improved w/a for rps on Baytrail")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: <stable@vger.kernel.org> # v4.1+
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  2 +-
>  drivers/gpu/drm/i915/i915_irq.c | 28 +++++++++-------------------
>  drivers/gpu/drm/i915/intel_pm.c |  5 +++--
>  3 files changed, 13 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 9c5b3dd9f6f1..d70bbbd6a5fd 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1366,7 +1366,7 @@ struct intel_gen6_power_mgmt {
>  	unsigned boosts;
>  
>  	/* manual wa residency calculations */
> -	struct intel_rps_ei up_ei, down_ei;
> +	struct intel_rps_ei ei;
>  
>  	/*
>  	 * Protects RPS/RC6 register access and PCU communication.
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index bc70e2c451b2..29b10bab38b6 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1050,10 +1050,10 @@ static void vlv_c0_read(struct drm_i915_private *dev_priv,
>  }
>  
>  static bool vlv_c0_above(struct drm_i915_private *dev_priv,
> -			 const struct intel_rps_ei *old,
>  			 const struct intel_rps_ei *now,
>  			 int threshold)
>  {
> +	const struct intel_rps_ei *old = &dev_priv->rps.ei;
>  	u64 time, c0;
>  	unsigned int mul = 100;
>  
> @@ -1079,8 +1079,7 @@ static bool vlv_c0_above(struct drm_i915_private *dev_priv,
>  
>  void gen6_rps_reset_ei(struct drm_i915_private *dev_priv)
>  {
> -	vlv_c0_read(dev_priv, &dev_priv->rps.down_ei);
> -	dev_priv->rps.up_ei = dev_priv->rps.down_ei;
> +	memset(&dev_priv->rps.ei, 0, sizeof(dev_priv->rps.ei));

With this change you will always downclock with the first change
after reset. Is this desired? (due to busy handling the rampup?).

I would have made it so that vlv_c0_above returns true if
the value was reset. Then we would be on the safe side going
upwards for the first tick.

-Mika


>  }
>  
>  static u32 vlv_wa_c0_ei(struct drm_i915_private *dev_priv, u32 pm_iir)
> @@ -1088,28 +1087,19 @@ static u32 vlv_wa_c0_ei(struct drm_i915_private *dev_priv, u32 pm_iir)
>  	struct intel_rps_ei now;
>  	u32 events = 0;
>  
> -	if ((pm_iir & (GEN6_PM_RP_DOWN_EI_EXPIRED | GEN6_PM_RP_UP_EI_EXPIRED)) == 0)
> +	if ((pm_iir & GEN6_PM_RP_UP_EI_EXPIRED) == 0)
>  		return 0;
>  
>  	vlv_c0_read(dev_priv, &now);
>  	if (now.cz_clock == 0)
>  		return 0;
>  
> -	if (pm_iir & GEN6_PM_RP_DOWN_EI_EXPIRED) {
> -		if (!vlv_c0_above(dev_priv,
> -				  &dev_priv->rps.down_ei, &now,
> -				  dev_priv->rps.down_threshold))
> -			events |= GEN6_PM_RP_DOWN_THRESHOLD;
> -		dev_priv->rps.down_ei = now;
> -	}
> +	if (vlv_c0_above(dev_priv, &now, dev_priv->rps.up_threshold))
> +		events |= GEN6_PM_RP_UP_THRESHOLD;
> +	else if (!vlv_c0_above(dev_priv, &now, dev_priv->rps.down_threshold))
> +		events |= GEN6_PM_RP_DOWN_THRESHOLD;
>  
> -	if (pm_iir & GEN6_PM_RP_UP_EI_EXPIRED) {
> -		if (vlv_c0_above(dev_priv,
> -				 &dev_priv->rps.up_ei, &now,
> -				 dev_priv->rps.up_threshold))
> -			events |= GEN6_PM_RP_UP_THRESHOLD;
> -		dev_priv->rps.up_ei = now;
> -	}
> +	dev_priv->rps.ei = now;
>  
>  	return events;
>  }
> @@ -4250,7 +4240,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
>  	/* Let's track the enabled rps events */
>  	if (IS_VALLEYVIEW(dev_priv))
>  		/* WaGsvRC0ResidencyMethod:vlv */
> -		dev_priv->pm_rps_events = GEN6_PM_RP_DOWN_EI_EXPIRED | GEN6_PM_RP_UP_EI_EXPIRED;
> +		dev_priv->pm_rps_events = GEN6_PM_RP_UP_EI_EXPIRED;
>  	else
>  		dev_priv->pm_rps_events = GEN6_PM_RPS_EVENTS;
>  
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index ac0cd82f61e5..56472905d782 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4916,8 +4916,9 @@ static u32 gen6_rps_pm_mask(struct drm_i915_private *dev_priv, u8 val)
>  {
>  	u32 mask = 0;
>  
> +	/* We use UP_EI_EXPIRED interupts for both up/down in manual mode */
>  	if (val > dev_priv->rps.min_freq_softlimit)
> -		mask |= GEN6_PM_RP_DOWN_EI_EXPIRED | GEN6_PM_RP_DOWN_THRESHOLD | GEN6_PM_RP_DOWN_TIMEOUT;
> +		mask |= GEN6_PM_RP_UP_EI_EXPIRED | GEN6_PM_RP_DOWN_THRESHOLD | GEN6_PM_RP_DOWN_TIMEOUT;
>  	if (val < dev_priv->rps.max_freq_softlimit)
>  		mask |= GEN6_PM_RP_UP_EI_EXPIRED | GEN6_PM_RP_UP_THRESHOLD;
>  
> @@ -5027,7 +5028,7 @@ void gen6_rps_busy(struct drm_i915_private *dev_priv)
>  	if (dev_priv->rps.enabled) {
>  		u8 freq;
>  
> -		if (dev_priv->pm_rps_events & (GEN6_PM_RP_DOWN_EI_EXPIRED | GEN6_PM_RP_UP_EI_EXPIRED))
> +		if (dev_priv->pm_rps_events & GEN6_PM_RP_UP_EI_EXPIRED)
>  			gen6_rps_reset_ei(dev_priv);
>  		I915_WRITE(GEN6_PMINTRMSK,
>  			   gen6_rps_pm_mask(dev_priv, dev_priv->rps.cur_freq));
> -- 
> 2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx