Re: [PATCH RFC 03/25] drm/atomic-helper: Set mode_changed on vrr_enabled change

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/atomic: mode_changed forced on VRR toggle adds all planes breaking fastsets
- [High] drm/amd/display: multi-display VRR toggle causes unintended modesets via DSC check
--

commit e5fcd3be2834f5245a6d72ea14c2fd1ffcde525f
Author: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

drm/atomic-helper: Set mode_changed on vrr_enabled change

This commit modifies the DRM atomic core to set the mode_changed flag when
the vrr_enabled property changes on a CRTC. This approach intertwines VRR
toggles with mode changes to replace ad-hoc driver implementations.

> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 9d006f98413a5..0664d57792826 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -694,6 +694,12 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>  			new_crtc_state->active_changed = true;
>  		}
>  
> +		if (old_crtc_state->vrr_enabled != new_crtc_state->vrr_enabled) {
> +			drm_dbg_atomic(dev, "[CRTC:%d:%s] VRR changed\n",
> +				       crtc->base.id, crtc->name);
> +			new_crtc_state->mode_changed = true;
                        ^^^^

[Severity: High]
Does setting mode_changed to true here destroy fast-path performance by
adding all planes to the atomic state?

Because new_crtc_state->mode_changed is true, drm_atomic_crtc_needs_modeset()
evaluates to true. Later in drm_atomic_helper_check_modeset(), this triggers:

    ret = drm_atomic_add_affected_planes(state, crtc);

This unconditionally adds all planes attached to the CRTC to the atomic state
and acquires their locks. Toggling VRR now forces the driver's commit tail
to process all planes, which significantly increases overhead and causes
unnecessary lock contention across all drivers using this helper.

[Severity: High]
Can this also cause unintended modesets in the AMD display driver when
multiple displays are connected and a DSC check occurs?

During amdgpu_dm_atomic_check(), the forced mode_changed value is cached:

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:amdgpu_dm_atomic_check() {
    ...
    dm_new_crtc_state->mode_changed_independent_from_dsc = new_crtc_state->mode_changed;
    ...
}

The AMD driver correctly identifies the VRR toggle as a fastset and clears
new_crtc_state->mode_changed to false. However, if another CRTC triggers a
modeset, lock_and_validation_needed becomes true, invoking
compute_mst_dsc_configs_for_state().

For the VRR CRTC, pre_compute_mst_dsc_configs_for_state() observes no timing
changes and erroneously restores the cached true value:

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c:pre_compute_mst_dsc_configs_for_state() {
    ...
    dm_new_crtc_state->base.mode_changed = dm_new_crtc_state->mode_changed_independent_from_dsc;
    ...
}

This incorrectly flags the VRR CRTC for a full modeset. This causes unexpected
screen blanking on the VRR display, and if userspace did not provide the
DRM_MODE_ATOMIC_ALLOW_MODESET flag (expecting a fastset), the atomic commit
will be rejected with -EINVAL.

> +		}
> +
>  		if (new_crtc_state->enable != has_connectors) {
>  			drm_dbg_atomic(dev, "[CRTC:%d:%s] enabled/connectors mismatch (%d/%d)\n",
>  				       crtc->base.id, crtc->name,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921-vrr-limiter-uapi-v1-0-2fcd7d011646@collabora.com?part=3