[PATCH 0/5] drm/nouveau/gsp/r570: Follow OpenRM's runtime PM process more closely

[PATCH 0/5] drm/nouveau/gsp/r570: Follow OpenRM's runtime PM process more closely

[Lyude Paul]

While we have runtime PM working a lot more reliably then in the past
with R570, we're still doing things a lot differently then OpenRM. For
instance, we don't save/restore comptag backing stores, and we don't ask
GSP whether it's OK to enter GcOff.

This series fixes this, and gets our runtime PM process to look a lot
closer to OpenRM's by implementing some missing pieces and workarounds.

Tested on my daily-driver desktop with an AD102, a ThinkPad P17 Gen1
with a GA103GLM, and a Dell Precision 7780 with an AD103.

Patch series based on drm-misc/drm-misc-next. Branch with patches here:

    https://gitlab.freedesktop.org/lyudess/linux/-/commits/nouveau/rpm-openrm-sync

Lyude Paul (5):
  drm/nouveau/gsp/r535: Add support for INTERNAL_GCX_ENTRY_PREREQUISITE
  drm/nouveau/gsp/r535: Add support for MEMSYS_GET_STATIC_CONFIG
  drm/nouveau/gsp/r570: Add comp mode workaround from issue #3172217
  drm/nouveau/gsp/r570: Start saving comptag backing stores
  drm/nouveau/gsp/r570: Enable Gcoff in fbsr again

 drivers/gpu/drm/nouveau/include/nvif/cl0080.h | 10 +++
 drivers/gpu/drm/nouveau/include/nvif/device.h |  1 +
 .../gpu/drm/nouveau/include/nvkm/subdev/gsp.h |  7 ++
 drivers/gpu/drm/nouveau/nouveau_drm.c         |  9 +++
 drivers/gpu/drm/nouveau/nvif/device.c         | 13 ++++
 .../gpu/drm/nouveau/nvkm/engine/device/user.c | 36 +++++++++
 .../gpu/drm/nouveau/nvkm/subdev/gsp/priv.h    |  1 +
 .../drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c | 43 +++++++++++
 .../nvkm/subdev/gsp/rm/r535/nvrm/gsp.h        | 52 +++++++++++++
 .../nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c    | 76 ++++++++++++++++++-
 .../drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c |  8 ++
 .../nvkm/subdev/gsp/rm/r570/nvrm/fbsr.h       | 29 +++++++
 .../nvkm/subdev/gsp/rm/r570/nvrm/gsp.h        | 50 ++++++++++++
 .../gpu/drm/nouveau/nvkm/subdev/gsp/rm/rm.h   |  2 +
 14 files changed, 335 insertions(+), 2 deletions(-)


base-commit: 9d4c98219598e77ca7044c453ae6994519ac0a96
-- 
2.55.0

[PATCH 1/5] drm/nouveau/gsp/r535: Add support for INTERNAL_GCX_ENTRY_PREREQUISITE

[Lyude Paul]

OpenRM's runtime PM handling looks a bit different then nouveau's, one part
in particular that differs from us: OpenRM actually consults GSP to ask
whether the GPU should be allowed to enter Gc6 and/or GcOff before runtime
suspending the GPU. In the event the card isn't ready, runtime suspend is
simply delayed for a few seconds before retrying.

Implement the command used for querying GSP about this,
NV2080_CTRL_CMD_INTERNAL_GCX_ENTRY_PREREQUISITE, and check to ensure that
the GPU is ready for runtime suspend in nouveau_pmops_runtime_suspend()
using this query. If the GPU can't be runtime suspended, update the last
busy counter of the device and then return -EBUSY from
nouveau_pmops_runtime_suspend() - essentially delaying the runtime suspend
process by whatever autosuspend_delay_ms is set to.

Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 drivers/gpu/drm/nouveau/include/nvif/cl0080.h | 10 ++++++
 drivers/gpu/drm/nouveau/include/nvif/device.h |  1 +
 .../gpu/drm/nouveau/include/nvkm/subdev/gsp.h |  3 ++
 drivers/gpu/drm/nouveau/nouveau_drm.c         |  9 +++++
 drivers/gpu/drm/nouveau/nvif/device.c         | 13 +++++++
 .../gpu/drm/nouveau/nvkm/engine/device/user.c | 36 +++++++++++++++++++
 .../gpu/drm/nouveau/nvkm/subdev/gsp/priv.h    |  1 +
 .../drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c | 26 ++++++++++++++
 .../nvkm/subdev/gsp/rm/r535/nvrm/gsp.h        |  7 ++++
 .../gpu/drm/nouveau/nvkm/subdev/gsp/rm/rm.h   |  1 +
 10 files changed, 107 insertions(+)

diff --git a/drivers/gpu/drm/nouveau/include/nvif/cl0080.h b/drivers/gpu/drm/nouveau/include/nvif/cl0080.h
index ea8267e0d8da2..9e639df1da467 100644
--- a/drivers/gpu/drm/nouveau/include/nvif/cl0080.h
+++ b/drivers/gpu/drm/nouveau/include/nvif/cl0080.h
@@ -4,6 +4,7 @@
 
 #define NV_DEVICE_V0_INFO                                                  0x00
 #define NV_DEVICE_V0_TIME                                                  0x01
+#define NV_DEVICE_V0_GCX_READY						   0x02
 
 struct nv_device_info_v0 {
 	__u8  version;
@@ -55,6 +56,15 @@ struct nv_device_time_v0 {
 	__u64 time;
 };
 
+#define NV_DEVICE_GC6_READY	BIT(0)
+#define NV_DEVICE_GCOFF_READY	BIT(1)
+
+struct nv_device_gcx_ready_v0 {
+	__u8 version;
+	__u8 pad01[6];
+	__u8 ready;
+};
+
 #define NV_DEVICE_INFO_UNIT                               (0xffffffffULL << 32)
 #define NV_DEVICE_INFO(n)                          ((n) | (0x00000000ULL << 32))
 #define NV_DEVICE_HOST(n)                          ((n) | (0x00000001ULL << 32))
diff --git a/drivers/gpu/drm/nouveau/include/nvif/device.h b/drivers/gpu/drm/nouveau/include/nvif/device.h
index 7877a2a79da9b..ce2fadcb05d9e 100644
--- a/drivers/gpu/drm/nouveau/include/nvif/device.h
+++ b/drivers/gpu/drm/nouveau/include/nvif/device.h
@@ -22,4 +22,5 @@ int  nvif_device_ctor(struct nvif_client *, const char *name, struct nvif_device
 void nvif_device_dtor(struct nvif_device *);
 int  nvif_device_map(struct nvif_device *);
 u64  nvif_device_time(struct nvif_device *);
+int  nvif_device_gcx_ready(struct nvif_device *);
 #endif
diff --git a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
index 64fed208e4cf8..2bccf58f0f53c 100644
--- a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
+++ b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
@@ -10,6 +10,9 @@
 #define GSP_PAGE_SHIFT 12
 #define GSP_PAGE_SIZE  BIT(GSP_PAGE_SHIFT)
 
+#define GSP_GCX_ENTRY_GC6_READY		BIT(0)
+#define GSP_GCX_ENTRY_GCOFF_READY	BIT(1)
+
 struct nvkm_gsp_mem {
 	struct device *dev;
 	size_t size;
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index b0f9fb10a74dd..12300700d331f 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -1157,6 +1157,7 @@ nouveau_pmops_runtime_suspend(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct nouveau_drm *drm = pci_get_drvdata(pdev);
+	struct nvif_device *nvif = &drm->client.device;
 	int ret;
 
 	if (!nouveau_pmops_runtime()) {
@@ -1164,6 +1165,14 @@ nouveau_pmops_runtime_suspend(struct device *dev)
 		return -EBUSY;
 	}
 
+	// Check if the GPU itself is ready for runtime suspend, otherwise mark as busy and check
+	// again in a bit.
+	if (!(nvif_device_gcx_ready(nvif) & NV_DEVICE_GCOFF_READY)) {
+		NV_DEBUG(drm, "GPU isn't ready for suspend yet, delaying...\n");
+		pm_runtime_mark_last_busy(dev);
+		return -EBUSY;
+	}
+
 	nouveau_switcheroo_optimus_dsm();
 	ret = nouveau_do_suspend(drm, true);
 	pci_save_state(pdev);
diff --git a/drivers/gpu/drm/nouveau/nvif/device.c b/drivers/gpu/drm/nouveau/nvif/device.c
index 24880931039f6..1be9fbe6cb703 100644
--- a/drivers/gpu/drm/nouveau/nvif/device.c
+++ b/drivers/gpu/drm/nouveau/nvif/device.c
@@ -38,6 +38,19 @@ nvif_device_time(struct nvif_device *device)
 	return device->user.func->time(&device->user);
 }
 
+int
+nvif_device_gcx_ready(struct nvif_device *device)
+{
+	struct nv_device_gcx_ready_v0 args = {};
+	int ret;
+
+	ret = nvif_object_mthd(&device->object, NV_DEVICE_V0_GCX_READY, &args, sizeof(args));
+	if (ret)
+		return ret;
+
+	return args.ready;
+}
+
 int
 nvif_device_map(struct nvif_device *device)
 {
diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/user.c b/drivers/gpu/drm/nouveau/nvkm/engine/device/user.c
index 23d11d8221cb6..f602e4a07fbb0 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/device/user.c
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/user.c
@@ -27,6 +27,8 @@
 
 #include <core/client.h>
 #include <subdev/fb.h>
+#include <subdev/gsp.h>
+#include <subdev/gsp/priv.h>
 #include <subdev/instmem.h>
 #include <subdev/timer.h>
 
@@ -189,6 +191,38 @@ nvkm_udevice_time(struct nvkm_udevice *udev, void *data, u32 size)
 	return ret;
 }
 
+static int
+nvkm_udevice_gcx_ready(struct nvkm_udevice *udev, void *data, u32 size)
+{
+	struct nvkm_object *object = &udev->object;
+	struct nvkm_device *device = udev->device;
+	struct nvkm_gsp *gsp = device->gsp;
+	union {
+		struct nv_device_gcx_ready_v0 v0;
+	} *args = data;
+	int ret = -ENOSYS;
+
+	/* XXX: We don't know any way of performing this check outside of GSP, so just assume
+	 * things are ready for non-GSP clients.
+	 */
+	if (!device->gsp)
+		return NV_DEVICE_GC6_READY | NV_DEVICE_GCOFF_READY;
+
+	nvif_ioctl(object, "device gcx ready size %d\n", size);
+	if (!(ret = nvif_unpack(ret, &data, &size, args->v0, 0, 0, false))) {
+		nvif_ioctl(object, "device gcx ready vers %d\n", args->v0.version);
+
+		ret = r535_gsp_gcx_ready(gsp);
+		if (ret < 0)
+			return ret;
+
+		args->v0.ready = ret;
+		ret = 0;
+	}
+
+	return ret;
+}
+
 static int
 nvkm_udevice_mthd(struct nvkm_object *object, u32 mthd, void *data, u32 size)
 {
@@ -199,6 +233,8 @@ nvkm_udevice_mthd(struct nvkm_object *object, u32 mthd, void *data, u32 size)
 		return nvkm_udevice_info(udev, data, size);
 	case NV_DEVICE_V0_TIME:
 		return nvkm_udevice_time(udev, data, size);
+	case NV_DEVICE_V0_GCX_READY:
+		return nvkm_udevice_gcx_ready(udev, data, size);
 	default:
 		break;
 	}
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/priv.h b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/priv.h
index 71b7203bef507..b07797813b049 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/priv.h
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/priv.h
@@ -93,6 +93,7 @@ void r535_gsp_dtor(struct nvkm_gsp *);
 int r535_gsp_oneinit(struct nvkm_gsp *);
 int r535_gsp_init(struct nvkm_gsp *);
 int r535_gsp_fini(struct nvkm_gsp *, enum nvkm_suspend_state suspend);
+int r535_gsp_gcx_ready(struct nvkm_gsp *gsp);
 
 int nvkm_gsp_new_(const struct nvkm_gsp_fwif *, struct nvkm_device *, enum nvkm_subdev_type, int,
 		  struct nvkm_gsp **);
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
index 94925f1590ea4..09b17b85dc1ac 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
@@ -30,6 +30,7 @@
 #include <engine/fifo/chan.h>
 #include <engine/sec2.h>
 #include <nvif/log.h>
+#include <nvif/cl0080.h>
 
 #include <nvfw/fw.h>
 
@@ -1721,6 +1722,31 @@ r535_gsp_sr_data_size(struct nvkm_gsp *gsp)
 	return meta->gspFwWprEnd - meta->gspFwWprStart;
 }
 
+int
+r535_gsp_gcx_ready(struct nvkm_gsp *gsp)
+{
+	NV2080_CTRL_INTERNAL_GCX_ENTRY_PREREQUISITE_PARAMS *ctrl;
+	int ret = 0;
+
+	ctrl = nvkm_gsp_rm_ctrl_rd(&gsp->internal.device.subdevice,
+				   NV2080_CTRL_CMD_INTERNAL_GCX_ENTRY_PREREQUISITE,
+				   sizeof(*ctrl));
+	if (IS_ERR(ctrl))
+		return PTR_ERR(ctrl);
+
+	if (ctrl->bIsGC6Satisfied)
+		ret |= NV_DEVICE_GC6_READY;
+	if (ctrl->bIsGCOFFSatisfied)
+		ret |= NV_DEVICE_GCOFF_READY;
+
+	nvkm_debug(&gsp->subdev,
+		   "GCX ready status: GC6=%s GCOFF=%s\n",
+		   str_yes_no(ctrl->bIsGC6Satisfied), str_yes_no(ctrl->bIsGCOFFSatisfied));
+
+	nvkm_gsp_rm_ctrl_done(&gsp->internal.client.object, ctrl);
+	return ret;
+}
+
 int
 r535_gsp_fini(struct nvkm_gsp *gsp, enum nvkm_suspend_state suspend)
 {
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/nvrm/gsp.h b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/nvrm/gsp.h
index b6683a5bf870c..fd1170037c030 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/nvrm/gsp.h
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/nvrm/gsp.h
@@ -782,6 +782,13 @@ typedef struct NV2080_CTRL_INTERNAL_INTR_GET_KERNEL_TABLE_PARAMS {
 
 #define GSP_FW_HEAP_PARAM_CLIENT_ALLOC_SIZE      ((48 << 10) * 2048)   // Support 2048 channels
 
+#define NV2080_CTRL_CMD_INTERNAL_GCX_ENTRY_PREREQUISITE (0x2080a7d7)
+
+typedef struct NV2080_CTRL_INTERNAL_GCX_ENTRY_PREREQUISITE_PARAMS {
+    NvBool bIsGC6Satisfied;
+    NvBool bIsGCOFFSatisfied;
+} NV2080_CTRL_INTERNAL_GCX_ENTRY_PREREQUISITE_PARAMS;
+
 typedef union rpc_message_rpc_union_field_v03_00
 {
     NvU32      spare;
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/rm.h b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/rm.h
index e9ac47d86b69a..3de7871e22934 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/rm.h
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/rm.h
@@ -174,6 +174,7 @@ int r535_gr_chan_new(struct nvkm_gr *, struct nvkm_chan *, const struct nvkm_ocl
 int r535_gr_promote_ctx(struct r535_gr *, bool golden, struct nvkm_vmm *,
 			struct nvkm_memory **pctxbuf_mem, struct nvkm_vma **pctxbuf_vma,
 			struct nvkm_gsp_object *chan);
+int r535_gsp_gcx_ready(struct nvkm_gsp *gsp);
 extern const struct nvkm_rm_api_engine r535_nvdec;
 extern const struct nvkm_rm_api_engine r535_nvenc;
 extern const struct nvkm_rm_api_engine r535_nvjpg;
-- 
2.55.0

[PATCH 2/5] drm/nouveau/gsp/r535: Add support for MEMSYS_GET_STATIC_CONFIG

[Lyude Paul]

This is a GSP structure describing various characteristics of the memory
management system that GSP provides. Start by fetching it during driver
load, but don't do anything with the information we get from it just yet.

Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 .../gpu/drm/nouveau/include/nvkm/subdev/gsp.h |  4 ++
 .../drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c | 17 +++++++
 .../nvkm/subdev/gsp/rm/r535/nvrm/gsp.h        | 45 +++++++++++++++++
 .../drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c |  8 +++
 .../nvkm/subdev/gsp/rm/r570/nvrm/gsp.h        | 50 +++++++++++++++++++
 .../gpu/drm/nouveau/nvkm/subdev/gsp/rm/rm.h   |  1 +
 6 files changed, 125 insertions(+)

diff --git a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
index 2bccf58f0f53c..916f3120c5ccc 100644
--- a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
+++ b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
@@ -159,6 +159,10 @@ struct nvkm_gsp {
 		struct sg_table fbsr;
 	} sr;
 
+	struct {
+		bool use_raw_mode_comptagline_alloc;
+	} memsys;
+
 	struct {
 		struct nvkm_gsp_mem mem;
 
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
index 09b17b85dc1ac..c57a3250d6b96 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
@@ -1808,6 +1808,23 @@ r535_gsp_fini(struct nvkm_gsp *gsp, enum nvkm_suspend_state suspend)
 	return 0;
 }
 
+int
+r535_gsp_get_static_memsys_info(struct nvkm_gsp *gsp)
+{
+	NV2080_CTRL_INTERNAL_MEMSYS_GET_STATIC_CONFIG_PARAMS *ctrl;
+
+	ctrl = nvkm_gsp_rm_ctrl_get(&gsp->internal.device.subdevice,
+				    NV2080_CTRL_CMD_INTERNAL_MEMSYS_GET_STATIC_CONFIG,
+				    sizeof(*ctrl));
+	if (IS_ERR(ctrl))
+		return PTR_ERR(ctrl);
+
+	gsp->memsys.use_raw_mode_comptagline_alloc = ctrl->bUseRawModeComptaglineAllocation;
+
+	nvkm_gsp_rm_ctrl_done(&gsp->internal.device.subdevice, ctrl);
+	return 0;
+}
+
 int
 r535_gsp_init(struct nvkm_gsp *gsp)
 {
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/nvrm/gsp.h b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/nvrm/gsp.h
index fd1170037c030..fee5418a72a68 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/nvrm/gsp.h
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/nvrm/gsp.h
@@ -789,6 +789,51 @@ typedef struct NV2080_CTRL_INTERNAL_GCX_ENTRY_PREREQUISITE_PARAMS {
     NvBool bIsGCOFFSatisfied;
 } NV2080_CTRL_INTERNAL_GCX_ENTRY_PREREQUISITE_PARAMS;
 
+typedef struct NV2080_CTRL_INTERNAL_MEMSYS_GET_STATIC_CONFIG_PARAMS {
+    /*! Determines if RM should use 1 to 1 Comptagline allocation policy */
+    NvBool bOneToOneComptagLineAllocation;
+
+    /*! Determines if RM should use 1 to 4 Comptagline allocation policy */
+    NvBool bUseOneToFourComptagLineAllocation;
+
+    /*! Determines if RM should use raw Comptagline allocation policy */
+    NvBool bUseRawModeComptaglineAllocation;
+
+    /*! Has COMPBIT_BACKING_SIZE been overridden to zero (i.e. disabled)? */
+    NvBool bDisableCompbitBacking;
+
+    /*! Determine if we need to disable post L2 compression */
+    NvBool bDisablePostL2Compression;
+
+    /*! Is ECC DRAM feature supported? */
+    NvBool bEnabledEccFBPA;
+
+    NvBool bL2PreFill;
+
+    /*! L2 cache size */
+    NV_DECLARE_ALIGNED(NvU64 l2CacheSize, 8);
+
+    /*! Indicate whether fpba is present or not */
+    NvBool bFbpaPresent;
+
+    /*! Size covered by one comptag */
+    NvU32  comprPageSize;
+
+    /*! log32(comprPageSize) */
+    NvU32  comprPageShift;
+
+    /*! RAM type */
+    NvU32  ramType;
+
+    /*! LTC count */
+    NvU32  ltcCount;
+
+    /*! LTS per LTC count */
+    NvU32  ltsPerLtcCount;
+} NV2080_CTRL_INTERNAL_MEMSYS_GET_STATIC_CONFIG_PARAMS;
+
+#define NV2080_CTRL_CMD_INTERNAL_MEMSYS_GET_STATIC_CONFIG        (0x20800a1c) /* finn: Evaluated from "(FINN_NV20_SUBDEVICE_0_INTERNAL_INTERFACE_ID << 8) | NV2080_CTRL_INTERNAL_MEMSYS_GET_STATIC_CONFIG_PARAMS_MESSAGE_ID" */
+
 typedef union rpc_message_rpc_union_field_v03_00
 {
     NvU32      spare;
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c
index b45781cd0dfdc..5a51fe7e5ab98 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c
@@ -137,6 +137,14 @@ r570_gsp_get_static_info(struct nvkm_gsp *gsp)
 		}
 	}
 
+	ret = r535_gsp_get_static_memsys_info(gsp);
+	if (ret) {
+		nvkm_error(&gsp->subdev, "Retrieving static memsys info failed\n");
+		return ret;
+	}
+	nvkm_debug(&gsp->subdev, "memsys: Use raw mode for comptag allocations? %s\n",
+		   str_yes_no(gsp->memsys.use_raw_mode_comptagline_alloc));
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/nvrm/gsp.h b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/nvrm/gsp.h
index c458569af9d72..e2d9d1949e4e1 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/nvrm/gsp.h
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/nvrm/gsp.h
@@ -494,6 +494,56 @@ typedef struct
 
 #define GSP_FW_WPR_META_REVISION  1
 
+#define NV2080_CTRL_INTERNAL_MEMSYS_GET_STATIC_CONFIG_PARAMS_MESSAGE_ID (0x1CU)
+
+typedef struct NV2080_CTRL_INTERNAL_MEMSYS_GET_STATIC_CONFIG_PARAMS {
+    /*! Determines if RM should use 1 to 1 Comptagline allocation policy */
+    NvBool bOneToOneComptagLineAllocation;
+
+    /*! Determines if RM should use 1 to 4 Comptagline allocation policy */
+    NvBool bUseOneToFourComptagLineAllocation;
+
+    /*! Determines if RM should use raw Comptagline allocation policy */
+    NvBool bUseRawModeComptaglineAllocation;
+
+    /*! Has COMPBIT_BACKING_SIZE been overridden to zero (i.e. disabled)? */
+    NvBool bDisableCompbitBacking;
+
+    /*! Determine if we need to disable post L2 compression */
+    NvBool bDisablePostL2Compression;
+
+    /*! Is ECC DRAM feature supported? */
+    NvBool bEnabledEccFBPA;
+
+    NvBool bL2PreFill;
+
+    /*! L2 cache size */
+    NV_DECLARE_ALIGNED(NvU64 l2CacheSize, 8);
+
+    /*! Indicate whether fpba is present or not */
+    NvBool bFbpaPresent;
+
+    /*! Size covered by one comptag */
+    NvU32  comprPageSize;
+
+    /*! log32(comprPageSize) */
+    NvU32  comprPageShift;
+
+    /*! RAM type */
+    NvU32  ramType;
+
+    /*! LTC count */
+    NvU32  ltcCount;
+
+    /*! LTS per LTC count */
+    NvU32  ltsPerLtcCount;
+} NV2080_CTRL_INTERNAL_MEMSYS_GET_STATIC_CONFIG_PARAMS;
+
+/*!
+ * Retrieve Memory System Static data.
+ */
+#define NV2080_CTRL_CMD_INTERNAL_MEMSYS_GET_STATIC_CONFIG        (0x20800a1c) /* finn: Evaluated from "(FINN_NV20_SUBDEVICE_0_INTERNAL_INTERFACE_ID << 8) | NV2080_CTRL_INTERNAL_MEMSYS_GET_STATIC_CONFIG_PARAMS_MESSAGE_ID" */
+
 typedef struct {
     NvU64 sharedMemPhysAddr;
     NvU32 pageTableEntryCount;
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/rm.h b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/rm.h
index 3de7871e22934..46fdd11c4d2b3 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/rm.h
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/rm.h
@@ -175,6 +175,7 @@ int r535_gr_promote_ctx(struct r535_gr *, bool golden, struct nvkm_vmm *,
 			struct nvkm_memory **pctxbuf_mem, struct nvkm_vma **pctxbuf_vma,
 			struct nvkm_gsp_object *chan);
 int r535_gsp_gcx_ready(struct nvkm_gsp *gsp);
+int r535_gsp_get_static_memsys_info(struct nvkm_gsp *gsp);
 extern const struct nvkm_rm_api_engine r535_nvdec;
 extern const struct nvkm_rm_api_engine r535_nvenc;
 extern const struct nvkm_rm_api_engine r535_nvjpg;
-- 
2.55.0

[PATCH 3/5] drm/nouveau/gsp/r570: Add comp mode workaround from issue #3172217

[Lyude Paul]

One of the things that OpenRM does right before initiating fbsr is apply a
special workaround (nvidia issue #3172217) which temporarily disables raw
compression mode on the GPU. It is later re-enabled after resuming with
fbsr completes.

Since we don't currently save the compbit backing with fbsr, this shouldn't
currently make any functional difference in the suspend/resume process. But
it will be required for implementing support for saving and restoring
compbit backings from the GPU.

Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 .../nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c    | 47 +++++++++++++++++++
 .../nvkm/subdev/gsp/rm/r570/nvrm/fbsr.h       |  6 +++
 2 files changed, 53 insertions(+)

diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c
index af5aa5065c3dd..7f5aae0c055b8 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c
@@ -26,6 +26,34 @@ r570_fbsr_suspend_channels(struct nvkm_gsp *gsp, bool suspend)
 	return nvkm_gsp_rm_ctrl_wr(&gsp->internal.device.subdevice, ctrl);
 }
 
+static int
+r570_memsys_enable_raw_comp_mode(struct nvkm_gsp *gsp, bool enable)
+{
+	NV2080_CTRL_INTERNAL_MEMSYS_PROGRAM_RAW_COMPRESSION_MODE_PARAMS *ctrl;
+	int ret;
+
+	ctrl = nvkm_gsp_rm_ctrl_get(&gsp->internal.device.object,
+				    NV2080_CTRL_CMD_INTERNAL_MEMSYS_PROGRAM_RAW_COMPRESSION_MODE,
+				    sizeof(*ctrl));
+	if (IS_ERR(ctrl))
+		return PTR_ERR(ctrl);
+
+	ctrl->bRawMode = enable;
+
+	ret = nvkm_gsp_rm_ctrl_wr(&gsp->internal.device.subdevice, ctrl);
+	if (!ret)
+		nvkm_debug(&gsp->subdev, "Raw compression mode %s\n", str_enabled_disabled(enable));
+
+	return ret;
+}
+
+static bool
+r570_need_raw_comp_war(struct nvkm_gsp *gsp, struct nvkm_device *device)
+{
+	return (device->card_type == GA100 || device->card_type == AD100) &&
+	    gsp->memsys.use_raw_mode_comptagline_alloc;
+}
+
 static void
 r570_fbsr_resume(struct nvkm_gsp *gsp)
 {
@@ -33,6 +61,7 @@ r570_fbsr_resume(struct nvkm_gsp *gsp)
 	struct nvkm_instmem *imem = device->imem;
 	struct nvkm_instobj *iobj;
 	struct nvkm_vmm *vmm;
+	int ret;
 
 	/* Restore BAR2 page tables via BAR0 window, and re-enable BAR2. */
 	list_for_each_entry(iobj, &imem->boot, head) {
@@ -59,6 +88,13 @@ r570_fbsr_resume(struct nvkm_gsp *gsp)
 
 	/* Finish cleaning up. */
 	r535_fbsr_resume(gsp);
+
+	/* Re-enable raw mode if it was previously disabled */
+	if (r570_need_raw_comp_war(gsp, device)) {
+		ret = r570_memsys_enable_raw_comp_mode(gsp, true);
+		if (ret)
+			nvkm_error(&gsp->subdev, "Failed to re-enable raw comp mode\n");
+	}
 }
 
 static int
@@ -104,6 +140,17 @@ r570_fbsr_suspend(struct nvkm_gsp *gsp)
 	/* Stop channel scheduling. */
 	r570_fbsr_suspend_channels(gsp, true);
 
+	/* Temporarily disable raw mode to prevent FBSR restore operations from corrupting
+	 * compressed surfaces. Required for ampere and ada.
+	 *
+	 * Nvidia bug #3172217
+	 */
+	if (r570_need_raw_comp_war(gsp, device)) {
+		ret = r570_memsys_enable_raw_comp_mode(gsp, false);
+		if (ret)
+			return ret;
+	}
+
 	/* Save BAR2 allocations to system memory. */
 	list_for_each_entry(iobj, &imem->list, head) {
 		if (iobj->preserve) {
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/nvrm/fbsr.h b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/nvrm/fbsr.h
index 8af432375f7a2..9050a8274b273 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/nvrm/fbsr.h
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/nvrm/fbsr.h
@@ -16,4 +16,10 @@ typedef struct NV2080_CTRL_INTERNAL_FBSR_INIT_PARAMS {
     NV_DECLARE_ALIGNED(NvU64 sysmemAddrOfSuspendResumeData, 8);
 } NV2080_CTRL_INTERNAL_FBSR_INIT_PARAMS;
 
+#define NV2080_CTRL_CMD_INTERNAL_MEMSYS_PROGRAM_RAW_COMPRESSION_MODE (0x20800a6f) /* finn: Evaluated from "(FINN_NV20_SUBDEVICE_0_INTERNAL_INTERFACE_ID << 8) | NV2080_CTRL_INTERNAL_MEMSYS_PROGRAM_RAW_COMPRESSION_MODE_PARAMS_MESSAGE_ID" */
+
+typedef struct NV2080_CTRL_INTERNAL_MEMSYS_PROGRAM_RAW_COMPRESSION_MODE_PARAMS {
+    NvBool bRawMode;
+} NV2080_CTRL_INTERNAL_MEMSYS_PROGRAM_RAW_COMPRESSION_MODE_PARAMS;
+
 #endif
-- 
2.55.0

[PATCH 4/5] drm/nouveau/gsp/r570: Start saving comptag backing stores

[Lyude Paul]

One of the portions of OpenRM's fbsr process that we never implemented is
the saving and restoring of comptag backing stores. This isn't strictly
necessary for fbsr to work (as long as we don't specify bEnteringGcOff =
1), but implementing it brings us much closer to matching OpenRM's fbsr
process - which means we can rely on things being well tested on Nvidia's
side.

Now that we have the required driver workarounds in place and fetch the
required information from GSP's memsys on driver load, let's implement
support for this by fetching the required space for the compbit backing
stores and adding it to the amount of memory that we allocate for fbsr.

With this, we should be able to safely enable bEnteringGcOff in fbsr.

Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 .../nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c    | 27 ++++++++++++++++++-
 .../nvkm/subdev/gsp/rm/r570/nvrm/fbsr.h       | 23 ++++++++++++++++
 2 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c
index 7f5aae0c055b8..58f426ba81191 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c
@@ -26,6 +26,24 @@ r570_fbsr_suspend_channels(struct nvkm_gsp *gsp, bool suspend)
 	return nvkm_gsp_rm_ctrl_wr(&gsp->internal.device.subdevice, ctrl);
 }
 
+static u64
+r570_fb_get_compbit_store_size(struct nvkm_gsp *gsp)
+{
+	NV0080_CTRL_FB_GET_COMPBIT_STORE_INFO_PARAMS *ctrl;
+	u64 size;
+
+	ctrl = nvkm_gsp_rm_ctrl_rd(&gsp->internal.device.object,
+				   NV0080_CTRL_CMD_FB_GET_COMPBIT_STORE_INFO,
+				   sizeof(*ctrl));
+	if (IS_ERR(ctrl))
+		return PTR_ERR(ctrl);
+
+	size = ctrl->Size;
+
+	nvkm_gsp_rm_ctrl_done(&gsp->internal.device.subdevice, ctrl);
+	return size;
+}
+
 static int
 r570_memsys_enable_raw_comp_mode(struct nvkm_gsp *gsp, bool enable)
 {
@@ -134,7 +152,7 @@ r570_fbsr_suspend(struct nvkm_gsp *gsp)
 	struct nvkm_device *device = subdev->device;
 	struct nvkm_instmem *imem = device->imem;
 	struct nvkm_instobj *iobj;
-	u64 size;
+	u64 size, cbc_size;
 	int ret;
 
 	/* Stop channel scheduling. */
@@ -151,6 +169,11 @@ r570_fbsr_suspend(struct nvkm_gsp *gsp)
 			return ret;
 	}
 
+	cbc_size = r570_fb_get_compbit_store_size(gsp);
+	if (cbc_size < 0)
+		return cbc_size;
+	nvkm_debug(&gsp->subdev, "fbsr: Compbit backing store size: 0x%llx bytes\n", cbc_size);
+
 	/* Save BAR2 allocations to system memory. */
 	list_for_each_entry(iobj, &imem->list, head) {
 		if (iobj->preserve) {
@@ -173,6 +196,8 @@ r570_fbsr_suspend(struct nvkm_gsp *gsp)
 	size  = gsp->fb.heap.size;
 	size += gsp->fb.rsvd_size;
 	size += gsp->fb.bios.vga_workspace.size;
+	size += cbc_size;
+
 	nvkm_debug(subdev, "fbsr: size: 0x%llx bytes\n", size);
 
 	ret = nvkm_gsp_sg(device, size, &gsp->sr.fbsr);
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/nvrm/fbsr.h b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/nvrm/fbsr.h
index 9050a8274b273..cb3e448415b42 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/nvrm/fbsr.h
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/nvrm/fbsr.h
@@ -16,6 +16,29 @@ typedef struct NV2080_CTRL_INTERNAL_FBSR_INIT_PARAMS {
     NV_DECLARE_ALIGNED(NvU64 sysmemAddrOfSuspendResumeData, 8);
 } NV2080_CTRL_INTERNAL_FBSR_INIT_PARAMS;
 
+#define NV0080_CTRL_CMD_FB_GET_COMPBIT_STORE_INFO (0x801306) /* finn: Evaluated from "(FINN_NV01_DEVICE_0_FB_INTERFACE_ID << 8) | NV0080_CTRL_FB_GET_COMPBIT_STORE_INFO_PARAMS_MESSAGE_ID" */
+
+typedef struct NV0080_CTRL_FB_GET_COMPBIT_STORE_INFO_PARAMS {
+    NV_DECLARE_ALIGNED(NvU64 Size, 8);
+    NV_DECLARE_ALIGNED(NvU64 Address, 8);
+    NvU32 AddressSpace;
+    NvU32 MaxCompbitLine;
+    NvU32 comptagsPerCacheLine;
+    NvU32 cacheLineSize;
+    NvU32 cacheLineSizePerSlice;
+    NvU32 cacheLineFetchAlignment;
+    NV_DECLARE_ALIGNED(NvU64 backingStoreBase, 8);
+    NvU32 gobsPerComptagPerSlice;
+    NvU32 backingStoreCbcBase;
+    NvU32 comptaglineAllocationPolicy;
+    NV_DECLARE_ALIGNED(NvU64 privRegionStartOffset, 8);
+    NvU32 cbcCoveragePerSlice;
+} NV0080_CTRL_FB_GET_COMPBIT_STORE_INFO_PARAMS;
+
+#define NV0080_CTRL_CMD_FB_GET_COMPBIT_STORE_INFO_ADDRESS_SPACE_UNKNOWN 0 // ADDR_UNKNOWN
+#define NV0080_CTRL_CMD_FB_GET_COMPBIT_STORE_INFO_ADDRESS_SPACE_SYSMEM  1 // ADDR_SYSMEM
+#define NV0080_CTRL_CMD_FB_GET_COMPBIT_STORE_INFO_ADDRESS_SPACE_FBMEM   2 // ADDR_FBMEM
+
 #define NV2080_CTRL_CMD_INTERNAL_MEMSYS_PROGRAM_RAW_COMPRESSION_MODE (0x20800a6f) /* finn: Evaluated from "(FINN_NV20_SUBDEVICE_0_INTERNAL_INTERFACE_ID << 8) | NV2080_CTRL_INTERNAL_MEMSYS_PROGRAM_RAW_COMPRESSION_MODE_PARAMS_MESSAGE_ID" */
 
 typedef struct NV2080_CTRL_INTERNAL_MEMSYS_PROGRAM_RAW_COMPRESSION_MODE_PARAMS {
-- 
2.55.0

[PATCH 5/5] drm/nouveau/gsp/r570: Enable Gcoff in fbsr again

[Lyude Paul]

Now that we're properly saving the compbit backing stores on fbsr init, we
can start setting bEnteringGcOff = 1 again without things breaking, which
brings us closer to following the exact same code-paths OpenRM does for
fbsr.

Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c
index 58f426ba81191..8545be1dde6ed 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c
@@ -135,7 +135,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 = 0;
+	ctrl->bEnteringGcoffState = 1;
 
 	ret = nvkm_gsp_rm_ctrl_wr(&gsp->internal.device.subdevice, ctrl);
 	if (ret)
-- 
2.55.0

Re: [PATCH 2/5] drm/nouveau/gsp/r535: Add support for MEMSYS_GET_STATIC_CONFIG

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `r535_gsp_get_static_memsys_info` function allocates an RPC command payload but never actually executes the command before reading its fields, resulting in reading uninitialized memory.
- [Medium] The memsys static info is never fetched when running on r535 firmware because the call to `r535_gsp_get_static_memsys_info` is omitted from `r535_gsp_get_static_info`.
--

commit 80c8c37934b6a701a8974b57594b260a8c0690c0
Author: Lyude Paul <lyude@redhat.com>

drm/nouveau/gsp/r535: Add support for MEMSYS_GET_STATIC_CONFIG

This patch adds support for retrieving a GSP structure describing memory
management system characteristics during driver load. It fetches the
configuration for MEMSYS_GET_STATIC_CONFIG and saves it for later use.

> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> index 09b17b85dc1ac..c57a3250d6b96 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
[ ... ]
> @@ -1808,6 +1808,23 @@ r535_gsp_fini(struct nvkm_gsp *gsp, enum nvkm_suspend_state suspend)
>  	return 0;
>  }
>  
> +int
> +r535_gsp_get_static_memsys_info(struct nvkm_gsp *gsp)
> +{
> +	NV2080_CTRL_INTERNAL_MEMSYS_GET_STATIC_CONFIG_PARAMS *ctrl;
> +
> +	ctrl = nvkm_gsp_rm_ctrl_get(&gsp->internal.device.subdevice,
> +				    NV2080_CTRL_CMD_INTERNAL_MEMSYS_GET_STATIC_CONFIG,
> +				    sizeof(*ctrl));
> +	if (IS_ERR(ctrl))
> +		return PTR_ERR(ctrl);
> +
> +	gsp->memsys.use_raw_mode_comptagline_alloc = ctrl->bUseRawModeComptaglineAllocation;

[Severity: High]
Does this actually execute the RPC command to fetch the data?

It looks like nvkm_gsp_rm_ctrl_get() only allocates the payload buffer in
memory, but without a subsequent call to nvkm_gsp_rm_ctrl_push() (or by using
nvkm_gsp_rm_ctrl_rd() instead), the command is never pushed to the firmware.

As a result, this might be reading uninitialized memory instead of the
firmware's response. This could silently break the workaround introduced in
the subsequent commit "drm/nouveau/gsp/r570: Add comp mode workaround from
issue #3172217", as use_raw_mode_comptagline_alloc will contain garbage or
zero, preventing the workaround from being applied properly.

> +
> +	nvkm_gsp_rm_ctrl_done(&gsp->internal.device.subdevice, ctrl);
> +	return 0;
> +}
[ ... ]
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c
> index b45781cd0dfdc..5a51fe7e5ab98 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c
> @@ -137,6 +137,14 @@ r570_gsp_get_static_info(struct nvkm_gsp *gsp)
>  		}
>  	}
>  
> +	ret = r535_gsp_get_static_memsys_info(gsp);

[Severity: Medium]
Should r535_gsp_get_static_info() also be updated to call
r535_gsp_get_static_memsys_info(gsp)?

Currently, this is only added to r570_gsp_get_static_info(), which means the
memsys static info is never fetched when running on r535 firmware, and
gsp->memsys.use_raw_mode_comptagline_alloc will remain uninitialized for r535.

While the follow-up commit "drm/nouveau/gsp/r570: Add comp mode workaround
from issue #3172217" currently only uses this flag in r570-specific suspend
paths, the commit message implies this configuration should be fetched on
r535 as well. Any future shared code relying on this flag on r535 could
receive uninitialized data.

> +	if (ret) {
> +		nvkm_error(&gsp->subdev, "Retrieving static memsys info failed\n");
> +		return ret;
> +	}
> +	nvkm_debug(&gsp->subdev, "memsys: Use raw mode for comptag allocations? %s\n",
> +		   str_yes_no(gsp->memsys.use_raw_mode_comptagline_alloc));
> +
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911222428.752641-1-lyude@redhat.com?part=2

Re: [PATCH 1/5] drm/nouveau/gsp/r535: Add support for INTERNAL_GCX_ENTRY_PREREQUISITE

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Negative error codes from `nvif_device_gcx_ready()` are treated as bitmasks, leading to unpredictable runtime suspend behavior.
- [High] Hardcoding `r535_gsp_gcx_ready(gsp)` bypasses the GSP function vtable and will cause a NULL pointer dereference on non-RM GSPs.
- [Medium] `nvkm_udevice_gcx_ready()` bypasses the ioctl unpack protocol and returns a positive bitmask directly as a return code.
- [Low] `GSP_GCX_ENTRY_GC6_READY` and `GSP_GCX_ENTRY_GCOFF_READY` are defined but never used.
--

commit 9f05a64653e46f7b990f534168f7024b8a4d408a
Author: Lyude Paul <lyude@redhat.com>

drm/nouveau/gsp/r535: Add support for INTERNAL_GCX_ENTRY_PREREQUISITE

OpenRM consults GSP to check if the GPU should be allowed to enter Gc6
and/or GcOff before runtime suspending. If not ready, runtime suspend is
delayed. This commit implements the NV2080_CTRL_CMD_INTERNAL_GCX_ENTRY_PREREQUISITE
query and adds readiness checks in nouveau_pmops_runtime_suspend().

> diff --git a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
> --- a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
> +++ b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
> @@ -10,6 +10,9 @@
>  #define GSP_PAGE_SHIFT 12
>  #define GSP_PAGE_SIZE  BIT(GSP_PAGE_SHIFT)
>  
> +#define GSP_GCX_ENTRY_GC6_READY		BIT(0)
> +#define GSP_GCX_ENTRY_GCOFF_READY	BIT(1)

[Severity: Low]
This isn't a bug, but these macros appear to be unused in this patch?
It looks like NV_DEVICE_GC6_READY and NV_DEVICE_GCOFF_READY are used
instead throughout the rest of the implementation.

[ ... ]

> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -1157,6 +1157,7 @@ nouveau_pmops_runtime_suspend(struct device *dev)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
>  	struct nouveau_drm *drm = pci_get_drvdata(pdev);
> +	struct nvif_device *nvif = &drm->client.device;
>  	int ret;
>  
>  	if (!nouveau_pmops_runtime()) {
> @@ -1164,6 +1165,14 @@ nouveau_pmops_runtime_suspend(struct device *dev)
>  		return -EBUSY;
>  	}
>  
> +	// Check if the GPU itself is ready for runtime suspend, otherwise mark as busy and check
> +	// again in a bit.
> +	if (!(nvif_device_gcx_ready(nvif) & NV_DEVICE_GCOFF_READY)) {

[Severity: High]
Can this bitwise check misinterpret negative error codes?

Looking at nvif_device_gcx_ready(), it can return negative error codes
like -ENOSYS or other transport errors. Since negative numbers usually
have higher bits set in two's complement (e.g., -ENOSYS often has bit 1
set), the bitwise AND with NV_DEVICE_GCOFF_READY (which is BIT(1)) might
evaluate to non-zero.

This could cause the driver to mistakenly assume the GPU is ready and
proceed with runtime suspend when the query actually failed, leading to
unpredictable power transition behavior.

> +		NV_DEBUG(drm, "GPU isn't ready for suspend yet, delaying...\n");
> +		pm_runtime_mark_last_busy(dev);
> +		return -EBUSY;
> +	}

[ ... ]

> diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/user.c b/drivers/gpu/drm/nouveau/nvkm/engine/device/user.c
> --- a/drivers/gpu/drm/nouveau/nvkm/engine/device/user.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/user.c
> @@ -189,6 +191,38 @@ nvkm_udevice_time(struct nvkm_udevice *udev, void *data, u32 size)
>  	return ret;
>  }
>  
> +static int
> +nvkm_udevice_gcx_ready(struct nvkm_udevice *udev, void *data, u32 size)
> +{
> +	struct nvkm_object *object = &udev->object;
> +	struct nvkm_device *device = udev->device;
> +	struct nvkm_gsp *gsp = device->gsp;
> +	union {
> +		struct nv_device_gcx_ready_v0 v0;
> +	} *args = data;
> +	int ret = -ENOSYS;
> +
> +	/* XXX: We don't know any way of performing this check outside of GSP, so just assume
> +	 * things are ready for non-GSP clients.
> +	 */
> +	if (!device->gsp)
> +		return NV_DEVICE_GC6_READY | NV_DEVICE_GCOFF_READY;

[Severity: Medium]
Does this early return break the NVIF mthd API contract?

The convention for mthd handlers is generally to unpack the arguments,
set the output fields in the args struct, and return 0 on success. Returning
a positive bitmask directly here bypasses the nvif_unpack sequence.

This positive value then bubbles up to the caller in nvif_device_gcx_ready(),
where it acts as a pseudo-error code and leaves the actual args.ready field
uninitialized.

> +
> +	nvif_ioctl(object, "device gcx ready size %d\n", size);
> +	if (!(ret = nvif_unpack(ret, &data, &size, args->v0, 0, 0, false))) {
> +		nvif_ioctl(object, "device gcx ready vers %d\n", args->v0.version);
> +
> +		ret = r535_gsp_gcx_ready(gsp);

[Severity: High]
Could this lead to a NULL pointer dereference on architectures running
without full Resource Manager (RM) functionality?

When falling back to firmware like gv100_gsp_nofw (e.g., on Turing or
Ampere), device->gsp is non-NULL, so the early return above is bypassed.
However, the RM structures are not initialized.

When r535_gsp_gcx_ready() is called in this state, it eventually invokes
nvkm_gsp_rm_ctrl_rd() -> nvkm_gsp_rm_ctrl_get(), which attempts to
dereference object->client->gsp->rm->api. If gsp->rm is NULL, this will
crash.

Would checking for !nvkm_gsp_rm(device->gsp) rather than !device->gsp
safeguard against this?

> +		if (ret < 0)
> +			return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911222428.752641-1-lyude@redhat.com?part=1

Re: [PATCH 3/5] drm/nouveau/gsp/r570: Add comp mode workaround from issue #3172217

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] API Misuse causing command rejection: `r570_memsys_enable_raw_comp_mode()` allocates the RPC with the device object instead of the subdevice object.

Pre-existing issues:
- [High] Missing state cleanup on error paths in the suspend callback causes the GPU to hang permanently after a failed suspend.
--

commit 9926d57d576468c9d718471d17ce97302fb87674
Author: Lyude Paul <lyude@redhat.com>

drm/nouveau/gsp/r570: Add comp mode workaround from issue #3172217

This commit temporarily disables raw compression mode on the GPU right
before initiating fbsr during suspend, and re-enables it upon resume.
This workaround resolves an issue on Ampere and Ada GPUs.

> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c
[ ... ]
> +static int
> +r570_memsys_enable_raw_comp_mode(struct nvkm_gsp *gsp, bool enable)
> +{
> +	NV2080_CTRL_INTERNAL_MEMSYS_PROGRAM_RAW_COMPRESSION_MODE_PARAMS *ctrl;
> +	int ret;
> +
> +	ctrl = nvkm_gsp_rm_ctrl_get(&gsp->internal.device.object,
> +				    NV2080_CTRL_CMD_INTERNAL_MEMSYS_PROGRAM_RAW_COMPRESSION_MODE,
> +				    sizeof(*ctrl));

[Severity: High]
Does this code allocate the RPC with the device object instead of the
subdevice object?

By passing &gsp->internal.device.object, the header receives the NV0080
device handle, but the command (NV2080_CTRL_CMD_INTERNAL_MEMSYS_...) is an
NV2080 subdevice command. This mismatch between the command class and the
target object typically causes the GSP firmware to reject the command,
causing the workaround to fail and suspend to abort.

> +	if (IS_ERR(ctrl))
> +		return PTR_ERR(ctrl);
> +
> +	ctrl->bRawMode = enable;
> +
> +	ret = nvkm_gsp_rm_ctrl_wr(&gsp->internal.device.subdevice, ctrl);

The write operation here correctly uses &gsp->internal.device.subdevice,
which seems to confirm the intended target object is the subdevice. Should
the nvkm_gsp_rm_ctrl_get() call above also use the subdevice?

[ ... ]
> @@ -104,6 +140,17 @@ r570_fbsr_suspend(struct nvkm_gsp *gsp)
>  	/* Stop channel scheduling. */
>  	r570_fbsr_suspend_channels(gsp, true);
>  
> +	/* Temporarily disable raw mode to prevent FBSR restore operations from corrupting
> +	 * compressed surfaces. Required for ampere and ada.
> +	 *
> +	 * Nvidia bug #3172217
> +	 */
> +	if (r570_need_raw_comp_war(gsp, device)) {
> +		ret = r570_memsys_enable_raw_comp_mode(gsp, false);
> +		if (ret)
> +			return ret;

[Severity: High]
This isn't a bug introduced by this patch, but does this error path leave
the GPU channels permanently disabled?

If this function returns an error, the Linux PM framework does not invoke
the device's resume callback. This means r570_fbsr_suspend_channels(gsp,
false) is never called, potentially causing a hard hang of the GPU session.

While not introduced by this patch, adding a new command that might fail
(especially given the object mismatch noted above) heavily exacerbates the
missing cleanup on this error path.

> +	}
> +
>  	/* Save BAR2 allocations to system memory. */
>  	list_for_each_entry(iobj, &imem->list, head) {
>  		if (iobj->preserve) {
>  			ret = nvkm_instobj_save(iobj);
>  			if (ret)
>  				return ret;

[Severity: High]
This isn't a bug introduced by this patch, but this error path now also skips
re-enabling raw compression mode in addition to leaving channels disabled.
Could we use a goto label to ensure proper state cleanup before returning
suspend errors?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911222428.752641-1-lyude@redhat.com?part=3

Re: [PATCH 4/5] drm/nouveau/gsp/r570: Start saving comptag backing stores

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Critical] The error check `if (cbc_size < 0)` is always false because `cbc_size` is an unsigned `u64`. When `r570_fb_get_compbit_store_size()` fails, it returns a negative error code that is implicitly cast to a large positive `u64`, bypassing the error check and causing an integer wrap-around.
- [Low] Mismatched GSP object handles used for API allocation and cleanup in `r570_fb_get_compbit_store_size()`.

Pre-existing issues:
- [High] Missing cleanup of suspended channels on error paths in `r570_fbsr_suspend()`.
--

commit caaa903dee4578bf9a609b4561ba8eb17ac6e144
Author: Lyude Paul <lyude@redhat.com>

drm/nouveau/gsp/r570: Start saving comptag backing stores

This commit implements support for saving and restoring comptag backing stores
in OpenRM's fbsr process by fetching the required space and allocating it.
The goal is to safely enable bEnteringGcOff in fbsr.

> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c
> index 7f5aae0c055b8..58f426ba81191 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c
> @@ -26,6 +26,24 @@ r570_fbsr_suspend_channels(struct nvkm_gsp *gsp, bool suspend)
>  	return nvkm_gsp_rm_ctrl_wr(&gsp->internal.device.subdevice, ctrl);
>  }
>  
> +static u64
> +r570_fb_get_compbit_store_size(struct nvkm_gsp *gsp)
> +{
> +	NV0080_CTRL_FB_GET_COMPBIT_STORE_INFO_PARAMS *ctrl;
> +	u64 size;
> +
> +	ctrl = nvkm_gsp_rm_ctrl_rd(&gsp->internal.device.object,
> +				   NV0080_CTRL_CMD_FB_GET_COMPBIT_STORE_INFO,
> +				   sizeof(*ctrl));
> +	if (IS_ERR(ctrl))
> +		return PTR_ERR(ctrl);

[Severity: Critical]
Since r570_fb_get_compbit_store_size() returns a u64, does this implicitly
cast the negative signed error code (like -ENOMEM) into a huge positive value?

> +
> +	size = ctrl->Size;
> +
> +	nvkm_gsp_rm_ctrl_done(&gsp->internal.device.subdevice, ctrl);

[Severity: Low]
Does this violate API symmetry by allocating the control struct using
&gsp->internal.device.object but cleaning it up with
&gsp->internal.device.subdevice?

> +	return size;
> +}
> +
>  static int
>  r570_memsys_enable_raw_comp_mode(struct nvkm_gsp *gsp, bool enable)
>  {

[ ... ]

> @@ -151,6 +169,11 @@ r570_fbsr_suspend(struct nvkm_gsp *gsp)
>  			return ret;

[Severity: High]
This isn't a bug introduced by this patch, but does this error path fail to
undo r570_fbsr_suspend_channels(gsp, true) called earlier in the function?

If the suspend fails here, it seems the channels are left permanently
suspended, making the GPU unusable until a reboot.

>  	}
>  
> +	cbc_size = r570_fb_get_compbit_store_size(gsp);
> +	if (cbc_size < 0)
> +		return cbc_size;

[Severity: Critical]
Is this error check dead code? Since cbc_size is an unsigned u64, it can
never be less than 0.

If r570_fb_get_compbit_store_size() fails, the implicitly cast large positive
error code will bypass this check, leading to an integer wrap-around when
added to size later in the function.

Could this cause an undersized memory allocation that results in memory
corruption when the firmware attempts to write the FBSR state out of bounds?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911222428.752641-1-lyude@redhat.com?part=4

Re: [PATCH 5/5] drm/nouveau/gsp/r570: Enable Gcoff in fbsr again

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] An always-false `cbc_size < 0` check fails to catch errors in `r570_fbsr_suspend()`, causing arithmetic wrap-around and an undersized scatter-gather table allocation.
- [High] Error paths in `r570_fbsr_suspend()` fail to restore hardware state, leaving the GPU suspended.
--

commit 26e4bfb04a1b75eda1612ecdcc3766ff92fd2fb3
Author: Lyude Paul <lyude@redhat.com>

drm/nouveau/gsp/r570: Enable Gcoff in fbsr again

Now that we're properly saving the compbit backing stores on fbsr init, we
can start setting bEnteringGcOff = 1 again without things breaking, which
brings us closer to following the exact same code-paths OpenRM does for
fbsr.

> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c
> index 58f426ba81191..8545be1dde6ed 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c
> @@ -135,7 +135,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 = 0;
> +	ctrl->bEnteringGcoffState = 1;
>  
>  	ret = nvkm_gsp_rm_ctrl_wr(&gsp->internal.device.subdevice, ctrl);
>  	if (ret)

[Severity: Critical]
This is a pre-existing issue, but I noticed a potential issue in
r570_fbsr_suspend() while reviewing this file:

drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c:r570_fbsr_suspend() {
    ...
	u64 size, cbc_size;
    ...
	cbc_size = r570_fb_get_compbit_store_size(gsp);
	if (cbc_size < 0)
		return cbc_size;
    ...
}

Because cbc_size is typed as u64, is it possible for the cbc_size < 0
check to ever be true?

If r570_fb_get_compbit_store_size() returns a negative error code
like PTR_ERR(ctrl), it will be cast to a massive unsigned value. This
huge value would then be added to size later in the function, which
could wrap around and lead to allocating an undersized SG table. Can
this overrun the allocated scatter-gather table during suspend?


[Severity: High]
This isn't a bug introduced by this patch, but I found another issue with
the error paths in r570_fbsr_suspend():

drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c:r570_fbsr_suspend() {
	/* Stop channel scheduling. */
	r570_fbsr_suspend_channels(gsp, true);

	/* Temporarily disable raw mode ... */
	if (r570_need_raw_comp_war(gsp, device)) {
		ret = r570_memsys_enable_raw_comp_mode(gsp, false);
		if (ret)
			return ret;
	}
    ...
	/* Save BAR2 allocations to system memory. */
	list_for_each_entry(iobj, &imem->list, head) {
		if (iobj->preserve) {
			ret = nvkm_instobj_save(iobj);
			if (ret)
				return ret;
		}
	}
    ...
}

If operations like r570_memsys_enable_raw_comp_mode() or nvkm_instobj_save()
fail, the function returns immediately. Does this leave the GPU with channels
suspended and raw compression mode disabled without rolling them back?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911222428.752641-1-lyude@redhat.com?part=5