Re: [PATCH 4/4] drm/amd/display: Compensate for pre-DCE12 BTR-VRR hw limitations.
On Wed, Apr 24, 2019 at 4:34 PM Kazlauskas, Nicholas
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
> > ---
>
> 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(>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(>ddev->event_lock, flags);
> > + mod_freesync_handle_v_update(
> > + adev->dm.freesync_module,
> > + acrtc_state->stream,
> > + _state->vrr_params);
> > +
> > + dc_stream_adjust_vmin_vmax(
> > + adev->dm.dc,
> > + acrtc_state->stream,
> > + _state->vrr_params.adjust);
> > + spin_unlock_irqrestore(>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(>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(>ddev->event_lock, flags);
> >
Re: [PATCH 4/4] drm/amd/display: Compensate for pre-DCE12 BTR-VRR hw limitations.
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
> ---
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(>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(>ddev->event_lock, flags);
> + mod_freesync_handle_v_update(
> + adev->dm.freesync_module,
> + acrtc_state->stream,
> + _state->vrr_params);
> +
> + dc_stream_adjust_vmin_vmax(
> + adev->dm.dc,
> + acrtc_state->stream,
> + _state->vrr_params.adjust);
> + spin_unlock_irqrestore(>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(>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(>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,
> _state->vrr_params.adjust);
> + spin_unlock_irqrestore(>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
