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

From: Aurabindo Pillai <aurabindo.pillai@amd.com>
To: "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com>
Cc: stylon.wang@amd.com, shashank.sharma@amd.com, thong.thai@amd.com,
	dri-devel@lists.freedesktop.org, amd-gfx@lists.freedesktop.org,
	wayne.lin@amd.com, alexander.deucher@amd.com,
	"Christian König" <christian.koenig@amd.com>
Subject: Re: [PATCH v3 3/3] drm/amd/display: Skip modeset for front porch\r
 change
Date: Sun, 17 Jan 2021 14:52:27 -0500	[thread overview]
Message-ID: <FJG3NQ.G71BWG7EC3AR1@amd.com> (raw)
In-Reply-To: <ca40cea2-cbdb-73ad-9995-f18e750fb090@amd.com>

--

Thanks & Regards,
Aurabindo Pillai

On Wed, Jan 6, 2021 at 15:02, "Kazlauskas, Nicholas" 
<nicholas.kazlauskas@amd.com> wrote:
> 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);
>>   \x7f+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;
>>   	}
>>   \x7f-	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);
>>   \x7f  	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;
>>   }
>>   \x7f+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);
>>   	}
>>   \x7f  	if (!dm_state)
>>   		drm_mode_set_crtcinfo(&mode, 0);
>>   \x7f-	/*
>> +	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);
>>   \x7f  	stream->timing.flags.DSC = 0;
>>   \x7f@@ -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;
>> -	}
>>   \x7f+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.

This is a duplicate check indeed. Will have it removed. However the 
check for freesync_video_mode is still needed. Will send a v5 along 
with other changes suggested above.

> 
> 
>> +		    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;
>>   \x7f  	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(
>>   \x7f  	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;
>> -	}
>>   \x7f+		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.

Will fix.

> 
>> +
>> +	}
>> +out:
>>   	new_crtc_state->freesync_config = config;
>>   }
>>   \x7f@@ -8897,6 +8985,51 @@ static void reset_freesync_config_for_crtc(
>>   	       sizeof(new_crtc_state->vrr_infopacket));
>>   }
>>   \x7f+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.

Freesync config is already copied in dm_crtc_duplicate_state, so we 
only need to add freesync_video_mode duplication there. Will add 
resetting freesync_video_mode as well. I'm not sure if setting this to 
false during stream removal is necessary though, since other freesync 
parameters appear to be untouched in dm's commit_tail. What I'm 
thinking is like:

---
static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)

[snip]
                                continue;
                        }

-                       if (dm_old_crtc_state->stream)
+                       if (dm_old_crtc_state->stream) {
                                remove_stream(adev, acrtc, 
dm_old_crtc_state->stream);
+                               dm_old_crtc_state->freesync_video_mode 
= 0;
+                       }

                        pm_runtime_get_noresume(dev->dev);
---

> 
>> +}
>> +
>>   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;
>>   \x7f+		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.
> 

Will do these two reorders.

> 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 {
>>   \x7f  	bool freesync_timing_changed;
>>   	bool freesync_vrr_info_changed;
>> +	bool freesync_video_mode;
>>   \x7f  	bool dsc_force_changed;
>>   	bool vrr_supported;
>> 
> 

--

Thanks & Regards,
Aurabindo

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel