[PATCH v2 2/3] drm/amd/display: Add freesync video modes based on preferred modes
Kazlauskas, Nicholas
nicholas.kazlauskas at amd.com
Mon Jan 4 16:06:56 UTC 2021
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 at amd.com>
>> ---
>> .../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);
>>
More information about the amd-gfx
mailing list