[Intel-gfx] [PATCH 4/5] drm/vblank: Restoring vblank counts after device PM events.

[Pandiyan, Dhinakaran]

On Fri, 2018-01-19 at 00:01 -0800, Rodrigo Vivi wrote:
> On Fri, Jan 12, 2018 at 09:57:06PM +0000, Dhinakaran Pandiyan wrote:
> > The HW frame counter can get reset if device enters a low power state after
> > vblank interrupts were disabled. This messes up any following vblank count
> > update as a negative diff (huge unsigned diff) is calculated from the HW
> > frame counter change. We cannot ignore negative diffs altogther as there
> > could be legitimate wrap arounds. So, allow drivers to update vblank->count
> > with missed vblanks for the time interrupts were disabled. This is similar
> > to _crtc_vblank_on() except that vblanks interrupts are not enabled at the
> > end as this function is expected to be called from the driver
> > _enable_vblank() vfunc.
> > 
> > v2: drm_crtc_vblank_restore should take crtc as arg. (Chris)
> >     Add docs and sprinkle some asserts.
> > 
> > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Michel Dänzer <michel at daenzer.net>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > ---
> >  drivers/gpu/drm/drm_vblank.c | 59 ++++++++++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_vblank.h     |  2 ++
> >  2 files changed, 61 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > index 2559d2d7b907..2690966694f0 100644
> > --- a/drivers/gpu/drm/drm_vblank.c
> > +++ b/drivers/gpu/drm/drm_vblank.c
> > @@ -1237,6 +1237,65 @@ void drm_crtc_vblank_on(struct drm_crtc *crtc)
> >  }
> >  EXPORT_SYMBOL(drm_crtc_vblank_on);
> >  
> > +/**
> > + * drm_vblank_restore - estimated vblanks using timestamps and update it.
> > + *
> > + * Power manamement features can cause frame counter resets between vblank
> > + * disable and enable. Drivers can then use this function in their
> > + * &drm_crtc_funcs.enable_vblank implementation to estimate the vblanks since
> > + * the last &drm_crtc_funcs.disable_vblank.
> > + *
> > + * This function is the legacy version of drm_crtc_vblank_restore().
> > + */
> > +void drm_vblank_restore(struct drm_device *dev, unsigned int pipe)
> > +{
> > +	ktime_t t_vblank;
> > +	struct drm_vblank_crtc *vblank;
> > +	int framedur_ns;
> > +	u64 diff_ns;
> > +	u32 cur_vblank, diff = 1;
> > +	int count = DRM_TIMESTAMP_MAXRETRIES;
> > +
> > +	if (WARN_ON(pipe >= dev->num_crtcs))
> > +		return;
> > +
> > +	assert_spin_locked(&dev->vbl_lock);
> > +	assert_spin_locked(&dev->vblank_time_lock);
> > +
> > +	vblank = &dev->vblank[pipe];
> > +	WARN_ONCE((drm_debug & DRM_UT_VBL) && !vblank->framedur_ns,
> 
> do we really only need this warn on debug vlb?
> 
> > +		  "Cannot compute missed vblanks without frame duration\n");
> 
> The message seems hard... if we *cannot* why do we move fwd?

We assume the diff is 1 and make an update, some kind of a default
similar to what is implemented in drm_update_vblank_count()

> 
> > +	framedur_ns = vblank->framedur_ns;
> > +
> > +	do {
> > +		cur_vblank = __get_vblank_counter(dev, pipe);
> > +		drm_get_last_vbltimestamp(dev, pipe, &t_vblank, false);
> > +	} while (cur_vblank != __get_vblank_counter(dev, pipe) && --count > 0);
> 
> Based on the commend of drm_update_vblank_count I don't feel that we have to


This one? -
"* We repeat the hardware vblank counter & timestamp query until
 * we get consistent results. This to prevent races between gpu
 * updating its hardware counter while we are retrieving the
 * corresponding vblank timestamp." 


I added the loop based on that comment. If the register if partially
updated, we want to discard that and loop until it is stable.


> do the loop here... And if we have maybe we should re-org things to avoid
> duplication?
> 

I considered that, we need to pass at least four args for three lines of
code. Felt it was too small to warrant a separate function.


> > +
> > +	diff_ns = ktime_to_ns(ktime_sub(t_vblank, vblank->time));
> > +	if (framedur_ns)
> > +		diff = DIV_ROUND_CLOSEST_ULL(diff_ns, framedur_ns);
> > +
> > +
> > +	DRM_DEBUG_VBL("missed %d vblanks in %lld ns, frame duration=%d ns, hw_diff=%d\n",
> > +		      diff, diff_ns, framedur_ns, cur_vblank - vblank->last);
> > +	store_vblank(dev, pipe, diff, t_vblank, cur_vblank);
> 
> hm... I wonder if the simple store_vblank(... __get_vblank_counter(dev, pipe));
> wouldn't work here...

We have to store a stable count.

> 
> > +}
> > +EXPORT_SYMBOL(drm_vblank_restore);
> > +
> > +/**
> > + * drm_crtc_vblank_restore - estimate vblanks using timestamps and update it.
> > + * Power manamement features can cause frame counter resets between vblank
> > + * disable and enable. Drivers can then use this function in their
> > + * &drm_crtc_funcs.enable_vblank implementation to estimate the vblanks since
> > + * the last &drm_crtc_funcs.disable_vblank.
> > + */
> > +void drm_crtc_vblank_restore(struct drm_crtc *crtc)
> > +{
> > +	drm_vblank_restore(crtc->dev, drm_crtc_index(crtc));
> > +}
> > +EXPORT_SYMBOL(drm_crtc_vblank_restore);
> > +
> >  static void drm_legacy_vblank_pre_modeset(struct drm_device *dev,
> >  					  unsigned int pipe)
> >  {
> > diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> > index a4c3b0a0a197..16d46e2a6854 100644
> > --- a/include/drm/drm_vblank.h
> > +++ b/include/drm/drm_vblank.h
> > @@ -180,6 +180,8 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc);
> >  void drm_crtc_vblank_reset(struct drm_crtc *crtc);
> >  void drm_crtc_vblank_on(struct drm_crtc *crtc);
> >  u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc);
> > +void drm_vblank_restore(struct drm_device *dev, unsigned int pipe);
> > +void drm_crtc_vblank_restore(struct drm_crtc *crtc);
> >  
> >  bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
> >  					   unsigned int pipe, int *max_error,
> > -- 
> > 2.11.0
> > 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx