From patchwork Tue Feb 24 16:12:20 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Imre Deak X-Patchwork-Id: 5873561 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 8C5C5BF440 for ; Tue, 24 Feb 2015 16:14:18 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id A2933201ED for ; Tue, 24 Feb 2015 16:14:17 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id A950F201B4 for ; Tue, 24 Feb 2015 16:14:16 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id CEA3389EB4; Tue, 24 Feb 2015 08:14:15 -0800 (PST) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTP id B32E889EB4; Tue, 24 Feb 2015 08:14:14 -0800 (PST) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga101.fm.intel.com with ESMTP; 24 Feb 2015 08:12:23 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.09,639,1418112000"; d="scan'208";a="682688413" Received: from ideak-desk.fi.intel.com ([10.237.72.74]) by fmsmga002.fm.intel.com with ESMTP; 24 Feb 2015 08:12:20 -0800 Message-ID: <1424794340.15554.3.camel@intel.com> Subject: Re: [PATCH] drm/i915: fix failure to power off after hibernate From: Imre Deak To: =?ISO-8859-1?Q?Bj=F8rn?= Mork Date: Tue, 24 Feb 2015 18:12:20 +0200 In-Reply-To: <1424789904-26699-1-git-send-email-bjorn@mork.no> References: <87bnkjcqjt.fsf@nemi.mork.no> <1424789904-26699-1-git-send-email-bjorn@mork.no> Organization: Intel X-Mailer: Evolution 3.12.7-0ubuntu1 Mime-Version: 1.0 Cc: intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, stable@vger.kernel.org, Daniel Vetter X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: imre.deak@intel.com Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, 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 On ti, 2015-02-24 at 15:58 +0100, Bjørn Mork wrote: > This fixes a bug where hibernation completes, but the system > fails to power off after the image has been saved. > > Bisection lead to commit da2bc1b9db33 ("drm/i915: add poweroff_late > handler") which added a .poweroff_late hook pointing to the same > function as the .freeze_late hook, without any justification or > explanation why this would be correct, except "for consistency". > > The assumption that this "makes the power off handling identical to > the S3 suspend and S4 freeze handling" is completely bogus. As clearly > documented in Documentation/power/devices.txt, the poweroff* hooks > are part of the hibernation API along with the freeze* hooks. The > phases involved in hibernation are: > > prepare, freeze, freeze_late, freeze_noirq, thaw_noirq, thaw_early, > thaw, complete, prepare, poweroff, poweroff_late, poweroff_noirq > > With the above sequence in mind, it should be fairly obvious that you > cannot save registers and disable the device in both the freeze_late > and poweroff_late phases. Or as Documentation/power/devices.txt > explains it: > > The poweroff, poweroff_late and poweroff_noirq callbacks should do essentially > the same things as the suspend, suspend_late and suspend_noirq callbacks, > respectively. The only notable difference is that they need not store the > device register values, because the registers should already have been stored > during the freeze, freeze_late or freeze_noirq phases. > > The "consistency" between suspend and hibernate was already in > place, with freeze_late pointing to the same function as suspend_late. > There is no need for any poweroff_late hook in this driver. The poweroff handlers undo the actions of the thaw handlers. As the original commit stated saving the registers is not needed there, but it's also not a big overhead and there should be no problem doing it. We are planning to optimize the hibernation sequence by for example not shutting down the display between freeze and thaw, and also getting rid of unnecessary steps at the power off phase. But before that we want to actually unify things rather than having special cases, as maintaining the special code paths caused already quite a lot of problems for us so far. Reverting the commit may hide some other issue, so before doing that could you try the following patch: http://lists.freedesktop.org/archives/intel-gfx/2015-February/060529.html If with that you still get the hang could you try on top of that the patch below, first having only pci_set_power_state uncommented, then both pci_set_power_state and pci_disable_device uncommented? Thanks, Imre diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 4badb23..dc91d4b 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -968,6 +968,23 @@ static int i915_pm_suspend_late(struct device *dev) return i915_drm_suspend_late(drm_dev); } +static int i915_pm_poweroff_late(struct device *dev) +{ + struct drm_device *drm_dev = dev_to_i915(dev)->dev; + struct drm_i915_private *dev_priv = drm_dev->dev_private; + int ret; + + ret = intel_suspend_complete(dev_priv); + + if (ret) + return ret; + + pci_disable_device(drm_dev->pdev); +// pci_set_power_state(drm_dev->pdev, PCI_D3hot); + + return 0; +} + static int i915_pm_resume_early(struct device *dev) { struct drm_device *drm_dev = dev_to_i915(dev)->dev; @@ -1535,7 +1552,7 @@ static const struct dev_pm_ops i915_pm_ops = { .thaw_early = i915_pm_resume_early, .thaw = i915_pm_resume, .poweroff = i915_pm_suspend, - .poweroff_late = i915_pm_suspend_late, + .poweroff_late = i915_pm_poweroff_late, .restore_early = i915_pm_resume_early, .restore = i915_pm_resume,