Re: [PATCH v4 08/10] drm/damage-helper: Remove old state from drm_atomic_helper_damage_merged()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The i915 driver fails to perform full plane updates when source coordinates change (e.g., panning or cropping), leading to visual corruption.
--

commit 020ae3c303979cf114157a7b95370725231ed90b
Author: Thomas Zimmermann <tzimmermann@suse.de>

drm/damage-helper: Remove old state from drm_atomic_helper_damage_merged()

Nothing in drm_atomic_helper_damage_merged() requires the old plane state.
Remove the parameter and mass-convert callers. Most callers now no longer
require the old plane state in their plane's atomic_update helper.

> diff --git a/drivers/gpu/drm/i915/display/intel_plane.c b/drivers/gpu/drm/i915/display/intel_plane.c
> index 2a52b36c646c7..3560e222a3eae 100644
> --- a/drivers/gpu/drm/i915/display/intel_plane.c
> +++ b/drivers/gpu/drm/i915/display/intel_plane.c
[ ... ]
> @@ -346,7 +346,6 @@ static void intel_plane_clear_hw_state(struct intel_plane_state *plane_state)
>  
>  static void
>  intel_plane_copy_uapi_plane_damage(struct intel_plane_state *new_plane_state,
> -				   const struct intel_plane_state *old_uapi_plane_state,
>  				   const struct intel_plane_state *new_uapi_plane_state)
>  {
>  	struct intel_display *display = to_intel_display(new_plane_state);
> @@ -356,10 +355,9 @@ intel_plane_copy_uapi_plane_damage(struct intel_plane_state *new_plane_state,
>  	if (DISPLAY_VER(display) < 12)
>  		return;
>  
> -	if (!drm_atomic_helper_damage_merged(&old_uapi_plane_state->uapi,
> -					     &new_uapi_plane_state->uapi,
> +	if (!drm_atomic_helper_damage_merged(&new_uapi_plane_state->uapi,
>  					     damage))

[Severity: High]
Does this code risk missing full plane updates when source coordinates change?

It appears that with the removal of the old state, the fallback checking
for source coordinate changes (like panning) was also removed from the
damage iterators. The DRM core now expects drivers to call
drm_atomic_helper_check_plane_damage() to set ignore_damage_clips = true
when the source changes.

Because i915 implements a custom atomic check phase in intel_atomic_check()
that does not call drm_atomic_helper_check_planes() or
drm_atomic_helper_check_plane_damage(), ignore_damage_clips remains unset.

Will drm_atomic_helper_damage_merged() incorrectly return only partial
damage here when the plane is panned, missing the full update?

> -		/* Incase helper fails, mark whole plane region as damage */
> +		/* In case the helper fails, mark whole plane region as damage */
>  		*damage = drm_plane_state_src(&new_uapi_plane_state->uapi);
>  }
[ ... ]
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index e138982dc91f6..22bab263d85d4 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
[ ... ]
> @@ -3018,8 +3018,7 @@ int intel_psr2_sel_fetch_update(struct intel_atomic_state *state,
>  		src = drm_plane_state_src(&new_plane_state->uapi);
>  		drm_rect_fp_to_int(&src, &src);
>  
> -		if (!drm_atomic_helper_damage_merged(&old_plane_state->uapi,
> -						     &new_plane_state->uapi, &damaged_area))
> +		if (!drm_atomic_helper_damage_merged(&new_plane_state->uapi, &damaged_area))

[Severity: High]
Can this cause severe visual corruption for PSR2 selective fetch?

If intel_psr2_sel_fetch_update() no longer forces a full update on source
coordinate changes, it seems this might only fetch the small damaged area
(such as a blinking cursor) while leaving the rest of the panned screen
stale.

>  			continue;
>  
>  		damaged_area.y1 += new_plane_state->uapi.dst.y1 - src.y1;

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