Claude review: drm/amdgpu: Replace VKMS vblank timer with common implementation

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

Patch Review

**Correctness of the macro substitutions:**

The `DRM_CRTC_VBLANK_TIMER_FUNCS` macro expands to:
```c
.enable_vblank = drm_crtc_vblank_helper_enable_vblank_timer,
.disable_vblank = drm_crtc_vblank_helper_disable_vblank_timer,
.get_vblank_timestamp = drm_crtc_vblank_helper_get_vblank_timestamp_from_timer
```
These replace the removed `amdgpu_vkms_enable_vblank`, `amdgpu_vkms_disable_vblank`, and `amdgpu_vkms_get_vblank_timestamp` respectively. Functionally equivalent — the shared helpers call `drm_crtc_vblank_start_timer()`, `drm_crtc_vblank_cancel_timer()`, and `drm_crtc_vblank_get_vblank_timeout()` which implement the same hrtimer logic, but with the timer state stored in `struct drm_vblank_crtc_timer` (inside `drm_vblank_crtc`) rather than in `amdgpu_crtc`.

The `DRM_CRTC_HELPER_VBLANK_FUNCS` macro expands to:
```c
.atomic_flush = drm_crtc_vblank_atomic_flush,
.atomic_enable = drm_crtc_vblank_atomic_enable,
.atomic_disable = drm_crtc_vblank_atomic_disable
```
These replace the three removed `amdgpu_vkms_crtc_atomic_*` functions. The logic is equivalent — `drm_crtc_vblank_on()`/`drm_crtc_vblank_off()` for enable/disable, and the arm-or-send-event pattern for flush.

**Improvements in the shared code over the old amdgpu code:**

1. **Deadlock avoidance in cancel:** The old code used `hrtimer_try_to_cancel()` in `disable_vblank` and `hrtimer_cancel()` in `sw_fini`. The shared `drm_crtc_vblank_cancel_timer()` explicitly avoids `hrtimer_cancel()` due to potential deadlocks between `vblank_time_lock` and `softirq_expiry_lock`, instead zeroing the interval and using `hrtimer_try_to_cancel()`. This is the "deadlocks and race conditions" the commit message references.

2. **Timestamp race protection:** The old `amdgpu_vkms_get_vblank_timestamp()` does a single `READ_ONCE()` of the timer expiry, then a simple `WARN_ON` comparison against `vblank->time`. The shared `drm_crtc_vblank_get_vblank_timeout()` uses a retry loop with `drm_crtc_vblank_count_and_time()` to protect against concurrent vblank timeouts updating the expiry between reads.

3. **atomic_flush locking:** The old code used `spin_lock_irqsave`/`spin_unlock_irqrestore` and cleared `crtc->state->event = NULL` outside the lock. The shared code uses `spin_lock_irq`/`spin_unlock_irq` (appropriate since atomic_flush runs in process context) and clears `crtc_state->event = NULL` inside the lock. The shared code also properly fetches `crtc_state` via `drm_atomic_get_new_crtc_state()` rather than directly accessing `crtc->state`.

**hrtimer init/fini removal:**

The removal of `hrtimer_setup()` from `amdgpu_vkms_crtc_init` and `hrtimer_cancel()` loop from `amdgpu_vkms_sw_fini` is correct — the shared implementation initializes the timer lazily on first `drm_crtc_vblank_start_timer()` call, and cleanup is handled by the vblank core.

**Minor nit — leftover dead fields in `amdgpu_vkms_output`:**

After this patch, the `period_ns` and `event` fields in `struct amdgpu_vkms_output` (in `amdgpu_vkms.h:21-22`) become completely unused. All three call sites that referenced `period_ns` via `drm_crtc_to_amdgpu_vkms_output()` are in the removed functions. The `event` field was already unused before this patch (the flush function used `crtc->state->event`, not `output->event`). The `drm_crtc_to_amdgpu_vkms_output` macro in `amdgpu_vkms.h:12-13` also loses all callers in `amdgpu_vkms.c` (its three uses at lines 49, 71, 94 are all in removed functions), though it is still used implicitly by `amdgpu_vkms_output_init()` which takes the struct by pointer. Consider cleaning up `period_ns` and `event` from the struct in this patch since they're directly related to the removed code.

**Reviewed-by assessment:** Patch is correct and safe to apply. The dead field cleanup is a minor nice-to-have, not a blocker.

---
Generated by Claude Code Patch Reviewer