Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Logic error and evaluation order race in damage clearing for joiner secondary planes. - [Low] Build failure due to undeclared identifier and invalid member access in plane_atomic_check(). --
--- Patch [1]: [PATCH v2] drm/i915/display: Handle struct drm_plane_state.ignore_damage_clips --- commit afbed1d4a86380370d4541c25d1804c472867576 Author: Thomas Zimmermann <[email protected]> drm/i915/display: Handle struct drm_plane_state.ignore_damage_clips Calling drm_atomic_helper_check_plane_damage() in plane_atomic_check() before copying the damage information guarantees that ignore_damage_clips from struct drm_plane_state has the correct value. Although i915 does not directly modify or use the flag, DRM's damage iterator will soon rely on it. Link: https://lore.kernel.org/dri-devel/[email protected]/ > diff --git a/drivers/gpu/drm/i915/display/intel_plane.c > b/drivers/gpu/drm/i915/display/intel_plane.c > index d0f99a87c42e8..ad7c5adc32cb0 100644 > --- a/drivers/gpu/drm/i915/display/intel_plane.c > +++ b/drivers/gpu/drm/i915/display/intel_plane.c [ ... ] > @@ -928,6 +928,14 @@ static int plane_atomic_check(struct intel_atomic_state > *state, > old_primary_crtc_plane_state = old_plane_state; > } > > + /* > + * Prepare plane-damage state before using it; do this on all > + * display versions to keep the underlying DRM plane state in > + * good shape. > + */ > + drm_atomic_helper_check_plane_damage(&new_plane_state->base, > + &new_uapi_plane_state->uapi); [Severity: Low] Is there a typo in the variable names here that causes a build failure? Looking at struct intel_plane_state, it doesn't contain a base member, and new_uapi_plane_state is undeclared in this function scope. Did you mean to pass &state->base and &new_primary_crtc_plane_state->uapi instead? [Severity: High] If this is naively fixed by passing the local plane state (&new_plane_state->uapi), does this introduce a logic error for joiner secondary planes during modesets? The code would clear the secondary plane's damage, but the immediately following call to intel_plane_copy_uapi_plane_damage() would overwrite this by copying damage from the primary CRTC's plane state (new_primary_crtc_plane_state). If the primary plane has not yet been processed by the unordered atomic_check loop, its damage will remain uncleared. The secondary plane would thus incorrectly copy partial, dirty damage instead of executing a full update. Should this explicitly evaluate the primary plane's state by passing &new_primary_crtc_plane_state->uapi instead? > + > intel_plane_copy_uapi_plane_damage(new_plane_state, > old_primary_crtc_plane_state, > new_primary_crtc_plane_state); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
