Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] Unbalanced teardown and PM rollback failure in r570_fbsr_suspend() on error paths, leading to permanent GPU malfunction. -- --- Patch [2]: [PATCH v5 2/4] drm/nouveau/gsp/r570: Set GcOff = 0 in fbsr --- commit 16fc4506e7114a3cfe615883d89f54f5f2c54a54 Author: Lyude Paul <[email protected]> drm/nouveau/gsp/r570: Set GcOff = 0 in fbsr This commit fixes suspend/resume on some desktops by setting GcOff to 0. It turns out that setting GcOff to 1 causes GSP to save and restore buffers that actually need to be reinitialized on resume. > 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 > @@ -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) [Severity: High] This isn't a bug introduced by this patch, but while looking at the suspend paths in this file, I noticed an issue in r570_fbsr_suspend(). If nvkm_instobj_save() fails, it returns without restoring channel scheduling: r570_fbsr_suspend() { ... ret = nvkm_instobj_save(iobj); if (ret) return ret; ... } Similarly, if nvkm_gsp_sg() fails later in the same function: r570_fbsr_suspend() { ... ret = nvkm_gsp_sg(device, size, &gsp->sr.fbsr); if (ret) return ret; ... } Does this leave the system in an unusable state if a memory allocation failure occurs? It appears that returning immediately here skips restoring the channels or freeing previously allocated iobj->suspend buffers. During a suspend failure, nvkm_device_fini() aborts the suspend process and attempts to restart devices by calling nvkm_subdev_init(), which invokes r535_gsp_init(). Because gsp->sr.meta.data was destroyed and set to NULL on the error path, r535_gsp_init() will skip calling rm->api->fbsr->resume(gsp). Could this permanently disable the channels and leave the GPU in a broken suspended state until the system is rebooted? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
