Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [Medium] Incomplete refactoring: drm_atomic_helper_duplicate_state still calls drm_atomic_add_affected_colorops. Pre-existing issues: - [High] Architectural Flaw: Colorops are omitted from the atomic state during normal plane updates. -- commit a131c9a42c3beaf39f56794937e21266031060d8 Author: Melissa Wen <[email protected]> drm/atomic: only add states of active or transient active colorops Only consider affected colorop states those that are part of an active color pipeline or a pipeline that is about to be activated or deactivated in the same atomic commit. > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 170de30c28ae4..0d36a955c4d43 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c [ ... ] > @@ -1591,11 +1642,9 @@ drm_atomic_add_affected_planes(struct > drm_atomic_commit *state, > if (IS_ERR(plane_state)) > return PTR_ERR(plane_state); > > - if (plane_state->color_pipeline) { > - ret = drm_atomic_add_affected_colorops(state, plane); > - if (ret) > - return ret; > - } > + ret = drm_atomic_add_pipeline_colorops(state, plane); > + if (ret) > + return ret; [Severity: High] This isn't a bug introduced by this patch, but since drm_atomic_add_affected_planes() executes exclusively during CRTC modesets, does this mean active colorops are never added to the atomic state during a simple plane update like a page flip? Drivers that iterate over state->colorops via for_each_new_colorop_in_state() (like intel_plane_color_copy_uapi_to_hw_state() in the intel driver) will silently fail on page flips since the hardware state blobs remain empty. Should the color pipeline dependency tracking be anchored to the plane's inclusion in the state, perhaps within drm_atomic_get_plane_state()? [Severity: Medium] Also, the commit message states the goal is to remove the condition for plane_state->color_pipeline to handle deactivations. Did this patch miss updating drm_atomic_helper_duplicate_state() in drm_atomic_helper.c? It still checks if (plane_state->color_pipeline) and calls the old drm_atomic_add_affected_colorops(), which loops over all colorops belonging to the plane: drivers/gpu/drm/drm_atomic_helper.c:drm_atomic_helper_duplicate_state() { ... if (plane_state->color_pipeline) { err = drm_atomic_add_affected_colorops(state, plane); if (err) goto free; } ... } Should it be updated to use the newly introduced drm_atomic_add_pipeline_colorops() unconditionally to ensure consistent state duplication during suspend/resume? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
