Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] drm/msm/a8xx: a8xx_nonctxt_config() clobbers LLCC SCID in GBIF_SCACHE_CNTL0 - [Medium] drm/msm/a8xx: nonctxt_reglist clobbers CX GBIF config in a8xx_nonctxt_config() --
commit 966abf648e13ba76b4165851da9f6e5975e5c769 Author: Rob Clark <[email protected]> drm/msm/a8xx: Switch to generated nonctx regs This commit removes the hand-maintained non-context register tables in drivers/gpu/drm/msm/adreno/a8xx_catalog.c and switches the A8xx GPU catalog entries to use the generated non-context register lists from the catalog headers. > diff --git a/drivers/gpu/drm/msm/adreno/a8xx_catalog.c > b/drivers/gpu/drm/msm/adreno/a8xx_catalog.c > index 15fc10d5db688..bc79013165c13 100644 > --- a/drivers/gpu/drm/msm/adreno/a8xx_catalog.c > +++ b/drivers/gpu/drm/msm/adreno/a8xx_catalog.c [ ... ] > @@ -1556,7 +1216,7 @@ static const struct adreno_info a8xx_gpus[] = { > .funcs = &a8xx_gpu_funcs, > .a6xx = &(const struct a6xx_info) { > .protect = &a830_protect, > - .nonctxt_reglist = a830_nonctxt_regs, > + .nonctxt_reglist = pakala_nonctxt_regs, > .pwrup_reglist = &a830_pwrup_reglist, > .dyn_pwrup_reglist = &a830_dyn_pwrup_reglist, > .ifpc_reglist = &a830_ifpc_reglist, [Severity: Medium] Will switching .nonctxt_reglist in a8xx_gpus to pakala_nonctxt_regs (and the other generated *_nonctxt_regs tables) cause a8xx_nonctxt_config() to clobber the LLCC slice IDs in REG_A6XX_GBIF_SCACHE_CNTL0? In drivers/gpu/drm/msm/adreno/catalog/adreno_pakala.h (as well as the glymur_nonctxt_regs, kaanapali_nonctxt_regs, milos_nonctxt_regs, maili_nonctxt_regs, and hawi_nonctxt_regs tables), the generated list includes: { 0x00003c01 /* GBIF_SCACHE_CNTL0 */, 0x00000100, BIT(PIPE_NONE) }, On initial GPU load (adreno_load_gpu()), job submission after runtime suspend (msm_gpu_submit()), or recovery (a8xx_recover()), pm_runtime_get_sync() runs adreno_runtime_resume() -> a6xx_gmu_pm_resume() in drivers/gpu/drm/msm/adreno/a6xx_gpu.c: gpu->needs_hw_init = true; ... if (adreno_is_a8xx(adreno_gpu)) a8xx_llc_activate(a6xx_gpu); In drivers/gpu/drm/msm/adreno/a8xx_gpu.c, a8xx_llc_activate() programs the system cache slice ID into REG_A6XX_GBIF_SCACHE_CNTL0 (0x3c01): gpu_write(gpu, REG_A6XX_GBIF_SCACHE_CNTL0, FIELD_PREP(GENMASK(27, 22), gpu_scid) | FIELD_PREP(GENMASK(21, 16), gpu_scid) | FIELD_PREP(GENMASK(15, 10), gpu_scid) | BIT(8)); Immediately afterwards, msm_gpu_hw_init() sees gpu->needs_hw_init == true and calls a8xx_hw_init() -> hw_init() -> a8xx_nonctxt_config(), which executes: gpu_write(gpu, regs[i].offset, regs[i].value); Because 0x00003c01 is in info->nonctxt_reglist with value 0x00000100, wouldn't this overwrite REG_A6XX_GBIF_SCACHE_CNTL0 and zero out the gpu_scid fields in bits [27:10] on every hardware init? > @@ -1601,7 +1261,7 @@ static const struct adreno_info a8xx_gpus[] = { > .funcs = &a8xx_gpu_funcs, > .a6xx = &(const struct a6xx_info) { > .protect = &x285_protect, > - .nonctxt_reglist = x285_nonctxt_regs, > + .nonctxt_reglist = glymur_nonctxt_regs, > .pwrup_reglist = &a840_pwrup_reglist, > .dyn_pwrup_reglist = &x285_dyn_pwrup_reglist, > .ifpc_reglist = &a840_ifpc_reglist, > @@ -1646,7 +1306,7 @@ static const struct adreno_info a8xx_gpus[] = { > .funcs = &a8xx_gpu_funcs, > .a6xx = &(const struct a6xx_info) { > .protect = &a840_protect, > - .nonctxt_reglist = a840_nonctxt_regs, > + .nonctxt_reglist = kaanapali_nonctxt_regs, > .pwrup_reglist = &a840_pwrup_reglist, > .dyn_pwrup_reglist = &a840_dyn_pwrup_reglist, > .ifpc_reglist = &a840_ifpc_reglist, > @@ -1692,7 +1352,7 @@ static const struct adreno_info a8xx_gpus[] = { > .zapfw = "gen80300_zap.mbn", > .a6xx = &(const struct a6xx_info) { > .protect = &a810_protect, > - .nonctxt_reglist = a810_nonctxt_regs, > + .nonctxt_reglist = milos_nonctxt_regs, > .pwrup_reglist = &a810_pwrup_reglist, > .dyn_pwrup_reglist = &a810_dyn_pwrup_reglist, > .ifpc_reglist = &a810_ifpc_reglist, > @@ -1735,7 +1395,7 @@ static const struct adreno_info a8xx_gpus[] = { > .funcs = &a8xx_gpu_funcs, > .a6xx = &(const struct a6xx_info) { > .protect = &x285_protect, > - .nonctxt_reglist = x285_nonctxt_regs, > + .nonctxt_reglist = glymur_nonctxt_regs, > .pwrup_reglist = &a840_pwrup_reglist, > .dyn_pwrup_reglist = &x285_dyn_pwrup_reglist, > .ifpc_reglist = &a840_ifpc_reglist, > @@ -1778,7 +1438,7 @@ static const struct adreno_info a8xx_gpus[] = { > .funcs = &a8xx_gpu_funcs, > .a6xx = &(const struct a6xx_info) { > .protect = &a850_protect, > - .nonctxt_reglist = a845_nonctxt_regs, > + .nonctxt_reglist = maili_nonctxt_regs, > .pwrup_reglist = &a850_pwrup_reglist, > .dyn_pwrup_reglist = &a850_dyn_pwrup_reglist, > .ifpc_reglist = &a850_ifpc_reglist, > @@ -1822,7 +1482,7 @@ static const struct adreno_info a8xx_gpus[] = { > .funcs = &a8xx_gpu_funcs, > .a6xx = &(const struct a6xx_info) { > .protect = &a850_protect, > - .nonctxt_reglist = a850_nonctxt_regs, > + .nonctxt_reglist = hawi_nonctxt_regs, > .pwrup_reglist = &a850_pwrup_reglist, > .dyn_pwrup_reglist = &a850_dyn_pwrup_reglist, > .ifpc_reglist = &a850_ifpc_reglist, [Severity: Medium] Should the CX GBIF registers (0x3c00..0x3c45) in the generated *_nonctxt_regs tables also be excluded or handled separately in a8xx_nonctxt_config()? Unlike the previous hand-maintained nonctxt_regs arrays in drivers/gpu/drm/msm/adreno/a8xx_catalog.c, all six generated *_nonctxt_regs tables include CX GBIF entries: { 0x00003c00 /* GBIF_CX_CONFIG */, 0x20023020, BIT(PIPE_NONE) }, { 0x00003c01 /* GBIF_SCACHE_CNTL0 */, 0x00000100, BIT(PIPE_NONE) }, { 0x00003c03 /* GBIF_QSB_SIDE0 */, 0x00071e20, BIT(PIPE_NONE) }, { 0x00003c04 /* GBIF_QSB_SIDE1 */, 0x00071e20, BIT(PIPE_NONE) }, { 0x00003c05 /* GBIF_QSB_SIDE2 */, 0x00071e20, BIT(PIPE_NONE) }, { 0x00003c06 /* GBIF_QSB_SIDE3 */, 0x00071e20, BIT(PIPE_NONE) }, { 0x00003c45 /* GBIF_HALT */, 0x00000008, BIT(PIPE_NONE) }, During resume, a6xx_gmu_pm_resume() -> a6xx_gmu_resume() -> a6xx_gmu_fw_start() configures gbif_cx and boots the GMU (which enables clock gating in REG_A6XX_GBIF_CX_CONFIG BIT(0), as noted in a8xx_set_hwcg() in drivers/gpu/drm/msm/adreno/a8xx_gpu.c): /* * GMU enables clk gating in GBIF during boot up. So, * override that here when hwcg feature is disabled */ gpu_rmw(gpu, REG_A6XX_GBIF_CX_CONFIG, BIT(0), 0); Then in drivers/gpu/drm/msm/adreno/a8xx_gpu.c, hw_init() explicitly clears REG_A6XX_GBIF_HALT before calling a8xx_nonctxt_config(): /* Clear GBIF halt in case GX domain was not collapsed */ gpu_write(gpu, REG_A6XX_GBIF_HALT, 0); gpu_read(gpu, REG_A6XX_GBIF_HALT); ... a8xx_nonctxt_config(gpu); When a8xx_nonctxt_config() subsequently writes the generated nonctxt_reglist, could writing 0x20023020 to REG_A6XX_GBIF_CX_CONFIG (0x3c00) clear BIT(0) that the GMU enabled on boot, and could writing 0x00000008 to REG_A6XX_GBIF_HALT (0x3c45) undo the earlier clear in hw_init() in addition to overwriting REG_A6XX_GBIF_SCACHE_CNTL0 (0x3c01)? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=10
