From: "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> To: Aurabindo Pillai <aurabindo.pillai@amd.com>, amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org Cc: stylon.wang@amd.com, thong.thai@amd.com, shashank.sharma@amd.com, wayne.lin@amd.com, alexander.deucher@amd.com, christian.koenig@amd.com Subject: Re: [PATCH 3/3] drm/amd/display: Skip modeset for front porch change Date: Thu, 21 Jan 2021 14:05:12 -0500 [thread overview] Message-ID: <e789fbc8-709f-41a7-1b2b-3ff328c384b1@amd.com> (raw) In-Reply-To: <20210119155029.18943-4-aurabindo.pillai@amd.com> On 2021-01-19 10:50 a.m., Aurabindo Pillai wrote: > [Why] > A seamless transition between modes can be performed if the new incoming > mode has the same timing parameters as the optimized mode on a display with a > variable vtotal min/max. > > Smooth video playback usecases can be enabled with this seamless transition by > switching to a new mode which has a refresh rate matching the video. > > [How] > Skip full modeset if userspace requested a compatible freesync mode which only > differs in the front porch timing from the current mode. > > Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com> > Acked-by: Christian König <christian.koenig@amd.com> > --- > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 233 +++++++++++++++--- > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 + > 2 files changed, 198 insertions(+), 36 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 aaef2fb528fd..d66494cdd8c8 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -213,6 +213,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 > * > @@ -4940,7 +4943,8 @@ static void fill_stream_properties_from_drm_display_mode( > const struct drm_connector *connector, > const struct drm_connector_state *connector_state, > const struct dc_stream_state *old_stream, > - int requested_bpc) > + int requested_bpc, > + bool is_in_modeset) > { > struct dc_crtc_timing *timing_out = &stream->timing; > const struct drm_display_info *info = &connector->display_info; > @@ -4995,19 +4999,28 @@ static void fill_stream_properties_from_drm_display_mode( > timing_out->hdmi_vic = hv_frame.vic; > } > > - timing_out->h_addressable = mode_in->crtc_hdisplay; > - timing_out->h_total = mode_in->crtc_htotal; > - timing_out->h_sync_width = > - mode_in->crtc_hsync_end - mode_in->crtc_hsync_start; > - timing_out->h_front_porch = > - mode_in->crtc_hsync_start - mode_in->crtc_hdisplay; > - timing_out->v_total = mode_in->crtc_vtotal; > - timing_out->v_addressable = mode_in->crtc_vdisplay; > - timing_out->v_front_porch = > - mode_in->crtc_vsync_start - mode_in->crtc_vdisplay; > - timing_out->v_sync_width = > - mode_in->crtc_vsync_end - mode_in->crtc_vsync_start; > - timing_out->pix_clk_100hz = mode_in->crtc_clock * 10; > + if (is_in_modeset) { > + timing_out->h_addressable = mode_in->hdisplay; > + timing_out->h_total = mode_in->htotal; > + timing_out->h_sync_width = mode_in->hsync_end - mode_in->hsync_start; > + timing_out->h_front_porch = mode_in->hsync_start - mode_in->hdisplay; > + timing_out->v_total = mode_in->vtotal; > + timing_out->v_addressable = mode_in->vdisplay; > + timing_out->v_front_porch = mode_in->vsync_start - mode_in->vdisplay; > + timing_out->v_sync_width = mode_in->vsync_end - mode_in->vsync_start; > + timing_out->pix_clk_100hz = mode_in->clock * 10; > + } else { > + timing_out->h_addressable = mode_in->crtc_hdisplay; > + timing_out->h_total = mode_in->crtc_htotal; > + timing_out->h_sync_width = mode_in->crtc_hsync_end - mode_in->crtc_hsync_start; > + timing_out->h_front_porch = mode_in->crtc_hsync_start - mode_in->crtc_hdisplay; > + timing_out->v_total = mode_in->crtc_vtotal; > + timing_out->v_addressable = mode_in->crtc_vdisplay; > + timing_out->v_front_porch = mode_in->crtc_vsync_start - mode_in->crtc_vdisplay; > + timing_out->v_sync_width = mode_in->crtc_vsync_end - mode_in->crtc_vsync_start; > + timing_out->pix_clk_100hz = mode_in->crtc_clock * 10; > + } > + Not sure if I commented on this last time but I don't really understand what this is_in_modeset logic is supposed to be doing here. We should be modifying crtc_vsync_* for the generated modes, no? Not just the vsync_* parameters. > timing_out->aspect_ratio = get_aspect_ratio(mode_in); > > stream->output_color_space = get_output_color_space(timing_out); > @@ -5227,6 +5240,33 @@ get_highest_refresh_rate_mode(struct amdgpu_dm_connector *aconnector, > return m_pref; > } > > +static bool is_freesync_video_mode(struct drm_display_mode *mode, > + struct amdgpu_dm_connector *aconnector) > +{ > + struct drm_display_mode *high_mode; > + int timing_diff; > + > + high_mode = get_highest_refresh_rate_mode(aconnector, false); > + if (!high_mode || !mode) > + return false; > + > + timing_diff = high_mode->vtotal - mode->vtotal; > + > + if (high_mode->clock == 0 || high_mode->clock != mode->clock || > + high_mode->hdisplay != mode->hdisplay || > + high_mode->vdisplay != mode->vdisplay || > + high_mode->hsync_start != mode->hsync_start || > + high_mode->hsync_end != mode->hsync_end || > + high_mode->htotal != mode->htotal || > + high_mode->hskew != mode->hskew || > + high_mode->vscan != mode->vscan || > + high_mode->vsync_start - mode->vsync_start != timing_diff || > + high_mode->vsync_end - mode->vsync_end != timing_diff) > + return false; > + else > + return true; > +} > + > static struct dc_stream_state * > create_stream_for_sink(struct amdgpu_dm_connector *aconnector, > const struct drm_display_mode *drm_mode, > @@ -5240,15 +5280,21 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector, > 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; > + 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 = false; > #if defined(CONFIG_DRM_AMD_DC_DCN) > struct dsc_dec_dpcd_caps dsc_caps; > uint32_t link_bandwidth_kbps; > #endif > struct dc_sink *sink = NULL; > + > + memset(&saved_mode, 0, sizeof(saved_mode)); > + > if (aconnector == NULL) { > DRM_ERROR("aconnector is NULL!\n"); > return stream; > @@ -5301,25 +5347,39 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector, > */ > DRM_DEBUG_DRIVER("No preferred mode found\n"); > } else { > - decide_crtc_timing_for_drm_display_mode( > + is_fs_vid_mode = amdgpu_freesync_vid_mode && > + is_freesync_video_mode(&mode, aconnector); > + if (is_fs_vid_mode) { > + freesync_mode = get_highest_refresh_rate_mode(aconnector, false); > + saved_mode = mode; > + mode = *freesync_mode; > + } else { > + decide_crtc_timing_for_drm_display_mode( > &mode, preferred_mode, > dm_state ? (dm_state->scaling != RMX_OFF) : false); > + } > + > preferred_refresh = drm_mode_vrefresh(preferred_mode); > } > > if (!dm_state) > drm_mode_set_crtcinfo(&mode, 0); > > - /* > + if (dm_state && is_fs_vid_mode) > + drm_mode_set_crtcinfo(&saved_mode, 0); > + > + Not sure what this above bit is doing here: (1) We're in atomic check and (2) It's a FS video mode -> set CRTC modesetting timing parameters? What's this for? /* > * 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) > - fill_stream_properties_from_drm_display_mode(stream, > - &mode, &aconnector->base, con_state, NULL, requested_bpc); > + if (!(scale && is_fs_vid_mode) || mode_refresh != preferred_refresh) > + fill_stream_properties_from_drm_display_mode( > + stream, &mode, &aconnector->base, con_state, NULL, > + requested_bpc, dm_state ? 1 : 0); > else > - fill_stream_properties_from_drm_display_mode(stream, > - &mode, &aconnector->base, con_state, old_stream, requested_bpc); > + fill_stream_properties_from_drm_display_mode( > + stream, &mode, &aconnector->base, con_state, old_stream, > + requested_bpc, dm_state ? 1 : 0); I don't see my feedback on previous patches addressed here - isn't it cleaner to just merge the is_scaling/is_fs_vid_mode checks here? The idea is that we're replacing the mode with the primary/preferred in both cases. Using dm_state as a check for validation / vs atomic check is questionable as well. In practice we're guaranteed to have it today but it might not be clear if there are future changes that it's being used for this purpose here if that changes. > > stream->timing.flags.DSC = 0; > > @@ -5456,6 +5516,7 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc) > state->abm_level = cur->abm_level; > state->vrr_supported = cur->vrr_supported; > state->freesync_config = cur->freesync_config; > + state->freesync_video_mode = cur->freesync_video_mode; > state->crc_src = cur->crc_src; > state->cm_has_degamma = cur->cm_has_degamma; > state->cm_is_degamma_srgb = cur->cm_is_degamma_srgb; > @@ -7149,7 +7210,7 @@ static void amdgpu_dm_connector_add_freesync_modes(struct drm_connector *connect > struct amdgpu_dm_connector *amdgpu_dm_connector = > to_amdgpu_dm_connector(connector); > > - if (!(amdgpu_exp_freesync_vid_mode && edid)) > + if (!(amdgpu_freesync_vid_mode && edid)) > return; > > if (edid->version == 1 && edid->revision > 1) { > @@ -7847,9 +7908,25 @@ static void update_stream_irq_parameters( > if (new_crtc_state->vrr_supported && > config.min_refresh_in_uhz && > config.max_refresh_in_uhz) { > - config.state = new_crtc_state->base.vrr_enabled ? > - VRR_STATE_ACTIVE_VARIABLE : > - VRR_STATE_INACTIVE; > + /* > + * 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 && This condition doesn't need to check max_refresh/min_refresh as it's already checked aboved. > + (!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; > + } else { > + config.state = new_crtc_state->base.vrr_enabled ? > + VRR_STATE_ACTIVE_VARIABLE : > + VRR_STATE_INACTIVE; > + } > + > } else { > config.state = VRR_STATE_UNSUPPORTED; > } > @@ -8171,7 +8248,8 @@ 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_video_mode) { Do we really need this check here? amdgpu_dm_vrr_active should pick up on VRR_STATE_ACTIVE_FIXED, no? If it doesn't then that should be fixed. > spin_lock_irqsave(&pcrtc->dev->event_lock, flags); > dc_stream_adjust_vmin_vmax( > dm->dc, acrtc_state->stream, > @@ -8442,8 +8520,10 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state) > continue; > } > > - if (dm_old_crtc_state->stream) > + if (dm_old_crtc_state->stream) { > remove_stream(adev, acrtc, dm_old_crtc_state->stream); > + dm_old_crtc_state->freesync_video_mode = 0; > + } > > pm_runtime_get_noresume(dev->dev); > > @@ -8454,8 +8534,10 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state) > } else if (modereset_required(new_crtc_state)) { > DRM_DEBUG_DRIVER("Atomic commit: RESET. crtc id %d:[%p]\n", acrtc->crtc_id, acrtc); > /* i.e. reset mode */ > - if (dm_old_crtc_state->stream) > + if (dm_old_crtc_state->stream) { > remove_stream(adev, acrtc, dm_old_crtc_state->stream); > + dm_old_crtc_state->freesync_video_mode = 0; > + } > mode_set_reset_required = true; > } > } /* for_each_crtc_in_state() */ > @@ -8867,6 +8949,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 && > @@ -8874,17 +8957,25 @@ 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) { > + config.state = VRR_STATE_ACTIVE_VARIABLE; > + } else { > + config.state = VRR_STATE_INACTIVE; > + } > + } > +out: > new_crtc_state->freesync_config = config; > } > > @@ -8897,6 +8988,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, > @@ -8987,6 +9123,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 (amdgpu_freesync_vid_mode && > + dm_new_crtc_state->stream && > + is_timing_unchanged_for_freesync(new_crtc_state, old_crtc_state)) > + 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)) { > @@ -9018,6 +9159,26 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm, > if (!dm_old_crtc_state->stream) > goto skip_modeset; > > + if (amdgpu_freesync_vid_mode && dm_new_crtc_state->stream && > + is_timing_unchanged_for_freesync(new_crtc_state, > + old_crtc_state)) { > + 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 (amdgpu_freesync_vid_mode && aconnector && > + is_freesync_video_mode(&new_crtc_state->mode, > + aconnector)) { > + set_freesync_fixed_config(dm_new_crtc_state); I don't think this extra check is needed here because we have the one above. Regards, Nicholas Kazlauskas > + } else if (dm_new_crtc_state->freesync_video_mode) { > + dm_new_crtc_state->freesync_video_mode = 0; > + } > + > 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 3ea85be9c546..ff4675572125 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h > @@ -474,6 +474,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; > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
WARNING: multiple messages have this Message-ID (diff)
From: "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> To: Aurabindo Pillai <aurabindo.pillai@amd.com>, amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org Cc: stylon.wang@amd.com, thong.thai@amd.com, shashank.sharma@amd.com, wayne.lin@amd.com, alexander.deucher@amd.com, christian.koenig@amd.com Subject: Re: [PATCH 3/3] drm/amd/display: Skip modeset for front porch change Date: Thu, 21 Jan 2021 14:05:12 -0500 [thread overview] Message-ID: <e789fbc8-709f-41a7-1b2b-3ff328c384b1@amd.com> (raw) In-Reply-To: <20210119155029.18943-4-aurabindo.pillai@amd.com> On 2021-01-19 10:50 a.m., Aurabindo Pillai wrote: > [Why] > A seamless transition between modes can be performed if the new incoming > mode has the same timing parameters as the optimized mode on a display with a > variable vtotal min/max. > > Smooth video playback usecases can be enabled with this seamless transition by > switching to a new mode which has a refresh rate matching the video. > > [How] > Skip full modeset if userspace requested a compatible freesync mode which only > differs in the front porch timing from the current mode. > > Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com> > Acked-by: Christian König <christian.koenig@amd.com> > --- > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 233 +++++++++++++++--- > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 + > 2 files changed, 198 insertions(+), 36 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 aaef2fb528fd..d66494cdd8c8 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -213,6 +213,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 > * > @@ -4940,7 +4943,8 @@ static void fill_stream_properties_from_drm_display_mode( > const struct drm_connector *connector, > const struct drm_connector_state *connector_state, > const struct dc_stream_state *old_stream, > - int requested_bpc) > + int requested_bpc, > + bool is_in_modeset) > { > struct dc_crtc_timing *timing_out = &stream->timing; > const struct drm_display_info *info = &connector->display_info; > @@ -4995,19 +4999,28 @@ static void fill_stream_properties_from_drm_display_mode( > timing_out->hdmi_vic = hv_frame.vic; > } > > - timing_out->h_addressable = mode_in->crtc_hdisplay; > - timing_out->h_total = mode_in->crtc_htotal; > - timing_out->h_sync_width = > - mode_in->crtc_hsync_end - mode_in->crtc_hsync_start; > - timing_out->h_front_porch = > - mode_in->crtc_hsync_start - mode_in->crtc_hdisplay; > - timing_out->v_total = mode_in->crtc_vtotal; > - timing_out->v_addressable = mode_in->crtc_vdisplay; > - timing_out->v_front_porch = > - mode_in->crtc_vsync_start - mode_in->crtc_vdisplay; > - timing_out->v_sync_width = > - mode_in->crtc_vsync_end - mode_in->crtc_vsync_start; > - timing_out->pix_clk_100hz = mode_in->crtc_clock * 10; > + if (is_in_modeset) { > + timing_out->h_addressable = mode_in->hdisplay; > + timing_out->h_total = mode_in->htotal; > + timing_out->h_sync_width = mode_in->hsync_end - mode_in->hsync_start; > + timing_out->h_front_porch = mode_in->hsync_start - mode_in->hdisplay; > + timing_out->v_total = mode_in->vtotal; > + timing_out->v_addressable = mode_in->vdisplay; > + timing_out->v_front_porch = mode_in->vsync_start - mode_in->vdisplay; > + timing_out->v_sync_width = mode_in->vsync_end - mode_in->vsync_start; > + timing_out->pix_clk_100hz = mode_in->clock * 10; > + } else { > + timing_out->h_addressable = mode_in->crtc_hdisplay; > + timing_out->h_total = mode_in->crtc_htotal; > + timing_out->h_sync_width = mode_in->crtc_hsync_end - mode_in->crtc_hsync_start; > + timing_out->h_front_porch = mode_in->crtc_hsync_start - mode_in->crtc_hdisplay; > + timing_out->v_total = mode_in->crtc_vtotal; > + timing_out->v_addressable = mode_in->crtc_vdisplay; > + timing_out->v_front_porch = mode_in->crtc_vsync_start - mode_in->crtc_vdisplay; > + timing_out->v_sync_width = mode_in->crtc_vsync_end - mode_in->crtc_vsync_start; > + timing_out->pix_clk_100hz = mode_in->crtc_clock * 10; > + } > + Not sure if I commented on this last time but I don't really understand what this is_in_modeset logic is supposed to be doing here. We should be modifying crtc_vsync_* for the generated modes, no? Not just the vsync_* parameters. > timing_out->aspect_ratio = get_aspect_ratio(mode_in); > > stream->output_color_space = get_output_color_space(timing_out); > @@ -5227,6 +5240,33 @@ get_highest_refresh_rate_mode(struct amdgpu_dm_connector *aconnector, > return m_pref; > } > > +static bool is_freesync_video_mode(struct drm_display_mode *mode, > + struct amdgpu_dm_connector *aconnector) > +{ > + struct drm_display_mode *high_mode; > + int timing_diff; > + > + high_mode = get_highest_refresh_rate_mode(aconnector, false); > + if (!high_mode || !mode) > + return false; > + > + timing_diff = high_mode->vtotal - mode->vtotal; > + > + if (high_mode->clock == 0 || high_mode->clock != mode->clock || > + high_mode->hdisplay != mode->hdisplay || > + high_mode->vdisplay != mode->vdisplay || > + high_mode->hsync_start != mode->hsync_start || > + high_mode->hsync_end != mode->hsync_end || > + high_mode->htotal != mode->htotal || > + high_mode->hskew != mode->hskew || > + high_mode->vscan != mode->vscan || > + high_mode->vsync_start - mode->vsync_start != timing_diff || > + high_mode->vsync_end - mode->vsync_end != timing_diff) > + return false; > + else > + return true; > +} > + > static struct dc_stream_state * > create_stream_for_sink(struct amdgpu_dm_connector *aconnector, > const struct drm_display_mode *drm_mode, > @@ -5240,15 +5280,21 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector, > 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; > + 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 = false; > #if defined(CONFIG_DRM_AMD_DC_DCN) > struct dsc_dec_dpcd_caps dsc_caps; > uint32_t link_bandwidth_kbps; > #endif > struct dc_sink *sink = NULL; > + > + memset(&saved_mode, 0, sizeof(saved_mode)); > + > if (aconnector == NULL) { > DRM_ERROR("aconnector is NULL!\n"); > return stream; > @@ -5301,25 +5347,39 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector, > */ > DRM_DEBUG_DRIVER("No preferred mode found\n"); > } else { > - decide_crtc_timing_for_drm_display_mode( > + is_fs_vid_mode = amdgpu_freesync_vid_mode && > + is_freesync_video_mode(&mode, aconnector); > + if (is_fs_vid_mode) { > + freesync_mode = get_highest_refresh_rate_mode(aconnector, false); > + saved_mode = mode; > + mode = *freesync_mode; > + } else { > + decide_crtc_timing_for_drm_display_mode( > &mode, preferred_mode, > dm_state ? (dm_state->scaling != RMX_OFF) : false); > + } > + > preferred_refresh = drm_mode_vrefresh(preferred_mode); > } > > if (!dm_state) > drm_mode_set_crtcinfo(&mode, 0); > > - /* > + if (dm_state && is_fs_vid_mode) > + drm_mode_set_crtcinfo(&saved_mode, 0); > + > + Not sure what this above bit is doing here: (1) We're in atomic check and (2) It's a FS video mode -> set CRTC modesetting timing parameters? What's this for? /* > * 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) > - fill_stream_properties_from_drm_display_mode(stream, > - &mode, &aconnector->base, con_state, NULL, requested_bpc); > + if (!(scale && is_fs_vid_mode) || mode_refresh != preferred_refresh) > + fill_stream_properties_from_drm_display_mode( > + stream, &mode, &aconnector->base, con_state, NULL, > + requested_bpc, dm_state ? 1 : 0); > else > - fill_stream_properties_from_drm_display_mode(stream, > - &mode, &aconnector->base, con_state, old_stream, requested_bpc); > + fill_stream_properties_from_drm_display_mode( > + stream, &mode, &aconnector->base, con_state, old_stream, > + requested_bpc, dm_state ? 1 : 0); I don't see my feedback on previous patches addressed here - isn't it cleaner to just merge the is_scaling/is_fs_vid_mode checks here? The idea is that we're replacing the mode with the primary/preferred in both cases. Using dm_state as a check for validation / vs atomic check is questionable as well. In practice we're guaranteed to have it today but it might not be clear if there are future changes that it's being used for this purpose here if that changes. > > stream->timing.flags.DSC = 0; > > @@ -5456,6 +5516,7 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc) > state->abm_level = cur->abm_level; > state->vrr_supported = cur->vrr_supported; > state->freesync_config = cur->freesync_config; > + state->freesync_video_mode = cur->freesync_video_mode; > state->crc_src = cur->crc_src; > state->cm_has_degamma = cur->cm_has_degamma; > state->cm_is_degamma_srgb = cur->cm_is_degamma_srgb; > @@ -7149,7 +7210,7 @@ static void amdgpu_dm_connector_add_freesync_modes(struct drm_connector *connect > struct amdgpu_dm_connector *amdgpu_dm_connector = > to_amdgpu_dm_connector(connector); > > - if (!(amdgpu_exp_freesync_vid_mode && edid)) > + if (!(amdgpu_freesync_vid_mode && edid)) > return; > > if (edid->version == 1 && edid->revision > 1) { > @@ -7847,9 +7908,25 @@ static void update_stream_irq_parameters( > if (new_crtc_state->vrr_supported && > config.min_refresh_in_uhz && > config.max_refresh_in_uhz) { > - config.state = new_crtc_state->base.vrr_enabled ? > - VRR_STATE_ACTIVE_VARIABLE : > - VRR_STATE_INACTIVE; > + /* > + * 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 && This condition doesn't need to check max_refresh/min_refresh as it's already checked aboved. > + (!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; > + } else { > + config.state = new_crtc_state->base.vrr_enabled ? > + VRR_STATE_ACTIVE_VARIABLE : > + VRR_STATE_INACTIVE; > + } > + > } else { > config.state = VRR_STATE_UNSUPPORTED; > } > @@ -8171,7 +8248,8 @@ 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_video_mode) { Do we really need this check here? amdgpu_dm_vrr_active should pick up on VRR_STATE_ACTIVE_FIXED, no? If it doesn't then that should be fixed. > spin_lock_irqsave(&pcrtc->dev->event_lock, flags); > dc_stream_adjust_vmin_vmax( > dm->dc, acrtc_state->stream, > @@ -8442,8 +8520,10 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state) > continue; > } > > - if (dm_old_crtc_state->stream) > + if (dm_old_crtc_state->stream) { > remove_stream(adev, acrtc, dm_old_crtc_state->stream); > + dm_old_crtc_state->freesync_video_mode = 0; > + } > > pm_runtime_get_noresume(dev->dev); > > @@ -8454,8 +8534,10 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state) > } else if (modereset_required(new_crtc_state)) { > DRM_DEBUG_DRIVER("Atomic commit: RESET. crtc id %d:[%p]\n", acrtc->crtc_id, acrtc); > /* i.e. reset mode */ > - if (dm_old_crtc_state->stream) > + if (dm_old_crtc_state->stream) { > remove_stream(adev, acrtc, dm_old_crtc_state->stream); > + dm_old_crtc_state->freesync_video_mode = 0; > + } > mode_set_reset_required = true; > } > } /* for_each_crtc_in_state() */ > @@ -8867,6 +8949,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 && > @@ -8874,17 +8957,25 @@ 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) { > + config.state = VRR_STATE_ACTIVE_VARIABLE; > + } else { > + config.state = VRR_STATE_INACTIVE; > + } > + } > +out: > new_crtc_state->freesync_config = config; > } > > @@ -8897,6 +8988,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, > @@ -8987,6 +9123,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 (amdgpu_freesync_vid_mode && > + dm_new_crtc_state->stream && > + is_timing_unchanged_for_freesync(new_crtc_state, old_crtc_state)) > + 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)) { > @@ -9018,6 +9159,26 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm, > if (!dm_old_crtc_state->stream) > goto skip_modeset; > > + if (amdgpu_freesync_vid_mode && dm_new_crtc_state->stream && > + is_timing_unchanged_for_freesync(new_crtc_state, > + old_crtc_state)) { > + 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 (amdgpu_freesync_vid_mode && aconnector && > + is_freesync_video_mode(&new_crtc_state->mode, > + aconnector)) { > + set_freesync_fixed_config(dm_new_crtc_state); I don't think this extra check is needed here because we have the one above. Regards, Nicholas Kazlauskas > + } else if (dm_new_crtc_state->freesync_video_mode) { > + dm_new_crtc_state->freesync_video_mode = 0; > + } > + > 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 3ea85be9c546..ff4675572125 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h > @@ -474,6 +474,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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
next prev parent reply other threads:[~2021-01-21 19:05 UTC|newest] Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-01-19 15:50 [PATCH 0/3] Experimental freesync video mode optimization Aurabindo Pillai 2021-01-19 15:50 ` Aurabindo Pillai 2021-01-19 15:50 ` [PATCH 1/3] drm/amd/display: Add module parameter for freesync video mode Aurabindo Pillai 2021-01-19 15:50 ` Aurabindo Pillai 2021-01-19 15:50 ` [PATCH 2/3] drm/amd/display: Add freesync video modes based on preferred modes Aurabindo Pillai 2021-01-19 15:50 ` Aurabindo Pillai 2021-01-19 15:50 ` [PATCH 3/3] drm/amd/display: Skip modeset for front porch change Aurabindo Pillai 2021-01-19 15:50 ` Aurabindo Pillai 2021-01-21 19:05 ` Kazlauskas, Nicholas [this message] 2021-01-21 19:05 ` Kazlauskas, Nicholas 2021-01-25 4:00 ` Aurabindo Pillai 2021-01-25 4:00 ` Aurabindo Pillai 2021-02-08 15:06 ` Kazlauskas, Nicholas 2021-02-08 15:06 ` Kazlauskas, Nicholas 2021-02-12 20:01 ` Aurabindo Pillai 2021-02-12 20:01 ` Aurabindo Pillai 2021-01-22 8:32 ` [PATCH 0/3] Experimental freesync video mode optimization Pekka Paalanen 2021-01-22 8:32 ` Pekka Paalanen 2021-01-22 9:42 ` Daniel Vetter 2021-01-22 9:42 ` Daniel Vetter 2021-01-22 9:44 ` Daniel Vetter 2021-01-22 9:44 ` Daniel Vetter 2021-01-22 15:16 ` Alex Deucher 2021-01-22 15:16 ` Alex Deucher 2021-02-02 14:27 ` Daniel Vetter 2021-02-02 14:27 ` Daniel Vetter -- strict thread matches above, loose matches on Subject: below -- 2020-12-10 2:45 Aurabindo Pillai 2020-12-10 2:45 ` [PATCH 3/3] drm/amd/display: Skip modeset for front porch change Aurabindo Pillai 2020-12-10 10:21 ` Christian König 2020-12-10 12:59 ` Shashank Sharma 2020-12-10 17:50 ` Aurabindo Pillai 2020-12-11 5:08 ` Shashank Sharma 2020-12-11 14:49 ` Kazlauskas, Nicholas 2020-12-11 15:35 ` Shashank Sharma 2020-12-11 16:20 ` Kazlauskas, Nicholas 2020-12-11 18:31 ` Shashank Sharma 2021-01-04 16:16 ` Kazlauskas, Nicholas 2021-01-04 20:43 ` Aurabindo Pillai
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=e789fbc8-709f-41a7-1b2b-3ff328c384b1@amd.com \ --to=nicholas.kazlauskas@amd.com \ --cc=alexander.deucher@amd.com \ --cc=amd-gfx@lists.freedesktop.org \ --cc=aurabindo.pillai@amd.com \ --cc=christian.koenig@amd.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=shashank.sharma@amd.com \ --cc=stylon.wang@amd.com \ --cc=thong.thai@amd.com \ --cc=wayne.lin@amd.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.