Re: [PATCH v2 2/5] drm/amd/display: Don't force a modeset when the ABM level changes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] drm/amd/display: missing companion planes on VRR CRTC in
  amdgpu_dm_atomic_check()
--

commit 827e5ec3b519e3ae27f4dddc2ca14ba587d16858
Author: Hugo Sales <hugo@hsal.es>

drm/amd/display: Don't force a modeset when the ABM level changes

This commit stops setting connectors_changed on the CRTC state in
amdgpu_dm_atomic_check() when a connector's abm_level changes, allowing
ABM level updates to proceed without a modeset.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 3077744b2d2d5..f5a45686df454 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -6371,8 +6371,7 @@ STATIC_IFN_KUNIT int amdgpu_dm_atomic_check(struct drm_device *dev,
>  			goto fail;
>  		}
>  
> -		if (dm_old_con_state->abm_level != dm_new_con_state->abm_level ||
> -		    dm_old_con_state->scaling != dm_new_con_state->scaling)
> +		if (dm_old_con_state->scaling != dm_new_con_state->scaling)
>  			new_crtc_state->connectors_changed = true;
>  	}

[Severity: Medium]
Can a connector-only abm_level commit on a VRR-active CRTC drop active
overlay planes from DC?

Without connectors_changed, drm_atomic_crtc_needs_modeset() is false, so
amdgpu_dm_atomic_check() skips drm_atomic_add_affected_planes().  With no
planes in the atomic state yet, the companion-plane loop in
amdgpu_dm_atomic_check() sees modified == false and adds nothing, and both
dm_update_plane_state() passes run on zero planes.

Later in amdgpu_dm_atomic_check(), drm_atomic_helper_check_planes() calls
amdgpu_dm_crtc_helper_atomic_check(), which pulls only crtc->primary into
the state when VRR is active:

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c:amdgpu_dm_crtc_helper_atomic_check() {
    ...
	if (!state->legacy_cursor_update && amdgpu_dm_crtc_vrr_active(dm_crtc_state)) {
		struct drm_plane_state *primary_state;

		/* Pull in primary plane for correct VRR handling */
		primary_state = drm_atomic_get_plane_state(state, crtc->primary);
		if (IS_ERR(primary_state))
			return PTR_ERR(primary_state);
	}
    ...
}

Because that happens after the companion-plane expansion and
dm_update_plane_state(), amdgpu_dm_commit_planes() submits only
crtc->primary (surface_count == 1) to dc_update_planes_and_stream().  If
an overlay plane is active on the stream, DC sees
stream_status->plane_count != surface_count, elevates to UPDATE_TYPE_FULL,
and replaces all planes on the stream with only crtc->primary, leaving the
overlay plane disabled in DC while DRM still considers it enabled and
causing later plane resets or disables on that CRTC to fail with -EINVAL
in dc_state_remove_plane().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-abm-sysfs-commit-v2-0-5808d17a7e89@hsal.es?part=2