drm/i915: Bring GPU Freq to min while suspending.

PATCH Superseded [email protected] 3 participants 11 messages Jun 17, 2014 – Jul 15, 2014

Subsystems (2)

DRM drivers Intel i915

lore

Share & tools

lore

[email protected]

author

2014-06-17 18:30 UTC #0 lore raw

PATCH v2 1/1 drm/i915: Bring GPU Freq to min while suspending.

From: Deepak S <[email protected]>

We might be leaving the GPU Frequency (and thus vnn) high during the suspend. Flush the delayed work queue should take care of this.

v2: Fixed typo in commit message (Deepak)

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

---
 drivers/gpu/drm/i915/i915_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpu/drm/i915/i915_gem.c +1 -1

index 7f643db..8d5ae82 100644
@@ -4541,7 +4541,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 · 1 line

1.9.1

Daniel Vetter

2014-06-17 22:17 UTC #1 lore raw

Quoted 17 lines · On Wed, Jun 18, 2014 at 05:30:53AM +0530, [email protected] wrote:

From: Deepak S <[email protected]>
We might be leaving the GPU Frequency (and thus vnn) high during the suspend. Flush the delayed work queue should take care of this.
v2: Fixed typo in commit message (Deepak)
Signed-off-by Deepak S <[email protected]>
---
 drivers/gpu/drm/i915/i915_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/i915/i915_gem.c +0 -0
index 7f643db..8d5ae82 100644

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

Quoted 5 lines

drivers/gpu/drm/i915/i915_gem.c +1 -1

@@ -4541,7 +4541,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);

Shouldn't we do that in suspend_gt_powersave instead? Also if we cancel the retire work the idle work won't necessarily get armed and we might miss the window. Just forcing the gt to the lowest freq in suspend_gt_powersave should be more reliable.
-Daniel

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Deepak S

author

2014-06-18 03:28 UTC #2 lore raw

Quoted 19 lines · Daniel Vetter

Quoted 18 lines · On Wed, Jun 18, 2014 at 05:30:53AM +0530, [email protected] wrote:
From: Deepak S <[email protected]>
We might be leaving the GPU Frequency (and thus vnn) high during the suspend. Flush the delayed work queue should take care of this.
v2: Fixed typo in commit message (Deepak)
Signed-off-by Deepak S <[email protected]>
---
  drivers/gpu/drm/i915/i915_gem.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/i915/i915_gem.c +0 -0
index 7f643db..8d5ae82 100644
@@ -4541,7 +4541,7 @@ i915_gem_suspend(struct drm_device *dev)
 

Quoted 9 lines

drivers/gpu/drm/i915/i915_gem.c i915_gem_suspend(struct drm_device *dev) · line 4544
Quoted 4 lines
drivers/gpu/drm/i915/i915_gem.c +1 -1
  	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);
Shouldn't we do that in suspend_gt_powersave instead? Also if we cancel the retire work the idle work won't necessarily get armed and we might miss the window. Just forcing the gt to the lowest freq in suspend_gt_powersave should be more reliable.
-Daniel

Since we a calling suspend_gt_powersave after i915_gem_suspend, i added the flush in suspend.

Yes i agree forcing the gt freq us more reliable

Daniel Vetter

2014-06-18 10:48 UTC #3 lore raw

Quoted 30 lines · Deepak S

Quoted 13 lines · Daniel Vetter
Quoted 12 lines · On Wed, Jun 18, 2014 at 05:30:53AM +0530, [email protected] wrote:
From: Deepak S <[email protected]>
We might be leaving the GPU Frequency (and thus vnn) high during the suspend. Flush the delayed work queue should take care of this.
v2: Fixed typo in commit message (Deepak)
Signed-off-by Deepak S <[email protected]>
---
 drivers/gpu/drm/i915/i915_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 14 lines
drivers/gpu/drm/i915/i915_gem.c i915_gem_suspend(struct drm_device *dev) · line 4543
Quoted 9 lines
drivers/gpu/drm/i915/i915_gem.c +1 -1
index 7f643db..8d5ae82 100644
@@ -4541,7 +4541,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);
Shouldn't we do that in suspend_gt_powersave instead? Also if we cancel the retire work the idle work won't necessarily get armed and we might miss the window. Just forcing the gt to the lowest freq in suspend_gt_powersave should be more reliable.
-Daniel

Quoted 1 line

Since we a calling suspend_gt_powersave after i915_gem_suspend, i added the flush in suspend.

I'm confused: So you're doing the change I've suggested or it doesn't work? Please unconfuse me ;-)

Quoted 1 line

Yes i agree forcing the gt freq us more reliable

So new patch or not?
-Daniel

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[email protected]

author

2014-06-19 12:05 UTC #4 lore raw

PATCH v3 1/1 drm/i915: Force GPU Freq to lowest while suspending.

In reply to Daniel Vetter

From: Deepak S <[email protected]>

We might be leaving the GPU Frequency (and thus vnn) high during the suspend. Force gt to move to lowest freq while suspending.

v2: Fixed typo in commit message (Deepak)

v3: Force gt to lowest freq in suspend_gt_powersave (Daniel)

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

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

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

index 2043c4b..6bbb90b 100644
@@ -4881,6 +4881,9 @@ void intel_suspend_gt_powersave(struct drm_device *dev)
 	/* Interrupts should be disabled already to avoid re-arming. */
 	WARN_ON(dev->irq_enabled);
 
+	/* Force GPU to min freq during suspend */
+	gen6_rps_idle(dev_priv);
+
 	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
 
 	cancel_work_sync(&dev_priv->rps.work);

Signature · 1 line

1.9.1

Daniel Vetter

2014-06-19 12:34 UTC #5 lore raw

Re: [PATCH v3] drm/i915: Force GPU Freq to lowest while suspending.

drivers/gpu/drm/i915/intel_pm.c void intel_suspend_gt_powersave(struct drm_device *dev) · line 4886

Quoted 12 lines · On Fri, Jun 20, 2014 at 1:59 PM, <[email protected]> wrote:

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

index 2043c4b..6bbb90b 100644
@@ -4881,6 +4881,9 @@ void intel_suspend_gt_powersave(struct drm_device *dev)
        /* Interrupts should be disabled already to avoid re-arming. */
        WARN_ON(dev->irq_enabled);
 
+       /* Force GPU to min freq during suspend */
+       gen6_rps_idle(dev_priv);
+

Shouldn't this be _after_ we've cancelled the rps works? Otherwise the work item might sneak in and undo the idling between the idle and work cancelling.
-Daniel

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Deepak S

author

2014-06-19 14:30 UTC #6 lore raw

Re: [PATCH v3] drm/i915: Force GPU Freq to lowest while suspending.

Quoted 9 lines · Daniel Vetter

Quoted 8 lines · On Fri, Jun 20, 2014 at 1:59 PM, <[email protected]> wrote:

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

index 2043c4b..6bbb90b 100644
@@ -4881,6 +4881,9 @@ void intel_suspend_gt_powersave(struct drm_device *dev)
         /* Interrupts should be disabled already to avoid re-arming. */
         WARN_ON(dev->irq_enabled);

Quoted 7 lines

drivers/gpu/drm/i915/intel_pm.c void intel_suspend_gt_powersave(struct drm_device *dev) · line 4886
Quoted 3 lines
drivers/gpu/drm/i915/intel_pm.c +3 -0
+       /* Force GPU to min freq during suspend */
+       gen6_rps_idle(dev_priv);
+
Shouldn't this be _after_ we've cancelled the rps works? Otherwise the work item might sneak in and undo the idling between the idle and work cancelling.
-Daniel

My mistake sending updated patch, -Deepak

[email protected]

author

2014-06-19 14:38 UTC #7 lore raw

PATCH v4 1/1 drm/i915: Force GPU Freq to lowest while suspending.

In reply to Daniel Vetter

From: Deepak S <[email protected]>

We might be leaving the GPU Frequency (and thus vnn) high during the suspend. Force gt to move to lowest freq while suspending.

v2: Fixed typo in commit message (Deepak)

v3: Force gt to lowest freq in suspend_gt_powersave (Daniel)

v4: Add GPU min freq set _after_ we've cancelled the rps works (Daniel)

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

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

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

index 2043c4b..0543407 100644
@@ -4884,6 +4884,9 @@ void intel_suspend_gt_powersave(struct drm_device *dev)
 	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
 
 	cancel_work_sync(&dev_priv->rps.work);
+
+	/* Force GPU to min freq during suspend */
+	gen6_rps_idle(dev_priv);
 }
 
 void intel_disable_gt_powersave(struct drm_device *dev)

Signature · 1 line

1.9.1

Deepak S

author

2014-07-14 07:41 UTC #8 lore raw

Re: [PATCH v4] drm/i915: Force GPU Freq to lowest while suspending.

Quoted 21 lines · On Friday 20 June 2014 08:03 PM, [email protected] wrote:

From: Deepak S <[email protected]>
We might be leaving the GPU Frequency (and thus vnn) high during the suspend. Force gt to move to lowest freq while suspending.
v2: Fixed typo in commit message (Deepak)
v3: Force gt to lowest freq in suspend_gt_powersave (Daniel)
v4: Add GPU min freq set _after_ we've cancelled the rps works (Daniel)
Signed-off-by Deepak S <[email protected]>
---
  drivers/gpu/drm/i915/intel_pm.c | 3 +++
  1 file changed, 3 insertions(+)
drivers/gpu/drm/i915/intel_pm.c +0 -0
index 2043c4b..0543407 100644

drivers/gpu/drm/i915/intel_pm.c void intel_suspend_gt_powersave(struct drm_device *dev) · line 4892

Quoted 9 lines

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

@@ -4884,6 +4884,9 @@ void intel_suspend_gt_powersave(struct drm_device *dev)
  	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
 
  	cancel_work_sync(&dev_priv->rps.work);
+
+	/* Force GPU to min freq during suspend */
+	gen6_rps_idle(dev_priv);
  }
 
  void intel_disable_gt_powersave(struct drm_device *dev)

Hi Jesse,

Please review the patch

Thanks
Deepak

Jesse Barnes

Reviewed-by

2014-07-14 15:16 UTC #9 lore raw

Re: [PATCH v4] drm/i915: Force GPU Freq to lowest while suspending.

Quoted 31 lines · On Tue, 15 Jul 2014 13:05:48 +0530 Deepak S <[email protected]> wrote:

Quoted 29 lines · On Friday 20 June 2014 08:03 PM, [email protected] wrote:
From: Deepak S <[email protected]>
We might be leaving the GPU Frequency (and thus vnn) high during the suspend. Force gt to move to lowest freq while suspending.
v2: Fixed typo in commit message (Deepak)
v3: Force gt to lowest freq in suspend_gt_powersave (Daniel)
v4: Add GPU min freq set _after_ we've cancelled the rps works (Daniel)
Signed-off-by Deepak S <[email protected]>
---
  drivers/gpu/drm/i915/intel_pm.c | 3 +++
  1 file changed, 3 insertions(+)
drivers/gpu/drm/i915/intel_pm.c +3 -0
index 2043c4b..0543407 100644
@@ -4884,6 +4884,9 @@ void intel_suspend_gt_powersave(struct drm_device *dev)
  	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
 
  	cancel_work_sync(&dev_priv->rps.work);
+
+	/* Force GPU to min freq during suspend */
+	gen6_rps_idle(dev_priv);
  }
 

Quoted 5 lines

drivers/gpu/drm/i915/intel_pm.c void intel_suspend_gt_powersave(struct drm_device *dev) · line 4892
Quoted 1 line
drivers/gpu/drm/i915/intel_pm.c +0 -0
  void intel_disable_gt_powersave(struct drm_device *dev)
Hi Jesse,
Please review the patch

Reviewed-by Jesse Barnes <[email protected]>

Signature · 1 line

Jesse Barnes, Intel Open Source Technology Center

Deepak S

author

2014-07-15 03:27 UTC #10 lore raw

Re: [PATCH v4] drm/i915: Force GPU Freq to lowest while suspending.

Quoted 33 lines · Jesse Barnes

Quoted 30 lines · On Tue, 15 Jul 2014 13:05:48 +0530 Deepak S <[email protected]> wrote:
Quoted 29 lines · On Friday 20 June 2014 08:03 PM, [email protected] wrote:
From: Deepak S <[email protected]>
We might be leaving the GPU Frequency (and thus vnn) high during the suspend. Force gt to move to lowest freq while suspending.
v2: Fixed typo in commit message (Deepak)
v3: Force gt to lowest freq in suspend_gt_powersave (Daniel)
v4: Add GPU min freq set _after_ we've cancelled the rps works (Daniel)
Signed-off-by Deepak S <[email protected]>
---
   drivers/gpu/drm/i915/intel_pm.c | 3 +++
   1 file changed, 3 insertions(+)
drivers/gpu/drm/i915/intel_pm.c +3 -0
index 2043c4b..0543407 100644
@@ -4884,6 +4884,9 @@ void intel_suspend_gt_powersave(struct drm_device *dev)
   	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
 
   	cancel_work_sync(&dev_priv->rps.work);
+
+	/* Force GPU to min freq during suspend */
+	gen6_rps_idle(dev_priv);
   }
 

Quoted 6 lines

Quoted 4 lines
drivers/gpu/drm/i915/intel_pm.c void intel_suspend_gt_powersave(struct drm_device *dev) · line 4892
Quoted 1 line
drivers/gpu/drm/i915/intel_pm.c +0 -0
   void intel_disable_gt_powersave(struct drm_device *dev)
Hi Jesse,
Please review the patch
Reviewed-by Jesse Barnes <[email protected]>

Thanks for reviewing