[PATCH v2] drm/vblank: Do not store a new vblank timestamp in drm_vblank_restore()

[Ville Syrjälä]

On Thu, Feb 18, 2021 at 06:03:05PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> 
> drm_vblank_restore() exists because certain power saving states
> can clobber the hardware frame counter. The way it does this is
> by guesstimating how many frames were missed purely based on
> the difference between the last stored timestamp vs. a newly
> sampled timestamp.
> 
> If we should call this function before a full frame has
> elapsed since we sampled the last timestamp we would end up
> with a possibly slightly different timestamp value for the
> same frame. Currently we will happily overwrite the already
> stored timestamp for the frame with the new value. This
> could cause userspace to observe two different timestamps
> for the same frame (and the timestamp could even go
> backwards depending on how much error we introduce when
> correcting the timestamp based on the scanout position).
> 
> To avoid that let's not update the stored timestamp at all,
> and instead we just fix up the last recorded hw vblank counter
> value such that the already stored timestamp/seq number will
> match. Thus the next time a vblank irq happens it will calculate
> the correct diff between the current and stored hw vblank counter
> values.
> 
> Sidenote: Another possible idea that came to mind would be to
> do this correction only if the power really was removed since
> the last time we sampled the hw frame counter. But to do that
> we would need a robust way to detect when it has occurred. Some
> possibilities could involve some kind of hardare power well
> transition counter, or potentially we could store a magic value
> in a scratch register that lives in the same power well. But
> I'm not sure either of those exist, so would need an actual
> investigation to find out. All of that is very hardware specific
> of course, so would have to be done in the driver code.

Forgot to mention that I wasn't able to test this with PSR
since HSW+PSR1 is bork, but I did test it a bit w/o PSR
by artificially adding arbitrary offsets to the reported
hw frame counter value. The behaviour seemed sane enough
at least.

> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> ---
>  drivers/gpu/drm/drm_vblank.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 2bd989688eae..3417e1ac7918 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -1478,6 +1478,7 @@ static void drm_vblank_restore(struct drm_device *dev, unsigned int pipe)
>  	u64 diff_ns;
>  	u32 cur_vblank, diff = 1;
>  	int count = DRM_TIMESTAMP_MAXRETRIES;
> +	u32 max_vblank_count = drm_max_vblank_count(dev, pipe);
>  
>  	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
>  		return;
> @@ -1504,7 +1505,7 @@ static void drm_vblank_restore(struct drm_device *dev, unsigned int pipe)
>  	drm_dbg_vbl(dev,
>  		    "missed %d vblanks in %lld ns, frame duration=%d ns, hw_diff=%d\n",
>  		    diff, diff_ns, framedur_ns, cur_vblank - vblank->last);
> -	store_vblank(dev, pipe, diff, t_vblank, cur_vblank);
> +	vblank->last = (cur_vblank - diff) & max_vblank_count;
>  }
>  
>  /**
> -- 
> 2.26.2

-- 
Ville Syrjälä
Intel