On 2020-12-11 12:54 a.m., Shashank Sharma wrote:
On 11/12/20 12:18 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@xxxxxxx>
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 167 ++++++++++++++++++
1 file changed, 167 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..d15453b400d2 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,54 @@ 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_refresh_rate_mode(struct amdgpu_dm_connector *aconnector,
+ bool use_probed_modes)
+{
+ struct drm_display_mode *m, *m_high = NULL;
I would prefer m_high to be renamed as m_pref, indicating it's the preferred mode
+ u16 current_refresh, highest_refresh;
+ struct list_head *list_head = use_probed_modes ?
+ &aconnector->base.probed_modes :
+ &aconnector->base.modes;
+ /* Find the preferred mode */
+ list_for_each_entry (m, list_head, head) {
+ if (!(m->type & DRM_MODE_TYPE_PREFERRED))
+ continue;
+
+ m_high = m;
+ 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
+ highest_refresh = drm_mode_vrefresh(m_high);
+ }
+
Optional cleanup suggested below makes code more readable:
/* Find the preferred mode */
list_for_each_entry (m, list_head, head) {
if (m->type & DRM_MODE_TYPE_PREFERRED) {
m_pref = m;
break;
}
}
if (!m_pref) {
/* Probably an EDID with no preferred mode. Fallback to first entry */
m_pref = list_first_entry_or_null(&aconnector->base.modes,
struct drm_display_mode, head);
if (!m_pref) {
DRM_DEBUG_DRIVER("No preferred mode found in EDID\n");
return NULL;
}
}
highest_refresh = drm_mode_vrefresh(m_pref);
Agreed with this cleanup - naming is confusing as is.
+ /*
+ * 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 == m_high->hdisplay &&
+ m->vdisplay == m_high->vdisplay &&
+ highest_refresh < current_refresh) {
+ highest_refresh = current_refresh;
+ 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 +7054,124 @@ 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 (drm_mode_equal(m, mode))
+ return true;
+ }
+
+ return false;
+}
+
+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;
num, den, target_vtotal, target_vtotal_diff can go inside the list_for_each_entry() loop;
+
+ /* 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
I missed this last time, but why don't we have 60 fps here ? Most preferred modes are 60fps in general. Or was it missed in comment only ?
It should be in this list, but that brings up another point that needs
to be addressed - if the mode already exists in the modelist then we
should skip adding a duplicate mode.
+ */
+ 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_refresh_rate_mode(aconnector, true);
+ if (!m_save)
+ goto out;
+
+ list_for_each_entry (m, &aconnector->base.probed_modes, head) {
+ if (m != m_save)
+ continue;
Now when I think about it again,
- we already went through the list (aconnector->base.probed_modes) in function get_highest_refresh_rate_mode(), and got the m_save mode.
- then we are again going through the same list, to find m = m_save, why ? Am I missing something or we can use m_save directly here
some thing like:
m_save = get_highest_refresh_rate_mode(aconnector, true);
if (m_save) {
for (i = 0; i < sizeof(neededrates) / sizeof(uint32_t); i++) {
// do the same thing here
}
}
This would save another iteration through the probed_modes.
+
+ for (i = 0; i < sizeof(neededrates) / sizeof(uint32_t); i++) {
ARRAY_SIZE() here; also instead of calculating it for every iteration, we can use a local variable u8 len = ARRAY_SIZE(neededrates); Not sure if compiler will do that for us though ;-)
+ 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 */
+ 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;
Just FYI, All the DMT modes and CEA_MODES from EDID also use this flag, so even though it's the right flag to set, it's not unique enough to identify it as FS mode.
- Shashank
I don't think we should even be bothering trying to identify whether the
mode was a FS mode or not since the front porch modeset skip
optimization can apply generically (at least as an experimental opt-in
feature for now).
Regards,
Nicholas Kazlauskas
+
+ if (!is_duplicate_mode(aconnector, new_mode)) {
+ 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 (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 +7187,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);
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
