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

On 15/12/20 3:50 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 Acked-by: Christian König <christian.koenig at amd.com>

---

.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 210 +++++++++++++++--- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 + 2 files changed, 179 insertions(+), 32 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 c1ffd33e9d83..4ba6be59dcd5 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -213,6 +213,9 @@ static bool amdgpu_dm_psr_disable_all(struct amdgpu_display_manager *dm); static const struct drm_format_info * amd_get_format_info(const struct drm_mode_fb_cmd2 *cmd);

+static bool +is_timing_unchanged_for_freesync(struct drm_crtc_state *old_crtc_state,

• 		 struct drm_crtc_state *new_crtc_state);
  

Do we need this forward declaration for a static function ?

/*

• dm_vblank_get_counter

@@ -4940,7 +4943,8 @@ static void fill_stream_properties_from_drm_display_mode( const struct drm_connector *connector, const struct drm_connector_state *connector_state, const struct dc_stream_state *old_stream,

• int requested_bpc)

• int requested_bpc,
• bool is_in_modeset)

{ struct dc_crtc_timing *timing_out = &stream->timing; const struct drm_display_info *info = &connector->display_info; @@ -4995,19 +4999,28 @@ static void fill_stream_properties_from_drm_display_mode( timing_out->hdmi_vic = hv_frame.vic; }

• timing_out->h_addressable = mode_in->crtc_hdisplay;
• timing_out->h_total = mode_in->crtc_htotal;
• timing_out->h_sync_width =

• mode_in->crtc_hsync_end - mode_in->crtc_hsync_start;
  

• timing_out->h_front_porch =

• mode_in->crtc_hsync_start - mode_in->crtc_hdisplay;
  

• timing_out->v_total = mode_in->crtc_vtotal;
• timing_out->v_addressable = mode_in->crtc_vdisplay;
• timing_out->v_front_porch =

• mode_in->crtc_vsync_start - mode_in->crtc_vdisplay;
  

• timing_out->v_sync_width =

• mode_in->crtc_vsync_end - mode_in->crtc_vsync_start;
  

• timing_out->pix_clk_100hz = mode_in->crtc_clock * 10;

• if (is_in_modeset) {

• timing_out->h_addressable = mode_in->hdisplay;
  

• timing_out->h_total = mode_in->htotal;
  

• timing_out->h_sync_width = mode_in->hsync_end - mode_in->hsync_start;
  

• timing_out->h_front_porch = mode_in->hsync_start - mode_in->hdisplay;
  

• timing_out->v_total = mode_in->vtotal;
  

• timing_out->v_addressable = mode_in->vdisplay;
  

• timing_out->v_front_porch = mode_in->vsync_start - mode_in->vdisplay;
  

• timing_out->v_sync_width = mode_in->vsync_end - mode_in->vsync_start;
  

• timing_out->pix_clk_100hz = mode_in->clock * 10;
  

• } else {

• timing_out->h_addressable = mode_in->crtc_hdisplay;
  

• timing_out->h_total = mode_in->crtc_htotal;
  

• timing_out->h_sync_width = mode_in->crtc_hsync_end - mode_in->crtc_hsync_start;
  

• timing_out->h_front_porch = mode_in->crtc_hsync_start - mode_in->crtc_hdisplay;
  

• timing_out->v_total = mode_in->crtc_vtotal;
  

• timing_out->v_addressable = mode_in->crtc_vdisplay;
  

• timing_out->v_front_porch = mode_in->crtc_vsync_start - mode_in->crtc_vdisplay;
  

• timing_out->v_sync_width = mode_in->crtc_vsync_end - mode_in->crtc_vsync_start;
  

• timing_out->pix_clk_100hz = mode_in->crtc_clock * 10;
  

• }

• timing_out->aspect_ratio = get_aspect_ratio(mode_in);

  stream->output_color_space = get_output_color_space(timing_out);

@@ -5227,6 +5240,24 @@ get_highest_refresh_rate_mode(struct amdgpu_dm_connector *aconnector, return m_pref; }

+static bool is_freesync_video_mode(struct drm_display_mode *mode,

• 		   struct amdgpu_dm_connector *aconnector)
  

+{

• struct drm_display_mode *high_mode;
• high_mode = get_highest_refresh_rate_mode(aconnector, false);
• if (!high_mode || !mode)

• return false;
  

• if (high_mode->clock == 0 ||

•    high_mode->hdisplay != mode->hdisplay ||
  

•    high_mode->vdisplay != mode->vdisplay ||
  

•    high_mode->clock != mode->clock)
  

• return false;
  

• else

• return true;
  

+}

static struct dc_stream_state * create_stream_for_sink(struct amdgpu_dm_connector *aconnector, const struct drm_display_mode *drm_mode, @@ -5240,15 +5271,21 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector, dm_state ? &dm_state->base : NULL; struct dc_stream_state *stream = NULL; struct drm_display_mode mode = *drm_mode;

• struct drm_display_mode saved_mode;
• struct drm_display_mode *freesync_mode = NULL; bool native_mode_found = false; bool scale = dm_state ? (dm_state->scaling != RMX_OFF) : false; int mode_refresh; int preferred_refresh = 0;
• bool is_fs_vid_mode = 0;

#if defined(CONFIG_DRM_AMD_DC_DCN) struct dsc_dec_dpcd_caps dsc_caps; uint32_t link_bandwidth_kbps; #endif struct dc_sink *sink = NULL;

• memset(&saved_mode, 0, sizeof(struct drm_display_mode));
• if (aconnector == NULL) { DRM_ERROR("aconnector is NULL!\n"); return stream;

@@ -5301,25 +5338,39 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector, */ DRM_DEBUG_DRIVER("No preferred mode found\n"); } else {

• decide_crtc_timing_for_drm_display_mode(
  

• is_fs_vid_mode = is_freesync_video_mode(&mode, aconnector);
  

• if (is_fs_vid_mode) {
  

• 	freesync_mode = get_highest_refresh_rate_mode(aconnector, false);
  

• 	saved_mode = mode;
  

• 	mode = *freesync_mode;
  

• }
  

• if (!is_fs_vid_mode)
  

This should go as else {} case for above if() instead of a new check

• 	decide_crtc_timing_for_drm_display_mode(
  	&mode, preferred_mode,
  	dm_state ? (dm_state->scaling != RMX_OFF) : false);
  

• preferred_refresh = drm_mode_vrefresh(preferred_mode); }

  if (!dm_state) drm_mode_set_crtcinfo(&mode, 0);

• /*

• if (dm_state && is_fs_vid_mode)

• drm_mode_set_crtcinfo(&saved_mode, 0);
  

Why is this step required ?

•   /*
  

  • If scaling is enabled and refresh rate didn't change
  • we copy the vic and polarities of the old timings
  */

• if (!scale || mode_refresh != preferred_refresh)

• fill_stream_properties_from_drm_display_mode(stream,
  

• 	&mode, &aconnector->base, con_state, NULL, requested_bpc);
  

• if (!(scale && is_fs_vid_mode) || mode_refresh != preferred_refresh)

• fill_stream_properties_from_drm_display_mode(
  

• 	stream, &mode, &aconnector->base, con_state, NULL,
  

• 	requested_bpc, dm_state ? 1 : 0);
  

  else

• fill_stream_properties_from_drm_display_mode(stream,
  

• 	&mode, &aconnector->base, con_state, old_stream, requested_bpc);
  

• fill_stream_properties_from_drm_display_mode(
  

• 	stream, &mode, &aconnector->base, con_state, old_stream,
  

• 	requested_bpc, dm_state ? 1 : 0);
  
  

  stream->timing.flags.DSC = 0;

@@ -7844,13 +7895,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

conventionally, if a if() condition has {}, its else condition should also have {}, even if it's a single statement. So you can skip this change.

config.state = VRR_STATE_UNSUPPORTED;

• }

+out: mod_freesync_build_vrr_params(dm->freesync_module, new_stream, &config, &vrr_params); @@ -8168,7 +8235,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,
  

@@ -8864,6 +8933,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 &&

@@ -8871,17 +8941,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; }

@@ -8894,6 +8973,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, @@ -8984,6 +9108,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)
  

keeping this flag check before the function call might allow the compiler to save the function call, when this flag is disabled.

• 	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)) {

@@ -9015,6 +9144,23 @@ 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) {
  

same here

• 	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) &&
  

• 	   amdgpu_exp_freesync_vid_mode)
  

• 	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 f5a5f1ad7dba..fd5fb9dafded 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h @@ -455,6 +455,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;

With the minor comments fixed/explained above,

this patch is Reviewed-by: Shashank Sharma shashank.sharma@amd.com

- Shashank