[PATCH v4 0/4] add PCIe workaround to fix runpm on laptops

[PATCH v4 0/4] add PCIe workaround to fix runpm on laptops

[Karol Herbst]

not much changed since the last time I sent those patches out, but there
are a couple of annoying bug fixes, which users would probably never hit
unless they do rmmod/modprobe nouveau cycles.

Biggest change is that I force the link to a 8.0 speed rather than the
speed the GPU came up with.

Also this series depends on the PCIe improvement patches I sent out
recently.

Karol Herbst (4):
  pci: enable pcie link changes for pascal
  pci: add nvkm_pcie_get_speed
  pci: set the pcie link speed to 8.0 when suspending
  drm: abort runtime suspend if we hit an error

 drm/nouveau/include/nvkm/core/device.h |  2 ++
 drm/nouveau/include/nvkm/subdev/pci.h  |  4 ++-
 drm/nouveau/nouveau_drm.c              |  6 +++++
 drm/nouveau/nvkm/subdev/clk/base.c     |  2 +-
 drm/nouveau/nvkm/subdev/pci/base.c     |  2 ++
 drm/nouveau/nvkm/subdev/pci/gk104.c    |  8 +++---
 drm/nouveau/nvkm/subdev/pci/gp100.c    | 11 ++++++++
 drm/nouveau/nvkm/subdev/pci/pcie.c     | 35 +++++++++++++++++++++++---
 drm/nouveau/nvkm/subdev/pci/priv.h     |  6 +++++
 9 files changed, 66 insertions(+), 10 deletions(-)

-- 
2.21.0

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[PATCH v4 1/4] pci: enable pcie link changes for pascal

[Karol Herbst]

v2: add force disable ASPM func pointer

Signed-off-by: Karol Herbst <kherbst@redhat.com>
Reviewed-by: Lyude Paul <lyude@redhat.com> (v1)
---
 drm/nouveau/nvkm/subdev/pci/gk104.c |  8 ++++----
 drm/nouveau/nvkm/subdev/pci/gp100.c | 11 +++++++++++
 drm/nouveau/nvkm/subdev/pci/priv.h  |  5 +++++
 3 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/drm/nouveau/nvkm/subdev/pci/gk104.c b/drm/nouveau/nvkm/subdev/pci/gk104.c
index 804ef017f..61c4e76e8 100644
--- a/drm/nouveau/nvkm/subdev/pci/gk104.c
+++ b/drm/nouveau/nvkm/subdev/pci/gk104.c
@@ -25,7 +25,7 @@
 
 #include <subdev/timer.h>
 
-static int
+int
 gk104_pcie_version_supported(struct nvkm_pci *pci)
 {
 	return (nvkm_rd32(pci->subdev.device, 0x8c1c0) & 0x4) == 0x4 ? 2 : 1;
@@ -110,7 +110,7 @@ gk104_pcie_lnkctl_speed(struct nvkm_pci *pci)
 	return -1;
 }
 
-static enum nvkm_pcie_speed
+enum nvkm_pcie_speed
 gk104_pcie_max_speed(struct nvkm_pci *pci)
 {
 	u32 max_speed = nvkm_rd32(pci->subdev.device, 0x8c1c0) & 0x300000;
@@ -156,7 +156,7 @@ gk104_pcie_set_link_speed(struct nvkm_pci *pci, enum nvkm_pcie_speed speed)
 	return 0;
 }
 
-static int
+int
 gk104_pcie_init(struct nvkm_pci * pci)
 {
 	enum nvkm_pcie_speed lnkctl_speed, max_speed, cap_speed;
@@ -188,7 +188,7 @@ gk104_pcie_init(struct nvkm_pci * pci)
 	return 0;
 }
 
-static int
+int
 gk104_pcie_set_link(struct nvkm_pci *pci, enum nvkm_pcie_speed speed, u8 width)
 {
 	struct nvkm_subdev *subdev = &pci->subdev;
diff --git a/drm/nouveau/nvkm/subdev/pci/gp100.c b/drm/nouveau/nvkm/subdev/pci/gp100.c
index 82c5234a0..163233f16 100644
--- a/drm/nouveau/nvkm/subdev/pci/gp100.c
+++ b/drm/nouveau/nvkm/subdev/pci/gp100.c
@@ -35,6 +35,17 @@ gp100_pci_func = {
 	.wr08 = nv40_pci_wr08,
 	.wr32 = nv40_pci_wr32,
 	.msi_rearm = gp100_pci_msi_rearm,
+
+	.pcie.init = gk104_pcie_init,
+	.pcie.set_link = gk104_pcie_set_link,
+
+	.pcie.max_speed = gk104_pcie_max_speed,
+	.pcie.cur_speed = g84_pcie_cur_speed,
+
+	.pcie.set_version = gf100_pcie_set_version,
+	.pcie.version = gf100_pcie_version,
+	.pcie.version_supported = gk104_pcie_version_supported,
+	.pcie.force_aspm_off = g84_pcie_force_aspm_off,
 };
 
 int
diff --git a/drm/nouveau/nvkm/subdev/pci/priv.h b/drm/nouveau/nvkm/subdev/pci/priv.h
index c6a9ef330..82c78befa 100644
--- a/drm/nouveau/nvkm/subdev/pci/priv.h
+++ b/drm/nouveau/nvkm/subdev/pci/priv.h
@@ -56,6 +56,11 @@ int gf100_pcie_cap_speed(struct nvkm_pci *);
 int gf100_pcie_init(struct nvkm_pci *);
 int gf100_pcie_set_link(struct nvkm_pci *, enum nvkm_pcie_speed, u8);
 
+int gk104_pcie_init(struct nvkm_pci *);
+int gk104_pcie_set_link(struct nvkm_pci *, enum nvkm_pcie_speed, u8 width);
+enum nvkm_pcie_speed gk104_pcie_max_speed(struct nvkm_pci *);
+int gk104_pcie_version_supported(struct nvkm_pci *);
+
 int nvkm_pcie_oneinit(struct nvkm_pci *);
 int nvkm_pcie_init(struct nvkm_pci *);
 #endif
-- 
2.21.0

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[PATCH v4 2/4] pci: add nvkm_pcie_get_speed

[Karol Herbst]

v2: fixed compilation error

Signed-off-by: Karol Herbst <kherbst@redhat.com>
Reviewed-by: Lyude Paul <lyude@redhat.com>
---
 drm/nouveau/include/nvkm/subdev/pci.h | 1 +
 drm/nouveau/nvkm/subdev/pci/pcie.c    | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/drm/nouveau/include/nvkm/subdev/pci.h b/drm/nouveau/include/nvkm/subdev/pci.h
index 4803a4fad..b29101e48 100644
--- a/drm/nouveau/include/nvkm/subdev/pci.h
+++ b/drm/nouveau/include/nvkm/subdev/pci.h
@@ -53,4 +53,5 @@ 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);
+enum nvkm_pcie_speed nvkm_pcie_get_speed(struct nvkm_pci *);
 #endif
diff --git a/drm/nouveau/nvkm/subdev/pci/pcie.c b/drm/nouveau/nvkm/subdev/pci/pcie.c
index 354ac4c85..b4203ff1a 100644
--- a/drm/nouveau/nvkm/subdev/pci/pcie.c
+++ b/drm/nouveau/nvkm/subdev/pci/pcie.c
@@ -177,3 +177,11 @@ nvkm_pcie_set_link(struct nvkm_pci *pci, enum nvkm_pcie_speed speed, u8 width)
 
 	return ret;
 }
+
+enum nvkm_pcie_speed
+nvkm_pcie_get_speed(struct nvkm_pci *pci)
+{
+	if (!pci || !pci_is_pcie(pci->pdev) || !pci->func->pcie.cur_speed)
+		return -ENODEV;
+	return pci->func->pcie.cur_speed(pci);
+}
-- 
2.21.0

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

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

[Karol Herbst]

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;
+
 	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

[PATCH v4 4/4] drm: abort runtime suspend if we hit an error

[Karol Herbst]

Signed-off-by: Karol Herbst <kherbst@redhat.com>
---
 drm/nouveau/nouveau_drm.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drm/nouveau/nouveau_drm.c b/drm/nouveau/nouveau_drm.c
index 78d55c525..6a6819d96 100644
--- a/drm/nouveau/nouveau_drm.c
+++ b/drm/nouveau/nouveau_drm.c
@@ -911,6 +911,7 @@ nouveau_pmops_runtime_suspend(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct drm_device *drm_dev = pci_get_drvdata(pdev);
+	struct nouveau_drm *drm = nouveau_drm(drm_dev);
 	int ret;
 
 	if (!nouveau_pmops_runtime()) {
@@ -920,6 +921,10 @@ nouveau_pmops_runtime_suspend(struct device *dev)
 
 	nouveau_switcheroo_optimus_dsm();
 	ret = nouveau_do_suspend(drm_dev, true);
+	if (ret) {
+		NV_ERROR(drm, "suspend failed with: %d\n", ret);
+		return ret;
+	}
 	pci_save_state(pdev);
 	pci_disable_device(pdev);
 	pci_ignore_hotplug(pdev);
-- 
2.21.0

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH v4 2/4] pci: add nvkm_pcie_get_speed

[Ben Skeggs]

On Fri, 13 Sep 2019 at 21:33, Karol Herbst <kherbst@redhat.com> wrote:
>
> v2: fixed compilation error
Is there any need for this patch at all now, if you're forcing 8_0
rather than the pre-DEVINIT speed?

>
> Signed-off-by: Karol Herbst <kherbst@redhat.com>
> Reviewed-by: Lyude Paul <lyude@redhat.com>
> ---
>  drm/nouveau/include/nvkm/subdev/pci.h | 1 +
>  drm/nouveau/nvkm/subdev/pci/pcie.c    | 8 ++++++++
>  2 files changed, 9 insertions(+)
>
> diff --git a/drm/nouveau/include/nvkm/subdev/pci.h b/drm/nouveau/include/nvkm/subdev/pci.h
> index 4803a4fad..b29101e48 100644
> --- a/drm/nouveau/include/nvkm/subdev/pci.h
> +++ b/drm/nouveau/include/nvkm/subdev/pci.h
> @@ -53,4 +53,5 @@ 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);
> +enum nvkm_pcie_speed nvkm_pcie_get_speed(struct nvkm_pci *);
>  #endif
> diff --git a/drm/nouveau/nvkm/subdev/pci/pcie.c b/drm/nouveau/nvkm/subdev/pci/pcie.c
> index 354ac4c85..b4203ff1a 100644
> --- a/drm/nouveau/nvkm/subdev/pci/pcie.c
> +++ b/drm/nouveau/nvkm/subdev/pci/pcie.c
> @@ -177,3 +177,11 @@ nvkm_pcie_set_link(struct nvkm_pci *pci, enum nvkm_pcie_speed speed, u8 width)
>
>         return ret;
>  }
> +
> +enum nvkm_pcie_speed
> +nvkm_pcie_get_speed(struct nvkm_pci *pci)
> +{
> +       if (!pci || !pci_is_pcie(pci->pdev) || !pci->func->pcie.cur_speed)
> +               return -ENODEV;
> +       return pci->func->pcie.cur_speed(pci);
> +}
> --
> 2.21.0
>
> _______________________________________________
> Nouveau mailing list
> Nouveau@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

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

[Ben Skeggs]

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
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

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

[Karol Herbst]

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

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

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

[Ben Skeggs]

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
>
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

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

[Karol Herbst]

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
> >

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

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

[Ben Skeggs]

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
> > >
>
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH v4 1/4] pci: enable pcie link changes for pascal

[Lyude Paul]

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

On Fri, 2019-09-13 at 13:33 +0200, Karol Herbst wrote:
> v2: add force disable ASPM func pointer
> 
> Signed-off-by: Karol Herbst <kherbst@redhat.com>
> Reviewed-by: Lyude Paul <lyude@redhat.com> (v1)
> ---
>  drm/nouveau/nvkm/subdev/pci/gk104.c |  8 ++++----
>  drm/nouveau/nvkm/subdev/pci/gp100.c | 11 +++++++++++
>  drm/nouveau/nvkm/subdev/pci/priv.h  |  5 +++++
>  3 files changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drm/nouveau/nvkm/subdev/pci/gk104.c
> b/drm/nouveau/nvkm/subdev/pci/gk104.c
> index 804ef017f..61c4e76e8 100644
> --- a/drm/nouveau/nvkm/subdev/pci/gk104.c
> +++ b/drm/nouveau/nvkm/subdev/pci/gk104.c
> @@ -25,7 +25,7 @@
>  
>  #include <subdev/timer.h>
>  
> -static int
> +int
>  gk104_pcie_version_supported(struct nvkm_pci *pci)
>  {
>  	return (nvkm_rd32(pci->subdev.device, 0x8c1c0) & 0x4) == 0x4 ? 2 : 1;
> @@ -110,7 +110,7 @@ gk104_pcie_lnkctl_speed(struct nvkm_pci *pci)
>  	return -1;
>  }
>  
> -static enum nvkm_pcie_speed
> +enum nvkm_pcie_speed
>  gk104_pcie_max_speed(struct nvkm_pci *pci)
>  {
>  	u32 max_speed = nvkm_rd32(pci->subdev.device, 0x8c1c0) & 0x300000;
> @@ -156,7 +156,7 @@ gk104_pcie_set_link_speed(struct nvkm_pci *pci, enum
> nvkm_pcie_speed speed)
>  	return 0;
>  }
>  
> -static int
> +int
>  gk104_pcie_init(struct nvkm_pci * pci)
>  {
>  	enum nvkm_pcie_speed lnkctl_speed, max_speed, cap_speed;
> @@ -188,7 +188,7 @@ gk104_pcie_init(struct nvkm_pci * pci)
>  	return 0;
>  }
>  
> -static int
> +int
>  gk104_pcie_set_link(struct nvkm_pci *pci, enum nvkm_pcie_speed speed, u8
> width)
>  {
>  	struct nvkm_subdev *subdev = &pci->subdev;
> diff --git a/drm/nouveau/nvkm/subdev/pci/gp100.c
> b/drm/nouveau/nvkm/subdev/pci/gp100.c
> index 82c5234a0..163233f16 100644
> --- a/drm/nouveau/nvkm/subdev/pci/gp100.c
> +++ b/drm/nouveau/nvkm/subdev/pci/gp100.c
> @@ -35,6 +35,17 @@ gp100_pci_func = {
>  	.wr08 = nv40_pci_wr08,
>  	.wr32 = nv40_pci_wr32,
>  	.msi_rearm = gp100_pci_msi_rearm,
> +
> +	.pcie.init = gk104_pcie_init,
> +	.pcie.set_link = gk104_pcie_set_link,
> +
> +	.pcie.max_speed = gk104_pcie_max_speed,
> +	.pcie.cur_speed = g84_pcie_cur_speed,
> +
> +	.pcie.set_version = gf100_pcie_set_version,
> +	.pcie.version = gf100_pcie_version,
> +	.pcie.version_supported = gk104_pcie_version_supported,
> +	.pcie.force_aspm_off = g84_pcie_force_aspm_off,
>  };
>  
>  int
> diff --git a/drm/nouveau/nvkm/subdev/pci/priv.h
> b/drm/nouveau/nvkm/subdev/pci/priv.h
> index c6a9ef330..82c78befa 100644
> --- a/drm/nouveau/nvkm/subdev/pci/priv.h
> +++ b/drm/nouveau/nvkm/subdev/pci/priv.h
> @@ -56,6 +56,11 @@ int gf100_pcie_cap_speed(struct nvkm_pci *);
>  int gf100_pcie_init(struct nvkm_pci *);
>  int gf100_pcie_set_link(struct nvkm_pci *, enum nvkm_pcie_speed, u8);
>  
> +int gk104_pcie_init(struct nvkm_pci *);
> +int gk104_pcie_set_link(struct nvkm_pci *, enum nvkm_pcie_speed, u8 width);
> +enum nvkm_pcie_speed gk104_pcie_max_speed(struct nvkm_pci *);
> +int gk104_pcie_version_supported(struct nvkm_pci *);
> +
>  int nvkm_pcie_oneinit(struct nvkm_pci *);
>  int nvkm_pcie_init(struct nvkm_pci *);
>  #endif
-- 
Cheers,
	Lyude Paul

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

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

[Lyude Paul]

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
-- 
Cheers,
	Lyude Paul

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau