[v4,3/4] pci: set the pcie link speed to 8.0 when suspending

Submitted by Karol Herbst on Sept. 13, 2019, 11:33 a.m.

Details

Message ID 20190913113306.20972-4-kherbst@redhat.com
State New
Headers show
Series "add PCIe workaround to fix runpm on laptops" ( rev: 1 ) in Nouveau

Not browsing as part of any series.

Commit Message

Karol Herbst Sept. 13, 2019, 11:33 a.m.
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

Signed-off-by: Karol Herbst <kherbst@redhat.com>
Reviewed-by: Lyude Paul <lyude@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(-)

Patch hide | download patch | download mbox

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

Comments

On Fri, 13 Sep 2019 at 21:33, Karol Herbst <kherbst@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?  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?

>
> Signed-off-by: Karol Herbst <kherbst@redhat.com>
> Reviewed-by: Lyude Paul <lyude@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau
On Tue, Sep 17, 2019 at 8:01 AM Ben Skeggs <skeggsb@gmail.com> wrote:
>
> On Fri, 13 Sep 2019 at 21:33, Karol Herbst <kherbst@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.

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.

> 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@redhat.com>
> > Reviewed-by: Lyude Paul <lyude@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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/nouveau
On Tue, 17 Sep 2019 at 18:07, Karol Herbst <kherbst@redhat.com> wrote:
>
> On Tue, Sep 17, 2019 at 8:01 AM Ben Skeggs <skeggsb@gmail.com> wrote:
> >
> > On Fri, 13 Sep 2019 at 21:33, Karol Herbst <kherbst@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?

>
> > 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@redhat.com>
> > > Reviewed-by: Lyude Paul <lyude@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@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/nouveau
>
On Tue, Sep 17, 2019 at 10:21 AM Ben Skeggs <skeggsb@gmail.com> wrote:
>
> On Tue, 17 Sep 2019 at 18:07, Karol Herbst <kherbst@redhat.com> wrote:
> >
> > On Tue, Sep 17, 2019 at 8:01 AM Ben Skeggs <skeggsb@gmail.com> wrote:
> > >
> > > On Fri, 13 Sep 2019 at 21:33, Karol Herbst <kherbst@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

> >
> > > 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@redhat.com>
> > > > Reviewed-by: Lyude Paul <lyude@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@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/nouveau
> >
On Tue, 17 Sep 2019 at 18:28, Karol Herbst <kherbst@redhat.com> wrote:
>
> On Tue, Sep 17, 2019 at 10:21 AM Ben Skeggs <skeggsb@gmail.com> wrote:
> >
> > On Tue, 17 Sep 2019 at 18:07, Karol Herbst <kherbst@redhat.com> wrote:
> > >
> > > On Tue, Sep 17, 2019 at 8:01 AM Ben Skeggs <skeggsb@gmail.com> wrote:
> > > >
> > > > On Fri, 13 Sep 2019 at 21:33, Karol Herbst <kherbst@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@redhat.com>
> > > > > Reviewed-by: Lyude Paul <lyude@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@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/nouveau
> > >
>
On Fri, 2019-09-13 at 13:33 +0200, Karol Herbst 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
> 
> Signed-off-by: Karol Herbst <kherbst@redhat.com>
> Reviewed-by: Lyude Paul <lyude@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;
> +

This could be a bitfield, up to you. Other then that:

Reviewed-by: Lyude Paul <lyude@redhat.com>

>  	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