From patchwork Fri Mar 7 23:08:05 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paulo Zanoni X-Patchwork-Id: 3795161 Return-Path: X-Original-To: patchwork-intel-gfx@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 201EEBF549 for ; Fri, 7 Mar 2014 23:08:49 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id D678F202EA for ; Fri, 7 Mar 2014 23:08:47 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id 69AD6202BE for ; Fri, 7 Mar 2014 23:08:46 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 4F42AFB616; Fri, 7 Mar 2014 15:08:44 -0800 (PST) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mail-yk0-f171.google.com (mail-yk0-f171.google.com [209.85.160.171]) by gabe.freedesktop.org (Postfix) with ESMTP id E9877FB614 for ; Fri, 7 Mar 2014 15:08:38 -0800 (PST) Received: by mail-yk0-f171.google.com with SMTP id q9so12535696ykb.2 for ; Fri, 07 Mar 2014 15:08:38 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=NzhNx2JaKhGAB3emduQOD8KVAqY1MUYnoV18sb4TfHw=; b=0zHbj79bu7fnQ2RCxFeCxm3ZGWfo9wzM74vK8eYNdr71hXLeo6FDbQbTmlhGzbe5wI +3wmiIGohr66eZ9oi6eRy9N9bHeJR8FSklSzSfEV9ZiQmK/1bK94Jxib1ilR0430PBIi xn83mEi0hMCFAtXwmM6LLAhNvU06ReBUHZQaPMulMwnSLrR6IyOA4B7CvdBFfLmrl2bt 9HmJs56ObG5ZeZWvQyZRZWnQJfkOzAqGtNQcdFDjOuem1XJWUTV6j6Dr/rkHDI2zVaxt f27cK2pnKqYY9E5oLw1jXaAOM/FJDX79WrvcWGFVOQk6NjVVD16RNgJyO69SyWBtRKO7 bYXA== X-Received: by 10.236.119.141 with SMTP id n13mr399387yhh.136.1394233718585; Fri, 07 Mar 2014 15:08:38 -0800 (PST) Received: from localhost.localdomain ([177.132.70.215]) by mx.google.com with ESMTPSA id d32sm32572865yhq.27.2014.03.07.15.08.37 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 07 Mar 2014 15:08:38 -0800 (PST) From: Paulo Zanoni To: intel-gfx@lists.freedesktop.org Date: Fri, 7 Mar 2014 20:08:05 -0300 Message-Id: <1394233699-3741-3-git-send-email-przanoni@gmail.com> X-Mailer: git-send-email 1.8.5.3 In-Reply-To: <1394233699-3741-1-git-send-email-przanoni@gmail.com> References: <1394233699-3741-1-git-send-email-przanoni@gmail.com> Cc: Paulo Zanoni Subject: [Intel-gfx] [PATCH 02/16] drm/i915: make PC8 be part of runtime PM suspend/resume X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org X-Spam-Status: No, score=-4.1 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_MED, T_DKIM_INVALID, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP From: Paulo Zanoni 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. v5: - Rebase, again. - Add a huge comment explaining the different forcewake usage (Chris, Daniel). - Use open-coded forcewake functions (Daniel). Signed-off-by: Paulo Zanoni Reviewed-by: Imre Deak --- 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 | 65 ++++++++++++++++-------------------- drivers/gpu/drm/i915/intel_drv.h | 3 +- drivers/gpu/drm/i915/intel_pm.c | 1 - 7 files changed, 43 insertions(+), 53 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index e4d2b9f..6aa23af 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1822,8 +1822,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 658fe24..6178c95 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -848,6 +848,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); @@ -862,6 +865,7 @@ static int i915_runtime_suspend(struct device *device) */ intel_opregion_notify_adapter(dev, PCI_D1); + DRM_DEBUG_KMS("Device suspended\n"); return 0; } @@ -878,6 +882,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 2a319ba..a5f1780 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1335,6 +1335,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 @@ -1388,7 +1392,6 @@ struct i915_package_c8 { bool enabled; int disable_count; struct mutex lock; - struct delayed_work enable_work; struct { uint32_t deimr; @@ -2092,8 +2095,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; int enable_cmd_parser; /* leave bools at the end to not create holes */ diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index a66ffb6..d1d7980 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, @@ -135,14 +133,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 9c3e2c5..ab02848 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6726,6 +6726,7 @@ static void hsw_disable_lcpll(struct drm_i915_private *dev_priv, static void hsw_restore_lcpll(struct drm_i915_private *dev_priv) { uint32_t val; + unsigned long irqflags; val = I915_READ(LCPLL_CTL); @@ -6733,9 +6734,22 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv) LCPLL_POWER_DOWN_ALLOW)) == LCPLL_PLL_LOCK) return; - /* 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); + /* + * Make sure we're not on PC8 state before disabling PC8, otherwise + * we'll hang the machine. To prevent PC8 state, just enable force_wake. + * + * The other problem is that hsw_restore_lcpll() is called as part of + * the runtime PM resume sequence, so we can't just call + * gen6_gt_force_wake_get() because that function calls + * intel_runtime_pm_get(), and we can't change the runtime PM refcount + * while we are on the resume sequence. So to solve this problem we have + * to call special forcewake code that doesn't touch runtime PM and + * doesn't enable the forcewake delayed work. + */ + spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); + if (dev_priv->uncore.forcewake_count++ == 0) + dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL); + spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); if (val & LCPLL_POWER_DOWN_ALLOW) { val &= ~LCPLL_POWER_DOWN_ALLOW; @@ -6769,14 +6783,20 @@ 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); + /* See the big comment above. */ + spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); + if (--dev_priv->uncore.forcewake_count == 0) + dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL); + spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); } -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; @@ -6792,22 +6812,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)); @@ -6818,15 +6822,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); @@ -6849,8 +6854,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); @@ -6859,14 +6862,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) @@ -6924,9 +6920,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 805d207..3f7ed0f 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -722,7 +722,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 ad58ce3..c5c36dc 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -6161,7 +6161,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); }