[11/11] drm: Fix vblank timestamp races

Commit Message

Ville Syrjälä Sept. 14, 2015, 7:43 p.m. UTC

From: Ville Syrjälä <[e-mail]>

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ä <[e-mail]>
---
 drivers/gpu/drm/drm_irq.c | 40 ++++++++++++++++++++++++----------------
 include/drm/drmP.h        | 12 ++++++------
 2 files changed, 30 insertions(+), 22 deletions(-)

Comments

Daniel Vetter Sept. 22, 2015, 9:10 a.m. UTC | #1

On Mon, Sep 14, 2015 at 10:43:52PM +0300, [e-mail] wrote:
> From: Ville Syrjälä <[e-mail]>
> 
> 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ä <[e-mail]>

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.

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
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Maarten Lankhorst Sept. 22, 2015, 11:15 a.m. UTC | #2

Op 22-09-15 om 11:10 schreef Daniel Vetter:
> On Mon, Sep 14, 2015 at 10:43:52PM +0300, [e-mail] wrote:
>> From: Ville Syrjälä <[e-mail]>
>>
>> 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ä <[e-mail]>
> 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.
>
> Series looks good otherwise, I'll wait for Maarten to r-b it and then pull
> it in.
>
R-b for 1-10.

Daniel Vetter Sept. 22, 2015, 11:31 a.m. UTC | #3

On Tue, Sep 22, 2015 at 01:15:01PM +0200, Maarten Lankhorst wrote:
> Op 22-09-15 om 11:10 schreef Daniel Vetter:
> > On Mon, Sep 14, 2015 at 10:43:52PM +0300, [e-mail] wrote:
> >> From: Ville Syrjälä <[e-mail]>
> >>
> >> 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ä <[e-mail]>
> > 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.
> >
> > Series looks good otherwise, I'll wait for Maarten to r-b it and then pull
> > it in.
> >
> R-b for 1-10.

Merged to drm-misc, thanks.
-Daniel

Ville Syrjälä Sept. 22, 2015, 12:36 p.m. UTC | #4

On Tue, Sep 22, 2015 at 11:10:50AM +0200, Daniel Vetter wrote:
> On Mon, Sep 14, 2015 at 10:43:52PM +0300, [e-mail] wrote:
> > From: Ville Syrjälä <[e-mail]>
> > 
> > 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ä <[e-mail]>
> 
> 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
> > [e-mail]
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Daniel Vetter Sept. 22, 2015, 12:57 p.m. UTC | #5

On Tue, Sep 22, 2015 at 03:36:44PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 22, 2015 at 11:10:50AM +0200, Daniel Vetter wrote:
> > On Mon, Sep 14, 2015 at 10:43:52PM +0300, [e-mail] wrote:
> > > From: Ville Syrjälä <[e-mail]>
> > > 
> > > 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ä <[e-mail]>
> > 
> > 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.

I guess we can keep it as a low-hanging thing for now, didn't seem to have
annoyed anyone yet. It should be fairly simple since all the vblank
counter access goes through the 2 functions I created a while back while
polishing the barriers a bit.
-Daniel

Patch

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 */