<div dir="ltr"><div>Looking over it now, will do some testing. Alex amdgpu-drm-next branch would be the best to test this?</div><div><br></div><div>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?</div><div><br></div><div>thanks,<br></div><div>-mario</div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, May 7, 2020 at 5:56 PM Leo Li <<a href="mailto:sunpeng.li@amd.com">sunpeng.li@amd.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
<br>
On 2020-05-06 3:47 p.m., Nicholas Kazlauskas wrote:<br>
> [Why]<br>
> We're the drm vblank event a frame too early in the case where the<br>
        ^sending<br>
<br>
Thanks for catching this!<br>
<br>
Reviewed-by: Leo Li <<a href="mailto:sunpeng.li@amd.com" target="_blank">sunpeng.li@amd.com</a>><br>
<br>
> pageflip happens close to VUPDATE and ends up blocking the signal.<br>
> <br>
> The implementation in DM was previously correct *before* we started<br>
> sending vblank events from VSTARTUP unconditionally to handle cases<br>
> where HUBP was off, OTG was ON and userspace was still requesting some<br>
> DRM planes enabled. As part of that patch series we dropped VUPDATE<br>
> since it was deemed close enough to VSTARTUP, but there's a key<br>
> difference betweeen VSTARTUP and VUPDATE - the VUPDATE signal can be<br>
> blocked if we're holding the pipe lock ><br>
> There was a fix recently to revert the unconditional behavior for the<br>
> DCN VSTARTUP vblank event since it was sending the pageflip event on<br>
> the wrong frame - once again, due to blocking VUPDATE and having the<br>
> address start scanning out two frames later.<br>
> <br>
> The problem with this fix is it didn't update the logic that calls<br>
> drm_crtc_handle_vblank(), so the timestamps are totally bogus now.<br>
> <br>
> [How]<br>
> Essentially reverts most of the original VSTARTUP series but retains<br>
> the behavior to send back events when active planes == 0.<br>
> <br>
> Some refactoring/cleanup was done to not have duplicated code in both<br>
> the handlers.<br>
> <br>
> Fixes: 16f17eda8bad ("drm/amd/display: Send vblank and user events at vsartup for DCN")<br>
> Fixes: 3a2ce8d66a4b ("drm/amd/display: Disable VUpdate interrupt for DCN hardware")<br>
> Fixes: 2b5aed9ac3f7 ("drm/amd/display: Fix pageflip event race condition for DCN.")<br>
> <br>
> Cc: Leo Li <<a href="mailto:sunpeng.li@amd.com" target="_blank">sunpeng.li@amd.com</a>><br>
> Cc: Mario Kleiner <<a href="mailto:mario.kleiner.de@gmail.com" target="_blank">mario.kleiner.de@gmail.com</a>><br>
> Signed-off-by: Nicholas Kazlauskas <<a href="mailto:nicholas.kazlauskas@amd.com" target="_blank">nicholas.kazlauskas@amd.com</a>><br>
> ---<br>
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 137 +++++++-----------<br>
>   1 file changed, 55 insertions(+), 82 deletions(-)<br>
> <br>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c<br>
> index 59f1d4a94f12..30ce28f7c444 100644<br>
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c<br>
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c<br>
> @@ -441,7 +441,7 @@ static void dm_vupdate_high_irq(void *interrupt_params)<br>
>   <br>
>   /**<br>
>    * dm_crtc_high_irq() - Handles CRTC interrupt<br>
> - * @interrupt_params: ignored<br>
> + * @interrupt_params: used for determining the CRTC instance<br>
>    *<br>
>    * Handles the CRTC/VSYNC interrupt by notfying DRM's VBLANK<br>
>    * event handler.<br>
> @@ -455,70 +455,6 @@ static void dm_crtc_high_irq(void *interrupt_params)<br>
>       unsigned long flags;<br>
>   <br>
>       acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VBLANK);<br>
> -<br>
> -     if (acrtc) {<br>
> -             acrtc_state = to_dm_crtc_state(acrtc->base.state);<br>
> -<br>
> -             DRM_DEBUG_VBL("crtc:%d, vupdate-vrr:%d\n",<br>
> -                           acrtc->crtc_id,<br>
> -                           amdgpu_dm_vrr_active(acrtc_state));<br>
> -<br>
> -             /* Core vblank handling at start of front-porch is only possible<br>
> -              * in non-vrr mode, as only there vblank timestamping will give<br>
> -              * valid results while done in front-porch. Otherwise defer it<br>
> -              * to dm_vupdate_high_irq after end of front-porch.<br>
> -              */<br>
> -             if (!amdgpu_dm_vrr_active(acrtc_state))<br>
> -                     drm_crtc_handle_vblank(&acrtc->base);<br>
> -<br>
> -             /* Following stuff must happen at start of vblank, for crc<br>
> -              * computation and below-the-range btr support in vrr mode.<br>
> -              */<br>
> -             amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);<br>
> -<br>
> -             if (acrtc_state->stream && adev->family >= AMDGPU_FAMILY_AI &&<br>
> -                 acrtc_state->vrr_params.supported &&<br>
> -                 acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE) {<br>
> -                     spin_lock_irqsave(&adev->ddev->event_lock, flags);<br>
> -                     mod_freesync_handle_v_update(<br>
> -                             adev->dm.freesync_module,<br>
> -                             acrtc_state->stream,<br>
> -                             &acrtc_state->vrr_params);<br>
> -<br>
> -                     dc_stream_adjust_vmin_vmax(<br>
> -                             adev->dm.dc,<br>
> -                             acrtc_state->stream,<br>
> -                             &acrtc_state->vrr_params.adjust);<br>
> -                     spin_unlock_irqrestore(&adev->ddev->event_lock, flags);<br>
> -             }<br>
> -     }<br>
> -}<br>
> -<br>
> -#if defined(CONFIG_DRM_AMD_DC_DCN)<br>
> -/**<br>
> - * dm_dcn_crtc_high_irq() - Handles VStartup interrupt for DCN generation ASICs<br>
> - * @interrupt params - interrupt parameters<br>
> - *<br>
> - * Notify DRM's vblank event handler at VSTARTUP<br>
> - *<br>
> - * Unlike DCE hardware, we trigger the handler at VSTARTUP. at which:<br>
> - * * We are close enough to VUPDATE - the point of no return for hw<br>
> - * * We are in the fixed portion of variable front porch when vrr is enabled<br>
> - * * We are before VUPDATE, where double-buffered vrr registers are swapped<br>
> - *<br>
> - * It is therefore the correct place to signal vblank, send user flip events,<br>
> - * and update VRR.<br>
> - */<br>
> -static void dm_dcn_crtc_high_irq(void *interrupt_params)<br>
> -{<br>
> -     struct common_irq_params *irq_params = interrupt_params;<br>
> -     struct amdgpu_device *adev = irq_params->adev;<br>
> -     struct amdgpu_crtc *acrtc;<br>
> -     struct dm_crtc_state *acrtc_state;<br>
> -     unsigned long flags;<br>
> -<br>
> -     acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VBLANK);<br>
> -<br>
>       if (!acrtc)<br>
>               return;<br>
>   <br>
> @@ -528,22 +464,35 @@ static void dm_dcn_crtc_high_irq(void *interrupt_params)<br>
>                        amdgpu_dm_vrr_active(acrtc_state),<br>
>                        acrtc_state->active_planes);<br>
>   <br>
> +     /**<br>
> +      * Core vblank handling at start of front-porch is only possible<br>
> +      * in non-vrr mode, as only there vblank timestamping will give<br>
> +      * valid results while done in front-porch. Otherwise defer it<br>
> +      * to dm_vupdate_high_irq after end of front-porch.<br>
> +      */<br>
> +     if (!amdgpu_dm_vrr_active(acrtc_state))<br>
> +             drm_crtc_handle_vblank(&acrtc->base);<br>
> +<br>
> +     /**<br>
> +      * Following stuff must happen at start of vblank, for crc<br>
> +      * computation and below-the-range btr support in vrr mode.<br>
> +      */<br>
>       amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);<br>
> -     drm_crtc_handle_vblank(&acrtc->base);<br>
> +<br>
> +     /* BTR updates need to happen before VUPDATE on Vega and above. */<br>
> +     if (adev->family < AMDGPU_FAMILY_AI)<br>
> +             return;<br>
>   <br>
>       spin_lock_irqsave(&adev->ddev->event_lock, flags);<br>
>   <br>
> -     if (acrtc_state->vrr_params.supported &&<br>
> +     if (acrtc_state->stream && acrtc_state->vrr_params.supported &&<br>
>           acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE) {<br>
> -             mod_freesync_handle_v_update(<br>
> -             adev->dm.freesync_module,<br>
> -             acrtc_state->stream,<br>
> -             &acrtc_state->vrr_params);<br>
> +             mod_freesync_handle_v_update(adev->dm.freesync_module,<br>
> +                                          acrtc_state->stream,<br>
> +                                          &acrtc_state->vrr_params);<br>
>   <br>
> -             dc_stream_adjust_vmin_vmax(<br>
> -                     adev->dm.dc,<br>
> -                     acrtc_state->stream,<br>
> -                     &acrtc_state->vrr_params.adjust);<br>
> +             dc_stream_adjust_vmin_vmax(adev->dm.dc, acrtc_state->stream,<br>
> +                                        &acrtc_state->vrr_params.adjust);<br>
>       }<br>
>   <br>
>       /*<br>
> @@ -556,7 +505,8 @@ static void dm_dcn_crtc_high_irq(void *interrupt_params)<br>
>        * avoid race conditions between flip programming and completion,<br>
>        * which could cause too early flip completion events.<br>
>        */<br>
> -     if (acrtc->pflip_status == AMDGPU_FLIP_SUBMITTED &&<br>
> +     if (adev->family >= AMDGPU_FAMILY_RV &&<br>
> +         acrtc->pflip_status == AMDGPU_FLIP_SUBMITTED &&<br>
>           acrtc_state->active_planes == 0) {<br>
>               if (acrtc->event) {<br>
>                       drm_crtc_send_vblank_event(&acrtc->base, acrtc->event);<br>
> @@ -568,7 +518,6 @@ static void dm_dcn_crtc_high_irq(void *interrupt_params)<br>
>   <br>
>       spin_unlock_irqrestore(&adev->ddev->event_lock, flags);<br>
>   }<br>
> -#endif<br>
>   <br>
>   static int dm_set_clockgating_state(void *handle,<br>
>                 enum amd_clockgating_state state)<br>
> @@ -2454,8 +2403,36 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)<br>
>               c_irq_params->adev = adev;<br>
>               c_irq_params->irq_src = int_params.irq_source;<br>
>   <br>
> +             amdgpu_dm_irq_register_interrupt(<br>
> +                     adev, &int_params, dm_crtc_high_irq, c_irq_params);<br>
> +     }<br>
> +<br>
> +     /* Use VUPDATE_NO_LOCK interrupt on DCN, which seems to correspond to<br>
> +      * the regular VUPDATE interrupt on DCE. We want DC_IRQ_SOURCE_VUPDATEx<br>
> +      * to trigger at end of each vblank, regardless of state of the lock,<br>
> +      * matching DCE behaviour.<br>
> +      */<br>
> +     for (i = DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT;<br>
> +          i <= DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT + adev->mode_info.num_crtc - 1;<br>
> +          i++) {<br>
> +             r = amdgpu_irq_add_id(adev, SOC15_IH_CLIENTID_DCE, i, &adev->vupdate_irq);<br>
> +<br>
> +             if (r) {<br>
> +                     DRM_ERROR("Failed to add vupdate irq id!\n");<br>
> +                     return r;<br>
> +             }<br>
> +<br>
> +             int_params.int_context = INTERRUPT_HIGH_IRQ_CONTEXT;<br>
> +             int_params.irq_source =<br>
> +                     dc_interrupt_to_irq_source(dc, i, 0);<br>
> +<br>
> +             c_irq_params = &adev->dm.vupdate_params[int_params.irq_source - DC_IRQ_SOURCE_VUPDATE1];<br>
> +<br>
> +             c_irq_params->adev = adev;<br>
> +             c_irq_params->irq_src = int_params.irq_source;<br>
> +<br>
>               amdgpu_dm_irq_register_interrupt(adev, &int_params,<br>
> -                             dm_dcn_crtc_high_irq, c_irq_params);<br>
> +                             dm_vupdate_high_irq, c_irq_params);<br>
>       }<br>
>   <br>
>       /* Use GRPH_PFLIP interrupt */<br>
> @@ -4544,10 +4521,6 @@ static inline int dm_set_vupdate_irq(struct drm_crtc *crtc, bool enable)<br>
>       struct amdgpu_device *adev = crtc->dev->dev_private;<br>
>       int rc;<br>
>   <br>
> -     /* Do not set vupdate for DCN hardware */<br>
> -     if (adev->family > AMDGPU_FAMILY_AI)<br>
> -             return 0;<br>
> -<br>
>       irq_source = IRQ_TYPE_VUPDATE + acrtc->otg_inst;<br>
>   <br>
>       rc = dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;<br>
> <br>
</blockquote></div>