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 <[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_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/[email protected]?part=3
