public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: Initialise min/max frequencies before updating RPS registers
@ 2013-08-25 16:32 Chris Wilson
  0 siblings, 0 replies; 6+ messages in thread
From: Chris Wilson @ 2013-08-25 16:32 UTC (permalink / raw)
  To: intel-gfx

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>
---
 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);
 	} 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 dce9f3c..1dfe8ce 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

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH] drm/i915: Initialise min/max frequencies before updating RPS registers
@ 2013-08-26 12:45 Chris Wilson
  2013-08-26 13:15 ` Ville Syrjälä
  0 siblings, 1 reply; 6+ messages in thread
From: Chris Wilson @ 2013-08-26 12:45 UTC (permalink / raw)
  To: intel-gfx

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>
---
 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);
 	} 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

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Initialise min/max frequencies before updating RPS registers
  2013-08-26 12:45 [PATCH] drm/i915: Initialise min/max frequencies before updating RPS registers Chris Wilson
@ 2013-08-26 13:15 ` Ville Syrjälä
  2013-09-24 10:47   ` Daniel Vetter
  0 siblings, 1 reply; 6+ messages in thread
From: Ville Syrjälä @ 2013-08-26 13:15 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

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@chris-wilson.co.uk>
> ---
>  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.

>  	} 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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Initialise min/max frequencies before updating RPS registers
  2013-08-26 13:15 ` Ville Syrjälä
@ 2013-09-24 10:47   ` Daniel Vetter
  2013-09-24 11:12     ` Chris Wilson
  0 siblings, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2013-09-24 10:47 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

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@chris-wilson.co.uk>
> > ---
> >  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@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> 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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Initialise min/max frequencies before updating RPS registers
  2013-09-24 10:47   ` Daniel Vetter
@ 2013-09-24 11:12     ` Chris Wilson
  2013-09-24 11:14       ` Daniel Vetter
  0 siblings, 1 reply; 6+ messages in thread
From: Chris Wilson @ 2013-09-24 11:12 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx

On Tue, Sep 24, 2013 at 12:47:21PM +0200, Daniel Vetter wrote:
> 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@chris-wilson.co.uk>
> > > ---
> > >  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.

Actually, the debugfs can serve a purpose for giving us hw values
instead of our bookkeeping values (which is what sysfs should provide).
So lose the ability to write values, but keep the low level reads
intact.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Initialise min/max frequencies before updating RPS registers
  2013-09-24 11:12     ` Chris Wilson
@ 2013-09-24 11:14       ` Daniel Vetter
  0 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2013-09-24 11:14 UTC (permalink / raw)
  To: Chris Wilson, Daniel Vetter, Ville Syrjälä, intel-gfx

On Tue, Sep 24, 2013 at 12:12:54PM +0100, Chris Wilson wrote:
> On Tue, Sep 24, 2013 at 12:47:21PM +0200, Daniel Vetter wrote:
> > 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@chris-wilson.co.uk>
> > > > ---
> > > >  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.
> 
> Actually, the debugfs can serve a purpose for giving us hw values
> instead of our bookkeeping values (which is what sysfs should provide).
> So lose the ability to write values, but keep the low level reads
> intact.

Yeah, that sounds useful. Maybe shovel it all into the aggregate info file
we already have in debugfs.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-09-24 11:14 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-08-26 12:45 [PATCH] drm/i915: Initialise min/max frequencies before updating RPS registers Chris Wilson
2013-08-26 13:15 ` Ville Syrjälä
2013-09-24 10:47   ` Daniel Vetter
2013-09-24 11:12     ` Chris Wilson
2013-09-24 11:14       ` Daniel Vetter
  -- strict thread matches above, loose matches on Subject: below --
2013-08-25 16:32 Chris Wilson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox