Re: [PATCH v3 02/19] drm/amd/display: Refactor amdgpu_dm_update_freesync_caps()

[Harry Wentland <harry.wentland@amd.com>]

On 2026-02-03 13:56, Tomasz Pakuła wrote:
> [Why]
> This function started to get very messy and hard to follow.
> 
> [How]
> Eject some functionality to separate functions and simplify greatly.
> 
> Changes in v3:
> - Less struct traversal in helper functions
> 
> Signed-off-by: Tomasz Pakuła <tomasz.pakula.oficjalny@gmail.com>
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 123 +++++++++++-------
>  1 file changed, 73 insertions(+), 50 deletions(-)
> 
> 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 29e4a047b455..2c5877ed5f32 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -13119,8 +13119,8 @@ static void parse_edid_displayid_vrr(struct drm_connector *connector,
>  	}
>  }
>  
> -static int parse_amd_vsdb(struct amdgpu_dm_connector *aconnector,
> -			  const struct edid *edid, struct amdgpu_hdmi_vsdb_info *vsdb_info)
> +static int parse_amd_vsdb_did(struct amdgpu_dm_connector *aconnector,
> +			      const struct edid *edid, struct amdgpu_hdmi_vsdb_info *vsdb_info)
>  {
>  	u8 *edid_ext = NULL;
>  	int i;
> @@ -13172,13 +13172,13 @@ static int parse_amd_vsdb(struct amdgpu_dm_connector *aconnector,
>  	return false;
>  }
>  
> -static int parse_hdmi_amd_vsdb(struct amdgpu_dm_connector *aconnector,
> -			       const struct edid *edid,
> -			       struct amdgpu_hdmi_vsdb_info *vsdb_info)
> +static int parse_amd_vsdb_cea(struct amdgpu_dm_connector *aconnector,
> +			      const struct edid *edid,
> +			      struct amdgpu_hdmi_vsdb_info *vsdb_info)
>  {
> +	struct amdgpu_hdmi_vsdb_info vsdb_local = {0};
>  	u8 *edid_ext = NULL;
>  	int i;
> -	bool valid_vsdb_found = false;
>  
>  	/*----- drm_find_cea_extension() -----*/
>  	/* No EDID or EDID extensions */
> @@ -13199,9 +13199,47 @@ static int parse_hdmi_amd_vsdb(struct amdgpu_dm_connector *aconnector,
>  	if (edid_ext[0] != CEA_EXT)
>  		return -ENODEV;
>  
> -	valid_vsdb_found = parse_edid_cea(aconnector, edid_ext, EDID_LENGTH, vsdb_info);
> +	if (!parse_edid_cea(aconnector, edid_ext, EDID_LENGTH, &vsdb_local))
> +		return -ENODEV;
>  
> -	return valid_vsdb_found ? i : -ENODEV;
> +	*vsdb_info = vsdb_local;
> +	return i;
> +}
> +
> +static bool is_monitor_range_invalid(const struct drm_connector *conn)
> +{
> +	return conn->display_info.monitor_range.min_vfreq == 0 ||
> +	       conn->display_info.monitor_range.max_vfreq == 0;
> +}
> +
> +/*
> + * Returns true if (max_vfreq - min_vfreq) > 10
> + */
> +static bool is_freesync_capable(const struct drm_monitor_range_info *range)
> +{
> +	return (range->max_vfreq - range->min_vfreq) > 10;
> +}
> +
> +static void monitor_range_from_vsdb(struct drm_display_info *display,
> +				    const struct amdgpu_hdmi_vsdb_info *vsdb)
> +{
> +	display->monitor_range.min_vfreq = vsdb->min_refresh_rate_hz;
> +	display->monitor_range.max_vfreq = vsdb->max_refresh_rate_hz;
> +}
> +
> +/*
> + * Returns true if connector is capable of freesync
> + * Optionally, can fetch the range from AMD vsdb
> + */
> +static bool copy_range_to_amdgpu_connector(struct drm_connector *conn)
> +{
> +	struct amdgpu_dm_connector *aconn = to_amdgpu_dm_connector(conn);
> +	struct drm_monitor_range_info *range = &conn->display_info.monitor_range;
> +
> +	aconn->min_vfreq = range->min_vfreq;
> +	aconn->max_vfreq = range->max_vfreq;
> +
> +	return is_freesync_capable(range);
>  }
>  
>  /**
> @@ -13218,13 +13256,14 @@ static int parse_hdmi_amd_vsdb(struct amdgpu_dm_connector *aconnector,
>  void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
>  				    const struct drm_edid *drm_edid)
>  {
> -	int i = 0;
>  	struct amdgpu_dm_connector *amdgpu_dm_connector =
>  			to_amdgpu_dm_connector(connector);
>  	struct dm_connector_state *dm_con_state = NULL;
>  	struct dc_sink *sink;
>  	struct amdgpu_device *adev = drm_to_adev(connector->dev);
>  	struct amdgpu_hdmi_vsdb_info vsdb_info = {0};
> +	struct amdgpu_hdmi_vsdb_info vsdb_did = {0};
> +	struct dpcd_caps dpcd_caps = {0};
>  	const struct edid *edid;
>  	bool freesync_capable = false;
>  	enum adaptive_sync_type as_type = ADAPTIVE_SYNC_TYPE_NONE;
> @@ -13256,62 +13295,46 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
>  		goto update;
>  
>  	edid = drm_edid_raw(drm_edid); // FIXME: Get rid of drm_edid_raw()
> +	parse_amd_vsdb_cea(amdgpu_dm_connector, edid, &vsdb_info);

This change says it's a refactor, which in my book should
never include a (subtle) functional change. But we're now
calling this function for all sink_signal types, whereas
before it was only called for HDMI_TYPE_A.

> +
> +	if (amdgpu_dm_connector->dc_link)
> +		dpcd_caps = amdgpu_dm_connector->dc_link->dpcd_caps;
>  
>  	/* Some eDP panels only have the refresh rate range info in DisplayID */
> -	if ((connector->display_info.monitor_range.min_vfreq == 0 ||
> -	     connector->display_info.monitor_range.max_vfreq == 0))
> +	if (is_monitor_range_invalid(connector))
>  		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;
> -		}
> +	if (sink->sink_signal == SIGNAL_TYPE_DISPLAY_PORT ||
> +	    sink->sink_signal == SIGNAL_TYPE_EDP) {
>  
> -		parse_amd_vsdb(amdgpu_dm_connector, edid, &vsdb_info);
> +		if (dpcd_caps.allow_invalid_MSA_timing_param)
> +			freesync_capable = copy_range_to_amdgpu_connector(connector);
>  
> -		if (vsdb_info.replay_mode) {
> -			amdgpu_dm_connector->vsdb_info.replay_mode = vsdb_info.replay_mode;
> -			amdgpu_dm_connector->vsdb_info.amd_vsdb_version = vsdb_info.amd_vsdb_version;
> +		/* eDP */
> +		if (edid)

Same here, I'm not entirely sure whether moving the edid
check down here won't have a subtle behavior change.

I'd like to be either convinced that these things cannot
change behavior, or I'd like to see this broken out into
two patches, (1) a true refactor patch, without possible
behavior changes, and (2) another patch that might affect
behavior.

Overall I'm in favor of the changes and thank you for
cleaning this up. I'm just worried about subtle bugs.

Harry

> +			parse_amd_vsdb_did(amdgpu_dm_connector, edid, &vsdb_did);
> +
> +		if (vsdb_did.replay_mode) {
> +			amdgpu_dm_connector->vsdb_info.replay_mode = vsdb_did.replay_mode;
> +			amdgpu_dm_connector->vsdb_info.amd_vsdb_version = vsdb_did.amd_vsdb_version;
>  			amdgpu_dm_connector->as_type = ADAPTIVE_SYNC_TYPE_EDP;
>  		}
>  
> -	} else if (drm_edid && sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A) {
> -		i = parse_hdmi_amd_vsdb(amdgpu_dm_connector, edid, &vsdb_info);
> -		if (i >= 0 && 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;
> -			if (amdgpu_dm_connector->max_vfreq - amdgpu_dm_connector->min_vfreq > 10)
> -				freesync_capable = true;
> -
> -			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 if (sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A && vsdb_info.freesync_supported) {
> +		monitor_range_from_vsdb(&connector->display_info, &vsdb_info);
> +		freesync_capable = copy_range_to_amdgpu_connector(connector);
>  	}
>  
>  	if (amdgpu_dm_connector->dc_link)
>  		as_type = dm_get_adaptive_sync_support_type(amdgpu_dm_connector->dc_link);
>  
> -	if (as_type == FREESYNC_TYPE_PCON_IN_WHITELIST) {
> -		i = parse_hdmi_amd_vsdb(amdgpu_dm_connector, edid, &vsdb_info);
> -		if (i >= 0 && vsdb_info.freesync_supported && vsdb_info.amd_vsdb_version > 0) {
> +	if (as_type == FREESYNC_TYPE_PCON_IN_WHITELIST && vsdb_info.freesync_supported) {
> +		amdgpu_dm_connector->pack_sdp_v1_3 = true;
> +		amdgpu_dm_connector->as_type = as_type;
> +		amdgpu_dm_connector->vsdb_info = vsdb_info;
>  
> -			amdgpu_dm_connector->pack_sdp_v1_3 = true;
> -			amdgpu_dm_connector->as_type = as_type;
> -			amdgpu_dm_connector->vsdb_info = vsdb_info;
> -
> -			amdgpu_dm_connector->min_vfreq = vsdb_info.min_refresh_rate_hz;
> -			amdgpu_dm_connector->max_vfreq = vsdb_info.max_refresh_rate_hz;
> -			if (amdgpu_dm_connector->max_vfreq - amdgpu_dm_connector->min_vfreq > 10)
> -				freesync_capable = true;
> -
> -			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;
> -		}
> +		monitor_range_from_vsdb(&connector->display_info, &vsdb_info);
> +		freesync_capable = copy_range_to_amdgpu_connector(connector);
>  	}
>  
>  update: