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 <[email protected]> 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/[email protected]?part=3
