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

[Shashank Sharma]

On 11/12/20 8:19 pm, Kazlauskas, Nicholas wrote:
> On 2020-12-11 12:08 a.m., Shashank Sharma wrote:
>> 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 at 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
> I'd rather we not make mode name part of a UAPI or to identify a 
> "FreeSync mode". This is already behind a module option and from the 
> driver's perspective we only need the timing to understand whether or 
> not we can do an optimized modeset using FreeSync into it. Driver 
> private flags can optimize the check away but it's only a few 
> comparisons so I don't see much benefit.
The module parameter is just to control the addition of freesync modes or not, but that doesn't let you differentiate between a genuine EDID mode or Freesync modes added by us, to accommodate freesync solution.  
>
> We will always need to reference the original preferred mode regardless 
> of how the FreeSync mode is identified since there could be a case where 
> we're enabling the CRTC from disabled -> enabled. The display was 
> previously blank and we need to reprogram the OTG timing to the mode 
> that doesn't have an extended front porch.

I think there is a gap in understanding my comment here. If you see the current implement of function "is_freesync_video_mode", what it does is it explores all the modes from the connector's probed_modes list, and compares them with possible_fs_modes, and decides if this mode is a freesync mode or not. My point is, we have already done this exercise once, while we were adding the freesync modes in the mode list already.

Now if we can add a driver_private flag, or some identification in the mode, we don't have to do this whole thing again.

Its like:

While adding freesync modes:

- add_freesync_modes ()

{

    get_preferred_mode()

    prepare_freesync_mode_from_preferred_modes()

    add_new_freesync_mode_in_connector_modelist()  //Add a driver private flag in this new freesync mode

}


Now, as the driver is the only source of the freesync modes, we can make the identifier function during the modeset() can be as small as:

- is_freesync_video_mode (mode)

{

    retrun (mode->flags & driver_private_flag);

}


Point being, there is no need to do the same thing again, in order to identify, if a mode is freesync mode or not, as the driver only has added these modes, and it should know which are these freesync modes.  

- Shashank

> Regards,
> Nicholas Kazlauskas
>
>>>>> +	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