Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/msm/a6xx: missing revn 618 on sm7150 and revn 615 in adreno_is_a615_family() - [Medium] drm/msm/a6xx: QoS register mismatch between a640_pwrup_reglist and hw_init() --
commit c21799f522c1e10807d8ac16ef0e01ab2fd9a092 Author: Konrad Dybcio <[email protected]> drm/msm/a6xx: Wire up IFPC on A615-family GPUs This commit enables ADRENO_QUIRK_IFPC and wires up a630_ifpc_reglist and a640_pwrup_reglist for the A615-family entries in a6xx_gpus[]. > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c > b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c > index 2d59f7735c907..257771d4eb4d5 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c > @@ -1012,7 +1012,8 @@ static const struct adreno_info a6xx_gpus[] = { > [ADRENO_FW_GMU] = "a630_gmu.bin", > }, > .gmem = SZ_512K, > - .quirks = ADRENO_QUIRK_4GB_VA, > + .quirks = ADRENO_QUIRK_4GB_VA | > + ADRENO_QUIRK_IFPC, > .inactive_period = DRM_MSM_INACTIVE_PERIOD, > .funcs = &a6xx_gpu_funcs, > .zapfw = "a615_zap.mdt", > @@ -1021,6 +1022,8 @@ static const struct adreno_info a6xx_gpus[] = { > .protect = &a630_protect, > .gmu_cgc_mode = 0x00000222, > .prim_fifo_threshold = 0x0018000, > + .ifpc_reglist = &a630_ifpc_reglist, > + .pwrup_reglist = &a640_pwrup_reglist, [Severity: Medium] Should the A615-family entries in a6xx_gpus[] either use a630_pwrup_reglist or have hw_init() updated to program REG_A6XX_RBBM_GBIF_CLIENT_QOS_CNTL for this family? In drivers/gpu/drm/msm/adreno/a6xx_gpu.c:hw_init(), all six A615-family entries have .family = ADRENO_6XX_GEN1 and fall into the final else branch, which writes 0x3 to REG_A6XX_RBBM_VBIF_CLIENT_QOS_CNTL and leaves REG_A6XX_RBBM_GBIF_CLIENT_QOS_CNTL unprogrammed: drivers/gpu/drm/msm/adreno/a6xx_gpu.c:hw_init() { ... if (adreno_is_a610_family(adreno_gpu) || adreno_is_a640_family(adreno_gpu) || adreno_is_a650_family(adreno_gpu)) { gpu_write(gpu, REG_A6XX_RBBM_GBIF_CLIENT_QOS_CNTL, 0x3); } else if (adreno_is_a7xx(adreno_gpu)) { gpu_write(gpu, REG_A6XX_RBBM_GBIF_CLIENT_QOS_CNTL, 0x2120212); } else { gpu_write(gpu, REG_A6XX_RBBM_VBIF_CLIENT_QOS_CNTL, 0x3); } ... } Later in hw_init(), a6xx_patch_pwrup_reglist() snapshots the current values of the registers listed in info->a6xx->pwrup_reglist via gpu_read() so the CP can restore them after each IFPC GX power collapse: drivers/gpu/drm/msm/adreno/a6xx_gpu.c:a6xx_patch_pwrup_reglist() { ... reglist = adreno_gpu->info->a6xx->pwrup_reglist; if (reglist) { for (i = 0; i < reglist->count; i++) { *dest++ = reglist->regs[i]; *dest++ = gpu_read(gpu, reglist->regs[i]); } lock->list_length += reglist->count * 2; } ... } Because a640_pwrup_reglist_regs lists REG_A6XX_RBBM_GBIF_CLIENT_QOS_CNTL rather than REG_A6XX_RBBM_VBIF_CLIENT_QOS_CNTL (which is the only difference between a640_pwrup_reglist and a630_pwrup_reglist), won't every IFPC cycle lose the programmed REG_A6XX_RBBM_VBIF_CLIENT_QOS_CNTL value and replay the uninitialized REG_A6XX_RBBM_GBIF_CLIENT_QOS_CNTL register instead? > }, > .speedbins = ADRENO_SPEEDBINS( > /* > @@ -1044,7 +1047,8 @@ static const struct adreno_info a6xx_gpus[] = { > .gmem = SZ_512K, > .inactive_period = DRM_MSM_INACTIVE_PERIOD, > .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT | > - ADRENO_QUIRK_4GB_VA, > + ADRENO_QUIRK_4GB_VA | > + ADRENO_QUIRK_IFPC, [Severity: High] Does enabling ADRENO_QUIRK_IFPC on the qcom,sm7150 (0x06010800) and A615 (0x06010500) entries cause them to run IFPC with A630 PDC and GMU clock gating settings instead of A618/A615-family settings? The qcom,sm7150 entry in drivers/gpu/drm/msm/adreno/a6xx_catalog.c omits .revn = 618, unlike the generic 0x06010800 entry right below it. In addition, adreno_is_a615_family() in drivers/gpu/drm/msm/adreno/adreno_gpu.h does not check for revn 615: drivers/gpu/drm/msm/adreno/adreno_gpu.h: /* TODO: 615/616 */ static inline int adreno_is_a615_family(const struct adreno_gpu *gpu) { return adreno_is_a618(gpu) || adreno_is_a619(gpu); } Because adreno_is_a618() checks adreno_is_revn(gpu, 618), both adreno_is_a618() and adreno_is_a615_family() evaluate to false on qcom,sm7150. In drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a6xx_gmu_rpmh_init(), this causes the driver to program A630 PDC sleep/wakeup values instead of A618 values: drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a6xx_gmu_rpmh_init() { ... else if (adreno_is_a618(adreno_gpu) || adreno_is_a640_family(adreno_gpu)) pdc_address_offset = 0x30090; else if (adreno_is_a619(adreno_gpu)) pdc_address_offset = 0x300a0; else pdc_address_offset = 0x30080; ... if (adreno_is_a618(adreno_gpu) || adreno_is_a619(adreno_gpu) || adreno_is_a650_family(adreno_gpu)) pdc_write(pdcptr, REG_A6XX_PDC_GPU_TCS3_CMD0_DATA + 4, 0x2); else pdc_write(pdcptr, REG_A6XX_PDC_GPU_TCS3_CMD0_DATA + 4, 0x3); ... } With ADRENO_QUIRK_IFPC setting gmu->idle_level = GMU_IDLE_STATE_IFPC, the GMU will execute those PDC sequences on every IFPC power collapse. Missing revn 618 on qcom,sm7150 and revn 615 in adreno_is_a615_family() also causes a6xx_set_hwcg() in drivers/gpu/drm/msm/adreno/a6xx_gpu.c to program A630 GMU CGC delay and hysteresis values (0x10111 / 0x5555) instead of A615-family values (0x111 / 0x555), and causes a6xx_hfi_send_bw_table() to skip a618_build_bw_table() on qcom,sm7150. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=8
