Re: [PATCH 2/3] drm: Add basic helper to allow precise pageflip timestamps in vrr.

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Feb 11, 2019 at 04:22:24AM +0100, Mario Kleiner wrote:
> The pageflip completion timestamps transmitted to userspace
> via pageflip completion events are supposed to describe the
> time at which the first pixel of the new post-pageflip scanout
> buffer leaves the video output of the gpu. This time is
> identical to end of vblank, when active scanout starts.
> 
> For a crtc in standard fixed refresh rate, the end of vblank
> is identical to the vblank timestamps calculated by
> drm_update_vblank_count() at each vblank interrupt, or each
> vblank dis-/enable. Therefore pageflip events just carry
> that vblank timestamp as their pageflip timestamp.
> 
> For a crtc switched to variable refresh rate mode (vrr), the
> pageflip completion timestamps are identical to the vblank
> timestamps iff the pageflip was executed early in vblank,
> before the minimum vblank duration elapsed. In this case
> the time of display onset is identical to when the crtc
> is running in fixed refresh rate.
> 
> However, if a pageflip completes later in the vblank, inside
> the "extended front porch" in vrr mode, then the vblank will
> terminate at a fixed (back porch) duration after flip, so
> the display onset time is delayed correspondingly. In this
> case the vblank timestamp computed at vblank irq time would
> be too early, and we need a way to calculate an estimated
> pageflip timestamp that will be later than the vblank timestamp.
> 
> How a driver determines such a "late flip" timestamp is hw
> and driver specific, but this patch adds a new helper function
> that allows the driver to propose such an alternate "late flip"
> timestamp for use in pageflip events:
> 
> drm_crtc_set_vrr_pageflip_timestamp(crtc, flip_timestamp);
> 
> When sending out pageflip events, we now compare that proposed
> flip_timestamp against the vblank timestamp of the current
> vblank of flip completion and choose to send out the greater/
> later timestamp as flip completion timestamp.
> 
> The most simple way for a kms driver to supply a suitable
> flip_timestamp in vrr mode would be to simply take a timestamp
> at start of the pageflip completion handler, e.g., pageflip
> irq handler: flip_timestamp = ktime_get(); and then set that
> as proposed "late" alternative timestamp via ...
> drm_crtc_set_vrr_pageflip_timestamp(crtc, flip_timestamp);
> 
> More clever approaches could try to add some corrective offset
> for fixed back porch duration, or ideally use hardware features
> like hw timestamps to calculate the exact end time of vblank.
> 
> Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
> Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>

Uh, this looks like a pretty bad hack. Can't we fix amdgpu to only give us
the right timestampe, once? With this I guess if you do a vblank query in
between the wrong and the right vblank you'll get the bogus value. Not
really great for userspace.
-Daniel

> ---
>  drivers/gpu/drm/drm_vblank.c | 49 +++++++++++++++++++++++++++++++++++-
>  include/drm/drm_vblank.h     |  8 ++++++
>  2 files changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 98e091175921..4b3a4c38fabe 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -814,10 +814,21 @@ static void send_vblank_event(struct drm_device *dev,
>  		u64 seq, ktime_t now)
>  {
>  	struct timespec64 tv;
> +	ktime_t alt_flip_time;
>  
>  	switch (e->event.base.type) {
> -	case DRM_EVENT_VBLANK:
>  	case DRM_EVENT_FLIP_COMPLETE:
> +		/*
> +		 * For flip completion events, override "now" time
> +		 * with alt_flip_time provided by the driver via
> +		 * drm_crtc_set_vrr_pageflip_timestamp() in VRR mode
> +		 * if that time is later than given "now" vblank time.
> +		 */
> +		alt_flip_time = dev->vblank[e->pipe].alt_flip_time;
> +		if (alt_flip_time > now)
> +			now = alt_flip_time;
> +		/* Fallthrough */
> +	case DRM_EVENT_VBLANK:
>  		tv = ktime_to_timespec64(now);
>  		e->event.vbl.sequence = seq;
>  		/*
> @@ -916,11 +927,47 @@ void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
>  
>  		now = ktime_get();
>  	}
> +
>  	e->pipe = pipe;
>  	send_vblank_event(dev, e, seq, now);
>  }
>  EXPORT_SYMBOL(drm_crtc_send_vblank_event);
>  
> +/**
> + * drm_crtc_set_vrr_pageflip_timestamp - helper to set alternate pageflip time
> + * @crtc: the source CRTC of the pageflip completion event
> + * @flip_time: The alternate pageflip completion timestamp in VRR mode
> + *
> + * In variable refresh rate mode (VRR), a pageflip completion timestamp carried
> + * by the pageflip event can never be earlier than the vblank timestamp of the
> + * vblank of flip completion, as that vblank timestamp defines the end of the
> + * shortest possible vblank duration. In case of a delayed flip completion
> + * inside the extended VRR front porch however, the end of vblank can be much
> + * later, so the driver must assign an estimated timestamp of that later end of
> + * vblank. For a CRTC in VRR mode, the driver should use this helper function to
> + * set an alternate flip completion timestamp in case of late flip completions
> + * in extended vblank. In the most simple case, this @flip_time timestamp could
> + * simply be a ktime_get() timestamp taken at the start of the pageflip
> + * completion routine, with some constant duration of the back porch interval
> + * added, although more precise estimates may be possible on some hardware if
> + * the hardware provides some means of timestamping the true end of vblank.
> + *
> + * When sending out pageflip events, e.g., via drm_crtc_send_vblank_event(), it
> + * will use either the standard vblank timestamp, calculated for a minimum
> + * duration vblank, or the provided @flip_time if that time is later than the
> + * vblank timestamp, to get the best possible estimate of start of display of
> + * the new post-pageflip scanout buffer.
> + */
> +void drm_crtc_set_vrr_pageflip_timestamp(struct drm_crtc *crtc,
> +					 ktime_t flip_time)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_vblank_crtc *vblank = &dev->vblank[drm_crtc_index(crtc)];
> +
> +	vblank->alt_flip_time = flip_time;
> +}
> +EXPORT_SYMBOL(drm_crtc_set_vrr_pageflip_timestamp);
> +
>  static int __enable_vblank(struct drm_device *dev, unsigned int pipe)
>  {
>  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> index 6ad9630d4f48..aacf44694ab6 100644
> --- a/include/drm/drm_vblank.h
> +++ b/include/drm/drm_vblank.h
> @@ -117,6 +117,12 @@ struct drm_vblank_crtc {
>  	 * @time: Vblank timestamp corresponding to @count.
>  	 */
>  	ktime_t time;
> +	/**
> +	 * @alt_flip_time: Vblank timestamp for end of extended vblank due to
> +	 * a late pageflip completion in variable refresh rate mode. Pageflip
> +	 * events will carry the later one of @time and @alt_flip_time.
> +	 */
> +	ktime_t alt_flip_time;
>  
>  	/**
>  	 * @refcount: Number of users/waiters of the vblank interrupt. Only when
> @@ -179,6 +185,8 @@ int drm_vblank_init(struct drm_device *dev, unsigned int num_crtcs);
>  u64 drm_crtc_vblank_count(struct drm_crtc *crtc);
>  u64 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc,
>  				   ktime_t *vblanktime);
> +void drm_crtc_set_vrr_pageflip_timestamp(struct drm_crtc *crtc,
> +					 ktime_t flip_time);
>  void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
>  			       struct drm_pending_vblank_event *e);
>  void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
> -- 
> 2.17.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel