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

[Shashank Sharma <shashank.sharma@amd.com>]

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