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@xxxxxxx
>>> ---
>>> .../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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
