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@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?
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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
