[PATCH] drm/vblank: Avoid storing a timestamp for the same frame twice

[Ville Syrjälä]

On Thu, Feb 04, 2021 at 04:32:16PM +0100, Daniel Vetter wrote:
> 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.
> 
> Hm I'm not getting the reason for why we store the updated hw vblank
> counter?

Because next time a vblank irq happens the code will do:
diff = current_hw_counter - vblank->last

which won't work very well if vblank->last is garbage.

Updating vblank->last is pretty much why drm_vblank_restore()
exists at all.

> There's definitely a race when we grab the hw timestamp at a bad time
> (which can't happen for the irq handler, realistically), so maybe we
> should first adjust that to make sure we never store anything inconsistent
> in the vblank state?

Not sure what race you mean, or what inconsistent thing we store?

> 
> And when we have that we should be able to pull the inc == 0 check out
> into _restore(), including comment. Which I think should be cleaner.
> 
> Or I'm totally off with why you want to store the hw vblank counter?
> 
> > 
> > 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 | 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

-- 
Ville Syrjälä
Intel