Re: [PATCH 09/14] drm/amd/display: Add Freesync HDMI support to DM

[Nathan Chancellor <nathan@kernel.org>]

On Thu, Feb 11, 2021 at 04:44:39PM -0500, Qingqing Zhuo wrote:
> From: Stylon Wang <stylon.wang@amd.com>
> 
> [Why]
> Add necessary support for Freesync HDMI in Linux DM
> 
> [How]
> - Support Freesync HDMI by calling DC interace
> - Report Freesync capability to vrr_range debugfs from DRM
> - Depends on coming DMCU/DMUB firmware to enable feature
> 
> Signed-off-by: Stylon Wang <stylon.wang@amd.com>
> Reviewed-by: Nicholas Kazlauskas <Nicholas.Kazlauskas@amd.com>
> Acked-by: Qingqing Zhuo <Qingqing.Zhuo@amd.com>
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 175 ++++++++++++++----
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   8 +
>  2 files changed, 144 insertions(+), 39 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 626a8cc92d65..c55ee0a24c26 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -34,6 +34,7 @@
>  #include "dc/inc/hw/dmcu.h"
>  #include "dc/inc/hw/abm.h"
>  #include "dc/dc_dmub_srv.h"
> +#include "dc/dc_edid_parser.h"
>  #include "amdgpu_dm_trace.h"
>  
>  #include "vid.h"
> @@ -6995,6 +6996,12 @@ static void amdgpu_dm_connector_ddc_get_modes(struct drm_connector *connector,
>  		 */
>  		drm_mode_sort(&connector->probed_modes);
>  		amdgpu_dm_get_native_mode(connector);
> +
> +		/* Freesync capabilities are reset by calling
> +		 * drm_add_edid_modes() and need to be
> +		 * restored here.
> +		 */
> +		amdgpu_dm_update_freesync_caps(connector, edid);
>  	} else {
>  		amdgpu_dm_connector->num_modes = 0;
>  	}
> @@ -9718,11 +9725,84 @@ static bool is_dp_capable_without_timing_msa(struct dc *dc,
>  
>  	return capable;
>  }
> +
> +static bool parse_edid_cea(struct amdgpu_dm_connector *aconnector,
> +		uint8_t *edid_ext, int len,
> +		struct amdgpu_hdmi_vsdb_info *vsdb_info)
> +{
> +	int i;
> +	struct amdgpu_device *adev = drm_to_adev(aconnector->base.dev);
> +	struct dc *dc = adev->dm.dc;
> +
> +	/* send extension block to DMCU for parsing */
> +	for (i = 0; i < len; i += 8) {
> +		bool res;
> +		int offset;
> +
> +		/* send 8 bytes a time */
> +		if (!dc_edid_parser_send_cea(dc, i, len, &edid_ext[i], 8))
> +			return false;
> +
> +		if (i+8 == len) {
> +			/* EDID block sent completed, expect result */
> +			int version, min_rate, max_rate;
> +
> +			res = dc_edid_parser_recv_amd_vsdb(dc, &version, &min_rate, &max_rate);
> +			if (res) {
> +				/* amd vsdb found */
> +				vsdb_info->freesync_supported = 1;
> +				vsdb_info->amd_vsdb_version = version;
> +				vsdb_info->min_refresh_rate_hz = min_rate;
> +				vsdb_info->max_refresh_rate_hz = max_rate;
> +				return true;
> +			}
> +			/* not amd vsdb */
> +			return false;
> +		}
> +
> +		/* check for ack*/
> +		res = dc_edid_parser_recv_cea_ack(dc, &offset);
> +		if (!res)
> +			return false;
> +	}
> +
> +	return false;
> +}
> +
> +static bool parse_hdmi_amd_vsdb(struct amdgpu_dm_connector *aconnector,
> +		struct edid *edid, struct amdgpu_hdmi_vsdb_info *vsdb_info)
> +{
> +	uint8_t *edid_ext = NULL;
> +	int i;
> +	bool valid_vsdb_found = false;
> +
> +	/*----- drm_find_cea_extension() -----*/
> +	/* No EDID or EDID extensions */
> +	if (edid == NULL || edid->extensions == 0)
> +		return false;
> +
> +	/* Find CEA extension */
> +	for (i = 0; i < edid->extensions; i++) {
> +		edid_ext = (uint8_t *)edid + EDID_LENGTH * (i + 1);
> +		if (edid_ext[0] == CEA_EXT)
> +			break;
> +	}
> +
> +	if (i == edid->extensions)
> +		return false;
> +
> +	/*----- cea_db_offsets() -----*/
> +	if (edid_ext[0] != CEA_EXT)
> +		return false;
> +
> +	valid_vsdb_found = parse_edid_cea(aconnector, edid_ext, EDID_LENGTH, vsdb_info);
> +	return valid_vsdb_found;
> +}
> +
>  void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
>  					struct edid *edid)
>  {
>  	int i;
> -	bool edid_check_required;
>  	struct detailed_timing *timing;
>  	struct detailed_non_pixel *data;
>  	struct detailed_data_monitor_range *range;
> @@ -9733,6 +9813,8 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
>  	struct drm_device *dev = connector->dev;
>  	struct amdgpu_device *adev = drm_to_adev(dev);
>  	bool freesync_capable = false;
> +	struct amdgpu_hdmi_vsdb_info vsdb_info = {0};
> +	bool hdmi_valid_vsdb_found = false;
>  
>  	if (!connector->state) {
>  		DRM_ERROR("%s - Connector has no state", __func__);
> @@ -9751,60 +9833,75 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
>  
>  	dm_con_state = to_dm_connector_state(connector->state);
>  
> -	edid_check_required = false;
>  	if (!amdgpu_dm_connector->dc_sink) {
>  		DRM_ERROR("dc_sink NULL, could not add free_sync module.\n");
>  		goto update;
>  	}
>  	if (!adev->dm.freesync_module)
>  		goto update;
> -	/*
> -	 * if edid non zero restrict freesync only for dp and edp
> -	 */
> -	if (edid) {
> -		if (amdgpu_dm_connector->dc_sink->sink_signal == SIGNAL_TYPE_DISPLAY_PORT
> -			|| amdgpu_dm_connector->dc_sink->sink_signal == SIGNAL_TYPE_EDP) {
> +
> +
> +	if (amdgpu_dm_connector->dc_sink->sink_signal == SIGNAL_TYPE_DISPLAY_PORT
> +		|| amdgpu_dm_connector->dc_sink->sink_signal == SIGNAL_TYPE_EDP) {
> +		bool edid_check_required = false;
> +
> +		if (edid) {
>  			edid_check_required = is_dp_capable_without_timing_msa(
>  						adev->dm.dc,
>  						amdgpu_dm_connector);
>  		}
> -	}
> -	if (edid_check_required == true && (edid->version > 1 ||
> -	   (edid->version == 1 && edid->revision > 1))) {
> -		for (i = 0; i < 4; i++) {
>  
> -			timing	= &edid->detailed_timings[i];
> -			data	= &timing->data.other_data;
> -			range	= &data->data.range;
> -			/*
> -			 * Check if monitor has continuous frequency mode
> -			 */
> -			if (data->type != EDID_DETAIL_MONITOR_RANGE)
> -				continue;
> -			/*
> -			 * Check for flag range limits only. If flag == 1 then
> -			 * no additional timing information provided.
> -			 * Default GTF, GTF Secondary curve and CVT are not
> -			 * supported
> -			 */
> -			if (range->flags != 1)
> -				continue;
> +		if (edid_check_required == true && (edid->version > 1 ||
> +		   (edid->version == 1 && edid->revision > 1))) {
> +			for (i = 0; i < 4; i++) {
> +
> +				timing	= &edid->detailed_timings[i];
> +				data	= &timing->data.other_data;
> +				range	= &data->data.range;
> +				/*
> +				 * Check if monitor has continuous frequency mode
> +				 */
> +				if (data->type != EDID_DETAIL_MONITOR_RANGE)
> +					continue;
> +				/*
> +				 * Check for flag range limits only. If flag == 1 then
> +				 * no additional timing information provided.
> +				 * Default GTF, GTF Secondary curve and CVT are not
> +				 * supported
> +				 */
> +				if (range->flags != 1)
> +					continue;
>  
> -			amdgpu_dm_connector->min_vfreq = range->min_vfreq;
> -			amdgpu_dm_connector->max_vfreq = range->max_vfreq;
> -			amdgpu_dm_connector->pixel_clock_mhz =
> -				range->pixel_clock_mhz * 10;
> +				amdgpu_dm_connector->min_vfreq = range->min_vfreq;
> +				amdgpu_dm_connector->max_vfreq = range->max_vfreq;
> +				amdgpu_dm_connector->pixel_clock_mhz =
> +					range->pixel_clock_mhz * 10;
>  
> -			connector->display_info.monitor_range.min_vfreq = range->min_vfreq;
> -			connector->display_info.monitor_range.max_vfreq = range->max_vfreq;
> +				connector->display_info.monitor_range.min_vfreq = range->min_vfreq;
> +				connector->display_info.monitor_range.max_vfreq = range->max_vfreq;
>  
> -			break;
> -		}
> +				break;
> +			}
>  
> -		if (amdgpu_dm_connector->max_vfreq -
> -		    amdgpu_dm_connector->min_vfreq > 10) {
> +			if (amdgpu_dm_connector->max_vfreq -
> +			    amdgpu_dm_connector->min_vfreq > 10) {
>  
> -			freesync_capable = true;
> +				freesync_capable = true;
> +			}
> +		}
> +	} else if (edid && amdgpu_dm_connector->dc_sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A) {
> +		hdmi_valid_vsdb_found = parse_hdmi_amd_vsdb(amdgpu_dm_connector, edid, &vsdb_info);
> +		if (hdmi_valid_vsdb_found && vsdb_info.freesync_supported) {
> +			timing  = &edid->detailed_timings[i];


This variable is uninitialized, as reported by clang:

$ make -skj"$(nproc)" CC=clang allyesconfig drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.o
drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:9811:38: warning: variable 'i' is uninitialized when used here [-Wuninitialized]
                        timing  = &edid->detailed_timings[i];
                                                          ^
drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:9721:7: note: initialize the variable 'i' to silence this warning
        int i;
             ^
              = 0
1 warning generated.

Cheers,
Nathan

> +			data    = &timing->data.other_data;
> +
> +			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;
>  		}
>  	}
>  
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index 38bc0f88b29c..5f9950fd216c 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -475,6 +475,14 @@ struct dm_connector_state {
>  	uint64_t pbn;
>  };
>  
> +struct amdgpu_hdmi_vsdb_info {
> +	unsigned int amd_vsdb_version;		/* VSDB version, should be used to determine which VSIF to send */
> +	bool freesync_supported;		/* FreeSync Supported */
> +	unsigned int min_refresh_rate_hz;	/* FreeSync Minimum Refresh Rate in Hz */
> +	unsigned int max_refresh_rate_hz;	/* FreeSync Maximum Refresh Rate in Hz */
> +};
> +
> +
>  #define to_dm_connector_state(x)\
>  	container_of((x), struct dm_connector_state, base)
>  
> -- 
> 2.17.1
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx