On Mon, Aug 26, 2013 at 04:15:16PM +0300, Ville Syrjälä wrote:
> On Mon, Aug 26, 2013 at 01:45:56PM +0100, Chris Wilson wrote:
> > 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@xxxxxxxxxxxxxxxxxx>
> > ---
> > 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 2a276c8..b2b1730 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2113,7 +2113,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);
>
> Not caused by your patch, but why on earth are we telling the GPU
> to switch to the new max_freq here?
>
> In the old way of doing things I presume this should have been
> set_rps(cur_delay). And in the new way we should add the
> same 'cur_delay > val' check here that we have in i915_sysfs.
>
> Maybe we should just have some kind of
> rps_set_minmax(new_min, new_max) func that takes care of
> this stuff in a single location.
We might as well just rip out the debugfs interfaces now that we have all
this stuff in sysfs.
-Daniel
>
> > } 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 c8c4112..05195c0 100644
> > --- a/drivers/gpu/drm/i915/i915_sysfs.c
> > +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> > @@ -294,15 +294,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;
> > @@ -359,15 +359,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 6767e2b..f995dda 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3263,26 +3263,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;
> > }
> > @@ -3382,7 +3375,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);
> > @@ -3405,7 +3397,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);
> >
> > @@ -3463,8 +3456,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.4.rc3
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
