From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 2/5] drm/i915: Initialise min/max frequencies before updating RPS registers Date: Thu, 7 Nov 2013 19:13:54 +0100 Message-ID: <20131107181354.GX14082@phenom.ffwll.local> References: <1383753389-12763-1-git-send-email-rodrigo.vivi@gmail.com> <1383753389-12763-3-git-send-email-rodrigo.vivi@gmail.com> <20131107174348.GZ5986@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-ee0-f44.google.com (mail-ee0-f44.google.com [74.125.83.44]) by gabe.freedesktop.org (Postfix) with ESMTP id 3A672F0924 for ; Thu, 7 Nov 2013 10:13:25 -0800 (PST) Received: by mail-ee0-f44.google.com with SMTP id d41so509385eek.31 for ; Thu, 07 Nov 2013 10:13:24 -0800 (PST) Content-Disposition: inline In-Reply-To: <20131107174348.GZ5986@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Thu, Nov 07, 2013 at 07:43:48PM +0200, Ville Syrj=E4l=E4 wrote: > On Wed, Nov 06, 2013 at 01:56:26PM -0200, Rodrigo Vivi wrote: > > From: Chris Wilson > > = > > 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 > > Signed-off-by: Rodrigo Vivi > > Reviewer: Ville Syrj=E4l=E4 > > CC: Ville Syrj=E4l=E4 > = > Reviewed-by: Ville Syrj=E4l=E4 > = > > --- > > 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 =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); > > } 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/i= 915_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 devic= e *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); 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 =3D val; > > - > > mutex_unlock(&dev_priv->rps.hw_lock); > > = > > return count; > > @@ -411,15 +411,15 @@ static ssize_t gt_min_freq_mhz_store(struct devic= e *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/int= el_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_de= vice *dev) > > * ourselves, instead of doing a rmw cycle (which might result in us c= learing > > * 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 avo= id > > * 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; > > } > > @@ -3533,7 +3526,6 @@ static void gen6_set_rps_thresholds(struct drm_i9= 15_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); > > @@ -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, u= 8 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.3.1 > = > -- = > Ville Syrj=E4l=E4 > Intel OTC > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- = Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch