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

[Daniel Vetter <daniel@ffwll.ch>]

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.

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