Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] Setting `crtc_state->mode_changed = true` inside `plane_atomic_check` without re-calling `drm_atomic_helper_check_modeset()` bypasses necessary modeset validations and state updates, leading to incomplete modesets. -- commit 0b5b8aa5c5b9f98bc80329fb6bac6de5430060f3 Author: Thomas Zimmermann <[email protected]> drm/ingenic: Remove calls to drm_atomic_helper_check_plane_damage() Atomic helpers call drm_atomic_helper_check_plane_damage() after the atomic_check anyway. Remove the calls from the planes' atomic_check. > diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > index 42c86f195c66b..e99b44e3ac923 100644 > --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c [ ... ] > @@ -519,9 +519,6 @@ static int ingenic_drm_plane_atomic_check(struct > drm_plane *plane, > old_plane_state->fb->format->format != > new_plane_state->fb->format->format)) > crtc_state->mode_changed = true; > [Severity: High] This is a pre-existing issue, but does setting crtc_state->mode_changed to true here in ingenic_drm_plane_atomic_check() bypass necessary validations? The DRM core documentation notes that if drivers set mode_changed = true during the plane check, they must re-call drm_atomic_helper_check_modeset(). Since the ingenic driver assigns drm_atomic_helper_check() directly to its global .atomic_check hook, check_modeset() runs before check_planes(). If a modeset check is not re-evaluated after planes alter the modeset requirement, affected connectors and bridges might never be added to the atomic state via drm_atomic_add_affected_connectors(). Will this cause the display pipeline to skip bridge and encoder modesets during the commit phase, potentially desynchronizing the output? > - if (priv->soc_info->map_noncoherent) > - drm_atomic_helper_check_plane_damage(state, new_plane_state); > - > return 0; > } > diff --git a/drivers/gpu/drm/ingenic/ingenic-ipu.c > b/drivers/gpu/drm/ingenic/ingenic-ipu.c > index 56143a191f361..fd17c642c7ac5 100644 > --- a/drivers/gpu/drm/ingenic/ingenic-ipu.c > +++ b/drivers/gpu/drm/ingenic/ingenic-ipu.c [ ... ] > @@ -611,7 +611,7 @@ static int ingenic_ipu_plane_atomic_check(struct > drm_plane *plane, > return -EINVAL; > > if (!osd_changed(new_plane_state, old_plane_state)) > - goto out_check_damage; > + return 0; > > crtc_state->mode_changed = true; > [Severity: High] This is also a pre-existing issue, but similar to the above, does setting crtc_state->mode_changed to true here in ingenic_ipu_plane_atomic_check() result in the same missing validation? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
