On 3/18/19 1:19 PM, Mario Kleiner wrote:
> We want vblank counts and timestamps of flip completion as sent
> in pageflip completion events to be consistent with the vblank
> count and timestamp of the vblank of flip completion, like in non
> VRR mode.
>
> In VRR mode, drm_update_vblank_count() - and thereby vblank
> count and timestamp updates - must be delayed until after the
> end of front-porch of each vblank, as it is only safe to
> calculate vblank timestamps outside of the front-porch, when
> we actually know when the vblank will end or has ended.
>
> The function drm_update_vblank_count() which updates timestamps
> and counts gets called by drm_crtc_accurate_vblank_count() or by
> drm_crtc_handle_vblank().
>
> Therefore we must make sure that pageflip events for a completed
> flip are only sent out after drm_crtc_accurate_vblank_count() or
> drm_crtc_handle_vblank() is executed, after end of front-porch
> for the vblank of flip completion.
>
> Two cases:
>
> a) Pageflip irq handler executes inside front-porch:
> In this case we must defer sending pageflip events until
> drm_crtc_handle_vblank() executes after end of front-porch,
> and thereby calculates proper vblank count and timestamp.
> Iow. the pflip irq handler must just arm a pageflip event
> to be sent out by drm_crtc_handle_vblank() later on.
>
> b) Pageflip irq handler executes after end of front-porch, e.g.,
> after flip completion in back-porch or due to a massively
> delayed handler invocation into the active scanout of the new
> frame. In this case we can call drm_crtc_accurate_vblank_count()
> to safely force calculation of a proper vblank count and
> timestamp, and must send the pageflip completion event
> ourselves from the pageflip irq handler.
>
> This is the same behaviour as needed for standard fixed refresh
> rate mode.
>
> To decide from within pageflip handler if we are in case a) or b),
> we check the current scanout position against the boundary of
> front-porch. In non-VRR mode we just do what we did in the past.
>
> Signed-off-by: Mario Kleiner <mario.kleiner.de@xxxxxxxxx>
Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@xxxxxxx>
This patch looks fine to me for the most part, but it's still pending on
the other patches in the series.
> ---
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 68 ++++++++++++++++++-----
> 1 file changed, 55 insertions(+), 13 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 555d9e9f..499284d 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -263,6 +263,10 @@ static void dm_pflip_high_irq(void *interrupt_params)
> struct common_irq_params *irq_params = interrupt_params;
> struct amdgpu_device *adev = irq_params->adev;
> unsigned long flags;
> + struct drm_pending_vblank_event *e;
> + struct dm_crtc_state *acrtc_state;
> + uint32_t vpos, hpos, v_blank_start, v_blank_end;
> + bool vrr_active;
>
> amdgpu_crtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_PFLIP);
>
> @@ -285,18 +289,57 @@ static void dm_pflip_high_irq(void *interrupt_params)
> return;
> }
>
> - /* Update to correct count(s) if racing with vblank irq */
> - drm_crtc_accurate_vblank_count(&amdgpu_crtc->base);
> + /* page flip completed. */
> + e = amdgpu_crtc->event;
> + amdgpu_crtc->event = NULL;
>
> - /* wake up userspace */
> - if (amdgpu_crtc->event) {
> - drm_crtc_send_vblank_event(&amdgpu_crtc->base, amdgpu_crtc->event);
> + if (!e)
> + WARN_ON(1);
>
> - /* page flip completed. clean up */
> - amdgpu_crtc->event = NULL;
> + acrtc_state = to_dm_crtc_state(amdgpu_crtc->base.state);
> + vrr_active = amdgpu_dm_vrr_active(acrtc_state);
> +
> + /* Fixed refresh rate, or VRR scanout position outside front-porch? */
> + if (!vrr_active ||
> + !dc_stream_get_scanoutpos(acrtc_state->stream, &v_blank_start,
> + &v_blank_end, &hpos, &vpos) ||
> + (vpos < v_blank_start)) {
> + /* Update to correct count and vblank timestamp if racing with
> + * vblank irq. This also updates to the correct vblank timestamp
> + * even in VRR mode, as scanout is past the front-porch atm.
> + */
> + drm_crtc_accurate_vblank_count(&amdgpu_crtc->base);
>
> - } else
> - WARN_ON(1);
> + /* Wake up userspace by sending the pageflip event with proper
> + * count and timestamp of vblank of flip completion.
> + */
> + if (e) {
> + drm_crtc_send_vblank_event(&amdgpu_crtc->base, e);
> +
> + /* Event sent, so done with vblank for this flip */
> + drm_crtc_vblank_put(&amdgpu_crtc->base);
> + }
> + } else if (e) {
> + /* VRR active and inside front-porch: vblank count and
> + * timestamp for pageflip event will only be up to date after
> + * drm_crtc_handle_vblank() has been executed from late vblank
> + * irq handler after start of back-porch (vline 0). We queue the
> + * pageflip event for send-out by drm_crtc_handle_vblank() with
> + * updated timestamp and count, once it runs after us.
> + *
> + * We need to open-code this instead of using the helper
> + * drm_crtc_arm_vblank_event(), as that helper would
> + * call drm_crtc_accurate_vblank_count(), which we must
> + * not call in VRR mode while we are in front-porch!
> + */
> +
> + /* sequence will be replaced by real count during send-out. */
> + e->sequence = drm_crtc_vblank_count(&amdgpu_crtc->base);
> + e->pipe = amdgpu_crtc->crtc_id;
> +
> + list_add_tail(&e->base.link, &adev->ddev->vblank_event_list);
> + e = NULL;
I guess drm_crtc_vblank_off handles sending any leftover events here if
there are any if the IRQ gets disabled incorrectly.
I wonder why we never just used this list in the first place for
pageflip vblank event handling instead of the locking and NULL set.
Though I suppose it's more useful to use this now for VRR vblank handling.
> + }
>
> /* Keep track of vblank of this flip for flip throttling. We use the
> * cooked hw counter, as that one incremented at start of this vblank
> @@ -309,10 +352,9 @@ static void dm_pflip_high_irq(void *interrupt_params)
> amdgpu_crtc->pflip_status = AMDGPU_FLIP_NONE;
> spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
>
> - DRM_DEBUG_DRIVER("%s - crtc :%d[%p], pflip_stat:AMDGPU_FLIP_NONE\n",
> - __func__, amdgpu_crtc->crtc_id, amdgpu_crtc);
> -
> - drm_crtc_vblank_put(&amdgpu_crtc->base);
> + DRM_DEBUG_DRIVER("crtc:%d[%p], pflip_stat:AMDGPU_FLIP_NONE, vrr[%d]-fp %d\n",
> + amdgpu_crtc->crtc_id, amdgpu_crtc,
> + vrr_active, (int) !e);
The debug output for this is a little strange, but I guess it makes it
makes as much sense as the original did. I'm glad the __func__ is gone
at least.
> }
>
> static void dm_vupdate_high_irq(void *interrupt_params)
>
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
