diff mbox

[3/7] drm/i915/skl: Restructured the gen6_set_rps_thresholds function

Message ID 1423234598-14781-4-git-send-email-akash.goel@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

akash.goel@intel.com Feb. 6, 2015, 2:56 p.m. UTC 
From: Akash Goel <akash.goel@intel.com>

Prior to SKL, the time period programmed in Up/Down EI & Up/Down
threshold registers was in units of 1.28 micro seconds. But for
SKL, the units have changed (1.333 micro seconds).
Have generalized the implementation of gen6_set_rps_thresholds function,
by removing the hard coding done in it as per 1.28 micro seconds.

Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 70 ++++++++++++++++++++---------------------
 1 file changed, 34 insertions(+), 36 deletions(-)

Comments

Chris Wilson Feb. 6, 2015, 3:48 p.m. UTC | #1 
On Fri, Feb 06, 2015 at 08:26:34PM +0530, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> Prior to SKL, the time period programmed in Up/Down EI & Up/Down
> threshold registers was in units of 1.28 micro seconds. But for
> SKL, the units have changed (1.333 micro seconds).
> Have generalized the implementation of gen6_set_rps_thresholds function,
> by removing the hard coding done in it as per 1.28 micro seconds.
> 
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 70 ++++++++++++++++++++---------------------
>  1 file changed, 34 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 58c8c0e..215b200 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3643,6 +3643,8 @@ static u32 gen6_rps_limits(struct drm_i915_private *dev_priv, u8 val)
>  static void gen6_set_rps_thresholds(struct drm_i915_private *dev_priv, u8 val)
>  {
>  	int new_power;
> +	u32 threshold_up_pct = 0, threshold_down_pct = 0;

Drop the _pct, unrequired early initialisation, just comment that
up/down are in %.

> +	u32 ei_up = 0, ei_down = 0;
>  
>  	new_power = dev_priv->rps.power;
>  	switch (dev_priv->rps.power) {
> @@ -3675,59 +3677,55 @@ static void gen6_set_rps_thresholds(struct drm_i915_private *dev_priv, u8 val)
>  	switch (new_power) {
>  	case LOW_POWER:
>  		/* Upclock if more than 95% busy over 16ms */
> -		I915_WRITE(GEN6_RP_UP_EI, 12500);
> -		I915_WRITE(GEN6_RP_UP_THRESHOLD, 11800);
> +		ei_up = 16000;
> +		threshold_up_pct = 95; /* x% */

Double comments that this is a %! Really doesn't seem to be required
with the preceeding comment.

> +	I915_WRITE(GEN6_RP_UP_EI,
> +		GT_FREQ_FROM_PERIOD(ei_up, dev_priv->dev));

Just pass dev_priv. It's magic.

> +	I915_WRITE(GEN6_RP_UP_THRESHOLD,
> +		GT_FREQ_FROM_PERIOD((ei_up * threshold_up_pct / 100),

I wonder if it is worth using base 128 instead of 100%.

Otherwise looks good and ties in with using it from vlv. Do you mind
reviewing those patches? They fix a bug in which the manual c0 counting
keeps interrupts alive whilst idle.
-Chris
akash.goel@intel.com Feb. 9, 2015, 4:51 a.m. UTC | #2 
On Fri, 2015-02-06 at 15:48 +0000, Chris Wilson wrote:
> On Fri, Feb 06, 2015 at 08:26:34PM +0530, akash.goel@intel.com wrote:
> > From: Akash Goel <akash.goel@intel.com>
> > 
> > Prior to SKL, the time period programmed in Up/Down EI & Up/Down
> > threshold registers was in units of 1.28 micro seconds. But for
> > SKL, the units have changed (1.333 micro seconds).
> > Have generalized the implementation of gen6_set_rps_thresholds function,
> > by removing the hard coding done in it as per 1.28 micro seconds.
> > 
> > Signed-off-by: Akash Goel <akash.goel@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 70 ++++++++++++++++++++---------------------
> >  1 file changed, 34 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 58c8c0e..215b200 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3643,6 +3643,8 @@ static u32 gen6_rps_limits(struct drm_i915_private *dev_priv, u8 val)
> >  static void gen6_set_rps_thresholds(struct drm_i915_private *dev_priv, u8 val)
> >  {
> >  	int new_power;
> > +	u32 threshold_up_pct = 0, threshold_down_pct = 0;
> 
> Drop the _pct, unrequired early initialisation, just comment that
> up/down are in %.
Fine, will remove the _pct suffix.

> > +	u32 ei_up = 0, ei_down = 0;
> >  
> >  	new_power = dev_priv->rps.power;
> >  	switch (dev_priv->rps.power) {
> > @@ -3675,59 +3677,55 @@ static void gen6_set_rps_thresholds(struct drm_i915_private *dev_priv, u8 val)
> >  	switch (new_power) {
> >  	case LOW_POWER:
> >  		/* Upclock if more than 95% busy over 16ms */
> > -		I915_WRITE(GEN6_RP_UP_EI, 12500);
> > -		I915_WRITE(GEN6_RP_UP_THRESHOLD, 11800);
> > +		ei_up = 16000;
> > +		threshold_up_pct = 95; /* x% */
> 
> Double comments that this is a %! Really doesn't seem to be required
> with the preceeding comment.
Fine, will remove the needless comment

> > +	I915_WRITE(GEN6_RP_UP_EI,
> > +		GT_FREQ_FROM_PERIOD(ei_up, dev_priv->dev));
> 
> Just pass dev_priv. It's magic.
Will modify the macro to accept the dev_priv instead of dev.

> 
> > +	I915_WRITE(GEN6_RP_UP_THRESHOLD,
> > +		GT_FREQ_FROM_PERIOD((ei_up * threshold_up_pct / 100),
> 
> I wonder if it is worth using base 128 instead of 100%.
Sorry couldn't get this comment. 
Actually the macro GT_FREQ_FROM_PERIOD has been used at other places,
where % conversion is not required. So not sure, how using a base of
128 would be better. 

> 
> Otherwise looks good and ties in with using it from vlv. Do you mind
> reviewing those patches? 
Sure, will review that patch, please provide the patch link. 

> They fix a bug in which the manual c0 counting
> keeps interrupts alive whilst idle.


> -Chris
>
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 58c8c0e..215b200 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3643,6 +3643,8 @@  static u32 gen6_rps_limits(struct drm_i915_private *dev_priv, u8 val)
 static void gen6_set_rps_thresholds(struct drm_i915_private *dev_priv, u8 val)
 {
 	int new_power;
+	u32 threshold_up_pct = 0, threshold_down_pct = 0;
+	u32 ei_up = 0, ei_down = 0;
 
 	new_power = dev_priv->rps.power;
 	switch (dev_priv->rps.power) {
@@ -3675,59 +3677,55 @@  static void gen6_set_rps_thresholds(struct drm_i915_private *dev_priv, u8 val)
 	switch (new_power) {
 	case LOW_POWER:
 		/* Upclock if more than 95% busy over 16ms */
-		I915_WRITE(GEN6_RP_UP_EI, 12500);
-		I915_WRITE(GEN6_RP_UP_THRESHOLD, 11800);
+		ei_up = 16000;
+		threshold_up_pct = 95; /* x% */
 
 		/* Downclock if less than 85% busy over 32ms */
-		I915_WRITE(GEN6_RP_DOWN_EI, 25000);
-		I915_WRITE(GEN6_RP_DOWN_THRESHOLD, 21250);
-
-		I915_WRITE(GEN6_RP_CONTROL,
-			   GEN6_RP_MEDIA_TURBO |
-			   GEN6_RP_MEDIA_HW_NORMAL_MODE |
-			   GEN6_RP_MEDIA_IS_GFX |
-			   GEN6_RP_ENABLE |
-			   GEN6_RP_UP_BUSY_AVG |
-			   GEN6_RP_DOWN_IDLE_AVG);
+		ei_down = 32000;
+		threshold_down_pct = 85;
 		break;
 
 	case BETWEEN:
 		/* Upclock if more than 90% busy over 13ms */
-		I915_WRITE(GEN6_RP_UP_EI, 10250);
-		I915_WRITE(GEN6_RP_UP_THRESHOLD, 9225);
+		ei_up = 13000;
+		threshold_up_pct = 90; /* x% */
 
 		/* Downclock if less than 75% busy over 32ms */
-		I915_WRITE(GEN6_RP_DOWN_EI, 25000);
-		I915_WRITE(GEN6_RP_DOWN_THRESHOLD, 18750);
-
-		I915_WRITE(GEN6_RP_CONTROL,
-			   GEN6_RP_MEDIA_TURBO |
-			   GEN6_RP_MEDIA_HW_NORMAL_MODE |
-			   GEN6_RP_MEDIA_IS_GFX |
-			   GEN6_RP_ENABLE |
-			   GEN6_RP_UP_BUSY_AVG |
-			   GEN6_RP_DOWN_IDLE_AVG);
+		ei_down = 32000;
+		threshold_down_pct = 75;
 		break;
 
 	case HIGH_POWER:
 		/* Upclock if more than 85% busy over 10ms */
-		I915_WRITE(GEN6_RP_UP_EI, 8000);
-		I915_WRITE(GEN6_RP_UP_THRESHOLD, 6800);
+		ei_up = 10000;
+		threshold_up_pct = 85; /* x% */
 
 		/* Downclock if less than 60% busy over 32ms */
-		I915_WRITE(GEN6_RP_DOWN_EI, 25000);
-		I915_WRITE(GEN6_RP_DOWN_THRESHOLD, 15000);
-
-		I915_WRITE(GEN6_RP_CONTROL,
-			   GEN6_RP_MEDIA_TURBO |
-			   GEN6_RP_MEDIA_HW_NORMAL_MODE |
-			   GEN6_RP_MEDIA_IS_GFX |
-			   GEN6_RP_ENABLE |
-			   GEN6_RP_UP_BUSY_AVG |
-			   GEN6_RP_DOWN_IDLE_AVG);
+		ei_down = 32000;
+		threshold_down_pct = 60;
 		break;
 	}
 
+	I915_WRITE(GEN6_RP_UP_EI,
+		GT_FREQ_FROM_PERIOD(ei_up, dev_priv->dev));
+	I915_WRITE(GEN6_RP_UP_THRESHOLD,
+		GT_FREQ_FROM_PERIOD((ei_up * threshold_up_pct / 100),
+		dev_priv->dev));
+
+	I915_WRITE(GEN6_RP_DOWN_EI,
+		GT_FREQ_FROM_PERIOD(ei_down, dev_priv->dev));
+	I915_WRITE(GEN6_RP_DOWN_THRESHOLD,
+		GT_FREQ_FROM_PERIOD((ei_down * threshold_down_pct / 100),
+		dev_priv->dev));
+
+	 I915_WRITE(GEN6_RP_CONTROL,
+		    GEN6_RP_MEDIA_TURBO |
+		    GEN6_RP_MEDIA_HW_NORMAL_MODE |
+		    GEN6_RP_MEDIA_IS_GFX |
+		    GEN6_RP_ENABLE |
+		    GEN6_RP_UP_BUSY_AVG |
+		    GEN6_RP_DOWN_IDLE_AVG);
+
 	dev_priv->rps.power = new_power;
 	dev_priv->rps.last_adj = 0;
 }