Claude review: drm/appletbdrm: Allocate request/response buffers in begin_fb_access

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

Patch Review

This is the most interesting patch in the series. It moves damage-dependent buffer allocation from `atomic_check` (where damage state isn't finalized) to `begin_fb_access` (which runs during commit, after damage is fully resolved).

The refactoring is clean -- `atomic_check` is simplified to just the standard `drm_atomic_helper_check_plane_state()` call, while buffer allocation moves to the new `begin_fb_access` wrapper that calls `drm_gem_begin_shadow_fb_access()` at the end.

Key change: `DRM_GEM_SHADOW_PLANE_HELPER_FUNCS` is expanded to its individual components:
```c
-	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
+	.begin_fb_access = appletbdrm_primary_plane_helper_begin_fb_access,
+	.end_fb_access = drm_gem_end_shadow_fb_access,
```

The `drm_atomic_helper_damage_iter_init` call now passes NULL for `old_state`:
```c
-	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, new_plane_state);
+	drm_atomic_helper_damage_iter_init(&iter, NULL, new_plane_state);
```

This is safe because `old_state` is only used in the current code for `drm_rect_equals(&state->src, &old_state->src)` check, and at this point in the series the `ignore_damage_clips` flag from patch 1 covers the modeset case. However, the src-coordinate comparison is **still** live in the `drm_atomic_helper_damage_iter_init()` code at this point in the series -- it's only removed in patch 6. So passing NULL here would crash on the `!drm_rect_equals(&state->src, &old_state->src)` line if `iter->clips` is non-NULL and `ignore_damage_clips` is false (since it would dereference `old_state` which is NULL).

**Potential issue:** In the current tree, `drm_atomic_helper_damage_iter_init()` at line 247 does `!drm_rect_equals(&state->src, &old_state->src)` which would dereference the NULL `old_state`. This is guarded by the short-circuit `!iter->clips || state->ignore_damage_clips ||`, so it's only reached if clips exist AND `ignore_damage_clips` is false. Since `begin_fb_access` runs during the commit phase (after the check phase where `ignore_damage_clips` gets set for modesets), there's a narrow window where clips exist, no modeset was triggered, but src coords changed. In that case, passing NULL would cause a NULL pointer dereference. Patch 6 fixes this by moving the src check into the check phase, but between patches 4 and 6 the appletbdrm code is unsafe.

Wait -- re-examining: the series was reordered in v4 specifically "to avoid error-prone intermediate state." Patches 4 comes before patch 5 (which removes the pre-check call) and patch 6 (which adds the src coord check to `drm_atomic_helper_check_plane_damage`). So between patches 4 and 5, the pre-check call at line 1037 still runs and sets `ignore_damage_clips` for modesets, but **not** for src coord changes. The src coord change detection only happens inside `drm_atomic_helper_damage_iter_init()` via the `old_state` param. So passing NULL for `old_state` in appletbdrm's `begin_fb_access` is **potentially unsafe** if src coords change without a modeset.

This is a **real bisectability concern** -- if someone bisects to a point between patches 4 and 6, the appletbdrm driver could NULL-deref on a src-coordinate change. In practice, this is unlikely to be hit since appletbdrm is a Touch Bar display driver with fixed src coords, but it's still technically a bug window.

---
Generated by Claude Code Patch Reviewer