On Thu, May 7, 2020 at 7:35 PM Kazlauskas, Nicholas < nicholas.kazlaus...@amd.com> wrote:
> It applies on top of Alex's amd-staging-drm-next-branch. > > It is essentially just a revert to the old behavior with > acrtc_state->active_planes == 0 special case you added on top and some > small refactoring. > > The only remaining bits that are kind of questionable is the > VUPDATE_NO_LOCK vs VUPDATE bit again along with some of the locking around > where we check if FreeSync is active or not. > > I also don't think that VSTARTUP is the correct place to be performing the > update for the registers since the offset between VSTARTUP and VUPDATE can > be small enough such that the programming won't finish in time for some > timings. We should be doing it on line 0 instead. > > All these issues existed before this patch series at least though. > > Regards, > Nicholas Kazlauskas > > Ok, thanks. Tested it on a Samsung monitor with 48 Hz - 144 Hz range on a Raven Ridge gpu. It worked with and without your patch, both with my tests, and with the that VRRTester application from GitHub, so i guess the problem is highly dependent on small timing differences in game execution, vblank durations, etc. So fwif here's my Reviewed-and-Tested-by: Mario Kleiner <mario.kleiner...@gmail.com> -mario On 2020-05-07 12:58 p.m., Mario Kleiner wrote: > > Looking over it now, will do some testing. Alex amdgpu-drm-next branch > would be the best to test this? > > It looks like a revert of the whole vstartup series, except for that one > if(acrtc_state->active_planes == 0) special case, so i expect it should be > fine? > > thanks, > -mario > > > On Thu, May 7, 2020 at 5:56 PM Leo Li <sunpeng...@amd.com> wrote: > >> >> >> On 2020-05-06 3:47 p.m., Nicholas Kazlauskas wrote: >> > [Why] >> > We're the drm vblank event a frame too early in the case where the >> ^sending >> >> Thanks for catching this! >> >> Reviewed-by: Leo Li <sunpeng...@amd.com> >> >> > pageflip happens close to VUPDATE and ends up blocking the signal. >> > >> > The implementation in DM was previously correct *before* we started >> > sending vblank events from VSTARTUP unconditionally to handle cases >> > where HUBP was off, OTG was ON and userspace was still requesting some >> > DRM planes enabled. As part of that patch series we dropped VUPDATE >> > since it was deemed close enough to VSTARTUP, but there's a key >> > difference betweeen VSTARTUP and VUPDATE - the VUPDATE signal can be >> > blocked if we're holding the pipe lock > >> > There was a fix recently to revert the unconditional behavior for the >> > DCN VSTARTUP vblank event since it was sending the pageflip event on >> > the wrong frame - once again, due to blocking VUPDATE and having the >> > address start scanning out two frames later. >> > >> > The problem with this fix is it didn't update the logic that calls >> > drm_crtc_handle_vblank(), so the timestamps are totally bogus now. >> > >> > [How] >> > Essentially reverts most of the original VSTARTUP series but retains >> > the behavior to send back events when active planes == 0. >> > >> > Some refactoring/cleanup was done to not have duplicated code in both >> > the handlers. >> > >> > Fixes: 16f17eda8bad ("drm/amd/display: Send vblank and user events at >> vsartup for DCN") >> > Fixes: 3a2ce8d66a4b ("drm/amd/display: Disable VUpdate interrupt for >> DCN hardware") >> > Fixes: 2b5aed9ac3f7 ("drm/amd/display: Fix pageflip event race >> condition for DCN.") >> > >> > Cc: Leo Li <sunpeng...@amd.com> >> > Cc: Mario Kleiner <mario.kleiner...@gmail.com> >> > Signed-off-by: Nicholas Kazlauskas <nicholas.kazlaus...@amd.com> >> > --- >> > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 137 +++++++----------- >> > 1 file changed, 55 insertions(+), 82 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 59f1d4a94f12..30ce28f7c444 100644 >> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> > @@ -441,7 +441,7 @@ static void dm_vupdate_high_irq(void >> *interrupt_params) >> > >> > /** >> > * dm_crtc_high_irq() - Handles CRTC interrupt >> > - * @interrupt_params: ignored >> > + * @interrupt_params: used for determining the CRTC instance >> > * >> > * Handles the CRTC/VSYNC interrupt by notfying DRM's VBLANK >> > * event handler. >> > @@ -455,70 +455,6 @@ static void dm_crtc_high_irq(void >> *interrupt_params) >> > unsigned long flags; >> > >> > acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - >> IRQ_TYPE_VBLANK); >> > - >> > - if (acrtc) { >> > - acrtc_state = to_dm_crtc_state(acrtc->base.state); >> > - >> > - DRM_DEBUG_VBL("crtc:%d, vupdate-vrr:%d\n", >> > - acrtc->crtc_id, >> > - amdgpu_dm_vrr_active(acrtc_state)); >> > - >> > - /* Core vblank handling at start of front-porch is only >> possible >> > - * in non-vrr mode, as only there vblank timestamping >> will give >> > - * valid results while done in front-porch. Otherwise >> defer it >> > - * to dm_vupdate_high_irq after end of front-porch. >> > - */ >> > - if (!amdgpu_dm_vrr_active(acrtc_state)) >> > - drm_crtc_handle_vblank(&acrtc->base); >> > - >> > - /* Following stuff must happen at start of vblank, for crc >> > - * computation and below-the-range btr support in vrr >> mode. >> > - */ >> > - amdgpu_dm_crtc_handle_crc_irq(&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); >> > - } >> > - } >> > -} >> > - >> > -#if defined(CONFIG_DRM_AMD_DC_DCN) >> > -/** >> > - * dm_dcn_crtc_high_irq() - Handles VStartup interrupt for DCN >> generation ASICs >> > - * @interrupt params - interrupt parameters >> > - * >> > - * Notify DRM's vblank event handler at VSTARTUP >> > - * >> > - * Unlike DCE hardware, we trigger the handler at VSTARTUP. at which: >> > - * * We are close enough to VUPDATE - the point of no return for hw >> > - * * We are in the fixed portion of variable front porch when vrr is >> enabled >> > - * * We are before VUPDATE, where double-buffered vrr registers are >> swapped >> > - * >> > - * It is therefore the correct place to signal vblank, send user flip >> events, >> > - * and update VRR. >> > - */ >> > -static void dm_dcn_crtc_high_irq(void *interrupt_params) >> > -{ >> > - struct common_irq_params *irq_params = 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); >> > - >> > if (!acrtc) >> > return; >> > >> > @@ -528,22 +464,35 @@ static void dm_dcn_crtc_high_irq(void >> *interrupt_params) >> > amdgpu_dm_vrr_active(acrtc_state), >> > acrtc_state->active_planes); >> > >> > + /** >> > + * Core vblank handling at start of front-porch is only possible >> > + * in non-vrr mode, as only there vblank timestamping will give >> > + * valid results while done in front-porch. Otherwise defer it >> > + * to dm_vupdate_high_irq after end of front-porch. >> > + */ >> > + if (!amdgpu_dm_vrr_active(acrtc_state)) >> > + drm_crtc_handle_vblank(&acrtc->base); >> > + >> > + /** >> > + * Following stuff must happen at start of vblank, for crc >> > + * computation and below-the-range btr support in vrr mode. >> > + */ >> > amdgpu_dm_crtc_handle_crc_irq(&acrtc->base); >> > - drm_crtc_handle_vblank(&acrtc->base); >> > + >> > + /* BTR updates need to happen before VUPDATE on Vega and above. */ >> > + if (adev->family < AMDGPU_FAMILY_AI) >> > + return; >> > >> > spin_lock_irqsave(&adev->ddev->event_lock, flags); >> > >> > - if (acrtc_state->vrr_params.supported && >> > + if (acrtc_state->stream && acrtc_state->vrr_params.supported && >> > acrtc_state->freesync_config.state == >> VRR_STATE_ACTIVE_VARIABLE) { >> > - mod_freesync_handle_v_update( >> > - adev->dm.freesync_module, >> > - acrtc_state->stream, >> > - &acrtc_state->vrr_params); >> > + 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); >> > + dc_stream_adjust_vmin_vmax(adev->dm.dc, >> acrtc_state->stream, >> > + >> &acrtc_state->vrr_params.adjust); >> > } >> > >> > /* >> > @@ -556,7 +505,8 @@ static void dm_dcn_crtc_high_irq(void >> *interrupt_params) >> > * avoid race conditions between flip programming and completion, >> > * which could cause too early flip completion events. >> > */ >> > - if (acrtc->pflip_status == AMDGPU_FLIP_SUBMITTED && >> > + if (adev->family >= AMDGPU_FAMILY_RV && >> > + acrtc->pflip_status == AMDGPU_FLIP_SUBMITTED && >> > acrtc_state->active_planes == 0) { >> > if (acrtc->event) { >> > drm_crtc_send_vblank_event(&acrtc->base, >> acrtc->event); >> > @@ -568,7 +518,6 @@ static void dm_dcn_crtc_high_irq(void >> *interrupt_params) >> > >> > spin_unlock_irqrestore(&adev->ddev->event_lock, flags); >> > } >> > -#endif >> > >> > static int dm_set_clockgating_state(void *handle, >> > enum amd_clockgating_state state) >> > @@ -2454,8 +2403,36 @@ static int dcn10_register_irq_handlers(struct >> amdgpu_device *adev) >> > c_irq_params->adev = adev; >> > c_irq_params->irq_src = int_params.irq_source; >> > >> > + amdgpu_dm_irq_register_interrupt( >> > + adev, &int_params, dm_crtc_high_irq, >> c_irq_params); >> > + } >> > + >> > + /* Use VUPDATE_NO_LOCK interrupt on DCN, which seems to >> correspond to >> > + * the regular VUPDATE interrupt on DCE. We want >> DC_IRQ_SOURCE_VUPDATEx >> > + * to trigger at end of each vblank, regardless of state of the >> lock, >> > + * matching DCE behaviour. >> > + */ >> > + for (i = DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT; >> > + i <= DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT + >> adev->mode_info.num_crtc - 1; >> > + i++) { >> > + r = amdgpu_irq_add_id(adev, SOC15_IH_CLIENTID_DCE, i, >> &adev->vupdate_irq); >> > + >> > + if (r) { >> > + DRM_ERROR("Failed to add vupdate irq id!\n"); >> > + return r; >> > + } >> > + >> > + int_params.int_context = INTERRUPT_HIGH_IRQ_CONTEXT; >> > + int_params.irq_source = >> > + dc_interrupt_to_irq_source(dc, i, 0); >> > + >> > + c_irq_params = >> &adev->dm.vupdate_params[int_params.irq_source - DC_IRQ_SOURCE_VUPDATE1]; >> > + >> > + c_irq_params->adev = adev; >> > + c_irq_params->irq_src = int_params.irq_source; >> > + >> > amdgpu_dm_irq_register_interrupt(adev, &int_params, >> > - dm_dcn_crtc_high_irq, c_irq_params); >> > + dm_vupdate_high_irq, c_irq_params); >> > } >> > >> > /* Use GRPH_PFLIP interrupt */ >> > @@ -4544,10 +4521,6 @@ static inline int dm_set_vupdate_irq(struct >> drm_crtc *crtc, bool enable) >> > struct amdgpu_device *adev = crtc->dev->dev_private; >> > int rc; >> > >> > - /* Do not set vupdate for DCN hardware */ >> > - if (adev->family > AMDGPU_FAMILY_AI) >> > - return 0; >> > - >> > irq_source = IRQ_TYPE_VUPDATE + acrtc->otg_inst; >> > >> > rc = dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : >> -EBUSY; >> > >> > >
_______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx