Re: [PATCH v2 2/3] drm/amd/display: Add freesync video modes based on preferred modes

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@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.

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);
> > >   
> 


_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux