Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Missing `passive_vrr_disabled` check in atomic check prevents hardware update. - [High] In-place mutation of shared `dc_stream_state` during atomic check. - [Medium] Missing `passive_vrr_capable` property on MST endpoints, but the feature is silently enabled. --
commit b13d5b0a2a71643131cf661b1a6d126b390f1d96 Author: Tomasz Pakuła <[email protected]> drm/amd/display: Use passive_vrr properties in amdgpu This commit wires the passive_vrr DRM properties into amdgpu for HDMI, DP, and eDP sinks. It attaches the passive_vrr_capable property alongside vrr_capable for non-MST connectors, records the freesync_on_desktop capability, and sets the stream freesync_on_desktop state based on the passive_vrr_disabled property. > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c > index 0e71ba498e8aa..8b2eec1a683d4 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c [ ... ] > @@ -3241,8 +3242,10 @@ void amdgpu_dm_connector_init_helper(struct > amdgpu_display_manager *dm, > connector_type == DRM_MODE_CONNECTOR_eDP) { > > drm_connector_attach_hdr_output_metadata_property(&aconnector->base); > > - if (!aconnector->mst_root) > + if (!aconnector->mst_root) { > > drm_connector_attach_vrr_capable_property(&aconnector->base); > + > drm_connector_attach_passive_vrr_capable_property(&aconnector->base); > + } [Severity: Medium] Will this result in passive VRR being silently enabled for MST endpoints without exposing the capability property to userspace? The passive_vrr_capable property is explicitly restricted to non-MST endpoints here. However, the logic in amdgpu_dm_update_freesync_caps() and amdgpu_dm_get_freesync_config_for_crtc() appears to unconditionally activate freesync_on_desktop if the display is FreeSync capable. Because the capability property isn't cloned to dynamic MST endpoints, those endpoints will have the feature activated by default (since the passive_vrr_disabled property defaults to false) without userspace being able to discover the capability. [ ... ] > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_freesync.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_freesync.c > index 281c6cec9f4bc..903d5ab387d4b 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_freesync.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_freesync.c > @@ -138,6 +138,12 @@ void amdgpu_dm_get_freesync_config_for_crtc( > config.vsif_supported = true; > config.btr = true; > > + if (new_con_state->freesync_on_desktop_capable) > + new_crtc_state->stream->freesync_on_desktop = > + !new_crtc_state->base.passive_vrr_disabled; > + else > + new_crtc_state->stream->freesync_on_desktop = false; > + [Severity: High] If userspace only toggles the passive_vrr_disabled property without triggering a modeset, will the hardware actually get updated? It appears that amdgpu_dm_atomic_check() in amdgpu_dm.c skips adding affected planes if only passive_vrr_disabled changes. Its fast-return path checks for color_mgmt_changed, vrr_enabled, and dsc_force_changed, but misses a check for passive_vrr_disabled. If planes aren't added to the atomic state, the subsequent commit will skip updating the stream and VRR state entirely, leaving hardware and software out of sync. [Severity: High] Does this assignment mutate the active stream state during a TEST_ONLY atomic commit? In amdgpu_dm_get_freesync_config_for_crtc(), new_crtc_state->stream points to a shared, refcounted dc_stream_state object that represents the current hardware state. It is not deep-copied during state duplication. If userspace performs a DRM_MODE_ATOMIC_TEST_ONLY commit, writing directly to stream->freesync_on_desktop permanently alters the running state and breaks atomic test-only invariants. > if (fs_vid_mode) { > config.state = VRR_STATE_ACTIVE_FIXED; > config.fixed_refresh_in_uhz = > new_crtc_state->freesync_config.fixed_refresh_in_uhz; [ ... ] -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
