<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, May 7, 2020 at 7:35 PM Kazlauskas, Nicholas <<a href="mailto:nicholas.kazlauskas@amd.com">nicholas.kazlauskas@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">
<div>
<div>It applies on top of Alex's
amd-staging-drm-next-branch.</div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>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.<br>
</div>
<div><br>
</div>
<div>All these issues existed before this
patch series at least though.<br>
</div>
<div><br>
</div>
<div>Regards,</div>
<div>Nicholas Kazlauskas<br>
</div>
<div><br></div></div></blockquote><div><br></div><div>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.</div><div><br></div><div>So fwif here's my</div><div><br></div><div>Reviewed-and-Tested-by: Mario Kleiner <<a href="mailto:mario.kleiner.de@gmail.com">mario.kleiner.de@gmail.com</a>></div><div> </div><div>-mario</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div>
</div>
<div>On 2020-05-07 12:58 p.m., Mario Kleiner
wrote:<br>
</div>
<blockquote type="cite">
<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" target="_blank">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>
</blockquote>
<p><br>
</p>
</div>
</blockquote></div></div>