Re: [PATCH 11/11] drm: Fix vblank timestamp races

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > 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@linux.intel.com>
> 
> 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@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx