Re: [PATCH v5 2/4] drm/nouveau/gsp/r570: Set GcOff = 0 in fbsr

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