On Thu, Jun 16, 2016 at 05:19:49PM +0200, Michał Winiarski wrote: > If the GPU load is low enough, it's possible that we'll be stuck at idle > frequency rather than transition into softmin frequency requested by > userspace. > Since we assume that idle_freq <= min_freq_softlimit and > valleyview_set_rps is already skipping write when > requested_freq == cur_freq we can also remove vlv_set_idle function. > > v2: Use intel_set_rps, drop vlv_set_idle > > References: https://bugs.freedesktop.org/show_bug.cgi?id=89728 > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > Cc: Imre Deak <imre.deak@xxxxxxxxx> > Signed-off-by: Michał Winiarski <michal.winiarski@xxxxxxxxx> > --- > drivers/gpu/drm/i915/intel_pm.c | 33 +++++++-------------------------- > 1 file changed, 7 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 658a756..41c5d25 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -4798,6 +4798,7 @@ static void valleyview_set_rps(struct drm_i915_private *dev_priv, u8 val) > WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock)); > WARN_ON(val > dev_priv->rps.max_freq); > WARN_ON(val < dev_priv->rps.min_freq); > + WARN_ON(val < dev_priv->rps.idle_freq); The hw range is min_freq, max_freq. We assert earlier if idle_freq is out of range. > > if (WARN_ONCE(IS_CHERRYVIEW(dev_priv) && (val & 1), > "Odd GPU freq value\n")) > @@ -4815,31 +4816,11 @@ static void valleyview_set_rps(struct drm_i915_private *dev_priv, u8 val) > trace_intel_gpu_freq_change(intel_gpu_freq(dev_priv, val)); > } > > -/* vlv_set_rps_idle: Set the frequency to idle, if Gfx clocks are down > - * > - * * If Gfx is Idle, then > - * 1. Forcewake Media well. > - * 2. Request idle freq. > - * 3. Release Forcewake of Media well. > -*/ > -static void vlv_set_rps_idle(struct drm_i915_private *dev_priv) > -{ > - u32 val = dev_priv->rps.idle_freq; > - > - if (dev_priv->rps.cur_freq <= val) > - return; > - > - /* Wake up the media well, as that takes a lot less > - * power than the Render well. */ > - intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA); > - valleyview_set_rps(dev_priv, val); > - intel_uncore_forcewake_put(dev_priv, FORCEWAKE_MEDIA); > -} > - > void gen6_rps_busy(struct drm_i915_private *dev_priv) > { > mutex_lock(&dev_priv->rps.hw_lock); > if (dev_priv->rps.enabled) { /* Ensure we start at the user's desired minimum frequency */ > + intel_set_rps(dev_priv, dev_priv->rps.min_freq_softlimit); Only if cur_freq < min_freq_softlimit > if (dev_priv->pm_rps_events & (GEN6_PM_RP_DOWN_EI_EXPIRED | GEN6_PM_RP_UP_EI_EXPIRED)) > gen6_rps_reset_ei(dev_priv); > I915_WRITE(GEN6_PMINTRMSK, > @@ -4852,10 +4833,7 @@ void gen6_rps_idle(struct drm_i915_private *dev_priv) > { > mutex_lock(&dev_priv->rps.hw_lock); > if (dev_priv->rps.enabled) { > - if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > - vlv_set_rps_idle(dev_priv); > - else > - gen6_set_rps(dev_priv, dev_priv->rps.idle_freq); > + intel_set_rps(dev_priv, dev_priv->rps.idle_freq); > dev_priv->rps.last_adj = 0; > I915_WRITE(GEN6_PMINTRMSK, 0xffffffff); > } > @@ -4905,8 +4883,11 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv, > > void intel_set_rps(struct drm_i915_private *dev_priv, u8 val) > { > - if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > + if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) { > + intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA); And that is bogus. The comment that was removed doesn't actually reflect what the code was doing! Lowlevel set_rps itself is doing forcewake gets, using whatever domain is actually required to access the registers. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
