Re: [PATCH v4 03/10] drm/ingenic: Remove calls to drm_atomic_helper_check_plane_damage()

[sashiko-bot@kernel.org]

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 <tzimmermann@suse.de>

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/20260530185716.65688-1-tzimmermann@suse.de?part=3