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);
> + /*
> + * 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 ?
> + */
> + 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
> +
> + 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
