Message ID | 1383753389-12763-3-git-send-email-rodrigo.vivi@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Wed, Nov 06, 2013 at 01:56:26PM -0200, Rodrigo Vivi wrote: > From: Chris Wilson <chris@chris-wilson.co.uk> > > The RPS register writing routines use the current value of min/max to > set certain limits and interrupt gating. If we set those afterwards, we > risk setting up the hw incorrectly and losing power management events, > and worse, trigger some internal assertions. > > Reorder the calling sequences to be correct, and remove the then > unrequired clamping from inside set_rps(). And for a bonus, fix the bug > of calling gen6_set_rps() from Valleyview. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> > Reviewer: Ville Syrjälä <ville.syrjala@linux.intel.com> > CC: Ville Syrjälä <ville.syrjala@linux.intel.com> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 2 +- > drivers/gpu/drm/i915/i915_sysfs.c | 16 ++++++++-------- > drivers/gpu/drm/i915/intel_pm.c | 19 +++++-------------- > 3 files changed, 14 insertions(+), 23 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 7008aac..5b28602 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -2758,7 +2758,7 @@ i915_max_freq_set(void *data, u64 val) > if (IS_VALLEYVIEW(dev)) { > val = vlv_freq_opcode(dev_priv->mem_freq, val); > dev_priv->rps.max_delay = val; > - gen6_set_rps(dev, val); > + valleyview_set_rps(dev, val); > } else { > do_div(val, GT_FREQUENCY_MULTIPLIER); > dev_priv->rps.max_delay = val; > diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c > index cef38fd..46291c4 100644 > --- a/drivers/gpu/drm/i915/i915_sysfs.c > +++ b/drivers/gpu/drm/i915/i915_sysfs.c > @@ -342,15 +342,15 @@ static ssize_t gt_max_freq_mhz_store(struct device *kdev, > DRM_DEBUG("User requested overclocking to %d\n", > val * GT_FREQUENCY_MULTIPLIER); > > + dev_priv->rps.max_delay = val; > + > if (dev_priv->rps.cur_delay > val) { > - if (IS_VALLEYVIEW(dev_priv->dev)) > - valleyview_set_rps(dev_priv->dev, val); > + if (IS_VALLEYVIEW(dev)) > + valleyview_set_rps(dev, val); > else > - gen6_set_rps(dev_priv->dev, val); > + gen6_set_rps(dev, val); > } > > - dev_priv->rps.max_delay = val; > - > mutex_unlock(&dev_priv->rps.hw_lock); > > return count; > @@ -411,15 +411,15 @@ static ssize_t gt_min_freq_mhz_store(struct device *kdev, > return -EINVAL; > } > > + dev_priv->rps.min_delay = val; > + > if (dev_priv->rps.cur_delay < val) { > if (IS_VALLEYVIEW(dev)) > valleyview_set_rps(dev, val); > else > - gen6_set_rps(dev_priv->dev, val); > + gen6_set_rps(dev, val); > } > > - dev_priv->rps.min_delay = val; > - > mutex_unlock(&dev_priv->rps.hw_lock); > > return count; > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 09ac9e7..830865e 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3414,26 +3414,19 @@ static void ironlake_disable_drps(struct drm_device *dev) > * ourselves, instead of doing a rmw cycle (which might result in us clearing > * all limits and the gpu stuck at whatever frequency it is at atm). > */ > -static u32 gen6_rps_limits(struct drm_i915_private *dev_priv, u8 *val) > +static u32 gen6_rps_limits(struct drm_i915_private *dev_priv, u8 val) > { > u32 limits; > > - limits = 0; > - > - if (*val >= dev_priv->rps.max_delay) > - *val = dev_priv->rps.max_delay; > - limits |= dev_priv->rps.max_delay << 24; > - > /* Only set the down limit when we've reached the lowest level to avoid > * getting more interrupts, otherwise leave this clear. This prevents a > * race in the hw when coming out of rc6: There's a tiny window where > * the hw runs at the minimal clock before selecting the desired > * frequency, if the down threshold expires in that window we will not > * receive a down interrupt. */ > - if (*val <= dev_priv->rps.min_delay) { > - *val = dev_priv->rps.min_delay; > + limits = dev_priv->rps.max_delay << 24; > + if (val <= dev_priv->rps.min_delay) > limits |= dev_priv->rps.min_delay << 16; > - } > > return limits; > } > @@ -3533,7 +3526,6 @@ static void gen6_set_rps_thresholds(struct drm_i915_private *dev_priv, u8 val) > void gen6_set_rps(struct drm_device *dev, u8 val) > { > struct drm_i915_private *dev_priv = dev->dev_private; > - u32 limits = gen6_rps_limits(dev_priv, &val); > > WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock)); > WARN_ON(val > dev_priv->rps.max_delay); > @@ -3556,7 +3548,8 @@ void gen6_set_rps(struct drm_device *dev, u8 val) > /* Make sure we continue to get interrupts > * until we hit the minimum or maximum frequencies. > */ > - I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, limits); > + I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, > + gen6_rps_limits(dev_priv, val)); > > POSTING_READ(GEN6_RPNSWREQ); > > @@ -3620,8 +3613,6 @@ void valleyview_set_rps(struct drm_device *dev, u8 val) > { > struct drm_i915_private *dev_priv = dev->dev_private; > > - gen6_rps_limits(dev_priv, &val); > - > WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock)); > WARN_ON(val > dev_priv->rps.max_delay); > WARN_ON(val < dev_priv->rps.min_delay); > -- > 1.8.3.1
On Thu, Nov 07, 2013 at 07:43:48PM +0200, Ville Syrjälä wrote: > On Wed, Nov 06, 2013 at 01:56:26PM -0200, Rodrigo Vivi wrote: > > From: Chris Wilson <chris@chris-wilson.co.uk> > > > > The RPS register writing routines use the current value of min/max to > > set certain limits and interrupt gating. If we set those afterwards, we > > risk setting up the hw incorrectly and losing power management events, > > and worse, trigger some internal assertions. > > > > Reorder the calling sequences to be correct, and remove the then > > unrequired clamping from inside set_rps(). And for a bonus, fix the bug > > of calling gen6_set_rps() from Valleyview. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> > > Reviewer: Ville Syrjälä <ville.syrjala@linux.intel.com> > > CC: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > drivers/gpu/drm/i915/i915_debugfs.c | 2 +- > > drivers/gpu/drm/i915/i915_sysfs.c | 16 ++++++++-------- > > drivers/gpu/drm/i915/intel_pm.c | 19 +++++-------------- > > 3 files changed, 14 insertions(+), 23 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > > index 7008aac..5b28602 100644 > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > @@ -2758,7 +2758,7 @@ i915_max_freq_set(void *data, u64 val) > > if (IS_VALLEYVIEW(dev)) { > > val = vlv_freq_opcode(dev_priv->mem_freq, val); > > dev_priv->rps.max_delay = val; > > - gen6_set_rps(dev, val); > > + valleyview_set_rps(dev, val); > > } else { > > do_div(val, GT_FREQUENCY_MULTIPLIER); > > dev_priv->rps.max_delay = val; > > diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c > > index cef38fd..46291c4 100644 > > --- a/drivers/gpu/drm/i915/i915_sysfs.c > > +++ b/drivers/gpu/drm/i915/i915_sysfs.c > > @@ -342,15 +342,15 @@ static ssize_t gt_max_freq_mhz_store(struct device *kdev, > > DRM_DEBUG("User requested overclocking to %d\n", > > val * GT_FREQUENCY_MULTIPLIER); > > > > + dev_priv->rps.max_delay = val; > > + > > if (dev_priv->rps.cur_delay > val) { > > - if (IS_VALLEYVIEW(dev_priv->dev)) > > - valleyview_set_rps(dev_priv->dev, val); > > + if (IS_VALLEYVIEW(dev)) > > + valleyview_set_rps(dev, val); > > else > > - gen6_set_rps(dev_priv->dev, val); > > + gen6_set_rps(dev, val); Extracting all these IS_VLV blocks and having one abstract entry point would be a nice follow-up ... Queued for -next, thanks for the patch. -Daniel > > } > > > > - dev_priv->rps.max_delay = val; > > - > > mutex_unlock(&dev_priv->rps.hw_lock); > > > > return count; > > @@ -411,15 +411,15 @@ static ssize_t gt_min_freq_mhz_store(struct device *kdev, > > return -EINVAL; > > } > > > > + dev_priv->rps.min_delay = val; > > + > > if (dev_priv->rps.cur_delay < val) { > > if (IS_VALLEYVIEW(dev)) > > valleyview_set_rps(dev, val); > > else > > - gen6_set_rps(dev_priv->dev, val); > > + gen6_set_rps(dev, val); > > } > > > > - dev_priv->rps.min_delay = val; > > - > > mutex_unlock(&dev_priv->rps.hw_lock); > > > > return count; > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index 09ac9e7..830865e 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -3414,26 +3414,19 @@ static void ironlake_disable_drps(struct drm_device *dev) > > * ourselves, instead of doing a rmw cycle (which might result in us clearing > > * all limits and the gpu stuck at whatever frequency it is at atm). > > */ > > -static u32 gen6_rps_limits(struct drm_i915_private *dev_priv, u8 *val) > > +static u32 gen6_rps_limits(struct drm_i915_private *dev_priv, u8 val) > > { > > u32 limits; > > > > - limits = 0; > > - > > - if (*val >= dev_priv->rps.max_delay) > > - *val = dev_priv->rps.max_delay; > > - limits |= dev_priv->rps.max_delay << 24; > > - > > /* Only set the down limit when we've reached the lowest level to avoid > > * getting more interrupts, otherwise leave this clear. This prevents a > > * race in the hw when coming out of rc6: There's a tiny window where > > * the hw runs at the minimal clock before selecting the desired > > * frequency, if the down threshold expires in that window we will not > > * receive a down interrupt. */ > > - if (*val <= dev_priv->rps.min_delay) { > > - *val = dev_priv->rps.min_delay; > > + limits = dev_priv->rps.max_delay << 24; > > + if (val <= dev_priv->rps.min_delay) > > limits |= dev_priv->rps.min_delay << 16; > > - } > > > > return limits; > > } > > @@ -3533,7 +3526,6 @@ static void gen6_set_rps_thresholds(struct drm_i915_private *dev_priv, u8 val) > > void gen6_set_rps(struct drm_device *dev, u8 val) > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > - u32 limits = gen6_rps_limits(dev_priv, &val); > > > > WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock)); > > WARN_ON(val > dev_priv->rps.max_delay); > > @@ -3556,7 +3548,8 @@ void gen6_set_rps(struct drm_device *dev, u8 val) > > /* Make sure we continue to get interrupts > > * until we hit the minimum or maximum frequencies. > > */ > > - I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, limits); > > + I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, > > + gen6_rps_limits(dev_priv, val)); > > > > POSTING_READ(GEN6_RPNSWREQ); > > > > @@ -3620,8 +3613,6 @@ void valleyview_set_rps(struct drm_device *dev, u8 val) > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > > > - gen6_rps_limits(dev_priv, &val); > > - > > WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock)); > > WARN_ON(val > dev_priv->rps.max_delay); > > WARN_ON(val < dev_priv->rps.min_delay); > > -- > > 1.8.3.1 > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 7008aac..5b28602 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2758,7 +2758,7 @@ i915_max_freq_set(void *data, u64 val) if (IS_VALLEYVIEW(dev)) { val = vlv_freq_opcode(dev_priv->mem_freq, val); dev_priv->rps.max_delay = val; - gen6_set_rps(dev, val); + valleyview_set_rps(dev, val); } else { do_div(val, GT_FREQUENCY_MULTIPLIER); dev_priv->rps.max_delay = val; diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c index cef38fd..46291c4 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.c +++ b/drivers/gpu/drm/i915/i915_sysfs.c @@ -342,15 +342,15 @@ static ssize_t gt_max_freq_mhz_store(struct device *kdev, DRM_DEBUG("User requested overclocking to %d\n", val * GT_FREQUENCY_MULTIPLIER); + dev_priv->rps.max_delay = val; + if (dev_priv->rps.cur_delay > val) { - if (IS_VALLEYVIEW(dev_priv->dev)) - valleyview_set_rps(dev_priv->dev, val); + if (IS_VALLEYVIEW(dev)) + valleyview_set_rps(dev, val); else - gen6_set_rps(dev_priv->dev, val); + gen6_set_rps(dev, val); } - dev_priv->rps.max_delay = val; - mutex_unlock(&dev_priv->rps.hw_lock); return count; @@ -411,15 +411,15 @@ static ssize_t gt_min_freq_mhz_store(struct device *kdev, return -EINVAL; } + dev_priv->rps.min_delay = val; + if (dev_priv->rps.cur_delay < val) { if (IS_VALLEYVIEW(dev)) valleyview_set_rps(dev, val); else - gen6_set_rps(dev_priv->dev, val); + gen6_set_rps(dev, val); } - dev_priv->rps.min_delay = val; - mutex_unlock(&dev_priv->rps.hw_lock); return count; diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 09ac9e7..830865e 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3414,26 +3414,19 @@ static void ironlake_disable_drps(struct drm_device *dev) * ourselves, instead of doing a rmw cycle (which might result in us clearing * all limits and the gpu stuck at whatever frequency it is at atm). */ -static u32 gen6_rps_limits(struct drm_i915_private *dev_priv, u8 *val) +static u32 gen6_rps_limits(struct drm_i915_private *dev_priv, u8 val) { u32 limits; - limits = 0; - - if (*val >= dev_priv->rps.max_delay) - *val = dev_priv->rps.max_delay; - limits |= dev_priv->rps.max_delay << 24; - /* Only set the down limit when we've reached the lowest level to avoid * getting more interrupts, otherwise leave this clear. This prevents a * race in the hw when coming out of rc6: There's a tiny window where * the hw runs at the minimal clock before selecting the desired * frequency, if the down threshold expires in that window we will not * receive a down interrupt. */ - if (*val <= dev_priv->rps.min_delay) { - *val = dev_priv->rps.min_delay; + limits = dev_priv->rps.max_delay << 24; + if (val <= dev_priv->rps.min_delay) limits |= dev_priv->rps.min_delay << 16; - } return limits; } @@ -3533,7 +3526,6 @@ static void gen6_set_rps_thresholds(struct drm_i915_private *dev_priv, u8 val) void gen6_set_rps(struct drm_device *dev, u8 val) { struct drm_i915_private *dev_priv = dev->dev_private; - u32 limits = gen6_rps_limits(dev_priv, &val); WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock)); WARN_ON(val > dev_priv->rps.max_delay); @@ -3556,7 +3548,8 @@ void gen6_set_rps(struct drm_device *dev, u8 val) /* Make sure we continue to get interrupts * until we hit the minimum or maximum frequencies. */ - I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, limits); + I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, + gen6_rps_limits(dev_priv, val)); POSTING_READ(GEN6_RPNSWREQ); @@ -3620,8 +3613,6 @@ void valleyview_set_rps(struct drm_device *dev, u8 val) { struct drm_i915_private *dev_priv = dev->dev_private; - gen6_rps_limits(dev_priv, &val); - WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock)); WARN_ON(val > dev_priv->rps.max_delay); WARN_ON(val < dev_priv->rps.min_delay);