Claude review: drm/msm/a6xx: Fix the bogus protect error on X2-85

[Claude Code Review Bot <claude-review@example.com>]

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 = 0; i < protect->count; i++) {
    if (protect->regs[i]) {
        gpu_write(gpu, REG_A8XX_CP_PROTECT_GLOBAL(i), protect->regs[i]);
        final_cfg = 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. Setting it to 15 makes x285 consistent with the a840 configuration (`DECLARE_ADRENO_PROTECT(a840_protect, 15)` at `a6xx_catalog.c:1883`), which is the correct 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 meaningless for a8xx — `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` must be <= `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 hardware's actual pipe protect register count, or at minimum a comment explaining why the a8xx entries are intentionally excluded from the `__build_asserts()` checks.

2. **The `count_max` field is semantically overloaded**: For a6xx GPUs, `count_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 register index for the infinite-range entry. This dual meaning is confusing and contributed to the original bug. Consider renaming the field or adding a code comment in `adreno_gpu.h` near the `struct adreno_protect` definition documenting 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 from there" which might confuse reviewers into thinking the a840 protect configuration itself is being removed. A clearer phrasing might be: "remove the A840 BUILD_BUG_ON check, which is invalid for a8xx since count_max represents 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=64` but didn't add a corresponding `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 — but it's worth noting that even the existing `BUILD_BUG_ON` for `a840_protect` (with count=54 > count_max=15) 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 are minor and could be addressed as follow-ups.

---
Generated by Claude Code Patch Reviewer