drm/nouveau: GSP runtime PM fixes

Runtime suspend/resume has been notoriously flakey since we started using the R570 GSP firmware, and possibly was also flakey for some time when we introduced GSP support initially.

These fixes get runtime PM to be dramatically more stable then it was before. Unfortunately even with this patch series, there are still some remaining issues that need to be fixed. These likely stem from the currently different code path that we need to take with these fixes in order to get suspend/resume working.

While we're making progress in understanding what exactly we're doing differently from OpenRM that makes this the case, it will likely take us a while before we actually have things working well enough to copy what OpenRM does. Taking precisely the same path that OpenRM takes however, will require some additional work - and at the very least these fixes do appear to get suspend/resume and runtime PM working the vast majority of the time.

Thanks for nvidia for providing context into what all of these various flags and settings do!

Previous version of this patch series:

https://patchwork.freedesktop.org/series/169457/

Patch-wide changes:
* Add magic sleep, this seems to get things stable - but boy I wish I could figure out why.

Lyude Paul (4):
Revert "nouveau/gsp: fix suspend/resume regression on r570 firmware" drm/nouveau/gsp/r570: Set GcOff = 0 in fbsr drm/nouveau/gsp/r570: Enable S/R Display workaround in GSP drm/nouveau/gsp: Increase delay for magic sleep in r535_gsp_fini()

 drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/fbsr.c    | 2 +-
 drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c     | 8 ++++++--
 drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c    | 8 ++++----
 drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c     | 3 ++-
 .../gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/nvrm/gsp.h    | 8 ++++++++
 drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/rm.h           | 2 +-
 6 files changed, 22 insertions(+), 9 deletions(-)

base-commit: f29ab62f7ed55eae7b246bb535ede56d3b338674

2.55.0

This reverts commit 8302d0afeaec0bc57d951dd085e0cffe997d4d18.

It turns out this looked like the right fix on some systems, but it's not - as this causes runtime PM to actually fail on many a laptop.

---
 drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/fbsr.c | 2 +-
 drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c  | 2 +-
 drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c | 8 ++++----
 drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/rm.h        | 2 +-
 4 files changed, 7 insertions(+), 7 deletions(-)

index f128330f30d7b..40bf83ea33ac0 100644
@@ -208,7 +208,7 @@ r535_fbsr_resume(struct nvkm_gsp *gsp)
 }
 
 static int
-r535_fbsr_suspend(struct nvkm_gsp *gsp, bool runtime)
+r535_fbsr_suspend(struct nvkm_gsp *gsp)
 {
 	struct nvkm_subdev *subdev = &gsp->subdev;
 	struct nvkm_device *device = subdev->device;

index f544afa12b6bb..4a3b771ded255 100644
@@ -1749,7 +1749,7 @@ r535_gsp_fini(struct nvkm_gsp *gsp, enum nvkm_suspend_state suspend)
 		sr->sysmemAddrOfSuspendResumeData = gsp->sr.radix3.lvl0.addr;
 		sr->sizeOfSuspendResumeData = len;
 
-		ret = rm->api->fbsr->suspend(gsp, suspend == NVKM_RUNTIME_SUSPEND);
+		ret = rm->api->fbsr->suspend(gsp);
 		if (ret) {
 			nvkm_gsp_mem_dtor(&gsp->sr.meta);
 			nvkm_gsp_radix3_dtor(gsp, &gsp->sr.radix3);

index 8ef8b4f655883..2945d5b4e5707 100644
@@ -62,7 +62,7 @@ r570_fbsr_resume(struct nvkm_gsp *gsp)
 }
 
 static int
-r570_fbsr_init(struct nvkm_gsp *gsp, struct sg_table *sgt, u64 size, bool runtime)
+r570_fbsr_init(struct nvkm_gsp *gsp, struct sg_table *sgt, u64 size)
 {
 	NV2080_CTRL_INTERNAL_FBSR_INIT_PARAMS *ctrl;
 	struct nvkm_gsp_object memlist;

@@ -81,7 +81,7 @@ r570_fbsr_init(struct nvkm_gsp *gsp, struct sg_table *sgt, u64 size, bool runtim
 	ctrl->hClient = gsp->internal.client.object.handle;
 	ctrl->hSysMem = memlist.handle;
 	ctrl->sysmemAddrOfSuspendResumeData = gsp->sr.meta.addr;
-	ctrl->bEnteringGcoffState = runtime ? 1 : 0;
+	ctrl->bEnteringGcoffState = 1;
 
 	ret = nvkm_gsp_rm_ctrl_wr(&gsp->internal.device.subdevice, ctrl);
 	if (ret)

@@ -92,7 +92,7 @@ r570_fbsr_init(struct nvkm_gsp *gsp, struct sg_table *sgt, u64 size, bool runtim
 }
 
 static int
-r570_fbsr_suspend(struct nvkm_gsp *gsp, bool runtime)
+r570_fbsr_suspend(struct nvkm_gsp *gsp)
 {
 	struct nvkm_subdev *subdev = &gsp->subdev;
 	struct nvkm_device *device = subdev->device;

@@ -133,7 +133,7 @@ r570_fbsr_suspend(struct nvkm_gsp *gsp, bool runtime)
 		return ret;
 
 	/* Initialise FBSR on RM. */
-	ret = r570_fbsr_init(gsp, &gsp->sr.fbsr, size, runtime);
+	ret = r570_fbsr_init(gsp, &gsp->sr.fbsr, size);
 	if (ret) {
 		nvkm_gsp_sg_free(device, &gsp->sr.fbsr);
 		return ret;

index a9af94adf9efc..0fb0e67406c67 100644
@@ -78,7 +78,7 @@ struct nvkm_rm_api {
 	} *device;
 
 	const struct nvkm_rm_api_fbsr {
-		int (*suspend)(struct nvkm_gsp *, bool runtime);
+		int (*suspend)(struct nvkm_gsp *);
 		void (*resume)(struct nvkm_gsp *);
 	} *fbsr;
 

2.55.0

Previously, it looked as if we were able to fix suspend/resume on some desktops by setting Gcoff based on whether or not we were entering runtime PM. This was a mistake though - the only time suspend/resume would end up actually working was if Gcoff = 0.

It seems like it's likely the main reason for this is the FBSR GcOff argument actually controls GSP's behavior with regards to which buffers it decides to save across suspend/resume. When GcOff = 1, RM reserved regions are saved unless they are marked as LOST_ON_SUSPEND, and RM channel-context and kernel-client buffers are also saved -including- when they are LOST_ON_SUSPEND. This means with GcOff = 1, we end up having GSP save and restore buffers that actually need to be reinitialized on resume - causing the failures we're setting.

Thanks to John Hubbard from Nvidia for providing some background on what these options do in the GSP firmware do!

---
V5:
* Fix commit title, GcOff should be 0 not 1

 drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 2945d5b4e5707..af5aa5065c3dd 100644
@@ -81,7 +81,7 @@ r570_fbsr_init(struct nvkm_gsp *gsp, struct sg_table *sgt, u64 size)
 	ctrl->hClient = gsp->internal.client.object.handle;
 	ctrl->hSysMem = memlist.handle;
 	ctrl->sysmemAddrOfSuspendResumeData = gsp->sr.meta.addr;
-	ctrl->bEnteringGcoffState = 1;
+	ctrl->bEnteringGcoffState = 0;
 
 	ret = nvkm_gsp_rm_ctrl_wr(&gsp->internal.device.subdevice, ctrl);
 	if (ret)

2.55.0

There's two flags that we've never been setting when asking GSP to suspend the GPU, which OpenRM does set:

GPU_STATE_FLAGS_PRESERVING
GPU_STATE_FLAGS_PM_TRANSITION

These flags aren't -supposed- to do much in GSP, they're mostly used by OpenRM itself for state tracking. The only thing they do from GSP's side is control whether or not a single display related workaround is applied during suspend.

But as it turns out, that single workaround is actually quite crucial for getting runtime PM working with nouveau - and without it set we end up seeing a lot more failures with runtime PM resume. So, let's start setting it.

---
 drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c     | 3 ++-
 .../gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/nvrm/gsp.h    | 8 ++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

index 996941c668ba9..3e391646d8f7d 100644
@@ -198,7 +198,8 @@ r570_gsp_set_rmargs(struct nvkm_gsp *gsp, bool resume)
 		args->srInitArguments.bInPMTransition = 0;
 	} else {
 		args->srInitArguments.oldLevel = NV2080_CTRL_GPU_SET_POWER_STATE_GPU_LEVEL_3;
-		args->srInitArguments.flags = 0;
+		args->srInitArguments.flags =
+			GPU_STATE_FLAGS_PRESERVING | GPU_STATE_FLAGS_PM_TRANSITION;
 		args->srInitArguments.bInPMTransition = 1;
 	}
 

index b6075021e74f5..c458569af9d72 100644
@@ -523,6 +523,14 @@ typedef struct
 
 #define NV2080_CTRL_GPU_SET_POWER_STATE_GPU_LEVEL_3            (0x00000003U)
 
+#define GPU_STATE_FLAGS_PRESERVING         BIT(0)  // GPU state is preserved
+#define GPU_STATE_FLAGS_VGA_TRANSITION     BIT(1)   // To be used with GPU_STATE_FLAGS_PRESERVING.
+#define GPU_STATE_FLAGS_PM_TRANSITION      BIT(2)   // To be used with GPU_STATE_FLAGS_PRESERVING.
+#define GPU_STATE_FLAGS_PM_SUSPEND         BIT(3)
+#define GPU_STATE_FLAGS_PM_HIBERNATE       BIT(4)
+#define GPU_STATE_FLAGS_GC6_TRANSITION     BIT(5)  // To be used with GPU_STATE_FLAGS_PRESERVING.
+#define GPU_STATE_FLAGS_FAST_UNLOAD        BIT(6)  // Used during windows restart, skips stateDestroy steps
+
 typedef struct
 {
     // Magic for verification by secure ucode

2.55.0

As it turns out, Turing isn't the only architecture that needs this. On this Dell Precision 7780 with an AD103 GPU, along with pretty much every other laptop I tested, runtime PM is still somewhat unreliable. At first glance it seems as if it's fixed, but lowering the autosuspend delay to 500ms and then doing a stress test of suspend/resume cycles on the GPU ends up causing everything to start timing out.

After quite a lot of digging, I eventually landed back on this magic timeout in r535_gsp_fini(). As it turns out, increasing the timeout ends up fixing the runtime PM issues as far as I can tell, even during intense stress testing.

Unfortunately after spending quite a bit of time trying to dig through OpenRM to figure out what this magic sleep is actually doing, I've also come up short with any reasonable explanation. In lieu of that, I'm going to include the observations I did make while trying to figure this out in hopes someone eventually does figure this out:

* The magic sleep has to occur after fbsr is initialized. Performing it at any time before that doesn't appear to work.
* In situations where runtime PM starts getting flaky, some rather interesting visual effects end up happening on occasion before the GPU fully falls over. In particular, squares that look like the result of an incomplete blitting operation to a tiled buffer end up showing up on applications like vkcube. Interestingly enough, they remain in precisely the same place between runtime PM cycles until the GPU falls over - even when restarting vkcube multiple times, and even when vkcube is actively updating the screen. Even more interestingly, they're not limited to a specific framebuffer - you can see the squares changing as the cube rotates around.
We cannot however, say that this is likely to be a incomplete fbsr operation. The magic sleep happens before fbsr is actually saved (which happens on the GSP unload), so it's something else.
* During a short bit of testing with a desktop that I have, the magic sleep seemed to make no difference to whether or not suspend/resume works. It seems to generally work almost always. So we can assume this is likely exclusive to runtime PM, not S3.

As well, here's a list of the things I tried before settling on the magic sleep:

* Hooking up NV2080_CTRL_CMD_INTERNAL_GCX_ENTRY_PREREQUISITE and then blocking runtime PM until OpenRM signals that GC6/GCOFF is ready appears to make no difference.
* Hooking up some (maybe not all, unsure about that part) bits of comptag saving including:
* Fetching static memsys information from GSP
* Adding the size of the comptag storage to the fbsr data
* Adding a GA103+ workaround for disabling raw compression mode during fbsr (it doesn't seem like it applies for any systems I tried it on anyhow)
* Setting bPreserveVideoMemoryAllocations=1 in GspSystemInfo

So, until we can figure this out properly - just sleep for longer.

---
 drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

index 4a3b771ded255..94925f1590ea4 100644
@@ -1761,8 +1761,12 @@ r535_gsp_fini(struct nvkm_gsp *gsp, enum nvkm_suspend_state suspend)
 		 * TODO: Debug the GSP firmware / RPC handling to find out why
 		 * without this Turing (but none of the other architectures)
 		 * ends up resetting all channels after resume.
+		 * Additionally, runtime suspend on other architectures quickly
+		 * becomes unreliable without this sleep. If you're experiencing
+		 * issues with runtime suspend, try bumping this delay up and
+		 * sending a patch if it fixes your GPU.
 		 */
-		msleep(50);
+		msleep(200);
 	}
 
 	ret = r535_gsp_rpc_unloading_guest_driver(gsp, suspend);

2.55.0

While I did not expect the ending to be make the magic sleep magicer, I appreciate I introduced the magic sleep and got to the same point in trying to explain it, and failed for it to make any sense.

For the series: