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

[Daniel Vetter]

On Tue, Feb 9, 2021 at 4:41 PM Ville Syrjälä
<ville.syrjala at linux.intel.com> wrote:
> On Tue, Feb 09, 2021 at 11:07:53AM +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.
> > >
> > > 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>
>
> Ta.
>
> Though I wonder if we should just do something like this instead:
> -       store_vblank(dev, pipe, diff, t_vblank, cur_vblank);
> +       vblank->last = (cur_vblank - diff) & max_vblank_count;
>
> to make it entirely obvious that this exists only to fix up
> the stored hw counter value?
>
> Would also avoid the problem the original patch tries to fix
> because we'd simply never store a new timestamp here.

Hm yeah, I think that would nicely limit the impact. But need to check
overflow/underflow math is all correct. And I think that would neatly
implement the trick I proposed to address the bug that wasn't there
:-)

The only thing that I've thought of as issue is that we might have
more wrap-around of the hw vblank counter, but that shouldn't be worse
than without this - anytime we have the vblank on for long enough we
fix the entire thing, and I think our wrap handling is now consistent
enough (there was some "let's just add a large bump" stuff for dri1
userspace iirc) that this shouldn't be any problem.

Plus the comment about _restore being very special would be in the
restore function, so this would also be rather tidy. If you go with
this maybe extend the kerneldoc for ->last to mention that
drm_vblank_restore() adjusts it?

The more I ponder this, the more I like it ... which probably means
I'm missing something, because this is drm_vblank.c?

Cheers, Daniel

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



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch