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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Jan 19, 2018 at 10:02:12PM +0000, Pandiyan, Dhinakaran wrote:
> 
> 
> 
> 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@ffwll.ch>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Michel Dänzer <michel@daenzer.net>
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@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.

I don't see why our counter wouldn't be stable at that point?

Our register is zero or the proper counter. Our frm_counter reg
doesn't stop when vblank interrupts are disabled afaik...

But well, I see your point of making something more robust, using
the well known and tested methods and preparing for the case
where someone start using this call in other platforms as well....
So, whatever you decide..

If you go wit this:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> 
> > 
> > > +}
> > > +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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel