From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] drm/i915: Initialise min/max frequencies before updating RPS registers Date: Mon, 26 Aug 2013 16:15:16 +0300 Message-ID: <20130826131516.GQ11428@intel.com> References: <1377521156-10045-1-git-send-email-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTP id EEEA6E7165 for ; Mon, 26 Aug 2013 06:15:38 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1377521156-10045-1-git-send-email-chris@chris-wilson.co.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Chris Wilson Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org 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 > --- > 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/i= 915_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 =3D vlv_freq_opcode(dev_priv->mem_freq, val); > dev_priv->rps.max_delay =3D 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. > } else { > do_div(val, GT_FREQUENCY_MULTIPLIER); > dev_priv->rps.max_delay =3D val; > diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i91= 5_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 =3D 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 =3D 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 =3D 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 =3D 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_devi= ce *dev) > * ourselves, instead of doing a rmw cycle (which might result in us cle= aring > * 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 =3D 0; > - > - if (*val >=3D dev_priv->rps.max_delay) > - *val =3D dev_priv->rps.max_delay; > - limits |=3D 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 <=3D dev_priv->rps.min_delay) { > - *val =3D dev_priv->rps.min_delay; > + limits =3D dev_priv->rps.max_delay << 24; > + if (val <=3D dev_priv->rps.min_delay) > limits |=3D 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 =3D dev->dev_private; > - u32 limits =3D 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 =3D 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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- = Ville Syrj=E4l=E4 Intel OTC