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

[sashiko-bot@kernel.org]

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