[Nouveau] [PATCH v4 3/4] pci: set the pcie link speed to 8.0 when suspending
Ben Skeggs
skeggsb at gmail.com
Tue Sep 17 08:35:48 UTC 2019
On Tue, 17 Sep 2019 at 18:28, Karol Herbst <kherbst at redhat.com> wrote:
>
> On Tue, Sep 17, 2019 at 10:21 AM Ben Skeggs <skeggsb at gmail.com> wrote:
> >
> > On Tue, 17 Sep 2019 at 18:07, Karol Herbst <kherbst at redhat.com> wrote:
> > >
> > > On Tue, Sep 17, 2019 at 8:01 AM Ben Skeggs <skeggsb at gmail.com> wrote:
> > > >
> > > > On Fri, 13 Sep 2019 at 21:33, Karol Herbst <kherbst at redhat.com> wrote:
> > > > >
> > > > > Apperantly things go south if we suspend the device with a PCIe link speed
> > > > > set to 2.5. Fixes runtime suspend on my gp107.
> > > > >
> > > > > This all looks like some bug inside the pci subsystem and I would prefer a
> > > > > fix there instead of nouveau, but maybe there is no real nice way of doing
> > > > > that outside of drivers?
> > > > >
> > > > > v2: squashed together patch 4 and 5
> > > > > v3: only restore pcie speed on machines with runpm
> > > > > add NvRunpmWorkaround config option to disable the workaround
> > > > > v4: only run the code on suspend
> > > > > always put the card into 8.0 mode, not what nouveau detected on load
> > > > Why this change?
> > >
> > > modprobe nouveau
> > > rmmod nouveau
> > > modprobe nouveau (saved 2.5 as the "boot" speed)
> > > runpm breaks.
> > Given that this is more than likely a hack/workaround for another
> > issue to begin with, that isn't the end of the world.
> >
> > >
> > > Mainly I don't actually want to depend on the initial state as we
> > > lower to 2.5/5.0 depending on what we queried is possible and not
> > > using 2.5 is actually the fix, not use whatever the GPU booted with.
> > Is 8 available everywhere we're going to be using this?
> >
>
> nouveau fallsback to a lower speed if the requested one is not
> available. We have to do this detection for pstates anyway, where the
> vbios requests 8.0, but the system can only do 5.0 or 2.5
Ack. I guess I'm OK with this then. I wouldn't mind a comment in
fini() before the call, explaining why we're actually doing this so
the rationale doesn't get lost in history at some point.
>
> > >
> > > > Also, I know we don't currently touch it, but what
> > > > if x16 isn't available on some systems and we break stuff when we do
> > > > start playing with link width?
> > > >
> > >
> > > I think if we add code for the width, we would add a maximum width
> > > detection as well as we do for the link speed.
> > >
> > > > >
> > > > > Signed-off-by: Karol Herbst <kherbst at redhat.com>
> > > > > Reviewed-by: Lyude Paul <lyude at redhat.com> (v2)
> > > > > ---
> > > > > drm/nouveau/include/nvkm/core/device.h | 2 ++
> > > > > drm/nouveau/include/nvkm/subdev/pci.h | 3 ++-
> > > > > drm/nouveau/nouveau_drm.c | 1 +
> > > > > drm/nouveau/nvkm/subdev/clk/base.c | 2 +-
> > > > > drm/nouveau/nvkm/subdev/pci/base.c | 2 ++
> > > > > drm/nouveau/nvkm/subdev/pci/pcie.c | 27 ++++++++++++++++++++++----
> > > > > drm/nouveau/nvkm/subdev/pci/priv.h | 1 +
> > > > > 7 files changed, 32 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/drm/nouveau/include/nvkm/core/device.h b/drm/nouveau/include/nvkm/core/device.h
> > > > > index 6d55cd047..4fb3f972f 100644
> > > > > --- a/drm/nouveau/include/nvkm/core/device.h
> > > > > +++ b/drm/nouveau/include/nvkm/core/device.h
> > > > > @@ -125,6 +125,8 @@ struct nvkm_device {
> > > > > u8 chiprev;
> > > > > u32 crystal;
> > > > >
> > > > > + bool has_runpm;
> > > > > +
> > > > > struct {
> > > > > struct notifier_block nb;
> > > > > } acpi;
> > > > > diff --git a/drm/nouveau/include/nvkm/subdev/pci.h b/drm/nouveau/include/nvkm/subdev/pci.h
> > > > > index b29101e48..7245513d9 100644
> > > > > --- a/drm/nouveau/include/nvkm/subdev/pci.h
> > > > > +++ b/drm/nouveau/include/nvkm/subdev/pci.h
> > > > > @@ -52,6 +52,7 @@ int gk104_pci_new(struct nvkm_device *, int, struct nvkm_pci **);
> > > > > int gp100_pci_new(struct nvkm_device *, int, struct nvkm_pci **);
> > > > >
> > > > > /* pcie functions */
> > > > > -int nvkm_pcie_set_link(struct nvkm_pci *, enum nvkm_pcie_speed, u8 width);
> > > > > +int nvkm_pcie_set_link(struct nvkm_pci *, enum nvkm_pcie_speed, u8 width,
> > > > > + bool save);
> > > > > enum nvkm_pcie_speed nvkm_pcie_get_speed(struct nvkm_pci *);
> > > > > #endif
> > > > > diff --git a/drm/nouveau/nouveau_drm.c b/drm/nouveau/nouveau_drm.c
> > > > > index 3d32afe8a..78d55c525 100644
> > > > > --- a/drm/nouveau/nouveau_drm.c
> > > > > +++ b/drm/nouveau/nouveau_drm.c
> > > > > @@ -676,6 +676,7 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
> > > > >
> > > > > if (nouveau_atomic)
> > > > > driver_pci.driver_features |= DRIVER_ATOMIC;
> > > > > + device->has_runpm = nouveau_pmops_runtime();
> > > > >
> > > > > drm_dev = drm_dev_alloc(&driver_pci, &pdev->dev);
> > > > > if (IS_ERR(drm_dev)) {
> > > > > diff --git a/drm/nouveau/nvkm/subdev/clk/base.c b/drm/nouveau/nvkm/subdev/clk/base.c
> > > > > index ba6a868d4..e30e77453 100644
> > > > > --- a/drm/nouveau/nvkm/subdev/clk/base.c
> > > > > +++ b/drm/nouveau/nvkm/subdev/clk/base.c
> > > > > @@ -277,7 +277,7 @@ nvkm_pstate_prog(struct nvkm_clk *clk, int pstatei)
> > > > > nvkm_debug(subdev, "setting performance state %d\n", pstatei);
> > > > > clk->pstate = pstatei;
> > > > >
> > > > > - nvkm_pcie_set_link(pci, pstate->pcie_speed, pstate->pcie_width);
> > > > > + nvkm_pcie_set_link(pci, pstate->pcie_speed, pstate->pcie_width, true);
> > > > >
> > > > > if (fb && fb->ram && fb->ram->func->calc) {
> > > > > struct nvkm_ram *ram = fb->ram;
> > > > > diff --git a/drm/nouveau/nvkm/subdev/pci/base.c b/drm/nouveau/nvkm/subdev/pci/base.c
> > > > > index ee2431a78..c9b60ef76 100644
> > > > > --- a/drm/nouveau/nvkm/subdev/pci/base.c
> > > > > +++ b/drm/nouveau/nvkm/subdev/pci/base.c
> > > > > @@ -90,6 +90,8 @@ nvkm_pci_fini(struct nvkm_subdev *subdev, bool suspend)
> > > > >
> > > > > if (pci->agp.bridge)
> > > > > nvkm_agp_fini(pci);
> > > > > + else if (pci_is_pcie(pci->pdev))
> > > > > + nvkm_pcie_fini(pci, suspend);
> > > > >
> > > > > return 0;
> > > > > }
> > > > > diff --git a/drm/nouveau/nvkm/subdev/pci/pcie.c b/drm/nouveau/nvkm/subdev/pci/pcie.c
> > > > > index b4203ff1a..5cab4a240 100644
> > > > > --- a/drm/nouveau/nvkm/subdev/pci/pcie.c
> > > > > +++ b/drm/nouveau/nvkm/subdev/pci/pcie.c
> > > > > @@ -23,6 +23,8 @@
> > > > > */
> > > > > #include "priv.h"
> > > > >
> > > > > +#include <core/option.h>
> > > > > +
> > > > > static char *nvkm_pcie_speeds[] = {
> > > > > "2.5GT/s",
> > > > > "5.0GT/s",
> > > > > @@ -106,11 +108,25 @@ nvkm_pcie_init(struct nvkm_pci *pci)
> > > > > pci->func->pcie.init(pci);
> > > > >
> > > > > if (pci->pcie.speed != -1)
> > > > > - nvkm_pcie_set_link(pci, pci->pcie.speed, pci->pcie.width);
> > > > > + nvkm_pcie_set_link(pci, pci->pcie.speed,
> > > > > + pci->pcie.width, false);
> > > > >
> > > > > return 0;
> > > > > }
> > > > >
> > > > > +int
> > > > > +nvkm_pcie_fini(struct nvkm_pci *pci, bool suspend)
> > > > > +{
> > > > > + struct nvkm_device *device = pci->subdev.device;
> > > > > + if (!device->has_runpm || !suspend)
> > > > > + return 0;
> > > > > +
> > > > > + if (!nvkm_boolopt(device->cfgopt, "NvRunpmWorkaround", true))
> > > > > + return 0;
> > > > > +
> > > > > + return nvkm_pcie_set_link(pci, NVKM_PCIE_SPEED_8_0, 16, false);
> > > > > +}
> > > > > +
> > > > > void
> > > > > nvkm_pcie_force_aspm_off(struct nvkm_pci *pci, bool status)
> > > > > {
> > > > > @@ -120,7 +136,8 @@ nvkm_pcie_force_aspm_off(struct nvkm_pci *pci, bool status)
> > > > > }
> > > > >
> > > > > int
> > > > > -nvkm_pcie_set_link(struct nvkm_pci *pci, enum nvkm_pcie_speed speed, u8 width)
> > > > > +nvkm_pcie_set_link(struct nvkm_pci *pci, enum nvkm_pcie_speed speed, u8 width,
> > > > > + bool save)
> > > > > {
> > > > > struct nvkm_subdev *subdev = &pci->subdev;
> > > > > enum nvkm_pcie_speed cur_speed, max_speed;
> > > > > @@ -154,8 +171,10 @@ nvkm_pcie_set_link(struct nvkm_pci *pci, enum nvkm_pcie_speed speed, u8 width)
> > > > > speed = max_speed;
> > > > > }
> > > > >
> > > > > - pci->pcie.speed = speed;
> > > > > - pci->pcie.width = width;
> > > > > + if (save) {
> > > > > + pci->pcie.speed = speed;
> > > > > + pci->pcie.width = width;
> > > > > + }
> > > > >
> > > > > if (speed == cur_speed) {
> > > > > nvkm_debug(subdev, "requested matches current speed\n");
> > > > > diff --git a/drm/nouveau/nvkm/subdev/pci/priv.h b/drm/nouveau/nvkm/subdev/pci/priv.h
> > > > > index 82c78befa..6bea37c15 100644
> > > > > --- a/drm/nouveau/nvkm/subdev/pci/priv.h
> > > > > +++ b/drm/nouveau/nvkm/subdev/pci/priv.h
> > > > > @@ -63,4 +63,5 @@ int gk104_pcie_version_supported(struct nvkm_pci *);
> > > > >
> > > > > int nvkm_pcie_oneinit(struct nvkm_pci *);
> > > > > int nvkm_pcie_init(struct nvkm_pci *);
> > > > > +int nvkm_pcie_fini(struct nvkm_pci *, bool suspend);
> > > > > #endif
> > > > > --
> > > > > 2.21.0
> > > > >
> > > > > _______________________________________________
> > > > > Nouveau mailing list
> > > > > Nouveau at lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/nouveau
> > >
>
More information about the Nouveau
mailing list