Re: [PATCH v3 04/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 2 potential issue(s) to consider:
- [Medium] Comparing plane `src` coordinates before `atomic_check` clips them causes unintended full frame updates for partially off-screen planes.
- [Low] Removal of unit tests for `src` coordinate changes without replacing them causes a loss of critical test coverage.
--

commit 70283d9d64d3987145c43d7d6e7df2c84b0de6ae
Author: Thomas Zimmermann <tzimmermann@suse.de>
drm/damage-helper: Test src coord in drm_atomic_helper_check_plane_damage()

This commit evaluates plane source coordinate changes during the atomic check
phase instead of the atomic commit phase. It sets the ignore_damage_clips flag
in the plane state if coordinates change and removes related unit tests.

> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1034,7 +1034,7 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
>  
>  		drm_atomic_helper_plane_changed(state, old_plane_state, new_plane_state, plane);
>  
> -		drm_atomic_helper_check_plane_damage(state, new_plane_state);
> +		drm_atomic_helper_check_plane_damage(state, old_plane_state, new_plane_state);

[Severity: Medium]
Is there a risk of unintended full frame updates for partially off-screen
planes here?

When userspace requests an update, the DRM core provides unclipped coordinates
into new_plane_state->src. The old_plane_state->src retains the coordinates
from the previous commit, which were already clipped during that commit's
atomic check phase via drm_atomic_helper_check_plane_state().

Because drm_atomic_helper_check_plane_damage() now performs the
drm_rect_equals check before atomic_check scales and clips the new
coordinates, does it erroneously detect a change in src and permanently latch
ignore_damage_clips = true? This could cause any plane positioned partially
off-screen to continuously ignore damage clips and perform a full update on
every single frame.

Note: This regression is fixed later in the patch series by commit
"drm/atomic_helper: Do not evaluate plane damage before atomic_check".

>  
>  		if (!funcs || !funcs->atomic_check)
>  			continue;

[ ... ]

> diff --git a/drivers/gpu/drm/tests/drm_damage_helper_test.c b/drivers/gpu/drm/tests/drm_damage_helper_test.c
> --- a/drivers/gpu/drm/tests/drm_damage_helper_test.c
> +++ b/drivers/gpu/drm/tests/drm_damage_helper_test.c

[ ... ]

> -static void drm_test_damage_iter_no_damage_src_moved(struct kunit *test)
> -{
> -	struct drm_damage_mock *mock = test->priv;
> -	struct drm_atomic_helper_damage_iter iter;
> -	struct drm_rect clip;
> -	u32 num_hits = 0;
> -
> -	/* Plane src moved since old plane state. */
> -	set_plane_src(&mock->old_state, 0, 0, 1024 << 16, 768 << 16);
> -	set_plane_src(&mock->state, 10 << 16, 10 << 16,
> -		      (10 + 1024) << 16, (10 + 768) << 16);
> -	drm_atomic_helper_damage_iter_init(&iter, &mock->old_state, &mock->state);
> -	drm_atomic_for_each_plane_damage(&iter, &clip)
> -		num_hits++;
> -
> -	KUNIT_EXPECT_EQ_MSG(test, num_hits, 1, "Should return plane src as damage.");
> -	check_damage_clip(test, &clip, 10, 10, 1034, 778);
> -}

[Severity: Low]
Does the removal of these unit tests cause a loss of valuable test coverage?

The commit message mentions there is a separate unit test for when
ignore_damage_clips is true, but without these removed tests, there are no
longer any tests ensuring that moving the src coordinates actually correctly
sets ignore_damage_clips = true via drm_atomic_helper_check_plane_damage().

Instead of completely deleting these tests, could they have been adapted to
call drm_atomic_helper_check_plane_damage() prior to initializing the
iterator, preserving the validation of the source coordinate tracking logic?

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