On Mon, 4 Jan 2021 16:07:58 -0500 Aurabindo Pillai aurabindo.pillai@amd.com wrote:

[Why&How] Adds a module parameter to enable experimental freesync video mode modeset optimization. Enabling this mode allows the driver to skip a full modeset when freesync compatible modes are requested by the userspace. This paramters also adds some standard modes based on the connected monitor's VRR range.

Signed-off-by: Aurabindo Pillai aurabindo.pillai@amd.com Acked-by: Christian König <christian.koenig at amd.com> Reviewed-by: Shashank Sharma shashank.sharma@amd.com

---

drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 + drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 12 ++++++++++++ 2 files changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h index 100a431f0792..12b13a90eddf 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h @@ -177,6 +177,7 @@ extern int amdgpu_gpu_recovery; extern int amdgpu_emu_mode; extern uint amdgpu_smu_memory_pool_size; extern uint amdgpu_dc_feature_mask; +extern uint amdgpu_exp_freesync_vid_mode; extern uint amdgpu_dc_debug_mask; extern uint amdgpu_dm_abm_level; extern struct amdgpu_mgpu_info mgpu_info; diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index b48d7a3c2a11..2badbc8b2294 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -158,6 +158,7 @@ int amdgpu_mes; int amdgpu_noretry = -1; int amdgpu_force_asic_type = -1; int amdgpu_tmz; +uint amdgpu_exp_freesync_vid_mode; int amdgpu_reset_method = -1; /* auto */ int amdgpu_num_kcq = -1;

@@ -786,6 +787,17 @@ module_param_named(abmlevel, amdgpu_dm_abm_level, uint, 0444); MODULE_PARM_DESC(tmz, "Enable TMZ feature (-1 = auto, 0 = off (default), 1 = on)"); module_param_named(tmz, amdgpu_tmz, int, 0444);

+/**

• • DOC: experimental_freesync_video (uint)
• • Enabled the optimization to adjust front porch timing to achieve seamless mode change experience
• • when setting a freesync supported mode for which full modeset is not needed.
• • The default value: 0 (off).
• */

+MODULE_PARM_DESC(

• freesync_video,
• "Enable freesync modesetting optimization feature (0 = off (default), 1 = on)");

+module_param_named(freesync_video, amdgpu_exp_freesync_vid_mode, uint, 0444);

/**

• DOC: reset_method (int)
• GPU reset method (-1 = auto (default), 0 = legacy, 1 = mode0, 2 = mode1, 3 = mode2, 4 = baco)

Hi,

please document somewhere that ends up in git history (commit message, code comments, description of the parameter would be the best but maybe there isn't enough space?) what Christian König explained in

https://lists.freedesktop.org/archives/dri-devel/2020-December/291254.html

that this is a stop-gap feature intended to be removed as soon as possible (when a better solution comes up, which could be years).

So far I have not seen a single mention of this intention in your patch submissions, and I think it is very important to make known.

I also did not see an explanation of why this instead of manufacturing these video modes in userspace (an idea mentioned by Christian in the referenced email). I think that too should be part of a commit message.

Thanks, pq

On 2021-01-04 4:08 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 Acked-by: Christian König christian.koenig@amd.com

---

.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 219 +++++++++++++++--- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 + 2 files changed, 188 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 aaef2fb528fd..315756207f0f 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -213,6 +213,9 @@ static bool amdgpu_dm_psr_disable_all(struct amdgpu_display_manager *dm); static const struct drm_format_info * amd_get_format_info(const struct drm_mode_fb_cmd2 *cmd);

+static bool +is_timing_unchanged_for_freesync(struct drm_crtc_state *old_crtc_state,

• 		 struct drm_crtc_state *new_crtc_state);
  

  /*
  • dm_vblank_get_counter

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

• int requested_bpc)

• int requested_bpc,
• bool is_in_modeset) { struct dc_crtc_timing *timing_out = &stream->timing; const struct drm_display_info *info = &connector->display_info;

@@ -4995,19 +4999,28 @@ static void fill_stream_properties_from_drm_display_mode( timing_out->hdmi_vic = hv_frame.vic; }

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

• mode_in->crtc_hsync_end - mode_in->crtc_hsync_start;
  

• timing_out->h_front_porch =

• mode_in->crtc_hsync_start - mode_in->crtc_hdisplay;
  

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

• mode_in->crtc_vsync_start - mode_in->crtc_vdisplay;
  

• timing_out->v_sync_width =

• mode_in->crtc_vsync_end - mode_in->crtc_vsync_start;
  

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

• if (is_in_modeset) {

• timing_out->h_addressable = mode_in->hdisplay;
  

• timing_out->h_total = mode_in->htotal;
  

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

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

• timing_out->v_total = mode_in->vtotal;
  

• timing_out->v_addressable = mode_in->vdisplay;
  

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

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

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

• } else {

• timing_out->h_addressable = mode_in->crtc_hdisplay;
  

• timing_out->h_total = mode_in->crtc_htotal;
  

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

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

• timing_out->v_total = mode_in->crtc_vtotal;
  

• timing_out->v_addressable = mode_in->crtc_vdisplay;
  

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

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

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

• }

• timing_out->aspect_ratio = get_aspect_ratio(mode_in);

  stream->output_color_space = get_output_color_space(timing_out);

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

+static bool is_freesync_video_mode(struct drm_display_mode *mode,

• 		   struct amdgpu_dm_connector *aconnector)
  

+{

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

• return false;
  

• timing_diff = high_mode->vtotal - mode->vtotal;
• if (high_mode->clock == 0 || high_mode->clock != mode->clock ||

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

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

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

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

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

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

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

•    high_mode->vsync_start - mode->vsync_start != timing_diff ||
  

•    high_mode->vsync_end - mode->vsync_end != timing_diff)
  

• return false;
  

• else

• return true;
  

+}

• static struct dc_stream_state * create_stream_for_sink(struct amdgpu_dm_connector *aconnector, const struct drm_display_mode *drm_mode,

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

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

nitpick: change 0 to false

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

• memset(&saved_mode, 0, sizeof(struct drm_display_mode));

nitpick: sizeof(saved_mode)

• if (aconnector == NULL) { DRM_ERROR("aconnector is NULL!\n"); return stream;

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

• decide_crtc_timing_for_drm_display_mode(
  

• is_fs_vid_mode = 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)
  

nitpick: don't need to check this twice, just use an } else {

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

• preferred_refresh = drm_mode_vrefresh(preferred_mode); }

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

• /*

• if (dm_state && is_fs_vid_mode)

• drm_mode_set_crtcinfo(&saved_mode, 0);
  

•   /*
  

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

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

• fill_stream_properties_from_drm_display_mode(stream,
  

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

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

• fill_stream_properties_from_drm_display_mode(
  

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

• 	requested_bpc, dm_state ? 1 : 0);
  

Some notes on this dm_state 1 : 0 stuff below...

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;

@@ -7847,13 +7907,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;
  

style nitpick: can just drop this goto if you do an } else { here.

• }
  

• config.state = new_crtc_state->base.vrr_enabled ? VRR_STATE_ACTIVE_VARIABLE : VRR_STATE_INACTIVE;

• } else {

• } else

style nitpick: prefer braces on all conditions if they're on any, ie. use } else {

config.state = VRR_STATE_UNSUPPORTED;

• }

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

You should be modifying the amdgpu_dm_vrr_active check instead. It's important that we're sending back the correct vblank events to userspace as long as we're in DRR.

The logic that does this is in the IRQ handlers, but you should only need to modify this check function.

•     acrtc_state->freesync_video_mode) {
  spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
  dc_stream_adjust_vmin_vmax(
  	dm->dc, acrtc_state->stream,
  

@@ -8867,6 +8945,7 @@ static void get_freesync_config_for_crtc( to_amdgpu_dm_connector(new_con_state->base.connector); struct drm_display_mode *mode = &new_crtc_state->base.mode; int vrefresh = drm_mode_vrefresh(mode);

• bool fs_vid_mode = false;

  new_crtc_state->vrr_supported = new_con_state->freesync_capable && vrefresh >= aconnector->min_vfreq &&

@@ -8874,17 +8953,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;
  

style nitpick: braces on all if conditions

You also don't need to check !fs_vid_mode, it's always false in this branch.

• }

+out: new_crtc_state->freesync_config = config; }

@@ -8897,6 +8985,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;

The 'freesync_video_mode' doesn't automatically carry over when CRTC state is duplicated. You'll need to add that to dm_crtc_duplicate_state as well, but keep in mind that I don't see you resetting this to false anywhere.

You might want to add that in on removing the stream, but I haven't looked too deep into this.

+}

• static int dm_update_crtc_state(struct amdgpu_display_manager *dm, struct drm_atomic_state *state, struct drm_crtc *crtc,

@@ -8987,6 +9120,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)
  

Reorder this so that we have amdgpu_exp_freesync_vid_mode as the first check. Small optimization.

• 	goto skip_modeset;
  

• if (dm_new_crtc_state->stream && dc_is_stream_unchanged(new_stream, dm_old_crtc_state->stream) && dc_is_stream_scaling_unchanged(new_stream, dm_old_crtc_state->stream)) {

@@ -9018,6 +9156,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) {
  

Reorder this so that we have amdgpu_exp_freesync_vid_mode as the first check. Small optimization.

Regards, Nicholas Kazlauskas

• 	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 3ea85be9c546..ff4675572125 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h @@ -474,6 +474,7 @@ struct dm_crtc_state {

bool freesync_timing_changed; bool freesync_vrr_info_changed;

• bool freesync_video_mode;

  bool dsc_force_changed; bool vrr_supported;