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

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

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