[PATCH 3/3] drm/amd/display: Compensate for pre-DCE12 BTR-VRR hw limitations. (v2)
Kazlauskas, Nicholas
Nicholas.Kazlauskas at amd.com
Fri Apr 26 17:12:12 UTC 2019
On 4/26/19 6:50 AM, Mario Kleiner wrote:
> Pre-DCE12 needs special treatment for BTR / low framerate
> compensation for more stable behaviour:
>
> According to comments in the code and some testing on DCE-8
> and DCE-11, DCE-11 and earlier only apply VTOTAL_MIN/MAX
> programming with a lag of one frame, so the special BTR hw
> programming for intermediate fixed duration frames must be
> done inside the current frame at flip submission in atomic
> commit tail, ie. one vblank earlier, and the fixed refresh
> intermediate frame mode must be also terminated one vblank
> earlier on pre-DCE12 display engines.
>
> To achieve proper termination on < DCE-12 shift the point
> when the switch-back from fixed vblank duration to variable
> vblank duration happens from the start of VBLANK (vblank irq,
> as done on DCE-12+) to back-porch or end of VBLANK (handled
> by vupdate irq handler). We must leave the switch-back code
> inside VBLANK irq for DCE12+, as before.
>
> Doing this, we get much better behaviour of BTR for up-sweeps,
> ie. going from short to long frame durations (~high to low fps)
> and for constant framerate flips, as tested on DCE-8 and
> DCE-11. Behaviour is still not quite as good as on DCN-1
> though.
>
> On down-sweeps, going from long to short frame durations
> (low fps to high fps) < DCE-12 is a little bit improved,
> although by far not as much as for up-sweeps and constant
> fps.
>
> v2: Fix some wrong locking, as pointed out by Nicholas.
> Signed-off-by: Mario Kleiner <mario.kleiner.de at gmail.com>
Still looks good to me, but I just noticed one other minor nitpick (sorry!)
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 45 +++++++++++++++++--
> 1 file changed, 42 insertions(+), 3 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 76b6e621793f..7241e1f3ebec 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -364,6 +364,7 @@ static void dm_vupdate_high_irq(void *interrupt_params)
> struct amdgpu_device *adev = irq_params->adev;
> struct amdgpu_crtc *acrtc;
> struct dm_crtc_state *acrtc_state;
> + unsigned long flags;
>
> acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VUPDATE);
>
> @@ -381,6 +382,22 @@ static void dm_vupdate_high_irq(void *interrupt_params)
> */
> if (amdgpu_dm_vrr_active(acrtc_state))
> drm_crtc_handle_vblank(&acrtc->base);
Can't this block be merged with the one below?
With the condition also changed to just:
if (acrtc_state->stream && adev->family < AMDGPU_FAMILY_AI)
...
I also noticed that the crtc_state itself is always accessed unlocked
for checking whether VRR is on/off which is probably a bug... but this
patch shouldn't be the one to fix that. At the very least though it can
leave things in a bit better shape like you've already done. Thanks!
Nicholas Kazlauskas
> +
> + if (acrtc_state->stream && adev->family < AMDGPU_FAMILY_AI &&
> + acrtc_state->vrr_params.supported &&
> + acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE) {
> + spin_lock_irqsave(&adev->ddev->event_lock, flags);
> + mod_freesync_handle_v_update(
> + adev->dm.freesync_module,
> + acrtc_state->stream,
> + &acrtc_state->vrr_params);
> +
> + dc_stream_adjust_vmin_vmax(
> + adev->dm.dc,
> + acrtc_state->stream,
> + &acrtc_state->vrr_params.adjust);
> + spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
> + }
> }
> }
>
> @@ -390,6 +407,7 @@ static void dm_crtc_high_irq(void *interrupt_params)
> struct amdgpu_device *adev = irq_params->adev;
> struct amdgpu_crtc *acrtc;
> struct dm_crtc_state *acrtc_state;
> + unsigned long flags;
>
> acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VBLANK);
>
> @@ -412,9 +430,10 @@ static void dm_crtc_high_irq(void *interrupt_params)
> */
> amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
>
> - if (acrtc_state->stream &&
> + if (acrtc_state->stream && adev->family >= AMDGPU_FAMILY_AI &&
> acrtc_state->vrr_params.supported &&
> acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE) {
> + spin_lock_irqsave(&adev->ddev->event_lock, flags);
> mod_freesync_handle_v_update(
> adev->dm.freesync_module,
> acrtc_state->stream,
> @@ -424,6 +443,7 @@ static void dm_crtc_high_irq(void *interrupt_params)
> adev->dm.dc,
> acrtc_state->stream,
> &acrtc_state->vrr_params.adjust);
> + spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
> }
> }
> }
> @@ -4878,8 +4898,10 @@ static void update_freesync_state_on_stream(
> struct dc_plane_state *surface,
> u32 flip_timestamp_in_us)
> {
> - struct mod_vrr_params vrr_params = new_crtc_state->vrr_params;
> + struct mod_vrr_params vrr_params;
> struct dc_info_packet vrr_infopacket = {0};
> + struct amdgpu_device *adev = dm->adev;
> + unsigned long flags;
>
> if (!new_stream)
> return;
> @@ -4892,6 +4914,9 @@ static void update_freesync_state_on_stream(
> if (!new_stream->timing.h_total || !new_stream->timing.v_total)
> return;
>
> + spin_lock_irqsave(&adev->ddev->event_lock, flags);
> + vrr_params = new_crtc_state->vrr_params;
> +
> if (surface) {
> mod_freesync_handle_preflip(
> dm->freesync_module,
> @@ -4899,6 +4924,12 @@ static void update_freesync_state_on_stream(
> new_stream,
> flip_timestamp_in_us,
> &vrr_params);
> +
> + if (adev->family < AMDGPU_FAMILY_AI &&
> + amdgpu_dm_vrr_active(new_crtc_state)) {
> + mod_freesync_handle_v_update(dm->freesync_module,
> + new_stream, &vrr_params);
> + }
> }
>
> mod_freesync_build_vrr_infopacket(
> @@ -4930,6 +4961,8 @@ static void update_freesync_state_on_stream(
> new_crtc_state->base.crtc->base.id,
> (int)new_crtc_state->base.vrr_enabled,
> (int)vrr_params.state);
> +
> + spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
> }
>
> static void pre_update_freesync_state_on_stream(
> @@ -4937,8 +4970,10 @@ static void pre_update_freesync_state_on_stream(
> struct dm_crtc_state *new_crtc_state)
> {
> struct dc_stream_state *new_stream = new_crtc_state->stream;
> - struct mod_vrr_params vrr_params = new_crtc_state->vrr_params;
> + struct mod_vrr_params vrr_params;
> struct mod_freesync_config config = new_crtc_state->freesync_config;
> + struct amdgpu_device *adev = dm->adev;
> + unsigned long flags;
>
> if (!new_stream)
> return;
> @@ -4950,6 +4985,9 @@ static void pre_update_freesync_state_on_stream(
> if (!new_stream->timing.h_total || !new_stream->timing.v_total)
> return;
>
> + spin_lock_irqsave(&adev->ddev->event_lock, flags);
> + vrr_params = new_crtc_state->vrr_params;
> +
> if (new_crtc_state->vrr_supported &&
> config.min_refresh_in_uhz &&
> config.max_refresh_in_uhz) {
> @@ -4970,6 +5008,7 @@ static void pre_update_freesync_state_on_stream(
> sizeof(vrr_params.adjust)) != 0);
>
> new_crtc_state->vrr_params = vrr_params;
> + spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
> }
>
> static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
>
More information about the amd-gfx
mailing list