Re: [PATCH v2 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 3 potential issue(s) to consider:
- [High] drm/nouveau/gsp: object class mismatch for NV2080_CTRL_CMD_INTERNAL_MEMSYS_PROGRAM_RAW_COMPRESSION_MODE
- [High] drm/nouveau/gsp: missing hardware state rollback on suspend failure leaks GPU state
- [High] drm/nouveau/gsp: LIFO violation resumes channels before re-enabling raw comp mode
--

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

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

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.

> 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..569732320f3dd 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,35 @@ 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));

[Severity: High]
Does this command need to be sent to device.subdevice instead of
device.object? The RM firmware validates the object class and will reject this
subdevice command when sent to the NV01_DEVICE object, which will cause this
function to fail and break system suspend on Ampere and Ada GPUs.

> +	if (IS_ERR(ctrl))
> +		return PTR_ERR(ctrl);
> +
> +	ctrl->bRawMode = enable;

[ ... ]

> @@ -59,6 +89,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);

[Severity: High]
Is it safe to re-enable raw mode after channel scheduling is resumed?

In r570_fbsr_suspend(), channel scheduling is stopped before raw mode is
disabled. In r570_fbsr_resume(), channel scheduling is resumed in code
preceding this snippet. Resuming channels before restoring raw compression
mode violates LIFO ordering and creates a race condition where channels might
begin processing commands while raw mode is still disabled.

> +		if (ret)
> +			nvkm_error(&gsp->subdev, "Failed to re-enable raw comp mode\n");
> +	}
>  }

[ ... ]

> @@ -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]
Does this code leak hardware state when r570_memsys_enable_raw_comp_mode()
or the subsequent nvkm_instobj_save() loop fails and returns early?

If either fails, the modifications from r570_fbsr_suspend_channels() and
r570_memsys_enable_raw_comp_mode() are not rolled back. The Linux PM
core will abort suspend without calling resume, leaving the GPU permanently
with suspended scheduling and disabled raw compression mode.

>  	/* Save BAR2 allocations to system memory. */
>  	list_for_each_entry(iobj, &imem->list, head) {
>  		if (iobj->preserve) {

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