[PATCH 4/4] drm/amd/display: Make pageflip event delivery compatible with VRR.

[Kazlauskas, Nicholas]

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 at gmail.com>

Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas at amd.com>

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)
>