Re: [PATCH v3 3/4] drm/amd/display: Clean up FreeSync capability detection

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing fallback to EDID monitor range when an AMD VSDB is present but does not advertise FreeSync support, breaking VRR on DP/eDP displays.
--

--- Patch [3]: [PATCH v3 3/4] drm/amd/display: Clean up FreeSync capability detection ---
commit 54e384ef3776414446c8d6bb002d768a87198a34
Author: Alex Huang <[email protected]>

drm/amd/display: Clean up FreeSync capability detection

This commit refactors FreeSync capability detection in the amdgpu_dm driver.
It extracts duplicate code for reading the AMD VSDB and sets up the connector
fields outside of the specific display interface branches. It also falls back
to the monitor_range data if the VSDB is not present.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 44b8dd000f5ca..6d83e9a425377 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
[ ... ]
> @@ -13766,63 +13766,57 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
>  	     connector->display_info.monitor_range.max_vfreq == 0))
>  		parse_edid_displayid_vrr(connector, edid);
>  
> -	if (edid && (sink->sink_signal == SIGNAL_TYPE_DISPLAY_PORT ||
> -		     sink->sink_signal == SIGNAL_TYPE_EDP)) {
> -		if (amdgpu_dm_connector->dc_link &&
> -		    amdgpu_dm_connector->dc_link->dpcd_caps.allow_invalid_MSA_timing_param) {
> -			amdgpu_dm_connector->min_vfreq = connector->display_info.monitor_range.min_vfreq;
> -			amdgpu_dm_connector->max_vfreq = connector->display_info.monitor_range.max_vfreq;
> -			if (amdgpu_dm_connector->max_vfreq - amdgpu_dm_connector->min_vfreq > 10)
> -				freesync_capable = true;
> +	has_vsdb = get_amd_vsdb(amdgpu_dm_connector, &vsdb_info) != 0;
> +
> +	if (has_vsdb) {
> +		amdgpu_dm_connector->vsdb_info = vsdb_info;
> +
> +		/* copy refresh rate info as long as VSDB advertises FreeSync */
> +		if (vsdb_info.freesync_supported) {
> +			amdgpu_dm_connector->min_vfreq = vsdb_info.min_refresh_rate_hz;
> +			amdgpu_dm_connector->max_vfreq = vsdb_info.max_refresh_rate_hz;
> +
> +			connector->display_info.monitor_range.min_vfreq =
> +				vsdb_info.min_refresh_rate_hz;
> +			connector->display_info.monitor_range.max_vfreq =
> +				vsdb_info.max_refresh_rate_hz;
>  		}
> +	} else {
> +		/* fall back to the base EDID range when there is no VSDB */
> +		amdgpu_dm_connector->min_vfreq = connector->display_info.monitor_range.min_vfreq;
> +		amdgpu_dm_connector->max_vfreq = connector->display_info.monitor_range.max_vfreq;
> +	}

[Severity: High]
Does this code inadvertently drop the EDID monitor range fallback when an AMD
VSDB is present but does not support FreeSync?

Looking at amdgpu_dm_update_freesync_caps(), if has_vsdb is true but
vsdb_info.freesync_supported is false, the code skips updating the frequency
limits in the if block.

Because has_vsdb is true, the else block is also bypassed, leaving
amdgpu_dm_connector->min_vfreq and max_vfreq at their default zero values.

Would this cause the subsequent VRR capability check 
max_vfreq - min_vfreq > 10 to fail, disabling FreeSync on DP/eDP displays?

Should the EDID fallback assignment be applied whenever
!vsdb_info.freesync_supported, rather than only when the VSDB is entirely
absent?

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