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

[Aurabindo Pillai <aurabindo.pillai@amd.com>]

On Tue, 2020-12-15 at 08:32 +0530, Shashank Sharma wrote:
> 
> On 15/12/20 3:50 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@amd.com>
> > Acked-by: Christian König <christian.koenig at amd.com>
> > ---
> >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 167
> > ++++++++++++++++++
> >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   2 +
> >  2 files changed, 169 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 e7ee2467eadb..c1ffd33e9d83 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -5174,6 +5174,59 @@ 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_pref = NULL;
> > +       u16 current_refresh, highest_refresh;
> > +       struct list_head *list_head = use_probed_modes ?
> > +                                                   &aconnector-
> > >base.probed_modes :
> > +                                                   &aconnector-
> > >base.modes;
> > +
> > +       if (aconnector->freesync_vid_base.clock != 0)
> > +               return &aconnector->freesync_vid_base;
> > +
> > +       /* 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_pref->hdisplay &&
> > +                   m->vdisplay == m_pref->vdisplay &&
> > +                   highest_refresh < current_refresh) {
> > +                       highest_refresh = current_refresh;
> > +                       m_pref = m;
> > +               }
> > +       }
> > +
> > +       aconnector->freesync_vid_base = *m_pref;
> > +       return m_pref;
> > +}
> > +
> >  static struct dc_stream_state *
> >  create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
> >                        const struct drm_display_mode *drm_mode,
> > @@ -6999,6 +7052,119 @@ 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;
> > +       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;
> My previous review comment about num,den, target_vtotal and
> target_vtotal_diff here was neither addressed nor explained.
> > +
> > +       /* 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
> > +        */
> My previous review comment about 60 fps here was neither addressed
> nor explained.
> 

Sorry about that, the list_for_each is gone, so I'll move it into the
for loop. I'll add the comment as well.

Thanks for the review!

--

Thanks & Regards,
Aurabindo Pillai

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