Re: [PATCH 2/3] drm/amd/display: Add freesync video modes based on preferred modes

[Shashank Sharma <shashank.sharma@amd.com>]

[-- Attachment #1.1: Type: text/plain, Size: 8633 bytes --]

Hello Aurabindo,

On 10/12/20 8:15 am, Aurabindo Pillai wrote:
> [Why&How]
> If experimental freesync video mode module parameter is enabled, add
> few extra display modes into the driver's mode list corresponding to common
> video frame rates. When userspace sets these modes, no modeset will be
> performed (if current mode was one of freesync modes or the base freesync mode
> based off which timings have been generated for the rest of the freesync modes)
> since these modes only differ from the base mode with front porch timing.
>
> Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 197 ++++++++++++++++++
>  1 file changed, 197 insertions(+)
>
> 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 fbff8d693e03..f699a3d41cad 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -5178,6 +5178,69 @@ static void dm_enable_per_frame_crtc_master_sync(struct dc_state *context)
>  	set_master_stream(context->streams, context->stream_count);
>  }
>  
> +static struct drm_display_mode *
> +get_highest_freesync_mode(struct amdgpu_dm_connector *aconnector,
> +			  bool use_probed_modes)
the function name is confusing, this function has nothing to do with freesync. Probably drop the 'freesync' from the name, get_highest_refresh_rate_mode or similar ?
> +{
> +	struct drm_display_mode *m, *m_high = NULL;
> +	u16 current_refresh, highest_refresh, preferred_mode_h, preferred_mode_w;
> +	struct list_head *list_head = use_probed_modes ?
> +						    &aconnector->base.probed_modes :
> +						    &aconnector->base.modes;
> +
> +	/*
> +	 * Find the preferred mode
> +	 */
Single line comment should be in /* */
> +
> +	list_for_each_entry (m, list_head, head) {
> +		if (!(m->type & DRM_MODE_TYPE_PREFERRED))
> +			continue;
> +
> +		m_high = m;
> +		preferred_mode_h = m_high->hdisplay;
> +		preferred_mode_w = m_high->vdisplay;
> +		highest_refresh = drm_mode_vrefresh(m_high);
> +		break;
> +	}
> +
> +	if (!m_high) {
> +
> +		/*
> +		 * Probably an EDID with no preferred mode.
> +		 * Fallback to first entry;
> +		 */
> +		m_high = list_first_entry_or_null(&aconnector->base.modes,
> +						  struct drm_display_mode, head);
> +		if (!m_high)
> +			return NULL;
> +		else {
> +			preferred_mode_h = m_high->hdisplay;
> +			preferred_mode_w = m_high->vdisplay;
> +			highest_refresh = drm_mode_vrefresh(m_high);
> +		}
> +	}
> +
> +	/*
> +	 * Find the mode with highest refresh rate with same resolution.
> +	 * For some monitors, preferred mode is not the mode with highest
> +	 * supported refresh rate.
> +	 */
> +	list_for_each_entry (m, list_head, head) {
> +		current_refresh  = drm_mode_vrefresh(m);
> +
> +		if (m->hdisplay == preferred_mode_h &&
> +		    m->vdisplay == preferred_mode_w &&
> +		    highest_refresh < current_refresh) {
> +			highest_refresh = current_refresh;
> +			preferred_mode_h = m->hdisplay;
> +			preferred_mode_w = m->vdisplay;
why do we need to save preferred_mode_h and w variables at all ? I thought the idea here was to get the mode with highest refresh rate, but same resolution, which is indicated in the if (cond) above also. I think we just need highest refresh rate isn't ?
> +			m_high = m;
> +		}
> +	}
> +
> +	return m_high;
> +}
> +
>  static struct dc_stream_state *
>  create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
>  		       const struct drm_display_mode *drm_mode,
> @@ -7006,6 +7069,139 @@ static void amdgpu_dm_connector_ddc_get_modes(struct drm_connector *connector,
>  	}
>  }
>  
> +static bool is_duplicate_mode(struct amdgpu_dm_connector *aconnector,
> +			      struct drm_display_mode *mode)
> +{
> +	struct drm_display_mode *m;
> +
> +	list_for_each_entry (m, &aconnector->base.probed_modes, head) {
> +		if (m->clock == mode->clock &&
> +		    m->htotal == mode->htotal &&
> +		    m->vtotal == mode->vtotal &&
> +		    m->hdisplay == mode->hdisplay &&
> +		    m->vdisplay == mode->vdisplay &&
> +		    m->hsync_start == mode->hsync_start &&
> +		    m->vsync_start == mode->vsync_start &&
> +		    m->vsync_end == mode->vsync_end &&
> +		    m->hsync_end == mode->hsync_end)
> +			return true;
> +	}
> +
> +	return false;
Why not usedrm_mode_equal() instead ?
> +}
> +
> +static uint add_fs_modes(struct amdgpu_dm_connector *aconnector,
> +			 struct detailed_data_monitor_range *range)
> +{
> +	const struct drm_display_mode *m, *m_save;
> +	struct drm_display_mode *new_mode;
> +	uint i;
> +	uint64_t target_vtotal, target_vtotal_diff;
> +	uint32_t new_modes_count = 0;
> +	uint64_t num, den;
> +
> +	/* Standard FPS values
> +	 *
> +	 * 23.976 - TV/NTSC
> +	 * 24	  - Cinema
> +	 * 25     - TV/PAL
> +	 * 29.97  - TV/NTSC
> +	 * 30     - TV/NTSC
> +	 * 48	  - Cinema HFR
> +	 * 50	  - TV/PAL
> +	 */
> +	const uint32_t neededrates[] = { 23976, 24000, 25000, 29970,
> +					 30000, 48000, 50000, 72000, 96000 };
> +
> +	/*
> +	 * Find mode with highest refresh rate with the same resolution
> +	 * as the preferred mode. Some monitors report a preferred mode
> +	 * with lower resolution than the highest refresh rate supported.
> +	 */
> +
> +	m_save = get_highest_freesync_mode(aconnector, true);
A NULL return check here to bypass the whole routine below ?
> +
> +	list_for_each_entry (m, &aconnector->base.probed_modes, head) {
> +		if (m != m_save)
> +			continue;
> +
> +		for (i = 0; i < sizeof(neededrates) / sizeof(uint32_t); i++) {
> +			if (drm_mode_vrefresh(m) * 1000 < neededrates[i])
> +				continue;
> +
> +			if (neededrates[i] < range->min_vfreq * 1000)
> +				continue;
> +
> +			num = (unsigned long long)m->clock * 1000 * 1000;
> +			den = neededrates[i] * (unsigned long long)m->htotal;
> +			target_vtotal = div_u64(num, den);
> +			target_vtotal_diff = target_vtotal - m->vtotal;
> +
> +			/*
> +			 * Check for illegal modes
> +			 */
Same here for single line comment
> +			if (m->vsync_start + target_vtotal_diff < m->vdisplay ||
> +			    m->vsync_end + target_vtotal_diff < m->vsync_start ||
> +			    m->vtotal + target_vtotal_diff < m->vsync_end)
> +				continue;
> +
> +			new_mode = drm_mode_duplicate(aconnector->base.dev, m);
> +			if (!new_mode)
> +				goto out;
> +
> +			new_mode->vtotal += (u16)target_vtotal_diff;
> +			new_mode->vsync_start += (u16)target_vtotal_diff;
> +			new_mode->vsync_end += (u16)target_vtotal_diff;
> +			new_mode->type &= ~DRM_MODE_TYPE_PREFERRED;
> +			new_mode->type |= DRM_MODE_TYPE_DRIVER;
> +			strcat(new_mode->name, "_FSV\0");
> +
> +			if (!is_duplicate_mode(aconnector, new_mode)) {
or drm_mode_equal here ?
> +				drm_mode_probed_add(&aconnector->base, new_mode);
> +				new_modes_count += 1;
> +			} else
> +				drm_mode_destroy(aconnector->base.dev, new_mode);
> +		}
> +	}
> + out:
> +	return new_modes_count;
> +}
> +
> +static void amdgpu_dm_connector_add_freesync_modes(struct drm_connector *connector,
> +						   struct edid *edid)
> +{
> +	uint8_t i;
> +	struct detailed_timing *timing;
> +	struct detailed_non_pixel *data;
> +	struct detailed_data_monitor_range *range;
> +	struct amdgpu_dm_connector *amdgpu_dm_connector =
> +		to_amdgpu_dm_connector(connector);
> +
> +	if (!(amdgpu_exp_freesync_vid_mode && edid))
> +		return;
> +
> +	if (!(amdgpu_dm_connector->dc_sink->sink_signal == SIGNAL_TYPE_EDP ||

do we want the freesync infra to be available for eDP also ?

- Shashank

> +	      amdgpu_dm_connector->dc_sink->sink_signal == SIGNAL_TYPE_DISPLAY_PORT ||
> +	      amdgpu_dm_connector->dc_sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A))
> +		return;
> +
> +	if (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 &&
> +			    range->max_vfreq - range->min_vfreq > 10) {
> +				amdgpu_dm_connector->num_modes += add_fs_modes(amdgpu_dm_connector, range);
> +				break;
> +			}
> +		}
> +	}
> +}
> +
>  static int amdgpu_dm_connector_get_modes(struct drm_connector *connector)
>  {
>  	struct amdgpu_dm_connector *amdgpu_dm_connector =
> @@ -7021,6 +7217,7 @@ static int amdgpu_dm_connector_get_modes(struct drm_connector *connector)
>  	} else {
>  		amdgpu_dm_connector_ddc_get_modes(connector, edid);
>  		amdgpu_dm_connector_add_common_modes(encoder, connector);
> +		amdgpu_dm_connector_add_freesync_modes(connector, edid);
>  	}
>  	amdgpu_dm_fbc_init(connector);
>  

[-- Attachment #1.2: Type: text/html, Size: 10818 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx