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(&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_ir
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(&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_inf