Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx> > Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx> > Cc: <stable@xxxxxxxxxxxxxxx> # v4.1+ > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > --- > 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