Skip to contentSkip to conversation

drm/i915: Drop WA to fix Voltage not getting dropped to Vmin when Gfx is power gated.

PATCH Superseded [email protected] 4 participants 11 messages Jun 13, 2014 – Jul 2, 2014

Subsystems (2)
DRM drivers Intel i915
lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 1/1 drm/i915: Drop WA to fix Voltage not getting dropped to Vmin when Gfx is power gated.

From: Deepak S <[email protected]>

Workaround fixed in BYT. Forcing Gfx clk up not needed, and Requesting the min freq should bring bring the voltage Vnn.

Signed-off-by Deepak S <[email protected]>
---
 drivers/gpu/drm/i915/intel_pm.c | 40 +---------------------------------------
 1 file changed, 1 insertion(+), 39 deletions(-)
drivers/gpu/drm/i915/intel_pm.c +1 -39
index 0b088fe..9aee28b 100644
@@ -3198,44 +3198,6 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
 	trace_intel_gpu_freq_change(val * 50);
 }
 
-/* vlv_set_rps_idle: Set the frequency to Rpn if Gfx clocks are down
- *
- * * If Gfx is Idle, then
- * 1. Mask Turbo interrupts
- * 2. Bring up Gfx clock
- * 3. Change the freq to Rpn and wait till P-Unit updates freq
- * 4. Clear the Force GFX CLK ON bit so that Gfx can down
- * 5. Unmask Turbo interrupts
-*/
-static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
-{
-	/*
-	 * When we are idle.  Drop to min voltage state.
-	 */
-
-	if (dev_priv->rps.cur_freq <= dev_priv->rps.min_freq_softlimit)
-		return;
-
-	/* Mask turbo interrupt so that they will not come in between */
-	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
-
-	vlv_force_gfx_clock(dev_priv, true);
-
-	dev_priv->rps.cur_freq = dev_priv->rps.min_freq_softlimit;
-
-	vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ,
-					dev_priv->rps.min_freq_softlimit);
-
-	if (wait_for(((vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS))
-				& GENFREQSTATUS) == 0, 5))
-		DRM_ERROR("timed out waiting for Punit\n");
-
-	vlv_force_gfx_clock(dev_priv, false);
-
-	I915_WRITE(GEN6_PMINTRMSK,
-		   gen6_rps_pm_mask(dev_priv, dev_priv->rps.cur_freq));
-}
-
 void gen6_rps_idle(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;

@@ -3243,7 +3205,7 @@ void gen6_rps_idle(struct drm_i915_private *dev_priv)
 	mutex_lock(&dev_priv->rps.hw_lock);
 	if (dev_priv->rps.enabled) {
 		if (IS_VALLEYVIEW(dev))
-			vlv_set_rps_idle(dev_priv);
+			valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
 		else
 			gen6_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
 		dev_priv->rps.last_adj = 0;
Signature · 1 line
1.9.1
Acked-by on v1 1/1
#1 lore raw
Quoted 60 lines · On Fri, Jun 13, 2014 at 03:46:14PM +0530, [email protected] wrote:

From: Deepak S <[email protected]>

Workaround fixed in BYT. Forcing Gfx clk up not needed, and Requesting the min freq should bring bring the voltage Vnn.

Signed-off-by Deepak S <[email protected]>
---
 drivers/gpu/drm/i915/intel_pm.c | 40 +---------------------------------------
 1 file changed, 1 insertion(+), 39 deletions(-)
drivers/gpu/drm/i915/intel_pm.c +0 -38
index 0b088fe..9aee28b 100644
@@ -3198,44 +3198,6 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
 	trace_intel_gpu_freq_change(val * 50);
 }
 
-/* vlv_set_rps_idle: Set the frequency to Rpn if Gfx clocks are down
- *
- * * If Gfx is Idle, then
- * 1. Mask Turbo interrupts
- * 2. Bring up Gfx clock
- * 3. Change the freq to Rpn and wait till P-Unit updates freq
- * 4. Clear the Force GFX CLK ON bit so that Gfx can down
- * 5. Unmask Turbo interrupts
-*/
-static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
-{
-	/*
-	 * When we are idle.  Drop to min voltage state.
-	 */
-
-	if (dev_priv->rps.cur_freq <= dev_priv->rps.min_freq_softlimit)
-		return;
-
-	/* Mask turbo interrupt so that they will not come in between */
-	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
-
-	vlv_force_gfx_clock(dev_priv, true);
-
-	dev_priv->rps.cur_freq = dev_priv->rps.min_freq_softlimit;
-
-	vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ,
-					dev_priv->rps.min_freq_softlimit);
-
-	if (wait_for(((vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS))
-				& GENFREQSTATUS) == 0, 5))
-		DRM_ERROR("timed out waiting for Punit\n");
-
-	vlv_force_gfx_clock(dev_priv, false);
-
-	I915_WRITE(GEN6_PMINTRMSK,
-		   gen6_rps_pm_mask(dev_priv, dev_priv->rps.cur_freq));
-}
-
 void gen6_rps_idle(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;

drivers/gpu/drm/i915/intel_pm.c void gen6_rps_idle(struct drm_i915_private *dev_priv) · line 3208

Quoted 5 lines
drivers/gpu/drm/i915/intel_pm.c +1 -1
@@ -3243,7 +3205,7 @@ void gen6_rps_idle(struct drm_i915_private *dev_priv)
 	mutex_lock(&dev_priv->rps.hw_lock);
 	if (dev_priv->rps.enabled) {
 		if (IS_VALLEYVIEW(dev))
-			vlv_set_rps_idle(dev_priv);
+			valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);

This should take care of https://bugs.freedesktop.org/show_bug.cgi?id=75244

I don't know when the hardware got fixed so I'm hesitant to r-b it, but at least my C0 works fine without this stuff, so:

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

However to avoid future mishaps I think we should have some kind of a comment before the valleyview_set_rps() call to let the reader know that we really need this on VLV to drop the voltage.

Also it now occurs to me that we might be leaving the GPU frequency (and thus Vnn) high during a system suspend. I think we need an explicit rps_idle() call in the suspend path somewhere. Runtime suspend should be fine already since it depends on intel_mark_idle() getting called before the last rpm reference is dropped.

Quoted 8 lines
drivers/gpu/drm/i915/intel_pm.c +0 -0
 		else
 			gen6_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);

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

Signature · 2 lines
Ville Syrjälä
Intel OTC
#2 lore raw
Quoted 77 lines · Ville Syrjälä
Quoted 60 lines · On Fri, Jun 13, 2014 at 03:46:14PM +0530, [email protected] wrote:

From: Deepak S <[email protected]>

Workaround fixed in BYT. Forcing Gfx clk up not needed, and Requesting the min freq should bring bring the voltage Vnn.

Signed-off-by Deepak S <[email protected]>
---
 drivers/gpu/drm/i915/intel_pm.c | 40 +---------------------------------------
 1 file changed, 1 insertion(+), 39 deletions(-)
drivers/gpu/drm/i915/intel_pm.c +0 -38
index 0b088fe..9aee28b 100644
@@ -3198,44 +3198,6 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
 	trace_intel_gpu_freq_change(val * 50);
 }
 
-/* vlv_set_rps_idle: Set the frequency to Rpn if Gfx clocks are down
- *
- * * If Gfx is Idle, then
- * 1. Mask Turbo interrupts
- * 2. Bring up Gfx clock
- * 3. Change the freq to Rpn and wait till P-Unit updates freq
- * 4. Clear the Force GFX CLK ON bit so that Gfx can down
- * 5. Unmask Turbo interrupts
-*/
-static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
-{
-	/*
-	 * When we are idle.  Drop to min voltage state.
-	 */
-
-	if (dev_priv->rps.cur_freq <= dev_priv->rps.min_freq_softlimit)
-		return;
-
-	/* Mask turbo interrupt so that they will not come in between */
-	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
-
-	vlv_force_gfx_clock(dev_priv, true);
-
-	dev_priv->rps.cur_freq = dev_priv->rps.min_freq_softlimit;
-
-	vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ,
-					dev_priv->rps.min_freq_softlimit);
-
-	if (wait_for(((vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS))
-				& GENFREQSTATUS) == 0, 5))
-		DRM_ERROR("timed out waiting for Punit\n");
-
-	vlv_force_gfx_clock(dev_priv, false);
-
-	I915_WRITE(GEN6_PMINTRMSK,
-		   gen6_rps_pm_mask(dev_priv, dev_priv->rps.cur_freq));
-}
-
 void gen6_rps_idle(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;

drivers/gpu/drm/i915/intel_pm.c void gen6_rps_idle(struct drm_i915_private *dev_priv) · line 3208

Quoted 5 lines
drivers/gpu/drm/i915/intel_pm.c +1 -1
@@ -3243,7 +3205,7 @@ void gen6_rps_idle(struct drm_i915_private *dev_priv)
 	mutex_lock(&dev_priv->rps.hw_lock);
 	if (dev_priv->rps.enabled) {
 		if (IS_VALLEYVIEW(dev))
-			vlv_set_rps_idle(dev_priv);
+			valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);

This should take care of https://bugs.freedesktop.org/show_bug.cgi?id=75244

I don't know when the hardware got fixed so I'm hesitant to r-b it, but at least my C0 works fine without this stuff, so:

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

However to avoid future mishaps I think we should have some kind of a comment before the valleyview_set_rps() call to let the reader know that we really need this on VLV to drop the voltage.

Quoted 5 lines

Also it now occurs to me that we might be leaving the GPU frequency (and thus Vnn) high during a system suspend. I think we need an explicit rps_idle() call in the suspend path somewhere. Runtime suspend should be fine already since it depends on intel_mark_idle() getting called before the last rpm reference is dropped.

Maybe this is all we need?

drivers/gpu/drm/i915/i915_gem.c +1 -1
index 3768199..fabd852 100644
@@ -4530,7 +4530,7 @@ i915_gem_suspend(struct drm_device *dev)
 
        del_timer_sync(&dev_priv->gpu_error.hangcheck_timer);
        cancel_delayed_work_sync(&dev_priv->mm.retire_work);
-       cancel_delayed_work_sync(&dev_priv->mm.idle_work);
+       flush_delayed_work(&dev_priv->mm.idle_work);
 
        return 0;
Signature · 2 lines
Ville Syrjälä
Intel OTC
author
#3 lore raw
Quoted 69 lines · Ville Syrjälä
Quoted 68 lines · Ville Syrjälä
Quoted 60 lines · On Fri, Jun 13, 2014 at 03:46:14PM +0530, [email protected] wrote:

From: Deepak S <[email protected]>

Workaround fixed in BYT. Forcing Gfx clk up not needed, and Requesting the min freq should bring bring the voltage Vnn.

Signed-off-by Deepak S <[email protected]>
---
  drivers/gpu/drm/i915/intel_pm.c | 40 +---------------------------------------
  1 file changed, 1 insertion(+), 39 deletions(-)
drivers/gpu/drm/i915/intel_pm.c +0 -38
index 0b088fe..9aee28b 100644
@@ -3198,44 +3198,6 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
  	trace_intel_gpu_freq_change(val * 50);
  }
 
-/* vlv_set_rps_idle: Set the frequency to Rpn if Gfx clocks are down
- *
- * * If Gfx is Idle, then
- * 1. Mask Turbo interrupts
- * 2. Bring up Gfx clock
- * 3. Change the freq to Rpn and wait till P-Unit updates freq
- * 4. Clear the Force GFX CLK ON bit so that Gfx can down
- * 5. Unmask Turbo interrupts
-*/
-static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
-{
-	/*
-	 * When we are idle.  Drop to min voltage state.
-	 */
-
-	if (dev_priv->rps.cur_freq <= dev_priv->rps.min_freq_softlimit)
-		return;
-
-	/* Mask turbo interrupt so that they will not come in between */
-	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
-
-	vlv_force_gfx_clock(dev_priv, true);
-
-	dev_priv->rps.cur_freq = dev_priv->rps.min_freq_softlimit;
-
-	vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ,
-					dev_priv->rps.min_freq_softlimit);
-
-	if (wait_for(((vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS))
-				& GENFREQSTATUS) == 0, 5))
-		DRM_ERROR("timed out waiting for Punit\n");
-
-	vlv_force_gfx_clock(dev_priv, false);
-
-	I915_WRITE(GEN6_PMINTRMSK,
-		   gen6_rps_pm_mask(dev_priv, dev_priv->rps.cur_freq));
-}
-
  void gen6_rps_idle(struct drm_i915_private *dev_priv)
  {
  	struct drm_device *dev = dev_priv->dev;

drivers/gpu/drm/i915/intel_pm.c void gen6_rps_idle(struct drm_i915_private *dev_priv) · line 3208

Quoted 5 lines
drivers/gpu/drm/i915/intel_pm.c +1 -1
@@ -3243,7 +3205,7 @@ void gen6_rps_idle(struct drm_i915_private *dev_priv)
  	mutex_lock(&dev_priv->rps.hw_lock);
  	if (dev_priv->rps.enabled) {
  		if (IS_VALLEYVIEW(dev))
-			vlv_set_rps_idle(dev_priv);
+			valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);

This should take care of https://bugs.freedesktop.org/show_bug.cgi?id=75244

Quoted 7 lines
Quoted 7 lines

I don't know when the hardware got fixed so I'm hesitant to r-b it, but at least my C0 works fine without this stuff, so:

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

However to avoid future mishaps I think we should have some kind of a comment before the valleyview_set_rps() call to let the reader know that we really need this on VLV to drop the voltage.

hmm, Yes we might need this for other stepping. I will add a comment

Thanks for the review

Quoted 12 lines
Quoted 5 lines

Also it now occurs to me that we might be leaving the GPU frequency (and thus Vnn) high during a system suspend. I think we need an explicit rps_idle() call in the suspend path somewhere. Runtime suspend should be fine already since it depends on intel_mark_idle() getting called before the last rpm reference is dropped.

Maybe this is all we need?

drivers/gpu/drm/i915/i915_gem.c +0 -0
index 3768199..fabd852 100644

drivers/gpu/drm/i915/i915_gem.c i915_gem_suspend(struct drm_device *dev) · line 4535

Quoted 7 lines
drivers/gpu/drm/i915/i915_gem.c +1 -1
@@ -4530,7 +4530,7 @@ i915_gem_suspend(struct drm_device *dev)
 
         del_timer_sync(&dev_priv->gpu_error.hangcheck_timer);
         cancel_delayed_work_sync(&dev_priv->mm.retire_work);
-       cancel_delayed_work_sync(&dev_priv->mm.idle_work);
+       flush_delayed_work(&dev_priv->mm.idle_work);
 
         return 0;

Yes, while suspending we need move GPU to min_freq. flush_delayed_work should be fine. Let me create a patch for this.

#4 lore raw
Quoted 74 lines · Deepak S
Quoted 72 lines · Ville Syrjälä
Quoted 71 lines · Ville Syrjälä
Quoted 59 lines · On Fri, Jun 13, 2014 at 03:46:14PM +0530, [email protected] wrote:

From: Deepak S <[email protected]>

Workaround fixed in BYT. Forcing Gfx clk up not needed, and Requesting the min freq should bring bring the voltage Vnn.

Signed-off-by Deepak S <[email protected]>
---
 drivers/gpu/drm/i915/intel_pm.c | 40 +---------------------------------------
 1 file changed, 1 insertion(+), 39 deletions(-)
drivers/gpu/drm/i915/intel_pm.c +0 -38
index 0b088fe..9aee28b 100644
@@ -3198,44 +3198,6 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
 	trace_intel_gpu_freq_change(val * 50);
 }
-/* vlv_set_rps_idle: Set the frequency to Rpn if Gfx clocks are down
- *
- * * If Gfx is Idle, then
- * 1. Mask Turbo interrupts
- * 2. Bring up Gfx clock
- * 3. Change the freq to Rpn and wait till P-Unit updates freq
- * 4. Clear the Force GFX CLK ON bit so that Gfx can down
- * 5. Unmask Turbo interrupts
-*/
-static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
-{
-	/*
-	 * When we are idle.  Drop to min voltage state.
-	 */
-
-	if (dev_priv->rps.cur_freq <= dev_priv->rps.min_freq_softlimit)
-		return;
-
-	/* Mask turbo interrupt so that they will not come in between */
-	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
-
-	vlv_force_gfx_clock(dev_priv, true);
-
-	dev_priv->rps.cur_freq = dev_priv->rps.min_freq_softlimit;
-
-	vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ,
-					dev_priv->rps.min_freq_softlimit);
-
-	if (wait_for(((vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS))
-				& GENFREQSTATUS) == 0, 5))
-		DRM_ERROR("timed out waiting for Punit\n");
-
-	vlv_force_gfx_clock(dev_priv, false);
-
-	I915_WRITE(GEN6_PMINTRMSK,
-		   gen6_rps_pm_mask(dev_priv, dev_priv->rps.cur_freq));
-}
-
 void gen6_rps_idle(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;

drivers/gpu/drm/i915/intel_pm.c void gen6_rps_idle(struct drm_i915_private *dev_priv) · line 3208

Quoted 5 lines
drivers/gpu/drm/i915/intel_pm.c +1 -1
@@ -3243,7 +3205,7 @@ void gen6_rps_idle(struct drm_i915_private *dev_priv)
 	mutex_lock(&dev_priv->rps.hw_lock);
 	if (dev_priv->rps.enabled) {
 		if (IS_VALLEYVIEW(dev))
-			vlv_set_rps_idle(dev_priv);
+			valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);

This should take care of https://bugs.freedesktop.org/show_bug.cgi?id=75244

I don't know when the hardware got fixed so I'm hesitant to r-b it, but at least my C0 works fine without this stuff, so:

Acked-by Ville Syrjälä <[email protected]>
Quoted 5 lines
Quoted 3 lines
Quoted 3 lines

However to avoid future mishaps I think we should have some kind of a comment before the valleyview_set_rps() call to let the reader know that we really need this on VLV to drop the voltage.

hmm, Yes we might need this for other stepping. I will add a comment

Please don't put the stepping info in the comment or commit message though, that freaks out people ;-) Usually we go with "pre-production" or "early revisions" or something non-specific.

Quoted 10 lines

Thanks for the review

Quoted 7 lines
Quoted 5 lines

Also it now occurs to me that we might be leaving the GPU frequency (and thus Vnn) high during a system suspend. I think we need an explicit rps_idle() call in the suspend path somewhere. Runtime suspend should be fine already since it depends on intel_mark_idle() getting called before the last rpm reference is dropped.

Maybe this is all we need?

Quoted 13 lines

drivers/gpu/drm/i915/i915_gem.c i915_gem_suspend(struct drm_device *dev) · line 4533

Quoted 10 lines
drivers/gpu/drm/i915/i915_gem.c +1 -1
index 3768199..fabd852 100644
@@ -4530,7 +4530,7 @@ i915_gem_suspend(struct drm_device *dev)
        del_timer_sync(&dev_priv->gpu_error.hangcheck_timer);
        cancel_delayed_work_sync(&dev_priv->mm.retire_work);
-       cancel_delayed_work_sync(&dev_priv->mm.idle_work);
+       flush_delayed_work(&dev_priv->mm.idle_work);
        return 0;

Yes, while suspending we need move GPU to min_freq. flush_delayed_work should be fine. Let me create a patch for this.

Since this is gt powersave related can you please also check whether we shouldn't move this to the intel_suspend_gt_powersave function Jesse recently added to -nightly?

Thanks, Daniel

Signature · 3 lines
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
author
#5 lore raw
Quoted 74 lines · Daniel Vetter
Quoted 73 lines · Deepak S
Quoted 72 lines · Ville Syrjälä
Quoted 71 lines · Ville Syrjälä
Quoted 59 lines · On Fri, Jun 13, 2014 at 03:46:14PM +0530, [email protected] wrote:

From: Deepak S <[email protected]>

Workaround fixed in BYT. Forcing Gfx clk up not needed, and Requesting the min freq should bring bring the voltage Vnn.

Signed-off-by Deepak S <[email protected]>
---
  drivers/gpu/drm/i915/intel_pm.c | 40 +---------------------------------------
  1 file changed, 1 insertion(+), 39 deletions(-)
drivers/gpu/drm/i915/intel_pm.c +0 -38
index 0b088fe..9aee28b 100644
@@ -3198,44 +3198,6 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
  	trace_intel_gpu_freq_change(val * 50);
  }
-/* vlv_set_rps_idle: Set the frequency to Rpn if Gfx clocks are down
- *
- * * If Gfx is Idle, then
- * 1. Mask Turbo interrupts
- * 2. Bring up Gfx clock
- * 3. Change the freq to Rpn and wait till P-Unit updates freq
- * 4. Clear the Force GFX CLK ON bit so that Gfx can down
- * 5. Unmask Turbo interrupts
-*/
-static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
-{
-	/*
-	 * When we are idle.  Drop to min voltage state.
-	 */
-
-	if (dev_priv->rps.cur_freq <= dev_priv->rps.min_freq_softlimit)
-		return;
-
-	/* Mask turbo interrupt so that they will not come in between */
-	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
-
-	vlv_force_gfx_clock(dev_priv, true);
-
-	dev_priv->rps.cur_freq = dev_priv->rps.min_freq_softlimit;
-
-	vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ,
-					dev_priv->rps.min_freq_softlimit);
-
-	if (wait_for(((vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS))
-				& GENFREQSTATUS) == 0, 5))
-		DRM_ERROR("timed out waiting for Punit\n");
-
-	vlv_force_gfx_clock(dev_priv, false);
-
-	I915_WRITE(GEN6_PMINTRMSK,
-		   gen6_rps_pm_mask(dev_priv, dev_priv->rps.cur_freq));
-}
-
  void gen6_rps_idle(struct drm_i915_private *dev_priv)
  {
  	struct drm_device *dev = dev_priv->dev;

drivers/gpu/drm/i915/intel_pm.c void gen6_rps_idle(struct drm_i915_private *dev_priv) · line 3208

Quoted 5 lines
drivers/gpu/drm/i915/intel_pm.c +1 -1
@@ -3243,7 +3205,7 @@ void gen6_rps_idle(struct drm_i915_private *dev_priv)
  	mutex_lock(&dev_priv->rps.hw_lock);
  	if (dev_priv->rps.enabled) {
  		if (IS_VALLEYVIEW(dev))
-			vlv_set_rps_idle(dev_priv);
+			valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);

This should take care of https://bugs.freedesktop.org/show_bug.cgi?id=75244

I don't know when the hardware got fixed so I'm hesitant to r-b it, but at least my C0 works fine without this stuff, so:

Acked-by Ville Syrjälä <[email protected]>
Quoted 7 lines
Quoted 4 lines
Quoted 3 lines
Quoted 3 lines

However to avoid future mishaps I think we should have some kind of a comment before the valleyview_set_rps() call to let the reader know that we really need this on VLV to drop the voltage.

hmm, Yes we might need this for other stepping. I will add a comment

Please don't put the stepping info in the comment or commit message though, that freaks out people ;-) Usually we go with "pre-production" or "early revisions" or something non-specific.

Ok Sure :)

Quoted 26 lines
Quoted 9 lines

Thanks for the review

Quoted 7 lines
Quoted 5 lines

Also it now occurs to me that we might be leaving the GPU frequency (and thus Vnn) high during a system suspend. I think we need an explicit rps_idle() call in the suspend path somewhere. Runtime suspend should be fine already since it depends on intel_mark_idle() getting called before the last rpm reference is dropped.

Maybe this is all we need?

Quoted 12 lines

drivers/gpu/drm/i915/i915_gem.c i915_gem_suspend(struct drm_device *dev) · line 4533

Quoted 10 lines
drivers/gpu/drm/i915/i915_gem.c +1 -1
index 3768199..fabd852 100644
@@ -4530,7 +4530,7 @@ i915_gem_suspend(struct drm_device *dev)
         del_timer_sync(&dev_priv->gpu_error.hangcheck_timer);
         cancel_delayed_work_sync(&dev_priv->mm.retire_work);
-       cancel_delayed_work_sync(&dev_priv->mm.idle_work);
+       flush_delayed_work(&dev_priv->mm.idle_work);
         return 0;

Yes, while suspending we need move GPU to min_freq. flush_delayed_work should be fine. Let me create a patch for this.

Since this is gt powersave related can you please also check whether we shouldn't move this to the intel_suspend_gt_powersave function Jesse recently added to -nightly?

Thanks, Daniel

author
#6 lore raw

PATCH v2 1/1 drm/i915: Drop WA to fix Voltage not getting dropped to Vmin when Gfx is power gated for latest VLV revision

In reply to Deepak S

From: Deepak S <[email protected]>

Workaround fixed in Latest VLV revision. Forcing Gfx clk up not needed, and Requesting the min freq should bring bring the voltage Vnn.

v2: Drop WA for Latest VLV revision (Ville)

Signed-off-by Deepak S <[email protected]>
---
 drivers/gpu/drm/i915/intel_pm.c | 8 ++++++++
 1 file changed, 8 insertions(+)
drivers/gpu/drm/i915/intel_pm.c +8 -0
index a90fdbd..6b6cfd4 100644
@@ -3212,6 +3212,14 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
 */
 static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
 {
+	struct drm_device *dev = dev_priv->dev;
+
+	/* Latest VLV doesn't need Vnn WA*/
+	if (dev->pdev->revision >= 0xd) {
+		valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
+		return;
+	}
+
 	/*
 	 * When we are idle.  Drop to min voltage state.
 	 */
Signature · 1 line
1.9.1
maintainer
#7 lore raw

Re: [PATCH v2] drm/i915: Drop WA to fix Voltage not getting dropped to Vmin when Gfx is power gated for latest VLV revision

Quoted 6 lines · On Sat, 28 Jun 2014, [email protected] wrote:

From: Deepak S <[email protected]>

Workaround fixed in Latest VLV revision. Forcing Gfx clk up not needed, and Requesting the min freq should bring bring the voltage Vnn.

v2: Drop WA for Latest VLV revision (Ville)

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75244

Quoted 31 lines
Signed-off-by Deepak S <[email protected]>
---
 drivers/gpu/drm/i915/intel_pm.c | 8 ++++++++
 1 file changed, 8 insertions(+)
drivers/gpu/drm/i915/intel_pm.c +8 -0
index a90fdbd..6b6cfd4 100644
@@ -3212,6 +3212,14 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
 */
 static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
 {
+	struct drm_device *dev = dev_priv->dev;
+
+	/* Latest VLV doesn't need Vnn WA*/
+	if (dev->pdev->revision >= 0xd) {
+		valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
+		return;
+	}
+
 	/*
 	 * When we are idle.  Drop to min voltage state.
 	 */
Signature · 6 lines
1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 1 line
Jani Nikula, Intel Open Source Technology Center
maintainer
#8 lore raw

Re: [PATCH v2] drm/i915: Drop WA to fix Voltage not getting dropped to Vmin when Gfx is power gated for latest VLV revision

Quoted 9 lines · Jani Nikula
Quoted 6 lines · On Sat, 28 Jun 2014, [email protected] wrote:

From: Deepak S <[email protected]>

Workaround fixed in Latest VLV revision. Forcing Gfx clk up not needed, and Requesting the min freq should bring bring the voltage Vnn.

v2: Drop WA for Latest VLV revision (Ville)

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75244

Per comments in the bug, scratch that.

Deepak, I presume this patch is needed nonetheless?

BR,
Jani.

Quoted 39 lines
Quoted 32 lines
Signed-off-by Deepak S <[email protected]>
---
 drivers/gpu/drm/i915/intel_pm.c | 8 ++++++++
 1 file changed, 8 insertions(+)
drivers/gpu/drm/i915/intel_pm.c +8 -0
index a90fdbd..6b6cfd4 100644
@@ -3212,6 +3212,14 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
 */
 static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
 {
+	struct drm_device *dev = dev_priv->dev;
+
+	/* Latest VLV doesn't need Vnn WA*/
+	if (dev->pdev->revision >= 0xd) {
+		valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
+		return;
+	}
+
 	/*
 	 * When we are idle.  Drop to min voltage state.
 	 */
Signature · 6 lines
1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 5 lines
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 1 line
Jani Nikula, Intel Open Source Technology Center
Reviewed-by on v2 1/1
#9 lore raw

Re: [PATCH v2] drm/i915: Drop WA to fix Voltage not getting dropped to Vmin when Gfx is power gated for latest VLV revision

In reply to [email protected]
Quoted 17 lines · On Sat, Jun 28, 2014 at 11:26:11AM +0530, [email protected] wrote:

From: Deepak S <[email protected]>

Workaround fixed in Latest VLV revision. Forcing Gfx clk up not needed, and Requesting the min freq should bring bring the voltage Vnn.

v2: Drop WA for Latest VLV revision (Ville)

Signed-off-by Deepak S <[email protected]>
---
 drivers/gpu/drm/i915/intel_pm.c | 8 ++++++++
 1 file changed, 8 insertions(+)
drivers/gpu/drm/i915/intel_pm.c +0 -0
index a90fdbd..6b6cfd4 100644

drivers/gpu/drm/i915/intel_pm.c void gen6_set_rps(struct drm_device *dev, u8 val) · line 3217

Quoted 6 lines
drivers/gpu/drm/i915/intel_pm.c +3 -0
@@ -3212,6 +3212,14 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
 */
 static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
 {
+	struct drm_device *dev = dev_priv->dev;
+
+	/* Latest VLV doesn't need Vnn WA*/

Maybe this should say "Latest VLV doesn't need to force the gfx clock" or something like that. We are still doing this to reduce Vnn after all.

Apart from that this matches my observations so:

Reviewed-by Ville Syrjälä <[email protected]>
Quoted 10 lines
drivers/gpu/drm/i915/intel_pm.c +5 -0
+	if (dev->pdev->revision >= 0xd) {
+		valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
+		return;
+	}
+
 	/*
 	 * When we are idle.  Drop to min voltage state.
 	 */
Signature · 1 line
1.9.1
Signature · 2 lines
Ville Syrjälä
Intel OTC
maintainer Applied
#10 lore raw

Re: [PATCH v2] drm/i915: Drop WA to fix Voltage not getting dropped to Vmin when Gfx is power gated for latest VLV revision

Quoted 28 lines · Ville Syrjälä
Quoted 17 lines · On Sat, Jun 28, 2014 at 11:26:11AM +0530, [email protected] wrote:

From: Deepak S <[email protected]>

Workaround fixed in Latest VLV revision. Forcing Gfx clk up not needed, and Requesting the min freq should bring bring the voltage Vnn.

v2: Drop WA for Latest VLV revision (Ville)

Signed-off-by Deepak S <[email protected]>
---
 drivers/gpu/drm/i915/intel_pm.c | 8 ++++++++
 1 file changed, 8 insertions(+)
drivers/gpu/drm/i915/intel_pm.c +0 -0
index a90fdbd..6b6cfd4 100644

drivers/gpu/drm/i915/intel_pm.c void gen6_set_rps(struct drm_device *dev, u8 val) · line 3217

Quoted 6 lines
drivers/gpu/drm/i915/intel_pm.c +3 -0
@@ -3212,6 +3212,14 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
 */
 static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
 {
+	struct drm_device *dev = dev_priv->dev;
+
+	/* Latest VLV doesn't need Vnn WA*/

Maybe this should say "Latest VLV doesn't need to force the gfx clock" or something like that. We are still doing this to reduce Vnn after all.

Quoted 2 lines

Apart from that this matches my observations so:

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

Pushed to -fixes with the comment changed and commit message massaged a bit. Thanks for the patch and review.

BR,
Jani.

Quoted 19 lines
Quoted 11 lines
drivers/gpu/drm/i915/intel_pm.c +5 -0
+	if (dev->pdev->revision >= 0xd) {
+		valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
+		return;
+	}
+
 	/*
 	 * When we are idle.  Drop to min voltage state.
 	 */
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 · 5 lines
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx