Re: [PATCH v1 2/3] drm/amd/display: Use passive_vrr properties in amdgpu

[sashiko-bot@kernel.org]

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 <tomasz.pakula.oficjalny@gmail.com>

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/20260901191251.2653684-1-jerry.zuo@amd.com?part=2