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

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

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?

> +	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
do the loop here... And if we have maybe we should re-org things to avoid
duplication?

> +
> +	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...

> +}
> +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