Mika Westerberg
2019-Nov-20 11:22 UTC
[Nouveau] [PATCH v4] pci: prevent putting nvidia GPUs into lower device states on certain intel bridges
On Wed, Nov 20, 2019 at 11:52:22AM +0100, Rafael J. Wysocki wrote:> On Wed, Nov 20, 2019 at 11:18 AM Mika Westerberg > <mika.westerberg at intel.com> wrote: > > > > Hi Karol, > > > > On Tue, Nov 19, 2019 at 11:26:45PM +0100, Karol Herbst wrote: > > > On Tue, Nov 19, 2019 at 10:50 PM Bjorn Helgaas <helgaas at kernel.org> wrote: > > > > > > > > [+cc Dave] > > > > > > > > On Thu, Oct 17, 2019 at 02:19:01PM +0200, Karol Herbst wrote: > > > > > Fixes state transitions of Nvidia Pascal GPUs from D3cold into higher device > > > > > states. > > > > > > > > > > v2: convert to pci_dev quirk > > > > > put a proper technical explanation of the issue as a in-code comment > > > > > v3: disable it only for certain combinations of intel and nvidia hardware > > > > > v4: simplify quirk by setting flag on the GPU itself > > > > > > > > I have zero confidence that we understand the real problem, but we do > > > > need to do something with this. I'll merge it for v5.5 if we get the > > > > minor procedural stuff below straightened out. > > > > > > > > > > Thanks, and I agree with your statement, but at this point I think > > > only Intel can help out digging deeper as I see no way to debug this > > > further. > > > > I don't have anything against this patch, as long as the quirk stays > > limited to the particular root port leading to the NVIDIA GPU. The > > reason why I think it should to be limited is that I'm pretty certain > > the problem is not in the root port itself. I have here a KBL based > > Thinkpad X1 Carbon 6th gen that can put the TBT controller into D3cold > > (it is connected to PCH root port) and it wakes up there just fine, so > > don't want to break that. > > > > Now, PCIe devices cannot go into D3cold all by themselves. They always > > need help from the platform side which is ACPI in this case. This is > > done by having the device to have _PR3 method that returns one or more > > power resources that the OS is supposed to turn off when the device is > > put into D3cold. All of that is implemented as form of ACPI methods that > > pretty much do the hardware specific things that are outside of PCIe > > spec to get the device into D3cold. At high level the _OFF() method > > causes the root port to broadcast PME_Turn_Off message that results the > > link to enter L2/3 ready, it then asserts PERST, configures WAKE (both > > can be GPIOs) and finally removes power (if the link goes into L3, > > otherwise it goes into L2). > > > > I think this is where the problem actually lies - the ASL methods that > > are used to put the device into D3cold and back. We know that in Windows > > this all works fine so unless Windows quirks the root port the same way > > there is another reason behind this. > > > > In case of Dell XPS 9560 (IIRC that's the machine you have) the > > corresponding power resource is called \_SB.PCI0.PEG0.PG00 and its > > _ON/_OFF methods end up calling PGON()/PGOF() accordingly. The methods > > itself do lots of things and it is hard to follow the dissassembled > > ASL which does not have any comments but there are couple of things that > > stand out where we may go into a different path. One of them is this in > > the PGOF() method: > > > > If (((OSYS <= 0x07D9) || ((OSYS == 0x07DF) && (_REV == 0x05)))) > > > > The ((OSYS == 0x07DF) && (_REV == 0x05)) checks specifically for Linux > > (see [1] and 18d78b64fddc ("ACPI / init: Make it possible to override > > _REV")) so it might be that Dell people tested this at some point in > > Linux as well. Added Mario in case he has any ideas. > > > > Previously I suggested you to try the ACPI method tracing to see what > > happens inside PGOF(). Did you have time to try it? It may provide more > > information about that is happening inside those methods and hopefully > > point us to the root cause. > > > > Also if you haven't tried already passing acpi_rev_override in the > > command line makes the _REV to return 5 so it should go into the "Linux" > > path in PGOF(). > > Oh, so does it look like we are trying to work around AML that tried > to work around some problematic behavior in Linux at one point?Yes, it looks like so if I read the ASL right. The whole method looks like below (the full acpidump was shared by Karol in v3 thread) and there is similar check in the _ON (PGON) method: Method (PGOF, 1, Serialized) { PIOF = Arg0 If ((PIOF == Zero)) { If ((SGGP == Zero)) { Return (Zero) } } ElseIf ((PIOF == One)) { If ((P1GP == Zero)) { Return (Zero) } } ElseIf ((PIOF == 0x02)) { If ((P2GP == Zero)) { Return (Zero) } } PEBA = \XBAS /* External reference */ PDEV = GDEV (PIOF) PFUN = GFUN (PIOF) Name (SCLK, Package (0x03) { One, 0x80, Zero }) If ((CCHK (PIOF, Zero) == Zero)) { Return (Zero) } \_SB.PCI0.PEG0.PEGP.LTRE = \_SB.PCI0.PEG0.LREN If ((Arg0 == Zero)) { ELC0 = LCT0 /* \_SB_.PCI0.LCT0 */ H0VI = S0VI /* \_SB_.PCI0.S0VI */ H0DI = S0DI /* \_SB_.PCI0.S0DI */ ECP0 = LCP0 /* \_SB_.PCI0.LCP0 */ } ElseIf ((Arg0 == One)) { ELC1 = LCT1 /* \_SB_.PCI0.LCT1 */ H1VI = S1VI /* \_SB_.PCI0.S1VI */ H1DI = S1DI /* \_SB_.PCI0.S1DI */ ECP1 = LCP1 /* \_SB_.PCI0.LCP1 */ } ElseIf ((Arg0 == 0x02)) { ELC2 = LCT2 /* \_SB_.PCI0.LCT2 */ H2VI = S2VI /* \_SB_.PCI0.S2VI */ H2DI = S2DI /* \_SB_.PCI0.S2DI */ ECP2 = LCP2 /* \_SB_.PCI0.LCP2 */ } If (((OSYS <= 0x07D9) || ((OSYS == 0x07DF) && (_REV == 0x05)))) { If ((PIOF == Zero)) { P0LD = One TCNT = Zero While ((TCNT < LDLY)) { If ((P0LT == 0x08)) { Break } Sleep (0x10) TCNT += 0x10 } P0RM = One P0AP = 0x03 } ElseIf ((PIOF == One)) { P1LD = One TCNT = Zero While ((TCNT < LDLY)) { If ((P1LT == 0x08)) { Break } Sleep (0x10) TCNT += 0x10 } P1RM = One P1AP = 0x03 } ElseIf ((PIOF == 0x02)) { P2LD = One TCNT = Zero While ((TCNT < LDLY)) { If ((P2LT == 0x08)) { Break } Sleep (0x10) TCNT += 0x10 } P2RM = One P2AP = 0x03 } If ((PBGE != Zero)) { If (SBDL (PIOF)) { MBDL = GMXB (PIOF) PDUB (PIOF, MBDL) } } } Else { LKDS (PIOF) } If ((DerefOf (SCLK [Zero]) != Zero)) { PCRO (0xDC, 0x100C, DerefOf (SCLK [One])) Sleep (0x10) } GPPR (PIOF, Zero) If ((OSYS != 0x07D9)) { DIWK (PIOF) } \_SB.SGOV (0x01010004, Zero) Sleep (0x14) Return (Zero) }
Rafael J. Wysocki
2019-Nov-20 11:48 UTC
[Nouveau] [PATCH v4] pci: prevent putting nvidia GPUs into lower device states on certain intel bridges
On Wed, Nov 20, 2019 at 12:22 PM Mika Westerberg <mika.westerberg at intel.com> wrote:> > On Wed, Nov 20, 2019 at 11:52:22AM +0100, Rafael J. Wysocki wrote: > > On Wed, Nov 20, 2019 at 11:18 AM Mika Westerberg > > <mika.westerberg at intel.com> wrote: > > > > > > Hi Karol, > > > > > > On Tue, Nov 19, 2019 at 11:26:45PM +0100, Karol Herbst wrote: > > > > On Tue, Nov 19, 2019 at 10:50 PM Bjorn Helgaas <helgaas at kernel.org> wrote: > > > > > > > > > > [+cc Dave] > > > > > > > > > > On Thu, Oct 17, 2019 at 02:19:01PM +0200, Karol Herbst wrote: > > > > > > Fixes state transitions of Nvidia Pascal GPUs from D3cold into higher device > > > > > > states. > > > > > > > > > > > > v2: convert to pci_dev quirk > > > > > > put a proper technical explanation of the issue as a in-code comment > > > > > > v3: disable it only for certain combinations of intel and nvidia hardware > > > > > > v4: simplify quirk by setting flag on the GPU itself > > > > > > > > > > I have zero confidence that we understand the real problem, but we do > > > > > need to do something with this. I'll merge it for v5.5 if we get the > > > > > minor procedural stuff below straightened out. > > > > > > > > > > > > > Thanks, and I agree with your statement, but at this point I think > > > > only Intel can help out digging deeper as I see no way to debug this > > > > further. > > > > > > I don't have anything against this patch, as long as the quirk stays > > > limited to the particular root port leading to the NVIDIA GPU. The > > > reason why I think it should to be limited is that I'm pretty certain > > > the problem is not in the root port itself. I have here a KBL based > > > Thinkpad X1 Carbon 6th gen that can put the TBT controller into D3cold > > > (it is connected to PCH root port) and it wakes up there just fine, so > > > don't want to break that. > > > > > > Now, PCIe devices cannot go into D3cold all by themselves. They always > > > need help from the platform side which is ACPI in this case. This is > > > done by having the device to have _PR3 method that returns one or more > > > power resources that the OS is supposed to turn off when the device is > > > put into D3cold. All of that is implemented as form of ACPI methods that > > > pretty much do the hardware specific things that are outside of PCIe > > > spec to get the device into D3cold. At high level the _OFF() method > > > causes the root port to broadcast PME_Turn_Off message that results the > > > link to enter L2/3 ready, it then asserts PERST, configures WAKE (both > > > can be GPIOs) and finally removes power (if the link goes into L3, > > > otherwise it goes into L2). > > > > > > I think this is where the problem actually lies - the ASL methods that > > > are used to put the device into D3cold and back. We know that in Windows > > > this all works fine so unless Windows quirks the root port the same way > > > there is another reason behind this. > > > > > > In case of Dell XPS 9560 (IIRC that's the machine you have) the > > > corresponding power resource is called \_SB.PCI0.PEG0.PG00 and its > > > _ON/_OFF methods end up calling PGON()/PGOF() accordingly. The methods > > > itself do lots of things and it is hard to follow the dissassembled > > > ASL which does not have any comments but there are couple of things that > > > stand out where we may go into a different path. One of them is this in > > > the PGOF() method: > > > > > > If (((OSYS <= 0x07D9) || ((OSYS == 0x07DF) && (_REV == 0x05)))) > > > > > > The ((OSYS == 0x07DF) && (_REV == 0x05)) checks specifically for Linux > > > (see [1] and 18d78b64fddc ("ACPI / init: Make it possible to override > > > _REV")) so it might be that Dell people tested this at some point in > > > Linux as well. Added Mario in case he has any ideas. > > > > > > Previously I suggested you to try the ACPI method tracing to see what > > > happens inside PGOF(). Did you have time to try it? It may provide more > > > information about that is happening inside those methods and hopefully > > > point us to the root cause. > > > > > > Also if you haven't tried already passing acpi_rev_override in the > > > command line makes the _REV to return 5 so it should go into the "Linux" > > > path in PGOF(). > > > > Oh, so does it look like we are trying to work around AML that tried > > to work around some problematic behavior in Linux at one point? > > Yes, it looks like so if I read the ASL right.OK, so that would call for a DMI-based quirk as the real cause for the issue seems to be the AML in question, which means a firmware problem.
Karol Herbst
2019-Nov-20 11:51 UTC
[Nouveau] [PATCH v4] pci: prevent putting nvidia GPUs into lower device states on certain intel bridges
On Wed, Nov 20, 2019 at 12:48 PM Rafael J. Wysocki <rafael at kernel.org> wrote:> > On Wed, Nov 20, 2019 at 12:22 PM Mika Westerberg > <mika.westerberg at intel.com> wrote: > > > > On Wed, Nov 20, 2019 at 11:52:22AM +0100, Rafael J. Wysocki wrote: > > > On Wed, Nov 20, 2019 at 11:18 AM Mika Westerberg > > > <mika.westerberg at intel.com> wrote: > > > > > > > > Hi Karol, > > > > > > > > On Tue, Nov 19, 2019 at 11:26:45PM +0100, Karol Herbst wrote: > > > > > On Tue, Nov 19, 2019 at 10:50 PM Bjorn Helgaas <helgaas at kernel.org> wrote: > > > > > > > > > > > > [+cc Dave] > > > > > > > > > > > > On Thu, Oct 17, 2019 at 02:19:01PM +0200, Karol Herbst wrote: > > > > > > > Fixes state transitions of Nvidia Pascal GPUs from D3cold into higher device > > > > > > > states. > > > > > > > > > > > > > > v2: convert to pci_dev quirk > > > > > > > put a proper technical explanation of the issue as a in-code comment > > > > > > > v3: disable it only for certain combinations of intel and nvidia hardware > > > > > > > v4: simplify quirk by setting flag on the GPU itself > > > > > > > > > > > > I have zero confidence that we understand the real problem, but we do > > > > > > need to do something with this. I'll merge it for v5.5 if we get the > > > > > > minor procedural stuff below straightened out. > > > > > > > > > > > > > > > > Thanks, and I agree with your statement, but at this point I think > > > > > only Intel can help out digging deeper as I see no way to debug this > > > > > further. > > > > > > > > I don't have anything against this patch, as long as the quirk stays > > > > limited to the particular root port leading to the NVIDIA GPU. The > > > > reason why I think it should to be limited is that I'm pretty certain > > > > the problem is not in the root port itself. I have here a KBL based > > > > Thinkpad X1 Carbon 6th gen that can put the TBT controller into D3cold > > > > (it is connected to PCH root port) and it wakes up there just fine, so > > > > don't want to break that. > > > > > > > > Now, PCIe devices cannot go into D3cold all by themselves. They always > > > > need help from the platform side which is ACPI in this case. This is > > > > done by having the device to have _PR3 method that returns one or more > > > > power resources that the OS is supposed to turn off when the device is > > > > put into D3cold. All of that is implemented as form of ACPI methods that > > > > pretty much do the hardware specific things that are outside of PCIe > > > > spec to get the device into D3cold. At high level the _OFF() method > > > > causes the root port to broadcast PME_Turn_Off message that results the > > > > link to enter L2/3 ready, it then asserts PERST, configures WAKE (both > > > > can be GPIOs) and finally removes power (if the link goes into L3, > > > > otherwise it goes into L2). > > > > > > > > I think this is where the problem actually lies - the ASL methods that > > > > are used to put the device into D3cold and back. We know that in Windows > > > > this all works fine so unless Windows quirks the root port the same way > > > > there is another reason behind this. > > > > > > > > In case of Dell XPS 9560 (IIRC that's the machine you have) the > > > > corresponding power resource is called \_SB.PCI0.PEG0.PG00 and its > > > > _ON/_OFF methods end up calling PGON()/PGOF() accordingly. The methods > > > > itself do lots of things and it is hard to follow the dissassembled > > > > ASL which does not have any comments but there are couple of things that > > > > stand out where we may go into a different path. One of them is this in > > > > the PGOF() method: > > > > > > > > If (((OSYS <= 0x07D9) || ((OSYS == 0x07DF) && (_REV == 0x05)))) > > > > > > > > The ((OSYS == 0x07DF) && (_REV == 0x05)) checks specifically for Linux > > > > (see [1] and 18d78b64fddc ("ACPI / init: Make it possible to override > > > > _REV")) so it might be that Dell people tested this at some point in > > > > Linux as well. Added Mario in case he has any ideas. > > > > > > > > Previously I suggested you to try the ACPI method tracing to see what > > > > happens inside PGOF(). Did you have time to try it? It may provide more > > > > information about that is happening inside those methods and hopefully > > > > point us to the root cause. > > > > > > > > Also if you haven't tried already passing acpi_rev_override in the > > > > command line makes the _REV to return 5 so it should go into the "Linux" > > > > path in PGOF(). > > > > > > Oh, so does it look like we are trying to work around AML that tried > > > to work around some problematic behavior in Linux at one point? > > > > Yes, it looks like so if I read the ASL right. > > OK, so that would call for a DMI-based quirk as the real cause for the > issue seems to be the AML in question, which means a firmware problem. >And I disagree as this is a linux specific workaround and windows goes that path and succeeds. This firmware based workaround was added, because it broke on Linux.
Mika Westerberg
2019-Nov-20 11:51 UTC
[Nouveau] [PATCH v4] pci: prevent putting nvidia GPUs into lower device states on certain intel bridges
On Wed, Nov 20, 2019 at 01:22:16PM +0200, Mika Westerberg wrote:> If (((OSYS <= 0x07D9) || ((OSYS == 0x07DF) && (_REV == > 0x05)))) > {The OSYS comes from this (in DSDT): If (_OSI ("Windows 2009")) { OSYS = 0x07D9 } If (_OSI ("Windows 2012")) { OSYS = 0x07DC } If (_OSI ("Windows 2013")) { OSYS = 0x07DD } If (_OSI ("Windows 2015")) { OSYS = 0x07DF } So I guess this particular check tries to identify Windows 7 and older, and Linux.> If ((PIOF == Zero)) > { > P0LD = One > TCNT = Zero > While ((TCNT < LDLY)) > { > If ((P0LT == 0x08)) > { > Break > } > > Sleep (0x10) > TCNT += 0x10 > } > > P0RM = One > P0AP = 0x03 > } > ElseIf ((PIOF == One)) > { > P1LD = One > TCNT = Zero > While ((TCNT < LDLY)) > { > If ((P1LT == 0x08)) > { > Break > } > > Sleep (0x10) > TCNT += 0x10 > } > > P1RM = One > P1AP = 0x03 > } > ElseIf ((PIOF == 0x02)) > { > P2LD = One > TCNT = Zero > While ((TCNT < LDLY)) > { > If ((P2LT == 0x08)) > { > Break > } > > Sleep (0x10) > TCNT += 0x10 > } > > P2RM = One > P2AP = 0x03 > } > > If ((PBGE != Zero)) > { > If (SBDL (PIOF)) > { > MBDL = GMXB (PIOF) > PDUB (PIOF, MBDL) > } > } > } > Else > { > LKDS (PIOF) > } > > If ((DerefOf (SCLK [Zero]) != Zero)) > { > PCRO (0xDC, 0x100C, DerefOf (SCLK [One])) > Sleep (0x10) > } > > GPPR (PIOF, Zero) > If ((OSYS != 0x07D9)) > { > DIWK (PIOF) > } > > \_SB.SGOV (0x01010004, Zero) > Sleep (0x14) > Return (Zero) > }
Karol Herbst
2019-Nov-20 11:54 UTC
[Nouveau] [PATCH v4] pci: prevent putting nvidia GPUs into lower device states on certain intel bridges
for newer Windows the firmware uses bit 0x80 on 0x248 (Q0L2 being the field name) on the bridge controller to turn of the device, on other versions it uses the "older"? 0xb0 register and the P0LD field, which is documented, where the former is not. On Wed, Nov 20, 2019 at 12:51 PM Mika Westerberg <mika.westerberg at intel.com> wrote:> > On Wed, Nov 20, 2019 at 01:22:16PM +0200, Mika Westerberg wrote: > > If (((OSYS <= 0x07D9) || ((OSYS == 0x07DF) && (_REV => > 0x05)))) > > { > > The OSYS comes from this (in DSDT): > > If (_OSI ("Windows 2009")) > { > OSYS = 0x07D9 > } > > If (_OSI ("Windows 2012")) > { > OSYS = 0x07DC > } > > If (_OSI ("Windows 2013")) > { > OSYS = 0x07DD > } > > If (_OSI ("Windows 2015")) > { > OSYS = 0x07DF > } > > So I guess this particular check tries to identify Windows 7 and older, > and Linux. > > > If ((PIOF == Zero)) > > { > > P0LD = One > > TCNT = Zero > > While ((TCNT < LDLY)) > > { > > If ((P0LT == 0x08)) > > { > > Break > > } > > > > Sleep (0x10) > > TCNT += 0x10 > > } > > > > P0RM = One > > P0AP = 0x03 > > } > > ElseIf ((PIOF == One)) > > { > > P1LD = One > > TCNT = Zero > > While ((TCNT < LDLY)) > > { > > If ((P1LT == 0x08)) > > { > > Break > > } > > > > Sleep (0x10) > > TCNT += 0x10 > > } > > > > P1RM = One > > P1AP = 0x03 > > } > > ElseIf ((PIOF == 0x02)) > > { > > P2LD = One > > TCNT = Zero > > While ((TCNT < LDLY)) > > { > > If ((P2LT == 0x08)) > > { > > Break > > } > > > > Sleep (0x10) > > TCNT += 0x10 > > } > > > > P2RM = One > > P2AP = 0x03 > > } > > > > If ((PBGE != Zero)) > > { > > If (SBDL (PIOF)) > > { > > MBDL = GMXB (PIOF) > > PDUB (PIOF, MBDL) > > } > > } > > } > > Else > > { > > LKDS (PIOF) > > } > > > > If ((DerefOf (SCLK [Zero]) != Zero)) > > { > > PCRO (0xDC, 0x100C, DerefOf (SCLK [One])) > > Sleep (0x10) > > } > > > > GPPR (PIOF, Zero) > > If ((OSYS != 0x07D9)) > > { > > DIWK (PIOF) > > } > > > > \_SB.SGOV (0x01010004, Zero) > > Sleep (0x14) > > Return (Zero) > > } >
Reasonably Related Threads
- [PATCH v4] pci: prevent putting nvidia GPUs into lower device states on certain intel bridges
- [PATCH v4] pci: prevent putting nvidia GPUs into lower device states on certain intel bridges
- [PATCH v4] pci: prevent putting nvidia GPUs into lower device states on certain intel bridges
- [PATCH v4] pci: prevent putting nvidia GPUs into lower device states on certain intel bridges
- [PATCH v4] pci: prevent putting nvidia GPUs into lower device states on certain intel bridges