[Intel-gfx] [PATCH] drm/vblank: Document and fix vblank count barrier semantics

[Daniel Vetter]

Noticed while reviewing code. I'm not sure whether this might or might
not explain some of the missed vblank hilarity we've been seeing. I
think those all go through the vblank completion event, which has
unconditional barriers - it always takes the spinlock. Therefore no
cc stable.

v2:
- Barrriers are hard, put them in in the right order (Chris).
- Improve the comments a bit.

v3:

Ville noticed that on 32bit we might be breaking up the load/stores,
now that the vblank counter has been switched over to be 64 bit. Fix
that up by switching to atomic64_t. This this happens so rarely in
practice I figured no need to cc: stable ...

Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
Cc: Keith Packard <keithp at keithp.com>
References: 570e86963a51 ("drm: Widen vblank count to 64-bits [v3]")
Cc: Rodrigo Siqueira <rodrigosiqueiramelo at gmail.com>
Cc: Chris Wilson <chris at chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
---
 drivers/gpu/drm/drm_vblank.c | 45 ++++++++++++++++++++++++++++++++----
 include/drm/drm_vblank.h     | 15 ++++++++++--
 2 files changed, 54 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 603ab105125d..03e37bceac9c 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -107,7 +107,7 @@ static void store_vblank(struct drm_device *dev, unsigned int pipe,
 
 	write_seqlock(&vblank->seqlock);
 	vblank->time = t_vblank;
-	vblank->count += vblank_count_inc;
+	atomic64_add(vblank_count_inc, &vblank->count);
 	write_sequnlock(&vblank->seqlock);
 }
 
@@ -273,7 +273,8 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
 
 	DRM_DEBUG_VBL("updating vblank count on crtc %u:"
 		      " current=%llu, diff=%u, hw=%u hw_last=%u\n",
-		      pipe, vblank->count, diff, cur_vblank, vblank->last);
+		      pipe, atomic64_read(&vblank->count), diff,
+		      cur_vblank, vblank->last);
 
 	if (diff == 0) {
 		WARN_ON_ONCE(cur_vblank != vblank->last);
@@ -295,11 +296,23 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
 static u64 drm_vblank_count(struct drm_device *dev, unsigned int pipe)
 {
 	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
+	u64 count;
 
 	if (WARN_ON(pipe >= dev->num_crtcs))
 		return 0;
 
-	return vblank->count;
+	count = atomic64_read(&vblank->count);
+
+	/*
+	 * This read barrier corresponds to the implicit write barrier of the
+	 * write seqlock in store_vblank(). Note that this is the only place
+	 * where we need an explicit barrier, since all other access goes
+	 * through drm_vblank_count_and_time(), which already has the required
+	 * read barrier curtesy of the read seqlock.
+	 */
+	smp_rmb();
+
+	return count;
 }
 
 /**
@@ -764,6 +777,14 @@ drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe,
  * vblank interrupt (since it only reports the software vblank counter), see
  * drm_crtc_accurate_vblank_count() for such use-cases.
  *
+ * Note that for a given vblank counter value drm_crtc_handle_vblank()
+ * and drm_crtc_vblank_count() or drm_crtc_vblank_count_and_time()
+ * provide a barrier: Any writes done before calling
+ * drm_crtc_handle_vblank() will be visible to callers of the later
+ * functions, iff the vblank count is the same or a later one.
+ *
+ * See also &drm_vblank_crtc.count.
+ *
  * Returns:
  * The software vblank counter.
  */
@@ -801,7 +822,7 @@ static u64 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe,
 
 	do {
 		seq = read_seqbegin(&vblank->seqlock);
-		vblank_count = vblank->count;
+		vblank_count = atomic64_read(&vblank->count);
 		*vblanktime = vblank->time;
 	} while (read_seqretry(&vblank->seqlock, seq));
 
@@ -818,6 +839,14 @@ static u64 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe,
  * vblank events since the system was booted, including lost events due to
  * modesetting activity. Returns corresponding system timestamp of the time
  * of the vblank interval that corresponds to the current vblank counter value.
+ *
+ * Note that for a given vblank counter value drm_crtc_handle_vblank()
+ * and drm_crtc_vblank_count() or drm_crtc_vblank_count_and_time()
+ * provide a barrier: Any writes done before calling
+ * drm_crtc_handle_vblank() will be visible to callers of the later
+ * functions, iff the vblank count is the same or a later one.
+ *
+ * See also &drm_vblank_crtc.count.
  */
 u64 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc,
 				   ktime_t *vblanktime)
@@ -1791,6 +1820,14 @@ EXPORT_SYMBOL(drm_handle_vblank);
  *
  * This is the native KMS version of drm_handle_vblank().
  *
+ * Note that for a given vblank counter value drm_crtc_handle_vblank()
+ * and drm_crtc_vblank_count() or drm_crtc_vblank_count_and_time()
+ * provide a barrier: Any writes done before calling
+ * drm_crtc_handle_vblank() will be visible to callers of the later
+ * functions, iff the vblank count is the same or a later one.
+ *
+ * See also &drm_vblank_crtc.count.
+ *
  * Returns:
  * True if the event was successfully handled, false on failure.
  */
diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
index 9fe4ba8bc622..c16c44052b3d 100644
--- a/include/drm/drm_vblank.h
+++ b/include/drm/drm_vblank.h
@@ -109,9 +109,20 @@ struct drm_vblank_crtc {
 	seqlock_t seqlock;
 
 	/**
-	 * @count: Current software vblank counter.
+	 * @count:
+	 *
+	 * Current software vblank counter.
+	 *
+	 * Note that for a given vblank counter value drm_crtc_handle_vblank()
+	 * and drm_crtc_vblank_count() or drm_crtc_vblank_count_and_time()
+	 * provide a barrier: Any writes done before calling
+	 * drm_crtc_handle_vblank() will be visible to callers of the later
+	 * functions, iff the vblank count is the same or a later one.
+	 *
+	 * IMPORTANT: This guarantee requires barriers, therefor never access
+	 * this field directly. Use drm_crtc_vblank_count() instead.
 	 */
-	u64 count;
+	atomic64_t count;
 	/**
 	 * @time: Vblank timestamp corresponding to @count.
 	 */
-- 
2.22.0