<html><head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body> Hello Aurabindo, <div class="moz-cite-prefix">On 10/12/20 8:15 am, Aurabindo Pillai wrote: </div> <blockquote type="cite" cite="mid:20201210024526.1151447-3-aurabindo.pillai@amd.com"> <pre class="moz-quote-pre" wrap="">[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 <a class="moz-txt-link-rfc2396E" href="mailto:aurabindo.pillai@amd.com"><aurabindo.pillai@amd.com></a> --- .../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)</pre> </blockquote> 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 ? <blockquote type="cite" cite="mid:20201210024526.1151447-3-aurabindo.pillai@amd.com"> <pre class="moz-quote-pre" wrap=""> +{ + 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 + */</pre> </blockquote> Single line comment should be in /* */ <blockquote type="cite" cite="mid:20201210024526.1151447-3-aurabindo.pillai@amd.com"> <pre class="moz-quote-pre" wrap=""> + + 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;</pre> </blockquote> 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 ? <blockquote type="cite" cite="mid:20201210024526.1151447-3-aurabindo.pillai@amd.com"> <pre class="moz-quote-pre" wrap=""> + 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;</pre> </blockquote> Why not use drm_mode_equal() instead ? <blockquote type="cite" cite="mid:20201210024526.1151447-3-aurabindo.pillai@amd.com"> <pre class="moz-quote-pre" wrap=""> +} + +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);</pre> </blockquote> A NULL return check here to bypass the whole routine below ? <blockquote type="cite" cite="mid:20201210024526.1151447-3-aurabindo.pillai@amd.com"> <pre class="moz-quote-pre" wrap=""> + + 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;</pre> </blockquote> <blockquote type="cite" cite="mid:20201210024526.1151447-3-aurabindo.pillai@amd.com"> <pre class="moz-quote-pre" wrap=""> + 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 + */</pre> </blockquote> Same here for single line comment <blockquote type="cite" cite="mid:20201210024526.1151447-3-aurabindo.pillai@amd.com"> <pre class="moz-quote-pre" wrap=""> + 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)) {</pre> </blockquote> or drm_mode_equal here ? <blockquote type="cite" cite="mid:20201210024526.1151447-3-aurabindo.pillai@amd.com"> <pre class="moz-quote-pre" wrap=""> + 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;</pre> </blockquote> <blockquote type="cite" cite="mid:20201210024526.1151447-3-aurabindo.pillai@amd.com"> <pre class="moz-quote-pre" wrap=""> + + if (!(amdgpu_dm_connector->dc_sink->sink_signal == SIGNAL_TYPE_EDP ||</pre> </blockquote> do we want the freesync infra to be available for eDP also ? - Shashank <blockquote type="cite" cite="mid:20201210024526.1151447-3-aurabindo.pillai@amd.com"> <pre class="moz-quote-pre" wrap=""> + 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); </pre> </blockquote> <blockquote type="cite" cite="mid:20201210024526.1151447-3-aurabindo.pillai@amd.com"> </blockquote> </body> </html>