Re: [Intel-gfx] [PATCH v2] drm/vblank: Do not store a new vblank timestamp in drm_vblank_restore()

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	intel-gfx@lists.freedesktop.org,
	Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>,
	dri-devel@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH v2] drm/vblank: Do not store a new vblank timestamp in drm_vblank_restore()
Date: Fri, 19 Feb 2021 17:47:40 +0200	[thread overview]
Message-ID: <YC/dnB479nssyrL8@intel.com> (raw)
In-Reply-To: <YC/UWTfV6tFSwluS@phenom.ffwll.local>

On Fri, Feb 19, 2021 at 04:08:09PM +0100, Daniel Vetter wrote:
> On Thu, Feb 18, 2021 at 06:03:05PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@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 at all,
> > and instead we just fix up the last recorded hw vblank counter
> > value such that the already stored timestamp/seq number will
> > match. Thus the next time a vblank irq happens it will calculate
> > the correct diff between the current and stored hw vblank counter
> > values.
> > 
> > Sidenote: Another possible idea that came to mind would be to
> > do this correction only if the power really was removed since
> > the last time we sampled the hw frame counter. But to do that
> > we would need a robust way to detect when it has occurred. Some
> > possibilities could involve some kind of hardare power well
> > transition counter, or potentially we could store a magic value
> > in a scratch register that lives in the same power well. But
> > I'm not sure either of those exist, so would need an actual
> > investigation to find out. All of that is very hardware specific
> > of course, so would have to be done in the driver code.
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> For testing, there's nothing else than hsw psr that needs this, or that's
> just the box you have locally?

Just the one I happen to have.

Any machine with PSR should be able to hit this. But now that I
refresh my memory I guess HSW/BDW don't actually fully reset the
hw frame counter since they don't have the DC5/6 stuff. But
even on HSW/BDW the frame counter would certainly stop while in
PSR, so maintaining sensible vblank seq numbers will still
require drm_vblank_restore(). Just my further idea of checking
some power well counter/scratch register would not help in cases
where DC states are not used. Instead we'd need some kind of PSR
residency counter/etc.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx