[PATCH] drm/vblank: Avoid storing a timestamp for the same frame twice
Daniel Vetter
daniel at ffwll.ch
Tue Feb 9 10:07:53 UTC 2021
On Thu, Feb 04, 2021 at 04:04:00AM +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 unless we're
> also incrementing the sequence counter. We do still want to update
> vblank->last with the freshly sampled hw frame counter value so
> that subsequent vblank irqs/queries can actually use the hw frame
> counter to determine how many frames have elapsed.
>
> 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>
Ok, top-posting because lol I got confused. I mixed up the guesstimation
work we do for when we don't have a vblank counter with the precise vblank
timestamp stuff.
I think it'd still be good to maybe lock down/document a bit better the
requirements for drm_crtc_vblank_restore, but I convinced myself now that
your patch looks correct.
Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> ---
> drivers/gpu/drm/drm_vblank.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 893165eeddf3..e127a7db2088 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -176,6 +176,17 @@ static void store_vblank(struct drm_device *dev, unsigned int pipe,
>
> vblank->last = last;
>
> + /*
> + * drm_vblank_restore() wants to always update
> + * vblank->last since we can't trust the frame counter
> + * across power saving states. But we don't want to alter
> + * the stored timestamp for the same frame number since
> + * that would cause userspace to potentially observe two
> + * different timestamps for the same frame.
> + */
> + if (vblank_count_inc == 0)
> + return;
> +
> write_seqlock(&vblank->seqlock);
> vblank->time = t_vblank;
> atomic64_add(vblank_count_inc, &vblank->count);
> --
> 2.26.2
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the dri-devel
mailing list