All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/vblank: Fixup and document timestamp update/read barriers
@ 2015-04-15  7:17 Daniel Vetter
  2015-04-15  8:17 ` Chris Wilson
                   ` (3 more replies)
  0 siblings, 4 replies; 31+ messages in thread
From: Daniel Vetter @ 2015-04-15  7:17 UTC (permalink / raw)
  To: Intel Graphics Development, DRI Development
  Cc: Daniel Vetter, Michel Dänzer, Daniel Vetter

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();
+}
+
 /**
  * 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 */
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH] drm/vblank: Fixup and document timestamp update/read barriers
  2015-04-15  7:17 [PATCH] drm/vblank: Fixup and document timestamp update/read barriers Daniel Vetter
@ 2015-04-15  8:17 ` Chris Wilson
  2015-04-15  9:25   ` Daniel Vetter
  2015-04-15 10:32 ` Daniel Vetter
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 31+ messages in thread
From: Chris Wilson @ 2015-04-15  8:17 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Michel Dänzer, DRI Development, Daniel Vetter,
	Intel Graphics Development

On Wed, Apr 15, 2015 at 09:17:02AM +0200, Daniel Vetter wrote:
> 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;
> +	}

It is not obvious this updates the right tslot in all circumstances.
Care to explain?

Otherwise the rest looks consistent with seqlock, using the
vblank->count as the latch.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] drm/vblank: Fixup and document timestamp update/read barriers
  2015-04-15  8:17 ` Chris Wilson
@ 2015-04-15  9:25   ` Daniel Vetter
  2015-04-15  9:36     ` Chris Wilson
  0 siblings, 1 reply; 31+ messages in thread
From: Daniel Vetter @ 2015-04-15  9:25 UTC (permalink / raw)
  To: Chris Wilson, Daniel Vetter, Intel Graphics Development,
	DRI Development, Mario Kleiner, Ville Syrjälä,
	Michel Dänzer, Daniel Vetter

On Wed, Apr 15, 2015 at 09:17:03AM +0100, Chris Wilson wrote:
> On Wed, Apr 15, 2015 at 09:17:02AM +0200, Daniel Vetter wrote:
> > 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;
> > +	}
> 
> It is not obvious this updates the right tslot in all circumstances.
> Care to explain?

Writers are synchronized with vblank_time_lock, so there shouldn't be any
races. Mario also has a patch to clear the ts slot if we don't have
anything to set it too (that one will conflict ofc).

Or what exactly do you mean?
-Daniel
-- 
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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] drm/vblank: Fixup and document timestamp update/read barriers
  2015-04-15  9:25   ` Daniel Vetter
@ 2015-04-15  9:36     ` Chris Wilson
  0 siblings, 0 replies; 31+ messages in thread
From: Chris Wilson @ 2015-04-15  9:36 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Daniel Vetter, Intel Graphics Development, DRI Development,
	Daniel Vetter, Michel Dänzer

On Wed, Apr 15, 2015 at 11:25:00AM +0200, Daniel Vetter wrote:
> On Wed, Apr 15, 2015 at 09:17:03AM +0100, Chris Wilson wrote:
> > On Wed, Apr 15, 2015 at 09:17:02AM +0200, Daniel Vetter wrote:
> > > 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;
> > > +	}
> > 
> > It is not obvious this updates the right tslot in all circumstances.
> > Care to explain?
> 
> Writers are synchronized with vblank_time_lock, so there shouldn't be any
> races. Mario also has a patch to clear the ts slot if we don't have
> anything to set it too (that one will conflict ofc).
> 
> Or what exactly do you mean?

I was staring at vblank->count and reading backwards from the smp_wmb().

Just something like:
if (t_vblank) {
	/* All writers hold the spinlock, but readers are serialized by
	 * the latching of vblank->count below.
	 */
	 u32 tslot = vblank->count + vblank_count_inc;
	 ...

would help me understand the relationship better.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH] drm/vblank: Fixup and document timestamp update/read barriers
  2015-04-15  7:17 [PATCH] drm/vblank: Fixup and document timestamp update/read barriers Daniel Vetter
  2015-04-15  8:17 ` Chris Wilson
@ 2015-04-15 10:32 ` Daniel Vetter
  2015-04-15 17:34   ` Daniel Vetter
  2015-04-16  0:18   ` shuang.he
  2015-04-15 13:00 ` Peter Hurley
  2015-04-15 19:40 ` shuang.he
  3 siblings, 2 replies; 31+ messages in thread
From: Daniel Vetter @ 2015-04-15 10:32 UTC (permalink / raw)
  To: Intel Graphics Development, DRI Development
  Cc: Daniel Vetter, Michel Dänzer, Daniel Vetter

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.

v2: Add comment to better explain how store_vblank works, suggested by
Chris.

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 | 95 +++++++++++++++++++++++++----------------------
 include/drm/drmP.h        |  8 +++-
 2 files changed, 57 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index c8a34476570a..8694b77d0002 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -74,6 +74,36 @@ 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) {
+		/* All writers hold the spinlock, but readers are serialized by
+		 * the latching of vblank->count below.
+		 */
+		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();
+}
+
 /**
  * drm_update_vblank_count - update the master vblank counter
  * @dev: DRM device
@@ -93,7 +123,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 +159,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 +242,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 +258,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 +867,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 +912,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 +1731,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 +1747,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 */
-- 
2.1.4

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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH] drm/vblank: Fixup and document timestamp update/read barriers
  2015-04-15  7:17 [PATCH] drm/vblank: Fixup and document timestamp update/read barriers Daniel Vetter
  2015-04-15  8:17 ` Chris Wilson
  2015-04-15 10:32 ` Daniel Vetter
@ 2015-04-15 13:00 ` Peter Hurley
  2015-04-15 17:31   ` Daniel Vetter
  2015-04-15 19:40 ` shuang.he
  3 siblings, 1 reply; 31+ messages in thread
From: Peter Hurley @ 2015-04-15 13:00 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Daniel Vetter, Intel Graphics Development, Michel Dänzer,
	DRI Development

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] drm/vblank: Fixup and document timestamp update/read barriers
  2015-04-15 13:00 ` Peter Hurley
@ 2015-04-15 17:31   ` Daniel Vetter
  2015-04-16 12:30     ` Peter Hurley
  0 siblings, 1 reply; 31+ messages in thread
From: Daniel Vetter @ 2015-04-15 17:31 UTC (permalink / raw)
  To: Peter Hurley
  Cc: Daniel Vetter, Intel Graphics Development, Michel Dänzer,
	DRI Development, Daniel Vetter

On Wed, Apr 15, 2015 at 09:00:04AM -0400, Peter Hurley wrote:
> 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.

Hm yeah. Otoh to me that's bordering on "code too clever for my own good".
That the spinlock is held I can assure. That no one goes around and does
multiple vblank updates (because somehow that code raced with the hw
itself) I can't easily assure with a simple assert or something similar.
It's not the case right now, but that can changes.

Also it's not contradictory here, since you'd need to audit all the
callers to be able to make the claim that the 2nd smp_wmb() is redundant.
I'll just add a comment about this.
-Daniel
-- 
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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH] drm/vblank: Fixup and document timestamp update/read barriers
  2015-04-15 10:32 ` Daniel Vetter
@ 2015-04-15 17:34   ` Daniel Vetter
  2015-04-15 17:49     ` Chris Wilson
                       ` (3 more replies)
  2015-04-16  0:18   ` shuang.he
  1 sibling, 4 replies; 31+ messages in thread
From: Daniel Vetter @ 2015-04-15 17:34 UTC (permalink / raw)
  To: Intel Graphics Development, DRI Development
  Cc: Peter Hurley, Daniel Vetter, Michel Dänzer, Daniel Vetter

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.

v2: Add comment to better explain how store_vblank works, suggested by
Chris.

v3: Peter noticed that as-is the 2nd smp_wmb is redundant with the
implicit barrier in the spin_unlock. But that can only be proven by
auditing all callers and my point in extracting this little helper was
to localize all the locking into just one place. Hence I think that
additional optimization is too risky.

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>
Cc: Peter Hurley <peter@hurleysoftware.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_irq.c | 95 +++++++++++++++++++++++++----------------------
 include/drm/drmP.h        |  8 +++-
 2 files changed, 57 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index c8a34476570a..8694b77d0002 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -74,6 +74,36 @@ 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) {
+		/* All writers hold the spinlock, but readers are serialized by
+		 * the latching of vblank->count below.
+		 */
+		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();
+}
+
 /**
  * drm_update_vblank_count - update the master vblank counter
  * @dev: DRM device
@@ -93,7 +123,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 +159,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 +242,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 +258,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 +867,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 +912,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 +1731,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 +1747,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 */
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH] drm/vblank: Fixup and document timestamp update/read barriers
  2015-04-15 17:34   ` Daniel Vetter
@ 2015-04-15 17:49     ` Chris Wilson
  2015-04-15 21:26     ` Mario Kleiner
                       ` (2 subsequent siblings)
  3 siblings, 0 replies; 31+ messages in thread
From: Chris Wilson @ 2015-04-15 17:49 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Peter Hurley, Michel Dänzer, DRI Development, Daniel Vetter,
	Intel Graphics Development

On Wed, Apr 15, 2015 at 07:34:43PM +0200, 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.
> 
> v2: Add comment to better explain how store_vblank works, suggested by
> Chris.
> 
> v3: Peter noticed that as-is the 2nd smp_wmb is redundant with the
> implicit barrier in the spin_unlock. But that can only be proven by
> auditing all callers and my point in extracting this little helper was
> to localize all the locking into just one place. Hence I think that
> additional optimization is too risky.
> 
> 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>
> Cc: Peter Hurley <peter@hurleysoftware.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Fwiw, there was no discernible difference in the time to query the
vblank counter (on an ivb i7-3720QM).

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] drm/vblank: Fixup and document timestamp update/read barriers
  2015-04-15  7:17 [PATCH] drm/vblank: Fixup and document timestamp update/read barriers Daniel Vetter
                   ` (2 preceding siblings ...)
  2015-04-15 13:00 ` Peter Hurley
@ 2015-04-15 19:40 ` shuang.he
  3 siblings, 0 replies; 31+ messages in thread
From: shuang.he @ 2015-04-15 19:40 UTC (permalink / raw)
  To: shuang.he, ethan.gao, intel-gfx, daniel.vetter

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6195
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                 -1              302/302              301/302
SNB                                  318/318              318/318
IVB                                  341/341              341/341
BYT                                  287/287              287/287
HSW                                  395/395              395/395
BDW                                  318/318              318/318
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt@gem_fenced_exec_thrash@no-spare-fences-busy-interruptible      PASS(2)      DMESG_WARN(1)PASS(1)
(dmesg patch applied)drm:i915_hangcheck_elapsed[i915]]*ERROR*Hangcheck_timer_elapsed...bsd_ring_idle@Hangcheck timer elapsed... bsd ring idle
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] drm/vblank: Fixup and document timestamp update/read barriers
  2015-04-15 17:34   ` Daniel Vetter
  2015-04-15 17:49     ` Chris Wilson
@ 2015-04-15 21:26     ` Mario Kleiner
  2015-04-16  1:29       ` Peter Hurley
  2015-04-16  8:54       ` Daniel Vetter
  2015-04-16  0:17     ` shuang.he
  2015-04-16  8:59     ` Daniel Vetter
  3 siblings, 2 replies; 31+ messages in thread
From: Mario Kleiner @ 2015-04-15 21:26 UTC (permalink / raw)
  To: Daniel Vetter, Intel Graphics Development, DRI Development
  Cc: Daniel Vetter, Michel Dänzer, Peter Hurley

A couple of questions to educate me and one review comment.

On 04/15/2015 07:34 PM, 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.
>
> v2: Add comment to better explain how store_vblank works, suggested by
> Chris.
>
> v3: Peter noticed that as-is the 2nd smp_wmb is redundant with the
> implicit barrier in the spin_unlock. But that can only be proven by
> auditing all callers and my point in extracting this little helper was
> to localize all the locking into just one place. Hence I think that
> additional optimization is too risky.
>
> 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>
> Cc: Peter Hurley <peter@hurleysoftware.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>   drivers/gpu/drm/drm_irq.c | 95 +++++++++++++++++++++++++----------------------
>   include/drm/drmP.h        |  8 +++-
>   2 files changed, 57 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index c8a34476570a..8694b77d0002 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -74,6 +74,36 @@ 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) {
> +		/* All writers hold the spinlock, but readers are serialized by
> +		 * the latching of vblank->count below.
> +		 */
> +		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();
> +}
> +
>   /**
>    * drm_update_vblank_count - update the master vblank counter
>    * @dev: DRM device
> @@ -93,7 +123,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 +159,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 +242,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 +258,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 +867,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;

I wrongly assumed atomic_read would guarantee more than it actually 
does, so please help me to learn something. Why don't we need some 
smp_rmb() here before returning vblank->count? What guarantees that 
drm_vblank_count() does return the latest value assigned to 
vblank->count in store_vblank()? In store_vblank() there is a smp_wmb(), 
but why don't we need a matching smp_rmb() here to benefit from it?

>   }
>   EXPORT_SYMBOL(drm_vblank_count);
>
> @@ -897,16 +912,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);
>

Similar question as above. We have a new smp_rmb() after the cur_vblank 
assignment and then after *vblanktime assignment. My original wrong 
assumption was that the first smp_rmb() wouldn't be needed because 
atomic_read() would imply that, and that the compiler/cpu couldn't 
reorder anything here because the *vblanktime assignment depends on 
cur_vblank from the preceeding atomic_read.

But why can we now do the comparison while(cur_vblank != vblank->count) 
without needing something like

	new_vblank = vblank->count;
	smp_rmb();
    } while (cur_vblank != new_vblank);

to make sure the value from the 2nd vblank->count read isn't stale wrt. 
to potential updates from store_vblank()?

Another question is why the drm_vblank_count_and_time() code ever worked 
without triggering any of my own tests and consistency checks in my 
software, or any of your igt tests? I run my tests very often, but only 
on Intel architecture cpus. I assume the same is true for the igt tests? 
Is there anything specific about Intel cpu's that makes this still work 
or very unlikely to break? Or are the tests insufficient to catch this? 
Or just luck?

I looked through kernels back to 3.16 and most uses of the function 
would be safe from races due to the locking around it, holding of vblank 
refcounts, or the place and order of execution, e.g., from within 
drm_handle_vblank(). But in some tight test loop just calling the 
drmWaitVblank ioctl to query current values i'd expect it to at least 
occassionally return corrupted timestamps, e.g., time jumping forward or 
backward, etc.?

>   	return cur_vblank;
>   }
> @@ -1715,7 +1731,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 +1747,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;

Why is count an unsigned long (= 64 bit on 64-bit kernels) instead of 
u32 when all users of count are u32? Is this intentional?


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

Thanks,
-mario
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] drm/vblank: Fixup and document timestamp update/read barriers
  2015-04-15 17:34   ` Daniel Vetter
  2015-04-15 17:49     ` Chris Wilson
  2015-04-15 21:26     ` Mario Kleiner
@ 2015-04-16  0:17     ` shuang.he
  2015-04-16  8:59     ` Daniel Vetter
  3 siblings, 0 replies; 31+ messages in thread
From: shuang.he @ 2015-04-16  0:17 UTC (permalink / raw)
  To: shuang.he, ethan.gao, intel-gfx, daniel.vetter

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6198
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  302/302              302/302
SNB                                  318/318              318/318
IVB                                  341/341              341/341
BYT                                  287/287              287/287
HSW                 -2              395/395              393/395
BDW                                  318/318              318/318
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 HSW  igt@gem_pwrite_pread@snooped-copy-performance      DMESG_WARN(1)PASS(2)      DMESG_WARN(1)PASS(1)
(dmesg patch applied)drm:i915_hangcheck_elapsed[i915]]*ERROR*Hangcheck_timer_elapsed...blitter_ring_idle@Hangcheck timer elapsed... blitter ring idle
*HSW  igt@gem_pwrite_pread@snooped-pwrite-blt-cpu_mmap-performance      PASS(4)      DMESG_WARN(1)PASS(1)
(dmesg patch applied)drm:i915_hangcheck_elapsed[i915]]*ERROR*Hangcheck_timer_elapsed...blitter_ring_idle@Hangcheck timer elapsed... blitter ring idle
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] drm/vblank: Fixup and document timestamp update/read barriers
  2015-04-15 10:32 ` Daniel Vetter
  2015-04-15 17:34   ` Daniel Vetter
@ 2015-04-16  0:18   ` shuang.he
  1 sibling, 0 replies; 31+ messages in thread
From: shuang.he @ 2015-04-16  0:18 UTC (permalink / raw)
  To: shuang.he, ethan.gao, intel-gfx, daniel.vetter

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6198
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  302/302              302/302
SNB                                  318/318              318/318
IVB                                  341/341              341/341
BYT                                  287/287              287/287
HSW                 -2              395/395              393/395
BDW                                  318/318              318/318
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 HSW  igt@gem_pwrite_pread@snooped-copy-performance      DMESG_WARN(1)PASS(2)      DMESG_WARN(1)PASS(1)
(dmesg patch applied)drm:i915_hangcheck_elapsed[i915]]*ERROR*Hangcheck_timer_elapsed...blitter_ring_idle@Hangcheck timer elapsed... blitter ring idle
*HSW  igt@gem_pwrite_pread@snooped-pwrite-blt-cpu_mmap-performance      PASS(4)      DMESG_WARN(1)PASS(1)
(dmesg patch applied)drm:i915_hangcheck_elapsed[i915]]*ERROR*Hangcheck_timer_elapsed...blitter_ring_idle@Hangcheck timer elapsed... blitter ring idle
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] drm/vblank: Fixup and document timestamp update/read barriers
  2015-04-15 21:26     ` Mario Kleiner
@ 2015-04-16  1:29       ` Peter Hurley
  2015-04-16  6:39         ` Mario Kleiner
  2015-04-16  8:54       ` Daniel Vetter
  1 sibling, 1 reply; 31+ messages in thread
From: Peter Hurley @ 2015-04-16  1:29 UTC (permalink / raw)
  To: Mario Kleiner
  Cc: Daniel Vetter, Michel Dänzer, DRI Development,
	Daniel Vetter, Intel Graphics Development

On 04/15/2015 05:26 PM, Mario Kleiner wrote:
> A couple of questions to educate me and one review comment.
> 
> On 04/15/2015 07:34 PM, 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.
>>
>> v2: Add comment to better explain how store_vblank works, suggested by
>> Chris.
>>
>> v3: Peter noticed that as-is the 2nd smp_wmb is redundant with the
>> implicit barrier in the spin_unlock. But that can only be proven by
>> auditing all callers and my point in extracting this little helper was
>> to localize all the locking into just one place. Hence I think that
>> additional optimization is too risky.
>>
>> 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>
>> Cc: Peter Hurley <peter@hurleysoftware.com>
>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>> ---
>>   drivers/gpu/drm/drm_irq.c | 95 +++++++++++++++++++++++++----------------------
>>   include/drm/drmP.h        |  8 +++-
>>   2 files changed, 57 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
>> index c8a34476570a..8694b77d0002 100644
>> --- a/drivers/gpu/drm/drm_irq.c
>> +++ b/drivers/gpu/drm/drm_irq.c
>> @@ -74,6 +74,36 @@ 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) {
>> +        /* All writers hold the spinlock, but readers are serialized by
>> +         * the latching of vblank->count below.
>> +         */
>> +        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();
>> +}
>> +
>>   /**
>>    * drm_update_vblank_count - update the master vblank counter
>>    * @dev: DRM device
>> @@ -93,7 +123,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 +159,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 +242,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 +258,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 +867,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;
> 
> I wrongly assumed atomic_read would guarantee more than it actually does, so please help me to learn something. Why don't we need some smp_rmb() here before returning vblank->count? What guarantees that drm_vblank_count() does return the latest value assigned to vblank->count in store_vblank()? In store_vblank() there is a smp_wmb(), but why don't we need a matching smp_rmb() here to benefit from it?

Well, you could but the vblank counter resolution is very low (60HZ),
so practically speaking, what would be the point?

The trailing write barrier in store_vblank() is only necessary to
force the ordering of the timestamp wrt to vblank count *in case there
was no write barrier executed between to calls to store_vblank()*,
not to ensure the cpu forces the write to main memory immediately.

Because the time scales for these events don't require that level of
resolution; consider how much code has to get executed between a
hardware vblank irq triggering and the vblank counter being updated.

Realistically, the only relevant requirement is that the timestamp
match the counter.

> 
>>   }
>>   EXPORT_SYMBOL(drm_vblank_count);
>>
>> @@ -897,16 +912,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);
>>
> 
> Similar question as above. We have a new smp_rmb() after the cur_vblank assignment and then after *vblanktime assignment. My original wrong assumption was that the first smp_rmb() wouldn't be needed because atomic_read() would imply that, and that the compiler/cpu couldn't reorder anything here because the *vblanktime assignment depends on cur_vblank from the preceeding atomic_read.
> 
> But why can we now do the comparison while(cur_vblank != vblank->count) without needing something like
> 
>     new_vblank = vblank->count;
>     smp_rmb();
>    } while (cur_vblank != new_vblank);
> 
> to make sure the value from the 2nd vblank->count read isn't stale wrt. to potential updates from store_vblank()?

Similar response here.

What matters is that the timestamp read is consistent with the
vblank count; not that you "just missed" new values.

> Another question is why the drm_vblank_count_and_time() code ever worked without triggering any of my own tests and consistency checks in my software, or any of your igt tests? I run my tests very often, but only on Intel architecture cpus. I assume the same is true for the igt tests? Is there anything specific about Intel cpu's that makes this still work or very unlikely to break? Or are the tests insufficient to catch this? Or just luck?

Intel x86 cpus are strongly-ordered, so smp_rmb() and smp_wmb() are only
compiler barriers on x86 arch.

The compiler could have hoisted the vblanktimestamp read in front of
the vblank count read, but chose not to.

> I looked through kernels back to 3.16 and most uses of the function would be safe from races due to the locking around it, holding of vblank refcounts, or the place and order of execution, e.g., from within drm_handle_vblank(). But in some tight test loop just calling the drmWaitVblank ioctl to query current values i'd expect it to at least occassionally return corrupted timestamps, e.g., time jumping forward or backward, etc.?

As a test, reverse the load order of vblanktimestamp wrt vblank count
and see if your tests trip; if not, you know the tests are insufficient.

Regards,
Peter Hurley

> 
>>       return cur_vblank;
>>   }
>> @@ -1715,7 +1731,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 +1747,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;
> 
> Why is count an unsigned long (= 64 bit on 64-bit kernels) instead of u32 when all users of count are u32? Is this intentional?
> 
> 
>> +    /* 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 */
>>
> 
> Thanks,
> -mario

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] drm/vblank: Fixup and document timestamp update/read barriers
  2015-04-16  1:29       ` Peter Hurley
@ 2015-04-16  6:39         ` Mario Kleiner
  2015-04-16  9:00           ` Peter Hurley
  2015-04-16 12:52           ` Peter Hurley
  0 siblings, 2 replies; 31+ messages in thread
From: Mario Kleiner @ 2015-04-16  6:39 UTC (permalink / raw)
  To: Peter Hurley
  Cc: Daniel Vetter, Michel Dänzer, DRI Development,
	Daniel Vetter, Intel Graphics Development

On 04/16/2015 03:29 AM, Peter Hurley wrote:
> On 04/15/2015 05:26 PM, Mario Kleiner wrote:
>> A couple of questions to educate me and one review comment.
>>
>> On 04/15/2015 07:34 PM, 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.
>>>
>>> v2: Add comment to better explain how store_vblank works, suggested by
>>> Chris.
>>>
>>> v3: Peter noticed that as-is the 2nd smp_wmb is redundant with the
>>> implicit barrier in the spin_unlock. But that can only be proven by
>>> auditing all callers and my point in extracting this little helper was
>>> to localize all the locking into just one place. Hence I think that
>>> additional optimization is too risky.
>>>
>>> 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>
>>> Cc: Peter Hurley <peter@hurleysoftware.com>
>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>> ---
>>>    drivers/gpu/drm/drm_irq.c | 95 +++++++++++++++++++++++++----------------------
>>>    include/drm/drmP.h        |  8 +++-
>>>    2 files changed, 57 insertions(+), 46 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
>>> index c8a34476570a..8694b77d0002 100644
>>> --- a/drivers/gpu/drm/drm_irq.c
>>> +++ b/drivers/gpu/drm/drm_irq.c
>>> @@ -74,6 +74,36 @@ 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) {
>>> +        /* All writers hold the spinlock, but readers are serialized by
>>> +         * the latching of vblank->count below.
>>> +         */
>>> +        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();
>>> +}
>>> +
>>>    /**
>>>     * drm_update_vblank_count - update the master vblank counter
>>>     * @dev: DRM device
>>> @@ -93,7 +123,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 +159,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 +242,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 +258,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 +867,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;
>>
>> I wrongly assumed atomic_read would guarantee more than it actually does, so please help me to learn something. Why don't we need some smp_rmb() here before returning vblank->count? What guarantees that drm_vblank_count() does return the latest value assigned to vblank->count in store_vblank()? In store_vblank() there is a smp_wmb(), but why don't we need a matching smp_rmb() here to benefit from it?
>
> Well, you could but the vblank counter resolution is very low (60HZ),
> so practically speaking, what would be the point?
>

Thanks for the explanations Peter.

Depends on the latency induced. A few microseconds don't matter, a 
millisecond or more would matter for some applications.

> The trailing write barrier in store_vblank() is only necessary to
> force the ordering of the timestamp wrt to vblank count *in case there
> was no write barrier executed between to calls to store_vblank()*,
> not to ensure the cpu forces the write to main memory immediately.
>

That part i understand, the possibly slightly earlier write of some 
store buffers to main memory is just a possible nice side effect. Bits 
and pieces of my memory about how cache coherency etc. work slowly come 
back to my own memory...

> Because the time scales for these events don't require that level of
> resolution; consider how much code has to get executed between a
> hardware vblank irq triggering and the vblank counter being updated.
>
> Realistically, the only relevant requirement is that the timestamp
> match the counter.
>

Yes that is the really important part. A msec delay would possibly 
matter for some timing sensitive apps like mine - some more exotic 
displays run at 200 Hz, and some apps need to synchronize to the vblank 
not strictly for graphics. But i assume potential delays here are more 
on the order of a few microseconds if some pending loads from the cache 
would get reordered for overall efficiency?

>>
>>>    }
>>>    EXPORT_SYMBOL(drm_vblank_count);
>>>
>>> @@ -897,16 +912,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);
>>>
>>
>> Similar question as above. We have a new smp_rmb() after the cur_vblank assignment and then after *vblanktime assignment. My original wrong assumption was that the first smp_rmb() wouldn't be needed because atomic_read() would imply that, and that the compiler/cpu couldn't reorder anything here because the *vblanktime assignment depends on cur_vblank from the preceeding atomic_read.
>>
>> But why can we now do the comparison while(cur_vblank != vblank->count) without needing something like
>>
>>      new_vblank = vblank->count;
>>      smp_rmb();
>>     } while (cur_vblank != new_vblank);
>>
>> to make sure the value from the 2nd vblank->count read isn't stale wrt. to potential updates from store_vblank()?
>
> Similar response here.
>
> What matters is that the timestamp read is consistent with the
> vblank count; not that you "just missed" new values.
>
>> Another question is why the drm_vblank_count_and_time() code ever worked without triggering any of my own tests and consistency checks in my software, or any of your igt tests? I run my tests very often, but only on Intel architecture cpus. I assume the same is true for the igt tests? Is there anything specific about Intel cpu's that makes this still work or very unlikely to break? Or are the tests insufficient to catch this? Or just luck?
>
> Intel x86 cpus are strongly-ordered, so smp_rmb() and smp_wmb() are only
> compiler barriers on x86 arch.
>

Ok, that makes sense as part of the explanation why stuff still worked, 
at least on the tested x86 arch.

> The compiler could have hoisted the vblanktimestamp read in front of
> the vblank count read, but chose not to.
>

This one still confuses me. I know why the smp_rmb after the 
vblanktimestamp read is there - i placed one there in the current code 
myself. But why could the compiler - in absence of the new smp_rmb 
compiler barrier in this patch  - reorder those two loads without 
violating the semantic of the code?

Why could it have turned this

cur_vblank = vblank->count;
// smp_rmb();
vblanktime = vblanktimestamp(dev, crtc, cur_vblank);

into this instead

*vblanktime = vblanktimestamp(dev, crtc, cur_vblank);
// smp_rmb();
cur_vblank = vblank->count;

when the first load would need the value of cur_vblank - and thereby the 
result of the 2nd load - as input to calculate its address for the 
*vblanktime = ... load?

I think i'm still not getting something about why the compiler would be 
allowed to reorder in this way in absence of the additional smp_rmb? Or 
is that barrier required for other archs which are less strongly ordered?

thanks,
-mario

>> I looked through kernels back to 3.16 and most uses of the function would be safe from races due to the locking around it, holding of vblank refcounts, or the place and order of execution, e.g., from within drm_handle_vblank(). But in some tight test loop just calling the drmWaitVblank ioctl to query current values i'd expect it to at least occassionally return corrupted timestamps, e.g., time jumping forward or backward, etc.?
>
> As a test, reverse the load order of vblanktimestamp wrt vblank count
> and see if your tests trip; if not, you know the tests are insufficient.
>
> Regards,
> Peter Hurley
>
>>
>>>        return cur_vblank;
>>>    }
>>> @@ -1715,7 +1731,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 +1747,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;
>>
>> Why is count an unsigned long (= 64 bit on 64-bit kernels) instead of u32 when all users of count are u32? Is this intentional?
>>
>>
>>> +    /* 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 */
>>>
>>
>> Thanks,
>> -mario
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] drm/vblank: Fixup and document timestamp update/read barriers
  2015-04-15 21:26     ` Mario Kleiner
  2015-04-16  1:29       ` Peter Hurley
@ 2015-04-16  8:54       ` Daniel Vetter
  1 sibling, 0 replies; 31+ messages in thread
From: Daniel Vetter @ 2015-04-16  8:54 UTC (permalink / raw)
  To: Mario Kleiner
  Cc: Peter Hurley, Daniel Vetter, Michel Dänzer, DRI Development,
	Daniel Vetter, Intel Graphics Development

On Wed, Apr 15, 2015 at 11:26:37PM +0200, Mario Kleiner wrote:
> A couple of questions to educate me and one review comment.
> 
> On 04/15/2015 07:34 PM, 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.
> >
> >v2: Add comment to better explain how store_vblank works, suggested by
> >Chris.
> >
> >v3: Peter noticed that as-is the 2nd smp_wmb is redundant with the
> >implicit barrier in the spin_unlock. But that can only be proven by
> >auditing all callers and my point in extracting this little helper was
> >to localize all the locking into just one place. Hence I think that
> >additional optimization is too risky.
> >
> >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>
> >Cc: Peter Hurley <peter@hurleysoftware.com>
> >Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> >---
> >  drivers/gpu/drm/drm_irq.c | 95 +++++++++++++++++++++++++----------------------
> >  include/drm/drmP.h        |  8 +++-
> >  2 files changed, 57 insertions(+), 46 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> >index c8a34476570a..8694b77d0002 100644
> >--- a/drivers/gpu/drm/drm_irq.c
> >+++ b/drivers/gpu/drm/drm_irq.c
> >@@ -74,6 +74,36 @@ 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) {
> >+		/* All writers hold the spinlock, but readers are serialized by
> >+		 * the latching of vblank->count below.
> >+		 */
> >+		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();
> >+}
> >+
> >  /**
> >   * drm_update_vblank_count - update the master vblank counter
> >   * @dev: DRM device
> >@@ -93,7 +123,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 +159,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 +242,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 +258,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 +867,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;
> 
> I wrongly assumed atomic_read would guarantee more than it actually does, so
> please help me to learn something. Why don't we need some smp_rmb() here
> before returning vblank->count? What guarantees that drm_vblank_count() does
> return the latest value assigned to vblank->count in store_vblank()? In
> store_vblank() there is a smp_wmb(), but why don't we need a matching
> smp_rmb() here to benefit from it?

Because atomic_read is unordered and atomic_t is just one cpu word
atomic_read() is actually the exact same thing as the replacement here.
Essentially drm_vblank_count just gives you a snapshot and makes no
guarantees about ordering, and from a quick look all callers are ok with
that.

> >  }
> >  EXPORT_SYMBOL(drm_vblank_count);
> >
> >@@ -897,16 +912,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);
> >
> 
> Similar question as above. We have a new smp_rmb() after the cur_vblank
> assignment and then after *vblanktime assignment. My original wrong
> assumption was that the first smp_rmb() wouldn't be needed because
> atomic_read() would imply that, and that the compiler/cpu couldn't reorder
> anything here because the *vblanktime assignment depends on cur_vblank from
> the preceeding atomic_read.

atomic_t is fully unordered in Linux, which is a big contrast to userspace
where atomics (afaik at least) are by default ordered acording to the
load-acquire store-release model. Which means that the cpu and compiler
are free to reorder things however they want (and hence we need explicit
barriers to make sure thing are ok).

The other thing to remember is that for unordered access (i.e. everything
which is not explicit or implicitly a barrier) compiler and cpu can do
whatever they want to with how loads and stores are visible on the
coherency fabric. There is only one restriction: The compiler/cpu is not
allowed to fabricate stores which are not in the code. I.e. if you have
the following code

	b = 0;
	b = 1;
	b = 2;

The only guarantee you really have is that eventually you'll see 2, and
you see nothing else but 0, 1, 2 in between.

The important part is that all the coherency ordering is _only_ from the
cpu up to the coherency fabric. It does _not_ extend to all the other
cpus, so if you want to make an ordered transaction, you have to have
barriers on both the sending and the receiving cpu. Specifically in this
case the following is allowed without the first barrier:

1. read the timestamp, racing with an update (so inconsistent data)
2. read vblank->count
3. smp_rmb();
4. read vblank->count again

So 2&4 read the same vblank count despite that the update race. Only if
you add the first smp_rmb() step 2 is guranateed to happen before step 1.

> But why can we now do the comparison while(cur_vblank != vblank->count)
> without needing something like
> 
> 	new_vblank = vblank->count;
> 	smp_rmb();
>    } while (cur_vblank != new_vblank);
> 
> to make sure the value from the 2nd vblank->count read isn't stale wrt. to
> potential updates from store_vblank()?

You only ever need barriers between different loads/stores and not between
load/stores and computation. So adding a smp_rmb() where you suggested
does exactly nothing.

Well not quite, there is one exception: Compilers are allowed to fabricate
as many loads as they want to. So if your code flow depends upon having a
consistent value (e.g. because you have a bunch of if checks or something
else) then you need to make sure the compiler only loads the value once
using ACCESS_ONCE. But here we only care about equality and only have one
check, so if the compiler goes nuts and reloads the value we don't care
since if it's inequal any value reloaded later on will still be unequal to
cur_vblank. But if you do something more fancy you might indeed need some
additional compiler or full barriers.

> Another question is why the drm_vblank_count_and_time() code ever worked
> without triggering any of my own tests and consistency checks in my
> software, or any of your igt tests? I run my tests very often, but only on
> Intel architecture cpus. I assume the same is true for the igt tests? Is
> there anything specific about Intel cpu's that makes this still work or very
> unlikely to break? Or are the tests insufficient to catch this? Or just
> luck?
> 
> I looked through kernels back to 3.16 and most uses of the function would be
> safe from races due to the locking around it, holding of vblank refcounts,
> or the place and order of execution, e.g., from within drm_handle_vblank().
> But in some tight test loop just calling the drmWaitVblank ioctl to query
> current values i'd expect it to at least occassionally return corrupted
> timestamps, e.g., time jumping forward or backward, etc.?

Luck and the x86 cpu actually being a lot more coherent than the linux
memory model is. x86 has
- fully ordered writes against writes
- fully ordered reads against reads
- the only thing where it reorders is that the cpu can move reads ahead of
  earlier writes (but not the other way round).

So the only risk here is gcc doing something funny, and that only happens
if gcc has a need (register pressure or whatever). The code is simple
enough for that to be practically impossible.

> 
> >  	return cur_vblank;
> >  }
> >@@ -1715,7 +1731,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 +1747,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;
> 
> Why is count an unsigned long (= 64 bit on 64-bit kernels) instead of u32
> when all users of count are u32? Is this intentional?

Well I figured it can't hurt. I can change it to u32 for consistency,
makes sense to do so.
-Daniel

> 
> 
> >+	/* 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 */
> >
> 
> Thanks,
> -mario

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH] drm/vblank: Fixup and document timestamp update/read barriers
  2015-04-15 17:34   ` Daniel Vetter
                       ` (2 preceding siblings ...)
  2015-04-16  0:17     ` shuang.he
@ 2015-04-16  8:59     ` Daniel Vetter
  2015-04-17  4:27       ` shuang.he
  3 siblings, 1 reply; 31+ messages in thread
From: Daniel Vetter @ 2015-04-16  8:59 UTC (permalink / raw)
  To: Intel Graphics Development, DRI Development
  Cc: Peter Hurley, Daniel Vetter, Michel Dänzer, Daniel Vetter

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.

v2: Add comment to better explain how store_vblank works, suggested by
Chris.

v3: Peter noticed that as-is the 2nd smp_wmb is redundant with the
implicit barrier in the spin_unlock. But that can only be proven by
auditing all callers and my point in extracting this little helper was
to localize all the locking into just one place. Hence I think that
additional optimization is too risky.

v4: Use u32 for consistency, suggested by Mario.

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>
Cc: Peter Hurley <peter@hurleysoftware.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v3)
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_irq.c | 95 +++++++++++++++++++++++++----------------------
 include/drm/drmP.h        |  8 +++-
 2 files changed, 57 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index c8a34476570a..d567f031892d 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -74,6 +74,36 @@ 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,
+			 u32 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) {
+		/* All writers hold the spinlock, but readers are serialized by
+		 * the latching of vblank->count below.
+		 */
+		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();
+}
+
 /**
  * drm_update_vblank_count - update the master vblank counter
  * @dev: DRM device
@@ -93,7 +123,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 +159,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 +242,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 +258,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 +867,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 +912,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 +1731,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 +1747,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..013e35645fbb 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 */
+	u32 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 */
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH] drm/vblank: Fixup and document timestamp update/read barriers
  2015-04-16  6:39         ` Mario Kleiner
@ 2015-04-16  9:00           ` Peter Hurley
  2015-04-16  9:06             ` Daniel Vetter
  2015-04-16 12:52           ` Peter Hurley
  1 sibling, 1 reply; 31+ messages in thread
From: Peter Hurley @ 2015-04-16  9:00 UTC (permalink / raw)
  To: Mario Kleiner
  Cc: Daniel Vetter, Michel Dänzer, DRI Development,
	Daniel Vetter, Intel Graphics Development

On 04/16/2015 02:39 AM, Mario Kleiner wrote:
> On 04/16/2015 03:29 AM, Peter Hurley wrote:
>> On 04/15/2015 05:26 PM, Mario Kleiner wrote:
>>> A couple of questions to educate me and one review comment.
>>>
>>> On 04/15/2015 07:34 PM, 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.
>>>>
>>>> v2: Add comment to better explain how store_vblank works, suggested by
>>>> Chris.
>>>>
>>>> v3: Peter noticed that as-is the 2nd smp_wmb is redundant with the
>>>> implicit barrier in the spin_unlock. But that can only be proven by
>>>> auditing all callers and my point in extracting this little helper was
>>>> to localize all the locking into just one place. Hence I think that
>>>> additional optimization is too risky.
>>>>
>>>> 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>
>>>> Cc: Peter Hurley <peter@hurleysoftware.com>
>>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/drm_irq.c | 95 +++++++++++++++++++++++++----------------------
>>>>    include/drm/drmP.h        |  8 +++-
>>>>    2 files changed, 57 insertions(+), 46 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
>>>> index c8a34476570a..8694b77d0002 100644
>>>> --- a/drivers/gpu/drm/drm_irq.c
>>>> +++ b/drivers/gpu/drm/drm_irq.c
>>>> @@ -74,6 +74,36 @@ 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) {
>>>> +        /* All writers hold the spinlock, but readers are serialized by
>>>> +         * the latching of vblank->count below.
>>>> +         */
>>>> +        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();
>>>> +}
>>>> +
>>>>    /**
>>>>     * drm_update_vblank_count - update the master vblank counter
>>>>     * @dev: DRM device
>>>> @@ -93,7 +123,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 +159,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 +242,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 +258,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 +867,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;
>>>
>>> I wrongly assumed atomic_read would guarantee more than it actually does, so please help me to learn something. Why don't we need some smp_rmb() here before returning vblank->count? What guarantees that drm_vblank_count() does return the latest value assigned to vblank->count in store_vblank()? In store_vblank() there is a smp_wmb(), but why don't we need a matching smp_rmb() here to benefit from it?
>>
>> Well, you could but the vblank counter resolution is very low (60HZ),
>> so practically speaking, what would be the point?
>>
> 
> Thanks for the explanations Peter.
> 
> Depends on the latency induced. A few microseconds don't matter, a millisecond or more would matter for some applications.
> 
>> The trailing write barrier in store_vblank() is only necessary to
>> force the ordering of the timestamp wrt to vblank count *in case there
>> was no write barrier executed between to calls to store_vblank()*,
>> not to ensure the cpu forces the write to main memory immediately.
>>
> 
> That part i understand, the possibly slightly earlier write of some store buffers to main memory is just a possible nice side effect. Bits and pieces of my memory about how cache coherency etc. work slowly come back to my own memory...
> 
>> Because the time scales for these events don't require that level of
>> resolution; consider how much code has to get executed between a
>> hardware vblank irq triggering and the vblank counter being updated.
>>
>> Realistically, the only relevant requirement is that the timestamp
>> match the counter.
>>
> 
> Yes that is the really important part. A msec delay would possibly matter for some timing sensitive apps like mine - some more exotic displays run at 200 Hz, and some apps need to synchronize to the vblank not strictly for graphics. But i assume potential delays here are more on the order of a few microseconds if some pending loads from the cache would get reordered for overall efficiency?
> 
>>>
>>>>    }
>>>>    EXPORT_SYMBOL(drm_vblank_count);
>>>>
>>>> @@ -897,16 +912,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);
>>>>
>>>
>>> Similar question as above. We have a new smp_rmb() after the cur_vblank assignment and then after *vblanktime assignment. My original wrong assumption was that the first smp_rmb() wouldn't be needed because atomic_read() would imply that, and that the compiler/cpu couldn't reorder anything here because the *vblanktime assignment depends on cur_vblank from the preceeding atomic_read.
>>>
>>> But why can we now do the comparison while(cur_vblank != vblank->count) without needing something like
>>>
>>>      new_vblank = vblank->count;
>>>      smp_rmb();
>>>     } while (cur_vblank != new_vblank);
>>>
>>> to make sure the value from the 2nd vblank->count read isn't stale wrt. to potential updates from store_vblank()?
>>
>> Similar response here.
>>
>> What matters is that the timestamp read is consistent with the
>> vblank count; not that you "just missed" new values.
>>
>>> Another question is why the drm_vblank_count_and_time() code ever worked without triggering any of my own tests and consistency checks in my software, or any of your igt tests? I run my tests very often, but only on Intel architecture cpus. I assume the same is true for the igt tests? Is there anything specific about Intel cpu's that makes this still work or very unlikely to break? Or are the tests insufficient to catch this? Or just luck?
>>
>> Intel x86 cpus are strongly-ordered, so smp_rmb() and smp_wmb() are only
>> compiler barriers on x86 arch.
>>
> 
> Ok, that makes sense as part of the explanation why stuff still worked, at least on the tested x86 arch.
> 
>> The compiler could have hoisted the vblanktimestamp read in front of
>> the vblank count read, but chose not to.
>>
> 
> This one still confuses me. I know why the smp_rmb after the vblanktimestamp read is there - i placed one there in the current code myself. But why could the compiler - in absence of the new smp_rmb compiler barrier in this patch  - reorder those two loads without violating the semantic of the code?
> 
> Why could it have turned this
> 
> cur_vblank = vblank->count;
> // smp_rmb();
> vblanktime = vblanktimestamp(dev, crtc, cur_vblank);
> 
> into this instead
> 
> *vblanktime = vblanktimestamp(dev, crtc, cur_vblank);
> // smp_rmb();
> cur_vblank = vblank->count;
> 
> when the first load would need the value of cur_vblank - and thereby the result of the 2nd load - as input to calculate its address for the *vblanktime = ... load?
> 
> I think i'm still not getting something about why the compiler would be allowed to reorder in this way in absence of the additional smp_rmb? Or is that barrier required for other archs which are less strongly ordered?

Apologies for the confusion; I missed that it was data-dependent load.

Regards,
Peter Hurley


> thanks,
> -mario
> 
>>> I looked through kernels back to 3.16 and most uses of the function would be safe from races due to the locking around it, holding of vblank refcounts, or the place and order of execution, e.g., from within drm_handle_vblank(). But in some tight test loop just calling the drmWaitVblank ioctl to query current values i'd expect it to at least occassionally return corrupted timestamps, e.g., time jumping forward or backward, etc.?
>>
>> As a test, reverse the load order of vblanktimestamp wrt vblank count
>> and see if your tests trip; if not, you know the tests are insufficient.
>>
>> Regards,
>> Peter Hurley
>>
>>>
>>>>        return cur_vblank;
>>>>    }
>>>> @@ -1715,7 +1731,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 +1747,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;
>>>
>>> Why is count an unsigned long (= 64 bit on 64-bit kernels) instead of u32 when all users of count are u32? Is this intentional?
>>>
>>>
>>>> +    /* 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 */
>>>>
>>>
>>> Thanks,
>>> -mario
>>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] drm/vblank: Fixup and document timestamp update/read barriers
  2015-04-16  9:00           ` Peter Hurley
@ 2015-04-16  9:06             ` Daniel Vetter
  0 siblings, 0 replies; 31+ messages in thread
From: Daniel Vetter @ 2015-04-16  9:06 UTC (permalink / raw)
  To: Peter Hurley
  Cc: Daniel Vetter, Intel Graphics Development, DRI Development,
	Daniel Vetter, Michel Dänzer

On Thu, Apr 16, 2015 at 05:00:02AM -0400, Peter Hurley wrote:
> On 04/16/2015 02:39 AM, Mario Kleiner wrote:
> > I think i'm still not getting something about why the compiler would
> > be allowed to reorder in this way in absence of the additional
> > smp_rmb? Or is that barrier required for other archs which are less
> > strongly ordered?
> 
> Apologies for the confusion; I missed that it was data-dependent load.

Oh right missed that too. Well, alpha can do anything, we'd need at least
a read_barrier_depends(); here. Tbh not sure that's worth it since with
the plain smp_rmb() we match the seqlock code exactly.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] drm/vblank: Fixup and document timestamp update/read barriers
  2015-04-15 17:31   ` Daniel Vetter
@ 2015-04-16 12:30     ` Peter Hurley
  2015-04-16 13:03       ` [Intel-gfx] " Daniel Vetter
  0 siblings, 1 reply; 31+ messages in thread
From: Peter Hurley @ 2015-04-16 12:30 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Daniel Vetter, Intel Graphics Development, Michel Dänzer,
	DRI Development, Daniel Vetter

On 04/15/2015 01:31 PM, Daniel Vetter wrote:
> On Wed, Apr 15, 2015 at 09:00:04AM -0400, Peter Hurley wrote:
>> 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.
> 
> Hm yeah. Otoh to me that's bordering on "code too clever for my own good".
> That the spinlock is held I can assure. That no one goes around and does
> multiple vblank updates (because somehow that code raced with the hw
> itself) I can't easily assure with a simple assert or something similar.
> It's not the case right now, but that can changes.

The algorithm would be broken if multiple updates for the same vblank
count were allowed; that's why it checks to see if the vblank count has
not advanced before storing a new timestamp.

Otherwise, the read side would not be able to determine that the
timestamp is valid by double-checking that the vblank count has not
changed.

And besides, even if the code looped without dropping the spinlock,
the correct write order would still be observed because it would still
be executing on the same cpu.

My objection to the write memory barrier is not about optimization;
it's about correct code.

Regards,
Peter Hurley
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] drm/vblank: Fixup and document timestamp update/read barriers
  2015-04-16  6:39         ` Mario Kleiner
  2015-04-16  9:00           ` Peter Hurley
@ 2015-04-16 12:52           ` Peter Hurley
  1 sibling, 0 replies; 31+ messages in thread
From: Peter Hurley @ 2015-04-16 12:52 UTC (permalink / raw)
  To: Mario Kleiner
  Cc: Daniel Vetter, Michel Dänzer, DRI Development,
	Daniel Vetter, Intel Graphics Development

On 04/16/2015 02:39 AM, Mario Kleiner wrote:
> On 04/16/2015 03:29 AM, Peter Hurley wrote:
>> On 04/15/2015 05:26 PM, Mario Kleiner wrote:

>> Because the time scales for these events don't require that level of
>> resolution; consider how much code has to get executed between a
>> hardware vblank irq triggering and the vblank counter being updated.
>>
>> Realistically, the only relevant requirement is that the timestamp
>> match the counter.
>>
> 
> Yes that is the really important part. A msec delay would possibly matter for some timing sensitive apps like mine - some more exotic displays run at 200 Hz, and some apps need to synchronize to the vblank not strictly for graphics. But i assume potential delays here are more on the order of a few microseconds if some pending loads from the cache would get reordered for overall efficiency?

I'd be surprised if the delay were as much as 1 us.

The latency to return to userspace significantly dwarfs any observable
effects having missed the vblank count update by 1 instruction.

Regards,
Peter Hurley
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/vblank: Fixup and document timestamp update/read barriers
  2015-04-16 12:30     ` Peter Hurley
@ 2015-04-16 13:03       ` Daniel Vetter
  2015-05-04  4:52         ` Mario Kleiner
  0 siblings, 1 reply; 31+ messages in thread
From: Daniel Vetter @ 2015-04-16 13:03 UTC (permalink / raw)
  To: Peter Hurley
  Cc: Daniel Vetter, Intel Graphics Development, DRI Development,
	Daniel Vetter, Michel Dänzer

On Thu, Apr 16, 2015 at 08:30:55AM -0400, Peter Hurley wrote:
> On 04/15/2015 01:31 PM, Daniel Vetter wrote:
> > On Wed, Apr 15, 2015 at 09:00:04AM -0400, Peter Hurley wrote:
> >> 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.
> > 
> > Hm yeah. Otoh to me that's bordering on "code too clever for my own good".
> > That the spinlock is held I can assure. That no one goes around and does
> > multiple vblank updates (because somehow that code raced with the hw
> > itself) I can't easily assure with a simple assert or something similar.
> > It's not the case right now, but that can changes.
> 
> The algorithm would be broken if multiple updates for the same vblank
> count were allowed; that's why it checks to see if the vblank count has
> not advanced before storing a new timestamp.
> 
> Otherwise, the read side would not be able to determine that the
> timestamp is valid by double-checking that the vblank count has not
> changed.
> 
> And besides, even if the code looped without dropping the spinlock,
> the correct write order would still be observed because it would still
> be executing on the same cpu.
> 
> My objection to the write memory barrier is not about optimization;
> it's about correct code.

Well diff=0 is not allowed, I guess I could enforce this with some
WARN_ON. And I still think my point of non-local correctness is solid.
With the smp_wmb() removed the following still works correctly:

spin_lock(vblank_time_lock);
store_vblank(dev, crtc, 1, ts1);
spin_unlock(vblank_time_lock);

spin_lock(vblank_time_lock);
store_vblank(dev, crtc, 1, ts2);
spin_unlock(vblank_time_lock);

But with the smp_wmb(); removed the following would be broken:

spin_lock(vblank_time_lock);
store_vblank(dev, crtc, 1, ts1);
store_vblank(dev, crtc, 1, ts2);
spin_unlock(vblank_time_lock);

because the compiler/cpu is free to reorder the store for vblank->count
_ahead_ of the store for the timestamp. And that would trick readers into
believing that they have a valid timestamp when they potentially raced.

Now you're correct that right now there's no such thing going on, and it's
unlikely to happen (given the nature of vblank updates). But my point is
that if we optimize this then the correctness can't be proven locally
anymore by just looking at store_vblank, but instead you must audit all
the callers. And leaking locking/barriers like that is too fragile design
for my taste.

But you insist that my approach is broken somehow and dropping the smp_wmb
is needed for correctness. I don't see how that's the case at all.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] drm/vblank: Fixup and document timestamp update/read barriers
  2015-04-16  8:59     ` Daniel Vetter
@ 2015-04-17  4:27       ` shuang.he
  0 siblings, 0 replies; 31+ messages in thread
From: shuang.he @ 2015-04-17  4:27 UTC (permalink / raw)
  To: shuang.he, ethan.gao, intel-gfx, daniel.vetter

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6213
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  302/302              302/302
SNB                                  318/318              318/318
IVB                                  341/341              341/341
BYT                                  287/287              287/287
HSW                                  395/395              395/395
BDW                                  318/318              318/318
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/vblank: Fixup and document timestamp update/read barriers
  2015-04-16 13:03       ` [Intel-gfx] " Daniel Vetter
@ 2015-05-04  4:52         ` Mario Kleiner
  2015-05-05 14:36           ` Peter Hurley
  0 siblings, 1 reply; 31+ messages in thread
From: Mario Kleiner @ 2015-05-04  4:52 UTC (permalink / raw)
  To: Daniel Vetter, Peter Hurley
  Cc: Daniel Vetter, Intel Graphics Development, Michel Dänzer,
	DRI Development, Daniel Vetter

On 04/16/2015 03:03 PM, Daniel Vetter wrote:
> On Thu, Apr 16, 2015 at 08:30:55AM -0400, Peter Hurley wrote:
>> On 04/15/2015 01:31 PM, Daniel Vetter wrote:
>>> On Wed, Apr 15, 2015 at 09:00:04AM -0400, Peter Hurley wrote:
>>>> 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.
>>>
>>> Hm yeah. Otoh to me that's bordering on "code too clever for my own good".
>>> That the spinlock is held I can assure. That no one goes around and does
>>> multiple vblank updates (because somehow that code raced with the hw
>>> itself) I can't easily assure with a simple assert or something similar.
>>> It's not the case right now, but that can changes.
>>
>> The algorithm would be broken if multiple updates for the same vblank
>> count were allowed; that's why it checks to see if the vblank count has
>> not advanced before storing a new timestamp.
>>
>> Otherwise, the read side would not be able to determine that the
>> timestamp is valid by double-checking that the vblank count has not
>> changed.
>>
>> And besides, even if the code looped without dropping the spinlock,
>> the correct write order would still be observed because it would still
>> be executing on the same cpu.
>>
>> My objection to the write memory barrier is not about optimization;
>> it's about correct code.
>
> Well diff=0 is not allowed, I guess I could enforce this with some
> WARN_ON. And I still think my point of non-local correctness is solid.
> With the smp_wmb() removed the following still works correctly:
>
> spin_lock(vblank_time_lock);
> store_vblank(dev, crtc, 1, ts1);
> spin_unlock(vblank_time_lock);
>
> spin_lock(vblank_time_lock);
> store_vblank(dev, crtc, 1, ts2);
> spin_unlock(vblank_time_lock);
>
> But with the smp_wmb(); removed the following would be broken:
>
> spin_lock(vblank_time_lock);
> store_vblank(dev, crtc, 1, ts1);
> store_vblank(dev, crtc, 1, ts2);
> spin_unlock(vblank_time_lock);
>
> because the compiler/cpu is free to reorder the store for vblank->count
> _ahead_ of the store for the timestamp. And that would trick readers into
> believing that they have a valid timestamp when they potentially raced.
>
> Now you're correct that right now there's no such thing going on, and it's
> unlikely to happen (given the nature of vblank updates). But my point is
> that if we optimize this then the correctness can't be proven locally
> anymore by just looking at store_vblank, but instead you must audit all
> the callers. And leaking locking/barriers like that is too fragile design
> for my taste.
>
> But you insist that my approach is broken somehow and dropping the smp_wmb
> is needed for correctness. I don't see how that's the case at all.
> -Daniel
>

Fwiw, i spent some time reeducating myself about memory barriers (thanks 
for your explanations) and thinking about this, and the last version of 
your patch looks good to me. It also makes sense to me to leave that 
last smb_wmb() in place to make future use of the helper robust - for 
non-local correctness, to avoid having to audit all future callers of 
that helper.

I also tested your patch + a slightly modified version of Chris vblank 
delayed disable / instant query patches + my fixes using my own stress 
tests and hardware timing test equipment on both intel and nouveau, and 
everything seems to work fine.

So i'm all for including this patch and it has my

Reviewed-and-tested-by: Mario Kleiner <mario.kleiner.de@gmail.com>

I just sent out an updated version of my patches, so they don't conflict 
with this one and also fix a compile failure of drm/qxl with yours.

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] drm/vblank: Fixup and document timestamp update/read barriers
  2015-05-04  4:52         ` Mario Kleiner
@ 2015-05-05 14:36           ` Peter Hurley
  2015-05-05 15:42             ` [Intel-gfx] " Daniel Vetter
  0 siblings, 1 reply; 31+ messages in thread
From: Peter Hurley @ 2015-05-05 14:36 UTC (permalink / raw)
  To: Mario Kleiner, Daniel Vetter
  Cc: Daniel Vetter, Intel Graphics Development, Michel Dänzer,
	DRI Development, Daniel Vetter

On 05/04/2015 12:52 AM, Mario Kleiner wrote:
> On 04/16/2015 03:03 PM, Daniel Vetter wrote:
>> On Thu, Apr 16, 2015 at 08:30:55AM -0400, Peter Hurley wrote:
>>> On 04/15/2015 01:31 PM, Daniel Vetter wrote:
>>>> On Wed, Apr 15, 2015 at 09:00:04AM -0400, Peter Hurley wrote:
>>>>> 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.
>>>>
>>>> Hm yeah. Otoh to me that's bordering on "code too clever for my own good".
>>>> That the spinlock is held I can assure. That no one goes around and does
>>>> multiple vblank updates (because somehow that code raced with the hw
>>>> itself) I can't easily assure with a simple assert or something similar.
>>>> It's not the case right now, but that can changes.
>>>
>>> The algorithm would be broken if multiple updates for the same vblank
>>> count were allowed; that's why it checks to see if the vblank count has
>>> not advanced before storing a new timestamp.
>>>
>>> Otherwise, the read side would not be able to determine that the
>>> timestamp is valid by double-checking that the vblank count has not
>>> changed.
>>>
>>> And besides, even if the code looped without dropping the spinlock,
>>> the correct write order would still be observed because it would still
>>> be executing on the same cpu.
>>>
>>> My objection to the write memory barrier is not about optimization;
>>> it's about correct code.
>>
>> Well diff=0 is not allowed, I guess I could enforce this with some
>> WARN_ON. And I still think my point of non-local correctness is solid.
>> With the smp_wmb() removed the following still works correctly:
>>
>> spin_lock(vblank_time_lock);
>> store_vblank(dev, crtc, 1, ts1);
>> spin_unlock(vblank_time_lock);
>>
>> spin_lock(vblank_time_lock);
>> store_vblank(dev, crtc, 1, ts2);
>> spin_unlock(vblank_time_lock);
>>
>> But with the smp_wmb(); removed the following would be broken:
>>
>> spin_lock(vblank_time_lock);
>> store_vblank(dev, crtc, 1, ts1);
>> store_vblank(dev, crtc, 1, ts2);
>> spin_unlock(vblank_time_lock);
>>
>> because the compiler/cpu is free to reorder the store for vblank->count
>> _ahead_ of the store for the timestamp. And that would trick readers into
>> believing that they have a valid timestamp when they potentially raced.
>>
>> Now you're correct that right now there's no such thing going on, and it's
>> unlikely to happen (given the nature of vblank updates). But my point is
>> that if we optimize this then the correctness can't be proven locally
>> anymore by just looking at store_vblank, but instead you must audit all
>> the callers. And leaking locking/barriers like that is too fragile design
>> for my taste.
>>
>> But you insist that my approach is broken somehow and dropping the smp_wmb
>> is needed for correctness. I don't see how that's the case at all.

Daniel,

I've been really busy this last week; my apologies for not replying promptly.

> Fwiw, i spent some time reeducating myself about memory barriers (thanks for your explanations) and thinking about this, and the last version of your patch looks good to me. It also makes sense to me to leave that last smb_wmb() in place to make future use of the helper robust - for non-local correctness, to avoid having to audit all future callers of that helper.

My concern wrt to unnecessary barriers in this algorithm is that the trailing
barrier now appears mandatory, when in fact it is not.

Moreover, this algorithm is, in general, fragile and not designed to handle
random or poorly-researched changes.

For example, if only the read and store operations are considered, it's obviously
unsafe, since a read may unwittingly retrieve an store in progress.


CPU 0                                   | CPU 1
                                        |
                             /* vblank->count == 0 */
                                        |
drm_vblank_count_and_time()             | store_vblank(.., inc = 2, ...)
                                        |
  cur_vblank <= LOAD vblank->count      |
                                        |   tslot = vblank->count + 2
                                        |   /* tslot == 2 */
                                        |   STORE vblanktime[0]
  - - - - - - - - - - - - - - - - - - - - - smp_wmb() - - - - - - - - - -
  /* cur_vblank == 0 */                 |
  local <= LOAD vblanktime[0]           |
  smp_rmb - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - 
                                        |
 * cpu has loaded the wrong timestamp * |
                                        |
  local <= LOAD vblank->count           |
  cur_vblank == local?                  |
  yes - exit loop                       |
                                        |   vblank->count += 2
  - - - - - - - - - - - - - - - - - - - - - smp_wmb() - - - - - - - - - -

Regards,
Peter Hurley


> I also tested your patch + a slightly modified version of Chris vblank delayed disable / instant query patches + my fixes using my own stress tests and hardware timing test equipment on both intel and nouveau, and everything seems to work fine.
>
> So i'm all for including this patch and it has my
> 
> Reviewed-and-tested-by: Mario Kleiner <mario.kleiner.de@gmail.com>
> 
> I just sent out an updated version of my patches, so they don't conflict with this one and also fix a compile failure of drm/qxl with yours.
> 
> Thanks,
> -mario

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/vblank: Fixup and document timestamp update/read barriers
  2015-05-05 14:36           ` Peter Hurley
@ 2015-05-05 15:42             ` Daniel Vetter
  2015-05-05 15:57               ` Peter Hurley
  0 siblings, 1 reply; 31+ messages in thread
From: Daniel Vetter @ 2015-05-05 15:42 UTC (permalink / raw)
  To: Peter Hurley
  Cc: Daniel Vetter, Intel Graphics Development, DRI Development,
	Daniel Vetter, Michel Dänzer

On Tue, May 05, 2015 at 10:36:24AM -0400, Peter Hurley wrote:
> On 05/04/2015 12:52 AM, Mario Kleiner wrote:
> > On 04/16/2015 03:03 PM, Daniel Vetter wrote:
> >> On Thu, Apr 16, 2015 at 08:30:55AM -0400, Peter Hurley wrote:
> >>> On 04/15/2015 01:31 PM, Daniel Vetter wrote:
> >>>> On Wed, Apr 15, 2015 at 09:00:04AM -0400, Peter Hurley wrote:
> >>>>> 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.
> >>>>
> >>>> Hm yeah. Otoh to me that's bordering on "code too clever for my own good".
> >>>> That the spinlock is held I can assure. That no one goes around and does
> >>>> multiple vblank updates (because somehow that code raced with the hw
> >>>> itself) I can't easily assure with a simple assert or something similar.
> >>>> It's not the case right now, but that can changes.
> >>>
> >>> The algorithm would be broken if multiple updates for the same vblank
> >>> count were allowed; that's why it checks to see if the vblank count has
> >>> not advanced before storing a new timestamp.
> >>>
> >>> Otherwise, the read side would not be able to determine that the
> >>> timestamp is valid by double-checking that the vblank count has not
> >>> changed.
> >>>
> >>> And besides, even if the code looped without dropping the spinlock,
> >>> the correct write order would still be observed because it would still
> >>> be executing on the same cpu.
> >>>
> >>> My objection to the write memory barrier is not about optimization;
> >>> it's about correct code.
> >>
> >> Well diff=0 is not allowed, I guess I could enforce this with some
> >> WARN_ON. And I still think my point of non-local correctness is solid.
> >> With the smp_wmb() removed the following still works correctly:
> >>
> >> spin_lock(vblank_time_lock);
> >> store_vblank(dev, crtc, 1, ts1);
> >> spin_unlock(vblank_time_lock);
> >>
> >> spin_lock(vblank_time_lock);
> >> store_vblank(dev, crtc, 1, ts2);
> >> spin_unlock(vblank_time_lock);
> >>
> >> But with the smp_wmb(); removed the following would be broken:
> >>
> >> spin_lock(vblank_time_lock);
> >> store_vblank(dev, crtc, 1, ts1);
> >> store_vblank(dev, crtc, 1, ts2);
> >> spin_unlock(vblank_time_lock);
> >>
> >> because the compiler/cpu is free to reorder the store for vblank->count
> >> _ahead_ of the store for the timestamp. And that would trick readers into
> >> believing that they have a valid timestamp when they potentially raced.
> >>
> >> Now you're correct that right now there's no such thing going on, and it's
> >> unlikely to happen (given the nature of vblank updates). But my point is
> >> that if we optimize this then the correctness can't be proven locally
> >> anymore by just looking at store_vblank, but instead you must audit all
> >> the callers. And leaking locking/barriers like that is too fragile design
> >> for my taste.
> >>
> >> But you insist that my approach is broken somehow and dropping the smp_wmb
> >> is needed for correctness. I don't see how that's the case at all.
> 
> Daniel,
> 
> I've been really busy this last week; my apologies for not replying promptly.
> 
> > Fwiw, i spent some time reeducating myself about memory barriers (thanks for your explanations) and thinking about this, and the last version of your patch looks good to me. It also makes sense to me to leave that last smb_wmb() in place to make future use of the helper robust - for non-local correctness, to avoid having to audit all future callers of that helper.
> 
> My concern wrt to unnecessary barriers in this algorithm is that the trailing
> barrier now appears mandatory, when in fact it is not.
> 
> Moreover, this algorithm is, in general, fragile and not designed to handle
> random or poorly-researched changes.

Less fragility is exactly why I want that surplus barrier. But I've run
out of new ideas for how to explain that ...

> For example, if only the read and store operations are considered, it's obviously
> unsafe, since a read may unwittingly retrieve an store in progress.
> 
> 
> CPU 0                                   | CPU 1
>                                         |
>                              /* vblank->count == 0 */
>                                         |
> drm_vblank_count_and_time()             | store_vblank(.., inc = 2, ...)
>                                         |
>   cur_vblank <= LOAD vblank->count      |
>                                         |   tslot = vblank->count + 2
>                                         |   /* tslot == 2 */
>                                         |   STORE vblanktime[0]

This line here is wrong, it should be "STORE vblanktime[2]"

The "STORE vblanktime[0]" happened way earlier, before 2 smp_wmb and the
previous updating of vblank->count.

I'm also somewhat confused about how you to a line across both cpus for
barriers because barriers only have cpu-local effects (which is why we
always need a barrier on both ends of a transaction).

In short I still don't follow what's wrong.
-Daniel

>   - - - - - - - - - - - - - - - - - - - - - smp_wmb() - - - - - - - - - -
>   /* cur_vblank == 0 */                 |
>   local <= LOAD vblanktime[0]           |
>   smp_rmb - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - 
>                                         |
>  * cpu has loaded the wrong timestamp * |
>                                         |
>   local <= LOAD vblank->count           |
>   cur_vblank == local?                  |
>   yes - exit loop                       |
>                                         |   vblank->count += 2
>   - - - - - - - - - - - - - - - - - - - - - smp_wmb() - - - - - - - - - -
> 
> Regards,
> Peter Hurley
> 
> 
> > I also tested your patch + a slightly modified version of Chris vblank delayed disable / instant query patches + my fixes using my own stress tests and hardware timing test equipment on both intel and nouveau, and everything seems to work fine.
> >
> > So i'm all for including this patch and it has my
> > 
> > Reviewed-and-tested-by: Mario Kleiner <mario.kleiner.de@gmail.com>
> > 
> > I just sent out an updated version of my patches, so they don't conflict with this one and also fix a compile failure of drm/qxl with yours.
> > 
> > Thanks,
> > -mario
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/vblank: Fixup and document timestamp update/read barriers
  2015-05-05 15:42             ` [Intel-gfx] " Daniel Vetter
@ 2015-05-05 15:57               ` Peter Hurley
  2015-05-05 19:01                 ` Peter Hurley
  2015-05-06  8:56                 ` Daniel Vetter
  0 siblings, 2 replies; 31+ messages in thread
From: Peter Hurley @ 2015-05-05 15:57 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Daniel Vetter, Michel Dänzer, DRI Development,
	Daniel Vetter, Intel Graphics Development

On 05/05/2015 11:42 AM, Daniel Vetter wrote:
> On Tue, May 05, 2015 at 10:36:24AM -0400, Peter Hurley wrote:
>> On 05/04/2015 12:52 AM, Mario Kleiner wrote:
>>> On 04/16/2015 03:03 PM, Daniel Vetter wrote:
>>>> On Thu, Apr 16, 2015 at 08:30:55AM -0400, Peter Hurley wrote:
>>>>> On 04/15/2015 01:31 PM, Daniel Vetter wrote:
>>>>>> On Wed, Apr 15, 2015 at 09:00:04AM -0400, Peter Hurley wrote:
>>>>>>> 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.
>>>>>>
>>>>>> Hm yeah. Otoh to me that's bordering on "code too clever for my own good".
>>>>>> That the spinlock is held I can assure. That no one goes around and does
>>>>>> multiple vblank updates (because somehow that code raced with the hw
>>>>>> itself) I can't easily assure with a simple assert or something similar.
>>>>>> It's not the case right now, but that can changes.
>>>>>
>>>>> The algorithm would be broken if multiple updates for the same vblank
>>>>> count were allowed; that's why it checks to see if the vblank count has
>>>>> not advanced before storing a new timestamp.
>>>>>
>>>>> Otherwise, the read side would not be able to determine that the
>>>>> timestamp is valid by double-checking that the vblank count has not
>>>>> changed.
>>>>>
>>>>> And besides, even if the code looped without dropping the spinlock,
>>>>> the correct write order would still be observed because it would still
>>>>> be executing on the same cpu.
>>>>>
>>>>> My objection to the write memory barrier is not about optimization;
>>>>> it's about correct code.
>>>>
>>>> Well diff=0 is not allowed, I guess I could enforce this with some
>>>> WARN_ON. And I still think my point of non-local correctness is solid.
>>>> With the smp_wmb() removed the following still works correctly:
>>>>
>>>> spin_lock(vblank_time_lock);
>>>> store_vblank(dev, crtc, 1, ts1);
>>>> spin_unlock(vblank_time_lock);
>>>>
>>>> spin_lock(vblank_time_lock);
>>>> store_vblank(dev, crtc, 1, ts2);
>>>> spin_unlock(vblank_time_lock);
>>>>
>>>> But with the smp_wmb(); removed the following would be broken:
>>>>
>>>> spin_lock(vblank_time_lock);
>>>> store_vblank(dev, crtc, 1, ts1);
>>>> store_vblank(dev, crtc, 1, ts2);
>>>> spin_unlock(vblank_time_lock);
>>>>
>>>> because the compiler/cpu is free to reorder the store for vblank->count
>>>> _ahead_ of the store for the timestamp. And that would trick readers into
>>>> believing that they have a valid timestamp when they potentially raced.
>>>>
>>>> Now you're correct that right now there's no such thing going on, and it's
>>>> unlikely to happen (given the nature of vblank updates). But my point is
>>>> that if we optimize this then the correctness can't be proven locally
>>>> anymore by just looking at store_vblank, but instead you must audit all
>>>> the callers. And leaking locking/barriers like that is too fragile design
>>>> for my taste.
>>>>
>>>> But you insist that my approach is broken somehow and dropping the smp_wmb
>>>> is needed for correctness. I don't see how that's the case at all.
>>
>> Daniel,
>>
>> I've been really busy this last week; my apologies for not replying promptly.
>>
>>> Fwiw, i spent some time reeducating myself about memory barriers (thanks for your explanations) and thinking about this, and the last version of your patch looks good to me. It also makes sense to me to leave that last smb_wmb() in place to make future use of the helper robust - for non-local correctness, to avoid having to audit all future callers of that helper.
>>
>> My concern wrt to unnecessary barriers in this algorithm is that the trailing
>> barrier now appears mandatory, when in fact it is not.
>>
>> Moreover, this algorithm is, in general, fragile and not designed to handle
>> random or poorly-researched changes.
> 
> Less fragility is exactly why I want that surplus barrier. But I've run
> out of new ideas for how to explain that ...
> 
>> For example, if only the read and store operations are considered, it's obviously
>> unsafe, since a read may unwittingly retrieve an store in progress.
>>
>>
>> CPU 0                                   | CPU 1
>>                                         |
>>                              /* vblank->count == 0 */
>>                                         |
>> drm_vblank_count_and_time()             | store_vblank(.., inc = 2, ...)
>>                                         |
>>   cur_vblank <= LOAD vblank->count      |
>>                                         |   tslot = vblank->count + 2
>>                                         |   /* tslot == 2 */
>>                                         |   STORE vblanktime[0]
> 
> This line here is wrong, it should be "STORE vblanktime[2]"
> 
> The "STORE vblanktime[0]" happened way earlier, before 2 smp_wmb and the
> previous updating of vblank->count.

&vblanktime[0] == &vblanktime[2]

That's why I keep trying to explain you actually have to look at and
understand the algorithm before blindly assuming local behavior is
sufficient.

> I'm also somewhat confused about how you to a line across both cpus for
> barriers because barriers only have cpu-local effects (which is why we
> always need a barrier on both ends of a transaction).
> 
> In short I still don't follow what's wrong.
> -Daniel
> 
>>   - - - - - - - - - - - - - - - - - - - - - smp_wmb() - - - - - - - - - -
>>   /* cur_vblank == 0 */                 |
>>   local <= LOAD vblanktime[0]           |
>>   smp_rmb - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - 
>>                                         |
>>  * cpu has loaded the wrong timestamp * |
>>                                         |
>>   local <= LOAD vblank->count           |
>>   cur_vblank == local?                  |
>>   yes - exit loop                       |
>>                                         |   vblank->count += 2
>>   - - - - - - - - - - - - - - - - - - - - - smp_wmb() - - - - - - - - - -
>>
>> Regards,
>> Peter Hurley
>>
>>
>>> I also tested your patch + a slightly modified version of Chris vblank delayed disable / instant query patches + my fixes using my own stress tests and hardware timing test equipment on both intel and nouveau, and everything seems to work fine.
>>>
>>> So i'm all for including this patch and it has my
>>>
>>> Reviewed-and-tested-by: Mario Kleiner <mario.kleiner.de@gmail.com>
>>>
>>> I just sent out an updated version of my patches, so they don't conflict with this one and also fix a compile failure of drm/qxl with yours.
>>>
>>> Thanks,
>>> -mario
>>
> 

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/vblank: Fixup and document timestamp update/read barriers
  2015-05-05 15:57               ` Peter Hurley
@ 2015-05-05 19:01                 ` Peter Hurley
  2015-05-06  8:56                 ` Daniel Vetter
  1 sibling, 0 replies; 31+ messages in thread
From: Peter Hurley @ 2015-05-05 19:01 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Daniel Vetter, Michel Dänzer, DRI Development,
	Daniel Vetter, Intel Graphics Development

On 05/05/2015 11:57 AM, Peter Hurley wrote:
> On 05/05/2015 11:42 AM, Daniel Vetter wrote:
>> I'm also somewhat confused about how you to a line across both cpus for
>> barriers because barriers only have cpu-local effects (which is why we
>> always need a barrier on both ends of a transaction).

I'm sorry if my barrier notation confuses you; I find that it clearly
identifies matching pairs.

Also, there is a distinction between "can be visible" and "must be visible";
the load and stores themselves are not cpu-local.

Regards,
Peter Hurley


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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] drm/vblank: Fixup and document timestamp update/read barriers
  2015-05-05 15:57               ` Peter Hurley
  2015-05-05 19:01                 ` Peter Hurley
@ 2015-05-06  8:56                 ` Daniel Vetter
  2015-05-07 11:56                   ` [Intel-gfx] " Peter Hurley
  1 sibling, 1 reply; 31+ messages in thread
From: Daniel Vetter @ 2015-05-06  8:56 UTC (permalink / raw)
  To: Peter Hurley
  Cc: Daniel Vetter, Intel Graphics Development, DRI Development,
	Daniel Vetter, Michel Dänzer

On Tue, May 05, 2015 at 11:57:42AM -0400, Peter Hurley wrote:
> On 05/05/2015 11:42 AM, Daniel Vetter wrote:
> > On Tue, May 05, 2015 at 10:36:24AM -0400, Peter Hurley wrote:
> >> On 05/04/2015 12:52 AM, Mario Kleiner wrote:
> >>> On 04/16/2015 03:03 PM, Daniel Vetter wrote:
> >>>> On Thu, Apr 16, 2015 at 08:30:55AM -0400, Peter Hurley wrote:
> >>>>> On 04/15/2015 01:31 PM, Daniel Vetter wrote:
> >>>>>> On Wed, Apr 15, 2015 at 09:00:04AM -0400, Peter Hurley wrote:
> >>>>>>> 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.
> >>>>>>
> >>>>>> Hm yeah. Otoh to me that's bordering on "code too clever for my own good".
> >>>>>> That the spinlock is held I can assure. That no one goes around and does
> >>>>>> multiple vblank updates (because somehow that code raced with the hw
> >>>>>> itself) I can't easily assure with a simple assert or something similar.
> >>>>>> It's not the case right now, but that can changes.
> >>>>>
> >>>>> The algorithm would be broken if multiple updates for the same vblank
> >>>>> count were allowed; that's why it checks to see if the vblank count has
> >>>>> not advanced before storing a new timestamp.
> >>>>>
> >>>>> Otherwise, the read side would not be able to determine that the
> >>>>> timestamp is valid by double-checking that the vblank count has not
> >>>>> changed.
> >>>>>
> >>>>> And besides, even if the code looped without dropping the spinlock,
> >>>>> the correct write order would still be observed because it would still
> >>>>> be executing on the same cpu.
> >>>>>
> >>>>> My objection to the write memory barrier is not about optimization;
> >>>>> it's about correct code.
> >>>>
> >>>> Well diff=0 is not allowed, I guess I could enforce this with some
> >>>> WARN_ON. And I still think my point of non-local correctness is solid.
> >>>> With the smp_wmb() removed the following still works correctly:
> >>>>
> >>>> spin_lock(vblank_time_lock);
> >>>> store_vblank(dev, crtc, 1, ts1);
> >>>> spin_unlock(vblank_time_lock);
> >>>>
> >>>> spin_lock(vblank_time_lock);
> >>>> store_vblank(dev, crtc, 1, ts2);
> >>>> spin_unlock(vblank_time_lock);
> >>>>
> >>>> But with the smp_wmb(); removed the following would be broken:
> >>>>
> >>>> spin_lock(vblank_time_lock);
> >>>> store_vblank(dev, crtc, 1, ts1);
> >>>> store_vblank(dev, crtc, 1, ts2);
> >>>> spin_unlock(vblank_time_lock);
> >>>>
> >>>> because the compiler/cpu is free to reorder the store for vblank->count
> >>>> _ahead_ of the store for the timestamp. And that would trick readers into
> >>>> believing that they have a valid timestamp when they potentially raced.
> >>>>
> >>>> Now you're correct that right now there's no such thing going on, and it's
> >>>> unlikely to happen (given the nature of vblank updates). But my point is
> >>>> that if we optimize this then the correctness can't be proven locally
> >>>> anymore by just looking at store_vblank, but instead you must audit all
> >>>> the callers. And leaking locking/barriers like that is too fragile design
> >>>> for my taste.
> >>>>
> >>>> But you insist that my approach is broken somehow and dropping the smp_wmb
> >>>> is needed for correctness. I don't see how that's the case at all.
> >>
> >> Daniel,
> >>
> >> I've been really busy this last week; my apologies for not replying promptly.
> >>
> >>> Fwiw, i spent some time reeducating myself about memory barriers (thanks for your explanations) and thinking about this, and the last version of your patch looks good to me. It also makes sense to me to leave that last smb_wmb() in place to make future use of the helper robust - for non-local correctness, to avoid having to audit all future callers of that helper.
> >>
> >> My concern wrt to unnecessary barriers in this algorithm is that the trailing
> >> barrier now appears mandatory, when in fact it is not.
> >>
> >> Moreover, this algorithm is, in general, fragile and not designed to handle
> >> random or poorly-researched changes.
> > 
> > Less fragility is exactly why I want that surplus barrier. But I've run
> > out of new ideas for how to explain that ...
> > 
> >> For example, if only the read and store operations are considered, it's obviously
> >> unsafe, since a read may unwittingly retrieve an store in progress.
> >>
> >>
> >> CPU 0                                   | CPU 1
> >>                                         |
> >>                              /* vblank->count == 0 */
> >>                                         |
> >> drm_vblank_count_and_time()             | store_vblank(.., inc = 2, ...)
> >>                                         |
> >>   cur_vblank <= LOAD vblank->count      |
> >>                                         |   tslot = vblank->count + 2
> >>                                         |   /* tslot == 2 */
> >>                                         |   STORE vblanktime[0]
> > 
> > This line here is wrong, it should be "STORE vblanktime[2]"
> > 
> > The "STORE vblanktime[0]" happened way earlier, before 2 smp_wmb and the
> > previous updating of vblank->count.
> 
> &vblanktime[0] == &vblanktime[2]
> 
> That's why I keep trying to explain you actually have to look at and
> understand the algorithm before blindly assuming local behavior is
> sufficient.

Ok now I think I got it, the issue is when the array (which is only 2
elements big) wraps around. And that's racy because we don't touch the
increment before _and_ after the write side update. But that seems like a
bug that's always been there?
-Daniel
-- 
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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/vblank: Fixup and document timestamp update/read barriers
  2015-05-06  8:56                 ` Daniel Vetter
@ 2015-05-07 11:56                   ` Peter Hurley
  2015-05-07 17:33                     ` Mario Kleiner
  0 siblings, 1 reply; 31+ messages in thread
From: Peter Hurley @ 2015-05-07 11:56 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Daniel Vetter, Michel Dänzer, DRI Development,
	Daniel Vetter, Intel Graphics Development

On 05/06/2015 04:56 AM, Daniel Vetter wrote:
> On Tue, May 05, 2015 at 11:57:42AM -0400, Peter Hurley wrote:
>> On 05/05/2015 11:42 AM, Daniel Vetter wrote:
>>> On Tue, May 05, 2015 at 10:36:24AM -0400, Peter Hurley wrote:
>>>> On 05/04/2015 12:52 AM, Mario Kleiner wrote:
>>>>> On 04/16/2015 03:03 PM, Daniel Vetter wrote:
>>>>>> On Thu, Apr 16, 2015 at 08:30:55AM -0400, Peter Hurley wrote:
>>>>>>> On 04/15/2015 01:31 PM, Daniel Vetter wrote:
>>>>>>>> On Wed, Apr 15, 2015 at 09:00:04AM -0400, Peter Hurley wrote:
>>>>>>>>> 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.
>>>>>>>>
>>>>>>>> Hm yeah. Otoh to me that's bordering on "code too clever for my own good".
>>>>>>>> That the spinlock is held I can assure. That no one goes around and does
>>>>>>>> multiple vblank updates (because somehow that code raced with the hw
>>>>>>>> itself) I can't easily assure with a simple assert or something similar.
>>>>>>>> It's not the case right now, but that can changes.
>>>>>>>
>>>>>>> The algorithm would be broken if multiple updates for the same vblank
>>>>>>> count were allowed; that's why it checks to see if the vblank count has
>>>>>>> not advanced before storing a new timestamp.
>>>>>>>
>>>>>>> Otherwise, the read side would not be able to determine that the
>>>>>>> timestamp is valid by double-checking that the vblank count has not
>>>>>>> changed.
>>>>>>>
>>>>>>> And besides, even if the code looped without dropping the spinlock,
>>>>>>> the correct write order would still be observed because it would still
>>>>>>> be executing on the same cpu.
>>>>>>>
>>>>>>> My objection to the write memory barrier is not about optimization;
>>>>>>> it's about correct code.
>>>>>>
>>>>>> Well diff=0 is not allowed, I guess I could enforce this with some
>>>>>> WARN_ON. And I still think my point of non-local correctness is solid.
>>>>>> With the smp_wmb() removed the following still works correctly:
>>>>>>
>>>>>> spin_lock(vblank_time_lock);
>>>>>> store_vblank(dev, crtc, 1, ts1);
>>>>>> spin_unlock(vblank_time_lock);
>>>>>>
>>>>>> spin_lock(vblank_time_lock);
>>>>>> store_vblank(dev, crtc, 1, ts2);
>>>>>> spin_unlock(vblank_time_lock);
>>>>>>
>>>>>> But with the smp_wmb(); removed the following would be broken:
>>>>>>
>>>>>> spin_lock(vblank_time_lock);
>>>>>> store_vblank(dev, crtc, 1, ts1);
>>>>>> store_vblank(dev, crtc, 1, ts2);
>>>>>> spin_unlock(vblank_time_lock);
>>>>>>
>>>>>> because the compiler/cpu is free to reorder the store for vblank->count
>>>>>> _ahead_ of the store for the timestamp. And that would trick readers into
>>>>>> believing that they have a valid timestamp when they potentially raced.
>>>>>>
>>>>>> Now you're correct that right now there's no such thing going on, and it's
>>>>>> unlikely to happen (given the nature of vblank updates). But my point is
>>>>>> that if we optimize this then the correctness can't be proven locally
>>>>>> anymore by just looking at store_vblank, but instead you must audit all
>>>>>> the callers. And leaking locking/barriers like that is too fragile design
>>>>>> for my taste.
>>>>>>
>>>>>> But you insist that my approach is broken somehow and dropping the smp_wmb
>>>>>> is needed for correctness. I don't see how that's the case at all.
>>>>
>>>> Daniel,
>>>>
>>>> I've been really busy this last week; my apologies for not replying promptly.
>>>>
>>>>> Fwiw, i spent some time reeducating myself about memory barriers (thanks for your explanations) and thinking about this, and the last version of your patch looks good to me. It also makes sense to me to leave that last smb_wmb() in place to make future use of the helper robust - for non-local correctness, to avoid having to audit all future callers of that helper.
>>>>
>>>> My concern wrt to unnecessary barriers in this algorithm is that the trailing
>>>> barrier now appears mandatory, when in fact it is not.
>>>>
>>>> Moreover, this algorithm is, in general, fragile and not designed to handle
>>>> random or poorly-researched changes.
>>>
>>> Less fragility is exactly why I want that surplus barrier. But I've run
>>> out of new ideas for how to explain that ...
>>>
>>>> For example, if only the read and store operations are considered, it's obviously
>>>> unsafe, since a read may unwittingly retrieve an store in progress.
>>>>
>>>>
>>>> CPU 0                                   | CPU 1
>>>>                                         |
>>>>                              /* vblank->count == 0 */
>>>>                                         |
>>>> drm_vblank_count_and_time()             | store_vblank(.., inc = 2, ...)
>>>>                                         |
>>>>   cur_vblank <= LOAD vblank->count      |
>>>>                                         |   tslot = vblank->count + 2
>>>>                                         |   /* tslot == 2 */
>>>>                                         |   STORE vblanktime[0]
>>>
>>> This line here is wrong, it should be "STORE vblanktime[2]"
>>>
>>> The "STORE vblanktime[0]" happened way earlier, before 2 smp_wmb and the
>>> previous updating of vblank->count.
>>
>> &vblanktime[0] == &vblanktime[2]
>>
>> That's why I keep trying to explain you actually have to look at and
>> understand the algorithm before blindly assuming local behavior is
>> sufficient.
> 
> Ok now I think I got it, the issue is when the array (which is only 2
> elements big) wraps around. And that's racy because we don't touch the
> increment before _and_ after the write side update. But that seems like a
> bug that's always been there?

I'm not sure if those conditions can actually occur; it's been a long time
since I analyzed vblank timestamping.
 

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH] drm/vblank: Fixup and document timestamp update/read barriers
  2015-05-07 11:56                   ` [Intel-gfx] " Peter Hurley
@ 2015-05-07 17:33                     ` Mario Kleiner
  0 siblings, 0 replies; 31+ messages in thread
From: Mario Kleiner @ 2015-05-07 17:33 UTC (permalink / raw)
  To: Peter Hurley, Daniel Vetter
  Cc: Daniel Vetter, Intel Graphics Development, Michel Dänzer,
	DRI Development, Daniel Vetter

On 05/07/2015 01:56 PM, Peter Hurley wrote:
> On 05/06/2015 04:56 AM, Daniel Vetter wrote:
>> On Tue, May 05, 2015 at 11:57:42AM -0400, Peter Hurley wrote:
>>> On 05/05/2015 11:42 AM, Daniel Vetter wrote:
>>>> On Tue, May 05, 2015 at 10:36:24AM -0400, Peter Hurley wrote:
>>>>> On 05/04/2015 12:52 AM, Mario Kleiner wrote:
>>>>>> On 04/16/2015 03:03 PM, Daniel Vetter wrote:
>>>>>>> On Thu, Apr 16, 2015 at 08:30:55AM -0400, Peter Hurley wrote:
>>>>>>>> On 04/15/2015 01:31 PM, Daniel Vetter wrote:
>>>>>>>>> On Wed, Apr 15, 2015 at 09:00:04AM -0400, Peter Hurley wrote:
>>>>>>>>>> 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.
>>>>>>>>>
>>>>>>>>> Hm yeah. Otoh to me that's bordering on "code too clever for my own good".
>>>>>>>>> That the spinlock is held I can assure. That no one goes around and does
>>>>>>>>> multiple vblank updates (because somehow that code raced with the hw
>>>>>>>>> itself) I can't easily assure with a simple assert or something similar.
>>>>>>>>> It's not the case right now, but that can changes.
>>>>>>>>
>>>>>>>> The algorithm would be broken if multiple updates for the same vblank
>>>>>>>> count were allowed; that's why it checks to see if the vblank count has
>>>>>>>> not advanced before storing a new timestamp.
>>>>>>>>
>>>>>>>> Otherwise, the read side would not be able to determine that the
>>>>>>>> timestamp is valid by double-checking that the vblank count has not
>>>>>>>> changed.
>>>>>>>>
>>>>>>>> And besides, even if the code looped without dropping the spinlock,
>>>>>>>> the correct write order would still be observed because it would still
>>>>>>>> be executing on the same cpu.
>>>>>>>>
>>>>>>>> My objection to the write memory barrier is not about optimization;
>>>>>>>> it's about correct code.
>>>>>>>
>>>>>>> Well diff=0 is not allowed, I guess I could enforce this with some
>>>>>>> WARN_ON. And I still think my point of non-local correctness is solid.
>>>>>>> With the smp_wmb() removed the following still works correctly:
>>>>>>>
>>>>>>> spin_lock(vblank_time_lock);
>>>>>>> store_vblank(dev, crtc, 1, ts1);
>>>>>>> spin_unlock(vblank_time_lock);
>>>>>>>
>>>>>>> spin_lock(vblank_time_lock);
>>>>>>> store_vblank(dev, crtc, 1, ts2);
>>>>>>> spin_unlock(vblank_time_lock);
>>>>>>>
>>>>>>> But with the smp_wmb(); removed the following would be broken:
>>>>>>>
>>>>>>> spin_lock(vblank_time_lock);
>>>>>>> store_vblank(dev, crtc, 1, ts1);
>>>>>>> store_vblank(dev, crtc, 1, ts2);
>>>>>>> spin_unlock(vblank_time_lock);
>>>>>>>
>>>>>>> because the compiler/cpu is free to reorder the store for vblank->count
>>>>>>> _ahead_ of the store for the timestamp. And that would trick readers into
>>>>>>> believing that they have a valid timestamp when they potentially raced.
>>>>>>>
>>>>>>> Now you're correct that right now there's no such thing going on, and it's
>>>>>>> unlikely to happen (given the nature of vblank updates). But my point is
>>>>>>> that if we optimize this then the correctness can't be proven locally
>>>>>>> anymore by just looking at store_vblank, but instead you must audit all
>>>>>>> the callers. And leaking locking/barriers like that is too fragile design
>>>>>>> for my taste.
>>>>>>>
>>>>>>> But you insist that my approach is broken somehow and dropping the smp_wmb
>>>>>>> is needed for correctness. I don't see how that's the case at all.
>>>>>
>>>>> Daniel,
>>>>>
>>>>> I've been really busy this last week; my apologies for not replying promptly.
>>>>>
>>>>>> Fwiw, i spent some time reeducating myself about memory barriers (thanks for your explanations) and thinking about this, and the last version of your patch looks good to me. It also makes sense to me to leave that last smb_wmb() in place to make future use of the helper robust - for non-local correctness, to avoid having to audit all future callers of that helper.
>>>>>
>>>>> My concern wrt to unnecessary barriers in this algorithm is that the trailing
>>>>> barrier now appears mandatory, when in fact it is not.
>>>>>
>>>>> Moreover, this algorithm is, in general, fragile and not designed to handle
>>>>> random or poorly-researched changes.
>>>>
>>>> Less fragility is exactly why I want that surplus barrier. But I've run
>>>> out of new ideas for how to explain that ...
>>>>
>>>>> For example, if only the read and store operations are considered, it's obviously
>>>>> unsafe, since a read may unwittingly retrieve an store in progress.
>>>>>
>>>>>
>>>>> CPU 0                                   | CPU 1
>>>>>                                          |
>>>>>                               /* vblank->count == 0 */
>>>>>                                          |
>>>>> drm_vblank_count_and_time()             | store_vblank(.., inc = 2, ...)
>>>>>                                          |
>>>>>    cur_vblank <= LOAD vblank->count      |
>>>>>                                          |   tslot = vblank->count + 2
>>>>>                                          |   /* tslot == 2 */
>>>>>                                          |   STORE vblanktime[0]
>>>>
>>>> This line here is wrong, it should be "STORE vblanktime[2]"
>>>>
>>>> The "STORE vblanktime[0]" happened way earlier, before 2 smp_wmb and the
>>>> previous updating of vblank->count.
>>>
>>> &vblanktime[0] == &vblanktime[2]
>>>
>>> That's why I keep trying to explain you actually have to look at and
>>> understand the algorithm before blindly assuming local behavior is
>>> sufficient.
>>
>> Ok now I think I got it, the issue is when the array (which is only 2
>> elements big) wraps around. And that's racy because we don't touch the
>> increment before _and_ after the write side update. But that seems like a
>> bug that's always been there?
>
> I'm not sure if those conditions can actually occur; it's been a long time
> since I analyzed vblank timestamping.
>
>

They shouldn't occur under correct use. Normally one has to wrap any 
call to drm_vblank_count() or drm_vblank_count_and_time() into a pair of 
drm_vblank_get() -> query -> drm_vblank_put(). Only drm_vblank_get() 
will call drm_update_vblank() on a refcount 0 -> 1 transition if vblanks 
were previously off, and only that function bumps the count by more than 
+1. Iow. the overflow case isn't executed in parallel with queries -> 
problem avoided.

Proper _get()->query->put() sequence is given for queueing vblank 
events, waiting for target vblank counts or during pageflip completion.

The one exception would be in Chris recently proposed "lockless instant 
query" patch where a pure query is done - That patch that triggered 
Daniels cleanup patch. I was just about to ok that one, and testing with 
my timing tests and under normal use didn't show problems. There 
drm_vblank_count_and_time() is used outside a get/put protected path. 
I'm not sure if some race there could happen under realistic conditions.

-mario
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 31+ messages in thread

end of thread, other threads:[~2015-05-07 17:33 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-15  7:17 [PATCH] drm/vblank: Fixup and document timestamp update/read barriers Daniel Vetter
2015-04-15  8:17 ` Chris Wilson
2015-04-15  9:25   ` Daniel Vetter
2015-04-15  9:36     ` Chris Wilson
2015-04-15 10:32 ` Daniel Vetter
2015-04-15 17:34   ` Daniel Vetter
2015-04-15 17:49     ` Chris Wilson
2015-04-15 21:26     ` Mario Kleiner
2015-04-16  1:29       ` Peter Hurley
2015-04-16  6:39         ` Mario Kleiner
2015-04-16  9:00           ` Peter Hurley
2015-04-16  9:06             ` Daniel Vetter
2015-04-16 12:52           ` Peter Hurley
2015-04-16  8:54       ` Daniel Vetter
2015-04-16  0:17     ` shuang.he
2015-04-16  8:59     ` Daniel Vetter
2015-04-17  4:27       ` shuang.he
2015-04-16  0:18   ` shuang.he
2015-04-15 13:00 ` Peter Hurley
2015-04-15 17:31   ` Daniel Vetter
2015-04-16 12:30     ` Peter Hurley
2015-04-16 13:03       ` [Intel-gfx] " Daniel Vetter
2015-05-04  4:52         ` Mario Kleiner
2015-05-05 14:36           ` Peter Hurley
2015-05-05 15:42             ` [Intel-gfx] " Daniel Vetter
2015-05-05 15:57               ` Peter Hurley
2015-05-05 19:01                 ` Peter Hurley
2015-05-06  8:56                 ` Daniel Vetter
2015-05-07 11:56                   ` [Intel-gfx] " Peter Hurley
2015-05-07 17:33                     ` Mario Kleiner
2015-04-15 19:40 ` shuang.he

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.