Re: [PATCH v4 06/10] drm/damage-helper: Test src coord in 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:
- [High] Accessor functions `drm_plane_get_damage_clips()` and `drm_plane_get_damage_clips_count()` return outdated partial clips instead of forcing a full update when `ignore_damage_clips` is set, causing drivers to skip full plane updates during modesets and `src` coordinate changes.
--

commit 18363d38b23996b07f12d04823d811d72b0f24d4
Author: Thomas Zimmermann <tzimmermann@suse.de>

drm/damage-helper: Test src coord in drm_atomic_helper_check_plane_damage()

This commit updates drm_atomic_helper_check_plane_damage() to evaluate if
the source coordinates change across atomic commits. If they do, it sets the
ignore_damage_clips flag in the plane state to force a full update, and
removes this check from drm_atomic_helper_damage_iter_init().

> diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c
> index 945fac8dc27b6..f492a59edbeb5 100644
> --- a/drivers/gpu/drm/drm_damage_helper.c
> +++ b/drivers/gpu/drm/drm_damage_helper.c

[ ... ]

> @@ -67,19 +68,23 @@ static void convert_clip_rect_to_rect(const struct drm_clip_rect *src,
>   * &drm_plane_state.src as damage.
>   */
>  void drm_atomic_helper_check_plane_damage(struct drm_atomic_commit *state,
> -					  struct drm_plane_state *plane_state)
> +					  const struct drm_plane_state *old_plane_state,
> +					  struct drm_plane_state *new_plane_state)
>  {
>  	struct drm_crtc_state *crtc_state;
>  
> -	if (plane_state->crtc) {
> +	if (!drm_rect_equals(&new_plane_state->src, &old_plane_state->src))
> +		new_plane_state->ignore_damage_clips = true;
> +

[Severity: High]
Will this cause display corruption in drivers that manually read the damage
clips?

The accessors drm_plane_get_damage_clips_count() and
__drm_plane_get_damage_clips() in drivers/gpu/drm/drm_plane.c do not check
the ignore_damage_clips flag. 

If a driver calls them directly, such as AMDGPU in fill_dc_dirty_rects():

    num_clips = drm_plane_get_damage_clips_count(new_plane_state);
    clips = drm_plane_get_damage_clips(new_plane_state);

won't it receive the stale or partial user-provided damage clips instead of
performing a full update when the src coordinates change or during a full
modeset? 

> +	if (new_plane_state->crtc) {
>  		crtc_state = drm_atomic_get_new_crtc_state(state,
> -							   plane_state->crtc);
> +							   new_plane_state->crtc);
>  
>  		if (WARN_ON(!crtc_state))
>  			return;
>  
>  		if (drm_atomic_crtc_needs_modeset(crtc_state))
> -			plane_state->ignore_damage_clips = true;
> +			new_plane_state->ignore_damage_clips = true;
>  	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260530185716.65688-1-tzimmermann@suse.de?part=6