diff mbox

[09/23] drm/i915: make PC8 be part of runtime PM suspend/resume

Message ID 1393540010-1582-10-git-send-email-przanoni@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Paulo Zanoni Feb. 27, 2014, 10:26 p.m. UTC 
From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Currently, when our driver becomes idle for i915.pc8_timeout (default:
5s) we enable PC8, so we save some power, but not everything we can.
Then, while PC8 is enabled, if we stay idle for more
autosuspend_delay_ms (default: 10s) we'll enter runtime PM and put the
graphics device in D3 state, saving even more power. The two features
are separate things with increasing levels of power savings, but if we
disable PC8 we'll never get into D3.

While from the modularity point of view it would be nice to keep these
features as separate, we have reasons to merge them:
 - We are not aware of anybody wanting a "PC8 without D3" environment.
 - If we keep both features as separate, we'll have to to test both
   PC8 and PC8+D3 code paths. We're already having a major pain to
   make QA do automated testing of just one thing, testing both paths
   will cost even more.
 - Only Haswell+ supports PC8, so if we want to add runtime PM support
   to, for example, IVB, we'll have to copy some code from the PC8
   feature to runtime PM, so merging both features as a single thing
   will make it easier for enabling runtime PM on other platforms.

This patch only does the very basic steps required to have PC8 and
runtime PM merged on a single feature: the next patches will take care
of cleaning up everything.

v2: - Rebase.
v3: - Rebase.
    - Fully remove the deprecated i915 params since Daniel doesn't
      consider them as part of the ABI.
v4: - Rebase.
    - Fix typo in the commit message.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c      |  2 --
 drivers/gpu/drm/i915/i915_drv.c      |  8 +++++++
 drivers/gpu/drm/i915/i915_drv.h      |  7 +++---
 drivers/gpu/drm/i915/i915_params.c   | 10 ---------
 drivers/gpu/drm/i915/intel_display.c | 43 ++++++++----------------------------
 drivers/gpu/drm/i915/intel_drv.h     |  3 ++-
 drivers/gpu/drm/i915/intel_pm.c      |  1 -
 7 files changed, 23 insertions(+), 51 deletions(-)

Comments

Chris Wilson Feb. 28, 2014, 9:42 a.m. UTC | #1 
On Thu, Feb 27, 2014 at 07:26:36PM -0300, Paulo Zanoni wrote:
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ce582eb..788b165 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6614,7 +6614,7 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
>  
>  	/* Make sure we're not on PC8 state before disabling PC8, otherwise
>  	 * we'll hang the machine! */
> -	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
> +	gen6_gt_force_wake_get_no_rpm(dev_priv, FORCEWAKE_ALL);

I want more indications that this is legal - i.e. that the function is
only called from a special context handling rpm. As it is, it is not
immediately clear that the rearrangements in disable_package_c8 is
complete. If you moved this forcwake dance higher and comment why the
intel_runtime_pm_get() handling is so special, you might not scare so
many causal readers.
-Chris
Daniel Vetter March 6, 2014, 10:48 p.m. UTC | #2 
On Fri, Feb 28, 2014 at 09:42:02AM +0000, Chris Wilson wrote:
> On Thu, Feb 27, 2014 at 07:26:36PM -0300, Paulo Zanoni wrote:
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index ce582eb..788b165 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6614,7 +6614,7 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
> >  
> >  	/* Make sure we're not on PC8 state before disabling PC8, otherwise
> >  	 * we'll hang the machine! */
> > -	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
> > +	gen6_gt_force_wake_get_no_rpm(dev_priv, FORCEWAKE_ALL);
> 
> I want more indications that this is legal - i.e. that the function is
> only called from a special context handling rpm. As it is, it is not
> immediately clear that the rearrangements in disable_package_c8 is
> complete. If you moved this forcwake dance higher and comment why the
> intel_runtime_pm_get() handling is so special, you might not scare so
> many causal readers.

I agree with Chris, this is tricky and should be obvious and stand out.

What about open-coding the _no_rpm function here (like we have an
open-coded fw get/put around register acccess) and putting a big comment
above this explaining what exactly is going on, why we need it and why
this is safe?

For this case here being explicit and verbose is imo more important than
the little common code extraction the _no_rpm function allows us to do.
Especially since we have open-coded forcewake dances at other places
already, too.
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 7688abc..da0b2ee 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1818,8 +1818,6 @@  int i915_driver_unload(struct drm_device *dev)
 	cancel_work_sync(&dev_priv->gpu_error.work);
 	i915_destroy_error_state(dev);
 
-	cancel_delayed_work_sync(&dev_priv->pc8.enable_work);
-
 	if (dev->pdev->msi_enabled)
 		pci_disable_msi(dev->pdev);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 2d05d7c..983ab56 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -850,6 +850,9 @@  static int i915_runtime_suspend(struct device *device)
 
 	DRM_DEBUG_KMS("Suspending device\n");
 
+	if (HAS_PC8(dev))
+		__hsw_do_enable_pc8(dev_priv);
+
 	i915_gem_release_all_mmaps(dev_priv);
 
 	del_timer_sync(&dev_priv->gpu_error.hangcheck_timer);
@@ -864,6 +867,7 @@  static int i915_runtime_suspend(struct device *device)
 	 */
 	intel_opregion_notify_adapter(dev, PCI_D1);
 
+	DRM_DEBUG_KMS("Device suspended\n");
 	return 0;
 }
 
@@ -880,6 +884,10 @@  static int i915_runtime_resume(struct device *device)
 	intel_opregion_notify_adapter(dev, PCI_D0);
 	dev_priv->pm.suspended = false;
 
+	if (HAS_PC8(dev))
+		__hsw_do_disable_pc8(dev_priv);
+
+	DRM_DEBUG_KMS("Device resumed\n");
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index dcaa130..f53e77d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1299,6 +1299,10 @@  struct ilk_wm_values {
 /*
  * This struct tracks the state needed for the Package C8+ feature.
  *
+ * TODO: we're merging the Package C8+ feature with the runtime PM support. To
+ * avoid having to update the documentation at each patch of the series, we'll
+ * do a final update at the end.
+ *
  * Package states C8 and deeper are really deep PC states that can only be
  * reached when all the devices on the system allow it, so even if the graphics
  * device allows PC8+, it doesn't mean the system will actually get to these
@@ -1352,7 +1356,6 @@  struct i915_package_c8 {
 	bool enabled;
 	int disable_count;
 	struct mutex lock;
-	struct delayed_work enable_work;
 
 	struct {
 		uint32_t deimr;
@@ -1968,8 +1971,6 @@  struct i915_params {
 	unsigned int preliminary_hw_support;
 	int disable_power_well;
 	int enable_ips;
-	int enable_pc8;
-	int pc8_timeout;
 	int invert_brightness;
 	/* leave bools at the end to not create holes */
 	bool enable_hangcheck;
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 3b48258..5bf38f9 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -42,8 +42,6 @@  struct i915_params i915 __read_mostly = {
 	.disable_power_well = 1,
 	.enable_ips = 1,
 	.fastboot = 0,
-	.enable_pc8 = 1,
-	.pc8_timeout = 5000,
 	.prefault_disable = 0,
 	.reset = true,
 	.invert_brightness = 0,
@@ -134,14 +132,6 @@  module_param_named(fastboot, i915.fastboot, bool, 0600);
 MODULE_PARM_DESC(fastboot,
 	"Try to skip unnecessary mode sets at boot time (default: false)");
 
-module_param_named(enable_pc8, i915.enable_pc8, int, 0600);
-MODULE_PARM_DESC(enable_pc8,
-	"Enable support for low power package C states (PC8+) (default: true)");
-
-module_param_named(pc8_timeout, i915.pc8_timeout, int, 0600);
-MODULE_PARM_DESC(pc8_timeout,
-	"Number of msecs of idleness required to enter PC8+ (default: 5000)");
-
 module_param_named(prefault_disable, i915.prefault_disable, bool, 0600);
 MODULE_PARM_DESC(prefault_disable,
 	"Disable page prefaulting for pread/pwrite/reloc (default:false). "
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ce582eb..788b165 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6614,7 +6614,7 @@  static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
 
 	/* Make sure we're not on PC8 state before disabling PC8, otherwise
 	 * we'll hang the machine! */
-	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
+	gen6_gt_force_wake_get_no_rpm(dev_priv, FORCEWAKE_ALL);
 
 	if (val & LCPLL_POWER_DOWN_ALLOW) {
 		val &= ~LCPLL_POWER_DOWN_ALLOW;
@@ -6648,14 +6648,16 @@  static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
 			DRM_ERROR("Switching back to LCPLL failed\n");
 	}
 
-	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
+	gen6_gt_force_wake_put_no_rpm(dev_priv, FORCEWAKE_ALL);
 }
 
-static void __hsw_do_enable_pc8(struct drm_i915_private *dev_priv)
+void __hsw_do_enable_pc8(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
 	uint32_t val;
 
+	WARN_ON(!HAS_PC8(dev));
+
 	DRM_DEBUG_KMS("Enabling package C8+\n");
 
 	dev_priv->pc8.enabled = true;
@@ -6671,22 +6673,6 @@  static void __hsw_do_enable_pc8(struct drm_i915_private *dev_priv)
 	hsw_disable_lcpll(dev_priv, true, true);
 }
 
-void hsw_enable_pc8_work(struct work_struct *__work)
-{
-	struct drm_i915_private *dev_priv =
-		container_of(to_delayed_work(__work), struct drm_i915_private,
-			     pc8.enable_work);
-	struct drm_device *dev = dev_priv->dev;
-
-	WARN_ON(!HAS_PC8(dev));
-
-	if (dev_priv->pc8.enabled)
-		return;
-
-	__hsw_do_enable_pc8(dev_priv);
-	intel_runtime_pm_put(dev_priv);
-}
-
 static void __hsw_enable_package_c8(struct drm_i915_private *dev_priv)
 {
 	WARN_ON(!mutex_is_locked(&dev_priv->pc8.lock));
@@ -6697,15 +6683,16 @@  static void __hsw_enable_package_c8(struct drm_i915_private *dev_priv)
 	if (dev_priv->pc8.disable_count != 0)
 		return;
 
-	schedule_delayed_work(&dev_priv->pc8.enable_work,
-			      msecs_to_jiffies(i915.pc8_timeout));
+	intel_runtime_pm_put(dev_priv);
 }
 
-static void __hsw_do_disable_package_c8(struct drm_i915_private *dev_priv)
+void __hsw_do_disable_pc8(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
 	uint32_t val;
 
+	WARN_ON(!HAS_PC8(dev));
+
 	DRM_DEBUG_KMS("Disabling package C8+\n");
 
 	hsw_restore_lcpll(dev_priv);
@@ -6728,8 +6715,6 @@  static void __hsw_do_disable_package_c8(struct drm_i915_private *dev_priv)
 
 static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv)
 {
-	struct drm_device *dev = dev_priv->dev;
-
 	WARN_ON(!mutex_is_locked(&dev_priv->pc8.lock));
 	WARN(dev_priv->pc8.disable_count < 0,
 	     "pc8.disable_count: %d\n", dev_priv->pc8.disable_count);
@@ -6738,14 +6723,7 @@  static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv)
 	if (dev_priv->pc8.disable_count != 1)
 		return;
 
-	WARN_ON(!HAS_PC8(dev));
-
-	cancel_delayed_work_sync(&dev_priv->pc8.enable_work);
-	if (!dev_priv->pc8.enabled)
-		return;
-
 	intel_runtime_pm_get(dev_priv);
-	__hsw_do_disable_package_c8(dev_priv);
 }
 
 void hsw_enable_package_c8(struct drm_i915_private *dev_priv)
@@ -6803,9 +6781,6 @@  static void hsw_update_package_c8(struct drm_device *dev)
 	if (!HAS_PC8(dev_priv->dev))
 		return;
 
-	if (!i915.enable_pc8)
-		return;
-
 	mutex_lock(&dev_priv->pc8.lock);
 
 	allow = hsw_can_enable_package_c8(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a4ffc02..ea24eae 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -720,7 +720,8 @@  unsigned long intel_gen4_compute_page_offset(int *x, int *y,
 					     unsigned int bpp,
 					     unsigned int pitch);
 void intel_display_handle_reset(struct drm_device *dev);
-void hsw_enable_pc8_work(struct work_struct *__work);
+void __hsw_do_enable_pc8(struct drm_i915_private *dev_priv);
+void __hsw_do_disable_pc8(struct drm_i915_private *dev_priv);
 void hsw_enable_package_c8(struct drm_i915_private *dev_priv);
 void hsw_disable_package_c8(struct drm_i915_private *dev_priv);
 void intel_dp_get_m_n(struct intel_crtc *crtc,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 50b80bb..d68fee2 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5789,7 +5789,6 @@  void intel_pm_setup(struct drm_device *dev)
 	dev_priv->pc8.irqs_disabled = false;
 	dev_priv->pc8.enabled = false;
 	dev_priv->pc8.disable_count = 1; /* requirements_met */
-	INIT_DELAYED_WORK(&dev_priv->pc8.enable_work, hsw_enable_pc8_work);
 	INIT_DELAYED_WORK(&dev_priv->rps.delayed_resume_work,
 			  intel_gen6_powersave_work);
 }