* Re: [PATCH 3/3] drm/amd/display: Skip modeset for front porch change
2020-12-10 2:45 ` [PATCH 3/3] drm/amd/display: Skip modeset for front porch change Aurabindo Pillai
@ 2020-12-10 10:21 ` Christian König
2020-12-10 12:59 ` Shashank Sharma
2021-01-04 16:16 ` Kazlauskas, Nicholas
2 siblings, 0 replies; 29+ messages in thread
From: Christian König @ 2020-12-10 10:21 UTC (permalink / raw)
To: Aurabindo Pillai, amd-gfx
Cc: stylon.wang, thong.thai, shashank.sharma, wayne.lin,
alexander.deucher, Harry.Wentland, nicholas.kazlauskas
Am 10.12.20 um 03:45 schrieb Aurabindo Pillai:
> [Why&How]
> Inorder to enable freesync video mode, driver adds extra
> modes based on preferred modes for common freesync frame rates.
> When commiting these mode changes, a full modeset is not needed.
> If the change in only in the front porch timing value, skip full
> modeset and continue using the same stream.
I would drop the test for amdgpu_exp_freesync_vid_mode here since this
is a valid optimization independent if we add the extra modes or not in
the kernel.
But that is not a hard requirement and apart from that the series looks
good to me. Feel free to add an Acked-by: Christian König
<christian.koenig@amd.com>.
Christian.
>
> Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 169 ++++++++++++++++--
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 +
> 2 files changed, 153 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index f699a3d41cad..c8c72887906a 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -217,6 +217,9 @@ static bool amdgpu_dm_psr_disable_all(struct amdgpu_display_manager *dm);
> static const struct drm_format_info *
> amd_get_format_info(const struct drm_mode_fb_cmd2 *cmd);
>
> +static bool
> +is_timing_unchanged_for_freesync(struct drm_crtc_state *old_crtc_state,
> + struct drm_crtc_state *new_crtc_state);
> /*
> * dm_vblank_get_counter
> *
> @@ -5096,8 +5099,11 @@ copy_crtc_timing_for_drm_display_mode(const struct drm_display_mode *src_mode,
> static void
> decide_crtc_timing_for_drm_display_mode(struct drm_display_mode *drm_mode,
> const struct drm_display_mode *native_mode,
> - bool scale_enabled)
> + bool scale_enabled, bool fs_mode)
> {
> + if (fs_mode)
> + return;
> +
> if (scale_enabled) {
> copy_crtc_timing_for_drm_display_mode(native_mode, drm_mode);
> } else if (native_mode->clock == drm_mode->clock &&
> @@ -5241,6 +5247,24 @@ get_highest_freesync_mode(struct amdgpu_dm_connector *aconnector,
> return m_high;
> }
>
> +static bool is_freesync_video_mode(struct drm_display_mode *mode,
> + struct amdgpu_dm_connector *aconnector)
> +{
> + struct drm_display_mode *high_mode;
> +
> + high_mode = get_highest_freesync_mode(aconnector, false);
> + if (!high_mode)
> + return false;
> +
> + if (high_mode->clock == 0 ||
> + high_mode->hdisplay != mode->hdisplay ||
> + high_mode->clock != mode->clock ||
> + !mode)
> + return false;
> + else
> + return true;
> +}
> +
> static struct dc_stream_state *
> create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
> const struct drm_display_mode *drm_mode,
> @@ -5253,17 +5277,21 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
> const struct drm_connector_state *con_state =
> dm_state ? &dm_state->base : NULL;
> struct dc_stream_state *stream = NULL;
> - struct drm_display_mode mode = *drm_mode;
> + struct drm_display_mode saved_mode, mode = *drm_mode;
> + struct drm_display_mode *freesync_mode = NULL;
> bool native_mode_found = false;
> bool scale = dm_state ? (dm_state->scaling != RMX_OFF) : false;
> int mode_refresh;
> int preferred_refresh = 0;
> + bool is_fs_vid_mode = 0;
> #if defined(CONFIG_DRM_AMD_DC_DCN)
> struct dsc_dec_dpcd_caps dsc_caps;
> #endif
> uint32_t link_bandwidth_kbps;
> -
> struct dc_sink *sink = NULL;
> +
> + memset(&saved_mode, 0, sizeof(struct drm_display_mode));
> +
> if (aconnector == NULL) {
> DRM_ERROR("aconnector is NULL!\n");
> return stream;
> @@ -5316,20 +5344,33 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
> */
> DRM_DEBUG_DRIVER("No preferred mode found\n");
> } else {
> + is_fs_vid_mode = is_freesync_video_mode(&mode, aconnector);
> + if (is_fs_vid_mode) {
> + freesync_mode = get_highest_freesync_mode(aconnector, false);
> + if (freesync_mode) {
> + saved_mode = mode;
> + mode = *freesync_mode;
> + }
> + }
> +
> decide_crtc_timing_for_drm_display_mode(
> &mode, preferred_mode,
> - dm_state ? (dm_state->scaling != RMX_OFF) : false);
> + dm_state ? (dm_state->scaling != RMX_OFF) : false,
> + freesync_mode ? true : false);
> preferred_refresh = drm_mode_vrefresh(preferred_mode);
> }
>
> if (!dm_state)
> drm_mode_set_crtcinfo(&mode, 0);
>
> - /*
> + if (dm_state && is_fs_vid_mode && freesync_mode)
> + drm_mode_set_crtcinfo(&saved_mode, 0);
> +
> + /*
> * If scaling is enabled and refresh rate didn't change
> * we copy the vic and polarities of the old timings
> */
> - if (!scale || mode_refresh != preferred_refresh)
> + if (!(scale && freesync_mode) || mode_refresh != preferred_refresh)
> fill_stream_properties_from_drm_display_mode(stream,
> &mode, &aconnector->base, con_state, NULL, requested_bpc);
> else
> @@ -7881,13 +7922,29 @@ static void update_stream_irq_parameters(
> if (new_crtc_state->vrr_supported &&
> config.min_refresh_in_uhz &&
> config.max_refresh_in_uhz) {
> + /*
> + * if freesync compatible mode was set, config.state will be set
> + * in atomic check
> + */
> + if (config.state == VRR_STATE_ACTIVE_FIXED &&
> + config.fixed_refresh_in_uhz && config.max_refresh_in_uhz &&
> + config.min_refresh_in_uhz &&
> + (!drm_atomic_crtc_needs_modeset(&new_crtc_state->base) ||
> + new_crtc_state->freesync_video_mode)) {
> + vrr_params.max_refresh_in_uhz = config.max_refresh_in_uhz;
> + vrr_params.min_refresh_in_uhz = config.min_refresh_in_uhz;
> + vrr_params.fixed_refresh_in_uhz = config.fixed_refresh_in_uhz;
> + vrr_params.state = VRR_STATE_ACTIVE_FIXED;
> + goto out;
> + }
> +
> config.state = new_crtc_state->base.vrr_enabled ?
> VRR_STATE_ACTIVE_VARIABLE :
> VRR_STATE_INACTIVE;
> - } else {
> + } else
> config.state = VRR_STATE_UNSUPPORTED;
> - }
>
> +out:
> mod_freesync_build_vrr_params(dm->freesync_module,
> new_stream,
> &config, &vrr_params);
> @@ -8205,7 +8262,9 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
> * as part of commit.
> */
> if (amdgpu_dm_vrr_active(dm_old_crtc_state) !=
> - amdgpu_dm_vrr_active(acrtc_state)) {
> + amdgpu_dm_vrr_active(acrtc_state) ||
> + acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_FIXED ||
> + acrtc_state->freesync_video_mode) {
> spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
> dc_stream_adjust_vmin_vmax(
> dm->dc, acrtc_state->stream,
> @@ -8896,6 +8955,7 @@ static void get_freesync_config_for_crtc(
> to_amdgpu_dm_connector(new_con_state->base.connector);
> struct drm_display_mode *mode = &new_crtc_state->base.mode;
> int vrefresh = drm_mode_vrefresh(mode);
> + bool fs_vid_mode = false;
>
> new_crtc_state->vrr_supported = new_con_state->freesync_capable &&
> vrefresh >= aconnector->min_vfreq &&
> @@ -8903,17 +8963,26 @@ static void get_freesync_config_for_crtc(
>
> if (new_crtc_state->vrr_supported) {
> new_crtc_state->stream->ignore_msa_timing_param = true;
> - config.state = new_crtc_state->base.vrr_enabled ?
> - VRR_STATE_ACTIVE_VARIABLE :
> - VRR_STATE_INACTIVE;
> - config.min_refresh_in_uhz =
> - aconnector->min_vfreq * 1000000;
> - config.max_refresh_in_uhz =
> - aconnector->max_vfreq * 1000000;
> + fs_vid_mode = new_crtc_state->freesync_config.state == VRR_STATE_ACTIVE_FIXED ||
> + new_crtc_state->freesync_video_mode;
> +
> + config.min_refresh_in_uhz = aconnector->min_vfreq * 1000000;
> + config.max_refresh_in_uhz = aconnector->max_vfreq * 1000000;
> config.vsif_supported = true;
> config.btr = true;
> - }
>
> + if (fs_vid_mode) {
> + config.state = VRR_STATE_ACTIVE_FIXED;
> + config.fixed_refresh_in_uhz = new_crtc_state->freesync_config.fixed_refresh_in_uhz;
> + goto out;
> + }
> + else if (new_crtc_state->base.vrr_enabled && !fs_vid_mode)
> + config.state = VRR_STATE_ACTIVE_VARIABLE;
> + else
> + config.state = VRR_STATE_INACTIVE;
> +
> + }
> +out:
> new_crtc_state->freesync_config = config;
> }
>
> @@ -8926,6 +8995,51 @@ static void reset_freesync_config_for_crtc(
> sizeof(new_crtc_state->vrr_infopacket));
> }
>
> +static bool
> +is_timing_unchanged_for_freesync(struct drm_crtc_state *old_crtc_state,
> + struct drm_crtc_state *new_crtc_state)
> +{
> + struct drm_display_mode old_mode, new_mode;
> +
> + if (!old_crtc_state || !new_crtc_state)
> + return false;
> +
> + old_mode = old_crtc_state->mode;
> + new_mode = new_crtc_state->mode;
> +
> + if (old_mode.clock == new_mode.clock &&
> + old_mode.hdisplay == new_mode.hdisplay &&
> + old_mode.vdisplay == new_mode.vdisplay &&
> + old_mode.htotal == new_mode.htotal &&
> + old_mode.vtotal != new_mode.vtotal &&
> + old_mode.hsync_start == new_mode.hsync_start &&
> + old_mode.vsync_start != new_mode.vsync_start &&
> + old_mode.hsync_end == new_mode.hsync_end &&
> + old_mode.vsync_end != new_mode.vsync_end &&
> + old_mode.hskew == new_mode.hskew &&
> + old_mode.vscan == new_mode.vscan &&
> + (old_mode.vsync_end - old_mode.vsync_start) ==
> + (new_mode.vsync_end - new_mode.vsync_start))
> + return true;
> +
> + return false;
> +}
> +
> +static void set_freesync_fixed_config(struct dm_crtc_state *dm_new_crtc_state) {
> + uint64_t num, den, res;
> + struct drm_crtc_state *new_crtc_state = &dm_new_crtc_state->base;
> +
> + dm_new_crtc_state->freesync_config.state = VRR_STATE_ACTIVE_FIXED;
> +
> + num = (unsigned long long)new_crtc_state->mode.clock * 1000 * 1000000;
> + den = (unsigned long long)new_crtc_state->mode.htotal *
> + (unsigned long long)new_crtc_state->mode.vtotal;
> +
> + res = div_u64(num, den);
> + dm_new_crtc_state->freesync_config.fixed_refresh_in_uhz = res;
> + dm_new_crtc_state->freesync_video_mode = true;
> +}
> +
> static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
> struct drm_atomic_state *state,
> struct drm_crtc *crtc,
> @@ -9016,6 +9130,11 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
> * TODO: Refactor this function to allow this check to work
> * in all conditions.
> */
> + if (dm_new_crtc_state->stream &&
> + is_timing_unchanged_for_freesync(new_crtc_state, old_crtc_state) &&
> + amdgpu_exp_freesync_vid_mode)
> + goto skip_modeset;
> +
> if (dm_new_crtc_state->stream &&
> dc_is_stream_unchanged(new_stream, dm_old_crtc_state->stream) &&
> dc_is_stream_scaling_unchanged(new_stream, dm_old_crtc_state->stream)) {
> @@ -9047,6 +9166,22 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
> if (!dm_old_crtc_state->stream)
> goto skip_modeset;
>
> + if (dm_new_crtc_state->stream &&
> + is_timing_unchanged_for_freesync(new_crtc_state, old_crtc_state) &&
> + amdgpu_exp_freesync_vid_mode) {
> + new_crtc_state->mode_changed = false;
> + DRM_DEBUG_DRIVER(
> + "Mode change not required for front porch change, "
> + "setting mode_changed to %d",
> + new_crtc_state->mode_changed);
> +
> + set_freesync_fixed_config(dm_new_crtc_state);
> +
> + goto skip_modeset;
> + } else if (aconnector &&
> + is_freesync_video_mode(&new_crtc_state->mode, aconnector))
> + set_freesync_fixed_config(dm_new_crtc_state);
> +
> ret = dm_atomic_get_state(state, &dm_state);
> if (ret)
> goto fail;
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index 251af783f6b1..28f2d8c9b260 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -453,6 +453,7 @@ struct dm_crtc_state {
>
> bool freesync_timing_changed;
> bool freesync_vrr_info_changed;
> + bool freesync_video_mode;
>
> bool dsc_force_changed;
> bool vrr_supported;
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/3] drm/amd/display: Skip modeset for front porch change
2020-12-10 2:45 ` [PATCH 3/3] drm/amd/display: Skip modeset for front porch change Aurabindo Pillai
2020-12-10 10:21 ` Christian König
@ 2020-12-10 12:59 ` Shashank Sharma
2020-12-10 17:50 ` Aurabindo Pillai
2021-01-04 16:16 ` Kazlauskas, Nicholas
2 siblings, 1 reply; 29+ messages in thread
From: Shashank Sharma @ 2020-12-10 12:59 UTC (permalink / raw)
To: Aurabindo Pillai, amd-gfx
Cc: stylon.wang, thong.thai, wayne.lin, alexander.deucher,
Harry.Wentland, nicholas.kazlauskas
On 10/12/20 8:15 am, Aurabindo Pillai wrote:
> [Why&How]
> Inorder to enable freesync video mode, driver adds extra
> modes based on preferred modes for common freesync frame rates.
> When commiting these mode changes, a full modeset is not needed.
> If the change in only in the front porch timing value, skip full
> modeset and continue using the same stream.
>
> Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 169 ++++++++++++++++--
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 +
> 2 files changed, 153 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index f699a3d41cad..c8c72887906a 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -217,6 +217,9 @@ static bool amdgpu_dm_psr_disable_all(struct amdgpu_display_manager *dm);
> static const struct drm_format_info *
> amd_get_format_info(const struct drm_mode_fb_cmd2 *cmd);
>
> +static bool
> +is_timing_unchanged_for_freesync(struct drm_crtc_state *old_crtc_state,
> + struct drm_crtc_state *new_crtc_state);
> /*
> * dm_vblank_get_counter
> *
> @@ -5096,8 +5099,11 @@ copy_crtc_timing_for_drm_display_mode(const struct drm_display_mode *src_mode,
> static void
> decide_crtc_timing_for_drm_display_mode(struct drm_display_mode *drm_mode,
> const struct drm_display_mode *native_mode,
> - bool scale_enabled)
> + bool scale_enabled, bool fs_mode)
> {
> + if (fs_mode)
> + return;
so we are adding an input flag just so that we can return from the function at top ? How about adding this check at the caller without changing the function parameters ?
> +
> if (scale_enabled) {
> copy_crtc_timing_for_drm_display_mode(native_mode, drm_mode);
> } else if (native_mode->clock == drm_mode->clock &&
> @@ -5241,6 +5247,24 @@ get_highest_freesync_mode(struct amdgpu_dm_connector *aconnector,
> return m_high;
> }
>
> +static bool is_freesync_video_mode(struct drm_display_mode *mode,
> + struct amdgpu_dm_connector *aconnector)
> +{
> + struct drm_display_mode *high_mode;
> +
I thought we were adding a string "_FSV" in the end for the mode->name, why can't we check that instead of going through the whole list of modes again ?
> + high_mode = get_highest_freesync_mode(aconnector, false);
> + if (!high_mode)
> + return false;
> +
> + if (high_mode->clock == 0 ||
> + high_mode->hdisplay != mode->hdisplay ||
> + high_mode->clock != mode->clock ||
> + !mode)
> + return false;
> + else
> + return true;
> +}
> +
> static struct dc_stream_state *
> create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
> const struct drm_display_mode *drm_mode,
> @@ -5253,17 +5277,21 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
> const struct drm_connector_state *con_state =
> dm_state ? &dm_state->base : NULL;
> struct dc_stream_state *stream = NULL;
> - struct drm_display_mode mode = *drm_mode;
> + struct drm_display_mode saved_mode, mode = *drm_mode;
How about shifting this definition to new line to follow the existing convention ?
> + struct drm_display_mode *freesync_mode = NULL;
> bool native_mode_found = false;
> bool scale = dm_state ? (dm_state->scaling != RMX_OFF) : false;
> int mode_refresh;
> int preferred_refresh = 0;
> + bool is_fs_vid_mode = 0;
> #if defined(CONFIG_DRM_AMD_DC_DCN)
> struct dsc_dec_dpcd_caps dsc_caps;
> #endif
> uint32_t link_bandwidth_kbps;
> -
> struct dc_sink *sink = NULL;
> +
> + memset(&saved_mode, 0, sizeof(struct drm_display_mode));
> +
> if (aconnector == NULL) {
> DRM_ERROR("aconnector is NULL!\n");
> return stream;
> @@ -5316,20 +5344,33 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
> */
> DRM_DEBUG_DRIVER("No preferred mode found\n");
> } else {
> + is_fs_vid_mode = is_freesync_video_mode(&mode, aconnector);
> + if (is_fs_vid_mode) {
> + freesync_mode = get_highest_freesync_mode(aconnector, false);
> + if (freesync_mode) {
As the freesync modes are being added by the driver, and we have passed one check which says is_fs_vid_mode, will it ever be the case where freesync_mode == NULL ? Ideally we should get atleast one mode equal to this isn't it ? in that case we can drop one if () check.
> + saved_mode = mode;
> + mode = *freesync_mode;
> + }
> + }
> +
> decide_crtc_timing_for_drm_display_mode(
> &mode, preferred_mode,
> - dm_state ? (dm_state->scaling != RMX_OFF) : false);
> + dm_state ? (dm_state->scaling != RMX_OFF) : false,
> + freesync_mode ? true : false);
> preferred_refresh = drm_mode_vrefresh(preferred_mode);
> }
>
> if (!dm_state)
> drm_mode_set_crtcinfo(&mode, 0);
>
> - /*
> + if (dm_state && is_fs_vid_mode && freesync_mode)
Same here, I guess if is_fs_vide_mode == true, freesync_mode must never be NULL
- Shashank
> + drm_mode_set_crtcinfo(&saved_mode, 0);
> +
> + /*
> * If scaling is enabled and refresh rate didn't change
> * we copy the vic and polarities of the old timings
> */
> - if (!scale || mode_refresh != preferred_refresh)
> + if (!(scale && freesync_mode) || mode_refresh != preferred_refresh)
> fill_stream_properties_from_drm_display_mode(stream,
> &mode, &aconnector->base, con_state, NULL, requested_bpc);
> else
> @@ -7881,13 +7922,29 @@ static void update_stream_irq_parameters(
> if (new_crtc_state->vrr_supported &&
> config.min_refresh_in_uhz &&
> config.max_refresh_in_uhz) {
> + /*
> + * if freesync compatible mode was set, config.state will be set
> + * in atomic check
> + */
> + if (config.state == VRR_STATE_ACTIVE_FIXED &&
> + config.fixed_refresh_in_uhz && config.max_refresh_in_uhz &&
> + config.min_refresh_in_uhz &&
> + (!drm_atomic_crtc_needs_modeset(&new_crtc_state->base) ||
> + new_crtc_state->freesync_video_mode)) {
> + vrr_params.max_refresh_in_uhz = config.max_refresh_in_uhz;
> + vrr_params.min_refresh_in_uhz = config.min_refresh_in_uhz;
> + vrr_params.fixed_refresh_in_uhz = config.fixed_refresh_in_uhz;
> + vrr_params.state = VRR_STATE_ACTIVE_FIXED;
> + goto out;
> + }
> +
> config.state = new_crtc_state->base.vrr_enabled ?
> VRR_STATE_ACTIVE_VARIABLE :
> VRR_STATE_INACTIVE;
> - } else {
> + } else
> config.state = VRR_STATE_UNSUPPORTED;
> - }
>
> +out:
> mod_freesync_build_vrr_params(dm->freesync_module,
> new_stream,
> &config, &vrr_params);
> @@ -8205,7 +8262,9 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
> * as part of commit.
> */
> if (amdgpu_dm_vrr_active(dm_old_crtc_state) !=
> - amdgpu_dm_vrr_active(acrtc_state)) {
> + amdgpu_dm_vrr_active(acrtc_state) ||
> + acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_FIXED ||
> + acrtc_state->freesync_video_mode) {
> spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
> dc_stream_adjust_vmin_vmax(
> dm->dc, acrtc_state->stream,
> @@ -8896,6 +8955,7 @@ static void get_freesync_config_for_crtc(
> to_amdgpu_dm_connector(new_con_state->base.connector);
> struct drm_display_mode *mode = &new_crtc_state->base.mode;
> int vrefresh = drm_mode_vrefresh(mode);
> + bool fs_vid_mode = false;
>
> new_crtc_state->vrr_supported = new_con_state->freesync_capable &&
> vrefresh >= aconnector->min_vfreq &&
> @@ -8903,17 +8963,26 @@ static void get_freesync_config_for_crtc(
>
> if (new_crtc_state->vrr_supported) {
> new_crtc_state->stream->ignore_msa_timing_param = true;
> - config.state = new_crtc_state->base.vrr_enabled ?
> - VRR_STATE_ACTIVE_VARIABLE :
> - VRR_STATE_INACTIVE;
> - config.min_refresh_in_uhz =
> - aconnector->min_vfreq * 1000000;
> - config.max_refresh_in_uhz =
> - aconnector->max_vfreq * 1000000;
> + fs_vid_mode = new_crtc_state->freesync_config.state == VRR_STATE_ACTIVE_FIXED ||
> + new_crtc_state->freesync_video_mode;
> +
> + config.min_refresh_in_uhz = aconnector->min_vfreq * 1000000;
> + config.max_refresh_in_uhz = aconnector->max_vfreq * 1000000;
> config.vsif_supported = true;
> config.btr = true;
> - }
>
> + if (fs_vid_mode) {
> + config.state = VRR_STATE_ACTIVE_FIXED;
> + config.fixed_refresh_in_uhz = new_crtc_state->freesync_config.fixed_refresh_in_uhz;
> + goto out;
> + }
> + else if (new_crtc_state->base.vrr_enabled && !fs_vid_mode)
> + config.state = VRR_STATE_ACTIVE_VARIABLE;
> + else
> + config.state = VRR_STATE_INACTIVE;
> +
> + }
> +out:
> new_crtc_state->freesync_config = config;
> }
>
> @@ -8926,6 +8995,51 @@ static void reset_freesync_config_for_crtc(
> sizeof(new_crtc_state->vrr_infopacket));
> }
>
> +static bool
> +is_timing_unchanged_for_freesync(struct drm_crtc_state *old_crtc_state,
> + struct drm_crtc_state *new_crtc_state)
> +{
> + struct drm_display_mode old_mode, new_mode;
> +
> + if (!old_crtc_state || !new_crtc_state)
> + return false;
> +
> + old_mode = old_crtc_state->mode;
> + new_mode = new_crtc_state->mode;
> +
> + if (old_mode.clock == new_mode.clock &&
> + old_mode.hdisplay == new_mode.hdisplay &&
> + old_mode.vdisplay == new_mode.vdisplay &&
> + old_mode.htotal == new_mode.htotal &&
> + old_mode.vtotal != new_mode.vtotal &&
> + old_mode.hsync_start == new_mode.hsync_start &&
> + old_mode.vsync_start != new_mode.vsync_start &&
> + old_mode.hsync_end == new_mode.hsync_end &&
> + old_mode.vsync_end != new_mode.vsync_end &&
> + old_mode.hskew == new_mode.hskew &&
> + old_mode.vscan == new_mode.vscan &&
> + (old_mode.vsync_end - old_mode.vsync_start) ==
> + (new_mode.vsync_end - new_mode.vsync_start))
> + return true;
> +
> + return false;
> +}
> +
> +static void set_freesync_fixed_config(struct dm_crtc_state *dm_new_crtc_state) {
> + uint64_t num, den, res;
> + struct drm_crtc_state *new_crtc_state = &dm_new_crtc_state->base;
> +
> + dm_new_crtc_state->freesync_config.state = VRR_STATE_ACTIVE_FIXED;
> +
> + num = (unsigned long long)new_crtc_state->mode.clock * 1000 * 1000000;
> + den = (unsigned long long)new_crtc_state->mode.htotal *
> + (unsigned long long)new_crtc_state->mode.vtotal;
> +
> + res = div_u64(num, den);
> + dm_new_crtc_state->freesync_config.fixed_refresh_in_uhz = res;
> + dm_new_crtc_state->freesync_video_mode = true;
> +}
> +
> static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
> struct drm_atomic_state *state,
> struct drm_crtc *crtc,
> @@ -9016,6 +9130,11 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
> * TODO: Refactor this function to allow this check to work
> * in all conditions.
> */
> + if (dm_new_crtc_state->stream &&
> + is_timing_unchanged_for_freesync(new_crtc_state, old_crtc_state) &&
> + amdgpu_exp_freesync_vid_mode)
> + goto skip_modeset;
> +
> if (dm_new_crtc_state->stream &&
> dc_is_stream_unchanged(new_stream, dm_old_crtc_state->stream) &&
> dc_is_stream_scaling_unchanged(new_stream, dm_old_crtc_state->stream)) {
> @@ -9047,6 +9166,22 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
> if (!dm_old_crtc_state->stream)
> goto skip_modeset;
>
> + if (dm_new_crtc_state->stream &&
> + is_timing_unchanged_for_freesync(new_crtc_state, old_crtc_state) &&
> + amdgpu_exp_freesync_vid_mode) {
> + new_crtc_state->mode_changed = false;
> + DRM_DEBUG_DRIVER(
> + "Mode change not required for front porch change, "
> + "setting mode_changed to %d",
> + new_crtc_state->mode_changed);
> +
> + set_freesync_fixed_config(dm_new_crtc_state);
> +
> + goto skip_modeset;
> + } else if (aconnector &&
> + is_freesync_video_mode(&new_crtc_state->mode, aconnector))
> + set_freesync_fixed_config(dm_new_crtc_state);
> +
> ret = dm_atomic_get_state(state, &dm_state);
> if (ret)
> goto fail;
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index 251af783f6b1..28f2d8c9b260 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -453,6 +453,7 @@ struct dm_crtc_state {
>
> bool freesync_timing_changed;
> bool freesync_vrr_info_changed;
> + bool freesync_video_mode;
>
> bool dsc_force_changed;
> bool vrr_supported;
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/3] drm/amd/display: Skip modeset for front porch change
2020-12-10 12:59 ` Shashank Sharma
@ 2020-12-10 17:50 ` Aurabindo Pillai
2020-12-11 5:08 ` Shashank Sharma
0 siblings, 1 reply; 29+ messages in thread
From: Aurabindo Pillai @ 2020-12-10 17:50 UTC (permalink / raw)
To: Shashank Sharma, amd-gfx
Cc: stylon.wang, thong.thai, wayne.lin, alexander.deucher,
Harry.Wentland, nicholas.kazlauskas
[-- Attachment #1.1: Type: text/plain, Size: 5527 bytes --]
On Thu, 2020-12-10 at 18:29 +0530, Shashank Sharma wrote:
> On 10/12/20 8:15 am, Aurabindo Pillai wrote:
> > [Why&How]
> > Inorder to enable freesync video mode, driver adds extra
> > modes based on preferred modes for common freesync frame rates.
> > When commiting these mode changes, a full modeset is not needed.
> > If the change in only in the front porch timing value, skip full
> > modeset and continue using the same stream.
> >
> > Signed-off-by: Aurabindo Pillai <
> > aurabindo.pillai@amd.com
> > >
> > ---
> > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 169
> > ++++++++++++++++--
> > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 +
> > 2 files changed, 153 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > index f699a3d41cad..c8c72887906a 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -217,6 +217,9 @@ static bool amdgpu_dm_psr_disable_all(struct
> > amdgpu_display_manager *dm);
> > static const struct drm_format_info *
> > amd_get_format_info(const struct drm_mode_fb_cmd2 *cmd);
> >
> > +static bool
> > +is_timing_unchanged_for_freesync(struct drm_crtc_state
> > *old_crtc_state,
> > + struct drm_crtc_state
> > *new_crtc_state);
> > /*
> > * dm_vblank_get_counter
> > *
> > @@ -5096,8 +5099,11 @@ copy_crtc_timing_for_drm_display_mode(const
> > struct drm_display_mode *src_mode,
> > static void
> > decide_crtc_timing_for_drm_display_mode(struct drm_display_mode
> > *drm_mode,
> > const struct drm_display_mode
> > *native_mode,
> > - bool scale_enabled)
> > + bool scale_enabled, bool
> > fs_mode)
> > {
> > + if (fs_mode)
> > + return;
>
> so we are adding an input flag just so that we can return from the
> function at top ? How about adding this check at the caller without
> changing the function parameters ?
Will fix this.
> > +
> > if (scale_enabled) {
> > copy_crtc_timing_for_drm_display_mode(native_mode,
> > drm_mode);
> > } else if (native_mode->clock == drm_mode->clock &&
> > @@ -5241,6 +5247,24 @@ get_highest_freesync_mode(struct
> > amdgpu_dm_connector *aconnector,
> > return m_high;
> > }
> >
> > +static bool is_freesync_video_mode(struct drm_display_mode *mode,
> > + struct amdgpu_dm_connector
> > *aconnector)
> > +{
> > + struct drm_display_mode *high_mode;
> > +
>
> I thought we were adding a string "_FSV" in the end for the mode-
> >name, why can't we check that instead of going through the whole
> list of modes again ?
Actually I only added _FSV to distinguish the newly added modes easily.
On second thoughts, I'm not sure if there are any userspace
applications that might depend on parsing the mode name, for maybe to
print the resolution. I think its better not to break any such
assumptions if they do exist by any chance. I think I'll just remove
_FSV from the mode name. We already set DRM_MODE_TYPE_DRIVER for
userspace to recognize these additional modes, so it shouldnt be a
problem.
> > + high_mode = get_highest_freesync_mode(aconnector, false);
> > + if (!high_mode)
> > + return false;
> > +
> > + if (high_mode->clock == 0 ||
> > + high_mode->hdisplay != mode->hdisplay ||
> > + high_mode->clock != mode->clock ||
> > + !mode)
> > + return false;
> > + else
> > + return true;
> > +}
> > +
> > static struct dc_stream_state *
> > create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
> > const struct drm_display_mode *drm_mode,
> > @@ -5253,17 +5277,21 @@ create_stream_for_sink(struct
> > amdgpu_dm_connector *aconnector,
> > const struct drm_connector_state *con_state =
> > dm_state ? &dm_state->base : NULL;
> > struct dc_stream_state *stream = NULL;
> > - struct drm_display_mode mode = *drm_mode;
> > + struct drm_display_mode saved_mode, mode = *drm_mode;
>
> How about shifting this definition to new line to follow the existing
> convention ?
Sure.
> > + struct drm_display_mode *freesync_mode = NULL;
> > bool native_mode_found = false;
> > bool scale = dm_state ? (dm_state->scaling != RMX_OFF) : false;
> > int mode_refresh;
> > int preferred_refresh = 0;
> > + bool is_fs_vid_mode = 0;
> > #if defined(CONFIG_DRM_AMD_DC_DCN)
> > struct dsc_dec_dpcd_caps dsc_caps;
> > #endif
> > uint32_t link_bandwidth_kbps;
> > -
> > struct dc_sink *sink = NULL;
> > +
> > + memset(&saved_mode, 0, sizeof(struct drm_display_mode));
> > +
> > if (aconnector == NULL) {
> > DRM_ERROR("aconnector is NULL!\n");
> > return stream;
> > @@ -5316,20 +5344,33 @@ create_stream_for_sink(struct
> > amdgpu_dm_connector *aconnector,
> > */
> > DRM_DEBUG_DRIVER("No preferred mode found\n");
> > } else {
> > + is_fs_vid_mode = is_freesync_video_mode(&mode,
> > aconnector);
> > + if (is_fs_vid_mode) {
> > + freesync_mode =
> > get_highest_freesync_mode(aconnector, false);
> > + if (freesync_mode) {
>
> As the freesync modes are being added by the driver, and we have
> passed one check which says is_fs_vid_mode, will it ever be the case
> where freesync_mode == NULL ? Ideally we should get atleast one mode
> equal to this isn't it ? in that case we can drop one if () check.
Yes, thanks for catching this. Will fix.
--
Regards,
Aurabindo Pillai
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
[-- Attachment #2: Type: text/plain, Size: 154 bytes --]
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/3] drm/amd/display: Skip modeset for front porch change
2020-12-10 17:50 ` Aurabindo Pillai
@ 2020-12-11 5:08 ` Shashank Sharma
2020-12-11 14:49 ` Kazlauskas, Nicholas
0 siblings, 1 reply; 29+ messages in thread
From: Shashank Sharma @ 2020-12-11 5:08 UTC (permalink / raw)
To: Aurabindo Pillai, amd-gfx
Cc: stylon.wang, thong.thai, wayne.lin, alexander.deucher,
Harry.Wentland, nicholas.kazlauskas
On 10/12/20 11:20 pm, Aurabindo Pillai wrote:
> On Thu, 2020-12-10 at 18:29 +0530, Shashank Sharma wrote:
>> On 10/12/20 8:15 am, Aurabindo Pillai wrote:
>>> [Why&How]
>>> Inorder to enable freesync video mode, driver adds extra
>>> modes based on preferred modes for common freesync frame rates.
>>> When commiting these mode changes, a full modeset is not needed.
>>> If the change in only in the front porch timing value, skip full
>>> modeset and continue using the same stream.
>>>
>>> Signed-off-by: Aurabindo Pillai <
>>> aurabindo.pillai@amd.com
>>> ---
>>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 169
>>> ++++++++++++++++--
>>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 +
>>> 2 files changed, 153 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> index f699a3d41cad..c8c72887906a 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> @@ -217,6 +217,9 @@ static bool amdgpu_dm_psr_disable_all(struct
>>> amdgpu_display_manager *dm);
>>> static const struct drm_format_info *
>>> amd_get_format_info(const struct drm_mode_fb_cmd2 *cmd);
>>>
>>> +static bool
>>> +is_timing_unchanged_for_freesync(struct drm_crtc_state
>>> *old_crtc_state,
>>> + struct drm_crtc_state
>>> *new_crtc_state);
>>> /*
>>> * dm_vblank_get_counter
>>> *
>>> @@ -5096,8 +5099,11 @@ copy_crtc_timing_for_drm_display_mode(const
>>> struct drm_display_mode *src_mode,
>>> static void
>>> decide_crtc_timing_for_drm_display_mode(struct drm_display_mode
>>> *drm_mode,
>>> const struct drm_display_mode
>>> *native_mode,
>>> - bool scale_enabled)
>>> + bool scale_enabled, bool
>>> fs_mode)
>>> {
>>> + if (fs_mode)
>>> + return;
>> so we are adding an input flag just so that we can return from the
>> function at top ? How about adding this check at the caller without
>> changing the function parameters ?
> Will fix this.
>
>>> +
>>> if (scale_enabled) {
>>> copy_crtc_timing_for_drm_display_mode(native_mode,
>>> drm_mode);
>>> } else if (native_mode->clock == drm_mode->clock &&
>>> @@ -5241,6 +5247,24 @@ get_highest_freesync_mode(struct
>>> amdgpu_dm_connector *aconnector,
>>> return m_high;
>>> }
>>>
>>> +static bool is_freesync_video_mode(struct drm_display_mode *mode,
>>> + struct amdgpu_dm_connector
>>> *aconnector)
>>> +{
>>> + struct drm_display_mode *high_mode;
>>> +
>> I thought we were adding a string "_FSV" in the end for the mode-
>>> name, why can't we check that instead of going through the whole
>> list of modes again ?
> Actually I only added _FSV to distinguish the newly added modes easily.
> On second thoughts, I'm not sure if there are any userspace
> applications that might depend on parsing the mode name, for maybe to
> print the resolution. I think its better not to break any such
> assumptions if they do exist by any chance. I think I'll just remove
> _FSV from the mode name. We already set DRM_MODE_TYPE_DRIVER for
> userspace to recognize these additional modes, so it shouldnt be a
> problem.
Actually, I am rather happy with this, as in when we want to test out this feature with a IGT type stuff, or if a userspace wants to utilize this option in any way, this method of differentiation would be useful. DRM_MODE_DRIVER is being used by some other places apart from freesync, so it might not be a unique identifier. So my recommendation would be to keep this.
My comment was, if we have already parsed the whole connector list once, and added the mode, there should be a better way of doing it instead of checking it again by calling "get_highest_freesync_mod"
Some things I can think on top of my mind would be:
- Add a read-only amdgpu driver private flag (not DRM flag), while adding a new freesync mode, which will uniquely identify if a mode is FS mode. On modeset, you have to just check that flag.
- As we are not handling a lot of modes, cache the FS modes locally and check only from that DB (instead of the whole modelist)
- Cache the VIC of the mode (if available) and then look into the VIC table (not sure if detailed modes provide VIC, like CEA-861 modes)
or something better than this.
- Shashank
>>> + high_mode = get_highest_freesync_mode(aconnector, false);
>>> + if (!high_mode)
>>> + return false;
>>> +
>>> + if (high_mode->clock == 0 ||
>>> + high_mode->hdisplay != mode->hdisplay ||
>>> + high_mode->clock != mode->clock ||
>>> + !mode)
>>> + return false;
>>> + else
>>> + return true;
>>> +}
>>> +
>>> static struct dc_stream_state *
>>> create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
>>> const struct drm_display_mode *drm_mode,
>>> @@ -5253,17 +5277,21 @@ create_stream_for_sink(struct
>>> amdgpu_dm_connector *aconnector,
>>> const struct drm_connector_state *con_state =
>>> dm_state ? &dm_state->base : NULL;
>>> struct dc_stream_state *stream = NULL;
>>> - struct drm_display_mode mode = *drm_mode;
>>> + struct drm_display_mode saved_mode, mode = *drm_mode;
>> How about shifting this definition to new line to follow the existing
>> convention ?
> Sure.
>
>>> + struct drm_display_mode *freesync_mode = NULL;
>>> bool native_mode_found = false;
>>> bool scale = dm_state ? (dm_state->scaling != RMX_OFF) : false;
>>> int mode_refresh;
>>> int preferred_refresh = 0;
>>> + bool is_fs_vid_mode = 0;
>>> #if defined(CONFIG_DRM_AMD_DC_DCN)
>>> struct dsc_dec_dpcd_caps dsc_caps;
>>> #endif
>>> uint32_t link_bandwidth_kbps;
>>> -
>>> struct dc_sink *sink = NULL;
>>> +
>>> + memset(&saved_mode, 0, sizeof(struct drm_display_mode));
>>> +
>>> if (aconnector == NULL) {
>>> DRM_ERROR("aconnector is NULL!\n");
>>> return stream;
>>> @@ -5316,20 +5344,33 @@ create_stream_for_sink(struct
>>> amdgpu_dm_connector *aconnector,
>>> */
>>> DRM_DEBUG_DRIVER("No preferred mode found\n");
>>> } else {
>>> + is_fs_vid_mode = is_freesync_video_mode(&mode,
>>> aconnector);
>>> + if (is_fs_vid_mode) {
>>> + freesync_mode =
>>> get_highest_freesync_mode(aconnector, false);
>>> + if (freesync_mode) {
>> As the freesync modes are being added by the driver, and we have
>> passed one check which says is_fs_vid_mode, will it ever be the case
>> where freesync_mode == NULL ? Ideally we should get atleast one mode
>> equal to this isn't it ? in that case we can drop one if () check.
> Yes, thanks for catching this. Will fix.
>
>
> --
>
> Regards,
> Aurabindo Pillai
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/3] drm/amd/display: Skip modeset for front porch change
2020-12-11 5:08 ` Shashank Sharma
@ 2020-12-11 14:49 ` Kazlauskas, Nicholas
2020-12-11 15:35 ` Shashank Sharma
0 siblings, 1 reply; 29+ messages in thread
From: Kazlauskas, Nicholas @ 2020-12-11 14:49 UTC (permalink / raw)
To: Shashank Sharma, Aurabindo Pillai, amd-gfx
Cc: alexander.deucher, stylon.wang, thong.thai, Harry.Wentland, wayne.lin
On 2020-12-11 12:08 a.m., Shashank Sharma wrote:
>
> On 10/12/20 11:20 pm, Aurabindo Pillai wrote:
>> On Thu, 2020-12-10 at 18:29 +0530, Shashank Sharma wrote:
>>> On 10/12/20 8:15 am, Aurabindo Pillai wrote:
>>>> [Why&How]
>>>> Inorder to enable freesync video mode, driver adds extra
>>>> modes based on preferred modes for common freesync frame rates.
>>>> When commiting these mode changes, a full modeset is not needed.
>>>> If the change in only in the front porch timing value, skip full
>>>> modeset and continue using the same stream.
>>>>
>>>> Signed-off-by: Aurabindo Pillai <
>>>> aurabindo.pillai@amd.com
>>>> ---
>>>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 169
>>>> ++++++++++++++++--
>>>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 +
>>>> 2 files changed, 153 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> index f699a3d41cad..c8c72887906a 100644
>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> @@ -217,6 +217,9 @@ static bool amdgpu_dm_psr_disable_all(struct
>>>> amdgpu_display_manager *dm);
>>>> static const struct drm_format_info *
>>>> amd_get_format_info(const struct drm_mode_fb_cmd2 *cmd);
>>>>
>>>> +static bool
>>>> +is_timing_unchanged_for_freesync(struct drm_crtc_state
>>>> *old_crtc_state,
>>>> + struct drm_crtc_state
>>>> *new_crtc_state);
>>>> /*
>>>> * dm_vblank_get_counter
>>>> *
>>>> @@ -5096,8 +5099,11 @@ copy_crtc_timing_for_drm_display_mode(const
>>>> struct drm_display_mode *src_mode,
>>>> static void
>>>> decide_crtc_timing_for_drm_display_mode(struct drm_display_mode
>>>> *drm_mode,
>>>> const struct drm_display_mode
>>>> *native_mode,
>>>> - bool scale_enabled)
>>>> + bool scale_enabled, bool
>>>> fs_mode)
>>>> {
>>>> + if (fs_mode)
>>>> + return;
>>> so we are adding an input flag just so that we can return from the
>>> function at top ? How about adding this check at the caller without
>>> changing the function parameters ?
>> Will fix this.
>>
>>>> +
>>>> if (scale_enabled) {
>>>> copy_crtc_timing_for_drm_display_mode(native_mode,
>>>> drm_mode);
>>>> } else if (native_mode->clock == drm_mode->clock &&
>>>> @@ -5241,6 +5247,24 @@ get_highest_freesync_mode(struct
>>>> amdgpu_dm_connector *aconnector,
>>>> return m_high;
>>>> }
>>>>
>>>> +static bool is_freesync_video_mode(struct drm_display_mode *mode,
>>>> + struct amdgpu_dm_connector
>>>> *aconnector)
>>>> +{
>>>> + struct drm_display_mode *high_mode;
>>>> +
>>> I thought we were adding a string "_FSV" in the end for the mode-
>>>> name, why can't we check that instead of going through the whole
>>> list of modes again ?
>> Actually I only added _FSV to distinguish the newly added modes easily.
>> On second thoughts, I'm not sure if there are any userspace
>> applications that might depend on parsing the mode name, for maybe to
>> print the resolution. I think its better not to break any such
>> assumptions if they do exist by any chance. I think I'll just remove
>> _FSV from the mode name. We already set DRM_MODE_TYPE_DRIVER for
>> userspace to recognize these additional modes, so it shouldnt be a
>> problem.
>
> Actually, I am rather happy with this, as in when we want to test out this feature with a IGT type stuff, or if a userspace wants to utilize this option in any way, this method of differentiation would be useful. DRM_MODE_DRIVER is being used by some other places apart from freesync, so it might not be a unique identifier. So my recommendation would be to keep this.
>
> My comment was, if we have already parsed the whole connector list once, and added the mode, there should be a better way of doing it instead of checking it again by calling "get_highest_freesync_mod"
>
> Some things I can think on top of my mind would be:
>
> - Add a read-only amdgpu driver private flag (not DRM flag), while adding a new freesync mode, which will uniquely identify if a mode is FS mode. On modeset, you have to just check that flag.
>
> - As we are not handling a lot of modes, cache the FS modes locally and check only from that DB (instead of the whole modelist)
>
> - Cache the VIC of the mode (if available) and then look into the VIC table (not sure if detailed modes provide VIC, like CEA-861 modes)
>
> or something better than this.
>
> - Shashank
I'd rather we not make mode name part of a UAPI or to identify a
"FreeSync mode". This is already behind a module option and from the
driver's perspective we only need the timing to understand whether or
not we can do an optimized modeset using FreeSync into it. Driver
private flags can optimize the check away but it's only a few
comparisons so I don't see much benefit.
We will always need to reference the original preferred mode regardless
of how the FreeSync mode is identified since there could be a case where
we're enabling the CRTC from disabled -> enabled. The display was
previously blank and we need to reprogram the OTG timing to the mode
that doesn't have an extended front porch.
Regards,
Nicholas Kazlauskas
>
>>>> + high_mode = get_highest_freesync_mode(aconnector, false);
>>>> + if (!high_mode)
>>>> + return false;
>>>> +
>>>> + if (high_mode->clock == 0 ||
>>>> + high_mode->hdisplay != mode->hdisplay ||
>>>> + high_mode->clock != mode->clock ||
>>>> + !mode)
>>>> + return false;
>>>> + else
>>>> + return true;
>>>> +}
>>>> +
>>>> static struct dc_stream_state *
>>>> create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
>>>> const struct drm_display_mode *drm_mode,
>>>> @@ -5253,17 +5277,21 @@ create_stream_for_sink(struct
>>>> amdgpu_dm_connector *aconnector,
>>>> const struct drm_connector_state *con_state =
>>>> dm_state ? &dm_state->base : NULL;
>>>> struct dc_stream_state *stream = NULL;
>>>> - struct drm_display_mode mode = *drm_mode;
>>>> + struct drm_display_mode saved_mode, mode = *drm_mode;
>>> How about shifting this definition to new line to follow the existing
>>> convention ?
>> Sure.
>>
>>>> + struct drm_display_mode *freesync_mode = NULL;
>>>> bool native_mode_found = false;
>>>> bool scale = dm_state ? (dm_state->scaling != RMX_OFF) : false;
>>>> int mode_refresh;
>>>> int preferred_refresh = 0;
>>>> + bool is_fs_vid_mode = 0;
>>>> #if defined(CONFIG_DRM_AMD_DC_DCN)
>>>> struct dsc_dec_dpcd_caps dsc_caps;
>>>> #endif
>>>> uint32_t link_bandwidth_kbps;
>>>> -
>>>> struct dc_sink *sink = NULL;
>>>> +
>>>> + memset(&saved_mode, 0, sizeof(struct drm_display_mode));
>>>> +
>>>> if (aconnector == NULL) {
>>>> DRM_ERROR("aconnector is NULL!\n");
>>>> return stream;
>>>> @@ -5316,20 +5344,33 @@ create_stream_for_sink(struct
>>>> amdgpu_dm_connector *aconnector,
>>>> */
>>>> DRM_DEBUG_DRIVER("No preferred mode found\n");
>>>> } else {
>>>> + is_fs_vid_mode = is_freesync_video_mode(&mode,
>>>> aconnector);
>>>> + if (is_fs_vid_mode) {
>>>> + freesync_mode =
>>>> get_highest_freesync_mode(aconnector, false);
>>>> + if (freesync_mode) {
>>> As the freesync modes are being added by the driver, and we have
>>> passed one check which says is_fs_vid_mode, will it ever be the case
>>> where freesync_mode == NULL ? Ideally we should get atleast one mode
>>> equal to this isn't it ? in that case we can drop one if () check.
>> Yes, thanks for catching this. Will fix.
>>
>>
>> --
>>
>> Regards,
>> Aurabindo Pillai
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/3] drm/amd/display: Skip modeset for front porch change
2020-12-11 14:49 ` Kazlauskas, Nicholas
@ 2020-12-11 15:35 ` Shashank Sharma
2020-12-11 16:20 ` Kazlauskas, Nicholas
0 siblings, 1 reply; 29+ messages in thread
From: Shashank Sharma @ 2020-12-11 15:35 UTC (permalink / raw)
To: Kazlauskas, Nicholas, Aurabindo Pillai, amd-gfx
Cc: alexander.deucher, stylon.wang, thong.thai, Harry.Wentland, wayne.lin
On 11/12/20 8:19 pm, Kazlauskas, Nicholas wrote:
> On 2020-12-11 12:08 a.m., Shashank Sharma wrote:
>> On 10/12/20 11:20 pm, Aurabindo Pillai wrote:
>>> On Thu, 2020-12-10 at 18:29 +0530, Shashank Sharma wrote:
>>>> On 10/12/20 8:15 am, Aurabindo Pillai wrote:
>>>>> [Why&How]
>>>>> Inorder to enable freesync video mode, driver adds extra
>>>>> modes based on preferred modes for common freesync frame rates.
>>>>> When commiting these mode changes, a full modeset is not needed.
>>>>> If the change in only in the front porch timing value, skip full
>>>>> modeset and continue using the same stream.
>>>>>
>>>>> Signed-off-by: Aurabindo Pillai <
>>>>> aurabindo.pillai@amd.com
>>>>> ---
>>>>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 169
>>>>> ++++++++++++++++--
>>>>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 +
>>>>> 2 files changed, 153 insertions(+), 17 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>> index f699a3d41cad..c8c72887906a 100644
>>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>> @@ -217,6 +217,9 @@ static bool amdgpu_dm_psr_disable_all(struct
>>>>> amdgpu_display_manager *dm);
>>>>> static const struct drm_format_info *
>>>>> amd_get_format_info(const struct drm_mode_fb_cmd2 *cmd);
>>>>>
>>>>> +static bool
>>>>> +is_timing_unchanged_for_freesync(struct drm_crtc_state
>>>>> *old_crtc_state,
>>>>> + struct drm_crtc_state
>>>>> *new_crtc_state);
>>>>> /*
>>>>> * dm_vblank_get_counter
>>>>> *
>>>>> @@ -5096,8 +5099,11 @@ copy_crtc_timing_for_drm_display_mode(const
>>>>> struct drm_display_mode *src_mode,
>>>>> static void
>>>>> decide_crtc_timing_for_drm_display_mode(struct drm_display_mode
>>>>> *drm_mode,
>>>>> const struct drm_display_mode
>>>>> *native_mode,
>>>>> - bool scale_enabled)
>>>>> + bool scale_enabled, bool
>>>>> fs_mode)
>>>>> {
>>>>> + if (fs_mode)
>>>>> + return;
>>>> so we are adding an input flag just so that we can return from the
>>>> function at top ? How about adding this check at the caller without
>>>> changing the function parameters ?
>>> Will fix this.
>>>
>>>>> +
>>>>> if (scale_enabled) {
>>>>> copy_crtc_timing_for_drm_display_mode(native_mode,
>>>>> drm_mode);
>>>>> } else if (native_mode->clock == drm_mode->clock &&
>>>>> @@ -5241,6 +5247,24 @@ get_highest_freesync_mode(struct
>>>>> amdgpu_dm_connector *aconnector,
>>>>> return m_high;
>>>>> }
>>>>>
>>>>> +static bool is_freesync_video_mode(struct drm_display_mode *mode,
>>>>> + struct amdgpu_dm_connector
>>>>> *aconnector)
>>>>> +{
>>>>> + struct drm_display_mode *high_mode;
>>>>> +
>>>> I thought we were adding a string "_FSV" in the end for the mode-
>>>>> name, why can't we check that instead of going through the whole
>>>> list of modes again ?
>>> Actually I only added _FSV to distinguish the newly added modes easily.
>>> On second thoughts, I'm not sure if there are any userspace
>>> applications that might depend on parsing the mode name, for maybe to
>>> print the resolution. I think its better not to break any such
>>> assumptions if they do exist by any chance. I think I'll just remove
>>> _FSV from the mode name. We already set DRM_MODE_TYPE_DRIVER for
>>> userspace to recognize these additional modes, so it shouldnt be a
>>> problem.
>> Actually, I am rather happy with this, as in when we want to test out this feature with a IGT type stuff, or if a userspace wants to utilize this option in any way, this method of differentiation would be useful. DRM_MODE_DRIVER is being used by some other places apart from freesync, so it might not be a unique identifier. So my recommendation would be to keep this.
>>
>> My comment was, if we have already parsed the whole connector list once, and added the mode, there should be a better way of doing it instead of checking it again by calling "get_highest_freesync_mod"
>>
>> Some things I can think on top of my mind would be:
>>
>> - Add a read-only amdgpu driver private flag (not DRM flag), while adding a new freesync mode, which will uniquely identify if a mode is FS mode. On modeset, you have to just check that flag.
>>
>> - As we are not handling a lot of modes, cache the FS modes locally and check only from that DB (instead of the whole modelist)
>>
>> - Cache the VIC of the mode (if available) and then look into the VIC table (not sure if detailed modes provide VIC, like CEA-861 modes)
>>
>> or something better than this.
>>
>> - Shashank
> I'd rather we not make mode name part of a UAPI or to identify a
> "FreeSync mode". This is already behind a module option and from the
> driver's perspective we only need the timing to understand whether or
> not we can do an optimized modeset using FreeSync into it. Driver
> private flags can optimize the check away but it's only a few
> comparisons so I don't see much benefit.
The module parameter is just to control the addition of freesync modes or not, but that doesn't let you differentiate between a genuine EDID mode or Freesync modes added by us, to accommodate freesync solution.
>
> We will always need to reference the original preferred mode regardless
> of how the FreeSync mode is identified since there could be a case where
> we're enabling the CRTC from disabled -> enabled. The display was
> previously blank and we need to reprogram the OTG timing to the mode
> that doesn't have an extended front porch.
I think there is a gap in understanding my comment here. If you see the current implement of function "is_freesync_video_mode", what it does is it explores all the modes from the connector's probed_modes list, and compares them with possible_fs_modes, and decides if this mode is a freesync mode or not. My point is, we have already done this exercise once, while we were adding the freesync modes in the mode list already.
Now if we can add a driver_private flag, or some identification in the mode, we don't have to do this whole thing again.
Its like:
While adding freesync modes:
- add_freesync_modes ()
{
get_preferred_mode()
prepare_freesync_mode_from_preferred_modes()
add_new_freesync_mode_in_connector_modelist() //Add a driver private flag in this new freesync mode
}
Now, as the driver is the only source of the freesync modes, we can make the identifier function during the modeset() can be as small as:
- is_freesync_video_mode (mode)
{
retrun (mode->flags & driver_private_flag);
}
Point being, there is no need to do the same thing again, in order to identify, if a mode is freesync mode or not, as the driver only has added these modes, and it should know which are these freesync modes.
- Shashank
> Regards,
> Nicholas Kazlauskas
>
>>>>> + high_mode = get_highest_freesync_mode(aconnector, false);
>>>>> + if (!high_mode)
>>>>> + return false;
>>>>> +
>>>>> + if (high_mode->clock == 0 ||
>>>>> + high_mode->hdisplay != mode->hdisplay ||
>>>>> + high_mode->clock != mode->clock ||
>>>>> + !mode)
>>>>> + return false;
>>>>> + else
>>>>> + return true;
>>>>> +}
>>>>> +
>>>>> static struct dc_stream_state *
>>>>> create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
>>>>> const struct drm_display_mode *drm_mode,
>>>>> @@ -5253,17 +5277,21 @@ create_stream_for_sink(struct
>>>>> amdgpu_dm_connector *aconnector,
>>>>> const struct drm_connector_state *con_state =
>>>>> dm_state ? &dm_state->base : NULL;
>>>>> struct dc_stream_state *stream = NULL;
>>>>> - struct drm_display_mode mode = *drm_mode;
>>>>> + struct drm_display_mode saved_mode, mode = *drm_mode;
>>>> How about shifting this definition to new line to follow the existing
>>>> convention ?
>>> Sure.
>>>
>>>>> + struct drm_display_mode *freesync_mode = NULL;
>>>>> bool native_mode_found = false;
>>>>> bool scale = dm_state ? (dm_state->scaling != RMX_OFF) : false;
>>>>> int mode_refresh;
>>>>> int preferred_refresh = 0;
>>>>> + bool is_fs_vid_mode = 0;
>>>>> #if defined(CONFIG_DRM_AMD_DC_DCN)
>>>>> struct dsc_dec_dpcd_caps dsc_caps;
>>>>> #endif
>>>>> uint32_t link_bandwidth_kbps;
>>>>> -
>>>>> struct dc_sink *sink = NULL;
>>>>> +
>>>>> + memset(&saved_mode, 0, sizeof(struct drm_display_mode));
>>>>> +
>>>>> if (aconnector == NULL) {
>>>>> DRM_ERROR("aconnector is NULL!\n");
>>>>> return stream;
>>>>> @@ -5316,20 +5344,33 @@ create_stream_for_sink(struct
>>>>> amdgpu_dm_connector *aconnector,
>>>>> */
>>>>> DRM_DEBUG_DRIVER("No preferred mode found\n");
>>>>> } else {
>>>>> + is_fs_vid_mode = is_freesync_video_mode(&mode,
>>>>> aconnector);
>>>>> + if (is_fs_vid_mode) {
>>>>> + freesync_mode =
>>>>> get_highest_freesync_mode(aconnector, false);
>>>>> + if (freesync_mode) {
>>>> As the freesync modes are being added by the driver, and we have
>>>> passed one check which says is_fs_vid_mode, will it ever be the case
>>>> where freesync_mode == NULL ? Ideally we should get atleast one mode
>>>> equal to this isn't it ? in that case we can drop one if () check.
>>> Yes, thanks for catching this. Will fix.
>>>
>>>
>>> --
>>>
>>> Regards,
>>> Aurabindo Pillai
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/3] drm/amd/display: Skip modeset for front porch change
2020-12-11 15:35 ` Shashank Sharma
@ 2020-12-11 16:20 ` Kazlauskas, Nicholas
2020-12-11 18:31 ` Shashank Sharma
0 siblings, 1 reply; 29+ messages in thread
From: Kazlauskas, Nicholas @ 2020-12-11 16:20 UTC (permalink / raw)
To: Shashank Sharma, Aurabindo Pillai, amd-gfx
Cc: alexander.deucher, stylon.wang, thong.thai, Harry.Wentland, wayne.lin
On 2020-12-11 10:35 a.m., Shashank Sharma wrote:
>
> On 11/12/20 8:19 pm, Kazlauskas, Nicholas wrote:
>> On 2020-12-11 12:08 a.m., Shashank Sharma wrote:
>>> On 10/12/20 11:20 pm, Aurabindo Pillai wrote:
>>>> On Thu, 2020-12-10 at 18:29 +0530, Shashank Sharma wrote:
>>>>> On 10/12/20 8:15 am, Aurabindo Pillai wrote:
>>>>>> [Why&How]
>>>>>> Inorder to enable freesync video mode, driver adds extra
>>>>>> modes based on preferred modes for common freesync frame rates.
>>>>>> When commiting these mode changes, a full modeset is not needed.
>>>>>> If the change in only in the front porch timing value, skip full
>>>>>> modeset and continue using the same stream.
>>>>>>
>>>>>> Signed-off-by: Aurabindo Pillai <
>>>>>> aurabindo.pillai@amd.com
>>>>>> ---
>>>>>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 169
>>>>>> ++++++++++++++++--
>>>>>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 +
>>>>>> 2 files changed, 153 insertions(+), 17 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>>> index f699a3d41cad..c8c72887906a 100644
>>>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>>> @@ -217,6 +217,9 @@ static bool amdgpu_dm_psr_disable_all(struct
>>>>>> amdgpu_display_manager *dm);
>>>>>> static const struct drm_format_info *
>>>>>> amd_get_format_info(const struct drm_mode_fb_cmd2 *cmd);
>>>>>>
>>>>>> +static bool
>>>>>> +is_timing_unchanged_for_freesync(struct drm_crtc_state
>>>>>> *old_crtc_state,
>>>>>> + struct drm_crtc_state
>>>>>> *new_crtc_state);
>>>>>> /*
>>>>>> * dm_vblank_get_counter
>>>>>> *
>>>>>> @@ -5096,8 +5099,11 @@ copy_crtc_timing_for_drm_display_mode(const
>>>>>> struct drm_display_mode *src_mode,
>>>>>> static void
>>>>>> decide_crtc_timing_for_drm_display_mode(struct drm_display_mode
>>>>>> *drm_mode,
>>>>>> const struct drm_display_mode
>>>>>> *native_mode,
>>>>>> - bool scale_enabled)
>>>>>> + bool scale_enabled, bool
>>>>>> fs_mode)
>>>>>> {
>>>>>> + if (fs_mode)
>>>>>> + return;
>>>>> so we are adding an input flag just so that we can return from the
>>>>> function at top ? How about adding this check at the caller without
>>>>> changing the function parameters ?
>>>> Will fix this.
>>>>
>>>>>> +
>>>>>> if (scale_enabled) {
>>>>>> copy_crtc_timing_for_drm_display_mode(native_mode,
>>>>>> drm_mode);
>>>>>> } else if (native_mode->clock == drm_mode->clock &&
>>>>>> @@ -5241,6 +5247,24 @@ get_highest_freesync_mode(struct
>>>>>> amdgpu_dm_connector *aconnector,
>>>>>> return m_high;
>>>>>> }
>>>>>>
>>>>>> +static bool is_freesync_video_mode(struct drm_display_mode *mode,
>>>>>> + struct amdgpu_dm_connector
>>>>>> *aconnector)
>>>>>> +{
>>>>>> + struct drm_display_mode *high_mode;
>>>>>> +
>>>>> I thought we were adding a string "_FSV" in the end for the mode-
>>>>>> name, why can't we check that instead of going through the whole
>>>>> list of modes again ?
>>>> Actually I only added _FSV to distinguish the newly added modes easily.
>>>> On second thoughts, I'm not sure if there are any userspace
>>>> applications that might depend on parsing the mode name, for maybe to
>>>> print the resolution. I think its better not to break any such
>>>> assumptions if they do exist by any chance. I think I'll just remove
>>>> _FSV from the mode name. We already set DRM_MODE_TYPE_DRIVER for
>>>> userspace to recognize these additional modes, so it shouldnt be a
>>>> problem.
>>> Actually, I am rather happy with this, as in when we want to test out this feature with a IGT type stuff, or if a userspace wants to utilize this option in any way, this method of differentiation would be useful. DRM_MODE_DRIVER is being used by some other places apart from freesync, so it might not be a unique identifier. So my recommendation would be to keep this.
>>>
>>> My comment was, if we have already parsed the whole connector list once, and added the mode, there should be a better way of doing it instead of checking it again by calling "get_highest_freesync_mod"
>>>
>>> Some things I can think on top of my mind would be:
>>>
>>> - Add a read-only amdgpu driver private flag (not DRM flag), while adding a new freesync mode, which will uniquely identify if a mode is FS mode. On modeset, you have to just check that flag.
>>>
>>> - As we are not handling a lot of modes, cache the FS modes locally and check only from that DB (instead of the whole modelist)
>>>
>>> - Cache the VIC of the mode (if available) and then look into the VIC table (not sure if detailed modes provide VIC, like CEA-861 modes)
>>>
>>> or something better than this.
>>>
>>> - Shashank
>> I'd rather we not make mode name part of a UAPI or to identify a
>> "FreeSync mode". This is already behind a module option and from the
>> driver's perspective we only need the timing to understand whether or
>> not we can do an optimized modeset using FreeSync into it. Driver
>> private flags can optimize the check away but it's only a few
>> comparisons so I don't see much benefit.
> The module parameter is just to control the addition of freesync modes or not, but that doesn't let you differentiate between a genuine EDID mode or Freesync modes added by us, to accommodate freesync solution.
It's for both the modeset optimization and the FreeSync mode generation.
>>
>> We will always need to reference the original preferred mode regardless
>> of how the FreeSync mode is identified since there could be a case where
>> we're enabling the CRTC from disabled -> enabled. The display was
>> previously blank and we need to reprogram the OTG timing to the mode
>> that doesn't have an extended front porch.
>
> I think there is a gap in understanding my comment here. If you see the current implement of function "is_freesync_video_mode", what it does is it explores all the modes from the connector's probed_modes list, and compares them with possible_fs_modes, and decides if this mode is a freesync mode or not. My point is, we have already done this exercise once, while we were adding the freesync modes in the mode list already.
>
> Now if we can add a driver_private flag, or some identification in the mode, we don't have to do this whole thing again.
>
> Its like:
>
> While adding freesync modes:
>
> - add_freesync_modes ()
>
> {
>
> get_preferred_mode()
>
> prepare_freesync_mode_from_preferred_modes()
>
> add_new_freesync_mode_in_connector_modelist() //Add a driver private flag in this new freesync mode
>
> }
>
>
> Now, as the driver is the only source of the freesync modes, we can make the identifier function during the modeset() can be as small as:
>
> - is_freesync_video_mode (mode)
>
> {
>
> retrun (mode->flags & driver_private_flag);
>
> }
>
>
> Point being, there is no need to do the same thing again, in order to identify, if a mode is freesync mode or not, as the driver only has added these modes, and it should know which are these freesync modes.
>
> - Shashank
To clarify, the original point was that knowing whether the FreeSync
mode is a video compatible mode isn't enough - we need to know what the
base mode's vtotal was to correctly program the hardware.
The base mode is basically the FreeSync optimized mode - preferred mode
+ highest refresh rate to support BTR.
So when determining whether we skip the modeset the logic should be:
1. Find the base/freesync optimized mode. We can just iterate the mode
list for this for an unoptimized implementation but I don't think we
really need to go further than this since this only happens on a modeset.
2. Compare the incoming mode against the freesync optimized mode. If the
only thing that differs is the vtotal/front porch duration and it's
within the range supported by the display then we can skip the blank.
Regards,
Nicholas Kazlauskas
>
>> Regards,
>> Nicholas Kazlauskas
>>
>>>>>> + high_mode = get_highest_freesync_mode(aconnector, false);
>>>>>> + if (!high_mode)
>>>>>> + return false;
>>>>>> +
>>>>>> + if (high_mode->clock == 0 ||
>>>>>> + high_mode->hdisplay != mode->hdisplay ||
>>>>>> + high_mode->clock != mode->clock ||
>>>>>> + !mode)
>>>>>> + return false;
>>>>>> + else
>>>>>> + return true;
>>>>>> +}
>>>>>> +
>>>>>> static struct dc_stream_state *
>>>>>> create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
>>>>>> const struct drm_display_mode *drm_mode,
>>>>>> @@ -5253,17 +5277,21 @@ create_stream_for_sink(struct
>>>>>> amdgpu_dm_connector *aconnector,
>>>>>> const struct drm_connector_state *con_state =
>>>>>> dm_state ? &dm_state->base : NULL;
>>>>>> struct dc_stream_state *stream = NULL;
>>>>>> - struct drm_display_mode mode = *drm_mode;
>>>>>> + struct drm_display_mode saved_mode, mode = *drm_mode;
>>>>> How about shifting this definition to new line to follow the existing
>>>>> convention ?
>>>> Sure.
>>>>
>>>>>> + struct drm_display_mode *freesync_mode = NULL;
>>>>>> bool native_mode_found = false;
>>>>>> bool scale = dm_state ? (dm_state->scaling != RMX_OFF) : false;
>>>>>> int mode_refresh;
>>>>>> int preferred_refresh = 0;
>>>>>> + bool is_fs_vid_mode = 0;
>>>>>> #if defined(CONFIG_DRM_AMD_DC_DCN)
>>>>>> struct dsc_dec_dpcd_caps dsc_caps;
>>>>>> #endif
>>>>>> uint32_t link_bandwidth_kbps;
>>>>>> -
>>>>>> struct dc_sink *sink = NULL;
>>>>>> +
>>>>>> + memset(&saved_mode, 0, sizeof(struct drm_display_mode));
>>>>>> +
>>>>>> if (aconnector == NULL) {
>>>>>> DRM_ERROR("aconnector is NULL!\n");
>>>>>> return stream;
>>>>>> @@ -5316,20 +5344,33 @@ create_stream_for_sink(struct
>>>>>> amdgpu_dm_connector *aconnector,
>>>>>> */
>>>>>> DRM_DEBUG_DRIVER("No preferred mode found\n");
>>>>>> } else {
>>>>>> + is_fs_vid_mode = is_freesync_video_mode(&mode,
>>>>>> aconnector);
>>>>>> + if (is_fs_vid_mode) {
>>>>>> + freesync_mode =
>>>>>> get_highest_freesync_mode(aconnector, false);
>>>>>> + if (freesync_mode) {
>>>>> As the freesync modes are being added by the driver, and we have
>>>>> passed one check which says is_fs_vid_mode, will it ever be the case
>>>>> where freesync_mode == NULL ? Ideally we should get atleast one mode
>>>>> equal to this isn't it ? in that case we can drop one if () check.
>>>> Yes, thanks for catching this. Will fix.
>>>>
>>>>
>>>> --
>>>>
>>>> Regards,
>>>> Aurabindo Pillai
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/3] drm/amd/display: Skip modeset for front porch change
2020-12-11 16:20 ` Kazlauskas, Nicholas
@ 2020-12-11 18:31 ` Shashank Sharma
0 siblings, 0 replies; 29+ messages in thread
From: Shashank Sharma @ 2020-12-11 18:31 UTC (permalink / raw)
To: Kazlauskas, Nicholas, Aurabindo Pillai, amd-gfx
Cc: alexander.deucher, stylon.wang, thong.thai, Harry.Wentland, wayne.lin
On 11/12/20 9:50 pm, Kazlauskas, Nicholas wrote:
> On 2020-12-11 10:35 a.m., Shashank Sharma wrote:
>> On 11/12/20 8:19 pm, Kazlauskas, Nicholas wrote:
>>> On 2020-12-11 12:08 a.m., Shashank Sharma wrote:
>>>> On 10/12/20 11:20 pm, Aurabindo Pillai wrote:
>>>>> On Thu, 2020-12-10 at 18:29 +0530, Shashank Sharma wrote:
>>>>>> On 10/12/20 8:15 am, Aurabindo Pillai wrote:
>>>>>>> [Why&How]
>>>>>>> Inorder to enable freesync video mode, driver adds extra
>>>>>>> modes based on preferred modes for common freesync frame rates.
>>>>>>> When commiting these mode changes, a full modeset is not needed.
>>>>>>> If the change in only in the front porch timing value, skip full
>>>>>>> modeset and continue using the same stream.
>>>>>>>
>>>>>>> Signed-off-by: Aurabindo Pillai <
>>>>>>> aurabindo.pillai@amd.com
>>>>>>> ---
>>>>>>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 169
>>>>>>> ++++++++++++++++--
>>>>>>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 +
>>>>>>> 2 files changed, 153 insertions(+), 17 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>>>> index f699a3d41cad..c8c72887906a 100644
>>>>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>>>> @@ -217,6 +217,9 @@ static bool amdgpu_dm_psr_disable_all(struct
>>>>>>> amdgpu_display_manager *dm);
>>>>>>> static const struct drm_format_info *
>>>>>>> amd_get_format_info(const struct drm_mode_fb_cmd2 *cmd);
>>>>>>>
>>>>>>> +static bool
>>>>>>> +is_timing_unchanged_for_freesync(struct drm_crtc_state
>>>>>>> *old_crtc_state,
>>>>>>> + struct drm_crtc_state
>>>>>>> *new_crtc_state);
>>>>>>> /*
>>>>>>> * dm_vblank_get_counter
>>>>>>> *
>>>>>>> @@ -5096,8 +5099,11 @@ copy_crtc_timing_for_drm_display_mode(const
>>>>>>> struct drm_display_mode *src_mode,
>>>>>>> static void
>>>>>>> decide_crtc_timing_for_drm_display_mode(struct drm_display_mode
>>>>>>> *drm_mode,
>>>>>>> const struct drm_display_mode
>>>>>>> *native_mode,
>>>>>>> - bool scale_enabled)
>>>>>>> + bool scale_enabled, bool
>>>>>>> fs_mode)
>>>>>>> {
>>>>>>> + if (fs_mode)
>>>>>>> + return;
>>>>>> so we are adding an input flag just so that we can return from the
>>>>>> function at top ? How about adding this check at the caller without
>>>>>> changing the function parameters ?
>>>>> Will fix this.
>>>>>
>>>>>>> +
>>>>>>> if (scale_enabled) {
>>>>>>> copy_crtc_timing_for_drm_display_mode(native_mode,
>>>>>>> drm_mode);
>>>>>>> } else if (native_mode->clock == drm_mode->clock &&
>>>>>>> @@ -5241,6 +5247,24 @@ get_highest_freesync_mode(struct
>>>>>>> amdgpu_dm_connector *aconnector,
>>>>>>> return m_high;
>>>>>>> }
>>>>>>>
>>>>>>> +static bool is_freesync_video_mode(struct drm_display_mode *mode,
>>>>>>> + struct amdgpu_dm_connector
>>>>>>> *aconnector)
>>>>>>> +{
>>>>>>> + struct drm_display_mode *high_mode;
>>>>>>> +
>>>>>> I thought we were adding a string "_FSV" in the end for the mode-
>>>>>>> name, why can't we check that instead of going through the whole
>>>>>> list of modes again ?
>>>>> Actually I only added _FSV to distinguish the newly added modes easily.
>>>>> On second thoughts, I'm not sure if there are any userspace
>>>>> applications that might depend on parsing the mode name, for maybe to
>>>>> print the resolution. I think its better not to break any such
>>>>> assumptions if they do exist by any chance. I think I'll just remove
>>>>> _FSV from the mode name. We already set DRM_MODE_TYPE_DRIVER for
>>>>> userspace to recognize these additional modes, so it shouldnt be a
>>>>> problem.
>>>> Actually, I am rather happy with this, as in when we want to test out this feature with a IGT type stuff, or if a userspace wants to utilize this option in any way, this method of differentiation would be useful. DRM_MODE_DRIVER is being used by some other places apart from freesync, so it might not be a unique identifier. So my recommendation would be to keep this.
>>>>
>>>> My comment was, if we have already parsed the whole connector list once, and added the mode, there should be a better way of doing it instead of checking it again by calling "get_highest_freesync_mod"
>>>>
>>>> Some things I can think on top of my mind would be:
>>>>
>>>> - Add a read-only amdgpu driver private flag (not DRM flag), while adding a new freesync mode, which will uniquely identify if a mode is FS mode. On modeset, you have to just check that flag.
>>>>
>>>> - As we are not handling a lot of modes, cache the FS modes locally and check only from that DB (instead of the whole modelist)
>>>>
>>>> - Cache the VIC of the mode (if available) and then look into the VIC table (not sure if detailed modes provide VIC, like CEA-861 modes)
>>>>
>>>> or something better than this.
>>>>
>>>> - Shashank
>>> I'd rather we not make mode name part of a UAPI or to identify a
>>> "FreeSync mode". This is already behind a module option and from the
>>> driver's perspective we only need the timing to understand whether or
>>> not we can do an optimized modeset using FreeSync into it. Driver
>>> private flags can optimize the check away but it's only a few
>>> comparisons so I don't see much benefit.
>> The module parameter is just to control the addition of freesync modes or not, but that doesn't let you differentiate between a genuine EDID mode or Freesync modes added by us, to accommodate freesync solution.
> It's for both the modeset optimization and the FreeSync mode generation.
>
>>> We will always need to reference the original preferred mode regardless
>>> of how the FreeSync mode is identified since there could be a case where
>>> we're enabling the CRTC from disabled -> enabled. The display was
>>> previously blank and we need to reprogram the OTG timing to the mode
>>> that doesn't have an extended front porch.
>> I think there is a gap in understanding my comment here. If you see the current implement of function "is_freesync_video_mode", what it does is it explores all the modes from the connector's probed_modes list, and compares them with possible_fs_modes, and decides if this mode is a freesync mode or not. My point is, we have already done this exercise once, while we were adding the freesync modes in the mode list already.
>>
>> Now if we can add a driver_private flag, or some identification in the mode, we don't have to do this whole thing again.
>>
>> Its like:
>>
>> While adding freesync modes:
>>
>> - add_freesync_modes ()
>>
>> {
>>
>> get_preferred_mode()
>>
>> prepare_freesync_mode_from_preferred_modes()
>>
>> add_new_freesync_mode_in_connector_modelist() //Add a driver private flag in this new freesync mode
>>
>> }
>>
>>
>> Now, as the driver is the only source of the freesync modes, we can make the identifier function during the modeset() can be as small as:
>>
>> - is_freesync_video_mode (mode)
>>
>> {
>>
>> retrun (mode->flags & driver_private_flag);
>>
>> }
>>
>>
>> Point being, there is no need to do the same thing again, in order to identify, if a mode is freesync mode or not, as the driver only has added these modes, and it should know which are these freesync modes.
>>
>> - Shashank
> To clarify, the original point was that knowing whether the FreeSync
> mode is a video compatible mode isn't enough - we need to know what the
> base mode's vtotal was to correctly program the hardware.
>
> The base mode is basically the FreeSync optimized mode - preferred mode
> + highest refresh rate to support BTR.
>
> So when determining whether we skip the modeset the logic should be:
>
> 1. Find the base/freesync optimized mode. We can just iterate the mode
> list for this for an unoptimized implementation but I don't think we
> really need to go further than this since this only happens on a modeset.
>
> 2. Compare the incoming mode against the freesync optimized mode. If the
> only thing that differs is the vtotal/front porch duration and it's
> within the range supported by the display then we can skip the blank.
>
> Regards,
> Nicholas Kazlauskas
Ah, thanks for this explanation, my understanding was slightly different. But in this case, it's even easier than I thought. We just have to cache the preferred_mode and highest refresh rate for freesync operation, and we can avoid going through this whole list again. My point is still the same, we have parsed the list once (or twice) while adding the modes, there should not be a need to do the whole thing again while identifying the same.
- Shashank
>>> Regards,
>>> Nicholas Kazlauskas
>>>
>>>>>>> + high_mode = get_highest_freesync_mode(aconnector, false);
>>>>>>> + if (!high_mode)
>>>>>>> + return false;
>>>>>>> +
>>>>>>> + if (high_mode->clock == 0 ||
>>>>>>> + high_mode->hdisplay != mode->hdisplay ||
>>>>>>> + high_mode->clock != mode->clock ||
>>>>>>> + !mode)
>>>>>>> + return false;
>>>>>>> + else
>>>>>>> + return true;
>>>>>>> +}
>>>>>>> +
>>>>>>> static struct dc_stream_state *
>>>>>>> create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
>>>>>>> const struct drm_display_mode *drm_mode,
>>>>>>> @@ -5253,17 +5277,21 @@ create_stream_for_sink(struct
>>>>>>> amdgpu_dm_connector *aconnector,
>>>>>>> const struct drm_connector_state *con_state =
>>>>>>> dm_state ? &dm_state->base : NULL;
>>>>>>> struct dc_stream_state *stream = NULL;
>>>>>>> - struct drm_display_mode mode = *drm_mode;
>>>>>>> + struct drm_display_mode saved_mode, mode = *drm_mode;
>>>>>> How about shifting this definition to new line to follow the existing
>>>>>> convention ?
>>>>> Sure.
>>>>>
>>>>>>> + struct drm_display_mode *freesync_mode = NULL;
>>>>>>> bool native_mode_found = false;
>>>>>>> bool scale = dm_state ? (dm_state->scaling != RMX_OFF) : false;
>>>>>>> int mode_refresh;
>>>>>>> int preferred_refresh = 0;
>>>>>>> + bool is_fs_vid_mode = 0;
>>>>>>> #if defined(CONFIG_DRM_AMD_DC_DCN)
>>>>>>> struct dsc_dec_dpcd_caps dsc_caps;
>>>>>>> #endif
>>>>>>> uint32_t link_bandwidth_kbps;
>>>>>>> -
>>>>>>> struct dc_sink *sink = NULL;
>>>>>>> +
>>>>>>> + memset(&saved_mode, 0, sizeof(struct drm_display_mode));
>>>>>>> +
>>>>>>> if (aconnector == NULL) {
>>>>>>> DRM_ERROR("aconnector is NULL!\n");
>>>>>>> return stream;
>>>>>>> @@ -5316,20 +5344,33 @@ create_stream_for_sink(struct
>>>>>>> amdgpu_dm_connector *aconnector,
>>>>>>> */
>>>>>>> DRM_DEBUG_DRIVER("No preferred mode found\n");
>>>>>>> } else {
>>>>>>> + is_fs_vid_mode = is_freesync_video_mode(&mode,
>>>>>>> aconnector);
>>>>>>> + if (is_fs_vid_mode) {
>>>>>>> + freesync_mode =
>>>>>>> get_highest_freesync_mode(aconnector, false);
>>>>>>> + if (freesync_mode) {
>>>>>> As the freesync modes are being added by the driver, and we have
>>>>>> passed one check which says is_fs_vid_mode, will it ever be the case
>>>>>> where freesync_mode == NULL ? Ideally we should get atleast one mode
>>>>>> equal to this isn't it ? in that case we can drop one if () check.
>>>>> Yes, thanks for catching this. Will fix.
>>>>>
>>>>>
>>>>> --
>>>>>
>>>>> Regards,
>>>>> Aurabindo Pillai
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/3] drm/amd/display: Skip modeset for front porch change
2020-12-10 2:45 ` [PATCH 3/3] drm/amd/display: Skip modeset for front porch change Aurabindo Pillai
2020-12-10 10:21 ` Christian König
2020-12-10 12:59 ` Shashank Sharma
@ 2021-01-04 16:16 ` Kazlauskas, Nicholas
2021-01-04 20:43 ` Aurabindo Pillai
2 siblings, 1 reply; 29+ messages in thread
From: Kazlauskas, Nicholas @ 2021-01-04 16:16 UTC (permalink / raw)
To: Aurabindo Pillai, amd-gfx
Cc: stylon.wang, thong.thai, shashank.sharma, wayne.lin,
alexander.deucher, Harry.Wentland
On 2020-12-09 9:45 p.m., Aurabindo Pillai wrote:
> [Why&How]
> Inorder to enable freesync video mode, driver adds extra
> modes based on preferred modes for common freesync frame rates.
> When commiting these mode changes, a full modeset is not needed.
> If the change in only in the front porch timing value, skip full
> modeset and continue using the same stream.
>
> Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 169 ++++++++++++++++--
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 +
> 2 files changed, 153 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index f699a3d41cad..c8c72887906a 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -217,6 +217,9 @@ static bool amdgpu_dm_psr_disable_all(struct amdgpu_display_manager *dm);
> static const struct drm_format_info *
> amd_get_format_info(const struct drm_mode_fb_cmd2 *cmd);
>
> +static bool
> +is_timing_unchanged_for_freesync(struct drm_crtc_state *old_crtc_state,
> + struct drm_crtc_state *new_crtc_state);
> /*
> * dm_vblank_get_counter
> *
> @@ -5096,8 +5099,11 @@ copy_crtc_timing_for_drm_display_mode(const struct drm_display_mode *src_mode,
> static void
> decide_crtc_timing_for_drm_display_mode(struct drm_display_mode *drm_mode,
> const struct drm_display_mode *native_mode,
> - bool scale_enabled)
> + bool scale_enabled, bool fs_mode)
> {
> + if (fs_mode)
> + return;
> +
> if (scale_enabled) {
> copy_crtc_timing_for_drm_display_mode(native_mode, drm_mode);
> } else if (native_mode->clock == drm_mode->clock &&
> @@ -5241,6 +5247,24 @@ get_highest_freesync_mode(struct amdgpu_dm_connector *aconnector,
> return m_high;
> }
>
> +static bool is_freesync_video_mode(struct drm_display_mode *mode,
> + struct amdgpu_dm_connector *aconnector)
> +{
> + struct drm_display_mode *high_mode;
> +
> + high_mode = get_highest_freesync_mode(aconnector, false);
> + if (!high_mode)
> + return false;
> +
> + if (high_mode->clock == 0 ||
> + high_mode->hdisplay != mode->hdisplay ||
> + high_mode->clock != mode->clock ||
> + !mode)
> + return false;
> + else
> + return true;
> +}
> +
Need to check that the other parameters are the same:
- hsync_start
- hsync_end
- htotal
- hskew
- vdisplay
- vscan
etc.
> static struct dc_stream_state *
> create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
> const struct drm_display_mode *drm_mode,
> @@ -5253,17 +5277,21 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
> const struct drm_connector_state *con_state =
> dm_state ? &dm_state->base : NULL;
> struct dc_stream_state *stream = NULL;
> - struct drm_display_mode mode = *drm_mode;
> + struct drm_display_mode saved_mode, mode = *drm_mode;
> + struct drm_display_mode *freesync_mode = NULL;
> bool native_mode_found = false;
> bool scale = dm_state ? (dm_state->scaling != RMX_OFF) : false;
> int mode_refresh;
> int preferred_refresh = 0;
> + bool is_fs_vid_mode = 0;
> #if defined(CONFIG_DRM_AMD_DC_DCN)
> struct dsc_dec_dpcd_caps dsc_caps;
> #endif
> uint32_t link_bandwidth_kbps;
> -
> struct dc_sink *sink = NULL;
> +
> + memset(&saved_mode, 0, sizeof(struct drm_display_mode));
> +
> if (aconnector == NULL) {
> DRM_ERROR("aconnector is NULL!\n");
> return stream;
> @@ -5316,20 +5344,33 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
> */
> DRM_DEBUG_DRIVER("No preferred mode found\n");
> } else {
> + is_fs_vid_mode = is_freesync_video_mode(&mode, aconnector);
> + if (is_fs_vid_mode) {
> + freesync_mode = get_highest_freesync_mode(aconnector, false);
> + if (freesync_mode) {
> + saved_mode = mode;
> + mode = *freesync_mode;
> + }
> + }
> +
> decide_crtc_timing_for_drm_display_mode(
> &mode, preferred_mode,
> - dm_state ? (dm_state->scaling != RMX_OFF) : false);
> + dm_state ? (dm_state->scaling != RMX_OFF) : false,
> + freesync_mode ? true : false);
I don't think we need an additional flag here - scaling/freesync behave
the same, maybe just rename the variable in the function.
Regards,
Nicholas Kazlauskas
> preferred_refresh = drm_mode_vrefresh(preferred_mode);
> }
>
> if (!dm_state)
> drm_mode_set_crtcinfo(&mode, 0);
>
> - /*
> + if (dm_state && is_fs_vid_mode && freesync_mode)
> + drm_mode_set_crtcinfo(&saved_mode, 0);
> +
> + /*
> * If scaling is enabled and refresh rate didn't change
> * we copy the vic and polarities of the old timings
> */
> - if (!scale || mode_refresh != preferred_refresh)
> + if (!(scale && freesync_mode) || mode_refresh != preferred_refresh)
> fill_stream_properties_from_drm_display_mode(stream,
> &mode, &aconnector->base, con_state, NULL, requested_bpc);
> else
> @@ -7881,13 +7922,29 @@ static void update_stream_irq_parameters(
> if (new_crtc_state->vrr_supported &&
> config.min_refresh_in_uhz &&
> config.max_refresh_in_uhz) {
> + /*
> + * if freesync compatible mode was set, config.state will be set
> + * in atomic check
> + */
> + if (config.state == VRR_STATE_ACTIVE_FIXED &&
> + config.fixed_refresh_in_uhz && config.max_refresh_in_uhz &&
> + config.min_refresh_in_uhz &&
> + (!drm_atomic_crtc_needs_modeset(&new_crtc_state->base) ||
> + new_crtc_state->freesync_video_mode)) {
> + vrr_params.max_refresh_in_uhz = config.max_refresh_in_uhz;
> + vrr_params.min_refresh_in_uhz = config.min_refresh_in_uhz;
> + vrr_params.fixed_refresh_in_uhz = config.fixed_refresh_in_uhz;
> + vrr_params.state = VRR_STATE_ACTIVE_FIXED;
> + goto out;
> + }
> +
> config.state = new_crtc_state->base.vrr_enabled ?
> VRR_STATE_ACTIVE_VARIABLE :
> VRR_STATE_INACTIVE;
> - } else {
> + } else
> config.state = VRR_STATE_UNSUPPORTED;
> - }
>
> +out:
> mod_freesync_build_vrr_params(dm->freesync_module,
> new_stream,
> &config, &vrr_params);
> @@ -8205,7 +8262,9 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
> * as part of commit.
> */
> if (amdgpu_dm_vrr_active(dm_old_crtc_state) !=
> - amdgpu_dm_vrr_active(acrtc_state)) {
> + amdgpu_dm_vrr_active(acrtc_state) ||
> + acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_FIXED ||
> + acrtc_state->freesync_video_mode) {
> spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
> dc_stream_adjust_vmin_vmax(
> dm->dc, acrtc_state->stream,
> @@ -8896,6 +8955,7 @@ static void get_freesync_config_for_crtc(
> to_amdgpu_dm_connector(new_con_state->base.connector);
> struct drm_display_mode *mode = &new_crtc_state->base.mode;
> int vrefresh = drm_mode_vrefresh(mode);
> + bool fs_vid_mode = false;
>
> new_crtc_state->vrr_supported = new_con_state->freesync_capable &&
> vrefresh >= aconnector->min_vfreq &&
> @@ -8903,17 +8963,26 @@ static void get_freesync_config_for_crtc(
>
> if (new_crtc_state->vrr_supported) {
> new_crtc_state->stream->ignore_msa_timing_param = true;
> - config.state = new_crtc_state->base.vrr_enabled ?
> - VRR_STATE_ACTIVE_VARIABLE :
> - VRR_STATE_INACTIVE;
> - config.min_refresh_in_uhz =
> - aconnector->min_vfreq * 1000000;
> - config.max_refresh_in_uhz =
> - aconnector->max_vfreq * 1000000;
> + fs_vid_mode = new_crtc_state->freesync_config.state == VRR_STATE_ACTIVE_FIXED ||
> + new_crtc_state->freesync_video_mode;
> +
> + config.min_refresh_in_uhz = aconnector->min_vfreq * 1000000;
> + config.max_refresh_in_uhz = aconnector->max_vfreq * 1000000;
> config.vsif_supported = true;
> config.btr = true;
> - }
>
> + if (fs_vid_mode) {
> + config.state = VRR_STATE_ACTIVE_FIXED;
> + config.fixed_refresh_in_uhz = new_crtc_state->freesync_config.fixed_refresh_in_uhz;
> + goto out;
> + }
> + else if (new_crtc_state->base.vrr_enabled && !fs_vid_mode)
> + config.state = VRR_STATE_ACTIVE_VARIABLE;
> + else
> + config.state = VRR_STATE_INACTIVE;
> +
> + }
> +out:
> new_crtc_state->freesync_config = config;
> }
>
> @@ -8926,6 +8995,51 @@ static void reset_freesync_config_for_crtc(
> sizeof(new_crtc_state->vrr_infopacket));
> }
>
> +static bool
> +is_timing_unchanged_for_freesync(struct drm_crtc_state *old_crtc_state,
> + struct drm_crtc_state *new_crtc_state)
> +{
> + struct drm_display_mode old_mode, new_mode;
> +
> + if (!old_crtc_state || !new_crtc_state)
> + return false;
> +
> + old_mode = old_crtc_state->mode;
> + new_mode = new_crtc_state->mode;
> +
> + if (old_mode.clock == new_mode.clock &&
> + old_mode.hdisplay == new_mode.hdisplay &&
> + old_mode.vdisplay == new_mode.vdisplay &&
> + old_mode.htotal == new_mode.htotal &&
> + old_mode.vtotal != new_mode.vtotal &&
> + old_mode.hsync_start == new_mode.hsync_start &&
> + old_mode.vsync_start != new_mode.vsync_start &&
> + old_mode.hsync_end == new_mode.hsync_end &&
> + old_mode.vsync_end != new_mode.vsync_end &&
> + old_mode.hskew == new_mode.hskew &&
> + old_mode.vscan == new_mode.vscan &&
> + (old_mode.vsync_end - old_mode.vsync_start) ==
> + (new_mode.vsync_end - new_mode.vsync_start))
> + return true;
> +
> + return false;
> +}
> +
> +static void set_freesync_fixed_config(struct dm_crtc_state *dm_new_crtc_state) {
> + uint64_t num, den, res;
> + struct drm_crtc_state *new_crtc_state = &dm_new_crtc_state->base;
> +
> + dm_new_crtc_state->freesync_config.state = VRR_STATE_ACTIVE_FIXED;
> +
> + num = (unsigned long long)new_crtc_state->mode.clock * 1000 * 1000000;
> + den = (unsigned long long)new_crtc_state->mode.htotal *
> + (unsigned long long)new_crtc_state->mode.vtotal;
> +
> + res = div_u64(num, den);
> + dm_new_crtc_state->freesync_config.fixed_refresh_in_uhz = res;
> + dm_new_crtc_state->freesync_video_mode = true;
> +}
> +
> static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
> struct drm_atomic_state *state,
> struct drm_crtc *crtc,
> @@ -9016,6 +9130,11 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
> * TODO: Refactor this function to allow this check to work
> * in all conditions.
> */
> + if (dm_new_crtc_state->stream &&
> + is_timing_unchanged_for_freesync(new_crtc_state, old_crtc_state) &&
> + amdgpu_exp_freesync_vid_mode)
> + goto skip_modeset;
> +
> if (dm_new_crtc_state->stream &&
> dc_is_stream_unchanged(new_stream, dm_old_crtc_state->stream) &&
> dc_is_stream_scaling_unchanged(new_stream, dm_old_crtc_state->stream)) {
> @@ -9047,6 +9166,22 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
> if (!dm_old_crtc_state->stream)
> goto skip_modeset;
>
> + if (dm_new_crtc_state->stream &&
> + is_timing_unchanged_for_freesync(new_crtc_state, old_crtc_state) &&
> + amdgpu_exp_freesync_vid_mode) {
> + new_crtc_state->mode_changed = false;
> + DRM_DEBUG_DRIVER(
> + "Mode change not required for front porch change, "
> + "setting mode_changed to %d",
> + new_crtc_state->mode_changed);
> +
> + set_freesync_fixed_config(dm_new_crtc_state);
> +
> + goto skip_modeset;
> + } else if (aconnector &&
> + is_freesync_video_mode(&new_crtc_state->mode, aconnector))
> + set_freesync_fixed_config(dm_new_crtc_state);
> +
> ret = dm_atomic_get_state(state, &dm_state);
> if (ret)
> goto fail;
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index 251af783f6b1..28f2d8c9b260 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -453,6 +453,7 @@ struct dm_crtc_state {
>
> bool freesync_timing_changed;
> bool freesync_vrr_info_changed;
> + bool freesync_video_mode;
>
> bool dsc_force_changed;
> bool vrr_supported;
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/3] drm/amd/display: Skip modeset for front porch change
2021-01-04 16:16 ` Kazlauskas, Nicholas
@ 2021-01-04 20:43 ` Aurabindo Pillai
0 siblings, 0 replies; 29+ messages in thread
From: Aurabindo Pillai @ 2021-01-04 20:43 UTC (permalink / raw)
To: Kazlauskas, Nicholas, amd-gfx
Cc: stylon.wang, thong.thai, shashank.sharma, wayne.lin,
alexander.deucher, Harry.Wentland
On Mon, 2021-01-04 at 11:16 -0500, Kazlauskas, Nicholas wrote:
> On 2020-12-09 9:45 p.m., Aurabindo Pillai wrote:
> > [Why&How]
> > Inorder to enable freesync video mode, driver adds extra
> > modes based on preferred modes for common freesync frame rates.
> > When commiting these mode changes, a full modeset is not needed.
> > If the change in only in the front porch timing value, skip full
> > modeset and continue using the same stream.
> >
> > Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
> > ---
> > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 169
> > ++++++++++++++++--
> > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 +
> > 2 files changed, 153 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > index f699a3d41cad..c8c72887906a 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -217,6 +217,9 @@ static bool amdgpu_dm_psr_disable_all(struct
> > amdgpu_display_manager *dm);
> > static const struct drm_format_info *
> > amd_get_format_info(const struct drm_mode_fb_cmd2 *cmd);
> >
> > +static bool
> > +is_timing_unchanged_for_freesync(struct drm_crtc_state
> > *old_crtc_state,
> > + struct drm_crtc_state
> > *new_crtc_state);
> > /*
> > * dm_vblank_get_counter
> > *
> > @@ -5096,8 +5099,11 @@ copy_crtc_timing_for_drm_display_mode(const
> > struct drm_display_mode *src_mode,
> > static void
> > decide_crtc_timing_for_drm_display_mode(struct drm_display_mode
> > *drm_mode,
> > const struct
> > drm_display_mode *native_mode,
> > - bool scale_enabled)
> > + bool scale_enabled, bool
> > fs_mode)
> > {
> > + if (fs_mode)
> > + return;
> > +
> > if (scale_enabled) {
> > copy_crtc_timing_for_drm_display_mode(native_mode,
> > drm_mode);
> > } else if (native_mode->clock == drm_mode->clock &&
> > @@ -5241,6 +5247,24 @@ get_highest_freesync_mode(struct
> > amdgpu_dm_connector *aconnector,
> > return m_high;
> > }
> >
> > +static bool is_freesync_video_mode(struct drm_display_mode *mode,
> > + struct amdgpu_dm_connector
> > *aconnector)
> > +{
> > + struct drm_display_mode *high_mode;
> > +
> > + high_mode = get_highest_freesync_mode(aconnector, false);
> > + if (!high_mode)
> > + return false;
> > +
> > + if (high_mode->clock == 0 ||
> > + high_mode->hdisplay != mode->hdisplay ||
> > + high_mode->clock != mode->clock ||
> > + !mode)
> > + return false;
> > + else
> > + return true;
> > +}
> > +
>
> Need to check that the other parameters are the same:
> - hsync_start
> - hsync_end
> - htotal
> - hskew
> - vdisplay
> - vscan
>
> etc.
Will do.
>
> > static struct dc_stream_state *
> > create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
> > const struct drm_display_mode *drm_mode,
> > @@ -5253,17 +5277,21 @@ create_stream_for_sink(struct
> > amdgpu_dm_connector *aconnector,
> > const struct drm_connector_state *con_state =
> > dm_state ? &dm_state->base : NULL;
> > struct dc_stream_state *stream = NULL;
> > - struct drm_display_mode mode = *drm_mode;
> > + struct drm_display_mode saved_mode, mode = *drm_mode;
> > + struct drm_display_mode *freesync_mode = NULL;
> > bool native_mode_found = false;
> > bool scale = dm_state ? (dm_state->scaling != RMX_OFF) :
> > false;
> > int mode_refresh;
> > int preferred_refresh = 0;
> > + bool is_fs_vid_mode = 0;
> > #if defined(CONFIG_DRM_AMD_DC_DCN)
> > struct dsc_dec_dpcd_caps dsc_caps;
> > #endif
> > uint32_t link_bandwidth_kbps;
> > -
> > struct dc_sink *sink = NULL;
> > +
> > + memset(&saved_mode, 0, sizeof(struct drm_display_mode));
> > +
> > if (aconnector == NULL) {
> > DRM_ERROR("aconnector is NULL!\n");
> > return stream;
> > @@ -5316,20 +5344,33 @@ create_stream_for_sink(struct
> > amdgpu_dm_connector *aconnector,
> > */
> > DRM_DEBUG_DRIVER("No preferred mode found\n");
> > } else {
> > + is_fs_vid_mode = is_freesync_video_mode(&mode,
> > aconnector);
> > + if (is_fs_vid_mode) {
> > + freesync_mode =
> > get_highest_freesync_mode(aconnector, false);
> > + if (freesync_mode) {
> > + saved_mode = mode;
> > + mode = *freesync_mode;
> > + }
> > + }
> > +
> > decide_crtc_timing_for_drm_display_mode(
> > &mode, preferred_mode,
> > - dm_state ? (dm_state->scaling !=
> > RMX_OFF) : false);
> > + dm_state ? (dm_state->scaling !=
> > RMX_OFF) : false,
> > + freesync_mode ? true : false);
>
> I don't think we need an additional flag here - scaling/freesync
> behave
> the same, maybe just rename the variable in the function.
This was fixed in v3. Will send out a v4 with above change and the
suggestion from Alex to change the module parameter name.
>
> Regards,
> Nicholas Kazlauskas
>
> > preferred_refresh =
> > drm_mode_vrefresh(preferred_mode);
> > }
> >
> > if (!dm_state)
> > drm_mode_set_crtcinfo(&mode, 0);
> >
> > - /*
> > + if (dm_state && is_fs_vid_mode && freesync_mode)
> > + drm_mode_set_crtcinfo(&saved_mode, 0);
> > +
> > + /*
> > * If scaling is enabled and refresh rate didn't change
> > * we copy the vic and polarities of the old timings
> > */
> > - if (!scale || mode_refresh != preferred_refresh)
> > + if (!(scale && freesync_mode) || mode_refresh !=
> > preferred_refresh)
> > fill_stream_properties_from_drm_display_mode(stream
> > ,
> > &mode, &aconnector->base, con_state, NULL,
> > requested_bpc);
> > else
> > @@ -7881,13 +7922,29 @@ static void update_stream_irq_parameters(
> > if (new_crtc_state->vrr_supported &&
> > config.min_refresh_in_uhz &&
> > config.max_refresh_in_uhz) {
> > + /*
> > + * if freesync compatible mode was set,
> > config.state will be set
> > + * in atomic check
> > + */
> > + if (config.state == VRR_STATE_ACTIVE_FIXED &&
> > + config.fixed_refresh_in_uhz &&
> > config.max_refresh_in_uhz &&
> > + config.min_refresh_in_uhz &&
> > +
> > (!drm_atomic_crtc_needs_modeset(&new_crtc_state->base) ||
> > + new_crtc_state->freesync_video_mode)) {
> > + vrr_params.max_refresh_in_uhz =
> > config.max_refresh_in_uhz;
> > + vrr_params.min_refresh_in_uhz =
> > config.min_refresh_in_uhz;
> > + vrr_params.fixed_refresh_in_uhz =
> > config.fixed_refresh_in_uhz;
> > + vrr_params.state = VRR_STATE_ACTIVE_FIXED;
> > + goto out;
> > + }
> > +
> > config.state = new_crtc_state->base.vrr_enabled ?
> > VRR_STATE_ACTIVE_VARIABLE :
> > VRR_STATE_INACTIVE;
> > - } else {
> > + } else
> > config.state = VRR_STATE_UNSUPPORTED;
> > - }
> >
> > +out:
> > mod_freesync_build_vrr_params(dm->freesync_module,
> > new_stream,
> > &config, &vrr_params);
> > @@ -8205,7 +8262,9 @@ static void amdgpu_dm_commit_planes(struct
> > drm_atomic_state *state,
> > * as part of commit.
> > */
> > if (amdgpu_dm_vrr_active(dm_old_crtc_state) !=
> > - amdgpu_dm_vrr_active(acrtc_state)) {
> > + amdgpu_dm_vrr_active(acrtc_state) ||
> > + acrtc_state->freesync_config.state ==
> > VRR_STATE_ACTIVE_FIXED ||
> > + acrtc_state->freesync_video_mode) {
> > spin_lock_irqsave(&pcrtc->dev->event_lock,
> > flags);
> > dc_stream_adjust_vmin_vmax(
> > dm->dc, acrtc_state->stream,
> > @@ -8896,6 +8955,7 @@ static void get_freesync_config_for_crtc(
> > to_amdgpu_dm_connector(new_con_state-
> > >base.connector);
> > struct drm_display_mode *mode = &new_crtc_state->base.mode;
> > int vrefresh = drm_mode_vrefresh(mode);
> > + bool fs_vid_mode = false;
> >
> > new_crtc_state->vrr_supported = new_con_state-
> > >freesync_capable &&
> > vrefresh >= aconnector-
> > >min_vfreq &&
> > @@ -8903,17 +8963,26 @@ static void get_freesync_config_for_crtc(
> >
> > if (new_crtc_state->vrr_supported) {
> > new_crtc_state->stream->ignore_msa_timing_param =
> > true;
> > - config.state = new_crtc_state->base.vrr_enabled ?
> > - VRR_STATE_ACTIVE_VARIABLE :
> > - VRR_STATE_INACTIVE;
> > - config.min_refresh_in_uhz =
> > - aconnector->min_vfreq * 1000000;
> > - config.max_refresh_in_uhz =
> > - aconnector->max_vfreq * 1000000;
> > + fs_vid_mode = new_crtc_state->freesync_config.state
> > == VRR_STATE_ACTIVE_FIXED ||
> > + new_crtc_state->freesync_video_mode;
> > +
> > + config.min_refresh_in_uhz = aconnector->min_vfreq *
> > 1000000;
> > + config.max_refresh_in_uhz = aconnector->max_vfreq *
> > 1000000;
> > config.vsif_supported = true;
> > config.btr = true;
> > - }
> >
> > + if (fs_vid_mode) {
> > + config.state = VRR_STATE_ACTIVE_FIXED;
> > + config.fixed_refresh_in_uhz =
> > new_crtc_state->freesync_config.fixed_refresh_in_uhz;
> > + goto out;
> > + }
> > + else if (new_crtc_state->base.vrr_enabled &&
> > !fs_vid_mode)
> > + config.state = VRR_STATE_ACTIVE_VARIABLE;
> > + else
> > + config.state = VRR_STATE_INACTIVE;
> > +
> > + }
> > +out:
> > new_crtc_state->freesync_config = config;
> > }
> >
> > @@ -8926,6 +8995,51 @@ static void reset_freesync_config_for_crtc(
> > sizeof(new_crtc_state->vrr_infopacket));
> > }
> >
> > +static bool
> > +is_timing_unchanged_for_freesync(struct drm_crtc_state
> > *old_crtc_state,
> > + struct drm_crtc_state
> > *new_crtc_state)
> > +{
> > + struct drm_display_mode old_mode, new_mode;
> > +
> > + if (!old_crtc_state || !new_crtc_state)
> > + return false;
> > +
> > + old_mode = old_crtc_state->mode;
> > + new_mode = new_crtc_state->mode;
> > +
> > + if (old_mode.clock == new_mode.clock &&
> > + old_mode.hdisplay == new_mode.hdisplay &&
> > + old_mode.vdisplay == new_mode.vdisplay &&
> > + old_mode.htotal == new_mode.htotal &&
> > + old_mode.vtotal != new_mode.vtotal &&
> > + old_mode.hsync_start == new_mode.hsync_start &&
> > + old_mode.vsync_start != new_mode.vsync_start &&
> > + old_mode.hsync_end == new_mode.hsync_end &&
> > + old_mode.vsync_end != new_mode.vsync_end &&
> > + old_mode.hskew == new_mode.hskew &&
> > + old_mode.vscan == new_mode.vscan &&
> > + (old_mode.vsync_end - old_mode.vsync_start) ==
> > + (new_mode.vsync_end - new_mode.vsync_start))
> > + return true;
> > +
> > + return false;
> > +}
> > +
> > +static void set_freesync_fixed_config(struct dm_crtc_state
> > *dm_new_crtc_state) {
> > + uint64_t num, den, res;
> > + struct drm_crtc_state *new_crtc_state = &dm_new_crtc_state-
> > >base;
> > +
> > + dm_new_crtc_state->freesync_config.state =
> > VRR_STATE_ACTIVE_FIXED;
> > +
> > + num = (unsigned long long)new_crtc_state->mode.clock * 1000
> > * 1000000;
> > + den = (unsigned long long)new_crtc_state->mode.htotal *
> > + (unsigned long long)new_crtc_state->mode.vtotal;
> > +
> > + res = div_u64(num, den);
> > + dm_new_crtc_state->freesync_config.fixed_refresh_in_uhz =
> > res;
> > + dm_new_crtc_state->freesync_video_mode = true;
> > +}
> > +
> > static int dm_update_crtc_state(struct amdgpu_display_manager
> > *dm,
> > struct drm_atomic_state *state,
> > struct drm_crtc *crtc,
> > @@ -9016,6 +9130,11 @@ static int dm_update_crtc_state(struct
> > amdgpu_display_manager *dm,
> > * TODO: Refactor this function to allow this check
> > to work
> > * in all conditions.
> > */
> > + if (dm_new_crtc_state->stream &&
> > +
> > is_timing_unchanged_for_freesync(new_crtc_state, old_crtc_state) &&
> > + amdgpu_exp_freesync_vid_mode)
> > + goto skip_modeset;
> > +
> > if (dm_new_crtc_state->stream &&
> > dc_is_stream_unchanged(new_stream,
> > dm_old_crtc_state->stream) &&
> > dc_is_stream_scaling_unchanged(new_stream,
> > dm_old_crtc_state->stream)) {
> > @@ -9047,6 +9166,22 @@ static int dm_update_crtc_state(struct
> > amdgpu_display_manager *dm,
> > if (!dm_old_crtc_state->stream)
> > goto skip_modeset;
> >
> > + if (dm_new_crtc_state->stream &&
> > +
> > is_timing_unchanged_for_freesync(new_crtc_state, old_crtc_state) &&
> > + amdgpu_exp_freesync_vid_mode) {
> > + new_crtc_state->mode_changed = false;
> > + DRM_DEBUG_DRIVER(
> > + "Mode change not required for front
> > porch change, "
> > + "setting mode_changed to %d",
> > + new_crtc_state->mode_changed);
> > +
> > + set_freesync_fixed_config(dm_new_crtc_state
> > );
> > +
> > + goto skip_modeset;
> > + } else if (aconnector &&
> > + is_freesync_video_mode(&new_crtc_state-
> > >mode, aconnector))
> > + set_freesync_fixed_config(dm_new_crtc_state
> > );
> > +
> > ret = dm_atomic_get_state(state, &dm_state);
> > if (ret)
> > goto fail;
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> > index 251af783f6b1..28f2d8c9b260 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> > @@ -453,6 +453,7 @@ struct dm_crtc_state {
> >
> > bool freesync_timing_changed;
> > bool freesync_vrr_info_changed;
> > + bool freesync_video_mode;
> >
> > bool dsc_force_changed;
> > bool vrr_supported;
> >
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread