Changes in V5 =============
* More info in commit messages on the rationale of changes being added to the kernel. * Minor fixes
Changes in V4 =============
1) Add module parameter for freesync video mode
* Change module parameter name to freesync_video
2) Add freesync video modes based on preferred modes:
* Cosmetic fixes * Added comments about all modes being added by the driver.
3) Skip modeset for front porch change
* Added more conditions for checking freesync video mode
Changes in V3 =============
1) Add freesync video modes based on preferred modes:
* Cache base freesync video mode during the first iteration to avoid iterating over modelist again later. * Add mode for 60 fps videos
2) Skip modeset for front porch change
* Fixes for bug exposed by caching of modes.
Changes in V2 =============
1) Add freesync video modes based on preferred modes:
* Remove check for connector type before adding freesync compatible modes as VRR support is being checked, and there is no reason to block freesync video support on eDP. * use drm_mode_equal() instead of creating same functionality. * Additional null pointer deference check * Removed unnecessary variables. * Cosmetic fixes.
2) Skip modeset for front porch change
* Remove _FSV string being appended to freesync video modes so as to not define new policies or break existing application that might use the mode name to figure out mode resolution. * Remove unnecessary variables * Cosmetic fixes.
--
This patchset enables freesync video mode usecase where the userspace can request a freesync compatible video mode such that switching to this mode does not trigger blanking.
This feature is guarded by a module parameter which is disabled by default. Enabling this paramters adds additional modes to the driver modelist, and also enables the optimization to skip modeset when using one of these modes.
--
Aurabindo Pillai (3): drm/amd/display: Add module parameter for freesync video mode drm/amd/display: Add freesync video modes based on preferred modes drm/amd/display: Skip modeset for front porch change
drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 + drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 12 + .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 401 ++++++++++++++++-- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 3 + 4 files changed, 382 insertions(+), 35 deletions(-)
[Why] This option shall be opt-in by default since it is a temporary solution until long term solution is agreed upon which may require userspace interface changes. There has been precedent of manufacturing modes in the kernel. In AMDGPU, the existing usage are for common modes and scaling modes. Other driver have a similar approach as well.
[How] Adds a module parameter to enable freesync video mode modeset optimization. Enabling this mode allows the driver to skip a full modeset when a freesync compatible mode is requested by the userspace. This parameter will also add some additional modes that are within the connected monitor's VRR range corresponding to common video modes, which media players can use for a seamless experience while making use of freesync.
Signed-off-by: Aurabindo Pillai aurabindo.pillai@amd.com Acked-by: Christian König <christian.koenig at amd.com> Reviewed-by: Shashank Sharma shashank.sharma@amd.com --- drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 + drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 12 ++++++++++++ 2 files changed, 13 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h index 100a431f0792..770e42fcaa62 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h @@ -177,6 +177,7 @@ extern int amdgpu_gpu_recovery; extern int amdgpu_emu_mode; extern uint amdgpu_smu_memory_pool_size; extern uint amdgpu_dc_feature_mask; +extern uint amdgpu_freesync_vid_mode; extern uint amdgpu_dc_debug_mask; extern uint amdgpu_dm_abm_level; extern struct amdgpu_mgpu_info mgpu_info; diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index b48d7a3c2a11..5c6dc8362e6d 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -158,6 +158,7 @@ int amdgpu_mes; int amdgpu_noretry = -1; int amdgpu_force_asic_type = -1; int amdgpu_tmz; +uint amdgpu_freesync_vid_mode; int amdgpu_reset_method = -1; /* auto */ int amdgpu_num_kcq = -1;
@@ -786,6 +787,17 @@ module_param_named(abmlevel, amdgpu_dm_abm_level, uint, 0444); MODULE_PARM_DESC(tmz, "Enable TMZ feature (-1 = auto, 0 = off (default), 1 = on)"); module_param_named(tmz, amdgpu_tmz, int, 0444);
+/** + * DOC: freesync_video (uint) + * Enabled the optimization to adjust front porch timing to achieve seamless mode change experience + * when setting a freesync supported mode for which full modeset is not needed. + * The default value: 0 (off). + */ +MODULE_PARM_DESC( + freesync_video, + "Enable freesync modesetting optimization feature (0 = off (default), 1 = on)"); +module_param_named(freesync_video, amdgpu_freesync_vid_mode, uint, 0444); + /** * DOC: reset_method (int) * GPU reset method (-1 = auto (default), 0 = legacy, 1 = mode0, 2 = mode1, 3 = mode2, 4 = baco)
[Why] While possible for userspace to create and add custom mode based off the optimized mode for the connected display which differs only in front porch timing, this patch set adds a list of common video modes in advance.
The list of common video refresh rates is small, well known and the optimized mode has specific requirements to be able to enable HW frame doubling and tripling so it makes most sense to create the modes that video players will need in advance. The optimized mode matches the preferred mode resolution but has the highest refresh rate available to enable the largest front porch extension.
[How] Find the optimized mode and store it on the connector so we can check it later during our optimized modeset.
Prepopulate the mode list with a list of common video mades based on the optimized mode (but with a longer front porch) if the panel doesn't support a variant of the mode natively.
Signed-off-by: Aurabindo Pillai aurabindo.pillai@amd.com Acked-by: Christian König christian.koenig@amd.com Reviewed-by: Shashank Sharma shashank.sharma@amd.com --- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 170 ++++++++++++++++++ .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 2 + 2 files changed, 172 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 245bd1284e5f..aaef2fb528fd 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,122 @@ 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; + uint32_t new_modes_count = 0; + + /* 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 + * 60 - Commonly used + * 48,72,96 - Multiples of 24 + */ + const uint32_t common_rates[] = { 23976, 24000, 25000, 29970, 30000, + 48000, 50000, 60000, 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 = get_highest_refresh_rate_mode(aconnector, true); + if (!m) + return 0; + + for (i = 0; i < ARRAY_SIZE(common_rates); i++) { + uint64_t target_vtotal, target_vtotal_diff; + uint64_t num, den; + + if (drm_mode_vrefresh(m) * 1000 < common_rates[i]) + continue; + + if (common_rates[i] < range->min_vfreq * 1000) + continue; + + num = (unsigned long long)m->clock * 1000 * 1000; + den = common_rates[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; + + 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 = @@ -7014,6 +7183,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);
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 c9d82b9e4d7e..3ea85be9c546 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h @@ -432,6 +432,8 @@ struct amdgpu_dm_connector { #endif bool force_yuv420_output; struct dsc_preferred_settings dsc_settings; + /* Cached display modes */ + struct drm_display_mode freesync_vid_base; };
#define to_amdgpu_dm_connector(x) container_of(x, struct amdgpu_dm_connector, base)
[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; + } + 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); + + /* * 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);
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 && + (!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) { 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); + } 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;
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);
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;@@ -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;
- }
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;
On 2021-01-21 2:05 p.m., Kazlauskas, Nicholas wrote:
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.
This is so because create_stream_for_link() that ends up calling this function has two callers, one which is for stream validation in which the created stream is immediately discarded. The other is during modeset. Depending on these two cases, we want to copy the right timing parameters. With this method, major refactor wasn't necessary with the upper layers.
We should be modifying crtc_vsync_* for the generated modes, no? Not just the vsync_* parameters.
This is already handled with:
if (!dm_state) drm_mode_set_crtcinfo(&mode, 0);
if (dm_state && is_fs_vid_mode) drm_mode_set_crtcinfo(&saved_mode, 0);
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?
Without this change, kernel would tell the userspace we're sending one of the freesync mode timing, but actually it would send the optimized mode timing parameter. For instance, if 144Hz is the optimized mode, and the user sets 48Hz, the display settings would reflect the change, but the monitor would still see 144Hz.
/*
* 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.
Sorry, I must have missed it. Will address this in the next iteration.
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.
This is because we're reusing this function for testing stream validation by calling it directly with a NULL parameter for dm_state. If this usage is not recommended, then a separate function can be used for just stream validation and this usage of dm_state can be dropped.
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.
Will fix.
+ (!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.
This specifically checks for a change the current and previous state. In both the states, freesync_video_mode can be active. This condition would return false in the absence of additional OR with freesync_video_mode. If freesync_video_mode is active, we want this block of code to get executed. If freesync_video_mode is not active, then comparison with old and new state is sufficient. I can add this logic into another function and keep it cleaner here.
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.
In the second branch, since we call is_freesync_video_mode(), the check with the module parameter was also necessary as this is the triggering point for the VRR api configuration change. Also, the first branch skips the modeset, while the second doesnt. So we cannot club them together. The second branch is for the case when a freesync mode was set accompanied by a modeset. This happens when the previous mode had different base timings and hence a modeset is necessary.
Thanks & Regards, Aurabindo Pillai
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;
On 2021-01-24 11:00 p.m., Aurabindo Pillai wrote:
On 2021-01-21 2:05 p.m., Kazlauskas, Nicholas wrote:
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.
This is so because create_stream_for_link() that ends up calling this function has two callers, one which is for stream validation in which the created stream is immediately discarded. The other is during modeset. Depending on these two cases, we want to copy the right timing parameters. With this method, major refactor wasn't necessary with the upper layers.
I don't understand why the timing parameters would change between what we validated and what we're planning on applying to the hardware. I think we should be validating the same thing in both cases, especially if it's for something internal like DC stream timing.
We should be modifying crtc_vsync_* for the generated modes, no? Not just the vsync_* parameters.
This is already handled with:
if (!dm_state) drm_mode_set_crtcinfo(&mode, 0);
if (dm_state && is_fs_vid_mode) drm_mode_set_crtcinfo(&saved_mode, 0);
OK, that's fine then.
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?
Without this change, kernel would tell the userspace we're sending one of the freesync mode timing, but actually it would send the optimized mode timing parameter. For instance, if 144Hz is the optimized mode, and the user sets 48Hz, the display settings would reflect the change, but the monitor would still see 144Hz.
Sorry, but I still don't understand why it depends on dm_state here. You're trying to avoid updating crtcinfo in mode validation for some reason?
/*
* 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.
Sorry, I must have missed it. Will address this in the next iteration.
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.
This is because we're reusing this function for testing stream validation by calling it directly with a NULL parameter for dm_state. If this usage is not recommended, then a separate function can be used for just stream validation and this usage of dm_state can be dropped.
Like my comments above I'm not sure why these would differ between validation/commit time - they (ideally) should not.
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.
Will fix.
+ (!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.
This specifically checks for a change the current and previous state. In both the states, freesync_video_mode can be active. This condition would return false in the absence of additional OR with freesync_video_mode. If freesync_video_mode is active, we want this block of code to get executed. If freesync_video_mode is not active, then comparison with old and new state is sufficient. I can add this logic into another function and keep it cleaner here.
I think acrtc_state->freesync_video_mode is redundant - it doesn't check prev/new, it only checks whether or not it's active. This should be already checked in amdgpu_dm_vrr_active(), and the:
amdgpu_dm_vrr_active(dm_old_crtc_state) != amdgpu_dm_vrr_active(acrtc_state)
should be doing this for you.
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.
In the second branch, since we call is_freesync_video_mode(), the check with the module parameter was also necessary as this is the triggering point for the VRR api configuration change. Also, the first branch skips the modeset, while the second doesnt. So we cannot club them together. The second branch is for the case when a freesync mode was set accompanied by a modeset. This happens when the previous mode had different base timings and hence a modeset is necessary.
Thanks & Regards, Aurabindo Pillai
Okay, I understand this block now. Thanks.
Regards, Nicholas Kazlauskas
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;
On 2021-02-08 10:06 a.m., Kazlauskas, Nicholas wrote:
On 2021-01-24 11:00 p.m., Aurabindo Pillai wrote:
On 2021-01-21 2:05 p.m., Kazlauskas, Nicholas wrote:
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.
This is so because create_stream_for_link() that ends up calling this function has two callers, one which is for stream validation in which the created stream is immediately discarded. The other is during modeset. Depending on these two cases, we want to copy the right timing parameters. With this method, major refactor wasn't necessary with the upper layers.
I don't understand why the timing parameters would change between what we validated and what we're planning on applying to the hardware. I think we should be validating the same thing in both cases, especially if it's for something internal like DC stream timing.
Actually, the validated timings didn't change, but the difference is where the correct values exist. I took a second looks at this, the timing is always present in mode_in, but not in crtc. However, originally it had crtc_ parameters being used for filling timing_out, so in order to keep the delta between the "freesync mode" and other normal modes, and in the interest keeping the functional impact of this patch minimum, I chose to apply them separately depending on is_in_modeset logic. In my testing, using the non crtc_ paramters are working fine, without the is_in_modeset logic.
We should be modifying crtc_vsync_* for the generated modes, no? Not just the vsync_* parameters.
This is already handled with:
if (!dm_state) drm_mode_set_crtcinfo(&mode, 0);
if (dm_state && is_fs_vid_mode) drm_mode_set_crtcinfo(&saved_mode, 0);
OK, that's fine then.
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?
Without this change, kernel would tell the userspace we're sending one of the freesync mode timing, but actually it would send the optimized mode timing parameter. For instance, if 144Hz is the optimized mode, and the user sets 48Hz, the display settings would reflect the change, but the monitor would still see 144Hz.
Sorry, but I still don't understand why it depends on dm_state here. You're trying to avoid updating crtcinfo in mode validation for some reason?
Yes, I could say that. The whole reason why we have dm_state in this function's argument seems to be just to reuse the code for validation. But this can be simplified into:
if (is_fs_vid_mode) drm_mode_set_crtcinfo(&saved_mode, 0); else drm_mode_set_crtcinfo(&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) - 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.
Sorry, I must have missed it. Will address this in the next iteration.
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.
This is because we're reusing this function for testing stream validation by calling it directly with a NULL parameter for dm_state. If this usage is not recommended, then a separate function can be used for just stream validation and this usage of dm_state can be dropped.
Like my comments above I'm not sure why these would differ between validation/commit time - they (ideally) should not.
This can be removed if I copy the non crtc_* parameters like I mentioned above. Thats the only reason dm_state was used.
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.
Will fix.
+ (!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.
This specifically checks for a change the current and previous state. In both the states, freesync_video_mode can be active. This condition would return false in the absence of additional OR with freesync_video_mode. If freesync_video_mode is active, we want this block of code to get executed. If freesync_video_mode is not active, then comparison with old and new state is sufficient. I can add this logic into another function and keep it cleaner here.
I think acrtc_state->freesync_video_mode is redundant - it doesn't check prev/new, it only checks whether or not it's active. This should be already checked in amdgpu_dm_vrr_active(), and the:
amdgpu_dm_vrr_active(dm_old_crtc_state) != amdgpu_dm_vrr_active(acrtc_state)
should be doing this for you.
This is exactly what the code was originally. The problem here is that checking the previous and next state difference is not enough, which is why I had to create an additional paramter to know whether we are currently in freesync mode. But this can be avoided, if we have an early return if fixed vrr mode is active, and an additional check for the fixed vrr state during modeset. Will add this to next iteration.
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.
In the second branch, since we call is_freesync_video_mode(), the check with the module parameter was also necessary as this is the triggering point for the VRR api configuration change. Also, the first branch skips the modeset, while the second doesnt. So we cannot club them together. The second branch is for the case when a freesync mode was set accompanied by a modeset. This happens when the previous mode had different base timings and hence a modeset is necessary.
Thanks & Regards, Aurabindo Pillai
Okay, I understand this block now. Thanks.
Regards, Nicholas Kazlauskas
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;
On Tue, 19 Jan 2021 10:50:26 -0500 Aurabindo Pillai aurabindo.pillai@amd.com wrote:
Changes in V5
- More info in commit messages on the rationale of changes being added
to the kernel.
- Minor fixes
Hi,
thank you for adding the explanations in the commit messages I asked for. It is now up to DRM maintainers to determine if this is conceptually fine.
I do see that apparently setting the opt-in option does not yet taint the kernel although Daniel Vetter suggested it might be a good idea. I guess tainting the kernel would make it easier to remove this feature in the future because it would be easier to dismiss people that claim a kernel regression due to the removal.
Thanks, pq
--
This patchset enables freesync video mode usecase where the userspace can request a freesync compatible video mode such that switching to this mode does not trigger blanking.
This feature is guarded by a module parameter which is disabled by default. Enabling this paramters adds additional modes to the driver modelist, and also enables the optimization to skip modeset when using one of these modes.
--
Aurabindo Pillai (3): drm/amd/display: Add module parameter for freesync video mode drm/amd/display: Add freesync video modes based on preferred modes drm/amd/display: Skip modeset for front porch change
drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 + drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 12 + .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 401 ++++++++++++++++-- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 3 + 4 files changed, 382 insertions(+), 35 deletions(-)
On Fri, Jan 22, 2021 at 10:32:48AM +0200, Pekka Paalanen wrote:
On Tue, 19 Jan 2021 10:50:26 -0500 Aurabindo Pillai aurabindo.pillai@amd.com wrote:
Changes in V5
- More info in commit messages on the rationale of changes being added
to the kernel.
- Minor fixes
Hi,
thank you for adding the explanations in the commit messages I asked for. It is now up to DRM maintainers to determine if this is conceptually fine.
I do see that apparently setting the opt-in option does not yet taint the kernel although Daniel Vetter suggested it might be a good idea. I guess tainting the kernel would make it easier to remove this feature in the future because it would be easier to dismiss people that claim a kernel regression due to the removal.
Reading the descriptions I'm honestly not sure why this isn't enabled by default?
Maybe the explanations should also capture why this is maybe not a good idea ... -Daniel
Thanks, pq
--
This patchset enables freesync video mode usecase where the userspace can request a freesync compatible video mode such that switching to this mode does not trigger blanking.
This feature is guarded by a module parameter which is disabled by default. Enabling this paramters adds additional modes to the driver modelist, and also enables the optimization to skip modeset when using one of these modes.
--
Aurabindo Pillai (3): drm/amd/display: Add module parameter for freesync video mode drm/amd/display: Add freesync video modes based on preferred modes drm/amd/display: Skip modeset for front porch change
drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 + drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 12 + .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 401 ++++++++++++++++-- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 3 + 4 files changed, 382 insertions(+), 35 deletions(-)
On Fri, Jan 22, 2021 at 10:42:53AM +0100, Daniel Vetter wrote:
On Fri, Jan 22, 2021 at 10:32:48AM +0200, Pekka Paalanen wrote:
On Tue, 19 Jan 2021 10:50:26 -0500 Aurabindo Pillai aurabindo.pillai@amd.com wrote:
Changes in V5
- More info in commit messages on the rationale of changes being added
to the kernel.
- Minor fixes
Hi,
thank you for adding the explanations in the commit messages I asked for. It is now up to DRM maintainers to determine if this is conceptually fine.
I do see that apparently setting the opt-in option does not yet taint the kernel although Daniel Vetter suggested it might be a good idea. I guess tainting the kernel would make it easier to remove this feature in the future because it would be easier to dismiss people that claim a kernel regression due to the removal.
Reading the descriptions I'm honestly not sure why this isn't enabled by default?
Maybe the explanations should also capture why this is maybe not a good idea ...
And also, if this is a bad idea uapi-vise, then it definitely needs to be behind the tainting module option until we've sorted it out (and then just enable it by default once that's done). -Daniel
-Daniel
Thanks, pq
--
This patchset enables freesync video mode usecase where the userspace can request a freesync compatible video mode such that switching to this mode does not trigger blanking.
This feature is guarded by a module parameter which is disabled by default. Enabling this paramters adds additional modes to the driver modelist, and also enables the optimization to skip modeset when using one of these modes.
--
Aurabindo Pillai (3): drm/amd/display: Add module parameter for freesync video mode drm/amd/display: Add freesync video modes based on preferred modes drm/amd/display: Skip modeset for front porch change
drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 + drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 12 + .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 401 ++++++++++++++++-- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 3 + 4 files changed, 382 insertions(+), 35 deletions(-)
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Fri, Jan 22, 2021 at 4:43 AM Daniel Vetter daniel@ffwll.ch wrote:
On Fri, Jan 22, 2021 at 10:32:48AM +0200, Pekka Paalanen wrote:
On Tue, 19 Jan 2021 10:50:26 -0500 Aurabindo Pillai aurabindo.pillai@amd.com wrote:
Changes in V5
- More info in commit messages on the rationale of changes being added
to the kernel.
- Minor fixes
Hi,
thank you for adding the explanations in the commit messages I asked for. It is now up to DRM maintainers to determine if this is conceptually fine.
I do see that apparently setting the opt-in option does not yet taint the kernel although Daniel Vetter suggested it might be a good idea. I guess tainting the kernel would make it easier to remove this feature in the future because it would be easier to dismiss people that claim a kernel regression due to the removal.
Reading the descriptions I'm honestly not sure why this isn't enabled by default?
Maybe the explanations should also capture why this is maybe not a good idea ...
I don't know that it's a bad idea, I guess we are just cautious to avoid unforeseen consequences until we have more real world experience using the feature. One issue brought up as a possible problem, but not actually verified to my knowledge, was the possibility of using a bit more power with freesync modes. E.g., if you just change the front porch to set the refresh to 48 Hz on a 90Hz mode, you are technically running the display phys at the higher clock speed so you can seamlessly transition to 90Hz or whatever. I don't know that it uses that much additional power and lets you get some nice features in a relatively seamless manner that already works today with most media players.
I guess for media player folks, is this something you'd like as is? A more explicit API may be nicer, but I think I think the ultimate solution will end up getting tied up in what are plans are for bigger features like more advanced flip queues and stuff like that. So this seems like a nice intermediate step.
Alex
-Daniel
Thanks, pq
--
This patchset enables freesync video mode usecase where the userspace can request a freesync compatible video mode such that switching to this mode does not trigger blanking.
This feature is guarded by a module parameter which is disabled by default. Enabling this paramters adds additional modes to the driver modelist, and also enables the optimization to skip modeset when using one of these modes.
--
Aurabindo Pillai (3): drm/amd/display: Add module parameter for freesync video mode drm/amd/display: Add freesync video modes based on preferred modes drm/amd/display: Skip modeset for front porch change
drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 + drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 12 + .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 401 ++++++++++++++++-- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 3 + 4 files changed, 382 insertions(+), 35 deletions(-)
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Jan 22, 2021 at 10:16:45AM -0500, Alex Deucher wrote:
On Fri, Jan 22, 2021 at 4:43 AM Daniel Vetter daniel@ffwll.ch wrote:
On Fri, Jan 22, 2021 at 10:32:48AM +0200, Pekka Paalanen wrote:
On Tue, 19 Jan 2021 10:50:26 -0500 Aurabindo Pillai aurabindo.pillai@amd.com wrote:
Changes in V5
- More info in commit messages on the rationale of changes being added
to the kernel.
- Minor fixes
Hi,
thank you for adding the explanations in the commit messages I asked for. It is now up to DRM maintainers to determine if this is conceptually fine.
I do see that apparently setting the opt-in option does not yet taint the kernel although Daniel Vetter suggested it might be a good idea. I guess tainting the kernel would make it easier to remove this feature in the future because it would be easier to dismiss people that claim a kernel regression due to the removal.
Reading the descriptions I'm honestly not sure why this isn't enabled by default?
Maybe the explanations should also capture why this is maybe not a good idea ...
I don't know that it's a bad idea, I guess we are just cautious to avoid unforeseen consequences until we have more real world experience using the feature. One issue brought up as a possible problem, but not actually verified to my knowledge, was the possibility of using a bit more power with freesync modes. E.g., if you just change the front porch to set the refresh to 48 Hz on a 90Hz mode, you are technically running the display phys at the higher clock speed so you can seamlessly transition to 90Hz or whatever. I don't know that it uses that much additional power and lets you get some nice features in a relatively seamless manner that already works today with most media players.
Hm yeah that's a good point. You can't change the dotclock to something lower on the fly? We have that on some intel laptops, but it's kinda been superseeded with manual update (PSR on DP) in most places, and then everything is shut down anyway. But either of these happen automatically, and we even have self refresh helpers now if you don't have hw assitance for these to automatically ramp back up to max refresh rate (Intel loves baking stuff into hw a bit too much, so drm/i915 isn't using it).
I guess for media player folks, is this something you'd like as is? A more explicit API may be nicer, but I think I think the ultimate solution will end up getting tied up in what are plans are for bigger features like more advanced flip queues and stuff like that. So this seems like a nice intermediate step.
Yeah I think knobs to figure out details is good, but ime they're just have a tendency to stay around forever and eventually we suffer from combinatorial explosion and also a bit (for this one here) uapi fragmentation. Which isn't nice if the solution to a user problem is "add this obscure kernel cmdline to your grub file".
That's why I think the taining module param is a good interim step for figuring stuff out, while reducing a bit the risk it'll become a permanent thing users have to fiddle.
Cheers, Daniel
Alex
-Daniel
Thanks, pq
--
This patchset enables freesync video mode usecase where the userspace can request a freesync compatible video mode such that switching to this mode does not trigger blanking.
This feature is guarded by a module parameter which is disabled by default. Enabling this paramters adds additional modes to the driver modelist, and also enables the optimization to skip modeset when using one of these modes.
--
Aurabindo Pillai (3): drm/amd/display: Add module parameter for freesync video mode drm/amd/display: Add freesync video modes based on preferred modes drm/amd/display: Skip modeset for front porch change
drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 + drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 12 + .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 401 ++++++++++++++++-- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 3 + 4 files changed, 382 insertions(+), 35 deletions(-)
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Aurabindo Pillai -
Daniel Vetter -
Kazlauskas, Nicholas -
Pekka Paalanen