Re: [PATCH v4 3/4] drm/amd/display: Add HDMI 2.1 VRR support from HF-VSDB

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The HF-VSDB VRR fallback is negated for HDMI TMDS connections due to a pre-existing AMD FreeSync MCCS check resetting `freesync_capable` to `false`.

Pre-existing issues:
- [High] The highest refresh rate mode cache (`freesync_vid_base`) is never invalidated across hotplugs, corrupting `max_vfreq` calculations for subsequently connected displays.
--

--- Patch [3]: [PATCH v4 3/4] drm/amd/display: Add HDMI 2.1 VRR support from HF-VSDB ---
commit cf4d7fc4ac615fcafd91467c91d4c4582773cd6b
Author: Fangzhi Zuo <[email protected]>

drm/amd/display: Add HDMI 2.1 VRR support from HF-VSDB

This commit updates the FreeSync capabilities logic to fall back to the
HDMI 2.1 VRR range parsed from the HF-VSDB when the AMD VSDB does not
provide a valid range.

> 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 9eaa20ec528ec..2fe7bfb1e88f7 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

[ ... ]

> @@ -3856,6 +3865,59 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
>  				connector->display_info.monitor_range.max_vfreq = vsdb_info.max_refresh_rate_hz;
>  			}
>  		}
> +
> +		drm_dbg_driver(adev_to_drm(adev),
> +			       "VRR: amd_vsdb i=%d fs_sup=%d min=%d max=%d fs_capable=%d\n",
> +			       i, vsdb_info.freesync_supported,
> +			       vsdb_info.min_refresh_rate_hz,
> +			       vsdb_info.max_refresh_rate_hz, freesync_capable);
> +
> +		/*
> +		 * If AMD VSDB didn't provide a valid FreeSync range, fall back to
> +		 * the HDMI 2.1 VRR capability parsed from the HF-VSDB.
> +		 */
> +		if (!freesync_capable && connector->display_info.hdmi.vrr_cap.supported) {
> +			struct drm_hdmi_vrr_cap *vrr_cap =
> +				&connector->display_info.hdmi.vrr_cap;
> +
> +			drm_dbg_driver(adev_to_drm(adev),
> +				       "VRR: HF-VSDB fallback: hdmi_vrr=1 vrr_cap[sup=%d min=%d max=%d] mrange_max=%d\n",
> +				       vrr_cap->supported, vrr_cap->vrr_min, vrr_cap->vrr_max,
> +				       connector->display_info.monitor_range.max_vfreq);
> +
> +			if (vrr_cap->supported && vrr_cap->vrr_min > 0) {
> +				amdgpu_dm_connector->min_vfreq = vrr_cap->vrr_min;
> +				amdgpu_dm_connector->max_vfreq = vrr_cap->vrr_max ?
> +					vrr_cap->vrr_max :
> +					connector->display_info.monitor_range.max_vfreq;
> +
> +				/*
> +				 * VRRMAX = 0 in the HF-VSDB means "up to the Base
> +				 * Refresh Rate". If the EDID also did not provide a
> +				 * monitor range max, fall back to the Base Refresh
> +				 * Rate (the highest refresh rate of the preferred
> +				 * timing) so a valid VRR range is still reported to
> +				 * userspace.
> +				 */
> +				if (!amdgpu_dm_connector->max_vfreq) {
> +					struct drm_display_mode *brr_mode =
> +						amdgpu_dm_get_highest_refresh_rate_mode(amdgpu_dm_connector, true);

[Severity: High]
This is a pre-existing issue, but does amdgpu_dm_get_highest_refresh_rate_mode()
return stale data across hotplugs?

It appears aconnector->freesync_vid_base is never cleared on hotplug:

amdgpu_dm_get_highest_refresh_rate_mode() {
    ...
    if (aconnector->freesync_vid_base.clock != 0)
        return &aconnector->freesync_vid_base;
    ...
}

If a high-refresh monitor is unplugged and an HDMI 2.1 VRR monitor with
VRRMAX = 0 is plugged into the same port, will this use the cached
high-refresh mode of the disconnected monitor and calculate an incorrect
max_vfreq?

> +
> +					if (brr_mode)
> +						amdgpu_dm_connector->max_vfreq =
> +							drm_mode_vrefresh(brr_mode);
> +				}
> +
> +				if (amdgpu_dm_connector->max_vfreq -
> +				    amdgpu_dm_connector->min_vfreq > 10)
> +					freesync_capable = true;

[Severity: High]
Does this new fallback get negated later in amdgpu_dm_update_freesync_caps()
for HDMI TMDS connections?

When an HDMI monitor without an AMD VSDB is connected, freesync_vcp_code
remains 0. Later in this same function, the pre-existing MCCS check does this:

    if ((sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A ||
        as_type == FREESYNC_TYPE_PCON_IN_WHITELIST) &&
        (!sink->edid_caps.freesync_vcp_code ||
        (sink->edid_caps.freesync_vcp_code && !sink->mccs_caps.freesync_supported)))
        freesync_capable = false;

Since !sink->edid_caps.freesync_vcp_code evaluates to true for TMDS,
could this prevent the HF-VSDB fallback from working for these displays?

> +
> +				connector->display_info.monitor_range.min_vfreq =
> +					amdgpu_dm_connector->min_vfreq;
> +				connector->display_info.monitor_range.max_vfreq =
> +					amdgpu_dm_connector->max_vfreq;
> +			}
> +		}
>  	}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3