Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/msm/a6xx: missing IFPC CP_SET_MARKER packets in a6xx_submit() - [Medium] drm/msm/a6xx: perfcntr SELECT regs lost on IFPC in a6xx_perfcntr_configure() --
commit bfad38c351383c6a537cd26f28ef21f13067df68 Author: Konrad Dybcio <[email protected]> drm/msm/a6xx: Add A6xx IFPC support This commit adds A6xx-specific IFPC plumbing, including GMU WFI configuration, power-up register list patching, CP_ME_INIT register list programming, and skipping the A7xx HFI IFPC enable message on A6xx. > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > index e7c3da5d3ac64..8ea9ab5b98861 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c [ ... ] > @@ -878,6 +887,43 @@ static void a7xx_patch_pwrup_reglist(struct msm_gpu *gpu) > a6xx_gpu->dynamic_sel_reglist_offset = dyn_pwrup_reglist_count; > } > > +static void a6xx_patch_pwrup_reglist(struct msm_gpu *gpu) > +{ > + struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); > + struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu); > + const struct adreno_reglist_list *reglist; > + void *ptr = a6xx_gpu->pwrup_reglist_ptr; > + struct cpu_gpu_lock *lock = ptr; > + u32 *dest = (u32 *)&lock->regs[0]; > + int i; > + > + lock->gpu_req = lock->cpu_req = lock->turn = 0; > + lock->list_length = 0; > + > + /* Static IFPC-only registers */ > + reglist = adreno_gpu->info->a6xx->ifpc_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; > + } > + > + /* The CP sees IFPC-only and IFPC + preemption as one list from > list_offset */ > + lock->list_offset = lock->list_length; > + > + /* Static IFPC + preemption registers */ > + 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; > + } > +} [Severity: Medium] When a performance counter stream is configured via DRM_IOCTL_MSM_PERFCNTR_CONFIG, msm_gpu_sysprof_no_ifpc() allows the GPU to enter IFPC between samples under the assumption that a6xx_perfcntr_configure() appends the configured SELECT registers to pwrup_reglist_ptr. However, a6xx_perfcntr_configure() only populates pwrup_reglist_ptr when info->family >= ADRENO_7XX_GEN1: 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); ... On A6xx, a6xx_patch_pwrup_reglist() uses the A6xx cpu_gpu_lock layout (lock->list_length and lock->list_offset with register/value pairs) without recording the base static offset in dynamic_sel_reglist_offset, and a6xx_perfcntr_configure() leaves reglist as NULL. Will entering IFPC on A6xx while a performance counter stream is active lose the GX-domain performance counter SELECT registers when the GX power domain collapses? [ ... ] > @@ -1485,8 +1564,12 @@ static int hw_init(struct msm_gpu *gpu) > /* Enable the SQE_to start the CP engine */ > gpu_write(gpu, REG_A6XX_CP_SQE_CNTL, 1); > > - if (adreno_is_a7xx(adreno_gpu) && !a6xx_gpu->pwrup_reglist_emitted) { > - a7xx_patch_pwrup_reglist(gpu); > + if (!a6xx_gpu->pwrup_reglist_emitted) { > + if (adreno_is_a7xx(adreno_gpu)) > + a7xx_patch_pwrup_reglist(gpu); > + else > + a6xx_patch_pwrup_reglist(gpu); > + > a6xx_gpu->pwrup_reglist_emitted = true; > } [Severity: High] In a7xx_submit(), the driver emits CP_SET_MARKER packets around command submissions to disable IFPC before indirect buffers execute and re-enable it after completion: drivers/gpu/drm/msm/adreno/a6xx_gpu.c:a7xx_submit() { ... OUT_PKT7(ring, CP_SET_MARKER, 1); OUT_RING(ring, 0x101); /* IFPC disable */ ... OUT_PKT7(ring, CP_SET_MARKER, 1); OUT_RING(ring, 0x100); /* IFPC enable */ } However, a6xx_submit() in drivers/gpu/drm/msm/adreno/a6xx_gpu.c does not emit these CP_SET_MARKER packets before the indirect buffer loop or after the CACHE_FLUSH_TS event write. Should a6xx_submit() also emit CP_SET_MARKER(0x101) and CP_SET_MARKER(0x100) so that REG_A6XX_CP_CP2GMU_STATUS properly tracks IFPC eligibility across submissions on A6xx? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
