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