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/atomic: introduce PAGE_FLIP_EVENT property Date: Thu, 04 Jun 2026 15:29:59 +1000 Message-ID: In-Reply-To: <20260530171422.4615-1-contact@emersion.fr> References: <20260530171422.4615-1-contact@emersion.fr> <20260530171422.4615-1-contact@emersion.fr> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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 =3D NULL; state->async_flip =3D false; ``` The new `page_flip_event_requested` field is a per-commit property (it alwa= ys reads as zero, must be explicitly set each commit), but it is **not** cl= eared in `__drm_atomic_helper_crtc_duplicate_state()`. After `memcpy(state,= crtc->state, sizeof(*state))`, if the previous committed state had `page_f= lip_event_requested =3D 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 =E2=80=94 add `state->page_flip_event= _requested =3D false;` alongside the other cleared fields. **Validation of `val !=3D 1` in set_property:** ```c + } else if (property =3D=3D config->prop_page_flip_event) { + if (val !=3D 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 =3D val; ``` This rejects `val =3D=3D 0`. The property is created as a range `[0, 1]`, s= o 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 thi= s property in a commit with value 0, which is inconsistent with how other p= roperties work. Consider: ```c if (val > 1) { ... return -EINVAL; } state->page_flip_event_requested =3D val; ``` Or alternatively, if 0 should truly be rejected, the range should be create= d as `(1, 1)` =E2=80=94 but that seems unnecessarily restrictive. **Interaction between flag and property:** ```c + page_flip_event_requested =3D 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 compatibili= ty. However, what happens if userspace uses *both* the flag and the propert= y? The commit message doesn't discuss this. It works fine because the resul= t is the same (event is requested), but it might be worth documenting that = the property takes precedence/supplements the flag. **Missing `c =3D=3D 0` check update:** At the end of `prepare_signaling()`: ```c if (c =3D=3D 0 && (arg->flags & DRM_MODE_PAGE_FLIP_EVENT)) { drm_dbg_atomic(dev, "need at least one CRTC for DRM_MODE_PAGE_FLIP_EVEN= T"); return -EINVAL; } ``` This check only looks at `arg->flags & DRM_MODE_PAGE_FLIP_EVENT` for the "n= o 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 st= ate), so this is probably fine. But it's worth verifying the intent =E2=80= =94 can userspace set the property on a CRTC that's not otherwise part of t= he commit? The property setter adds it to the state, so `c` would be increm= ented, 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 exis= ting `DRM_MODE_PAGE_FLIP_EVENT` flag =E2=80=94 i.e., that this is a per-CRT= C 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=3D=3D0 rejection is worth reconsidering for consistency w= ith other range properties. The docstring field name mismatch is minor but = should be fixed. --- Generated by Claude Code Patch Reviewer