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: Fix the bogus protect error on X2-85 Date: Fri, 27 Feb 2026 13:55:25 +1000 Message-ID: In-Reply-To: <20260225-glymur-protect-fix-v1-1-0deddedf9277@oss.qualcomm.com> References: <20260225-glymur-protect-fix-v1-1-0deddedf9277@oss.qualcomm.com> <20260225-glymur-protect-fix-v1-1-0deddedf9277@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 **The `count_max` fix (correct):** ```c -DECLARE_ADRENO_PROTECT(x285_protect, 64); +DECLARE_ADRENO_PROTECT(x285_protect, 15); ``` This is correct. Looking at `a8xx_set_cp_protect()` in `a8xx_gpu.c:246-259`: ```c for (i =3D 0; i < protect->count; i++) { if (protect->regs[i]) { gpu_write(gpu, REG_A8XX_CP_PROTECT_GLOBAL(i), protect->regs[i]); final_cfg =3D protect->regs[i]; } } a8xx_write_pipe(gpu, PIPE_BR, REG_A8XX_CP_PROTECT_PIPE(protect->count_max),= final_cfg); a8xx_write_pipe(gpu, PIPE_BV, REG_A8XX_CP_PROTECT_PIPE(protect->count_max),= final_cfg); ``` For a8xx, all entries go into **Global** protect registers (no count limit = enforced by `count_max`), and `count_max` is used solely as the index into = the **Pipe-specific** protect registers for the infinite-range entry. Setti= ng it to 15 makes x285 consistent with the a840 configuration (`DECLARE_ADR= ENO_PROTECT(a840_protect, 15)` at `a6xx_catalog.c:1883`), which is the corr= ect last pipe protect register index for this hardware generation. **The BUILD_BUG_ON removal (necessary but deserves comment):** ```c - BUILD_BUG_ON(a840_protect.count > a840_protect.count_max); ``` This removal is necessary because the `count > count_max` check is meaningl= ess for a8xx =E2=80=94 `count` is the number of **global** protect entries = (54 for a840, 52 for x285) while `count_max` is the **pipe** register index= (15). These are different address spaces and there's no reason `count` mus= t be <=3D `count_max`. **Minor observations / suggestions:** 1. **Missing replacement sanity check**: The patch removes the only compile= -time check for a8xx protect configurations without adding an alternative. = It would be valuable to add a check that `count_max` doesn't exceed the har= dware's actual pipe protect register count, or at minimum a comment explain= ing why the a8xx entries are intentionally excluded from the `__build_asser= ts()` checks. 2. **The `count_max` field is semantically overloaded**: For a6xx GPUs, `co= unt_max` means "total number of protect registers" (used as `count_max - 1`= for the last index). For a8xx GPUs, `count_max` is the direct pipe registe= r index for the infinite-range entry. This dual meaning is confusing and co= ntributed to the original bug. Consider renaming the field or adding a code= comment in `adreno_gpu.h` near the `struct adreno_protect` definition docu= menting this distinction. This isn't something the patch needs to fix, but = it's worth noting. 3. **Commit message nit**: The commit message says "remove the A840 entry f= rom there" which might confuse reviewers into thinking the a840 protect con= figuration itself is being removed. A clearer phrasing might be: "remove th= e A840 BUILD_BUG_ON check, which is invalid for a8xx since count_max repres= ents the pipe register index, not the global register count." 4. **No BUILD_BUG_ON was ever added for x285_protect**: The original commit= `01ff3bf27215` added x285 with `count_max=3D64` but didn't add a correspon= ding `BUILD_BUG_ON(x285_protect.count > x285_protect.count_max)`. If it had= , the bug would have been hidden (52 < 64 passes), so this is moot =E2=80= =94 but it's worth noting that even the existing `BUILD_BUG_ON` for `a840_p= rotect` (with count=3D54 > count_max=3D15) appears to not actually fire at = compile time, likely because struct member access through a `static const` = variable isn't treated as a constant expression by the compiler. This means= these checks may have been silently no-ops for a8xx all along. Overall: the fix is correct and should be applied. The suggestions above ar= e minor and could be addressed as follow-ups. --- Generated by Claude Code Patch Reviewer