[PATCH v2 2/3] drm/amd/display: Add freesync video modes based on preferred modes
Aurabindo Pillai
aurabindo.pillai at amd.com
Mon Jan 4 21:02:47 UTC 2021
On Mon, 2021-01-04 at 11:06 -0500, Kazlauskas, Nicholas wrote:
> 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.
This was fixed in latest revision.
>
> > > + /*
> > > + * 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 exi28310fd6dabcsts in the
> modelist then we
> should skip adding a duplicate mode.
>
This was fixed in latest revision.
> > > + */
> > > + 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