Re: [PATCH v4 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:
- [High] nouveau/gsp: guaranteed RPC failure in r570_memsys_enable_raw_comp_mode due to object mismatch
- [High] nouveau/gsp: missing hardware state rollback on fbsr suspend error paths
--

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

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

This commit adds a workaround to temporarily disable raw compression mode on
the GPU right before initiating fbsr, and re-enables it upon resuming. This
change is intended to prepare for saving and restoring compbit backings from
the GPU.

> 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..fe618a94461dc 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
[ ... ]
> +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.object, ctrl);

[Severity: High]
Does this command need to be sent to the subdevice object rather than the
device object?

The NV2080 prefix indicates that
NV2080_CTRL_CMD_INTERNAL_MEMSYS_PROGRAM_RAW_COMPRESSION_MODE
is a subdevice-level command. If it is sent to the device object
(&gsp->internal.device.object) instead of the subdevice object
(&gsp->internal.device.subdevice), won't the GSP firmware reject the RPC
due to a class mismatch?

If this fails unconditionally on Ampere and Ada GPUs, it would abort the
suspend/resume sequence.

[ ... ]
> @@ -104,6 +141,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]
Should we resume the channels before returning an error here?

If r570_memsys_enable_raw_comp_mode() fails, we exit r570_fbsr_suspend()
with an error, bypassing the rest of the suspend. This leaves the channels
permanently suspended since we don't roll back the call to
r570_fbsr_suspend_channels(gsp, true) made just above this block.

> +	}
> +
>  	/* 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]
Similarly, if nvkm_instobj_save() fails, do we need to re-enable the raw
compression mode and resume the channels?

Returning directly here leaves the hardware state modified without a rollback
path, which might cause the GPU to be stuck with suspended channels and
disabled compression until a system reboot or driver reload.

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