Nouveau - Nov 2019 - [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.

On Wed, Nov 20, 2019 at 12:51 PM Karol Herbst <kherbst at redhat.com>
wrote:
>
> 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:
> > > > >
[cut]
> > > >
> > > > 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.
Apparently so at the time it was added, but would it still break after
the kernel changes made since then?

Moreover, has it not become harmful now?  IOW, wouldn't it work after
removing the "Linux workaround" from the AML?

The only way to verify that I can see would be to run the system with
custom ACPI tables without the "Linux workaround" in the AML in
question.

On Wed, Nov 20, 2019 at 1:06 PM Rafael J. Wysocki <rafael at kernel.org>
wrote:
>
> On Wed, Nov 20, 2019 at 12:51 PM Karol Herbst <kherbst at redhat.com>
wrote:
> >
> > 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:
> > > > > >
>
> [cut]
>
> > > > >
> > > > > 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.
>
> Apparently so at the time it was added, but would it still break after
> the kernel changes made since then?
>
> Moreover, has it not become harmful now?  IOW, wouldn't it work after
> removing the "Linux workaround" from the AML?
>
> The only way to verify that I can see would be to run the system with
> custom ACPI tables without the "Linux workaround" in the AML in
> question.
>
the workaround is not enabled by default, because it has to be
explicitly enabled by the user.

On Wed, Nov 20, 2019 at 1:06 PM Rafael J. Wysocki <rafael at kernel.org>
wrote:
>
> On Wed, Nov 20, 2019 at 12:51 PM Karol Herbst <kherbst at redhat.com>
wrote:
> >
> > 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:
> > > > > >
>
> [cut]
>
> > > > >
> > > > > 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.
>
> Apparently so at the time it was added, but would it still break after
> the kernel changes made since then?
>
> Moreover, has it not become harmful now?  IOW, wouldn't it work after
> removing the "Linux workaround" from the AML?
>
> The only way to verify that I can see would be to run the system with
> custom ACPI tables without the "Linux workaround" in the AML in
> question.
Or running it with acpi_rev_override as suggested by Mika, which
effectively would be the same thing.