Re: [PATCH 3/3] drm/amd/display: Skip modeset for front porch change

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

 



On Mon, 2021-01-04 at 11:16 -0500, Kazlauskas, Nicholas wrote:
> On 2020-12-09 9:45 p.m., Aurabindo Pillai wrote:
> > [Why&How]
> > Inorder to enable freesync video mode, driver adds extra
> > modes based on preferred modes for common freesync frame rates.
> > When commiting these mode changes, a full modeset is not needed.
> > If the change in only in the front porch timing value, skip full
> > modeset and continue using the same stream.
> > 
> > Signed-off-by: Aurabindo Pillai <aurabindo.pillai@xxxxxxx>
> > ---
> >   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 169
> > ++++++++++++++++--
> >   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   1 +
> >   2 files changed, 153 insertions(+), 17 deletions(-)
> > 
> > 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 f699a3d41cad..c8c72887906a 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -217,6 +217,9 @@ static bool amdgpu_dm_psr_disable_all(struct
> > amdgpu_display_manager *dm);
> >   static const struct drm_format_info *
> >   amd_get_format_info(const struct drm_mode_fb_cmd2 *cmd);
> >   
> > +static bool
> > +is_timing_unchanged_for_freesync(struct drm_crtc_state
> > *old_crtc_state,
> > +                                struct drm_crtc_state
> > *new_crtc_state);
> >   /*
> >    * dm_vblank_get_counter
> >    *
> > @@ -5096,8 +5099,11 @@ copy_crtc_timing_for_drm_display_mode(const
> > struct drm_display_mode *src_mode,
> >   static void
> >   decide_crtc_timing_for_drm_display_mode(struct drm_display_mode
> > *drm_mode,
> >                                         const struct
> > drm_display_mode *native_mode,
> > -                                       bool scale_enabled)
> > +                                       bool scale_enabled, bool
> > fs_mode)
> >   {
> > +       if (fs_mode)
> > +               return;
> > +
> >         if (scale_enabled) {
> >                 copy_crtc_timing_for_drm_display_mode(native_mode,
> > drm_mode);
> >         } else if (native_mode->clock == drm_mode->clock &&
> > @@ -5241,6 +5247,24 @@ get_highest_freesync_mode(struct
> > amdgpu_dm_connector *aconnector,
> >         return m_high;
> >   }
> >   
> > +static bool is_freesync_video_mode(struct drm_display_mode *mode,
> > +                                  struct amdgpu_dm_connector
> > *aconnector)
> > +{
> > +       struct drm_display_mode *high_mode;
> > +
> > +       high_mode = get_highest_freesync_mode(aconnector, false);
> > +       if (!high_mode)
> > +               return false;
> > +
> > +       if (high_mode->clock == 0 ||
> > +           high_mode->hdisplay != mode->hdisplay ||
> > +           high_mode->clock != mode->clock ||
> > +           !mode)
> > +               return false;
> > +       else
> > +               return true;
> > +}
> > +
> 
> Need to check that the other parameters are the same:
> - hsync_start
> - hsync_end
> - htotal
> - hskew
> - vdisplay
> - vscan
> 
> etc.

Will do.

> 
> >   static struct dc_stream_state *
> >   create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
> >                        const struct drm_display_mode *drm_mode,
> > @@ -5253,17 +5277,21 @@ create_stream_for_sink(struct
> > amdgpu_dm_connector *aconnector,
> >         const struct drm_connector_state *con_state =
> >                 dm_state ? &dm_state->base : NULL;
> >         struct dc_stream_state *stream = NULL;
> > -       struct drm_display_mode mode = *drm_mode;
> > +       struct drm_display_mode saved_mode, mode = *drm_mode;
> > +       struct drm_display_mode *freesync_mode = NULL;
> >         bool native_mode_found = false;
> >         bool scale = dm_state ? (dm_state->scaling != RMX_OFF) :
> > false;
> >         int mode_refresh;
> >         int preferred_refresh = 0;
> > +       bool is_fs_vid_mode = 0;
> >   #if defined(CONFIG_DRM_AMD_DC_DCN)
> >         struct dsc_dec_dpcd_caps dsc_caps;
> >   #endif
> >         uint32_t link_bandwidth_kbps;
> > -
> >         struct dc_sink *sink = NULL;
> > +
> > +       memset(&saved_mode, 0, sizeof(struct drm_display_mode));
> > +
> >         if (aconnector == NULL) {
> >                 DRM_ERROR("aconnector is NULL!\n");
> >                 return stream;
> > @@ -5316,20 +5344,33 @@ create_stream_for_sink(struct
> > amdgpu_dm_connector *aconnector,
> >                  */
> >                 DRM_DEBUG_DRIVER("No preferred mode found\n");
> >         } else {
> > +               is_fs_vid_mode = is_freesync_video_mode(&mode,
> > aconnector);
> > +               if (is_fs_vid_mode) {
> > +                       freesync_mode =
> > get_highest_freesync_mode(aconnector, false);
> > +                       if (freesync_mode) {
> > +                               saved_mode = mode;
> > +                               mode = *freesync_mode;
> > +                       }
> > +               }
> > +
> >                 decide_crtc_timing_for_drm_display_mode(
> >                                 &mode, preferred_mode,
> > -                               dm_state ? (dm_state->scaling !=
> > RMX_OFF) : false);
> > +                               dm_state ? (dm_state->scaling !=
> > RMX_OFF) : false,
> > +                               freesync_mode ? true : false);
> 
> I don't think we need an additional flag here - scaling/freesync
> behave 
> the same, maybe just rename the variable in the function.

This was fixed in v3. Will send out a v4 with above change and the
suggestion from Alex to change the module parameter name.

> 
> Regards,
> Nicholas Kazlauskas
> 
> >                 preferred_refresh =
> > drm_mode_vrefresh(preferred_mode);
> >         }
> >   
> >         if (!dm_state)
> >                 drm_mode_set_crtcinfo(&mode, 0);
> >   
> > -       /*
> > +       if (dm_state && is_fs_vid_mode && freesync_mode)
> > +               drm_mode_set_crtcinfo(&saved_mode, 0);
> > +
> > +       /*
> >         * If scaling is enabled and refresh rate didn't change
> >         * we copy the vic and polarities of the old timings
> >         */
> > -       if (!scale || mode_refresh != preferred_refresh)
> > +       if (!(scale && freesync_mode) || mode_refresh !=
> > preferred_refresh)
> >                 fill_stream_properties_from_drm_display_mode(stream
> > ,
> >                         &mode, &aconnector->base, con_state, NULL,
> > requested_bpc);
> >         else
> > @@ -7881,13 +7922,29 @@ static void update_stream_irq_parameters(
> >         if (new_crtc_state->vrr_supported &&
> >             config.min_refresh_in_uhz &&
> >             config.max_refresh_in_uhz) {
> > +               /*
> > +                * if freesync compatible mode was set,
> > config.state will be set
> > +                * in atomic check
> > +                */
> > +               if (config.state == VRR_STATE_ACTIVE_FIXED &&
> > +                   config.fixed_refresh_in_uhz &&
> > config.max_refresh_in_uhz &&
> > +                   config.min_refresh_in_uhz &&
> > +                  
> > (!drm_atomic_crtc_needs_modeset(&new_crtc_state->base) ||
> > +                    new_crtc_state->freesync_video_mode)) {
> > +                       vrr_params.max_refresh_in_uhz =
> > config.max_refresh_in_uhz;
> > +                       vrr_params.min_refresh_in_uhz =
> > config.min_refresh_in_uhz;
> > +                       vrr_params.fixed_refresh_in_uhz =
> > config.fixed_refresh_in_uhz;
> > +                       vrr_params.state = VRR_STATE_ACTIVE_FIXED;
> > +                       goto out;
> > +               }
> > +
> >                 config.state = new_crtc_state->base.vrr_enabled ?
> >                         VRR_STATE_ACTIVE_VARIABLE :
> >                         VRR_STATE_INACTIVE;
> > -       } else {
> > +       } else
> >                 config.state = VRR_STATE_UNSUPPORTED;
> > -       }
> >   
> > +out:
> >         mod_freesync_build_vrr_params(dm->freesync_module,
> >                                       new_stream,
> >                                       &config, &vrr_params);
> > @@ -8205,7 +8262,9 @@ static void amdgpu_dm_commit_planes(struct
> > drm_atomic_state *state,
> >                  * as part of commit.
> >                  */
> >                 if (amdgpu_dm_vrr_active(dm_old_crtc_state) !=
> > -                   amdgpu_dm_vrr_active(acrtc_state)) {
> > +                   amdgpu_dm_vrr_active(acrtc_state) ||
> > +                   acrtc_state->freesync_config.state ==
> > VRR_STATE_ACTIVE_FIXED ||
> > +                   acrtc_state->freesync_video_mode) {
> >                         spin_lock_irqsave(&pcrtc->dev->event_lock,
> > flags);
> >                         dc_stream_adjust_vmin_vmax(
> >                                 dm->dc, acrtc_state->stream,
> > @@ -8896,6 +8955,7 @@ static void get_freesync_config_for_crtc(
> >                         to_amdgpu_dm_connector(new_con_state-
> > >base.connector);
> >         struct drm_display_mode *mode = &new_crtc_state->base.mode;
> >         int vrefresh = drm_mode_vrefresh(mode);
> > +       bool fs_vid_mode = false;
> >   
> >         new_crtc_state->vrr_supported = new_con_state-
> > >freesync_capable &&
> >                                         vrefresh >= aconnector-
> > >min_vfreq &&
> > @@ -8903,17 +8963,26 @@ static void get_freesync_config_for_crtc(
> >   
> >         if (new_crtc_state->vrr_supported) {
> >                 new_crtc_state->stream->ignore_msa_timing_param =
> > true;
> > -               config.state = new_crtc_state->base.vrr_enabled ?
> > -                               VRR_STATE_ACTIVE_VARIABLE :
> > -                               VRR_STATE_INACTIVE;
> > -               config.min_refresh_in_uhz =
> > -                               aconnector->min_vfreq * 1000000;
> > -               config.max_refresh_in_uhz =
> > -                               aconnector->max_vfreq * 1000000;
> > +               fs_vid_mode = new_crtc_state->freesync_config.state
> > == VRR_STATE_ACTIVE_FIXED ||
> > +                       new_crtc_state->freesync_video_mode;
> > +
> > +               config.min_refresh_in_uhz = aconnector->min_vfreq *
> > 1000000;
> > +               config.max_refresh_in_uhz = aconnector->max_vfreq *
> > 1000000;
> >                 config.vsif_supported = true;
> >                 config.btr = true;
> > -       }
> >   
> > +               if (fs_vid_mode) {
> > +                       config.state = VRR_STATE_ACTIVE_FIXED;
> > +                       config.fixed_refresh_in_uhz =
> > new_crtc_state->freesync_config.fixed_refresh_in_uhz;
> > +                       goto out;
> > +               }
> > +               else if (new_crtc_state->base.vrr_enabled &&
> > !fs_vid_mode)
> > +                       config.state = VRR_STATE_ACTIVE_VARIABLE;
> > +               else
> > +                       config.state = VRR_STATE_INACTIVE;
> > +
> > +       }
> > +out:
> >         new_crtc_state->freesync_config = config;
> >   }
> >   
> > @@ -8926,6 +8995,51 @@ static void reset_freesync_config_for_crtc(
> >                sizeof(new_crtc_state->vrr_infopacket));
> >   }
> >   
> > +static bool
> > +is_timing_unchanged_for_freesync(struct drm_crtc_state
> > *old_crtc_state,
> > +                                struct drm_crtc_state
> > *new_crtc_state)
> > +{
> > +       struct drm_display_mode old_mode, new_mode;
> > +
> > +       if (!old_crtc_state || !new_crtc_state)
> > +               return false;
> > +
> > +       old_mode = old_crtc_state->mode;
> > +       new_mode = new_crtc_state->mode;
> > +
> > +       if (old_mode.clock       == new_mode.clock &&
> > +           old_mode.hdisplay    == new_mode.hdisplay &&
> > +           old_mode.vdisplay    == new_mode.vdisplay &&
> > +           old_mode.htotal      == new_mode.htotal &&
> > +           old_mode.vtotal      != new_mode.vtotal &&
> > +           old_mode.hsync_start == new_mode.hsync_start &&
> > +           old_mode.vsync_start != new_mode.vsync_start &&
> > +           old_mode.hsync_end   == new_mode.hsync_end &&
> > +           old_mode.vsync_end   != new_mode.vsync_end &&
> > +           old_mode.hskew       == new_mode.hskew &&
> > +           old_mode.vscan       == new_mode.vscan &&
> > +           (old_mode.vsync_end - old_mode.vsync_start) ==
> > +           (new_mode.vsync_end - new_mode.vsync_start))
> > +               return true;
> > +
> > +       return false;
> > +}
> > +
> > +static void set_freesync_fixed_config(struct dm_crtc_state
> > *dm_new_crtc_state) {
> > +       uint64_t num, den, res;
> > +       struct drm_crtc_state *new_crtc_state = &dm_new_crtc_state-
> > >base;
> > +
> > +       dm_new_crtc_state->freesync_config.state =
> > VRR_STATE_ACTIVE_FIXED;
> > +
> > +       num = (unsigned long long)new_crtc_state->mode.clock * 1000
> > * 1000000;
> > +       den = (unsigned long long)new_crtc_state->mode.htotal *
> > +             (unsigned long long)new_crtc_state->mode.vtotal;
> > +
> > +       res = div_u64(num, den);
> > +       dm_new_crtc_state->freesync_config.fixed_refresh_in_uhz =
> > res;
> > +       dm_new_crtc_state->freesync_video_mode = true;
> > +}
> > +
> >   static int dm_update_crtc_state(struct amdgpu_display_manager
> > *dm,
> >                                 struct drm_atomic_state *state,
> >                                 struct drm_crtc *crtc,
> > @@ -9016,6 +9130,11 @@ static int dm_update_crtc_state(struct
> > amdgpu_display_manager *dm,
> >                  * TODO: Refactor this function to allow this check
> > to work
> >                  * in all conditions.
> >                  */
> > +               if (dm_new_crtc_state->stream &&
> > +                  
> > is_timing_unchanged_for_freesync(new_crtc_state, old_crtc_state) &&
> > +                   amdgpu_exp_freesync_vid_mode)
> > +                       goto skip_modeset;
> > +
> >                 if (dm_new_crtc_state->stream &&
> >                     dc_is_stream_unchanged(new_stream,
> > dm_old_crtc_state->stream) &&
> >                     dc_is_stream_scaling_unchanged(new_stream,
> > dm_old_crtc_state->stream)) {
> > @@ -9047,6 +9166,22 @@ static int dm_update_crtc_state(struct
> > amdgpu_display_manager *dm,
> >                 if (!dm_old_crtc_state->stream)
> >                         goto skip_modeset;
> >   
> > +               if (dm_new_crtc_state->stream &&
> > +                  
> > is_timing_unchanged_for_freesync(new_crtc_state, old_crtc_state) &&
> > +                   amdgpu_exp_freesync_vid_mode) {
> > +                       new_crtc_state->mode_changed = false;
> > +                       DRM_DEBUG_DRIVER(
> > +                               "Mode change not required for front
> > porch change, "
> > +                               "setting mode_changed to %d",
> > +                               new_crtc_state->mode_changed);
> > +
> > +                       set_freesync_fixed_config(dm_new_crtc_state
> > );
> > +
> > +                       goto skip_modeset;
> > +               } else if (aconnector &&
> > +                          is_freesync_video_mode(&new_crtc_state-
> > >mode, aconnector))
> > +                       set_freesync_fixed_config(dm_new_crtc_state
> > );
> > +
> >                 ret = dm_atomic_get_state(state, &dm_state);
> >                 if (ret)
> >                         goto fail;
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> > index 251af783f6b1..28f2d8c9b260 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> > @@ -453,6 +453,7 @@ struct dm_crtc_state {
> >   
> >         bool freesync_timing_changed;
> >         bool freesync_vrr_info_changed;
> > +       bool freesync_video_mode;
> >   
> >         bool dsc_force_changed;
> >         bool vrr_supported;
> > 
> 


_______________________________________________
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