From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/msm/a6xx: Restore sysprof_active Date: Sun, 12 Apr 2026 08:58:12 +1000 Message-ID: In-Reply-To: <20260411150312.257937-1-robin.clark@oss.qualcomm.com> References: <20260411150312.257937-1-robin.clark@oss.qualcomm.com> <20260411150312.257937-1-robin.clark@oss.qualcomm.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Correctness: Good.** The `.sysprof_setup` field belongs to `struct msm_gp= u_funcs` (defined at `msm_gpu.h:95`), and the assignment is correctly place= d inside the `.base =3D { }` initializer block of both `a6xx_gpu_funcs` and= `a7xx_gpu_funcs`. The function `a6xx_gmu_sysprof_setup()` exists in `a6xx_= gmu.c:2037` and is declared in `a6xx_gpu.h:274`. The call site in `msm_submitqueue.c:44-45`: ```c if (gpu->funcs->sysprof_setup) gpu->funcs->sysprof_setup(gpu); ``` null-checks the pointer before calling, so the omission didn't cause a cras= h =E2=80=94 it silently failed to block IFPC, causing perfcounter corruptio= n during profiling. **Placement within the initializer:** The new line is placed after `.progre= ss` and before the closing `},` of the `.base` block, which is consistent w= ith the existing ordering. Fine. **Scope question:** The patch correctly skips `a6xx_gmuwrapper_funcs`, sinc= e that variant doesn't use the GMU and `a6xx_gmu_sysprof_setup()` accesses = `a6xx_gpu->gmu` internals. However, `a8xx_gpu_funcs` (at `a6xx_gpu.c:2820`) also uses the GMU (`pm_sus= pend =3D a6xx_gmu_pm_suspend`, `pm_resume =3D a6xx_gmu_pm_resume`) and has = the same inline OOB handling in `a8xx_hw_init` (`a8xx_gpu.c:852-856`) that = a6xx has in its hw_init (`a6xx_gpu.c:1610-1614`). The hw_init code only cov= ers the resume/recovery path =E2=80=94 it does not cover the dynamic case w= here userspace starts profiling while the GPU is already running, which is = what `a6xx_gmu_sysprof_setup()` handles. Should `a8xx_gpu_funcs` also get `= .sysprof_setup =3D a6xx_gmu_sysprof_setup`? If a8xx was added after the cat= alogue refactor, this would be a pre-existing gap rather than a regression,= but it seems like the same bug. **Fixes tag:** Correctly references the commit that caused the regression. **Minor nit on subject line:** The subject says "Restore sysprof_active" bu= t what's actually being restored is the `.sysprof_setup` vfunc assignment. = `sysprof_active` is the refcount that the setup function reads =E2=80=94 no= t the thing being restored. Very minor. --- Generated by Claude Code Patch Reviewer