Claude review: drm/atomic: introduce PAGE_FLIP_EVENT property

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

Patch Review

**Bug: Missing reset in `__drm_atomic_helper_crtc_duplicate_state()`**

Looking at `drivers/gpu/drm/drm_atomic_state_helper.c:135-161`, when a new CRTC state is duplicated from the current state, several per-commit fields are explicitly zeroed:

```c
state->event = NULL;
state->async_flip = false;
```

The new `page_flip_event_requested` field is a per-commit property (it always reads as zero, must be explicitly set each commit), but it is **not** cleared in `__drm_atomic_helper_crtc_duplicate_state()`. After `memcpy(state, crtc->state, sizeof(*state))`, if the previous committed state had `page_flip_event_requested = true`, the new state would inherit that value. This would cause spurious page-flip events on subsequent commits that don't set the property.

This is the most important fix needed — add `state->page_flip_event_requested = false;` alongside the other cleared fields.

**Validation of `val != 1` in set_property:**

```c
+	} else if (property == config->prop_page_flip_event) {
+		if (val != 1) {
+			drm_dbg_atomic(crtc->dev,
+				       "[CRTC:%d:%s] PAGE_FLIP_EVENT can only be set to 1",
+				       crtc->base.id, crtc->name);
+			return -EINVAL;
+		}
+		state->page_flip_event_requested = val;
```

This rejects `val == 0`. The property is created as a range `[0, 1]`, so userspace can set it to 0 or 1. Setting it to 0 should be a valid no-op (it's the default). Rejecting 0 means userspace can't explicitly include this property in a commit with value 0, which is inconsistent with how other properties work. Consider:

```c
if (val > 1) {
    ...
    return -EINVAL;
}
state->page_flip_event_requested = val;
```

Or alternatively, if 0 should truly be rejected, the range should be created as `(1, 1)` — but that seems unnecessarily restrictive.

**Interaction between flag and property:**

```c
+		page_flip_event_requested = arg->flags & DRM_MODE_PAGE_FLIP_EVENT ||
+					    crtc_state->page_flip_event_requested;
```

The OR logic means the global flag still works as before, and the per-CRTC property adds granularity on top. This is correct for backwards compatibility. However, what happens if userspace uses *both* the flag and the property? The commit message doesn't discuss this. It works fine because the result is the same (event is requested), but it might be worth documenting that the property takes precedence/supplements the flag.

**Missing `c == 0` check update:**

At the end of `prepare_signaling()`:

```c
if (c == 0 && (arg->flags & DRM_MODE_PAGE_FLIP_EVENT)) {
    drm_dbg_atomic(dev, "need at least one CRTC for DRM_MODE_PAGE_FLIP_EVENT");
    return -EINVAL;
}
```

This check only looks at `arg->flags & DRM_MODE_PAGE_FLIP_EVENT` for the "no CRTCs in commit" error. With the new property, `c` will always be > 0 if any CRTC has `page_flip_event_requested` set (because the CRTC is in the state), so this is probably fine. But it's worth verifying the intent — can userspace set the property on a CRTC that's not otherwise part of the commit? The property setter adds it to the state, so `c` would be incremented, meaning this check is correct.

**Property documentation:**

```c
+ * PAGE_FLIP_EVENT:
+ *	Atomic property for requesting a page-flip event on this CRTC.
+ *
+ *	The value of this property is an integer value which always reads as
+ *	zero and can be written with 1 to request the event.
```

The documentation is clear but could mention the relationship with the existing `DRM_MODE_PAGE_FLIP_EVENT` flag — i.e., that this is a per-CRTC alternative to the global flag, and both can be used together (the result is OR'd).

**Struct member docstring mismatch:**

In `drm_mode_config.h`:

```c
+	/**
+	 * @page_flip_event_property: property to request page-flip events.
+	 */
+	struct drm_property *prop_page_flip_event;
```

The doc name `@page_flip_event_property` doesn't match the field name `prop_page_flip_event`. It should be `@prop_page_flip_event`.

**Overall:** A clean and well-motivated patch. The main issue to fix before merging is the missing state reset in `__drm_atomic_helper_crtc_duplicate_state()`. The val==0 rejection is worth reconsidering for consistency with other range properties. The docstring field name mismatch is minor but should be fixed.

---
Generated by Claude Code Patch Reviewer