[PATCH] drm/amd/display: Fix vblank and pageflip event handling for FreeSync
Mario Kleiner
mario.kleiner.de at gmail.com
Fri May 8 20:03:27 UTC 2020
On Thu, May 7, 2020 at 7:35 PM Kazlauskas, Nicholas <
nicholas.kazlauskas at 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.de at 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.li at 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.li at 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.li at amd.com>
>> > Cc: Mario Kleiner <mario.kleiner.de at gmail.com>
>> > Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas at 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;
>> >
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20200508/663f9e0f/attachment-0001.htm>
More information about the amd-gfx
mailing list