Improvements to VRR below-the-range/low framerate compensation.

Improvements to VRR below-the-range/low framerate compensation.

[Mario Kleiner]

Hi

This patch-series tries to improve amdgpu's below-the-range
behaviour with Freesync, hopefully not only for my use case,
but also for games etc.

Patch 1/4 adds a bit of debug output i found very useful, so maybe
worth adding?

Patch 2/4 fixes a bug i found when reading over the freesync code.

Patches 3/4 and 4/4 are optimizations to improve stability
in BTR.

My desired application of VRR for neuroscience/vision research
is to control the timing of when frames show up onscreen, e.g.,
to show animations at different "unconventional" framerates,
so i'm mostly interested in how well one can control the timing
between successive OpenGL bufferswaps. This is a bit different
from what a game wants to get out of VRR, probably closer to
what a movie player might want to do.

I spent quite a bit of time testing how FreeSync behaves when
flipping at a rate below the displays VRR minimum refresh rate.

For that, my own application was submitting glXSwapBuffers() flip
requests at different fps rates / time delays between successive
flips. The test script is for GNU/Octave + psychtoolbox-3, but
in principle the C equivalent would be this pseudo-code:

for (i = 0; i < n; i++) {
  // Wait for pending flip to complete, get pageflip timestamp t_last
  // of flip completion:
  glXWaitForSbcOML(...., &t_last[i],...);

  // Fetch some delay value until next flip tdelay[i]
  tdelay[i] = Some function of varying frame delay over samples.

  // Try to flip tdelay[i] secs after previous flip t_last[i]:
  t_next = t_last[i] + tdelay[i];
  clock_nanosleep(t_next);

  // Flip
  glXSwapBuffers(...);
}

For tdelay[i] i used different test profiles, e.g., on a display with
a VRR range from 30 Hz to 144 Hz ~ 7 msecs - 33 msecs:

tdelay[i] = 0.050 // One flip each 50 msecs, want constant 20 fps.
tdelay[i] = rand() // Some randomly chosen delay for each flip.
tdelay[i] = 0.007 + some sin() sine profile of changing delays/fps.
tdelay[i] = 0.007 + i * 0.001; linear increase in delay by 1 msec/flip,
            starting at 7 msecs.
tdelay[i] = ... linear decrease by 1 msec/flip.. starting at 120 msecs.

etc. Then i plotted requested flip delays tdelay[] against actual
flip delays (~ t_last[i+1] - t_last[i]) to see how well VRR can follow
requested fps.

Inside the VRR range ~ 7 msecs - 33 msecs, Freesync behaved basically
perfect with average errors of less than 0.1 msecs and jitter of less
than 1 msec.

When going for tdelay's > 33 msecs, ie. when low framerate compensation/
BTR kicked in, my DCN-1 Raven Ridge APU behaved almost as well as within
the VRR range (for reasonably smooth changes in fps). When doing the
same on a DCE-8 and DCE-11 gpu, BTR made much bigger errors between what
was requested and what was measured.

Patch 3/4 helps avoiding glitches on DCN when transitioning from VRR range
to below min VRR range, and helps even more in avoiding glitches on DCE.

Patch 4/4 tries to improve behaviour on pre-DCE12. It helps quite a lot
when testing on DCE-8 and DCE-11 as described in the commit message. This
makes sense, as the code has some TODO comment about the different hw
behaviour of pre-DCE12, mentioning that pre-DCE12 hw only responds to 
programming different VTOTAL_MIN/MAX values with a lag of 1 frame. The
patch tries to work around this hardware limitation with some success.
DCE8/11 behaviour is still not as good as DCN-1 behaviour, but at least
it is not totally useless for my type of application with this patchset.

I don't have a Vega discrete gpu with DCE-12, but assume it would behave
like DCN-1 if the comments in the code are correct? Therefore the patch
switches BTR handling for < AMDGPU_FAMILY_AI vs. >= AMDGPU_FAMILY_AI.

Thanks,
-mario


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

[PATCH 1/4] drm/amd/display: Add some debug output for VRR BTR.

[Mario Kleiner]

Helps with debugging issues with low framerate compensation.

Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
---
 .../amd/display/modules/freesync/freesync.c    | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
index 3d867e34f8b3..71274683da04 100644
--- a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
+++ b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
@@ -1041,6 +1041,11 @@ void mod_freesync_handle_preflip(struct mod_freesync *mod_freesync,
 		average_render_time_in_us += last_render_time_in_us;
 		average_render_time_in_us /= DC_PLANE_UPDATE_TIMES_MAX;
 
+		DRM_DEBUG_DRIVER("vrr flip: avg %d us, last %d us, max %d us\n",
+				 average_render_time_in_us,
+				 last_render_time_in_us,
+				 in_out_vrr->max_duration_in_us);
+
 		if (in_out_vrr->btr.btr_enabled) {
 			apply_below_the_range(core_freesync,
 					stream,
@@ -1053,6 +1058,10 @@ void mod_freesync_handle_preflip(struct mod_freesync *mod_freesync,
 				in_out_vrr);
 		}
 
+		DRM_DEBUG_DRIVER("vrr btr_active:%d - num %d of dur %d us\n",
+				 in_out_vrr->btr.btr_active,
+				 in_out_vrr->btr.frames_to_insert,
+				 in_out_vrr->btr.inserted_duration_in_us);
 	}
 }
 
@@ -1090,11 +1099,17 @@ void mod_freesync_handle_v_update(struct mod_freesync *mod_freesync,
 				in_out_vrr->btr.inserted_duration_in_us);
 			in_out_vrr->adjust.v_total_max =
 				in_out_vrr->adjust.v_total_min;
+			DRM_DEBUG_DRIVER("btr start: c=%d, vtotal=%d\n",
+					 in_out_vrr->btr.frames_to_insert,
+					 in_out_vrr->adjust.v_total_min);
 		}
 
 		if (in_out_vrr->btr.frame_counter > 0)
 			in_out_vrr->btr.frame_counter--;
 
+		DRM_DEBUG_DRIVER("btr upd: count %d\n",
+				 in_out_vrr->btr.frame_counter);
+
 		/* Restore FreeSync */
 		if (in_out_vrr->btr.frame_counter == 0) {
 			in_out_vrr->adjust.v_total_min =
@@ -1103,6 +1118,9 @@ void mod_freesync_handle_v_update(struct mod_freesync *mod_freesync,
 			in_out_vrr->adjust.v_total_max =
 				calc_v_total_from_refresh(stream,
 				in_out_vrr->min_refresh_in_uhz);
+			DRM_DEBUG_DRIVER("btr end: vtotal_min=%d/max=%d\n",
+					 in_out_vrr->adjust.v_total_min,
+					 in_out_vrr->adjust.v_total_max);
 		}
 	}
 
-- 
2.20.1

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

[PATCH 2/4] drm/amd/display: Fix and simplify apply_below_the_range()

[Mario Kleiner]

The comparison of inserted_frame_duration_in_us against a
duration calculated from max_refresh_in_uhz is both wrong
in its math and not needed, as the min_duration_in_us value
is already cached in in_out_vrr for reuse. No need to
recalculate it wrongly at each invocation.

Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
---
 drivers/gpu/drm/amd/display/modules/freesync/freesync.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
index 71274683da04..e56543c36eba 100644
--- a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
+++ b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
@@ -437,10 +437,8 @@ static void apply_below_the_range(struct core_freesync *core_freesync,
 			inserted_frame_duration_in_us = last_render_time_in_us /
 							frames_to_insert;
 
-		if (inserted_frame_duration_in_us <
-			(1000000 / in_out_vrr->max_refresh_in_uhz))
-			inserted_frame_duration_in_us =
-				(1000000 / in_out_vrr->max_refresh_in_uhz);
+		if (inserted_frame_duration_in_us < in_out_vrr->min_duration_in_us)
+			inserted_frame_duration_in_us = in_out_vrr->min_duration_in_us;
 
 		/* Cache the calculated variables */
 		in_out_vrr->btr.inserted_duration_in_us =
-- 
2.20.1

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

[PATCH 3/4] drm/amd/display: Enter VRR BTR earlier.

[Mario Kleiner]

Use a 2 msecs switching headroom not only for slightly
delayed exiting of BTR mode, but now also for entering
it a bit before the max frame duration is exceeded.

With slowly changing time delay between successive flips
or with a bit of jitter in arrival of flips, this adapts
vblank early and prevents missed vblanks at the border
between non-BTR and BTR.

Testing on DCE-8, DCE-11 and DCN-1.0 shows that this more
often avoids skipped frames when moving across the BTR
boundary, especially on DCE-8 and DCE-11 with the followup
commit for dealing with pre-DCE-12 hw.

Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
---
 drivers/gpu/drm/amd/display/modules/freesync/freesync.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
index e56543c36eba..a965ab5466dc 100644
--- a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
+++ b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
@@ -350,7 +350,7 @@ static void apply_below_the_range(struct core_freesync *core_freesync,
 			in_out_vrr->btr.frame_counter = 0;
 			in_out_vrr->btr.btr_active = false;
 		}
-	} else if (last_render_time_in_us > max_render_time_in_us) {
+	} else if (last_render_time_in_us + BTR_EXIT_MARGIN > max_render_time_in_us) {
 		/* Enter Below the Range */
 		in_out_vrr->btr.btr_active = true;
 	}
-- 
2.20.1

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

[PATCH 4/4] drm/amd/display: Compensate for pre-DCE12 BTR-VRR hw limitations.

[Mario Kleiner]

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.

Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 ++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

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..9c8c94f82b35 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);
+
+		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);
 		}
 	}
 }
@@ -4880,6 +4900,8 @@ static void update_freesync_state_on_stream(
 {
 	struct mod_vrr_params vrr_params = new_crtc_state->vrr_params;
 	struct dc_info_packet vrr_infopacket = {0};
+	struct amdgpu_device *adev = dm->adev;
+	unsigned long flags;
 
 	if (!new_stream)
 		return;
@@ -4899,6 +4921,14 @@ 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)) {
+			spin_lock_irqsave(&adev->ddev->event_lock, flags);
+			mod_freesync_handle_v_update(dm->freesync_module,
+						     new_stream, &vrr_params);
+			spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
+		}
 	}
 
 	mod_freesync_build_vrr_infopacket(
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Improvements to VRR below-the-range/low framerate compensation.

[Michel Dänzer]

On 2019-04-18 5:51 a.m., Mario Kleiner wrote:
> 
> My desired application of VRR for neuroscience/vision research
> is to control the timing of when frames show up onscreen, e.g.,
> to show animations at different "unconventional" framerates,
> so i'm mostly interested in how well one can control the timing
> between successive OpenGL bufferswaps. This is a bit different
> from what a game wants to get out of VRR, probably closer to
> what a movie player might want to do.

As discussed before, I expect that games which care about smooth
animation will want to control the presentation time as well in the long
run, so that they can make the presentation time match the simulation
time corresponding to the frame contents.

There is already support for specifying the target presentation time in
(at least, there might be more I'm not aware of) VDPAU, the Vulkan
VK_GOOGLE_display_timing extension, and the X11 Present extension
protocol (not actually implemented yet in xserver though).

Ideally, this should also be available in the kernel UAPI. That should
allow hitting the target more accurately / reliably, and might also
allow making better use of compensation mechanisms such as those touched
by this series, since userspace can call into the kernel ahead of the
target time, giving the kernel an opportunity to make adjustments earlier.

Since you have a strong use-case for this functionality, maybe you'd be
interested in working on it?


-- 
Earthling Michel Dänzer               |              https://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Improvements to VRR below-the-range/low framerate compensation.

[Kazlauskas, Nicholas]

On 4/18/19 10:24 AM, Michel Dänzer wrote:
> On 2019-04-18 5:51 a.m., Mario Kleiner wrote:
>>
>> My desired application of VRR for neuroscience/vision research
>> is to control the timing of when frames show up onscreen, e.g.,
>> to show animations at different "unconventional" framerates,
>> so i'm mostly interested in how well one can control the timing
>> between successive OpenGL bufferswaps. This is a bit different
>> from what a game wants to get out of VRR, probably closer to
>> what a movie player might want to do.
> 
> As discussed before, I expect that games which care about smooth
> animation will want to control the presentation time as well in the long
> run, so that they can make the presentation time match the simulation
> time corresponding to the frame contents.
> 
> There is already support for specifying the target presentation time in
> (at least, there might be more I'm not aware of) VDPAU, the Vulkan
> VK_GOOGLE_display_timing extension, and the X11 Present extension
> protocol (not actually implemented yet in xserver though).
> 
> Ideally, this should also be available in the kernel UAPI. That should
> allow hitting the target more accurately / reliably, and might also
> allow making better use of compensation mechanisms such as those touched
> by this series, since userspace can call into the kernel ahead of the
> target time, giving the kernel an opportunity to make adjustments earlier.
> 
> Since you have a strong use-case for this functionality, maybe you'd be
> interested in working on it?
> 
> 

This would be a good real-world userspace application that takes 
advantage of this.

There were some threads before in the original VRR RFC and patch series 
that discussion this, but I think the summary was that most people 
interested were in favor of a timestamp in nanoseconds on the CRTC.

My thoughts on the matter would be that the driver would be enforced not 
to present the frame any earlier than the timestamp given. If the driver 
isn't capable of doing that then it can return -EINVAL (ie. userspace 
asks for a really large timestamp and the driver would have to stall 
forever in order to display it).

There could still be some leeway in terms of when the frame could be 
displayed. Maybe another property could be also used to specify the 
upper bound for presentation.

I would imagine any properties created here would be optional to drivers 
to add. In hindsight, the VRR CRTC property should probably have been 
optional as well.

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

Re: [PATCH 1/4] drm/amd/display: Add some debug output for VRR BTR.

[Kazlauskas, Nicholas]

On 4/17/19 11:51 PM, Mario Kleiner wrote:
> Helps with debugging issues with low framerate compensation.
> 
> Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
> ---

This looks like it'd generate a ton of debug output (the flip stuff is 
already a bit too spammy).

But more importantly the DC and module code doesn't touch the debug 
prints directly.

DC has the DC_LOG_* defines but the modules shouldn't touch anything 
logger related.

Nicholas Kazlauskas

>   .../amd/display/modules/freesync/freesync.c    | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> index 3d867e34f8b3..71274683da04 100644
> --- a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> +++ b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> @@ -1041,6 +1041,11 @@ void mod_freesync_handle_preflip(struct mod_freesync *mod_freesync,
>   		average_render_time_in_us += last_render_time_in_us;
>   		average_render_time_in_us /= DC_PLANE_UPDATE_TIMES_MAX;
>   
> +		DRM_DEBUG_DRIVER("vrr flip: avg %d us, last %d us, max %d us\n",
> +				 average_render_time_in_us,
> +				 last_render_time_in_us,
> +				 in_out_vrr->max_duration_in_us);
> +
>   		if (in_out_vrr->btr.btr_enabled) {
>   			apply_below_the_range(core_freesync,
>   					stream,
> @@ -1053,6 +1058,10 @@ void mod_freesync_handle_preflip(struct mod_freesync *mod_freesync,
>   				in_out_vrr);
>   		}
>   
> +		DRM_DEBUG_DRIVER("vrr btr_active:%d - num %d of dur %d us\n",
> +				 in_out_vrr->btr.btr_active,
> +				 in_out_vrr->btr.frames_to_insert,
> +				 in_out_vrr->btr.inserted_duration_in_us);
>   	}
>   }
>   
> @@ -1090,11 +1099,17 @@ void mod_freesync_handle_v_update(struct mod_freesync *mod_freesync,
>   				in_out_vrr->btr.inserted_duration_in_us);
>   			in_out_vrr->adjust.v_total_max =
>   				in_out_vrr->adjust.v_total_min;
> +			DRM_DEBUG_DRIVER("btr start: c=%d, vtotal=%d\n",
> +					 in_out_vrr->btr.frames_to_insert,
> +					 in_out_vrr->adjust.v_total_min);
>   		}
>   
>   		if (in_out_vrr->btr.frame_counter > 0)
>   			in_out_vrr->btr.frame_counter--;
>   
> +		DRM_DEBUG_DRIVER("btr upd: count %d\n",
> +				 in_out_vrr->btr.frame_counter);
> +
>   		/* Restore FreeSync */
>   		if (in_out_vrr->btr.frame_counter == 0) {
>   			in_out_vrr->adjust.v_total_min =
> @@ -1103,6 +1118,9 @@ void mod_freesync_handle_v_update(struct mod_freesync *mod_freesync,
>   			in_out_vrr->adjust.v_total_max =
>   				calc_v_total_from_refresh(stream,
>   				in_out_vrr->min_refresh_in_uhz);
> +			DRM_DEBUG_DRIVER("btr end: vtotal_min=%d/max=%d\n",
> +					 in_out_vrr->adjust.v_total_min,
> +					 in_out_vrr->adjust.v_total_max);
>   		}
>   	}
>   
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/4] drm/amd/display: Fix and simplify apply_below_the_range()

[Kazlauskas, Nicholas]

On 4/17/19 11:51 PM, Mario Kleiner wrote:
> The comparison of inserted_frame_duration_in_us against a
> duration calculated from max_refresh_in_uhz is both wrong
> in its math and not needed, as the min_duration_in_us value
> is already cached in in_out_vrr for reuse. No need to
> recalculate it wrongly at each invocation.
> 
> Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>

Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>

Looks reasonable to me.

> ---
>   drivers/gpu/drm/amd/display/modules/freesync/freesync.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> index 71274683da04..e56543c36eba 100644
> --- a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> +++ b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> @@ -437,10 +437,8 @@ static void apply_below_the_range(struct core_freesync *core_freesync,
>   			inserted_frame_duration_in_us = last_render_time_in_us /
>   							frames_to_insert;
>   
> -		if (inserted_frame_duration_in_us <
> -			(1000000 / in_out_vrr->max_refresh_in_uhz))
> -			inserted_frame_duration_in_us =
> -				(1000000 / in_out_vrr->max_refresh_in_uhz);
> +		if (inserted_frame_duration_in_us < in_out_vrr->min_duration_in_us)
> +			inserted_frame_duration_in_us = in_out_vrr->min_duration_in_us;
>   
>   		/* Cache the calculated variables */
>   		in_out_vrr->btr.inserted_duration_in_us =
> 

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

Re: [PATCH 4/4] drm/amd/display: Compensate for pre-DCE12 BTR-VRR hw limitations.

[Kazlauskas, Nicholas]

On 4/17/19 11:51 PM, 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.
> 
> Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
> ---

I did some debugging/testing with this patch and it certainly does 
improve things quite a bit for pre DCE12 (since this really should have 
been handled in VUPDATE before).

I have one comment inline below:


>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 ++++++++++++++++++-
>   1 file changed, 31 insertions(+), 1 deletion(-)
> 
> 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..9c8c94f82b35 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);
> +
> +		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);
>   		}
>   	}
>   }
> @@ -4880,6 +4900,8 @@ static void update_freesync_state_on_stream(
>   {
>   	struct mod_vrr_params vrr_params = new_crtc_state->vrr_params;
>   	struct dc_info_packet vrr_infopacket = {0};
> +	struct amdgpu_device *adev = dm->adev;
> +	unsigned long flags;
>   
>   	if (!new_stream)
>   		return;
> @@ -4899,6 +4921,14 @@ 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)) {
> +			spin_lock_irqsave(&adev->ddev->event_lock, flags);
> +			mod_freesync_handle_v_update(dm->freesync_module,
> +						     new_stream, &vrr_params);
> +			spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
> +		}

Shouldn't the locking here actually be around setting the 
new_crtc_state->adjust/vrr_infopacket/vrr_params?

I forgot the locking here before but it seems like we should be doing it 
there since all that state is accessed from the IRQs.

>   	}
>   
>   	mod_freesync_build_vrr_infopacket(
> 

Nicholas Kazlauskas
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm/amd/display: Compensate for pre-DCE12 BTR-VRR hw limitations.

[Mario Kleiner]

On Wed, Apr 24, 2019 at 4:34 PM Kazlauskas, Nicholas
<Nicholas.Kazlauskas@amd.com> wrote:
>
> On 4/17/19 11:51 PM, 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.
> >
> > Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
> > ---
>
> I did some debugging/testing with this patch and it certainly does
> improve things quite a bit for pre DCE12 (since this really should have
> been handled in VUPDATE before).
>

Do you know at which exact point in the frame cycle or vblank the new
VTOTAL_MIN/MAX gets latched by the hw?

Btw. for reference i made a little git repo with a cleaned up version
my test script VRRTest.m and some pdf's with some plots from my
testing with a slightly earlier version of that script:

https://github.com/kleinerm/VRRTestPlots

Easy to use on a Debian/Ubuntu based system as you can get GNU/Octave
+ psychtoolbox-3 from the distro repo. Explanations in the Readme.md

> I have one comment inline below:
>
>
> >   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 ++++++++++++++++++-
> >   1 file changed, 31 insertions(+), 1 deletion(-)
> >
> > 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..9c8c94f82b35 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);
> > +
> > +             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);
> >               }
> >       }
> >   }
> > @@ -4880,6 +4900,8 @@ static void update_freesync_state_on_stream(
> >   {
> >       struct mod_vrr_params vrr_params = new_crtc_state->vrr_params;
> >       struct dc_info_packet vrr_infopacket = {0};
> > +     struct amdgpu_device *adev = dm->adev;
> > +     unsigned long flags;
> >
> >       if (!new_stream)
> >               return;
> > @@ -4899,6 +4921,14 @@ 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)) {
> > +                     spin_lock_irqsave(&adev->ddev->event_lock, flags);
> > +                     mod_freesync_handle_v_update(dm->freesync_module,
> > +                                                  new_stream, &vrr_params);
> > +                     spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
> > +             }
>
> Shouldn't the locking here actually be around setting the
> new_crtc_state->adjust/vrr_infopacket/vrr_params?
>
> I forgot the locking here before but it seems like we should be doing it
> there since all that state is accessed from the IRQs.
>

Yes, you're right. I think we need to put the whole function into the
lock, and also the pre_update_freesync_state... function.
I'll send out an updated patch after testing.

thanks,
-mario

> >       }
> >
> >       mod_freesync_build_vrr_infopacket(
> >
>
> Nicholas Kazlauskas
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx