Re: [PATCH] drm/vblank: Fixup and document timestamp update/read barriers

[Peter Hurley <peter@hurleysoftware.com>]

Hi Daniel,

On 04/15/2015 03:17 AM, Daniel Vetter wrote:
> This was a bit too much cargo-culted, so lets make it solid:
> - vblank->count doesn't need to be an atomic, writes are always done
>   under the protection of dev->vblank_time_lock. Switch to an unsigned
>   long instead and update comments. Note that atomic_read is just a
>   normal read of a volatile variable, so no need to audit all the
>   read-side access specifically.
> 
> - The barriers for the vblank counter seqlock weren't complete: The
>   read-side was missing the first barrier between the counter read and
>   the timestamp read, it only had a barrier between the ts and the
>   counter read. We need both.
> 
> - Barriers weren't properly documented. Since barriers only work if
>   you have them on boths sides of the transaction it's prudent to
>   reference where the other side is. To avoid duplicating the
>   write-side comment 3 times extract a little store_vblank() helper.
>   In that helper also assert that we do indeed hold
>   dev->vblank_time_lock, since in some cases the lock is acquired a
>   few functions up in the callchain.
> 
> Spotted while reviewing a patch from Chris Wilson to add a fastpath to
> the vblank_wait ioctl.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mario Kleiner <mario.kleiner.de@gmail.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Michel Dänzer <michel@daenzer.net>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_irq.c | 92 ++++++++++++++++++++++++-----------------------
>  include/drm/drmP.h        |  8 +++--
>  2 files changed, 54 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index c8a34476570a..23bfbc61a494 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -74,6 +74,33 @@ module_param_named(vblankoffdelay, drm_vblank_offdelay, int, 0600);
>  module_param_named(timestamp_precision_usec, drm_timestamp_precision, int, 0600);
>  module_param_named(timestamp_monotonic, drm_timestamp_monotonic, int, 0600);
>  
> +static void store_vblank(struct drm_device *dev, int crtc,
> +			 unsigned vblank_count_inc,
> +			 struct timeval *t_vblank)
> +{
> +	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
> +	u32 tslot;
> +
> +	assert_spin_locked(&dev->vblank_time_lock);
> +
> +	if (t_vblank) {
> +		tslot = vblank->count + vblank_count_inc;
> +		vblanktimestamp(dev, crtc, tslot) = *t_vblank;
> +	}
> +
> +	/*
> +	 * vblank timestamp updates are protected on the write side with
> +	 * vblank_time_lock, but on the read side done locklessly using a
> +	 * sequence-lock on the vblank counter. Ensure correct ordering using
> +	 * memory barrriers. We need the barrier both before and also after the
> +	 * counter update to synchronize with the next timestamp write.
> +	 * The read-side barriers for this are in drm_vblank_count_and_time.
> +	 */
> +	smp_wmb();
> +	vblank->count += vblank_count_inc;
> +	smp_wmb();

The comment and the code are each self-contradictory.

If vblank->count writes are always protected by vblank_time_lock (something I
did not verify but that the comment above asserts), then the trailing write
barrier is not required (and the assertion that it is in the comment is incorrect).

A spin unlock operation is always a write barrier.

Regards,
Peter Hurley

> +}
> +
>  /**
>   * drm_update_vblank_count - update the master vblank counter
>   * @dev: DRM device
> @@ -93,7 +120,7 @@ module_param_named(timestamp_monotonic, drm_timestamp_monotonic, int, 0600);
>  static void drm_update_vblank_count(struct drm_device *dev, int crtc)
>  {
>  	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
> -	u32 cur_vblank, diff, tslot;
> +	u32 cur_vblank, diff;
>  	bool rc;
>  	struct timeval t_vblank;
>  
> @@ -129,18 +156,12 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc)
>  	if (diff == 0)
>  		return;
>  
> -	/* Reinitialize corresponding vblank timestamp if high-precision query
> -	 * available. Skip this step if query unsupported or failed. Will
> -	 * reinitialize delayed at next vblank interrupt in that case.
> +	/*
> +	 * Only reinitialize corresponding vblank timestamp if high-precision query
> +	 * available and didn't fail. Will reinitialize delayed at next vblank
> +	 * interrupt in that case.
>  	 */
> -	if (rc) {
> -		tslot = atomic_read(&vblank->count) + diff;
> -		vblanktimestamp(dev, crtc, tslot) = t_vblank;
> -	}
> -
> -	smp_mb__before_atomic();
> -	atomic_add(diff, &vblank->count);
> -	smp_mb__after_atomic();
> +	store_vblank(dev, crtc, diff, rc ? &t_vblank : NULL);
>  }
>  
>  /*
> @@ -218,7 +239,7 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc)
>  	/* Compute time difference to stored timestamp of last vblank
>  	 * as updated by last invocation of drm_handle_vblank() in vblank irq.
>  	 */
> -	vblcount = atomic_read(&vblank->count);
> +	vblcount = vblank->count;
>  	diff_ns = timeval_to_ns(&tvblank) -
>  		  timeval_to_ns(&vblanktimestamp(dev, crtc, vblcount));
>  
> @@ -234,17 +255,8 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc)
>  	 * available. In that case we can't account for this and just
>  	 * hope for the best.
>  	 */
> -	if (vblrc && (abs64(diff_ns) > 1000000)) {
> -		/* Store new timestamp in ringbuffer. */
> -		vblanktimestamp(dev, crtc, vblcount + 1) = tvblank;
> -
> -		/* Increment cooked vblank count. This also atomically commits
> -		 * the timestamp computed above.
> -		 */
> -		smp_mb__before_atomic();
> -		atomic_inc(&vblank->count);
> -		smp_mb__after_atomic();
> -	}
> +	if (vblrc && (abs64(diff_ns) > 1000000))
> +		store_vblank(dev, crtc, 1, &tvblank);
>  
>  	spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);
>  }
> @@ -852,7 +864,7 @@ u32 drm_vblank_count(struct drm_device *dev, int crtc)
>  
>  	if (WARN_ON(crtc >= dev->num_crtcs))
>  		return 0;
> -	return atomic_read(&vblank->count);
> +	return vblank->count;
>  }
>  EXPORT_SYMBOL(drm_vblank_count);
>  
> @@ -897,16 +909,17 @@ u32 drm_vblank_count_and_time(struct drm_device *dev, int crtc,
>  	if (WARN_ON(crtc >= dev->num_crtcs))
>  		return 0;
>  
> -	/* Read timestamp from slot of _vblank_time ringbuffer
> -	 * that corresponds to current vblank count. Retry if
> -	 * count has incremented during readout. This works like
> -	 * a seqlock.
> +	/*
> +	 * Vblank timestamps are read lockless. To ensure consistency the vblank
> +	 * counter is rechecked and ordering is ensured using memory barriers.
> +	 * This works like a seqlock. The write-side barriers are in store_vblank.
>  	 */
>  	do {
> -		cur_vblank = atomic_read(&vblank->count);
> +		cur_vblank = vblank->count;
> +		smp_rmb();
>  		*vblanktime = vblanktimestamp(dev, crtc, cur_vblank);
>  		smp_rmb();
> -	} while (cur_vblank != atomic_read(&vblank->count));
> +	} while (cur_vblank != vblank->count);
>  
>  	return cur_vblank;
>  }
> @@ -1715,7 +1728,7 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc)
>  	 */
>  
>  	/* Get current timestamp and count. */
> -	vblcount = atomic_read(&vblank->count);
> +	vblcount = vblank->count;
>  	drm_get_last_vbltimestamp(dev, crtc, &tvblank, DRM_CALLED_FROM_VBLIRQ);
>  
>  	/* Compute time difference to timestamp of last vblank */
> @@ -1731,20 +1744,11 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc)
>  	 * e.g., due to spurious vblank interrupts. We need to
>  	 * ignore those for accounting.
>  	 */
> -	if (abs64(diff_ns) > DRM_REDUNDANT_VBLIRQ_THRESH_NS) {
> -		/* Store new timestamp in ringbuffer. */
> -		vblanktimestamp(dev, crtc, vblcount + 1) = tvblank;
> -
> -		/* Increment cooked vblank count. This also atomically commits
> -		 * the timestamp computed above.
> -		 */
> -		smp_mb__before_atomic();
> -		atomic_inc(&vblank->count);
> -		smp_mb__after_atomic();
> -	} else {
> +	if (abs64(diff_ns) > DRM_REDUNDANT_VBLIRQ_THRESH_NS)
> +		store_vblank(dev, crtc, 1, &tvblank);
> +	else
>  		DRM_DEBUG("crtc %d: Redundant vblirq ignored. diff_ns = %d\n",
>  			  crtc, (int) diff_ns);
> -	}
>  
>  	spin_unlock(&dev->vblank_time_lock);
>  
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 62c40777c009..4c31a2cc5a33 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -686,9 +686,13 @@ struct drm_pending_vblank_event {
>  struct drm_vblank_crtc {
>  	struct drm_device *dev;		/* pointer to the drm_device */
>  	wait_queue_head_t queue;	/**< VBLANK wait queue */
> -	struct timeval time[DRM_VBLANKTIME_RBSIZE];	/**< timestamp of current count */
>  	struct timer_list disable_timer;		/* delayed disable timer */
> -	atomic_t count;			/**< number of VBLANK interrupts */
> +
> +	/* vblank counter, protected by dev->vblank_time_lock for writes */
> +	unsigned long count;
> +	/* vblank timestamps, protected by dev->vblank_time_lock for writes */
> +	struct timeval time[DRM_VBLANKTIME_RBSIZE];
> +
>  	atomic_t refcount;		/* number of users of vblank interruptsper crtc */
>  	u32 last;			/* protected by dev->vbl_lock, used */
>  					/* for wraparound handling */
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel