Message-ID: <1424889214.5991.4.camel@intel.com>
Subject: Re: [PATCH] drm/i915: fix failure to power off after hibernate
From: Imre Deak <imre.deak@intel.com>
Reply-To: imre.deak@intel.com
To: =?ISO-8859-1?Q?Bj=F8rn?= Mork <bjorn@mork.no>
Cc: linux-kernel@vger.kernel.org, Daniel Vetter <daniel.vetter@intel.com>,
        Jani Nikula <jani.nikula@linux.intel.com>,
        intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
        stable@vger.kernel.org,
        Ville =?ISO-8859-1?Q?Syrj=E4l=E4?= 
	<ville.syrjala@linux.intel.com>
Date: Wed, 25 Feb 2015 20:33:34 +0200
In-Reply-To: <878ufnjfrr.fsf@nemi.mork.no>
References: <87bnkjcqjt.fsf@nemi.mork.no>
	 <1424789904-26699-1-git-send-email-bjorn@mork.no>
	 <1424794340.15554.3.camel@intel.com> <878ufnjfrr.fsf@nemi.mork.no>
Organization: Intel
Content-Type: multipart/mixed; boundary="=-59BYt5QPbUQzLj2OODt5"
Mime-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 7379
Lines: 146


--=-59BYt5QPbUQzLj2OODt5
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit

On ti, 2015-02-24 at 20:00 +0100, Bjørn Mork wrote:
> Imre Deak <imre.deak@intel.com> writes:
> 
> > 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.
> 
> That sounds like a worthy goal.  I don't understand what you hope to
> achieve by having a poweroff_late hook, since there aren't really
> anything useful left to do at the point it is called, but if you want a
> dummy callback there for code structure reasons then fine.

There are the following reasons for shutting down the device properly
during hibernation:
- ACPI mandates that the OSPM (the kernel in our case) puts all devices
into D3 that are not wake-up sources (i915 is not) (Kudos to Ville for
pointing this out)
- Embedded panels have a well defined shutdown sequence. We don't have
any good reason to not follow this, in fact for some panels the
subsequent reinitialization could be problematic in case of a hard
power-off. (Thanks to Jani for this info)

In fact the only reason why we didn't put the device into PCI D3 before
the patch you bisected, is kind of arbitrary. The PCI core puts every
device into D3 unless its driver saves the device's PCI state on its
own. Since before that patch we did save the state, but haven't put the
device into D3 it stayed in D0. That was definitely not intentional and
as such we depended on the BIOS to correct this for us afterwards.

> But you cannot just run around breaking stuff while slowly moving
> towards this goal over multiple releases... v3.19 is currently broken
> and that seems totally unnecessary.
> 
> In any case: You should have noticed this problem while testing your
> patches.  The breakage is 100% reproducible. Unfortunately I had to do a
> bisect to realize what you had done to the i915 driver, something I
> unfortunately didn't find time to do before v3.19 was released.  But I
> do find it unnecessary to release with such bugs.  Any attempt to
> exercise the code path you modified would have revealed the bug.

We tested these patches on numerous platforms and haven't seen the issue
you reported. Based on your feedback the current assumption is that this
is a bug in your BIOS, which tries to access the device despite it being
powered down. We're trying our best to test each change we commit on a
big set of platforms, but - especially on older hardware with random
BIOS versions/configurations - full coverage is not possible. So we
depend on reports like yours a lot to fill in the gaps.

> > 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
> 
> Makes no difference.  I assume that patch fixes an unrelated bug? The
> age and reported symptoms indicates so.  Note that I am reporting a
> regression introduced after v3.18, while that seems to fix a bug
> introduced in v3.17. Both v3.17 and v3.18 (including v3.18.6), as
> well as earlier releases, work fine for me.

Ok, thanks for trying.

> > 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?
> 
> That patch fixes the problem, with only pci_set_power_state commented
> out.  Do you still want me to try with pci_disable_device() commented
> out as well?

No, but it would help if you could still try the two attached patch
separately, without any of the previous workarounds. Based on the
result, we'll follow up with a fix that adds for your specific platform
either of the attached workarounds or simply avoids putting the device
into D3 (corresponding to the patch you already tried).

Thanks,
Imre

--=-59BYt5QPbUQzLj2OODt5
Content-Disposition: attachment;
	filename*0=0001-drm-i915-zero-PCI_COMMAND-at-the-end-of-hibernation.patc;
	filename*1=h
Content-Transfer-Encoding: base64
Content-Type: text/x-patch;
	name="0001-drm-i915-zero-PCI_COMMAND-at-the-end-of-hibernation.patch";
	charset="UTF-8"

RnJvbSBmZThiOTBjOTc2ZjE0ZWFiODBjYjY3MTVkMDQwNTE1NzE2M2QzMTZlIE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBJbXJlIERlYWsgPGltcmUuZGVha0BpbnRlbC5jb20+CkRhdGU6
IFdlZCwgMjUgRmViIDIwMTUgMTk6Mzg6NTMgKzAyMDAKU3ViamVjdDogW1BBVENIXSBkcm0vaTkx
NTogemVybyBQQ0lfQ09NTUFORCBhdCB0aGUgZW5kIG9mIGhpYmVybmF0aW9uCgpTaWduZWQtb2Zm
LWJ5OiBJbXJlIERlYWsgPGltcmUuZGVha0BpbnRlbC5jb20+Ci0tLQogZHJpdmVycy9ncHUvZHJt
L2k5MTUvaTkxNV9kcnYuYyB8IDIgKysKIDEgZmlsZSBjaGFuZ2VkLCAyIGluc2VydGlvbnMoKykK
CmRpZmYgLS1naXQgYS9kcml2ZXJzL2dwdS9kcm0vaTkxNS9pOTE1X2Rydi5jIGIvZHJpdmVycy9n
cHUvZHJtL2k5MTUvaTkxNV9kcnYuYwppbmRleCA0YmFkYjIzLi5iMjI2Y2M2IDEwMDY0NAotLS0g
YS9kcml2ZXJzL2dwdS9kcm0vaTkxNS9pOTE1X2Rydi5jCisrKyBiL2RyaXZlcnMvZ3B1L2RybS9p
OTE1L2k5MTVfZHJ2LmMKQEAgLTY1Myw2ICs2NTMsOCBAQCBzdGF0aWMgaW50IGk5MTVfZHJtX3N1
c3BlbmRfbGF0ZShzdHJ1Y3QgZHJtX2RldmljZSAqZHJtX2RldikKIAlwY2lfZGlzYWJsZV9kZXZp
Y2UoZHJtX2Rldi0+cGRldik7CiAJcGNpX3NldF9wb3dlcl9zdGF0ZShkcm1fZGV2LT5wZGV2LCBQ
Q0lfRDNob3QpOwogCisJcGNpX3dyaXRlX2NvbmZpZ193b3JkKGRybV9kZXYtPnBkZXYsIFBDSV9D
T01NQU5ELCAwKTsKKwogCXJldHVybiAwOwogfQogCi0tIAoyLjEuMAoK


--=-59BYt5QPbUQzLj2OODt5
Content-Disposition: attachment;
	filename*0=0002-drm-i915-use-D3cold-instead-of-D3hot-during-suspend-.pat;
	filename*1=ch
Content-Transfer-Encoding: base64
Content-Type: text/x-patch;
	name="0002-drm-i915-use-D3cold-instead-of-D3hot-during-suspend-.patch";
	charset="UTF-8"

RnJvbSAxODMxMDYyOWNiZDMyMzI3ZWRkOGJjMzA5NjllMjNhN2IyMGEzY2UzIE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBJbXJlIERlYWsgPGltcmUuZGVha0BpbnRlbC5jb20+CkRhdGU6
IFdlZCwgMjUgRmViIDIwMTUgMTk6NDM6MzMgKzAyMDAKU3ViamVjdDogW1BBVENIXSBkcm0vaTkx
NTogdXNlIEQzY29sZCBpbnN0ZWFkIG9mIEQzaG90IGR1cmluZwogc3VzcGVuZC9oaWJlcm5hdGUK
ClNpZ25lZC1vZmYtYnk6IEltcmUgRGVhayA8aW1yZS5kZWFrQGludGVsLmNvbT4KLS0tCiBkcml2
ZXJzL2dwdS9kcm0vaTkxNS9pOTE1X2Rydi5jIHwgMiArLQogMSBmaWxlIGNoYW5nZWQsIDEgaW5z
ZXJ0aW9uKCspLCAxIGRlbGV0aW9uKC0pCgpkaWZmIC0tZ2l0IGEvZHJpdmVycy9ncHUvZHJtL2k5
MTUvaTkxNV9kcnYuYyBiL2RyaXZlcnMvZ3B1L2RybS9pOTE1L2k5MTVfZHJ2LmMKaW5kZXggNGJh
ZGIyMy4uODA3NDQzYSAxMDA2NDQKLS0tIGEvZHJpdmVycy9ncHUvZHJtL2k5MTUvaTkxNV9kcnYu
YworKysgYi9kcml2ZXJzL2dwdS9kcm0vaTkxNS9pOTE1X2Rydi5jCkBAIC02NTEsNyArNjUxLDcg
QEAgc3RhdGljIGludCBpOTE1X2RybV9zdXNwZW5kX2xhdGUoc3RydWN0IGRybV9kZXZpY2UgKmRy
bV9kZXYpCiAJfQogCiAJcGNpX2Rpc2FibGVfZGV2aWNlKGRybV9kZXYtPnBkZXYpOwotCXBjaV9z
ZXRfcG93ZXJfc3RhdGUoZHJtX2Rldi0+cGRldiwgUENJX0QzaG90KTsKKwlwY2lfc2V0X3Bvd2Vy
X3N0YXRlKGRybV9kZXYtPnBkZXYsIFBDSV9EM2NvbGQpOwogCiAJcmV0dXJuIDA7CiB9Ci0tIAoy
LjEuMAoK


--=-59BYt5QPbUQzLj2OODt5--

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/