On Tue, Feb 9, 2021 at 4:41 PM Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> 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@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> > > 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
