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@xxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Michel Dänzer <michel@xxxxxxxxxxx>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> ---
> 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
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
