On Fri, 2026-09-11 at 22:37 +0000, [email protected] wrote: > 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.
Wrong piece of feedback, but there was an issue here. > > 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 <[email protected]> > > 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_P > > ARAMS *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?
