| Message ID | 20190418035122.15791-5-mario.kleiner.de@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] drm/amd/display: Add some debug output for VRR BTR. | expand |
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
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
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(
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(-)