CHV PM fix & Improvements

From: Deepak S <[email protected]>

On CHV, PUNIT team confirmed that 'VLV_GFX_CLK_STATUS_BIT' is not a sticky bit and it will always be set. So ignore Check for previous Gfx force off during suspend and allow the force clk as part S0ix Sequence

Signed-off-by Deepak S <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/i915_drv.c +4 -2

index 82f8be4..182d6a7 100644
@@ -1194,11 +1194,13 @@ int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool force_on)
 	int err;
 
 	val = I915_READ(VLV_GTLC_SURVIVABILITY_REG);
-	WARN_ON(!!(val & VLV_GFX_CLK_FORCE_ON_BIT) == force_on);
 
 #define COND (I915_READ(VLV_GTLC_SURVIVABILITY_REG) & VLV_GFX_CLK_STATUS_BIT)
 	/* Wait for a previous force-off to settle */
-	if (force_on) {
+	if (force_on && !IS_CHERRYVIEW(dev_priv->dev)) {
+		/* WARN_ON only for the Valleyview */
+		WARN_ON(!!(val & VLV_GFX_CLK_FORCE_ON_BIT) == force_on);
+
 		err = wait_for(!COND, 20);
 		if (err) {
 			DRM_ERROR("timeout waiting for GFX clock force-off (%08x)\n",

Signature · 1 line

1.9.1

Quoted 7 lines · On Sat, Mar 28, 2015 at 03:23:34PM +0530, [email protected] wrote:

From: Deepak S <[email protected]>

On CHV, PUNIT team confirmed that 'VLV_GFX_CLK_STATUS_BIT' is not a sticky bit and it will always be set. So ignore Check for previous Gfx force off during suspend and allow the force clk as part S0ix Sequence

Quoted 1 line

Signed-off-by Deepak S <[email protected]>

Yeah seems OK. We still do the "allow wake" dance even though we skip the gunit register save, so I guess the force gfx clock makes sense as part of that.

Reviewed-by Ville Syrjälä <[email protected]>

Quoted 14 lines

---
  drivers/gpu/drm/i915/i915_drv.c | 6 ++++--
  1 file changed, 4 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/i915_drv.c +0 -1

index 82f8be4..182d6a7 100644
@@ -1194,11 +1194,13 @@ int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool force_on)
  	int err;
 
  	val = I915_READ(VLV_GTLC_SURVIVABILITY_REG);
-	WARN_ON(!!(val & VLV_GFX_CLK_FORCE_ON_BIT) == force_on);
 

Quoted 12 lines

drivers/gpu/drm/i915/i915_drv.c +4 -1

  #define COND (I915_READ(VLV_GTLC_SURVIVABILITY_REG) & VLV_GFX_CLK_STATUS_BIT)
  	/* Wait for a previous force-off to settle */
-	if (force_on) {
+	if (force_on && !IS_CHERRYVIEW(dev_priv->dev)) {
+		/* WARN_ON only for the Valleyview */
+		WARN_ON(!!(val & VLV_GFX_CLK_FORCE_ON_BIT) == force_on);
+
  		err = wait_for(!COND, 20);
  		if (err) {
  			DRM_ERROR("timeout waiting for GFX clock force-off (%08x)\n",

Signature · 1 line

1.9.1

Quoted 7 lines · On Sat, Mar 28, 2015 at 03:23:34PM +0530, [email protected] wrote:

From: Deepak S <[email protected]>

On CHV, PUNIT team confirmed that 'VLV_GFX_CLK_STATUS_BIT' is not a sticky bit and it will always be set. So ignore Check for previous Gfx force off during suspend and allow the force clk as part S0ix Sequence

Quoted 1 line

Signed-off-by Deepak S <[email protected]>

Yeah seems OK. We still do the "allow wake" dance even though we skip the gunit register save, so I guess the force gfx clock makes sense as part of that.

Reviewed-by Ville Syrjälä <[email protected]>

Quoted 27 lines

---
 drivers/gpu/drm/i915/i915_drv.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/i915_drv.c +4 -2

index 82f8be4..182d6a7 100644
@@ -1194,11 +1194,13 @@ int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool force_on)
 	int err;
 
 	val = I915_READ(VLV_GTLC_SURVIVABILITY_REG);
-	WARN_ON(!!(val & VLV_GFX_CLK_FORCE_ON_BIT) == force_on);
 
 #define COND (I915_READ(VLV_GTLC_SURVIVABILITY_REG) & VLV_GFX_CLK_STATUS_BIT)
 	/* Wait for a previous force-off to settle */
-	if (force_on) {
+	if (force_on && !IS_CHERRYVIEW(dev_priv->dev)) {
+		/* WARN_ON only for the Valleyview */
+		WARN_ON(!!(val & VLV_GFX_CLK_FORCE_ON_BIT) == force_on);
+
 		err = wait_for(!COND, 20);
 		if (err) {
 			DRM_ERROR("timeout waiting for GFX clock force-off (%08x)\n",

Signature · 1 line

1.9.1

Signature · 6 lines

Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

From: Deepak S <[email protected]>

After feedback from the hardware team we are changing the RC6 promotional timer to increase the power saving without changing performance.

Signed-off-by Deepak S <[email protected]>

Reviewed-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_pm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/intel_pm.c +2 -2

index fa4ccb3..44428e4 100644
@@ -4996,8 +4996,8 @@ static void cherryview_enable_rps(struct drm_device *dev)
 		I915_WRITE(RING_MAX_IDLE(ring->mmio_base), 10);
 	I915_WRITE(GEN6_RC_SLEEP, 0);
 
-	/* TO threshold set to 1750 us ( 0x557 * 1.28 us) */
-	I915_WRITE(GEN6_RC6_THRESHOLD, 0x557);
+	/* TO threshold set to 500 us ( 0x186 * 1.28 us) */
+	I915_WRITE(GEN6_RC6_THRESHOLD, 0x186);
 
 	/* allows RC6 residency counter to work */
 	I915_WRITE(VLV_COUNTER_CONTROL,

Signature · 6 lines

1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

From: Deepak S <[email protected]>

After feedback from the hardware team, now we set the GPU min/idel freq to RPe. Punit is expecting us to operate GPU between Rpe & Rp0. If we drop the frequency to RPn, punit is failing to change the input voltage to minimum :(

Since Punit validates the rps range [RPe, RP0]. This patch removes unused cherryview_rps_min_freq function.

v2: Change commit message

v3: set min_freq before idle_freq (chris)

v4: Squash 'Remove unused rps min function' patch

Signed-off-by Deepak S <[email protected]>

Acked-by Chris Wilson <[email protected]>

---
 drivers/gpu/drm/i915/intel_pm.c | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

drivers/gpu/drm/i915/intel_pm.c +2 -19

index a7516ed..78c89ff 100644
@@ -4715,24 +4715,6 @@ static int cherryview_rps_guar_freq(struct drm_i915_private *dev_priv)
 	return rp1;
 }
 
-static int cherryview_rps_min_freq(struct drm_i915_private *dev_priv)
-{
-	struct drm_device *dev = dev_priv->dev;
-	u32 val, rpn;
-
-	if (dev->pdev->revision >= 0x20) {
-		val = vlv_punit_read(dev_priv, FB_GFX_FMIN_AT_VMIN_FUSE);
-		rpn = ((val >> FB_GFX_FMIN_AT_VMIN_FUSE_SHIFT) &
-		       FB_GFX_FREQ_FUSE_MASK);
-	} else { /* For pre-production hardware */
-		val = vlv_punit_read(dev_priv, PUNIT_GPU_STATUS_REG);
-		rpn = ((val >> PUNIT_GPU_STATIS_GFX_MIN_FREQ_SHIFT) &
-		       PUNIT_GPU_STATUS_GFX_MIN_FREQ_MASK);
-	}
-
-	return rpn;
-}
-
 static int valleyview_rps_guar_freq(struct drm_i915_private *dev_priv)
 {
 	u32 val, rp1;

@@ -4984,7 +4966,8 @@ static void cherryview_init_gt_powersave(struct drm_device *dev)
 			 intel_gpu_freq(dev_priv, dev_priv->rps.rp1_freq),
 			 dev_priv->rps.rp1_freq);
 
-	dev_priv->rps.min_freq = cherryview_rps_min_freq(dev_priv);
+	/* PUnit validated range is only [RPe, RP0] */
+	dev_priv->rps.min_freq = dev_priv->rps.efficient_freq;
 	DRM_DEBUG_DRIVER("min GPU freq: %d MHz (%u)\n",
 			 intel_gpu_freq(dev_priv, dev_priv->rps.min_freq),
 			 dev_priv->rps.min_freq);

Signature · 1 line

1.9.1

Quoted 6 lines · On Wed, Apr 29, 2015 at 08:23:21AM +0530, [email protected] wrote:

From: Deepak S <[email protected]>

After feedback from the hardware team, now we set the GPU min/idel freq to RPe. Punit is expecting us to operate GPU between Rpe & Rp0. If we drop the frequency to RPn, punit is failing to change the input voltage to minimum :(

So far I can't reproduce this problem on my BSW. In fact what I see that the voltage at RPn is lower than the voltage at RPe, even while we're in rc6.

without forcewake:
RPn -> 0x66
RPe -> 0x67
RP0 -> 0x69

with forcewake:
RPn -> 0x66
RPe -> 0x76
RP0 -> 0x9d

Also asking Punit to change the frequency after the GPU has gone to rc6 does absolutely nothing (remind anyone of VLV?). I think I need to retest my VLV C0 to see if my earlier observations there were accurate. The shared Vnn rail does make it harder to observe this stuff on VLV though.

So based on my tests this patch feels a bit wrong.

Quoted 57 lines

Since Punit validates the rps range [RPe, RP0]. This patch removes unused cherryview_rps_min_freq function.

v2: Change commit message

v3: set min_freq before idle_freq (chris)

v4: Squash 'Remove unused rps min function' patch

Signed-off-by Deepak S <[email protected]>

Acked-by Chris Wilson <[email protected]>

---
 drivers/gpu/drm/i915/intel_pm.c | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

drivers/gpu/drm/i915/intel_pm.c +2 -19

index a7516ed..78c89ff 100644
@@ -4715,24 +4715,6 @@ static int cherryview_rps_guar_freq(struct drm_i915_private *dev_priv)
 	return rp1;
 }
 
-static int cherryview_rps_min_freq(struct drm_i915_private *dev_priv)
-{
-	struct drm_device *dev = dev_priv->dev;
-	u32 val, rpn;
-
-	if (dev->pdev->revision >= 0x20) {
-		val = vlv_punit_read(dev_priv, FB_GFX_FMIN_AT_VMIN_FUSE);
-		rpn = ((val >> FB_GFX_FMIN_AT_VMIN_FUSE_SHIFT) &
-		       FB_GFX_FREQ_FUSE_MASK);
-	} else { /* For pre-production hardware */
-		val = vlv_punit_read(dev_priv, PUNIT_GPU_STATUS_REG);
-		rpn = ((val >> PUNIT_GPU_STATIS_GFX_MIN_FREQ_SHIFT) &
-		       PUNIT_GPU_STATUS_GFX_MIN_FREQ_MASK);
-	}
-
-	return rpn;
-}
-
 static int valleyview_rps_guar_freq(struct drm_i915_private *dev_priv)
 {
 	u32 val, rp1;

@@ -4984,7 +4966,8 @@ static void cherryview_init_gt_powersave(struct drm_device *dev)
 			 intel_gpu_freq(dev_priv, dev_priv->rps.rp1_freq),
 			 dev_priv->rps.rp1_freq);
 
-	dev_priv->rps.min_freq = cherryview_rps_min_freq(dev_priv);
+	/* PUnit validated range is only [RPe, RP0] */
+	dev_priv->rps.min_freq = dev_priv->rps.efficient_freq;
 	DRM_DEBUG_DRIVER("min GPU freq: %d MHz (%u)\n",
 			 intel_gpu_freq(dev_priv, dev_priv->rps.min_freq),
 			 dev_priv->rps.min_freq);

Signature · 1 line

1.9.1

Signature · 6 lines

Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 21 lines · Ville Syrjälä

Quoted 6 lines · On Wed, Apr 29, 2015 at 08:23:21AM +0530, [email protected] wrote:

From: Deepak S <[email protected]>

After feedback from the hardware team, now we set the GPU min/idel freq to RPe. Punit is expecting us to operate GPU between Rpe & Rp0. If we drop the frequency to RPn, punit is failing to change the input voltage to minimum :(

So far I can't reproduce this problem on my BSW. In fact what I see that the voltage at RPn is lower than the voltage at RPe, even while we're in rc6.

without forcewake:
RPn -> 0x66
RPe -> 0x67
RP0 -> 0x69

with forcewake:
RPn -> 0x66
RPe -> 0x76
RP0 -> 0x9d

Quoted 5 lines

Also asking Punit to change the frequency after the GPU has gone to rc6 does absolutely nothing (remind anyone of VLV?). I think I need to retest my VLV C0 to see if my earlier observations there were accurate. The shared Vnn rail does make it harder to observe this stuff on VLV though.

I went back to my VLVs (had a B3 and C0 actually). And I'm seeing the exact same behaviour on both, ie. requesting a new frequency from Punit does nothing when the GPU is in rc6, and if I let it enter rc6 with a high frequency Vnn also remains high. Previously I had thought that C0 fixed this, but now it definitely shows the same problem here. I must have had some accidental forcewake somewhere when I originally tested it,

So based on that, your other patch to remove the stepping check from vlv_set_rps_idle() is in fact correct.

The question remains however what should we do with CHV. According to my testing to get the minimum voltage we should keep RPn around, and we should also do the vlv_set_rps_idle() workaround on CHV.

Oh and I also observed something else on VLV. Normally when entering rc6 the GPLL ref clock gets trunk gated at CCK (by Punit I assume). However when using the vlv_gfx_clock_force() that doesn't happen. So I'm not entirely sure the GPLL gets turned off properly in that case. Maybe we should just use forcewake instead? Oh and BTW, CHV doesn't do the trunk gating in either case. I'm not sure where to check if the GPLL is actually running or not.

Quoted 50 lines

So based on my tests this patch feels a bit wrong.

Quoted 48 lines

Since Punit validates the rps range [RPe, RP0]. This patch removes unused cherryview_rps_min_freq function.

v2: Change commit message

v3: set min_freq before idle_freq (chris)

v4: Squash 'Remove unused rps min function' patch

Signed-off-by Deepak S <[email protected]>

Acked-by Chris Wilson <[email protected]>

---
  drivers/gpu/drm/i915/intel_pm.c | 21 ++-------------------
  1 file changed, 2 insertions(+), 19 deletions(-)

drivers/gpu/drm/i915/intel_pm.c +0 -18

index a7516ed..78c89ff 100644
@@ -4715,24 +4715,6 @@ static int cherryview_rps_guar_freq(struct drm_i915_private *dev_priv)
  	return rp1;
  }
 
-static int cherryview_rps_min_freq(struct drm_i915_private *dev_priv)
-{
-	struct drm_device *dev = dev_priv->dev;
-	u32 val, rpn;
-
-	if (dev->pdev->revision >= 0x20) {
-		val = vlv_punit_read(dev_priv, FB_GFX_FMIN_AT_VMIN_FUSE);
-		rpn = ((val >> FB_GFX_FMIN_AT_VMIN_FUSE_SHIFT) &
-		       FB_GFX_FREQ_FUSE_MASK);
-	} else { /* For pre-production hardware */
-		val = vlv_punit_read(dev_priv, PUNIT_GPU_STATUS_REG);
-		rpn = ((val >> PUNIT_GPU_STATIS_GFX_MIN_FREQ_SHIFT) &
-		       PUNIT_GPU_STATUS_GFX_MIN_FREQ_MASK);
-	}
-
-	return rpn;
-}
-
  static int valleyview_rps_guar_freq(struct drm_i915_private *dev_priv)
  {
  	u32 val, rp1;

@@ -4984,7 +4966,8 @@ static void cherryview_init_gt_powersave(struct drm_device *dev)
  			 intel_gpu_freq(dev_priv, dev_priv->rps.rp1_freq),
  			 dev_priv->rps.rp1_freq);
 

Quoted 15 lines

Quoted 8 lines

drivers/gpu/drm/i915/intel_pm.c +2 -1

-	dev_priv->rps.min_freq = cherryview_rps_min_freq(dev_priv);
+	/* PUnit validated range is only [RPe, RP0] */
+	dev_priv->rps.min_freq = dev_priv->rps.efficient_freq;
  	DRM_DEBUG_DRIVER("min GPU freq: %d MHz (%u)\n",
  			 intel_gpu_freq(dev_priv, dev_priv->rps.min_freq),
  			 dev_priv->rps.min_freq);

Signature · 1 line

1.9.1

Signature · 6 lines

Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 42 lines · Ville Syrjälä

Quoted 21 lines · Ville Syrjälä

Quoted 6 lines · On Wed, Apr 29, 2015 at 08:23:21AM +0530, [email protected] wrote:

From: Deepak S <[email protected]>

After feedback from the hardware team, now we set the GPU min/idel freq to RPe. Punit is expecting us to operate GPU between Rpe & Rp0. If we drop the frequency to RPn, punit is failing to change the input voltage to minimum :(

So far I can't reproduce this problem on my BSW. In fact what I see that the voltage at RPn is lower than the voltage at RPe, even while we're in rc6.

without forcewake:
RPn -> 0x66
RPe -> 0x67
RP0 -> 0x69

with forcewake:
RPn -> 0x66
RPe -> 0x76
RP0 -> 0x9d

Quoted 5 lines

Also asking Punit to change the frequency after the GPU has gone to rc6 does absolutely nothing (remind anyone of VLV?). I think I need to retest my VLV C0 to see if my earlier observations there were accurate. The shared Vnn rail does make it harder to observe this stuff on VLV though.

I went back to my VLVs (had a B3 and C0 actually). And I'm seeing the exact same behaviour on both, ie. requesting a new frequency from Punit does nothing when the GPU is in rc6, and if I let it enter rc6 with a high frequency Vnn also remains high. Previously I had thought that C0 fixed this, but now it definitely shows the same problem here. I must have had some accidental forcewake somewhere when I originally tested it,

So based on that, your other patch to remove the stepping check from vlv_set_rps_idle() is in fact correct.

The question remains however what should we do with CHV. According to my testing to get the minimum voltage we should keep RPn around, and we should also do the vlv_set_rps_idle() workaround on CHV.

Quoted 8 lines

Oh and I also observed something else on VLV. Normally when entering rc6 the GPLL ref clock gets trunk gated at CCK (by Punit I assume). However when using the vlv_gfx_clock_force() that doesn't happen. So I'm not entirely sure the GPLL gets turned off properly in that case. Maybe we should just use forcewake instead? Oh and BTW, CHV doesn't do the trunk gating in either case. I'm not sure where to check if the GPLL is actually running or not.

Hi Ville,

Thanks Ville for verifying on VLV and CHV. Its interesting to see when Idle, Voltage not dropping to Vnn on CHV :( This was supposed to be fixed in BSW/CHV :(.

As you suggested it would be better to extend the VLV WA to CHV also to make sure we drop the voltage when idle.

Below is the sequence I think we should follow (based on your comments).
1. forcewake power wells
2. do gfx force clock on
3. request freq to punit
4. release gfx force clock on
5. release forcewake of power wells.

Please share your thoughts?

Thanks
Deepak

Quoted 65 lines

Quoted 65 lines

So based on my tests this patch feels a bit wrong.

Quoted 56 lines

Since Punit validates the rps range [RPe, RP0]. This patch removes unused cherryview_rps_min_freq function.

v2: Change commit message

v3: set min_freq before idle_freq (chris)

v4: Squash 'Remove unused rps min function' patch

Signed-off-by Deepak S <[email protected]>

Acked-by Chris Wilson <[email protected]>

---
  drivers/gpu/drm/i915/intel_pm.c | 21 ++-------------------
  1 file changed, 2 insertions(+), 19 deletions(-)

drivers/gpu/drm/i915/intel_pm.c +2 -19

index a7516ed..78c89ff 100644
@@ -4715,24 +4715,6 @@ static int cherryview_rps_guar_freq(struct drm_i915_private *dev_priv)
  	return rp1;
  }
 
-static int cherryview_rps_min_freq(struct drm_i915_private *dev_priv)
-{
-	struct drm_device *dev = dev_priv->dev;
-	u32 val, rpn;
-
-	if (dev->pdev->revision >= 0x20) {
-		val = vlv_punit_read(dev_priv, FB_GFX_FMIN_AT_VMIN_FUSE);
-		rpn = ((val >> FB_GFX_FMIN_AT_VMIN_FUSE_SHIFT) &
-		       FB_GFX_FREQ_FUSE_MASK);
-	} else { /* For pre-production hardware */
-		val = vlv_punit_read(dev_priv, PUNIT_GPU_STATUS_REG);
-		rpn = ((val >> PUNIT_GPU_STATIS_GFX_MIN_FREQ_SHIFT) &
-		       PUNIT_GPU_STATUS_GFX_MIN_FREQ_MASK);
-	}
-
-	return rpn;
-}
-
  static int valleyview_rps_guar_freq(struct drm_i915_private *dev_priv)
  {
  	u32 val, rp1;

@@ -4984,7 +4966,8 @@ static void cherryview_init_gt_powersave(struct drm_device *dev)
  			 intel_gpu_freq(dev_priv, dev_priv->rps.rp1_freq),
  			 dev_priv->rps.rp1_freq);
 
-	dev_priv->rps.min_freq = cherryview_rps_min_freq(dev_priv);
+	/* PUnit validated range is only [RPe, RP0] */
+	dev_priv->rps.min_freq = dev_priv->rps.efficient_freq;
  	DRM_DEBUG_DRIVER("min GPU freq: %d MHz (%u)\n",
  			 intel_gpu_freq(dev_priv, dev_priv->rps.min_freq),
  			 dev_priv->rps.min_freq);

Signature · 1 line

1.9.1

Signature · 6 lines

Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 10 lines · Deepak S

As you suggested it would be better to extend the VLV WA to CHV also to make sure we drop the voltage when idle.

Below is the sequence I think we should follow (based on your comments).
1. forcewake power wells
2. do gfx force clock on
3. request freq to punit
4. release gfx force clock on
5. release forcewake of power wells.

Quoted 1 line

Please share your thoughts?

I'm thinking we shouldn't need the gfx clock force since forcewake should already cause the clock to be enabled.

I've also not verified what happens if we drop the forcewake before Punit has actually finished the frequency change. I'll try to hack up some kind of test to see if I can make that happen.

Quoted 70 lines

Thanks
Deepak

Quoted 65 lines

Quoted 65 lines

So based on my tests this patch feels a bit wrong.

Quoted 56 lines

Since Punit validates the rps range [RPe, RP0]. This patch removes unused cherryview_rps_min_freq function.

v2: Change commit message

v3: set min_freq before idle_freq (chris)

v4: Squash 'Remove unused rps min function' patch

Signed-off-by Deepak S <[email protected]>

Acked-by Chris Wilson <[email protected]>

---
  drivers/gpu/drm/i915/intel_pm.c | 21 ++-------------------
  1 file changed, 2 insertions(+), 19 deletions(-)

drivers/gpu/drm/i915/intel_pm.c +2 -19

index a7516ed..78c89ff 100644
@@ -4715,24 +4715,6 @@ static int cherryview_rps_guar_freq(struct drm_i915_private *dev_priv)
  	return rp1;
  }
 
-static int cherryview_rps_min_freq(struct drm_i915_private *dev_priv)
-{
-	struct drm_device *dev = dev_priv->dev;
-	u32 val, rpn;
-
-	if (dev->pdev->revision >= 0x20) {
-		val = vlv_punit_read(dev_priv, FB_GFX_FMIN_AT_VMIN_FUSE);
-		rpn = ((val >> FB_GFX_FMIN_AT_VMIN_FUSE_SHIFT) &
-		       FB_GFX_FREQ_FUSE_MASK);
-	} else { /* For pre-production hardware */
-		val = vlv_punit_read(dev_priv, PUNIT_GPU_STATUS_REG);
-		rpn = ((val >> PUNIT_GPU_STATIS_GFX_MIN_FREQ_SHIFT) &
-		       PUNIT_GPU_STATUS_GFX_MIN_FREQ_MASK);
-	}
-
-	return rpn;
-}
-
  static int valleyview_rps_guar_freq(struct drm_i915_private *dev_priv)
  {
  	u32 val, rp1;

@@ -4984,7 +4966,8 @@ static void cherryview_init_gt_powersave(struct drm_device *dev)
  			 intel_gpu_freq(dev_priv, dev_priv->rps.rp1_freq),
  			 dev_priv->rps.rp1_freq);
 
-	dev_priv->rps.min_freq = cherryview_rps_min_freq(dev_priv);
+	/* PUnit validated range is only [RPe, RP0] */
+	dev_priv->rps.min_freq = dev_priv->rps.efficient_freq;
  	DRM_DEBUG_DRIVER("min GPU freq: %d MHz (%u)\n",
  			 intel_gpu_freq(dev_priv, dev_priv->rps.min_freq),
  			 dev_priv->rps.min_freq);

Signature · 1 line

1.9.1

Signature · 6 lines

Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 2 lines

Ville Syrjälä
Intel OTC

Quoted 11 lines · Ville Syrjälä

Quoted 10 lines · Deepak S

As you suggested it would be better to extend the VLV WA to CHV also to make sure we drop the voltage when idle.

Below is the sequence I think we should follow (based on your comments).
1. forcewake power wells
2. do gfx force clock on
3. request freq to punit
4. release gfx force clock on
5. release forcewake of power wells.

Quoted 7 lines

Quoted 1 line

Please share your thoughts?

I'm thinking we shouldn't need the gfx clock force since forcewake should already cause the clock to be enabled.

I've also not verified what happens if we drop the forcewake before Punit has actually finished the frequency change. I'll try to hack up some kind of test to see if I can make that happen.

OK, after some hacking I see that the forcewake is enough, and also we don't seem to need to wait for the Punit to finish the frequency change before dropping forcewake. It seems to finish the change even after forcewake has been dropped.

Also for a bit of extra micro optimization we should perhaps wake up the media well only, as that takes a lot less power than the render well.

Quoted 68 lines

Quoted 68 lines

Thanks
Deepak

Quoted 65 lines

Quoted 65 lines

So based on my tests this patch feels a bit wrong.

Quoted 56 lines

Since Punit validates the rps range [RPe, RP0]. This patch removes unused cherryview_rps_min_freq function.

v2: Change commit message

v3: set min_freq before idle_freq (chris)

v4: Squash 'Remove unused rps min function' patch

Signed-off-by Deepak S <[email protected]>

Acked-by Chris Wilson <[email protected]>

---
   drivers/gpu/drm/i915/intel_pm.c | 21 ++-------------------
   1 file changed, 2 insertions(+), 19 deletions(-)

drivers/gpu/drm/i915/intel_pm.c +2 -19

index a7516ed..78c89ff 100644
@@ -4715,24 +4715,6 @@ static int cherryview_rps_guar_freq(struct drm_i915_private *dev_priv)
   	return rp1;
   }
 
-static int cherryview_rps_min_freq(struct drm_i915_private *dev_priv)
-{
-	struct drm_device *dev = dev_priv->dev;
-	u32 val, rpn;
-
-	if (dev->pdev->revision >= 0x20) {
-		val = vlv_punit_read(dev_priv, FB_GFX_FMIN_AT_VMIN_FUSE);
-		rpn = ((val >> FB_GFX_FMIN_AT_VMIN_FUSE_SHIFT) &
-		       FB_GFX_FREQ_FUSE_MASK);
-	} else { /* For pre-production hardware */
-		val = vlv_punit_read(dev_priv, PUNIT_GPU_STATUS_REG);
-		rpn = ((val >> PUNIT_GPU_STATIS_GFX_MIN_FREQ_SHIFT) &
-		       PUNIT_GPU_STATUS_GFX_MIN_FREQ_MASK);
-	}
-
-	return rpn;
-}
-
   static int valleyview_rps_guar_freq(struct drm_i915_private *dev_priv)
   {
   	u32 val, rp1;

@@ -4984,7 +4966,8 @@ static void cherryview_init_gt_powersave(struct drm_device *dev)
   			 intel_gpu_freq(dev_priv, dev_priv->rps.rp1_freq),
   			 dev_priv->rps.rp1_freq);
 
-	dev_priv->rps.min_freq = cherryview_rps_min_freq(dev_priv);
+	/* PUnit validated range is only [RPe, RP0] */
+	dev_priv->rps.min_freq = dev_priv->rps.efficient_freq;
   	DRM_DEBUG_DRIVER("min GPU freq: %d MHz (%u)\n",
   			 intel_gpu_freq(dev_priv, dev_priv->rps.min_freq),
   			 dev_priv->rps.min_freq);

Signature · 1 line

1.9.1

Signature · 6 lines

Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 3 lines

Signature · 2 lines

Ville Syrjälä
Intel OTC

From: Deepak S <[email protected]>

After feedback from the hardware team, now we set the GPU min/idel freq to RPe. Punit is expecting us to operate GPU between Rpe & Rp0. If we drop the frequency to RPn, punit is failing to change the input voltage to minimum :(

Since Punit validates the rps range [RPe, RP0]. This patch removes unused cherryview_rps_min_freq function.

v2: Change commit message

v3: set min_freq before idle_freq (chris)

v4: Squash 'Remove unused rps min function' patch

Signed-off-by Deepak S <[email protected]>

Acked-by Chris Wilson <[email protected]>

---
 drivers/gpu/drm/i915/intel_pm.c | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

drivers/gpu/drm/i915/intel_pm.c +0 -18

index a7516ed..78c89ff 100644
@@ -4715,24 +4715,6 @@ static int cherryview_rps_guar_freq(struct drm_i915_private *dev_priv)
 	return rp1;
 }
 
-static int cherryview_rps_min_freq(struct drm_i915_private *dev_priv)
-{
-	struct drm_device *dev = dev_priv->dev;
-	u32 val, rpn;
-
-	if (dev->pdev->revision >= 0x20) {
-		val = vlv_punit_read(dev_priv, FB_GFX_FMIN_AT_VMIN_FUSE);
-		rpn = ((val >> FB_GFX_FMIN_AT_VMIN_FUSE_SHIFT) &
-		       FB_GFX_FREQ_FUSE_MASK);
-	} else { /* For pre-production hardware */
-		val = vlv_punit_read(dev_priv, PUNIT_GPU_STATUS_REG);
-		rpn = ((val >> PUNIT_GPU_STATIS_GFX_MIN_FREQ_SHIFT) &
-		       PUNIT_GPU_STATUS_GFX_MIN_FREQ_MASK);
-	}
-
-	return rpn;
-}
-
 static int valleyview_rps_guar_freq(struct drm_i915_private *dev_priv)
 {
 	u32 val, rp1;

drivers/gpu/drm/i915/intel_pm.c +0 -0

 	DRM_DEBUG_DRIVER("min GPU freq: %d MHz (%u)\n",
 			 intel_gpu_freq(dev_priv, dev_priv->rps.min_freq),
 			 dev_priv->rps.min_freq);

Signature · 6 lines

1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

From: Deepak S <[email protected]>

Based on the spec, Setting up static BIAS for GPU to improve the rps performace.

Signed-off-by Deepak S <[email protected]>

---
 drivers/gpu/drm/i915/i915_reg.h |  5 +++++
 drivers/gpu/drm/i915/intel_pm.c | 12 ++++++++++++
 2 files changed, 17 insertions(+)

drivers/gpu/drm/i915/i915_reg.h +0 -0

index b522eb6..ea708ba 100644

drivers/gpu/drm/i915/i915_reg.h +1 -0

+
 #define VLV_CZ_CLOCK_TO_MILLI_SEC		100000
 #define VLV_RP_UP_EI_THRESHOLD			90
 #define VLV_RP_DOWN_EI_THRESHOLD		70

drivers/gpu/drm/i915/intel_pm.c +12 -0

index 88e71a3..673612b 100644
@@ -5015,6 +5015,12 @@ static void cherryview_enable_rps(struct drm_device *dev)
 		   GEN6_RP_UP_BUSY_AVG |
 		   GEN6_RP_DOWN_IDLE_AVG);
 
+	/* Setting Fixed Bias */
+	val = VLV_OVERRIDE_RPS_MASK |
+		  VLV_ENABLE_BIAS_SHARE |
+		  VLV_BIAS_VAL;
+	vlv_punit_write(dev_priv, VLV_IOSFB_RPS_OVERRIDE, val);
+
 	val = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
 
 	/* RPS code assumes GPLL is used */

@@ -5099,6 +5105,12 @@ static void valleyview_enable_rps(struct drm_device *dev)
 
 	I915_WRITE(GEN6_RC_CONTROL, rc6_mode);
 
+	/* Setting Fixed Bias */
+	val = VLV_OVERRIDE_RPS_MASK |
+		  VLV_ENABLE_BIAS_SHARE |
+		  VLV_BIAS_VAL;
+	vlv_punit_write(dev_priv, VLV_IOSFB_RPS_OVERRIDE, val);
+
 	val = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
 
 	/* RPS code assumes GPLL is used */

Signature · 1 line

1.9.1

From: Deepak S <[email protected]>

Based on the spec, Setting up static BIAS for GPU to improve the rps performace.

v2: rename reg defn to match spec. (Ville)

v3: Updated bias setting for chv (Deepak)

Signed-off-by Deepak S <[email protected]>

---
 drivers/gpu/drm/i915/i915_reg.h |  6 ++++++
 drivers/gpu/drm/i915/intel_pm.c | 12 ++++++++++++
 2 files changed, 18 insertions(+)

drivers/gpu/drm/i915/i915_reg.h +6 -0

index 36805b6..048987e 100644
@@ -670,6 +670,12 @@ enum skl_disp_power_wells {
 #define   FB_FMAX_VMIN_FREQ_LO_SHIFT		27
 #define   FB_FMAX_VMIN_FREQ_LO_MASK		0xf8000000
 
+#define VLV_TURBO_SOC_OVERRIDE	0x04
+#define 	VLV_OVERRIDE_EN	1
+#define 	VLV_SOC_TDP_EN	(1 << 1)
+#define 	VLV_BIAS_CPU_125_SOC_875 (6 << 2)
+#define 	CHV_BIAS_CPU_50_SOC_50 (3 << 2)
+
 #define VLV_CZ_CLOCK_TO_MILLI_SEC		100000
 
 /* vlv2 north clock has */

drivers/gpu/drm/i915/intel_pm.c +12 -0

index 78c89ff..3689d0e 100644
@@ -5065,6 +5065,12 @@ static void cherryview_enable_rps(struct drm_device *dev)
 		   GEN6_RP_UP_BUSY_AVG |
 		   GEN6_RP_DOWN_IDLE_AVG);
 
+	/* Setting Fixed Bias */
+	val = VLV_OVERRIDE_EN |
+		  VLV_SOC_TDP_EN |
+		  CHV_BIAS_CPU_50_SOC_50;
+	vlv_punit_write(dev_priv, VLV_TURBO_SOC_OVERRIDE, val);
+
 	val = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
 
 	/* RPS code assumes GPLL is used */

@@ -5149,6 +5155,12 @@ static void valleyview_enable_rps(struct drm_device *dev)
 
 	I915_WRITE(GEN6_RC_CONTROL, rc6_mode);
 
+	/* Setting Fixed Bias */
+	val = VLV_OVERRIDE_EN |
+		  VLV_SOC_TDP_EN |
+		  VLV_BIAS_CPU_125_SOC_875;
+	vlv_punit_write(dev_priv, VLV_TURBO_SOC_OVERRIDE, val);
+
 	val = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
 
 	/* RPS code assumes GPLL is used */

Signature · 1 line

1.9.1

Quoted 9 lines · On Wed, Apr 29, 2015 at 08:36:24AM +0530, [email protected] wrote:

From: Deepak S <[email protected]>

Based on the spec, Setting up static BIAS for GPU to improve the rps performace.

v2: rename reg defn to match spec. (Ville)

v3: Updated bias setting for chv (Deepak)

Quoted 1 line

Signed-off-by Deepak S <[email protected]>

Matches the spec. Whether the chosen bias is really the best, I can't really say. But favoring the GPU does seem like a sensible idea if we want to keep the UI stuff fluid enough while there's some CPU heavy tasks running at the same time.

Reviewed-by Ville Syrjälä <[email protected]>

Quoted 51 lines

---
  drivers/gpu/drm/i915/i915_reg.h |  6 ++++++
  drivers/gpu/drm/i915/intel_pm.c | 12 ++++++++++++
  2 files changed, 18 insertions(+)

drivers/gpu/drm/i915/i915_reg.h +6 -0

index 36805b6..048987e 100644
@@ -670,6 +670,12 @@ enum skl_disp_power_wells {
  #define   FB_FMAX_VMIN_FREQ_LO_SHIFT		27
  #define   FB_FMAX_VMIN_FREQ_LO_MASK		0xf8000000
 
+#define VLV_TURBO_SOC_OVERRIDE	0x04
+#define 	VLV_OVERRIDE_EN	1
+#define 	VLV_SOC_TDP_EN	(1 << 1)
+#define 	VLV_BIAS_CPU_125_SOC_875 (6 << 2)
+#define 	CHV_BIAS_CPU_50_SOC_50 (3 << 2)
+
  #define VLV_CZ_CLOCK_TO_MILLI_SEC		100000
 
  /* vlv2 north clock has */

drivers/gpu/drm/i915/intel_pm.c +12 -0

index 78c89ff..3689d0e 100644
@@ -5065,6 +5065,12 @@ static void cherryview_enable_rps(struct drm_device *dev)
  		   GEN6_RP_UP_BUSY_AVG |
  		   GEN6_RP_DOWN_IDLE_AVG);
 
+	/* Setting Fixed Bias */
+	val = VLV_OVERRIDE_EN |
+		  VLV_SOC_TDP_EN |
+		  CHV_BIAS_CPU_50_SOC_50;
+	vlv_punit_write(dev_priv, VLV_TURBO_SOC_OVERRIDE, val);
+
  	val = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
 
  	/* RPS code assumes GPLL is used */

@@ -5149,6 +5155,12 @@ static void valleyview_enable_rps(struct drm_device *dev)
 
  	I915_WRITE(GEN6_RC_CONTROL, rc6_mode);
 
+	/* Setting Fixed Bias */
+	val = VLV_OVERRIDE_EN |
+		  VLV_SOC_TDP_EN |
+		  VLV_BIAS_CPU_125_SOC_875;
+	vlv_punit_write(dev_priv, VLV_TURBO_SOC_OVERRIDE, val);
+
  	val = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
 

Quoted 3 lines

drivers/gpu/drm/i915/intel_pm.c +0 -0

  	/* RPS code assumes GPLL is used */

Signature · 1 line

1.9.1

Quoted 16 lines · Ville Syrjälä

Quoted 10 lines · On Wed, Apr 29, 2015 at 08:36:24AM +0530, [email protected] wrote:

From: Deepak S <[email protected]>

Based on the spec, Setting up static BIAS for GPU to improve the rps performace.

v2: rename reg defn to match spec. (Ville)

v3: Updated bias setting for chv (Deepak)

Signed-off-by Deepak S <[email protected]>

Matches the spec. Whether the chosen bias is really the best, I can't really say. But favoring the GPU does seem like a sensible idea if we want to keep the UI stuff fluid enough while there's some CPU heavy tasks running at the same time.

Quoted 1 line

Reviewed-by Ville Syrjälä <[email protected]>

Thanks Ville for reviewing,
Yes our aim is to keep user experience smooth.

Quoted 40 lines

Quoted 40 lines

---
 drivers/gpu/drm/i915/i915_reg.h |  6 ++++++
 drivers/gpu/drm/i915/intel_pm.c | 12 ++++++++++++
 2 files changed, 18 insertions(+)

drivers/gpu/drm/i915/i915_reg.h +6 -0

index 36805b6..048987e 100644
@@ -670,6 +670,12 @@ enum skl_disp_power_wells {
 #define   FB_FMAX_VMIN_FREQ_LO_SHIFT		27
 #define   FB_FMAX_VMIN_FREQ_LO_MASK		0xf8000000
+#define VLV_TURBO_SOC_OVERRIDE	0x04
+#define 	VLV_OVERRIDE_EN	1
+#define 	VLV_SOC_TDP_EN	(1 << 1)
+#define 	VLV_BIAS_CPU_125_SOC_875 (6 << 2)
+#define 	CHV_BIAS_CPU_50_SOC_50 (3 << 2)
+
 #define VLV_CZ_CLOCK_TO_MILLI_SEC		100000
 /* vlv2 north clock has */

drivers/gpu/drm/i915/intel_pm.c +8 -0

index 78c89ff..3689d0e 100644
@@ -5065,6 +5065,12 @@ static void cherryview_enable_rps(struct drm_device *dev)
 		   GEN6_RP_UP_BUSY_AVG |
 		   GEN6_RP_DOWN_IDLE_AVG);
+	/* Setting Fixed Bias */
+	val = VLV_OVERRIDE_EN |
+		  VLV_SOC_TDP_EN |
+		  CHV_BIAS_CPU_50_SOC_50;
+	vlv_punit_write(dev_priv, VLV_TURBO_SOC_OVERRIDE, val);
+
 	val = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
 	/* RPS code assumes GPLL is used */

@@ -5149,6 +5155,12 @@ static void valleyview_enable_rps(struct drm_device *dev)
 	I915_WRITE(GEN6_RC_CONTROL, rc6_mode);
+	/* Setting Fixed Bias */
+	val = VLV_OVERRIDE_EN |

Quoted 8 lines

Quoted 8 lines

drivers/gpu/drm/i915/intel_pm.c +4 -0

+		  VLV_SOC_TDP_EN |
+		  VLV_BIAS_CPU_125_SOC_875;
+	vlv_punit_write(dev_priv, VLV_TURBO_SOC_OVERRIDE, val);
+
 	val = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
 	/* RPS code assumes GPLL is used */

Signature · 1 line

1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 19 lines · Deepak S

Quoted 16 lines · Ville Syrjälä

Quoted 10 lines · On Wed, Apr 29, 2015 at 08:36:24AM +0530, [email protected] wrote:

From: Deepak S <[email protected]>

Based on the spec, Setting up static BIAS for GPU to improve the rps performace.

v2: rename reg defn to match spec. (Ville)

v3: Updated bias setting for chv (Deepak)

Signed-off-by Deepak S <[email protected]>

Matches the spec. Whether the chosen bias is really the best, I can't really say. But favoring the GPU does seem like a sensible idea if we want to keep the UI stuff fluid enough while there's some CPU heavy tasks running at the same time.

Quoted 4 lines

Quoted 1 line

Reviewed-by Ville Syrjälä <[email protected]>

Thanks Ville for reviewing,
Yes our aim is to keep user experience smooth.

Since the aim is smooth UI ... how does this interact with the rps boosting that was just enabled with Chris' patches for vlv/chv too?

A static bias seems a lot less what we want now that we should have something dynamic. Specifically I'm thinking of

commit 6ad790c0f5ac55fd13f322c23519f0d6f0721864

Author: Chris Wilson <[email protected]>
Date:   Tue Apr 7 16:20:31 2015 +0100

drm/i915: Boost GPU frequency if we detect outstanding pageflips

Cheers, Daniel

Quoted 53 lines

Quoted 40 lines

Quoted 40 lines

---
 drivers/gpu/drm/i915/i915_reg.h |  6 ++++++
 drivers/gpu/drm/i915/intel_pm.c | 12 ++++++++++++
 2 files changed, 18 insertions(+)

drivers/gpu/drm/i915/i915_reg.h +6 -0

index 36805b6..048987e 100644
@@ -670,6 +670,12 @@ enum skl_disp_power_wells {
 #define   FB_FMAX_VMIN_FREQ_LO_SHIFT		27
 #define   FB_FMAX_VMIN_FREQ_LO_MASK		0xf8000000
+#define VLV_TURBO_SOC_OVERRIDE	0x04
+#define 	VLV_OVERRIDE_EN	1
+#define 	VLV_SOC_TDP_EN	(1 << 1)
+#define 	VLV_BIAS_CPU_125_SOC_875 (6 << 2)
+#define 	CHV_BIAS_CPU_50_SOC_50 (3 << 2)
+
 #define VLV_CZ_CLOCK_TO_MILLI_SEC		100000
 /* vlv2 north clock has */

drivers/gpu/drm/i915/intel_pm.c +8 -0

index 78c89ff..3689d0e 100644
@@ -5065,6 +5065,12 @@ static void cherryview_enable_rps(struct drm_device *dev)
 		   GEN6_RP_UP_BUSY_AVG |
 		   GEN6_RP_DOWN_IDLE_AVG);
+	/* Setting Fixed Bias */
+	val = VLV_OVERRIDE_EN |
+		  VLV_SOC_TDP_EN |
+		  CHV_BIAS_CPU_50_SOC_50;
+	vlv_punit_write(dev_priv, VLV_TURBO_SOC_OVERRIDE, val);
+
 	val = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
 	/* RPS code assumes GPLL is used */

@@ -5149,6 +5155,12 @@ static void valleyview_enable_rps(struct drm_device *dev)
 	I915_WRITE(GEN6_RC_CONTROL, rc6_mode);
+	/* Setting Fixed Bias */
+	val = VLV_OVERRIDE_EN |

Quoted 8 lines

Quoted 8 lines

drivers/gpu/drm/i915/intel_pm.c +4 -0

+		  VLV_SOC_TDP_EN |
+		  VLV_BIAS_CPU_125_SOC_875;
+	vlv_punit_write(dev_priv, VLV_TURBO_SOC_OVERRIDE, val);
+
 	val = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
 	/* RPS code assumes GPLL is used */

Signature · 1 line

1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Quoted 32 lines · Daniel Vetter

Quoted 13 lines · Deepak S

Quoted 11 lines · Ville Syrjälä

Quoted 10 lines · On Wed, Apr 29, 2015 at 08:36:24AM +0530, [email protected] wrote:

From: Deepak S <[email protected]>

Based on the spec, Setting up static BIAS for GPU to improve the rps performace.

v2: rename reg defn to match spec. (Ville)

v3: Updated bias setting for chv (Deepak)

Signed-off-by Deepak S <[email protected]>

Quoted 8 lines

Quoted 6 lines

Matches the spec. Whether the chosen bias is really the best, I can't really say. But favoring the GPU does seem like a sensible idea if we want to keep the UI stuff fluid enough while there's some CPU heavy tasks running at the same time.

Reviewed-by Ville Syrjälä <[email protected]>

Thanks Ville for reviewing,
Yes our aim is to keep user experience smooth.

Since the aim is smooth UI ... how does this interact with the rps boosting that was just enabled with Chris' patches for vlv/chv too?

A static bias seems a lot less what we want now that we should have something dynamic. Specifically I'm thinking of

commit 6ad790c0f5ac55fd13f322c23519f0d6f0721864

Author: Chris Wilson <[email protected]>
Date:   Tue Apr 7 16:20:31 2015 +0100

Quoted 1 line

drm/i915: Boost GPU frequency if we detect outstanding pageflips

Totally separate topic. This only affects how the Punit splits up the available energy credits between the CPU and the GPU. So only relevant when thermally constrained and both CPU and GPU would like to run faster than the limit allows.

Quoted 53 lines

Cheers, Daniel

Quoted 51 lines

Quoted 39 lines

Quoted 39 lines

---
  drivers/gpu/drm/i915/i915_reg.h |  6 ++++++
  drivers/gpu/drm/i915/intel_pm.c | 12 ++++++++++++
  2 files changed, 18 insertions(+)

drivers/gpu/drm/i915/i915_reg.h +6 -0

index 36805b6..048987e 100644
@@ -670,6 +670,12 @@ enum skl_disp_power_wells {
  #define   FB_FMAX_VMIN_FREQ_LO_SHIFT		27
  #define   FB_FMAX_VMIN_FREQ_LO_MASK		0xf8000000
+#define VLV_TURBO_SOC_OVERRIDE	0x04
+#define 	VLV_OVERRIDE_EN	1
+#define 	VLV_SOC_TDP_EN	(1 << 1)
+#define 	VLV_BIAS_CPU_125_SOC_875 (6 << 2)
+#define 	CHV_BIAS_CPU_50_SOC_50 (3 << 2)
+
  #define VLV_CZ_CLOCK_TO_MILLI_SEC		100000
  /* vlv2 north clock has */

drivers/gpu/drm/i915/intel_pm.c +8 -0

index 78c89ff..3689d0e 100644
@@ -5065,6 +5065,12 @@ static void cherryview_enable_rps(struct drm_device *dev)
  		   GEN6_RP_UP_BUSY_AVG |
  		   GEN6_RP_DOWN_IDLE_AVG);
+	/* Setting Fixed Bias */
+	val = VLV_OVERRIDE_EN |
+		  VLV_SOC_TDP_EN |
+		  CHV_BIAS_CPU_50_SOC_50;
+	vlv_punit_write(dev_priv, VLV_TURBO_SOC_OVERRIDE, val);
+
  	val = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
  	/* RPS code assumes GPLL is used */

@@ -5149,6 +5155,12 @@ static void valleyview_enable_rps(struct drm_device *dev)
  	I915_WRITE(GEN6_RC_CONTROL, rc6_mode);
+	/* Setting Fixed Bias */
+	val = VLV_OVERRIDE_EN |

Quoted 8 lines

Quoted 8 lines

drivers/gpu/drm/i915/intel_pm.c +4 -0

+		  VLV_SOC_TDP_EN |
+		  VLV_BIAS_CPU_125_SOC_875;
+	vlv_punit_write(dev_priv, VLV_TURBO_SOC_OVERRIDE, val);
+
  	val = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
  	/* RPS code assumes GPLL is used */

Signature · 1 line

1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 4 lines

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Quoted 33 lines · Ville Syrjälä

Quoted 32 lines · Daniel Vetter

Quoted 13 lines · Deepak S

Quoted 11 lines · Ville Syrjälä

Quoted 10 lines · On Wed, Apr 29, 2015 at 08:36:24AM +0530, [email protected] wrote:

From: Deepak S <[email protected]>

Based on the spec, Setting up static BIAS for GPU to improve the rps performace.

v2: rename reg defn to match spec. (Ville)

v3: Updated bias setting for chv (Deepak)

Signed-off-by Deepak S <[email protected]>

Quoted 8 lines

Quoted 6 lines

Matches the spec. Whether the chosen bias is really the best, I can't really say. But favoring the GPU does seem like a sensible idea if we want to keep the UI stuff fluid enough while there's some CPU heavy tasks running at the same time.

Reviewed-by Ville Syrjälä <[email protected]>

Thanks Ville for reviewing,
Yes our aim is to keep user experience smooth.

Since the aim is smooth UI ... how does this interact with the rps boosting that was just enabled with Chris' patches for vlv/chv too?

A static bias seems a lot less what we want now that we should have something dynamic. Specifically I'm thinking of

commit 6ad790c0f5ac55fd13f322c23519f0d6f0721864

Author: Chris Wilson <[email protected]>
Date:   Tue Apr 7 16:20:31 2015 +0100

Quoted 6 lines

Quoted 1 line

drm/i915: Boost GPU frequency if we detect outstanding pageflips

Totally separate topic. This only affects how the Punit splits up the available energy credits between the CPU and the GPU. So only relevant when thermally constrained and both CPU and GPU would like to run faster than the limit allows.

Thanks Ville.

This is completely a different topic, more to do with punit budget constraints & this does not impact "Boot RPS logic"

Quoted 40 lines · Deepak S

Quoted 32 lines · Ville Syrjälä

Quoted 31 lines · Daniel Vetter

Quoted 12 lines · Deepak S

Quoted 11 lines · Ville Syrjälä

Quoted 10 lines · On Wed, Apr 29, 2015 at 08:36:24AM +0530, [email protected] wrote:

From: Deepak S <[email protected]>

Based on the spec, Setting up static BIAS for GPU to improve the rps performace.

v2: rename reg defn to match spec. (Ville)

v3: Updated bias setting for chv (Deepak)

Signed-off-by Deepak S <[email protected]>

Quoted 8 lines

Quoted 6 lines

Matches the spec. Whether the chosen bias is really the best, I can't really say. But favoring the GPU does seem like a sensible idea if we want to keep the UI stuff fluid enough while there's some CPU heavy tasks running at the same time.

Reviewed-by Ville Syrjälä <[email protected]>

Thanks Ville for reviewing,
Yes our aim is to keep user experience smooth.

Since the aim is smooth UI ... how does this interact with the rps boosting that was just enabled with Chris' patches for vlv/chv too?

A static bias seems a lot less what we want now that we should have something dynamic. Specifically I'm thinking of

commit 6ad790c0f5ac55fd13f322c23519f0d6f0721864

Author: Chris Wilson <[email protected]>
Date:   Tue Apr 7 16:20:31 2015 +0100

Quoted 6 lines

Quoted 1 line

drm/i915: Boost GPU frequency if we detect outstanding pageflips

Totally separate topic. This only affects how the Punit splits up the available energy credits between the CPU and the GPU. So only relevant when thermally constrained and both CPU and GPU would like to run faster than the limit allows.

Quoted 4 lines

Thanks Ville.

This is completely a different topic, more to do with punit budget constraints & this does not impact "Boot RPS logic"

Well I thought it'd be related since the justification was better UI interactivity. And that tends to not be a thermal constrained load but something really spike-y. And a static bias doesn't seem like a solution to that problem.

I'll just go ahead and merge, but still feels like at least I don't know why exactly we need this.
-Daniel