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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing fallback to EDID monitor range when an AMD VSDB is present but does not advertise FreeSync support.
- [Low] Using integer literal 0 to initialize a boolean variable.
--

--- Patch [3]: [PATCH 3/4] drm/amd/display: Clean up FreeSync capability detection ---
commit f8ae668c998c7d60648194bc1a8a582d9e952ad8
Author: Alex Huang <Alex.Huang2@amd.com>

drm/amd/display: Clean up FreeSync capability detection

This commit extracts duplicate FreeSync capability detection code into
common code, pulling VSDB data once and falling back to monitor ranges
when needed.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -13707,7 +13707,7 @@ static int get_amd_vsdb(struct amdgpu_dm_connector *aconnector,
>  void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
>  				    const struct drm_edid *drm_edid, bool do_mccs)
>  {
> -	int i = 0;
> +	bool has_vsdb = 0;

[Severity: Low]
This isn't a bug, but is it better to initialize this boolean variable
with 'false' instead of '0'?

[ ... ]

> +	if (has_vsdb) {
> +		amdgpu_dm_connector->vsdb_info = vsdb_info;
> +		amdgpu_dm_connector->vsdb_info.amd_vsdb_version = vsdb_info.amd_vsdb_version;
> +
> +		/* 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]
What happens if a monitor has an AMD VSDB (has_vsdb is true) but does not
advertise FreeSync support (vsdb_info.freesync_supported is false)?

In this scenario, it looks like the code skips assigning the VSDB refresh
rates but also skips the else block that falls back to the EDID monitor
range. Will amdgpu_dm_connector->min_vfreq and max_vfreq be left
unmodified, potentially retaining stale values from a previously connected
monitor?

[ ... ]

> +	if (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->max_vfreq - amdgpu_dm_connector->min_vfreq > 10)
> +			freesync_capable = true;

[Severity: High]
If min_vfreq and max_vfreq retain stale values, could this arithmetic check
evaluate to true and erroneously enable FreeSync?

For example, when hotplugging an eDP panel that has a VSDB for Replay mode
but no FreeSync, it seems freesync_capable might be incorrectly set to true
based on the previous display's refresh rates.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721164430.2121326-1-Alex.Huang2@amd.com?part=3