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@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. > > 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@xxxxxxxxx> > > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx> > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > --- > > 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
