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

["Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com>]

On 2020-12-11 10:35 a.m., Shashank Sharma wrote:
> 
> 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@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.

It's for both the modeset optimization and the FreeSync mode generation.

>>
>> 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

To clarify, the original point was that knowing whether the FreeSync 
mode is a video compatible mode isn't enough - we need to know what the 
base mode's vtotal was to correctly program the hardware.

The base mode is basically the FreeSync optimized mode - preferred mode 
+ highest refresh rate to support BTR.

So when determining whether we skip the modeset the logic should be:

1. Find the base/freesync optimized mode. We can just iterate the mode 
list for this for an unoptimized implementation but I don't think we 
really need to go further than this since this only happens on a modeset.

2. Compare the incoming mode against the freesync optimized mode. If the 
only thing that differs is the vtotal/front porch duration and it's 
within the range supported by the display then we can skip the blank.

Regards,
Nicholas Kazlauskas

> 
>> 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

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx