On Tue, Sep 22, 2015 at 11:10:50AM +0200, Daniel Vetter wrote:
> On Mon, Sep 14, 2015 at 10:43:52PM +0300, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > The vblank timestamp ringbuffer only has two entries, so if the
> > vblank->count is incremented by an even number readers may end up seeing
> > the new vblank timestamp alongside the old vblank counter value.
> >
> > Fix the problem by storing the vblank counter in a ringbuffer as well,
> > and always increment the ringbuffer "slot" by one when storing a new
> > timestamp+counter pair.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> Imo if we bother with this we might as well just switch over to using
> full-blown seqlocks. They internally use a two-stage update which means
> race-free even with just one copy of the data we protect. Also more
> standardized to boot.
Any volunteers for that? I don't have time to start redoing this right
now.
>
> Series looks good otherwise, I'll wait for Maarten to r-b it and then pull
> it in.
> -Daniel
>
> > ---
> > drivers/gpu/drm/drm_irq.c | 40 ++++++++++++++++++++++++----------------
> > include/drm/drmP.h | 12 ++++++------
> > 2 files changed, 30 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > index 88fbee4..8de236a 100644
> > --- a/drivers/gpu/drm/drm_irq.c
> > +++ b/drivers/gpu/drm/drm_irq.c
> > @@ -43,8 +43,10 @@
> > #include <linux/export.h>
> >
> > /* Access macro for slots in vblank timestamp ringbuffer. */
> > -#define vblanktimestamp(dev, pipe, count) \
> > - ((dev)->vblank[pipe].time[(count) % DRM_VBLANKTIME_RBSIZE])
> > +#define vblanktimestamp(dev, pipe, slot) \
> > + ((dev)->vblank[pipe].time[(slot) % DRM_VBLANK_RBSIZE])
> > +#define vblankcount(dev, pipe, slot) \
> > + ((dev)->vblank[pipe].count[(slot) % DRM_VBLANK_RBSIZE])
> >
> > /* Retry timestamp calculation up to 3 times to satisfy
> > * drm_timestamp_precision before giving up.
> > @@ -76,20 +78,23 @@ module_param_named(timestamp_monotonic, drm_timestamp_monotonic, int, 0600);
> >
> > static void store_vblank(struct drm_device *dev, unsigned int pipe,
> > u32 vblank_count_inc,
> > - struct timeval *t_vblank, u32 last)
> > + const struct timeval *t_vblank, u32 last)
> > {
> > struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> > - u32 tslot;
> > + u32 slot;
> >
> > assert_spin_locked(&dev->vblank_time_lock);
> >
> > + slot = vblank->slot + 1;
> > +
> > vblank->last = last;
> >
> > /* All writers hold the spinlock, but readers are serialized by
> > - * the latching of vblank->count below.
> > + * the latching of vblank->slot below.
> > */
> > - tslot = vblank->count + vblank_count_inc;
> > - vblanktimestamp(dev, pipe, tslot) = *t_vblank;
> > + vblankcount(dev, pipe, slot) =
> > + vblankcount(dev, pipe, vblank->slot) + vblank_count_inc;
> > + vblanktimestamp(dev, pipe, slot) = *t_vblank;
> >
> > /*
> > * vblank timestamp updates are protected on the write side with
> > @@ -100,7 +105,7 @@ static void store_vblank(struct drm_device *dev, unsigned int pipe,
> > * The read-side barriers for this are in drm_vblank_count_and_time.
> > */
> > smp_wmb();
> > - vblank->count += vblank_count_inc;
> > + vblank->slot = slot;
> > smp_wmb();
> > }
> >
> > @@ -202,7 +207,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> > const struct timeval *t_old;
> > u64 diff_ns;
> >
> > - t_old = &vblanktimestamp(dev, pipe, vblank->count);
> > + t_old = &vblanktimestamp(dev, pipe, vblank->slot);
> > diff_ns = timeval_to_ns(&t_vblank) - timeval_to_ns(t_old);
> >
> > /*
> > @@ -223,7 +228,8 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> >
> > DRM_DEBUG("updating vblank count on crtc %u:"
> > " current=%u, diff=%u, hw=%u hw_last=%u\n",
> > - pipe, vblank->count, diff, cur_vblank, vblank->last);
> > + pipe, vblankcount(dev, pipe, vblank->slot),
> > + diff, cur_vblank, vblank->last);
> >
> > if (diff == 0) {
> > WARN_ON_ONCE(cur_vblank != vblank->last);
> > @@ -883,7 +889,7 @@ u32 drm_vblank_count(struct drm_device *dev, int pipe)
> > if (WARN_ON(pipe >= dev->num_crtcs))
> > return 0;
> >
> > - return vblank->count;
> > + return vblankcount(dev, pipe, vblank->slot);
> > }
> > EXPORT_SYMBOL(drm_vblank_count);
> >
> > @@ -923,7 +929,8 @@ u32 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe,
> > {
> > struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> > int count = DRM_TIMESTAMP_MAXRETRIES;
> > - u32 cur_vblank;
> > + u32 vblankcount;
> > + u32 slot;
> >
> > if (WARN_ON(pipe >= dev->num_crtcs))
> > return 0;
> > @@ -934,13 +941,14 @@ u32 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe,
> > * This works like a seqlock. The write-side barriers are in store_vblank.
> > */
> > do {
> > - cur_vblank = vblank->count;
> > + slot = vblank->slot;
> > smp_rmb();
> > - *vblanktime = vblanktimestamp(dev, pipe, cur_vblank);
> > + *vblanktime = vblanktimestamp(dev, pipe, slot);
> > + vblankcount = vblankcount(dev, pipe, slot);
> > smp_rmb();
> > - } while (cur_vblank != vblank->count && --count > 0);
> > + } while (slot != vblank->slot && --count > 0);
> >
> > - return cur_vblank;
> > + return vblankcount;
> > }
> > EXPORT_SYMBOL(drm_vblank_count_and_time);
> >
> > diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> > index 6717a7d..9de9fde 100644
> > --- a/include/drm/drmP.h
> > +++ b/include/drm/drmP.h
> > @@ -374,10 +374,10 @@ struct drm_master {
> > void *driver_priv;
> > };
> >
> > -/* Size of ringbuffer for vblank timestamps. Just double-buffer
> > +/* Size of ringbuffer for vblank counts/timestamps. Just double-buffer
> > * in initial implementation.
> > */
> > -#define DRM_VBLANKTIME_RBSIZE 2
> > +#define DRM_VBLANK_RBSIZE 2
> >
> > /* Flags and return codes for get_vblank_timestamp() driver function. */
> > #define DRM_CALLED_FROM_VBLIRQ 1
> > @@ -692,10 +692,10 @@ struct drm_vblank_crtc {
> > wait_queue_head_t queue; /**< VBLANK wait queue */
> > struct timer_list disable_timer; /* delayed disable timer */
> >
> > - /* vblank counter, protected by dev->vblank_time_lock for writes */
> > - u32 count;
> > - /* vblank timestamps, protected by dev->vblank_time_lock for writes */
> > - struct timeval time[DRM_VBLANKTIME_RBSIZE];
> > + u32 slot;
> > + /* vblank timestamps and counter, protected by dev->vblank_time_lock for writes */
> > + struct timeval time[DRM_VBLANK_RBSIZE];
> > + u32 count[DRM_VBLANK_RBSIZE];
> >
> > atomic_t refcount; /* number of users of vblank interruptsper crtc */
> > u32 last; /* protected by dev->vbl_lock, used */
> > --
> > 2.4.6
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
