| Message ID | 1466090389-17469-1-git-send-email-michal.winiarski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@chris-wilson.co.uk> > Cc: Imre Deak <imre.deak@intel.com> > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> > --- > 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
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@chris-wilson.co.uk> > Cc: Imre Deak <imre.deak@intel.com> > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> > --- > 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); > > 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) { > + intel_set_rps(dev_priv, dev_priv->rps.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); > valleyview_set_rps(dev_priv, val); > + intel_uncore_forcewake_put(dev_priv, FORCEWAKE_MEDIA); Why should we take the extra hit from waking the media well for every RPS changes? > + } > else > gen6_set_rps(dev_priv, val); > } > -- > 2.8.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Jun 16, 2016 at 04:42:30PM +0100, Chris Wilson wrote: > On Thu, Jun 16, 2016 at 05:19:49PM +0200, Michał Winiarski wrote: > > 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 Actually thinking something like intel_set_rps(dev_priv, clamp(dev_priv->rps.cur_freq, dev_priv->rps.min_freq_softlimit, dev_priv->rps.max_freq_softlimit)); will do the trick. A request to set cur_freq will be filtered out by intel_set_rps. -Chris
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); 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) { + intel_set_rps(dev_priv, dev_priv->rps.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); valleyview_set_rps(dev_priv, val); + intel_uncore_forcewake_put(dev_priv, FORCEWAKE_MEDIA); + } else gen6_set_rps(dev_priv, val); }
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@chris-wilson.co.uk> Cc: Imre Deak <imre.deak@intel.com> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 33 +++++++-------------------------- 1 file changed, 7 insertions(+), 26 deletions(-)