Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] drm/msm/a6xx: perfcntr select regs lost across IFPC on A630 - [Medium] drm/msm/a6xx: shutdown idle timeout in a6xx_gmu_shutdown() on A630 --
commit e4f747633a2babaf9194dc940b8cefe42bf15c0f Author: Konrad Dybcio <[email protected]> drm/msm/a6xx: Add IFPC tables for Adreno 630 This commit adds the IFPC and power-up register list tables for Adreno 630 and enables ADRENO_QUIRK_IFPC in its catalog entry. > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c > b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c > index 4ef2bd6e7ad26..5e1d91a73a4e1 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c [ ... ] > @@ -1014,7 +1092,8 @@ static const struct adreno_info a6xx_gpus[] = { > }, > .gmem = SZ_1M, > .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT | > - ADRENO_QUIRK_4GB_VA, > + ADRENO_QUIRK_4GB_VA | > + ADRENO_QUIRK_IFPC, [Severity: Medium] Does enabling ADRENO_QUIRK_IFPC on Adreno 630 cause a6xx_gmu_shutdown() to time out waiting for idle when the GPU is runtime-resumed and suspended without running hw_init()? On Adreno 630 (ADRENO_6XX_GEN1), a6xx_gmu_init() sets gmu->legacy = true, and with ADRENO_QUIRK_IFPC set it now also configures gmu->idle_level = GMU_IDLE_STATE_IFPC. When the GPU is resumed via a6xx_gmu_pm_resume() -> a6xx_gmu_resume() -> a6xx_gmu_fw_start(), a6xx_gmu_gfx_rail_on() asserts GMU_OOB_BOOT_SLUMBER: drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a6xx_gmu_gfx_rail_on() { ... /* Let the GMU know the boot sequence has started */ return a6xx_gmu_set_oob(gmu, GMU_OOB_BOOT_SLUMBER); } Normally, GMU_OOB_BOOT_SLUMBER is cleared at the end of hw_init(): drivers/gpu/drm/msm/adreno/a6xx_gpu.c:hw_init() { ... if (a6xx_gpu->gmu.legacy) { /* Take the GMU out of its special boot mode */ a6xx_gmu_clear_oob(&a6xx_gpu->gmu, GMU_OOB_BOOT_SLUMBER); } } However, if the GPU is woken without running msm_gpu_hw_init() (for example, when userspace queries MSM_PARAM_TIMESTAMP in adreno_get_param() or configures sysprof in msm_context_set_sysprof()), adreno_gpu->base.needs_hw_init remains true when runtime suspend calls a6xx_gmu_shutdown(): drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a6xx_gmu_shutdown() { ... if (adreno_gpu->base.needs_hw_init) { if (a6xx_gmu_set_oob(&a6xx_gpu->gmu, GMU_OOB_GPU_SET)) goto force_off; a6xx_gmu_clear_oob(&a6xx_gpu->gmu, GMU_OOB_GPU_SET); } if (test_and_clear_bit(GMU_STATUS_OOB_PERF_SET, &gmu->status)) a6xx_gmu_clear_oob(gmu, GMU_OOB_PERFCOUNTER_SET); ret = a6xx_gmu_wait_for_idle(gmu); if (ret) goto force_off; } Because a6xx_gmu_shutdown() does not clear GMU_OOB_BOOT_SLUMBER when needs_hw_init is true on a legacy GMU, the GX rail stays powered on while a6xx_gmu_wait_for_idle() -> a6xx_gmu_check_idle_level() polls for GMU_IDLE_STATE_IFPC and !adreno_gpu->funcs->gx_is_on(adreno_gpu), which times out and falls back to force_off. > .inactive_period = DRM_MSM_INACTIVE_PERIOD, > .funcs = &a6xx_gpu_funcs, > .zapfw = "a630_zap.mdt", > @@ -1023,6 +1102,8 @@ static const struct adreno_info a6xx_gpus[] = { > .protect = &a630_protect, > .gmu_cgc_mode = 0x00020202, > .prim_fifo_threshold = 0x00180000, > + .ifpc_reglist = &a630_ifpc_reglist, > + .pwrup_reglist = &a630_pwrup_reglist, [Severity: Medium] With IFPC enabled on Adreno 630, will performance counter select registers configured for global performance counter streaming (MSM_PERFCNTR_STREAM) be lost across IFPC power collapse? Commit 632ad5510114 ("drm/msm/a6xx: Restore perfcntr Selects on IFPC") changed msm_gpu_sysprof_no_ifpc() to only check refcount_read(&gpu->sysprof_active) > 1, allowing IFPC while gpu->perfcntrs->stream is active and relying on a6xx_perfcntr_configure() to append the configured select registers to pwrup_reglist so the CP restores them on IFPC exit. However, a6xx_perfcntr_configure() only populates reglist for ADRENO_7XX_GEN1 and newer: drivers/gpu/drm/msm/adreno/a6xx_gpu.c:a6xx_perfcntr_configure() { ... if (to_adreno_gpu(gpu)->info->family >= ADRENO_7XX_GEN1) { WARN_ON(!a6xx_gpu->pwrup_reglist_emitted); struct cpu_gpu_lock *lock = a6xx_gpu->pwrup_reglist_ptr; int off = (2 * lock->ifpc_list_len) + (2 * lock->preemption_list_len) + (3 * a6xx_gpu->dynamic_sel_reglist_offset); reglist = (uint32_t *)&lock->regs[0]; reglist += off; reglist_sel_start = reglist; ... } } Similarly, a6xx_patch_pwrup_reglist() populates the A6xx 2-dword (offset, value) list in lock->list_length without tracking dynamic select register entries. On Adreno 630 (ADRENO_6XX_GEN1), reglist remains NULL in a6xx_perfcntr_configure(), so entering IFPC between sample intervals will reset the PERFCTR_*_SEL registers without restoring them on wakeup. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
