On Thu, Feb 04, 2021 at 04:04:00AM +0200, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > 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@xxxxxxxxx> > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> 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@xxxxxxxx> > --- > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
