[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);
+
+	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)
+			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:
-- 
2.52.0

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:

On Fri, 2026-02-06 at 13:22 -0500, Harry Wentland wrote:
> 
> 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.

Got it. I'll explain it better in the next version. I think the edid
check was there only to guard against parsing it in parse_amd_vsdb(). I
must say the code there was not the clearest but I can't think of a
reason to check for edid in case of DP. If it's missing, the
display_info won't have a valid range.

The parsing functions check for edid as well so this check is actually
redundant and could be entirely removed. vsdb structs are initialized to
0 either way so nothing will break and nothing will get enabled by
mistake.

Quite honestly, looking at (before changes) parse_edid_displayid_vrr(),
parse_amd_vsdb(), parse_hdmi_amd_vsdb() there's quite a bit of code
duplication and especially the former two are almost the same.

> > +
> > +	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.

Will do. Now that I'm looking at this with a clear head, it's too much
in one go even for me :) I think this will end up as 3-4 patches to
clean up the vsdb parsing functions as well.

> 
> 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:
>